Skip to content
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

Fixing #30583 -- XML serializer doesn't handle JSONFields #11538

Closed
wants to merge 1 commit into from

Conversation

chason
Copy link
Contributor

@chason chason commented Jul 4, 2019

When using the XML serializer on a JSON field, it was previously trying
to serialize the raw python data object that the JSONField outputs. This
change causes the XML serializer to use json.dumps and json.loads for
serializing and deserializing JSONField data.

When using the XML serializer on a JSON field, it was previously trying
to serialize the raw python data object that the JSONField outputs. This
change causes the XML serializer to use json.dumps and json.loads for
serializing and deserializing JSONField data.
@chason
Copy link
Contributor Author

chason commented Jul 4, 2019

I'm not 100% happy with this solution, as it creates special case logic in the XML serializer for JSONFields. However, any other solution I came up with ran into problems with how the JSON serializer (improperly?) works: instead of serializing the JSON data into a string, it includes the JSON data into the subsequent JSON data structure. This means that using the existing methods on the JSONField (i.e. value_to_string which doesn't actually output a string) and using them to serialize/deserialize the data would cause backwards compatibility problems.

I'm open to suggestions for how to improve this if the current solution is deemed sub-optimal.

Copy link
Member

@felixxm felixxm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chason Thanks for this patch 👍 I left some comments.

with self.subTest(value=value):
instance = JSONModel(field=value)
data = serializers.serialize('xml', [instance], fields=["field"])
self.assertIn(test_xml.format(serialized), data)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use self.assertXMLEqual, e.g.

def test_xml_serialization(self):
    test_xml_data = (
        '<django-objects version="1.0">'
        '<object model="postgres_tests.jsonmodel">'
        '<field name="field" type="JSONField">%s'
        '</field></object></django-objects>'
    )
    for value, serialized in self.test_values:
        with self.subTest(value=value):
            ...
            self.assertXMLEqual(data, test_xml_data % serialized)
            ...

# outputs string.
if field_node.getAttribute("type") == "JSONField":
value = json.loads(value)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that this solution is quite clunky 😞, we could change to_python() and value_to_string() for JSONField, e.g.

def to_python(self, value):
    if isinstance(value, str):
      value = json.loads(value)
    return value

def value_to_string(self, obj):
    return json.dumps(self.value_from_object(obj))

but it will break JSON serialization (see TestSerialization.test_dumping and TestSerialization.test_loading).

@felixxm
Copy link
Member

felixxm commented Mar 23, 2020

Updated in #12613.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants