Opened 7 years ago

Closed 7 years ago

#28534 closed Bug (fixed)

Changing JSONField on inline in admin doesn't always trigger change

Reported by: john-parton Owned by: shangdahao
Component: Forms Version: 1.11
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

If you have an inline with a JSONField and an initial value of {'key': 1} and you try to change it to {'key': True} without any other changes, the admin won't save your change. I believe this is because JSONField.has_changed() just uses simple equality checking and {'key': True} == {'key': 1} with a basic equality test in python.

Here's the minimal test case:

>>> from django.contrib.postgres.forms import JSONField
>>> JSONField().has_changed({'a': True}, '{"a": 1}')
False

Let me know if there's any other information you need.

Thanks.

Change History (10)

comment:1 by john-parton, 7 years ago

Type: UncategorizedBug

comment:2 by Claude Paroz, 7 years ago

Triage Stage: UnreviewedAccepted

comment:3 by shangdahao, 7 years ago

Owner: changed from nobody to shangdahao
Status: newassigned

comment:4 by john-parton, 7 years ago

Here's some information that might be helpful: https://stackoverflow.com/questions/45888263/strict-comparison-of-dictionaries-in-python

It looks like there's two approaches:

  1. Convert both the old value and new value to some string representation and compare them. (Either using json.dumps or pprint.pformat)
  2. Recursively compare the values

I think (1) with json.dumps would probably work best. pprint.pformat was suggested to handle objects that can't be json-serialized, but obviously everything that can go into a JSONField can be json-serialized.

comment:5 by shangdahao, 7 years ago

Has patch: set

PR

Hi, @john-partonm, for (1), json.dumps works well, but I think str() is a good choice too, so I use it.

in reply to:  5 ; comment:6 by john-parton, 7 years ago

Replying to hui shang:

PR

Hi, @john-partonm, for (1), json.dumps works well, but I think str() is a good choice too, so I use it.

Does str() always sort dictionary keys? I'm not totally sure, but under certain circumstances I think your fix might flag {'a': 1, 'b': 2} as being different from {'b': 2, 'a': 1}.

Thanks for the quick response and your hard work!

EDIT:

At least with Python3.6, the string representation of two dicts can differ:

  Python 3.6.1 (default, Dec 2015, 13:05:11)
  [GCC 4.8.2] on linux
   left = {}
   left['a'] = 1
   left['b'] = 2
   right = {}
   right['b'] = 2
   right['a'] = 1
str(left) == str(right)
=> False   
Last edited 7 years ago by john-parton (previous) (diff)

comment:7 by shangdahao, 7 years ago

Patch needs improvement: set

in reply to:  6 comment:8 by shangdahao, 7 years ago

Replying to john-parton:

Replying to hui shang:

PR

Hi, @john-partonm, for (1), json.dumps works well, but I think str() is a good choice too, so I use it.

Does str() always sort dictionary keys? I'm not totally sure, but under certain circumstances I think your fix might flag {'a': 1, 'b': 2} as being different from {'b': 2, 'a': 1}.

Thanks for the quick response and your hard work!

EDIT:

At least with Python3.6, the string representation of two dicts can differ:

  Python 3.6.1 (default, Dec 2015, 13:05:11)
  [GCC 4.8.2] on linux
   left = {}
   left['a'] = 1
   left['b'] = 2
   right = {}
   right['b'] = 2
   right['a'] = 1
str(left) == str(right)
=> False   

Yes, you are right, thank you for the explanation. I have changed the comparison from str() to json.dumps

comment:9 by shangdahao, 7 years ago

Patch needs improvement: unset

comment:10 by Tim Graham <timograham@…>, 7 years ago

Resolution: fixed
Status: assignedclosed

In 1907fc9b:

Fixed #28534 -- Made JSONField.has_changed() ignore key order and consider True/1 values as different.

Note: See TracTickets for help on using tickets.
Back to Top