luckyframework / avram

A Crystal database wrapper for reading, writing, and migrating Postgres databases.
https://luckyframework.github.io/avram/
MIT License
165 stars 64 forks source link

Consider a new interface for Enums #697

Open jwoertink opened 3 years ago

jwoertink commented 3 years ago

The ability to use enums in your models is pretty nice, but the current interface did require a small bit of hacking around some of the type restrictions. This has led to using enums being a bit difficult.

Here's the current interface:

class GameEvent < BaseModel
  avram_enum Type do
    Connected
    Started
    Ended
    Disconnected
  end
  table do
    column type : GameEvent::Type
  end
end

SaveGameEvent.create!(type: GameEvent::Type.new(:connected))
GameEventQuery.new.type(GameEvent::Type.new(:connected).value)

Here's a few of the issues I've found with this setup:

  1. The migration requires you to know to create your column type as the Integer. add type : Int32, default: 0
  2. The model column definition requires the model namespace due to macro stuff. column type : Type would fail (probably with a not so pretty error)
  3. Saving the enum value requires instantiating this magical class, but querying requires the raw int value
  4. If you have a lot of enum members (say 26 like I have in my app), then running SQL and seeing 17 means nothing. (This is more of a preference deal, it may just be a "you're using enums, deal with it" type situation)
  5. Getting the enum member is a bit error prone. If your enum member is multi word, you define it like ControllerActivated, but to get the value, you either need to use "ControllerActivated" or :controller_activated. If you use :controlleractivated, then you get a pretty bad error that doesn't tell you why. The same goes for if you did :contoller_activated (misspelled controller).

Along with 5 https://cdn.discordapp.com/attachments/743896265057632259/862356608227344427/unknown.png You'll see here it says you can't pass a Symbol. That's because Crystal autocasts Symbol to enum when it matches, so in that case :super_admin is treated more like an enum as where :superadmin is treated like Symbol. If you're new to Crystal than this is super confusing.

As for how we solve this, I have no clue... Postgres does support enums directly https://www.postgresql.org/docs/10/datatype-enum.html If we decided to go that route, it would solve 4, but that also means that anytime you wanted to add a new enum member, you'd always have to generate a migration. Adding a new one to your model without the migration would cause issues. Maybe that could be caught by the SchemaEnforcer?

Another option could be to maybe have an alternate column macro... So it could look like this:

class GameEvent < BaseModel
  enum Type
    Connected
    Started
    Ended
    Disconnected
  end
  table do
    enum_column type : Type = Type::Connected
  end
end

SaveGameEvent.create!(type: GameEvent::Type::Connected)
GameEventQuery.new.type(GameEvent::Type::Connected)

In this case, we couldn't call the column macro "enum" due to conflict with the actual enum keyword. We'd also forgo the ability to use Symbols or Strings because having a typo with the constants would allow for better error messages.

I have no clue if this would even work, or how the code would look, but I wanted to get the conversation rolling and allow others to chime in with some thoughts on this.

jaitaiwan commented 3 years ago

I wonder if there’s a serialiser that you can have for Types so you can reference the type directly in the migration and have the serialiser determine the actual DB type. Then if you have Postgres features like enum turned on, you could try a dedicated serialiser function before the main serialiser so that Postgres enums were returned instead.

matthewmcgarvey commented 3 years ago

With https://github.com/luckyframework/avram/pull/698 merged, some of the points are no longer an issue.

I think the main one, though, is supporting different database values. Specifically, support for postgres enums and enums mapped to strings would solve a bunch of problems.

jwoertink commented 3 years ago

That would be baller if we had a way to use normal Crystal enums, and then map them to postgres enums. After that PR was merged, it's probably not crazy to add in, but definitely something we can look at doing later.

jubishop commented 2 years ago

FYI I'm working on a stab at this problem.

jwoertink commented 7 months ago

I have nothing to prove this, but I think if https://github.com/luckyframework/avram/pull/1009 gets merged in, a custom converter could be used here to convert the postgres enum strings up in to regular enums... Or maybe it makes adding that option easier?

robacarp commented 7 months ago

For what it's worth, my experience with postgres enums has told me that I would not recommend using them in all but a very few circumstances. It's worth mentioning here these two points which made it painful:

All that said, I'd sure sure like to have string backed enums to make my database queries easier to read!