Opened 13 months ago

Closed 9 months ago

Last modified 9 months ago

#22534 closed Bug (fixed)

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

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

Description

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 13 months ago by timo

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted
  • Version changed from master to 1.7-beta-2

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

comment:2 Changed 13 months ago 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.

comment:3 Changed 13 months ago 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).

comment:4 Changed 13 months ago by andrewgodwin

  • Owner changed from nobody to andrewgodwin
  • Status changed from new to assigned

comment:5 Changed 13 months ago by andrewgodwin

  • Severity changed from Release blocker to Normal

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 12 months ago by bendavis78

  • Cc bendavis78 added

comment:7 Changed 12 months ago 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 the referenced model to be swapped out". Is that correct?

Last edited 12 months ago by bendavis78 (previous) (diff)

comment:8 Changed 12 months ago 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.

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

  • Resolution set to fixed
  • Status changed from assigned to closed

In 6d504562f5f27698a668759254101290b82e89b9:

Fixed #22534: Reinforce swappable documentation

comment:10 Changed 9 months 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