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

Consider a new interface for Enums #697

Open
jwoertink opened this issue Jul 7, 2021 · 6 comments
Open

Consider a new interface for Enums #697

jwoertink opened this issue Jul 7, 2021 · 6 comments

Comments

@jwoertink
Copy link
Member

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
Copy link
Contributor

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
Copy link
Member

With #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
Copy link
Member Author

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
Copy link

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

@jwoertink
Copy link
Member Author

I have nothing to prove this, but I think if #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
Copy link
Contributor

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:

  • Enums in postgres are database wide, not table scoped. This means that if you have a Role enum on Users, you have to name it UsersRole rather than just Role or your risk colliding with a Role enum on some other table.
  • It is not possible to remove a value from an enum after it is created. This has led to some hijinks in the elixir world to get this to work, such as creating a new enum without the value, re-typing all the columns that follow that enum, deleting the old enum, and renaming the enum back into place 🥴

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

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

No branches or pull requests

5 participants