Changed signature of method #28
Comments
|
I am also seeing this issue using pyasn1-0.5.0 and pysnmp-4.4.12 when I use getcmd(). https://pysnmp.readthedocs.io/en/latest/docs/hlapi/v3arch/asyncore/sync/manager/cmdgen/getcmd.html If I revert to pyasn1==0.4.8 then the issue is resolved. Full error dump is: |
|
Changes in commit 93e11a2 leak into client code via But:
|
|
@haxtibal You can find primary forks in etingof/pysnmp#429 BTW, your patch for pysnmp doesn't works for older pyasn1 releases, so probably not likely to be accepted by the forks without modification. But it is a great start. |
|
The issue (for me at least) seems to be that https://github.com/pyasn1/pyasn1/blob/main/pyasn1/codec/ber/decoder.py#L1765 generates a Although If I replace the affected line with the suggested |
|
I don't have enough capacity now to work on pysnmp's backwards compatibility change (and, as far as I know, we don't have the option to update pysnmp now, as the maintainer has passed away). But PRs are welcome. :) @ralphje if this PR #30 doesn't work for you, could you please post your test case and output in the PR's discussion? |
|
Of course. The following is an example where recursion changes how the result is handled, even with #30 applied. The string This is the result: |
Just took a closer look at how substrateFun semantics differ between 0.4.8 and 0.5.0. 0.4.8
0.5.0
This makes it harder to find a substrateFun for #30 that retains full compatibilty with 0.4.8. A naive implementation could be something like: The diff makes 0.5.0 behave like 0.4.8 in @ralphje s example. Don't know, but I doubt that's how the new API was meant to be used. |
* Update requirements from branch 'master'
to 3442fd6ffe1517716743205973e2c7b84777acea
- Revert upgrade of pyasn1
Version 0.5.0 of pyasn1 breaks compatiblity with pysnmp (which is
expected) and pysnmplib (which is definitely not expected).
For more information please look at:
- pyasn1/pyasn1#28
- ralphje/signify#37
Change-Id: Ibd7a04c79aa710f98b1c781551464036ced4817f
Version 0.5.0 of pyasn1 breaks compatiblity with pysnmp (which is expected) and pysnmplib (which is definitely not expected). For more information please look at: - pyasn1/pyasn1#28 - ralphje/signify#37 Change-Id: Ibd7a04c79aa710f98b1c781551464036ced4817f
|
According to this commit by Illya in 2020, the breaking change in pyasn1 seems to be expected and he intentionally forced pysnmp 4.4.13+ to avoid pyasn1 0.5+. Sadly pysnmp 4.4.13 was never released. |
|
From my understanding, this change seems to have more impact than expected during release, though given the situation around @etingof makes this entirely understandable. If the change is intentional, the issue of backwards incompatibility in substrateFun should at least be addressed in the release notes with sufficient context to understand and adjust code accordingly to the change. Additionally, as I have shown previously, the recursiveFlag changes behaviour significantly, which should also be addressed, apart from the bug that was fixed in #30. As for now, to me it is unclear what the intended solution will be from the current pyasn1 package maintainers, and it is unclear whether future versions of pyasn1 will have the exact same behaviour as 0.5.0+#30 or that the backwards incompatibility issue may be partially remedied in a future patch. This currently hinders development in packages that depend on pyasn1, and impedes progress in upgrading to 0.5.0+. At the very least, test cases should cover all of this to ensure future API stability. |
- Fix version due to backward incompatibility issues (pyasn1/pyasn1#28)
- Fix version due to backward incompatibility issues (pyasn1/pyasn1#28)
pyasn1/pyasn1#28 On 20230420: https://pypi.org/project/pyasn1/#history (cherry picked from commit 7d6cbd0)
This solution worked for me. |
|
I'm getting the same issue... pinned the version to |
Could you elaborate? |
|
@okainov If you read my comment above, you will see the decision was made by the original developer and it cannot be wrong (at least not this moment). New packages depending on pyasn1 should upgrade and adapt to the changes in 0.5.x, and for pysnmp users they need to pick up a reliable fork. |
While I have a lot of respect for Ilya, I'm not sure we can say the decision is necessarily right based solely on the fact that it came from him. As your previous comment points out, the decision was made over 3 years ago. It may be a good time to revisit that decision. Most projects follow semantic versioning. Based on that, I think it is right to consider a breaking change between minor versions to be a bug. |
As pointed out in 93e11a2, API compatibility was considered by wrapping
This works, except for the corner case of |
Unfortunately Ilya passed away, so we are unable to figure out if the API change was deliberate or a mistake. I can only speculate that he either hadn't noticed the API breakage or he had plans to release new versions of pyasn1 and pysnmp stack at the same time. I agree with you that we should revisit breaking change and attempt to reconcile the API.
tl;dr Let's try to address the API breakage, PRs welcome! |
0.5.0 introduced streaming decoders with new API and decoder.decode was intended to stay as a compatibility layer. This mostly worked, but it broke when a substrateFun was used, because substrateFuns are also expected to be streaming style since 0.5.0. If callers passed a non-streaming variant, it broke, because of different signature and semantics. Now we add a wrapper to 'class Decoder' that converts non-streaming substrateFuns to streaming ones as part of the compatibiltiy layer. Fixes pyasn1#28.
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 let decode.decode() receive a 0.4.8 style non-streaming substrateFun, and transparently convert it to a streaming substrateFun as expected by the innner streaming decoders. Fixes pyasn1#28.
testPartialDecodeWithCustomSubstrateFun is inspired by pysnmp 4.4.12, pysnmp.proto.api.verdec.decodeMessageVersion. testPartialDecodeWithDefaultSubstrateFun is inspired by pyasn1#28 issuecomment-1518753039. Both cases broke after pyasn1 0.5.0 was released and now work again.
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 let decode.decode() receive a v0.4 style non-streaming substrateFun, and transparently convert it to a streaming substrateFun as expected by the innner streaming decoders. Fixes pyasn1#28.
testPartialDecodeWithCustomSubstrateFun is inspired by pysnmp 4.4.12, pysnmp.proto.api.verdec.decodeMessageVersion. testPartialDecodeWithDefaultSubstrateFun is inspired by pyasn1#28 issuecomment-1518753039. Both cases broke after pyasn1 0.5.0 was released and now work again.
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 let decode.decode() receive a v0.4 style non-streaming substrateFun, and transparently convert it to a streaming substrateFun as expected by the innner streaming decoders. Fixes pyasn1#28.
testPartialDecodeWithCustomSubstrateFun is inspired by pysnmp 4.4.12, pysnmp.proto.api.verdec.decodeMessageVersion. testPartialDecodeWithDefaultSubstrateFun is inspired by pyasn1#28 issuecomment-1518753039. Both cases broke after pyasn1 0.5.0 was released and now work again.
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 let decode.decode() receive a v0.4 style non-streaming substrateFun, and transparently convert it to a streaming substrateFun as expected by the innner streaming decoders. Fixes pyasn1#28.
testPartialDecodeWithCustomSubstrateFun is inspired by pysnmp 4.4.12, pysnmp.proto.api.verdec.decodeMessageVersion. testPartialDecodeWithDefaultSubstrateFun is inspired by pyasn1#28 issuecomment-1518753039. Both cases broke after pyasn1 0.5.0 was released and now work again.
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 let decode.decode() receive a v0.4 style non-streaming substrateFun, and transparently convert it to a streaming substrateFun as expected by the innner streaming decoders. Fixes pyasn1#28.
testPartialDecodeWithCustomSubstrateFun is inspired by pysnmp 4.4.12, pysnmp.proto.api.verdec.decodeMessageVersion. testPartialDecodeWithDefaultSubstrateFun is inspired by pyasn1#28 issuecomment-1518753039. Both cases broke after pyasn1 0.5.0 was released and now work again.
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 let decode.decode() receive a v0.4 style non-streaming substrateFun, and transparently convert it to a streaming substrateFun as expected by the innner streaming decoders. Fixes pyasn1#28.
testPartialDecodeWithCustomSubstrateFun is inspired by pysnmp 4.4.12, pysnmp.proto.api.verdec.decodeMessageVersion. testPartialDecodeWithDefaultSubstrateFun is inspired by pyasn1#28 issuecomment-1518753039. Both cases broke after pyasn1 0.5.0 was released and now work again.
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 let decode.decode() receive a v0.4 style non-streaming substrateFun, and transparently convert it to a streaming substrateFun as expected by the innner streaming decoders. Fixes pyasn1#28.
testPartialDecodeWithCustomSubstrateFun is inspired by pysnmp 4.4.12, pysnmp.proto.api.verdec.decodeMessageVersion. testPartialDecodeWithDefaultSubstrateFun is inspired by pyasn1#28 issuecomment-1518753039. Both cases broke after pyasn1 0.5.0 was released and now work again.
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.
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 #28.
pyasn1/pyasn1#28 On 20230420: https://pypi.org/project/pyasn1/#history (cherry picked from commit 7d6cbd0)
pyasn1/pyasn1#28 On 20230420: https://pypi.org/project/pyasn1/#history (cherry picked from commit 7d6cbd0)
pyasn1/pyasn1#28 On 20230420: https://pypi.org/project/pyasn1/#history (cherry picked from commit 7d6cbd0)
pyasn1/pyasn1#28 On 20230420: https://pypi.org/project/pyasn1/#history (cherry picked from commit 7d6cbd0)
pyasn1/pyasn1#28 On 20230420: https://pypi.org/project/pyasn1/#history (cherry picked from commit 7d6cbd0)
pyasn1/pyasn1#28 On 20230420: https://pypi.org/project/pyasn1/#history (cherry picked from commit 7d6cbd0)
pyasn1/pyasn1#28 On 20230420: https://pypi.org/project/pyasn1/#history (cherry picked from commit 7d6cbd0)
The text was updated successfully, but these errors were encountered: