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

fix(migration): make migration compatible with django 3 and 4 #10

Merged
merged 3 commits into from
Jul 29, 2024

Conversation

pkyosx
Copy link
Contributor

@pkyosx pkyosx commented Jul 29, 2024

Why need this change ? / Root Cause :

When we try to migrate django 3 to django 4, the existing migration script will fail due to the following reason.

[2024-07-29T02:12:15.059Z]   File "/usr/local/lib/python3.8/site-packages/django/db/models/options.py", line 681, in get_field
[2024-07-29T02:12:15.059Z]     return self.fields_map[field_name]
[2024-07-29T02:12:15.059Z] KeyError: 'user'
[2024-07-29T02:12:15.059Z] 
[2024-07-29T02:12:15.059Z] During handling of the above exception, another exception occurred:
[2024-07-29T02:12:15.059Z] 
[2024-07-29T02:12:15.059Z] Traceback (most recent call last):
[2024-07-29T02:12:15.059Z]   File "./manage.py", line 56, in <module>
[2024-07-29T02:12:15.059Z]     main()
[2024-07-29T02:12:15.059Z]   File "./manage.py", line 34, in main
[2024-07-29T02:12:15.059Z]     execute_from_command_line(sys.argv)
[2024-07-29T02:12:15.059Z]   File "/usr/local/lib/python3.8/site-packages/django/core/management/__init__.py", line 442, in execute_from_command_line
[2024-07-29T02:12:15.059Z]     utility.execute()
[2024-07-29T02:12:15.059Z]   File "/usr/local/lib/python3.8/site-packages/django/core/management/__init__.py", line 436, in execute
[2024-07-29T02:12:15.059Z]     self.fetch_command(subcommand).run_from_argv(self.argv)
[2024-07-29T02:12:15.059Z]   File "/home/fms/lib/util_command.py", line 83, in run_from_argv
[2024-07-29T02:12:15.059Z]     return super().run_from_argv(*args, **kwargs)
[2024-07-29T02:12:15.059Z]   File "/usr/local/lib/python3.8/site-packages/django/core/management/base.py", line 412, in run_from_argv
[2024-07-29T02:12:15.059Z]     self.execute(*args, **cmd_options)
[2024-07-29T02:12:15.059Z]   File "/home/fms/lib/util_command.py", line 87, in execute
[2024-07-29T02:12:15.059Z]     return super().execute(*args, **kwargs)
[2024-07-29T02:12:15.059Z]   File "/home/fms/lib/util_command.py", line 54, in execute
[2024-07-29T02:12:15.059Z]     output = super().execute(*args, **options)
[2024-07-29T02:12:15.059Z]   File "/usr/local/lib/python3.8/site-packages/django/core/management/base.py", line 458, in execute
[2024-07-29T02:12:15.059Z]     output = self.handle(*args, **options)
[2024-07-29T02:12:15.059Z]   File "/usr/local/lib/python3.8/site-packages/django/core/management/base.py", line 106, in wrapper
[2024-07-29T02:12:15.059Z]     res = handle_func(*args, **kwargs)
[2024-07-29T02:12:15.059Z]   File "/usr/local/lib/python3.8/site-packages/django/core/management/commands/migrate.py", line 356, in handle
[2024-07-29T02:12:15.059Z]     post_migrate_state = executor.migrate(
[2024-07-29T02:12:15.059Z]   File "/usr/local/lib/python3.8/site-packages/django/db/migrations/executor.py", line 135, in migrate
[2024-07-29T02:12:15.059Z]     state = self._migrate_all_forwards(
[2024-07-29T02:12:15.059Z]   File "/usr/local/lib/python3.8/site-packages/django/db/migrations/executor.py", line 167, in _migrate_all_forwards
[2024-07-29T02:12:15.059Z]     state = self.apply_migration(
[2024-07-29T02:12:15.059Z]   File "/usr/local/lib/python3.8/site-packages/django/db/migrations/executor.py", line 252, in apply_migration
[2024-07-29T02:12:15.059Z]     state = migration.apply(state, schema_editor)
[2024-07-29T02:12:15.059Z]   File "/usr/local/lib/python3.8/site-packages/django/db/migrations/migration.py", line 132, in apply
[2024-07-29T02:12:15.059Z]     operation.database_forwards(
[2024-07-29T02:12:15.059Z]   File "/usr/local/lib/python3.8/site-packages/django/db/migrations/operations/models.py", line 1178, in database_forwards
[2024-07-29T02:12:15.059Z]     schema_editor.remove_constraint(model, constraint)
[2024-07-29T02:12:15.059Z]   File "/usr/local/lib/python3.8/site-packages/django/db/backends/mysql/schema.py", line 125, in remove_constraint
[2024-07-29T02:12:15.059Z]     and constraint.create_sql(model, self) is not None
[2024-07-29T02:12:15.059Z]   File "/usr/local/lib/python3.8/site-packages/django/db/models/constraints.py", line 234, in create_sql
[2024-07-29T02:12:15.059Z]     fields = [model._meta.get_field(field_name) for field_name in self.fields]
[2024-07-29T02:12:15.059Z]   File "/usr/local/lib/python3.8/site-packages/django/db/models/constraints.py", line 234, in <listcomp>
[2024-07-29T02:12:15.059Z]     fields = [model._meta.get_field(field_name) for field_name in self.fields]
[2024-07-29T02:12:15.059Z]   File "/usr/local/lib/python3.8/site-packages/django/db/models/options.py", line 683, in get_field
[2024-07-29T02:12:15.059Z]     raise FieldDoesNotExist(
[2024-07-29T02:12:15.059Z] django.core.exceptions.FieldDoesNotExist: IdempotencyKey has no field named 'user'

Changed Made :

  • Adjust migration script and make sure it works on both django 3 and django 4.
  • Remove potential duplicated keys when we drop user
  • Upgrade isort, autoflake and flake8

Test Scope / Change Impact :

@pkyosx pkyosx requested review from xeonchen and CJHwong July 29, 2024 05:04
@pkyosx pkyosx force-pushed the feat/django-4-migration branch from 505a304 to 8b2639d Compare July 29, 2024 05:09
@pkyosx pkyosx requested a review from CJHwong July 29, 2024 08:10
Copy link
Member

@mattwang44 mattwang44 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Q: 那 django 3.x 也能使用這個版本嗎?之前好像是踩到 issue 才改變 migration 順序的
會不會需要去 setup.py 裡面限定 compatible 的 django 版本?

@pkyosx
Copy link
Contributor Author

pkyosx commented Jul 29, 2024

Q: 那 django 3.x 也能使用這個版本嗎?之前好像是踩到 issue 才改變 migration 順序的 會不會需要去 setup.py 裡面限定 compatible 的 django 版本?

可以,這個 PR 就是為了讓 3.x 跟 4.x 都相容,原先遇到的問題是不能夠在 foreign key 還在的狀態下拔掉 unique key,所以我們在前面加上一個步驟先把 foreign key 變成單純的 integer

@pkyosx pkyosx merged commit 7d8108b into main Jul 29, 2024
2 checks passed
Comment on lines +9 to +11
for id_key in IdempotencyKey.objects.all():
if id_key.idempotency_key in existed_keys:
id_key.delete()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 questions:

  • I'm wondering if it's safe to delete instances within a loop. Gemini told me it's unsafe but it doesn't provide a reference.
  • I don't see any sorting options, how do you decide which one to delete when duplicated?

Copy link
Contributor Author

@pkyosx pkyosx Jul 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. 他擔心會發生 partial failure 吧,假設後面發生錯誤但是前面已經 delete 掉一些東西了。 不過我們的邏輯相對單純不太需要擔心他跑到後面 exception 跳掉。
  2. 因為這個 duplication 源自於不同的 user,實際上會重複 IdempotencyKey 的狀況我們發現也都幾乎是同時發出來的,是基於 bug 導致一個 user=None 的狀況使得他認成兩個。 當我們砍掉 user 以後兩個 record 就視為同一個了,在這種狀況下不管砍掉哪一個差異不大。

Copy link
Collaborator

@xeonchen xeonchen Jul 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

他擔心會發生 partial failure 吧

No, in most programming languages (even in Python), you shouldn't remove any item from a list during the iteration. It's because the removal changes the list itself.

I'm not sure if the django orm query support this operation.

實際上會重複 IdempotencyKey 的狀況我們發現也都幾乎是同時發出來的,是基於 bug 導致一個 user=None 的狀況使得他認成兩個

what if other columns are different? why not remove both?

Copy link
Contributor Author

@pkyosx pkyosx Jul 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. 這個說法蠻奇怪的,因為我不是從 list 裡面移除東西,而是把 list 裡面的某個 item 內容作改變 (decoupled from DB),Gemini 那段描述是沒有問題的,的確不應該一邊 iterate list 一邊更動 list,但是他套用的精確度有待加強。

要做到他說的事情 要走類似下面的方式 不過新版的 python 會噴就是了

> x = {i: i for i in range(3)}
> for i in x: del x[i]

Traceback (most recent call last):
  File "/Users/seth_wang/.pyenv/versions/3.8.16/lib/python3.8/code.py", line 90, in runcode
    exec(code, self.locals)
  File "<console>", line 1, in <module>
RuntimeError: dictionary changed size during iteration

這種手法interpreter就判斷不出來了,但是我們要操作的會是 container 本身 (list, dict, set, ...),不是裡面的 item

> x = [1,2,3]
> for i in x: x.remove(i)
# x will be [2]
  1. 如果都砍掉的話我們剛好在 migrate 期間的 lock 會瞬間被清掉不受保護。 建議還是至少保留一個比較安全。

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

Successfully merging this pull request may close these issues.

4 participants