Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#22534 closed Bug (fixed)

Checks fail on non-swappable ForeignKeys when the related model is swapped out

Reported by: Ben Davis Owned by: Andrew Godwin
Component: Migrations Version: 1.7-beta-2
Severity: Normal Keywords:
Cc: Ben Davis Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no


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.

Change History (10)

comment:1 Changed 2 years ago by Tim Graham

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset
Triage Stage: UnreviewedAccepted
Version: master1.7-beta-2

Haven't completely verified this, but I don't see any tests for swappable=False in c9de1b4a55713ad1557f8c40621226253038f665.

comment:2 Changed 2 years ago by Ben Davis

For reference, the swappable keyword was originally proposed here:!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.

comment:3 Changed 2 years ago by Ben Davis

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):

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).

comment:4 Changed 2 years ago by Andrew Godwin

Owner: changed from nobody to Andrew Godwin
Status: newassigned

comment:5 Changed 2 years ago by Andrew Godwin

Severity: Release blockerNormal

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).

comment:6 Changed 2 years ago by Ben Davis

Cc: Ben Davis added

comment:7 Changed 2 years ago by Ben Davis

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 the referenced model to be swapped out". Is that correct?

Last edited 2 years ago by Ben Davis (previous) (diff)

comment:8 Changed 2 years ago by Andrew Godwin

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.

comment:9 Changed 2 years ago by Andrew Godwin <andrew@…>

Resolution: fixed
Status: assignedclosed

In 6d504562f5f27698a668759254101290b82e89b9:

Fixed #22534: Reinforce swappable documentation

comment:10 Changed 2 years ago by Andrew Godwin <andrew@…>

In ffa9e60638bc1447cba9ddef4585b70267c435f7:

[1.7.x] Fixed #22534: Reinforce swappable documentation

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