jsonpickle 2.0.0 fails to decode pickled files created by jsonpickle 1.4.1

[Verequus]

I've been attempting to upgrade the jsonpickle package in our project, where we store a number of files in pickled format. It happens that in some UTs for that area their evaluation failed, when I only change the jsonpickle package to 2.0.0. For 1.5.2 it still works. I also locally tested with the real data, in case it would be case of overly specific UTs. But those ended up with corrupted JSON data. As one can see in the screenshot, instead of

"previousFeatureTeam": {
        "py/id": 1
      },

on the left side created with 1.5.2, the version created by jsonpickle suddenly creates a list and effectively doubles the output by reusing existing data (but even that seems corrupted).

I suspect that the format changed or that maybe 2.0.0 introduced a regression, but I couldn't find any information on what actually changed for 2.0.0. Do I need to do something special for conversions from older data?

[Theelx]

Sorry for the late response, I forgot that I didn't have this repo watched, so I didn't get notifications:
#351 was where we noticed a backwards-compatibility issue with 1.5.1, so we released 1.5.2 to try and fix that. 2.0.0 is the release where we went ahead with the changes, because they fixed a bug (I don't remember the specific issue with the bug, but it'd be in CHANGES.rst).

Essentially, according to SemVer 2.0, which this project follows, incrementing the major version number (in this case from 1 to 2) signals a backwards-incompatible change. So yeah, 2.0.0's format changes a bit. If you still need help migrating (again, I'm very sorry for the late response), I can see if I can write a small migration script.

[Verequus]

Yes, having such script would be very helpful. Not just for my project, but for anyone who would want to keep the old data available. In particular, since it is not possible to have both jsonpickle modules loaded at once (which I considered as possible migration path).

[Theelx]

Small update: I believe it's possible to fix this by adding a compatibility mode to the Unpickler.restore_dict method that controls whether a function is called or not. Specifically, this line:

jsonpickle/jsonpickle/unpickler.py

Line 545 in a15517e

self._mkref(data)

Adding an option to toggle it on or off would allow the unpickler to switch between v2 behavior or v1 behavior for unpickling, and then the data could be re-encoded in the v2 format.

However, I want to preemptively state that I don't think it's a good idea to modify the pickler to allow encoding in the v1 format because enabling this current behavior fixed #255. Thoughts @davvid?

[Verequus]

My 2 cents, I agree with not allowing v1 encoding. Being able to decode v1 data like that sounds like a great option though.

[davvid]

This sounds like it could be a useful option. We don't currently provide any tools, but a python -m jsonpickle.tool.convert_to_v2 <filename> could be a convenient command-line interface to add as well, but that'd be a cherry on top for sure.

[Theelx]

This sounds like it could be a useful option. We don't currently provide any tools, but a python -m jsonpickle.tool.convert_to_v2 <filename> could be a convenient command-line interface to add as well, but that'd be a cherry on top for sure.

Regarding a CLI tool, I think that that's possible, but it'd be a bit clumsy when the user could just call an API function like py jsonpickle.decode_from_v1(pickled_string, *flags), use it, and re-encode it in the default v2 format like normal when they're done using it. Plus, a CLI tool would make it hard to pass classes and context arguments when decoding (since the command line doesn't have access to arbitrary Python objects), and other arguments might be affected too. I'd be interested to here a out-in-the-wild user's opinion though, what do you think @Verequus ?

Sorry if this turns out with poor formatting, I'm on my phone.

[Verequus]

Not that familiar with the internals, but if access to Python classes is needed, could some path given to import everything?

[davvid]

Super valid points @Theelx a CLI tool is not a good idea in hindsight. We definitely don't need a CLI interface, and those points highlight that adding one incurs an entire category of maintenance burden and issues that are best avoided altogether.

decode_from_v1 sounds like a sensible approach to me since it should then be straightforward for users to write their own site-specific tool for doing conversions if they need it.

For simple cases this conversion tool might look like jsonpickle.encode(jsonpickle.decode_from_v1(json_string)) but that doesn't handle the general case. The general case is that the encode(...) arguments must match what the project is doing internally when they call encode(...), which we can't know in advance, so the CLI idea is a non-starter. Thanks for the reality check, and sorry for the distraction from the main topic.

[Theelx]

Thanks for the reality check, and sorry for the distraction from the main topic.

Ah your CLI idea was absolutely on topic, since we want to make it as easy as possible for users to migrate, and a CLI could work quite well for some users if we didn't have so many options to deal with!

