[Django] #22534: Checks fail on non-swappable ForeignKeys when the related model is swapped out
Django
out
---------------------------------+--------------------
Reporter: bendavis78 | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: master
Severity: Release blocker | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
---------------------------------+--------------------
The documentation states that setting `swappable=False` on a `ForeignKey`
allows you to reference a swappable model even if that model is swapped
out. Unfortunately this doesn't work because RelatedField's check function
will look for that model in `apps.get_models()`, which doesn't include
swapped models by default.
If a `ForeignKey` references a swapped out model and has swappable=False,
shouldn't that model be loaded? It seems to me that `swapped=False` will
never work because of this.
--
Ticket URL: <https://code.djangoproject.com/ticket/22534>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
out
Reporter: bendavis78 | Owner: nobody
Type: Bug | Status: new
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timo):
* needs_better_patch: => 0
* version: master => 1.7-beta-2
* stage: Unreviewed => Accepted
* needs_tests: => 0
* needs_docs: => 0
Comment:
Haven't completely verified this, but I don't see any tests for
`swappable=False` in c9de1b4a55713ad1557f8c40621226253038f665.
--
Ticket URL: <https://code.djangoproject.com/ticket/22534#comment:1>
Django
out
Reporter: bendavis78 | Owner: nobody
Type: Bug | Status: new
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by bendavis78):
For reference, the swappable keyword was originally proposed here:
https://groups.google.com/forum/#!msg/django-
developers/arSS604xc6U/24j1vKtlESoJ
Although, it was not implemented as proposed. Russel suggested that the
`swappable` keyword default to `None`, as we can safely assume that when
the "to" model is not a string, it should point to the original. If `to`
is a string that points to a swappable model, we would assume that it
should point to the swapped model. Any other case would be an edge case,
and can be controlled explicitly by setting the keyword to either `True`
or `False`.
--
Ticket URL: <https://code.djangoproject.com/ticket/22534#comment:2>
Django
out
Reporter: bendavis78 | Owner: nobody
Type: Bug | Status: new
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by bendavis78):
This is my initial attempt to fix this. It passes current tests but it
still needs tests for `swappable=None/True/False` (which I can do soon):
https://github.com/bendavis78/django/commit/cc70dd04fcc2a36592a6896b253c6adda8651cf4
The current change fixes the issue for startup checks, however I'm not
100% sure what should be done with migrations, if anything. Migrations
will still fail if a foreign key points to a swappable model that has been
swapped out (regardless of whether or not its a string). It may be that it
should stay this way but I'd probably need someone to chime in on that.
Not sure if I'm on the right track here, or even if this bug would
realistically effect anyone else (I do have a workaround for my use case).
--
Ticket URL: <https://code.djangoproject.com/ticket/22534#comment:3>
Django
out
Reporter: bendavis78 | Owner: andrewgodwin
Type: Bug | Status: assigned
Component: Migrations | Version: 1.7-beta-2
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by andrewgodwin):
* owner: nobody => andrewgodwin
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/22534#comment:4>
Django
out
Reporter: bendavis78 | Owner: andrewgodwin
Type: Bug | Status: assigned
Component: Migrations | Version: 1.7-beta-2
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by andrewgodwin):
* severity: Release blocker => Normal
Comment:
Your parsing of what swappable means is incorrect; it's not for a
swappable model, it's for a model that might be swapped in in place of
that model. It won't allow you to access models that are swapped out, and
that _should_ fail the checks system immediately.
I'm bumping this down from release blocker as it's just a documentation
clarification and a missing test for the false side IMO (we didn't
implement Russ' plan exactly; from what I remember, the None option wasn't
necessary in the end due to the limited options migrations had dealing
with it).
--
Ticket URL: <https://code.djangoproject.com/ticket/22534#comment:5>
Django
out
Reporter: bendavis78 | Owner: andrewgodwin
Type: Bug | Status: assigned
Component: Migrations | Version: 1.7-beta-2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by bendavis78):
* cc: bendavis78 (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/22534#comment:6>
Django
out
Reporter: bendavis78 | Owner: andrewgodwin
Type: Bug | Status: assigned
Component: Migrations | Version: 1.7-beta-2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by bendavis78):
Ah, so if I understand correctly, the meaning of `swappable=False` is
"System check should fail if the referenced model is swapped out", and
`swappable=True` (the default) means "Allow model to be swapped out". Is
that correct?
--
Ticket URL: <https://code.djangoproject.com/ticket/22534#comment:7>
Django
out
Reporter: bendavis78 | Owner: andrewgodwin
Type: Bug | Status: assigned
Component: Migrations | Version: 1.7-beta-2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by andrewgodwin):
Not quite; first of all, this is designed for FKs that point to models
that might be the swap-in replacement - e.g. `myapp.User`, not
`django.contrib.auth.User`.
`swappable=True` means "if the other end of this foreignkey is currently
the value of AUTH_USER_MODEL then make this FK point to
`settings.AUTH_USER_MODEL`
`swappable=False` means "even if the other end of this is the current
value of AUTH_USER_MODEL, don't do a swappable link, do it directly, as I
really want to always link to `myapp.User`, never to the generic user
model". It's meant for companion models that are definitely only for a
certain user model.
--
Ticket URL: <https://code.djangoproject.com/ticket/22534#comment:8>
Django
out
Reporter: bendavis78 | Owner: andrewgodwin
Component: Migrations | Version: 1.7-beta-2
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Andrew Godwin <andrew@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"6d504562f5f27698a668759254101290b82e89b9"]:
{{{
#!CommitTicketReference repository=""
revision="6d504562f5f27698a668759254101290b82e89b9"
Fixed #22534: Reinforce swappable documentation
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/22534#comment:9>
Django
out
Reporter: bendavis78 | Owner: andrewgodwin
Type: Bug | Status: closed
Component: Migrations | Version: 1.7-beta-2
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Andrew Godwin <andrew@…>):
In [changeset:"ffa9e60638bc1447cba9ddef4585b70267c435f7"]:
{{{
#!CommitTicketReference repository=""
revision="ffa9e60638bc1447cba9ddef4585b70267c435f7"
[1.7.x] Fixed #22534: Reinforce swappable documentation
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/22534#comment:10>