Release 1.5.1 breaks backward compatibility

[m9ra]

Hi, the recent release of jsonpickle counts object ids differently, which breaks the backward compatibility.

A way to reproduce

Dump

import sys
import jsonpickle

print(f"Python {sys.version}")
print(f"jsonpickle=={jsonpickle.__version__}")

obj = object()
print(jsonpickle.dumps({'a': obj, 'b': obj}))

Output1

Python 3.7.8 (tags/v3.7.8:4b47a5b6ba, Jun 28 2020, 08:53:46) [MSC v.1916 64 bit (AMD64)]
jsonpickle==1.5.0
{"a": {"py/object": "builtins.object"}, "b": {"py/id": 0}}

Output2

Python 3.7.8 (tags/v3.7.8:4b47a5b6ba, Jun 28 2020, 08:53:46) [MSC v.1916 64 bit (AMD64)]
jsonpickle==1.5.1
{"a": {"py/object": "builtins.object"}, "b": {"py/id": 1}}

Load

Obviously, when we try to load the Output1 pickle with jsonpickle 1.5.1 the id mismatch causes incorrect reconstruction:

import sys
import jsonpickle

print(f"Python {sys.version}")
print(f"jsonpickle=={jsonpickle.__version__}")
print(jsonpickle.loads('{"a": {"py/object": "builtins.object"}, "b": {"py/id": 0}}'))

Output3 - correctly reconstructs the obj under both indexes

Python 3.7.8 (tags/v3.7.8:4b47a5b6ba, Jun 28 2020, 08:53:46) [MSC v.1916 64 bit (AMD64)]
jsonpickle==1.5.0
{'a': <object object at 0x000001CA9E2B4230>, 'b': <object object at 0x000001CA9E2B4230>}

Output4 - incorrectly reconstructs a dictionary on the index b

Python 3.7.8 (tags/v3.7.8:4b47a5b6ba, Jun 28 2020, 08:53:46) [MSC v.1916 64 bit (AMD64)]
jsonpickle==1.5.1
{'a': <object object at 0x000001991ED37230>, 'b': {...}}

[m9ra]

My guess from quickly looking at the code is, that the issue comes from this a5725d8

[Theelx]

@davvid Important problem with backwards compatibility being broken on a patch release

[davvid]

Hmm I guess it's a question of whether the on-the-wire format is considered part of the semver API or not.

What I could do is revert that change and deploy another patch release so that the wire format does not change.

Then we'll bump up a major version and re-apply the change. What do you all think?

[Theelx]

I think it's part of the semver API (it affects user-facing function output), so I think you'd need to release 1.5.2 with this fixed and then do 2.0.0 with this change, yeah

https://semver.org/#what-do-i-do-if-i-accidentally-release-a-backwards-incompatible-change-as-a-minor-version

(also can you comment on #350 please, it's about adding pytest benchmarks to the test suite)

[m9ra]

Just a comment from a person without any deep knowledge of your library: Why not fix the issue by having the dict's occupying the last py/id numbers. That would keep the feature working while not breaking the previous pickles.

[Theelx]

Just a comment from a person without any deep knowledge of your library: Why not fix the issue by having the dict's occupying the last py/id numbers. That would keep the feature working while not breaking the previous pickles.

The fix, as you said, should be relatively straightforward, so I think the issue is more so the versioning than the fix. A backwards-incompatible patch release happened, which should be avoided at all costs.

[Theelx]

@davvid Sorry if I'm rushing, but I think it's important to fix this and properly version it ASAP so a new working version can be released.

[davvid]

Sorry for missing that. v1.5.2 has been deployed to pypi. There is also a new major release, v2.0.0, that brings back the dict identity preservation.