-
Notifications
You must be signed in to change notification settings - Fork 126
Syncrepl fails to parse SyncInfoMessage when the message is a syncIdSet #351
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Previously, the SyncInfoMessage parser used a for loop to test for the presence of each named choice with with the structure. However, because refreshDelete and refreshPresent both are able to be fully resolved as defaults, and due to how pyasn1 accesses named types, it was not checking the choice tag, and would return a fully populated refreshDelete to the caller instead. This fixes the parser to always return the inner component, and retrieves the name based on the current choice of the tagged item. Author: William Brown <william@blackhats.net.au>
Codecov Report
@@ Coverage Diff @@
## master #351 +/- ##
==========================================
+ Coverage 71.30% 71.34% +0.03%
==========================================
Files 50 50
Lines 4796 4795 -1
Branches 802 801 -1
==========================================
+ Hits 3420 3421 +1
+ Misses 1045 1043 -2
Partials 331 331
Continue to review full report at Codecov.
|
Thanks for the fix, William. Do you have a reproducer bug for the test suite? |
Added a test case for you. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the test case!
After two espressos and a starring contest with ASN.1, I agree that the fix is correct. I merely propose to test the other attributes of the sync info message more thoroughly.
Good suggestion! Thanks! Co-authored-by: Christian Heimes <christian@python.org>
Thanks mate, appreciate the suggestion! I was a bit focused on the logic not the mesagge decode, but more coverage helps and helps guarantee correctness. Thanks! |
@encukou This fix is a candidate for 3.3.0 release, too. |
Previously, the SyncInfoMessage parser used a for loop to test for the
presence of each named choice with with the structure. However, because
refreshDelete and refreshPresent both are able to be fully resolved
as defaults, and due to how pyasn1 accesses named types, it was not
checking the choice tag, and would return a fully populated refreshDelete
to the caller instead.
This fixes the parser to always return the inner component, and retrieves
the name based on the current choice of the tagged item.
Author: William Brown william@blackhats.net.au