Use atoms for enums

[pguillory]

Currently Enum values are represented by integers in Elixir.

enum Color { RED = 1, GREEN = 2, BLUE = 3 }

struct Pixel {
  1: optional Color color
}

require Color
assert Color.RED == 1
pixel = %Pixel{color: Color.RED}

# Is equivalent to:
pixel = %Pixel{color: 1}

case pixel.color do
  Color.RED -> :ok
end

It seems like it would be nicer if enums were represented by atoms.

pixel = %Pixel{color: :RED}

case pixel.color do
  :RED -> :ok
end

Pros

1. If you inspect/print a value, it's confusing to see a number and have to go find the thrift definition and map the number back into a name that you can reason about. An atom is directly readable.

> IO.inspect user
%User{status: 7}           # what is that?

...versus...

> IO.inspect user
%User{status: :INACTIVE}   # OK makes sense

2. Less generated code, faster compile times.

3. You don't have to require a generated enum module to use the values.

4. It's more Elixir-y, I think.

Cons

1. No longer catches invalid enum values. However it didn't catch enums being used in the wrong context, so we need Dialyzer annotations either way for that.

[fishcakez]

It's more Elixir-y, I think.

Definitely more idiomatic for Elixir.

[dantswain]

Sorry I missed this. This would definitely break production code for me :(

The value sent over the wire is always going to be an integer. In a lot of cases we do something along the lines of

• Grab user_type_id from database
• Stuff user_type_id into thrift struct
• Send it over the wire

With the values being integers, if we trust the database (in our case, we do), we never have to do anything other than copy the value directly into the thrift struct.

If nothing else, this would definitely be a breaking change for some people.

[dantswain]

I've honestly gone back and forth on this a few times. Even before using thrift, and before we started using Ecto (:older_man:), I would have situations where I pulled something from a database that was essentially an enumerated field. I've tried this both ways a few times. Thrash actually had enums as atoms and I was skeptical when you guys implemented them as integers at first, but after the initial migration I've started to actually prefer it that way.

Some observations, which I present in the humblest way possible:

• Fundamentally, I think enums are "enumerated values". From that standpoint, the atoms we're using are the names of the values, not the values themselves.
• In other languages, I believe they treat them as integers. It's sort of based around the way that enums work in C/C++.
• The debugging case could potentially be addressed by creating an inspect implementation so that we display the atom on screen.
• Using atoms means you need to introduce a layer to do the translation between integer and atom.
• Consider the pass-through case - service A -> service B -> service C. If service B doesn't care about the business logic related to the enum names, it's going to end up doing a translation that it wouldn't otherwise need to do.
• Related to the pass-through case, the conversion layer adds a maintenance burden. If we add new values to the enum, we need to upgrade service B so that it knows how to do the translation. I think this is sort of un-thrift-y. If they are integer values, service B doesn't care. We take advantage of this in our code a fair amount.

Hopefully this doesn't seem confrontational or objection-y - it's just a topic I've already put a fair amount of thought into, and I'm fairly overloaded so I'm trying to brain dump :/ Totally willing to discuss, and ultimately, though I really appreciate you guys including me as a contributor, it's you guys doing the lion's share of the work so I won't put up too much of a fight.

[jparise]

One bit of prior art: https://github.com/gjaldon/ecto_enum

[jparise]

I think it's particularly helpful to keep in mind that, in Thrift, enumerated values are always integers (i32), as they are in C. We don't need to design a solution that allows for other value types.

[fishcakez]

One bit of prior art: https://github.com/gjaldon/ecto_enum

This only supports PostgreSQL, and that database exposes them as strings over the wire to user's applications. So when selecting an enum column the value would be a string and no number attached. I say number because postgreSQL uses float32 IIRC, for ordering and uniqueness.

[dantswain]

To clarify - I think a lot of the issues I've had in the past related to this kind of thing stem from the difference in type between the name and the value, not necessarily the values having different types (they're almost always integers). My previous comment enumerates some specifics, but I think it mostly stems from that fundamental difference. The values are integers, the act of enumerating them is to give them names. When you try to treat the values as their names, things seem to get funky.

[fishcakez]

The debugging case could potentially be addressed by creating an inspect implementation so that we display the atom on screen.

It's nice to keep the inspect pattern so that it easy to copy and paste. We could support inspecting with atoms but then we may want to support serializing from atoms as well as integers but always deserialize to integers (assuming we kept existing behavior).

Using atoms means you need to introduce a layer to do the translation between integer and atom.

Consider the pass-through case - service A -> service B -> service C. If service B doesn't care about the business logic related to the enum names, it's going to end up doing a translation that it wouldn't otherwise need to do.

Thats true. Note that the consecutive integers -> atoms is unbelievable fast in the VM, and atoms -> integers is also very fast but not quite as fast. The memory usage is the same and both are immediate values. The difference should be small enough that we would pick the better behavior in terms of usability:

Related to the pass-through case, the conversion layer adds a maintenance burden. If we add new values to the enum, we need to upgrade service B so that it knows how to do the translation. I think this is sort of un-thrift-y. If they are integer values, service B doesn't care. We take advantage of this in our code a fair amount.

This suggests that we would want to deserialize unknown integers to integers, and allow serializing integers to integers. Similar to above. I am unsure how I feel about either way.

It would be quite easy to write a version of ecto_enum that uses the thrift enum values and that gets recompiled when enums change.

The integer approach has a pro and a con: the calls of the macros get recompiled when the enum changes. Whereas atoms won't trigger a recompile. In new versions of Elixir we should have user code get recompiled if the struct keys change.

[fishcakez]

The values are integers, the act of enumerating them is to give them names. When you try to treat the values as their names, things seem to get funky.

I view this the other way around, an enum is a set of identifiers that happen to get serialized as integers. From a users perspective the set of identifiers is more useful and powerful.

[dantswain]

Another "fundamental" concern to me: Backwards and forwards compatibility is a core concern in thrift. The protocol and existing clients allow for the contents of a message to change. I think the implementation around enums should comply. If an enum value is added upstream, I don't think client applications should start crashing because they don't know how to convert the enum to its name.

[fishcakez]

Another "fundamental" concern to me: Backwards and forwards compatibility is a core concern in thrift. The protocol and existing clients allow for the contents of a message to change. I think the implementation around enums should comply. If an enum value is added upstream, I don't think client applications should start crashing because they don't know how to convert the enum to its name.

Is this covered by:

This suggests that we would want to deserialize unknown integers to integers, and allow serializing integers to integers.

Or do you have another concern?

[dantswain]

No I think that's kind of it. Nothing in the thrift spec says that a client is required to validate the values of an enumerated field. That may or may not be intentional, but I think it jives with forward/backward compatibility.