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

[BUG] Sort m2m fields before comparing them with diff(...) #271

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

mykolasolodukha
Copy link
Contributor

@mykolasolodukha mykolasolodukha commented Oct 13, 2022

What?

  • Sort new_m2m_fields before comparing them using dictdiffer.diff(...).

Why?

Otherwise, if one adds more m2m fields, the diff would produce the action="change" result, instead of the action="add" one. See:

How to test?

Try migrating this:

# models.py
class User(tortoise.Model):

    id = fields.BigIntField(pk=True, generated=False)

    groups: fields.ManyToManyRelation[Group]

class Group(tortoise.Model):

    users: fields.ManyToManyRelation[User] = fields.ManyToManyField(
        "bot.User", through="group__user", related_name="groups"
    )

To this:

# models.py
class User(tortoise.Model):

    id = fields.BigIntField(pk=True, generated=False)

    groups: fields.ManyToManyRelation[Group]
    admin_in_groups: fields.ManyToManyRelation[Group]

class Group(tortoise.Model):

    users: fields.ManyToManyRelation[User] = fields.ManyToManyField(
        "bot.User", through="group__user", related_name="groups"
    )
    admins: fields.ManyToManyRelation[User] = fields.ManyToManyField(
        "bot.User", through="group__admin", related_name="admin_in_groups"
    )
aerich migrate --name "add_group_admins"

Anything else?

I think this kind of sorting might help with other types of fields as well, but I have no time to investigate that right now. The fix in this PR is urgent to me, though.

@mykolasolodukha mykolasolodukha force-pushed the hotfix/sort_m2m_fields_before_comparison branch from f9b911d to d7dd05e Compare October 13, 2022 17:09
@LanceMoe
Copy link

I have the same problem, aerich doesn't work at all. This PR solves the problem. Please merge this!

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.

None yet

2 participants