Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Depending on field-order in earlier migrations, not all necessary migrations are detected #725

Closed
vanschelven opened this issue Sep 1, 2023 · 3 comments

Comments

@vanschelven
Copy link
Contributor

vanschelven commented Sep 1, 2023

In a specific combination of circumstances Django does not detect changes to the CurrencyField that's associated with a given MoneyField when it should.

The problem occurs when the order in which the fields originally appear in your migrations is MoneyField-before-CurrencyField. As of django-money==3.2 (and Django==3.2) this order occurs when a MoneyField is added, but not when it is originally present in an initial CreateModel migration.

Such a migration looks like so:

migrations.AddField(
    model_name='bar',
    name='price',
    field=djmoney.models.fields.MoneyField(decimal_places=2, default=Decimal('0.0'), default_currency='EUR', max_digits=14),
),
migrations.AddField(
    model_name='bar',
    name='price_currency',
    field=djmoney.models.fields.CurrencyField(choices=[('XUA', 'ADB Unit of Account'), ...], default='EUR', editable=False, max_length=3),
),

At a later point it may happen that the CurrencyField changes (but not the MoneyField), e.g. because of an upgrade to pymoneyed. This in turn affects the choices of the CurrencyField and should result in makemigrations detecting a necessary change. However, this necessity is not detected.

To understand why, note that, to detect whether changes in the model have been made, Django constructs a fake set of apps/models based on the existing migrations and compares this to the currently existing one.

However, when the fields are ordered Money-before-Currency, the addition of the MoneyField to the fake model in the fake set will trigger the immediate addition of a CurrencyField of the current kind, i.e. not as it was at the time of the migration, to the model. When it's finally time to add the CurrencyField as defined in the migration, this is prevented by a check in the code. Later, when fake past and actual present are compared, they appear identical because the fake past is affected by the actual present and makemigrations declares there are no changes detected.

By contrast, when the CurrencyField comes first in the migration file, this condition does not occur, because the field from the past is already added in (it's in the migration file) when the MoneyField attempts to do its thing. And when MoneyField attempts to add in the CurrencyField in its present form (the problem described in the above), this is prevented by the same check as mentioned in the above.

All of this is probably quite irrelevant in practice, because choices don't affect databases, but it did confuse me immensely and was hard to debug. Because migrations affect data and should not be dealt with lightly this still merited quite a lot of research. It may have similar effects on other people too.

The condition which exposed this for me was: squashing migrations, leading to reordering of fields because AddField was mixed into a CreateModel rather than as a separate step. This all of a sudden exposed the necessity for a migration that was in reality long overdue (the upgrade of pymoneyed happened a long time ago).

@vanschelven
Copy link
Contributor Author

Looking at the commit logs it looks like a previous version of django-money did not have this problem, because it explicitly checked whether it was running as part of a migration or not. However, this code was removed, presumably as part of the south-to-regular-djangomigrations process

vanschelven added a commit to vanschelven/django-money that referenced this issue Sep 1, 2023
in that case the currency field should/will simply be added because it
is explicitly defined as part of some migration.

See django-money#725
@vanschelven
Copy link
Contributor Author

Inspired by the previously existing solution, I have contributed what I think could/should be the direction forwards.

@benjaoming
Copy link
Contributor

Impressive work @vanschelven 🙏

vanschelven added a commit to vanschelven/django-money that referenced this issue Sep 4, 2023
benjaoming pushed a commit that referenced this issue Sep 4, 2023
…726)

* Don't contribute a currency field when in the 'fake' migration world

in that case the currency field should/will simply be added because it
is explicitly defined as part of some migration.

See #725

* Changelog for #725
DragoonAethis added a commit to DragoonAethis/Coriolis that referenced this issue Oct 2, 2023
django-money/django-money#725 caused an empty migration to crash due to
a missing currency field in the fake DB state. Reverting to 3.2.0 fixes
this for now, needs more debug later.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants