Opened 22 months ago

Closed 22 months ago

Last modified 22 months ago

#29125 closed Bug (fixed)

Q.deconstruct() is nondeterministic if the Q has multiple kwargs

Reported by: Harro Owned by: nobody
Component: Database layer (models, ORM) Version: 2.0
Severity: Release blocker Keywords:
Cc: hvdklauw@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Harro)

Here is a branch with a randomly failing test that proves this: https://github.com/hvdklauw/django/blob/bug/q_destruct/tests/queries/test_q.py#L63

The biggest issue is that now makemigrations is detecting changes since we upgraded to django 2.0 (only with python 3.5) that aren't changes at all, just reordered kwargs on the Q objects in out limit_choices_to on some foreignkeys.

This also means we randomly can't commit/release because we have pre-commit hooks and CI that runs makemigrations --check --dryrun

Upgrading to python 3.6 would fix it (because of ordered kwargs) but as Ubuntu 16.04 is still the latest LTS, python 3.5 is what we are stuck with.

This is the commit that broke it: https://github.com/django/django/commit/508b5debfb16843a8443ebac82c1fb91f15da687

Change History (7)

comment:1 Changed 22 months ago by Harro

Description: modified (diff)

comment:2 Changed 22 months ago by Simon Charette

Triage Stage: UnreviewedAccepted

Apart from preserving the kwargs nature of some children I'd argue that Q.deconstruct should also use django.utils.models.Q as path instead of django.db.models.query_utils.Q like we do with django.db.models.fields.* and should avoid adding _connector if it's 'AND' and _negated if it's False to kwargs because these are the default values.

comment:3 Changed 22 months ago by Tim Graham

Has patch: set
Keywords: has_test removed
Summary: BUG: Q object deconstruct is inconsistent when passing multiple kwargs.Q.deconstruct() is nondeterministic if the Q has multiple kwargs

PR

I don't understand why {'_connector': 'AND'} should be omitted but I implemented it.

comment:4 Changed 22 months ago by Tim Graham <timograham@…>

Resolution: fixed
Status: newclosed

In b95c49c9:

Fixed #29125 -- Made Q.deconstruct() deterministic with multiple keyword arguments.

comment:5 Changed 22 months ago by Tim Graham <timograham@…>

In 9ba3df8:

Refs #29125 -- Made Q.deconstruct() omit 'query_utils' in the path and _connector='AND' since it's a default value.

comment:6 Changed 22 months ago by Tim Graham <timograham@…>

In aeb35548:

[2.0.x] Fixed #29125 -- Made Q.deconstruct() deterministic with multiple keyword arguments.

Backport of b95c49c954e3b75678bb258e9fb2ec30d0d960bb from master

comment:7 Changed 22 months ago by Tim Graham <timograham@…>

In fd18345e:

[2.0.x] Refs #29125 -- Made Q.deconstruct() omit 'query_utils' in the path and _connector='AND' since it's a default value.

Backport of 9ba3df82402e7e23b353da20aea6894935241ef9 from master

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