Add support for composite primary keys

[paulcsmith]

trafficstars

Composite (PRIMARY KEY (id_1, id_2))

Extracted from #96

[Whaxion]

Any news on this ? Is there a way I can help about this ?

[jwoertink]

Hey @Whaxion! There hasn't been any movement on this yet. We know it's something we'd eventually like to add, but sort of a low priority for the core team at the moment while we try to clean up some other issues. We'd welcome any PR though if you're willing to give this a shot. @matthewmcgarvey recently did some work on primary key stuff, and may be able to assist or provide guidance.

[Whaxion]

Hello, I've started working on an implementation of composite primary keys, if you want to see my progression, you can check here https://github.com/Whaxion/avram/tree/composite-primary-keys :smile:

[matthewmcgarvey]

@Whaxion before you get too far on that, I'd really like for you to discuss your plans in here first. Looking at your branch, I see some things that we might not want to do in Avram.

The things I'd really like to know before diving into this

• how do other ORMs handle this? (Rails, Elixir/Ecto, statically typed orms like Diesel)
• What kind of API changes will this entail? (I assume UserQuery.find will take multiple args but shouldn't they be named to match the oclumns?)
• How can we break this down into small pieces?

Edit: Really excited to have you helping out with this! I'm always up for pair coding if you'd like to discuss some things

[jwoertink]

Really excited to have you helping out with this!

I'd like to second that @Whaxion! Super stoked to see someone taking this on. We know it's a pretty huge task.

[Whaxion]

@matthewmcgarvey To be honest, I haven't thought of looking at how other ORMs handle this, my bad. I'll do it just after posting this really long message. About the API changes, I'm trying to avoid breaking changes and currently, I haven't made any breaking changes.

So, now I'll explain what I've already made. Currently, the migrator primary_key in create_table support composite primary keys. It works like this

primary_key id1 : Int64, id2 : UUID

Because of the way I've done it, a non composite primary_key still works exactly like before (I almost haven't touched the old code) So primary_key id : Int64 still works exactly like before (the spec is full green) For the Model#primary_key it works the same so primary_key id1 : Int64, id2 : UUID and old code still works exactly like before. But I'm not totally satisfied about it. Because of the fact it's composite PK, it can be composed of one or multiple foreign key(s). So I want to implement it in a way that this works primary_key author : User, category : Category and it will do a PK named author_id with a reference to the user table. But I'll implement it in a way that doesn't change previous behavior (so it'll only work with composite because I don't see an interest for non-composite but if there is one I can also implement it for non-composite PK)

About the UserQuery.find, for non-composite PK, it'll not be changed. (No changes made to primary_key_queryable.cr) But, for composite PK, right now I've made a composite_primary_key_queryable.cr that implements a find that accept two way of finding a record.

UserQuery.find [123_i64, UUID.new("60604bb2-c8ed-49bb-a961-bec7891871c6")]
# and
UserQuery.find 123_i64, UUID.new("60604bb2-c8ed-49bb-a961-bec7891871c6")

But I don't really like it, so I plan to change the second find by

UserQuery.find id1: 123_i64, id2: UUID.new("60604bb2-c8ed-49bb-a961-bec7891871c6")

I've also already implemented insert, update and delete, but I need to further check if it works as intended. For theses, it works exactly like non-composite PK models.

I've also made specs for all of my features, so migration, initialize model, insert, update, delete

I still need to works on migrator to add add_belongs_to and on models for belongs_to, has_one, has_many

I've probably missed some functionalities and maybe I've implemented it in a way that doesn't match the way Avram works so I'm totally open to suggestions.

So if you have already done a TODO list for this functionality, I'd be really pleased to know it but if there is not, it's not a problem, I still have my own.

And it's also a pleasure to help Avram because it's the first time I help a project that is so big and I really enjoy working on this feature! (even if the code base is a bit overwhelming at start) :smile:

[matthewmcgarvey]

@Whaxion thanks for laying it out. And ya, the find implementation was what I was worried about.

I'm thinking that we could break this down and maybe you could submit a PR for just the migration portion and then just the model portion. I'm fine with being able to create a database table with a composite key and not being able to represent it in the model for a bit. And we would release the library with both changes anyways.

Since a composite primary key is a combination of existing columns, what do you think about the primary_key call in the migration not actually creating the columns

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

And all that does is create the text PRIMARY KEY(user_id, post_id). Might even call it composite_primary_key to have clear separation.

What do you think?

[Whaxion]

@matthewmcgarvey Thanks for the proposition, I think it's a great idea to split it in two PR.

