Fix 0.5.0 API breakage of decoder.decode(substrateFun=...) #39

haxtibal · 2023-07-20T23:12:04Z

This fixes the API breakage regarding decoder.decode(substrateFun=...) in 0.5.0. See commit messages for details.

A substrateFun passed to decoder.decode() can now be either v0.4 Non-Streaming or v0.5 Streaming. pyasn1 will detect and handle both cases transparently.

A substrateFun passed to one of the new streaming decoders is still expected to be v0.5 Streaming only.

Non-Streaming means

take 3 args (uninitialized object, substrate, length)
return (value, remainder)

Streaming means

take 4 args (uninitialized object, substrate, length, options kwargs)
yield value
remainder is what's left in the stream

I've considered pysnmp 4.4.12 verdec.py and @ralphje s example as real world cases, both should work.

Fixes #28

droideck

Awesome work! Thank you!
Could you also, please, add more comments to the code? (mostly, it'll be fine to copy your explanations from the commits, I think)

droideck · 2023-07-28T03:45:08Z

pyasn1/codec/ber/decoder.py

+                substrate_bytes = substrate.read()
+                if length == -1:
+                    length = len(substrate_bytes)
+                value, nextSubstrate = nonStreamingSubstrateFun(asn1Object, substrate_bytes, length)
+                nbytes = substrate.write(nextSubstrate)
+                substrate.truncate()
+                substrate.seek(-nbytes, os.SEEK_CUR)


I'm worrying a bit about the lack of error handling here. I think we need to check for different possible issues as it's done in streaming.py

substrate will always be of type io.BytesIO when entering substrateFunWrapper. That's an invariant ensured by substrate = asSeekableStream(substrate) and should be treated as that (I've added an assertion). Do you see further unhandled error cases?

droideck · 2023-07-28T03:45:41Z

tests/codec/ber/test_decoder.py

@@ -138,13 +138,13 @@ def testIndefModeChunked(self):
    def testDefModeChunkedSubst(self):
        assert decoder.decode(
            ints2octs((35, 8, 3, 2, 0, 169, 3, 2, 1, 138)),
-            substrateFun=lambda a, b, c, d: streaming.readFromStream(b, c)
+            substrateFun=lambda a, b, c: (b, b[c:])


I think we should keep the old tests, so we can confirm that both approaches work.

That's not only about tests, it also means there will be API variants to maintain. But it's doable. I've added it as two optional commits on top, so you can decide if you want to pick it or not.

droideck · 2023-07-28T03:51:10Z

@tiran please, when you have time, take a look at the PR too. You have more experience with the project so you may find more issues that I possibly missed.

Also, I'll try my luck and cast summon @janpipek, who has done a lot of awesome work on the streaming pyasn1 feature.
Jan, if you have time, could you please take a look at the issue too?

haxtibal · 2023-07-29T22:33:09Z

Could you also, please, add more comments to the code?

Did that mostly in the form of docstrings, but have not yet checked if the resulting Sphinx docs make sense. Need to find some more time for it. Feel free to edit commits anyways.

droideck

Sorry for the late review; I have so many things on my plate lately...

BTW, I also run the build with your changes here:
freeipa-pr-ci2/freeipa#2968

Just to see how it behaves in a large IdM environment.

pyasn1/codec/ber/decoder.py

haxtibal · 2023-09-15T18:21:21Z

Sorry for the late review; I have so many things on my plate lately...

No need to explain. I'm aware maintaining such projects is demanding and have great respect for those who do it - so thank you :-)

BTW, I also run the build with your changes here: freeipa-pr-ci2/freeipa#2968

Just to see how it behaves in a large IdM environment.

Nice idea. Will changed functions actually be triggered there? freeipa itself doesn't seem to use pyasn1.codec.ber.decoder - only encoder.

droideck · 2023-09-15T19:13:29Z

Sorry for the late review; I have so many things on my plate lately...

No need to explain. I'm aware maintaining such projects is demanding and have great respect for those who do it - so thank you :-)

Thank you for the kind words! And for your contributions!

BTW, I also run the build with your changes here: freeipa-pr-ci2/freeipa#2968
Just to see how it behaves in a large IdM environment.

Nice idea. Will changed functions actually be triggered there? freeipa itself doesn't seem to use pyasn1.codec.ber.decoder - only encoder.

Yep, still FreeIPA includes more projects as dependencies - so it's possible that it may affect one of them at some point of execution.

I also plan to run pysnmp and python-ldap with this PR (different versions, used in Fedora, at least).

And after that, I think, we are good. :)

