enum traits do not use prefix

[tjbarker]

Description

When defining enums with a prefix or suffix the trait that is automatically generated does not take into account the prefix or suffix.

Reproduction Steps

class Foo
  enum bar: [:baz, :bing], _prefix: :bang 
end

FactoryBot.define do
  factory :foo do
    ...
  end
end

# in a test
create(:foo, :bang_baz)

Expected behavior

I would expect the trait to correspond to the predicate or bang methods defined with the enum. in the above examples an instance of Foo would have methods bang_baz? and bang_baz!.

Actual behavior

The traits are defined from only the enum values. So in the above example the traits would be used:

create(:foo, :baz)
create(:foo, :bing)

System configuration

factory_bot version: 6.1.0 rails version: 5.2.4.4 ruby version: 2.5.5

[rhannequin]

Hello, it looks like it is not due to FactoryBot's implementation but ActiveRecord's. FactoryBot retrieves enum values from its pluralized name, as defined in ActiveRecord.

ActiveRecord doesn't itself include suffix and prefix in the list of enum's values:

class Book < ApplicationRecord
  enum language: %i[english japanese], _prefix: :in
end

Book.languages # => {"english"=>0, "japanese"=>1}

So a solution would be to change ActiveRecord's enum definition but forcing to use prefix and suffix in the list of enum's values. But I don't think this is the way for this feature: _prefix and _suffix were added to avoid conflict between a model's column and an enum's value (https://github.com/rails/rails/issues/17511).

What do you think?

[agvald]

Hi, I recently ran into this problem and decided to take a look at it. I've taken a look at ActiveRecord and I don't know how I would go about creating a public api to inspect how the enums objects are constructed, but what i do now is meta programming...

I wonder if a PR with a somewhat dirty solution like i attached here would be accepted? Of course it needs some cleaning, and better error handling, and a couple of specs. Please let me know if this is interesting, I'll gladly take a look at it :)

In lib/factory_bot/enum.rb we can redefine the enum_values to be like this:

