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

Schema: Add foreign key constraints #46

Closed
wants to merge 13 commits into from
Closed

Schema: Add foreign key constraints #46

wants to merge 13 commits into from

Conversation

Marlysson
Copy link
Contributor

@Marlysson Marlysson commented Mar 3, 2020

Indexes: Create/Drop foreign indexes by name

  • Add a new index named as linked tables[1]
  • Drop indexes [2]

Constraints: Create/Drop indexes by name

  • Create a foreign index indicating the linked tables[3]
  • Drop foreign index [2]

References:

[1] https://orator-orm.com/docs/0.9/schema_builder.html#adding-indexes
[2] https://orator-orm.com/docs/0.9/schema_builder.html#dropping-indexes
[3] https://orator-orm.com/docs/0.9/schema_builder.html#foreign-keys

@Marlysson
Copy link
Contributor Author

Marlysson commented Mar 3, 2020

@josephmancuso Does our unsigned method will have the different signature from others orms?

Instead:

blueprint.integer('some_value').unsigned()

Will be, right ?

blueprint.unsigned('some_value')
blueprint.unsigned_integer('some_value')

@josephmancuso
Copy link
Member

@Marlysson I think both

@josephmancuso
Copy link
Member

These 2 ways:

blueprint.integer('some_value').unsigned()
blueprint.unsigned_integer('some_value')

@Marlysson
Copy link
Contributor Author

Marlysson commented Mar 3, 2020

@josephmancuso I've tried abstract the relations with some way but just came in my mind use of dictionary haha, do you have some other way?[1] For now, are working, now I will implement BaseGrammar to handle these relations and test the complete constraint.

[1] I started developing one class called ForeignKeyRelation but it just has 3 attributes and nothing more. I avoided this idea.

class ForeignKeyRelation:

    # init method

    def foreign(self, local_column=None):
        self.local_column = local_column

    def references(self, external_column=None):
        self.external_column = external_column

    def on(self, table=None):
        self.foreign_table = table

[2] I think the use of constraint_type = "foreign_key" is too necessary ( to match with method constraint ), but I don't see where to put it on workflow.

@Marlysson Marlysson changed the title Adding some initial test cases to mysql foreign key. Schema: Add foreign key constraints Mar 5, 2020
self, inspect.currentframe().f_code.co_name.replace("test_", "")
)()
self.assertEqual(to_sql, sql)

Copy link
Member

Choose a reason for hiding this comment

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

You'll need a test that looks like this:

    def test_sets_foreign_constraint(self):
        with self.schema.create('users') as table:
             table.string('name')
             table.integer('profile_id')

             table.foreign('profile_id').references('user').on('users')

        sql = getattr(
            self, inspect.currentframe().f_code.co_name.replace("test_", "")
        )()
        self.assertEqual(to_sql, sql)

and then make the child test like this:

def sets_foreign_constraint(self):
     return (
        "CREATE TABLE `users` ("
            "`name` VARCHAR(255) NOT NULL, "
            "`profile_id` INT UNSIGNED,  "
                "INDEX profile_users_profile_id, "
                "FOREIGN KEY (`profile_id`)"
                "REFERENCES `profile`(`id`)"
        ")"
    )

Copy link
Contributor Author

@Marlysson Marlysson Mar 6, 2020

Choose a reason for hiding this comment

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

Ok, After that, I remove the exceptions that ensure fields are filled?
I will change the tests to this.

@@ -196,16 +199,33 @@ def unsigned(self, column, length=None, nullable=False):
def unsigned_integer(self):
pass

def foreign(self, *columns):
Copy link
Member

Choose a reason for hiding this comment

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

This definition looks wrong. It should only accept a column name:

table.foreign('profile_id').references('profile').on('id')

Copy link
Contributor Author

@Marlysson Marlysson Mar 6, 2020

Choose a reason for hiding this comment

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

Ok. I did put *args because of this statement:

FOREIGN KEY (product_category, product_id)
      REFERENCES product(category, id)

Referencing two columns in the same constraint.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here the reference: https://dev.mysql.com/doc/refman/8.0/en/create-table-foreign-keys.html#foreign-key-restrictions

The columns and table aren't quoted with `, I found it strange.

@josephmancuso
Copy link
Member

Is there a checkbox for you on the right side where it says something like "allow commits from repo owner"?

@josephmancuso
Copy link
Member

I'd like to work on this with you but by default idt I can push to this pull request

@Marlysson
Copy link
Contributor Author

It's checked, but with a different message.
Sem título

@@ -146,12 +146,12 @@ def create_column_length(self):
def unique_constraint_string(self):
return "CONSTRAINT {clean_column}_unique UNIQUE ({clean_column})"
Copy link
Contributor Author

@Marlysson Marlysson Mar 12, 2020

Choose a reason for hiding this comment

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

@corentinalcoy @josephmancuso
Does here is just add a *args to receive more than one column?

Copy link

Choose a reason for hiding this comment

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

No wait, Joe is looking the way the schema module is done. I think we have a problem...

@josephmancuso
Copy link
Member

Closing in favor of #77

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.

2 participants