P.S. I also see that docs/readthedocs.org:pyasn1 is failing but I still haven't chance to look at that.

haxtibal · 2023-09-17T16:38:31Z

P.S. I also see that docs/readthedocs.org:pyasn1 is failing but I still haven't chance to look at that.

Where can I watch it failing? tox -e docs works locally for me.

droideck

Here are a couple more thoughts I got while rechecking the code before merging.

P.S. The docs issue should be resolved soon! (it's not related to your PR but I'd like to run your test with the change - just to be sure)

pyasn1/codec/ber/decoder.py

haxtibal · 2023-11-09T22:49:03Z

@droideck The commit series was structured for optional cherry picking. If you're going to take the whole set, please tell me, then I'd squash some of them for nicer history.

droideck · 2023-11-10T01:34:08Z

@droideck The commit series was structured for optional cherry picking. If you're going to take the whole set, please tell me, then I'd squash some of them for nicer history.

Yes, please, squash them the way you like (I'll be okay even with one combined commit).

Besides that, I think we are good!
I'll run a bunch of tests one more time over the weekend and I'll do the build next week first thing! Yay!

Thank you for all of the work you've done! It's very structured and nicely executed. :)

Partial BER decoding was possible in 0.4.8 and is no longer possible in 0.5.0. It's currently prevented by the strict bytesRead check in state stDecodeValue. We have to relax length check a bit. As a custom substrateFuns can be used for partial decoding, reading less than the definite length is fine and expected. Reading more is fishy. Treat the latter as error. If the length check breaks your existing code, please file a bug for pyasn1 and explain the use case for reading too much.

0.5.0 introduced streaming decoders with new API. decoder.decode() was meant as a compatibility layer, but API broke anyways if users passed a custom substrateFun to decoder.decode(). This happened because substrateFun API and semantics also changed with 0.5.0. To establish full backwards compatibility, we now let decode.decode() accept both v0.4 and v0.5 substrateFuns. v0.4 functions are detected and automatically wrapped to behave streaming-like. Detection of v0.4 style works by checking for TypeError stemming from a call with wrong number of arguments. The try-except approach was chosen over 'import inspect' for performance reasons. We avoid to interfere with user code TypeErrors by checking the traceback depth. Fixes pyasn1#28.

Use streaming semantics to avoid needless auto-conversion: - don't return value as 1st tuple item, but yield as scalar - don't return next substrate as 2nd tuple item; it's now what's left in the stream

haxtibal · 2023-11-10T13:54:56Z

Besides that, I think we are good! I'll run a bunch of tests one more time over the weekend and I'll do the build next week first thing! Yay!

Thank you for all of the work you've done! It's very structured and nicely executed. :)

Let's see... Will keep watching to help in case of regressions.

droideck

The results are good (there are some failures, but they seem to be not related to this change - freeipa-pr-ci2/freeipa#3139)
And I ran pysnmp, python-ldap and 389-ds-base tests with the new scratch-build.
All good!

Let's go ahead and merge it then if you are okay with that. :)

Thank you for all of the great work! This contribution is VERY appreciated!

haxtibal · 2023-11-14T23:28:54Z

there are some failures, but they seem to be not related to this change - freeipa-pr-ci2/freeipa#3139

Tried to look into runner.log and report.html and couldn't find immediate hints about root causes. But it's a large test system. I can't really reason about it. This is up to you...

Let's go ahead and merge it then if you are okay with that. :)

If you're confident then I'm too - let's go.

Thank you for all of the great work! This contribution is VERY appreciated!

Thank you :)

droideck requested changes Jul 28, 2023

View reviewed changes

haxtibal force-pushed the bugfix/0.4.8_compatibility branch from 69856d6 to 025e020 Compare July 29, 2023 22:05

