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

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.

About this issue

  • Original URL
  • State: closed
  • Created 8 years ago
  • Reactions: 26
  • Comments: 15

Most upvoted comments

@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

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.

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. 😃

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

@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.

(@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.