Anyway, I'm not sure whether the solution will be to make a jsonpickle.decode_from_v1(string_to_decode, *decoding_flags) function or to add a from_v1=boolean argument to jsonpickle.decode. I'm leaning towards the former because it'll be more explicit about what the decoder is doing, easier to find+replace, and also not clutter the decode function with more arguments, but the latter will reduce code duplication, and also kinda fits with what seems like the intended ethos of jsonpickle: add a flag for everything.

Edit: Both approaches would end up setting a class variable named something like v1_decoding on Unpickler, which the _restore_dict function would check when deciding whether to call _mkref. What I'm wondering is whether to present this option to the user as its own function or as an argument to decode.

[Theelx]

@Verequus Can you send the specific data + code that produced that errant stream (or a minimal reproducible example)? I've been trying to reproduce the issue by comparing both versions so that I can have data to test the fix on, but I can't seem to use any combination of dictionaries and class objects to achieve that.

It seems to need "py/id" to show up as a tag, and that only happens when there's a cyclical reference (detected by _mkref returning False when encoding), but I have never created a cyclical reference in the context of jsonpickle before, so I'm actually stumped as to how to do that.

[Theelx]

Based on the blacking out of data in the screenshot you sent, I assume that you have some private data that this is running on. In that case, can you git clone jsonpickle, apply this diff on jsonpickler/unpickler.py, pip install your local version (pip install . in the cloned directory), run it on the data, and let us know if the issue is fixed?
The line numbers may be off on this diff, I can't remember if I have any uncommitted changes in this directory from a long time ago.

diff --git a/jsonpickle/unpickler.py b/jsonpickle/unpickler.py
index 45eaed7..7c08be3 100644
--- a/jsonpickle/unpickler.py
+++ b/jsonpickle/unpickler.py
@@ -17,7 +17,8 @@ from .backend import json
 
 
 def decode(
-    string, backend=None, context=None, keys=False, reset=True, safe=False, classes=None
+    string, backend=None, context=None, keys=False, reset=True, safe=False,
+    classes=None, v1_decode=False
 ):
     """Convert a JSON string into a Python object.
 
@@ -45,7 +46,7 @@ def decode(
     36
     """
     backend = backend or json
-    context = context or Unpickler(keys=keys, backend=backend, safe=safe)
+    context = context or Unpickler(keys=keys, backend=backend, safe=safe, v1_decode=v1_decode)
     data = backend.decode(string)
     return context.restore(data, reset=reset, classes=classes)
 
@@ -121,10 +122,11 @@ def _obj_setvalue(obj, idx, proxy):
 
 
 class Unpickler(object):
-    def __init__(self, backend=None, keys=False, safe=False):
+    def __init__(self, backend=None, keys=False, safe=False, v1_decode=False):
         self.backend = backend or json
         self.keys = keys
         self.safe = safe
+        self.v1_decode = v1_decode
 
         self.reset()
 
@@ -540,9 +542,11 @@ class Unpickler(object):
     def _restore_set(self, obj):
         return {self._restore(v) for v in obj[tags.SET]}
 
-    def _restore_dict(self, obj):
+    def _restore_dict(self, obj, v1_decode=False):
         data = {}
-        self._mkref(data)
+        # We can remove v1_decode when it's time for jsonpickle 3
+        if not v1_decode:
+            self._mkref(data)

[Verequus]

Since I'm on vacation, I can't do it myself until middle January. But I'll tell my colleagues how to check this and hopefully we get faster feedback.

[Theelx]

Ok, thanks! I should add for completeness' sake, that applying that diff and pip installing alone won't be enough, your colleagues will also need to pass v1_decode=True to the decode function.

[Verequus]

I've got feedback. It seems that there is a small bug with your patch:

•    if not v1_decode:
  

should be

•    if not self.v1_decode:
  

Otherwise, the failing UT works again as expected. If you could publish a 2.0.1 with that fix, I'd appreciate it.

[Theelx]

Ohhh, yes thanks! I don't have permissions to publish to PyPi, but I'll commit the fix and hopefully @davvid will have time to publish it.

Edit: I'm going to fix it with a new flag to decode, to minimize code duplication.

Edit 2: I'm also not including the unneeded v1_decode parameter for _restore_dict that I included in the diff, in the final PR, as it's unused. I'm also going to make a pull request instead of committing directly so that tests can run.