haxtibal force-pushed the bugfix/0.4.8_compatibility branch from 025e020 to 81d6a72 Compare July 29, 2023 22:57

droideck force-pushed the bugfix/0.4.8_compatibility branch from 81d6a72 to bd73330 Compare August 22, 2023 05:19

jgyates mentioned this pull request Sep 6, 2023

SNMP jgyates/genmon#938

Closed

droideck requested changes Sep 15, 2023

View reviewed changes

pyasn1/codec/ber/decoder.py Outdated Show resolved Hide resolved

haxtibal force-pushed the bugfix/0.4.8_compatibility branch from bd73330 to 1e64537 Compare September 15, 2023 17:58

haxtibal force-pushed the bugfix/0.4.8_compatibility branch from 1e64537 to f62ab81 Compare September 17, 2023 16:29

droideck reviewed Oct 3, 2023

View reviewed changes

pyasn1/codec/ber/decoder.py Outdated Show resolved Hide resolved

pyasn1/codec/ber/decoder.py Outdated Show resolved Hide resolved

droideck requested changes Nov 7, 2023

View reviewed changes

pyasn1/codec/ber/decoder.py Outdated Show resolved Hide resolved

haxtibal force-pushed the bugfix/0.4.8_compatibility branch 2 times, most recently from 08fdf9c to e55c777 Compare November 9, 2023 22:40

Tobias Deiminger added 3 commits November 10, 2023 14:45

Make auto-provided substrateFun streaming

f91d5d6

Use streaming semantics to avoid needless auto-conversion: - don't return value as 1st tuple item, but yield as scalar - don't return next substrate as 2nd tuple item; it's now what's left in the stream

haxtibal force-pushed the bugfix/0.4.8_compatibility branch from e55c777 to f91d5d6 Compare November 10, 2023 13:50

droideck approved these changes Nov 14, 2023

View reviewed changes

droideck merged commit 2113545 into pyasn1:main Nov 15, 2023
14 of 15 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix 0.5.0 API breakage of decoder.decode(substrateFun=...) #39

Fix 0.5.0 API breakage of decoder.decode(substrateFun=...) #39

haxtibal commented Jul 20, 2023 •

edited

droideck left a comment

droideck Jul 28, 2023

haxtibal Jul 29, 2023 •

edited

droideck Jul 28, 2023 •

edited

haxtibal Jul 29, 2023

droideck commented Jul 28, 2023

haxtibal commented Jul 29, 2023

droideck left a comment

haxtibal commented Sep 15, 2023

droideck commented Sep 15, 2023

haxtibal commented Sep 17, 2023

droideck left a comment •

edited

haxtibal commented Nov 9, 2023

droideck commented Nov 10, 2023 •

edited

haxtibal commented Nov 10, 2023

droideck left a comment •

edited

haxtibal commented Nov 14, 2023

Fix 0.5.0 API breakage of decoder.decode(substrateFun=...) #39

Fix 0.5.0 API breakage of decoder.decode(substrateFun=...) #39

Conversation

haxtibal commented Jul 20, 2023 • edited

droideck left a comment

Choose a reason for hiding this comment

droideck Jul 28, 2023

Choose a reason for hiding this comment

haxtibal Jul 29, 2023 • edited

Choose a reason for hiding this comment

droideck Jul 28, 2023 • edited

Choose a reason for hiding this comment

haxtibal Jul 29, 2023

Choose a reason for hiding this comment

droideck commented Jul 28, 2023

haxtibal commented Jul 29, 2023

droideck left a comment

Choose a reason for hiding this comment

haxtibal commented Sep 15, 2023

droideck commented Sep 15, 2023

haxtibal commented Sep 17, 2023

droideck left a comment • edited

Choose a reason for hiding this comment

haxtibal commented Nov 9, 2023

droideck commented Nov 10, 2023 • edited

haxtibal commented Nov 10, 2023

droideck left a comment • edited

Choose a reason for hiding this comment

haxtibal commented Nov 14, 2023

haxtibal commented Jul 20, 2023 •

edited

haxtibal Jul 29, 2023 •

edited

droideck Jul 28, 2023 •

edited

droideck left a comment •

edited

droideck commented Nov 10, 2023 •

edited

droideck left a comment •

edited