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

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
Contributor

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

Copy link
Contributor

@waketzheng waketzheng left a comment

Choose a reason for hiding this comment

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

It is better to only sort new m2m fields when it has more than one elements:

if len(new_m2m_fields) > 1 and old_m2m_fields:
    length = len(old_m2m_fields)
    field_index = {f['name']: i for i, f in enumerate(new_m2m_fields)}
    new_m2m_fields.sort(key=lambda field: field_index.get(field['name'], length))

@waketzheng
Copy link
Contributor

This PR will fixes #150

@waketzheng waketzheng merged commit 8cefe68 into tortoise:dev Dec 5, 2024
5 checks passed
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.

3 participants