Shoulda-matchers: PR #829 breaks "define_enum_for" matcher for enums that do not actually use integer columns

[begedin]

Say we want a status enum that looks like

enum status: { active: "active", pending: "pending", rejected: "rejected" }

With the changes made in PR #829, we can no longer spec for that enum with

it { should define_enum_for(:status).with(active: "active", pending: "pending", rejected: "rejected")

Are active record enums with strings as underlying column types not allowed? I couldn't find any information that would explicitly make it so.

I believe the matcher should only enforce integer type if the provided expected enum value is an array, or if it's a hash with values all being integer.

[guilhermearaujo]

I agree. There is absolutely no reason to enforce integers.

It started with #827, but in the author's example, he is storing an :integer in a column of type :string, and his enum maps the keys to integers. If he wanted to use integers, he could just use an array of symbols.

Had he done that, than yes, the matcher could enforce integer type in case of array, or enforce nothing otherwise.

[lucasgomide]

cc: @mcmire

can you review this ?

[mcmire]

Sorry for the lack of reply on this, I've been out.

I agree that this is a bug. I'm not sure why we had the matcher assume integers except that that was the original use case.

I can't say when I'll get to this, but I've at least seen it. :)

[tiii]

I don't think this is a bug. Using strings as keys is not the way to go with ActiveRecord::Enum. There is no mention about using something else than integer columns and integer values in the official documentation.

4.1.8: http://apidock.com/rails/ActiveRecord/Enum
5.0.0: http://api.rubyonrails.org/classes/ActiveRecord/Enum.html

If you use a string type column and non string enum values (integers or symbols) you lose the ability to retrieve the key by calling the getter method on your model (shown here https://github.com/thoughtbot/shoulda-matchers/issues/827#issuecomment-152551748).

In my opinion rails should enforce the use of an integer column, but as long as rails does not I don't mind that should-matchers enforces it.

If we still want to allow strings as keys we should check the enum values too:
Integer column: only allow integer enum values
string column: only allow string enum values

[guilhermearaujo]

(@tiii I suppose that by 'key' in your message you mean 'value', as in the value held by the key being a string, and not the key itself. Is that correct?)

I have to disagree. If your database has a varchar column to store your enum data and your enum maps a symbol to an integer, IMHO you are doing it wrong.
On the other hand, if your column is of type integer and your enum maps a symbol to a string, you are doing it wrong too.

In my opinion, if you have:

enum status: { active: 'active', pending: 'pending', rejected: 'rejected' }

you should create a column of type varchar;

Accordingly, if you have:

enum status: { active: 0, pending: 1, rejected: 2 } # or
enum status: [:active, :pending, :rejected]

then your column should be of type integer. As simple as that.

Should/can Rails be smart enough to make the required type conversions? Possibly yes.
Should all enums have integer values? This is debatable.
Is it up to shoulda-matchers decide whether they should be integer or string? My opinion is: definitely not.

[tiii]

@guilhermearaujo yeah, that's what i meant, sorry for that.

Going through the commit history for the enum module, i finally found a test for strings as enum values. (https://github.com/rails/rails/commit/67c1719012506c3387df067961252b5df50a97ce)

Since there was never any mention about using string as values in the documentation or code I assumed that the behaviour of using strings for values is rather a coincidence.

Therefore I agree, that shoulda should not decide if the values used for enums should be integer or string.

[foucist]

Just chiming in, we're using a legacy database with a variety of enumerable-like columns based on varchar strings rather than integers. It works fine in rails 4.2 & 5, and I was successfully using define_enum_for with shoulda-matcher 2.8.0, but it seems it's been broken since then.

[ZeroInputCtrl]

with what @foucist said, we have a similar issue where when we created the database we intended a field to be a string but instead decided to use it as an enum. This has worked til recently. Luckily we have a small database so it won't require any downtime to run a migration for it but I can see the need to still support this kind of functionality.

[valscion]

Seems like https://github.com/thoughtbot/shoulda-matchers/pull/1063 would fix this second-most :+1:-reactioned bug issue. Would there be any chance in getting it in at some point? Anything I could do to help with it?

[mcmire]

@valscion Yes! Sorry this took me so long. A fix for this is now in the master branch. To address @begedin's original problem, you'd be able to fix this by saying:

it do
  should define_enum_for(:status).
    with(active: "active", pending: "pending", rejected: "rejected").
    backed_by_column_of_type(:string)
end

[valscion]

Thank you! It's understandable that sometimes it takes a while to get time for open source projects, thank you for your work here!

[rafaeldev]

The method backed_by_column_of_type was refactored? I try use it but an error is raised:
undefined method 'backed_by_column_of_type' for #<Shoulda::Matchers::ActiveRecord::DefineEnumForMatcher:xxx>

[mcmire]

@rafaeldev What version are you using?

[rafaeldev]

Hi! Sorry for late... The versions:

• shoulda-matchers (3.1.3)
• rails (5.2.2)

The spec example:
should define_enum_for(:kind).with(a: 0, b: 1, c: 2).backed_by_column_of_type(:string)

[mcmire]

@rafaeldev The backed_by_column_of_type qualifier is available in 4.0.0! Upgrade to get it.