emmett-framework / emmett

The web framework for inventors
BSD 3-Clause "New" or "Revised" License
1.08k stars 72 forks source link

Bug in migrations with MSSQL Server #365

Closed josejachuf closed 3 years ago

josejachuf commented 3 years ago

Generally use PostgreSQL, but now I am using MSSQL Server (Freetds + Unixodbc + pyodbc). When I try to use migrations, an error occurs.

I already know what it is but I have not managed to solve it

This is the generated SQL:

CREATE TABLE "auth_memberships"(
    "id" BIGINT IDENTITY PRIMARY KEY,
    "created_at" DATETIME NULL,
    "updated_at" DATETIME NULL,
    "user" BIGINT NULL, CONSTRAINT "auth_memberships_user__constraint" FOREIGN KEY ("id") REFERENCES "auth_users" ("id") ON DELETE CASCADE,
    "auth_group" BIGINT NULL, CONSTRAINT "auth_memberships_auth_group__constraint" FOREIGN KEY ("id") REFERENCES "auth_groups" ("id") ON DELETE CASCADE
);

Note:

FOREIGN KEY ("id")

And should generate the following:

CREATE TABLE "auth_memberships"(
    "id" BIGINT IDENTITY PRIMARY KEY,
    "created_at" DATETIME NULL,
    "updated_at" DATETIME NULL,
    "user" BIGINT NULL, CONSTRAINT "auth_memberships_user__constraint" FOREIGN KEY ("user") REFERENCES "auth_users" ("id") ON DELETE CASCADE,
    "auth_group" BIGINT NULL, CONSTRAINT "auth_memberships_auth_group__constraint" FOREIGN KEY ("auth_group") REFERENCES "auth_groups" ("id") ON DELETE CASCADE
);

FOREIGN KEY ("user")

and

FOREIGN KEY ("auth_group")

They are the right ones

Jose

gi0baro commented 3 years ago

@josejachuf I'm checking several parts of Emmett and pyDAL code and I'm struggling find the problematic lines.

Do migrations look good when you define a relation in your models? Or, put in another way, is the issue related just to auth models or spread over everything?

josejachuf commented 3 years ago

Hi @gi0baro The generated migration file does well, at least equal to PostgreSQL.

self.create_table(
      'auth_memberships',
      migrations.Column('id', 'id'),
      migrations.Column('created_at', 'datetime'),
      migrations.Column('updated_at', 'datetime'),
      migrations.Column('user', 'reference auth_users', ondelete='CASCADE'),
      migrations.Column('auth_group', 'reference auth_groups', ondelete='CASCADE'))

From pydal:

    @sqltype_for('big-reference')
    def type_big_reference(self):
        return 'BIGINT%(null)s%(unique)s, CONSTRAINT %(constraint_name)s' + \
            ' FOREIGN KEY (%(field_name)s) REFERENCES %(foreign_key)s ' + \
            'ON DELETE %(on_delete_action)s'

    @sqltype_for('reference FK')
    def type_reference_fk(self):
        return ', CONSTRAINT FK_%(constraint_name)s FOREIGN KEY ' + \
            '(%(field_name)s) REFERENCES %(foreign_key)s ON DELETE ' + \
            '%(on_delete_action)s'

Apparently this is fine:

FOREIGN KEY (%(field_name)s)

I think that when it is called this, the parameter is going wrong

Jose

gi0baro commented 3 years ago

@josejachuf this should be fixed with https://github.com/emmett-framework/emmett/commit/46c765114ce5078b93361b218ef8197e461fad8f, I gonna backport this to 2.2 release later today

josejachuf commented 3 years ago

@gi0baro, this [1] patch works fine, but this broken the theme of the FK and ondelete, at least with MSSQL Server 2017 and my desing. For example:

class Signature(Model):
    refers_to (
        {'created_by': 'User'},
        {'updated_by': 'User'}
        )

    created_at = Field.datetime()
    updated_at = Field.datetime()

    default_values = {
        'created_by': lambda: get_user(),
        'updated_by': lambda: get_user(),
        'created_at': lambda: local_now(),
        'updated_at': lambda: local_now()
    }

    update_values = {
        'updated_by': lambda: get_user(),
        'updated_at': lambda: local_now()
    }

    fields_rw = {
        'created_by': False,
        'updated_by': False,
        'created_at': False,
        'updated_at': False
    }

The other models inherit from Signature

Does not like the relations refers_to, also, does not like it when there is more than one FK field with reference to the same table

refers_to create something similar to: migrations.Column('created_by', 'reference auth_users', ondelete='SET NULL'),

I had to change all the SET NULL to NO ACTION, In Model Signature I had to remove created_by and updated_by

With this you can complete the migration of the model.

Reading I see that the error message:

Introducing FOREIGN KEY constraint 'mytable__constraint' on table 'mytable' may cause cycles or multiple cascade paths. Specify ON DELETE NO ACTION or ON UPDATE NO ACTION, or modify other FOREIGN KEY constraints.

is something known at MSSQL Server (I do not know if in all versions) and quite frequent appears in the queries.

Jose

[1] https://github.com/emmett-framework/emmett/commit/46c765114ce5078b93361b218ef8197e461fad8f [2] https://www.mssqltips.com/sqlservertip/2733/solving-the-sql-server-multiple-cascade-path-issue-with-a-trigger/ [3] https://stackoverflow.com/questions/851625/foreign-key-constraint-may-cause-cycles-or-multiple-cascade-paths

gi0baro commented 3 years ago

Does not like the relations refers_to, also, does not like it when there is more than one FK field with reference to the same table

refers_to create something similar to: migrations.Column('created_by', 'reference auth_users', ondelete='SET NULL'),

I had to change all the SET NULL to NO ACTION, In Model Signature I had to remove created_by and updated_by

With this you can complete the migration of the model.

Reading I see that the error message:

Introducing FOREIGN KEY constraint 'mytable__constraint' on table 'mytable' may cause cycles or multiple cascade paths. Specify ON DELETE NO ACTION or ON UPDATE NO ACTION, or modify other FOREIGN KEY constraints.

is something known at MSSQL Server (I do not know if in all versions) and quite frequent appears in the queries.

Jose

[1] 46c7651 [2] https://www.mssqltips.com/sqlservertip/2733/solving-the-sql-server-multiple-cascade-path-issue-with-a-trigger/ [3] https://stackoverflow.com/questions/851625/foreign-key-constraint-may-cause-cycles-or-multiple-cascade-paths

@josejachuf I'm not sure how to fix this. I'm not even sure Emmett should deal with a specific engine limitations.

Here is the best I can do: add something on Model definition to specify relations on_delete policies, but this won't be backported in 2.2 since is a "new feature". Gonna open up a new issue for that and target 2.3, and close this as soon as I release a new 2.2 patch version.

gi0baro commented 3 years ago

@josejachuf this is fixed in master and 2.2.4

The follow-up part regarding on-delete policies can be found in #368