Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#32237 closed New feature (duplicate)

JSONField isnull and =None

Reported by: Gordon Wrigley Owned by: nobody
Component: Database layer (models, ORM) Version: 3.1
Severity: Normal Keywords:
Cc: Adam Johnson Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

JSONFields are as far as I know the only place in the system where Q(field__subfield__isnull=True) and Q(field__subfield=None) are not synonyms.

This also creates a situation where filter and exclude are not complements, which as far as I know is also unique to JSONFields.

And finally it creates a situation where MyModel.objects.filter(field__subfield__isnull=False).values(field__subfield) may include None.

All three of these violate things I understood to be invariants of the ORM. Additionally it is not obvious at the call site that non standard logic is in effect. You have to remember that field is a JSONField and that JSONFields treat None / null differently from other fields.

I think I understand the logic of this and how to some extent it falls naturally out of the implementation, but having worked with it a bunch now I think it creates a dangerous trap for the unwary and unexperienced.

I think isnull and =None on JSONFields should be changed so both match either an actual JSON null value or a missing field.

And either the has_key lookup should be extended to work at all levels so it can be used to distinguish between these cases with filter(field__has_key="a__b").

Or a new lookup should be added, (for the sake of discussion key_exists) that does what the current isnull lookup is doing so you can distinguish with filter(field__a__b__key_exists=True).

For the sake of discussion I've enumerated the current behaviour below.

With regard to this code [o.field for o in MyModel.objects.all()] we get:

0 all()                                           [{}, {'subfield': None}, {'subfield': 123}]

11 filter(field__subfield=None)                   [{'subfield': None}]
12 filter(field__subfield__isnull=True)           [{}]
13 filter(field__subfield__isnull=False)          [{'subfield': None}, {'subfield': 123}]
14 exclude(field__subfield=None)                  [{'subfield': 123}]
15 exclude(field__subfield__isnull=True)          [{'subfield': None}, {'subfield': 123}] 
16 exclude(field__subfield__isnull=False)         [{}]

21 filter(Q(field__subfield=None))                [{'subfield': None}]
22 filter(Q(field__subfield__isnull=True))        [{}]
23 filter(Q(field__subfield__isnull=False))       [{'subfield': None}, {'subfield': 123}]
24 exclude(Q(field__subfield=None))               [{'subfield': 123}]
25 exclude(Q(field__subfield__isnull=True))       [{'subfield': None}, {'subfield': 123}] 
26 exclude(Q(field__subfield__isnull=False))      [{}]

31 filter(~Q(field__subfield=None))               [{'subfield': 123}]
32 filter(~Q(field__subfield__isnull=True))       [{'subfield': None}, {'subfield': 123}]
33 filter(~Q(field__subfield__isnull=False))      [{}]
34 exclude(~Q(field__subfield=None))              [{'subfield': None}]
35 exclude(~Q(field__subfield__isnull=True))      [{}] 
36 exclude(~Q(field__subfield__isnull=False))     [{'subfield': None}, {'subfield': 123}]

Things to note:
The 2* lines behave the same as the 1* lines and can be ignored hence forth.
The *1, *4 pairs are not complements, by which I mean they do not between them return all rows.
The *1, *2 pairs and the *4, *5 pairs are not synonyms.

Change History (8)

comment:1 by Adam Johnson, 4 years ago

Cc: Adam Johnson added
Component: UncategorizedDatabase layer (models, ORM)

comment:2 by Mariusz Felisiak, 4 years ago

Resolution: duplicate
Status: newclosed

Thanks for this ticket, however it was already discussed and it's a documented behavior, see Storing and querying for None and Key, index, and path transforms docs.

And either the has_key lookup should be extended to work at all levels so it can be used to distinguish between these cases with filter(field__has_key="a__b").

__has_key works on all levels, you can use filter(field__a__has_key='b').

Duplicate of #31324 and #31894.

comment:3 by Mariusz Felisiak, 4 years ago

Type: UncategorizedNew feature

comment:4 by Gordon Wrigley, 4 years ago

has_key works on all levels, you can use filter(fieldahas_key='b').

That's interesting, I will need to go test that, it's not what the docs say.

"Returns objects where the given key is in the top-level of the data."

https://docs.djangoproject.com/en/3.1/topics/db/queries/#has-key

in reply to:  4 comment:5 by Mariusz Felisiak, 4 years ago

When a key transform returns a JSONField you can use any of these lookups or transforms, see tests.

comment:6 by Carlton Gibson, 4 years ago

Duplicate of #31324 and #31894.

Yes, exactly. (I was just looking up those tickets. 🙂)
c.f. also #25718 for a related ticket from before the new field.

comment:7 by Gordon Wrigley, 4 years ago

Thanks for this ticket, however it was already discussed and it's a documented behavior

I appreciate that it's documented behaviour, what I'm saying is the documented behaviour is bad in the surprising and dangerous sense and I think it should be improved.

Do I need to raise this to the developers mailing list?

in reply to:  7 comment:8 by Mariusz Felisiak, 4 years ago

Replying to Gordon Wrigley:

Thanks for this ticket, however it was already discussed and it's a documented behavior

I appreciate that it's documented behaviour, what I'm saying is the documented behaviour is bad in the surprising and dangerous sense and I think it should be improved.

Do I need to raise this to the developers mailing list?

Yes, you can start a discussion on DevelopersMailingList if you don't agree. Please refer to the original ticket and take into account that we need to reach a strong consensus on the mailing list to make a backward incompatible change in a documented behavior. It will be good to start from summarizing arguments mentioned in the original ticket and PR.

Personally, I believe that the current behavior is correct.

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