def enum_values(klass)
  return @values if @values
  # Fetching AcitveRecord::Enums

  enum = klass.send(@attribute_name.to_s.pluralize)
  # it will return here if there is no _prefix or _suffix on the enum.
  return enum if klass.respond_to?(enum.keys.first)

  enum_key = enum.keys.sample.to_s
  all_methods = klass.send(:methods)
  all_methods.select!{ |met| # find all the ones with a prefix or a suffix
    met.match?(/_#{enum_key}|#{enum_key}_/)
  }

  all_methods.select! { |met| # all the enum scopes have a matching `not_...`
    all_methods.include?("not_#{met}".to_sym)
  }

  if all_methods.size > 1
    # either register/print a warning, or raise "oopsie, I crashed."
    return {}
    # right now im a bit out of ideas ..., but I can improve this part.

    # terrible idea nr 1: compare `klass.method(met).source` with the following
    #          singleton_class.define_method(name) do |*args|
    #            scope = all._exec_scope(*args, &body)
    #            scope = scope.extending(extension) if extension
    #            scope
    #          end
  end
  prefix, suffix = all_methods.first.to_s.split("#{enum_key}", 2).map(&:presence)

  enum.transform_keys { |key| "#{prefix}#{key}#{suffix}" }
  # maybe we should do some checks here that all the methods we expect from a enum is actually existing? with `klass.new.respons_to?("#{prefix}#{key}#{suffix}!" )`
end

[composerinteralia]

I love that you are exploring this, but it seems difficult to reliably get the right method like this. For example:

all the enum scopes have a matching `not_

Not if the enum has the scopes: false option

There's also a ton of methods on ActiveRecord objects so this wouldn't necessarily be efficient.

There's a sort of ugly and probably brittle way to narrow down to the methods we care about:

relevant_methods = klass.ancestors.filter_map do |ancestor|
  if ancestor.class.name == "ActiveRecord::Enum::EnumMethods"
    ancestor.instance_methods(false)
  end
end.flatten

But I'm not sure we could merge that since it could change in Rails at any time.

[agvald]

Another idea, but this time through using a monkey_patch, this will at least be more stable.

Then we can lookup in ActiveRecordEnumRecorder.enums inside lib/factory_bot/enum.rb.

In my perspective, it would make sense if rails exposed something like this name in defined_enums, set here https://github.com/rails/rails/blob/a692e63bf4000cdeb9f230fb395c97edd3b12b99/activerecord/lib/active_record/enum.rb#L182 But they don't, maybe they are open to expanding that?

How do you like the monkey_patch? I don't have a standalone script for this, this script is using models from some rails application so I attached the relevant part of models here.

# class Car < ApplicationRecord
#   enum _prefix: "123", name: %i[derp_pimper killer_frank], seats: %i[four five], _suffix: "asd"
# end

# class Role < ApplicationRecord
#   string_enum :resource, %i[valida unit global], _prefix: "hasdawd", _suffix: "dash"
#   string_enum :name, %i[sjappa blings dings], _prefix: "bar"
# end

class ActiveRecordEnumRecorder
  class << self
    def add(klass, attribute, name)
      @enums ||= {}
      @enums[klass] ||= {}
      @enums[klass][attribute] = name
    end

    def enums
      @enums
    end
  end
end

  module ActiveRecord
    module Enum
      alias original_enum enum
      def enum(*args, **kwargs)
        kwargs
          .except(:_prefix, :_suffix, :_scopes, :_default)
          .each do |attribute_name, values|
            ActiveRecordEnumRecorder.add(self.to_s, attribute_name, [kwargs[:_prefix], attribute_name, kwargs[:_suffix]].compact.join("_"))
          end
        original_enum(*args, **kwargs)
      end
    end
  end


puts Role.last
puts Car.last

pp ActiveRecordEnumRecorder.enums
# => {
#   "Role"=>{:resource=>"hasdawd_resource_dash", :name=>"bar_name"},
#   "Car"=>{:name=>"123_name_asd", :seats=>"123_seats_asd"}
# }
pp Car.names
# => {"derp_pimper"=>0, "killer_frank"=>1}
pp Role.resources
# => {"valida"=>"valida", "unit"=>"unit", "global"=>"global"}
pp Role.names
# => {"sjappa"=>"sjappa", "blings"=>"blings", "dings"=>"dings"}

[agvald]

@composerinteralia, WDYT about the monkey patch solution?

[composerinteralia]

Similar feelings to the other solutions so far. It's clever, but worries me a bit. Patching code in tests only can introduce subtle differences between tests and production. I'm also not sure we can guarantee that patch loads before any models with enums are loaded.

I poked around in Rails to see if there's an obvious way to expose the enum method names, but I'm finding it hard to justify the change beyond my wanting it for factory_bot.

[tjbarker]

Hi sorry I've dropped out of this for a while, I think my initial thinking on this was to not touch active record (or rely on anything within it) as that just feels like a recipe for things breaking in the future.

I think i was imagining (at least for now) a "hook" that could be implemented within the factory to alert it to the prefix or suffix. This of course would end up with having to explicitly define the enum in the factory again (of which the goal was to not have to do) but given that this is a special situation, i don't think a nice factory api that handles it would a be big deal.

I was thinking maybe something like:

FactoryBot.define do
  factory :foo do
     enum bar:,  _prefix: :bang 
  end
end

This would allow the factory to know to find the enum list, and to know the prefix to apply. I would also think it would be nice if it then wouldn't define the original enum traits, but would only define the ones with prefixes. I think that the added maintenance of having to have the prefix in 2 places would be ok given that this code would now be completely within the scope of factory_bot itself.

[composerinteralia]

That approach makes sense to me. I think we could add prefix and suffix keyword arguments to the existing traits_for_enum method (see the second half of the enum traits docs) without too much trouble.

[tjbarker]

https://github.com/thoughtbot/factory_bot/pull/1513

this is a go at adding it to traits_for_enum

[agvald]

Hey, I think the approach in #1513, making users of factorybot define the enum in the factory as well as in the model, is sort of like, we don't want to write dirty code in our project so y'all have to do dirty code in your projects(duplicate your code).

#1513 makes perfect sense as an improvement if you are using factory bot without active_record. But I dislike this as a solution for working with active_record :(
Also, I firmly believe it will be as hard to maintain this interface, keeping it up with changes to active_record's interface(Which i believe would be the most reasonable to do?), as maintaining the monkey patch i suggested.

Question, if someone implements an api to extract _suffix and _prefix for enums in active_record, would you still like to merge #1513?

I belive rails-team will be open for having an API exposing this for their enums, I can make a proposal to implement said API and post it on the rails project for the rails-team to look at it around 10th of October.

[composerinteralia]

if someone implements an api to extract _suffix and _prefix for enums in active_record, would you still like to merge #1513

I think #1513 is still worthwhile even if we do find a way to make automatic trait generation take the prefix and suffix into consideration. Some folks might have automatic trait generation turned off but still want to add enum traits manually via traits_for_enum, or as you said there may also be use cases outside of Active Record enums.

we don't want to write dirty code in our project

It's not really that I don't want dirty code in factory_bot (I'm sure there's plenty of it!), it's that I want to make sure the library is stable. Monkey patching or otherwise using Rails internal APIs means that Rails can change those at any time without warning, and a minor Rails bump could suddenly cause the test suite to fail. That's not a tradeoff I'm willing to make.

it will be as hard to maintain this interface

The interface in #1513 is related to, but separate from Rails. It doesn't use any additional Rails APIs, so it wouldn't necessarily need to change if Rails changes. If Rails completely changes the way enums work, maybe we'd reconsider our interface, but that seems unlikely and we'd at least get some warning via deprecations and major releases.

I can make a proposal to implement said API

That would be great! If Rails exposes the prefix and suffix, we'll use them! I looked into this, but didn't see a way of doing it beyond storing extra information that might only be useful to factory_bot.