Also, I find your idea of a composite_primary_key that only create PRIMARY KEY(user_id, post_id) beautiful, it's so simple but so effective.

I'll implement this composite_primary_key (I think it needs to be clearly separated from a non-composite), add a spec and then make a PR about the migration portion.

After that, I think I'll try to implement a similar composite_primary_key in the model portion. I'll tell here more once I've figured out exactly how I'll implement the model portion. Right now, all I know is that the find will probably be like

UserQuery.find id1: 123_i64, id2: UUID.new("60604bb2-c8ed-49bb-a961-bec7891871c6")

But I'll figure that out after making the easy migrator part

[Whaxion]

I've done the migration part, but I don't really like it. The reason is because, with add_belongs_to (or add), for Int64 it's a bigint. But if we use primary_key, it's a bigserial. So, maybe I should instead make it to works like this

def migrate
  create table_for(UserPost) do
    add_belongs_to user : User, on_delete: :cascade, primary_key: true
    add_belongs_to post : Post, on_delete: :cascade, primary_key: true
    # add quantity : Int64, primary_key: true
  end
end

If primary_key is set to true, it calls the primary key builder (without the PRIMARY KEY at the end) instead of the classic column builder. But if I do that, I'd also need to add references to primary key builders.

What do you think of that ?

Edit: Here is the code for the composite_primary_key method https://github.com/luckyframework/avram/compare/master...Whaxion:composite-pk-migration

[matthewmcgarvey]

@Whaxion "serial" in postgres is an incrementing number data type and we wouldn't want composite key columns to do that.

I don't see anything wrong with your composite primary key branch other than you might try adding a check to make sure there are at least two columns provided with a helpful error message.

[Whaxion]

@matthewmcgarvey Oh I didn't knew that, thanks for the info! I'll add this check (and erorr message) and then I'll do the PR.

[Whaxion]

I've thinked about the model part. This is what I've comed up. (we will assume that the migration is done correctly)

class User < BaseModel
    table do
        column name : String
    end
end
class SaveUser < User::SaveOperation
  permit_columns :name
end

class Post < BaseModel
    table do
        column title : String
        belongs_to author : User
    end
end
class SavePost < User::SaveOperation
  permit_columns :title, :author_id
end

class Comment < BaseModel
    skip_default_columns
    table do
        belongs_to author : User
        belongs_to post : Post
        composite_primary_key :author_id, :post_id
        column text : String
    end
end
class SaveComment < User::SaveOperation
  permit_columns :author_id, :post_id, :text
end
class CommentQuery < Comment::BaseQuery
end

user = SaveUser.create!(name: "Nicolas")
post = SavePost.create!(author_id: user.id, title: "hello, world!")
comment = SaveComment.create!(author_id: user.id, post_id: post.id, text: "nice")

new_comment = CommentQuery.new.find(author_id: user.id, post_id: post.id)
# or
new_comment = CommentQuery.new.find(user.id, post.id) # same order as composite_primary_key

What do you think of that ? Have I missed something ?

After that I'll still need to do something to make this works too

class Reaction < BaseModel
    skip_default_columns
    table do
        belongs_to author : User
        belongs_to comment : Comment
        composite_primary_key :author_id, :post_id, :comment_id # not sure about how people should write this, any ideas ?
        column type : String
    end
end

[matthewmcgarvey]

Here's some things that come to mind:

• composite primary keys should always be defined after the columns they use
• I think we could translate the declaration into a NamedTuple {author_id: Int32, post_id: UUID} by pulling from the COLUMNS list (consequentially how we would also check if the columns have already been defined)
• it would not be part of the DB.mapping setup (I don't think 😕)
• In the .find definitions, we could "splat" the NamedTuple to provide the parameters and types
• composite_primary_key should raise an error if primary_key has already been set (maybe they forgot to call skip_default_columns)

Definitely take a crack at it and see what you get! Let me know if there's anything I can do to help

[Whaxion]

I write this message to tell you that I still plan to implement this feature, it's just that right now, I've been really busy with other things. I'll try to get some time to work on composite PKs.

[notramo]

I need to use composite primary keys in a project, at least on the database level. Is it OK if I add a dummy integer column id which I set as primary_key in Avram, but in the Postgres migration I set the composite primary key on the 2 other columns with execute? I know that I wouldn't be able to use #find(), but that could be worked around.

[jwoertink]

I have no clue if that would work or not. I'd say give it a shot and see. Sounds like it would need a bit of some hacks until it gets added in.