Custom Primary Key support

[jwoertink]

Ref: https://gitter.im/luckyframework/Lobby?at=5cfedb153dcdab400324037a

Related:
#6
#21
#87
#89

The current support for primary keys in Avram is limited to UUID UUID and SERIAL Int32. Other options requested are:

• - Int32 (id SERIAL)
• - UUID (id UUID)
• - Int64 (id BIGSERIAL) Customize defaults and primary key #121
• - String (id character varying(18) NOT NULL) or something like (KSUID) Feature request: Add support for prefixed KSUID primary_key_type #21
• - Composite (PRIMARY KEY (id_1, id_2)) Add support for composite primary keys #129
• - Int16 (id SMALLSERIAL) Add Int16 small serial primary key #130
• - no primary key (table with no id field.)
• - renaming the primary_key column to something else.

Since several people request this, and there's already quite a few issues open about it, I wanted to put them all in to 1 location to track. If you have a need for a custom primary key, let us know what your use case is, and if the current support is blocking or not for you. This helps us to prioritize and/or gauge what is wanted.

[paulcsmith]

Great idea putting this all in one spot 👍

[watzon]

It would be really nice to have the no option and then have a primary option on the add method. For instance:

class CreateUsers::V20190610161436 < Avram::Migrator::Migration::V1
  def migrate
    create :users, :none do
      add id : Int64, primary: true
    end
  end

  def rollback
    drop :users
  end
end

Mainly I want it to not automatically generate id's, but still allow the creation of a primary key without using a custom SQL query.

[jwoertink]

Adding in my own personal use cases:

I have an app that needed to migrate data from an old database in mysql with int32, to a new postgres app. We had to differentiate between the two by using id character varying(18) NOT NULL for all of our new records, and then had a secondary column that stored the Int for the old DB. KSUID would have worked better had we known what that was at the time.

Some of the join tables in this rails app have no primary key at all. So the current setup throws an error when trying to fetch an id column that doesn't exist. We could just add an id column, but a SQL dump of this DB is 62GB, and this would just increase the size for no reason.

So supporting custom String, and no id are both options I would like to see.

[paulcsmith]

Also, I want to make it so the primary key type is WAY more extendable. I think the primary key should be just like any other column but be included in the generator. In other words, no default primary key or timestamps, instead add the defaults in the generated code. primary_key id : Int64or primary_key id : UUID. Then people can remove it or write extensions w/o needing to make a PR to Avram

[jwoertink]

I actually love that idea @paulcsmith.

Doing this would just about support the whole list!

create :users do
  primary_key id : Int64
  add name : String 
end

[watzon]

I love that idea too. Having a default is a very ActiveRecord thing. It's nice for vanilla db setups, but makes you dig if you want to do anything extra.

[paulcsmith]

@watzon Yeah I like having a default so you have something to get going with, but like you said, it is hard to customize. Doing it in code with the generators still gives us a nice default, but also makes things discoverable. I think it should work well!

[paulcsmith]

Ok so I think there are a couple potential issues with this approach

Potential issues

• Having to type/remember more stuff
• Having to read the same thing in every model (primary key, timestamps
• A fairly large portion of users do not use the generators in Lucky or in other frameworks

Solving it

I like the idea of making this explicit instead of having a million config options, but I don't love typing the same stuff in every model.

So what if we add a macro to the generated BaseModel

abstract class BaseModel < Avram::Model
  macro default_columns
    primary_key id : UUID
    timestamps # sets up created_at/updated_at and callbacks to set them on create/update
  end
end

Then in the generators (or if people type by hand)

class User < BaseModel
  table do
    default_columns
    # Other columns
  end
end

This is shorter, easier to remember, easy to customize.

Thoughts? cc @edwardloveall @HarrisonB

[jwoertink]

First thing is, did you leave off the :users definition, or are you thinking just have that pulled in automagically? Because having the table name inferred would be pretty clean! Of course we'd need to allow overriding, but that's nice.

I'd be ok with this solution too. I have a project using UUID, and it's already a pain redefining primary_key_type: :uuid on every model. This would clean that up quite a bit. Plus it would allow removing those for tables that don't need it.

[paulcsmith]

Actually you can leave off the table name right now. But of course it is not documented :)

Glad you like it. I was thinking the same. It is more flexible. Still easy to use and not too much typing. At least that’s my hope haha

[paulcsmith]

Also I think it’s good we don’t document that yet until we have some kind of table name inference in migrations too. Like create table_for(User) do/end

[watzon]

Wait a second.. You can leave the table name out? I was definitely going to be suggesting that, so that's great news!

[edwardloveall]

I like this. While reading through I through about a bunch of different ways this could be less than ideal.

• The idea of forgetting this on a table seems easy to do without generators as was mentioned.
• It's not what people are used to from ActiveRecord so there's a higher burden for migration (har).
• I could see people making a mess of multiple "base" models.

I think what Paul came up with has the right tradeoffs. It's easy to remember (most people probably won't even know what Avram::Model is) and easy to customize.

[paulcsmith]

Excellent points @edwardloveall. I think you're right and I think what I proposed might be something that people can still forget so I think we can add just one more step to make this extra easy, even when not using generators.

Maybe when calling the table macro it automatically calls the default_columns macro. Then you can override the default_columns macro to be whatever you want or use skip_default_columns on certain models if you want to skip. I think this way it is super flexible, and you don't need to remember to add default_columns every time

[jwoertink]

That's a pretty good compromise. Plus we will document this to make sure it's easy to understand.

[lesderid]

Does a97c2b7 or a previous commit allow for composite keys (see original post of this issue)? If not, this issue shouldn't be closed yet.

[paulcsmith]

No it doesn't. I'll open separate issues for those other items so we don't have one big one that is hard to track. Thanks for calling that out

[paulcsmith]

#129 this will require a lot more work since it will require accepting multiple values for a lot of methods. Update, find, delete, and lots of internal methods so might take longer to finish

[lesderid]

No worries, just wanted to have an issue to track.