Skip to content

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

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Jul 29, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1e2f50f
fix: upgrade pre-commit outdated packages
pkyosx Jul 29, 2024
c5367d3
fix(migration): fix migration failure
pkyosx Jul 29, 2024
8b2639d
feat: bump version
pkyosx Jul 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions .pre-commit-config.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,23 +7,23 @@ repos:
- id: end-of-file-fixer
- id: trailing-whitespace
- repo: https://github.com/myint/autoflake
rev: v1.4
rev: v2.0.0
hooks:
- id: autoflake
args:
- --in-place
- --remove-unused-variables
- --remove-all-unused-imports
- repo: https://github.com/pycqa/isort
rev: 5.10.1
rev: 5.12.0
hooks:
- id: isort
- repo: https://github.com/psf/black
rev: 22.3.0
hooks:
- id: black
language_version: python3 # Should be a command that runs python3.6+
- repo: https://gitlab.com/pycqa/flake8
rev: '3.9.2'
- repo: https://github.com/PyCQA/flake8
rev: 6.0.0
hooks:
- id: flake8
9 changes: 9 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,15 @@
Changelog
=========

1.0.3
-----

Fixes:

- Fail to run migration script for django 4
- Fail to run migration script when duplicated idempotency_key exists


1.0.2
-----

Expand Down
2 changes: 1 addition & 1 deletion rest_framework_idempotency_key/__version__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
__version__ = '1.0.2'
__version__ = '1.0.3'
18 changes: 17 additions & 1 deletion rest_framework_idempotency_key/migrations/0003_remove_user.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,21 +3,37 @@
from django.db import migrations, models


def remove_duplicated_records(apps, schema_editor):
IdempotencyKey = apps.get_model('rest_framework_idempotency_key', 'IdempotencyKey')
existed_keys = set()
for id_key in IdempotencyKey.objects.all():
if id_key.idempotency_key in existed_keys:
id_key.delete()
Comment on lines +9 to +11
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 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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 會瞬間被清掉不受保護。 建議還是至少保留一個比較安全。

else:
existed_keys.add(id_key.idempotency_key)


class Migration(migrations.Migration):

dependencies = [
('rest_framework_idempotency_key', '0002_alter_idempotencykey_user'),
]

operations = [
migrations.RemoveField(
migrations.RunPython(remove_duplicated_records, migrations.RunPython.noop),
migrations.AlterField(
model_name='idempotencykey',
name='user',
field=models.IntegerField(null=True),
),
migrations.RemoveConstraint(
model_name='idempotencykey',
name='Unique IdempotencyKey (user, idempotency_key)',
),
migrations.RemoveField(
model_name='idempotencykey',
name='user',
),
migrations.AddConstraint(
model_name='idempotencykey',
constraint=models.UniqueConstraint(
Expand Down
5 changes: 1 addition & 4 deletions setup.cfg
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,5 @@
[flake8]
extend-ignore=
E203, # See https://github.com/PyCQA/pycodestyle/issues/373
E501, # style related, follow black
W503, # style related, follow black
extend-ignore= E203,E501,W503
max-line-length = 120
max-complexity = 100

Expand Down
Loading