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

Composite primary key migration #616

Merged
merged 3 commits into from
Jan 21, 2021

Conversation

Whaxion
Copy link
Contributor

@Whaxion Whaxion commented Jan 21, 2021

This PR is about the migration part of the composite primary key implementation.
It adds composite_primary_key to the create table migrator.
It works like this

def migrate
  create table_for(UserPost) do
    add_belongs_to user : User, on_delete: :cascade
    add_belongs_to post : Post, on_delete: :cascade
    primary_key :user_id, :post_id
  end
end

There is still some work to do with add_belongs_to if it's a composite PK but for that it needs some work on the model part.
Issue #129

{% if columns.size < 2 %}
{% raise "composite_primary_key expected at least two primary keys, instead got #{columns.size}" %}
{% end %}
constraints << " PRIMARY KEY ({{columns.join(", ").id}})"
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain why you added this constraints array instead of adding it to the rows array?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's because constraints must be at the end of create table declaration, so if it was in row, the user should call composite_primary_key at the end of the migration.
So user couldn't do a migration like this:

def migrate
  create table_for(UserPost) do
    add_belongs_to user : User, on_delete: :cascade
    add_belongs_to post : Post, on_delete: :cascade
    composite_primary_key :user_id, :post_id

    add example : String
  end
end

But should do this:

def migrate
  create table_for(UserPost) do
    add_belongs_to user : User, on_delete: :cascade
    add_belongs_to post : Post, on_delete: :cascade

    add example : String

    composite_primary_key :user_id, :post_id
  end
end

And I find that's sad that we can't put the composite_primary_key where we want so I added a constraints array.
But if that's not okay I can change that.

Copy link
Member

Choose a reason for hiding this comment

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

@Whaxion I didn't know that it had to be at the end. Good thinking. Could you add a spec showing that a composite key is added at the end of the sql statement even if it is declared first? I'd like to make sure we remember to check for that behavior even if we change things in the future.

Copy link
Member

@matthewmcgarvey matthewmcgarvey left a comment

Choose a reason for hiding this comment

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

Nice job @Whaxion 🎉

@matthewmcgarvey matthewmcgarvey merged commit fa1ba00 into luckyframework:master Jan 21, 2021
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