rails icon indicating copy to clipboard operation
rails copied to clipboard

Enum conflict error message is misleading

Open gabo-cs opened this issue 2 years ago • 12 comments

The ArgumentError message being raised when some enums conflicts occur is sometimes misleading.

Steps to reproduce

Add the following to any model:

enum foo: { active: 0, inactive: 1 }
enum bar: { on: 0, off: 1, inactive: 2 }

or simply add a brand new enum with any value that already exists in a previos enum inside the same model.

Expected behavior

The error message should be:

You tried to define an enum named "bar" on the model "User", but this will generate an instance method "inactive?", which is already defined by another enum. (ArgumentError)

  • Notice the "an" determiner, that should probably be fixed too, it's currently a %{type} but type can be instance, for instance (no pun intended).

Actual behavior

The error message is:

You tried to define an enum named "foo" on the model "User", but this will generate a instance method "active?", which is already defined by another enum. (ArgumentError)

It seems to be taking enum_name, and method_name as the first value of the first enum, instead of displaying the ones that are actually causing the issue, in the repro steps example, foo is defined first so the problem should be with bar, and active is clearly not the conflictive enum value.

System configuration

Rails version: Rails 6.1.3.1

Ruby version: Ruby 2.7.2

gabo-cs avatar Apr 05 '22 01:04 gabo-cs

Hey @gabo-cs, I tried reproducing the error you are seeing but I was unable to (on 6.1, 7.0, or main):

# frozen_string_literal: true

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"

  git_source(:github) { |repo| "https://github.com/#{repo}.git" }

  gem "rails", "~> 6.1.0"
  gem "sqlite3"
end

require "active_record"
require "minitest/autorun"
require "logger"

# This connection will do for database-independent bug reports.
ActiveRecord::Base.establish_connection(adapter: "sqlite3", database: ":memory:")
ActiveRecord::Base.logger = Logger.new(STDOUT)

ActiveRecord::Schema.define do
  create_table :posts, force: true do |t|
  end
end

class Post < ActiveRecord::Base
end

class BugTest < Minitest::Test
  def test_association_stuff
    error = assert_raises(ArgumentError) do
      Post.class_eval do
        enum foo: { active: 0, inactive: 1 }
        enum bar: { on: 0, off: 1, inactive: 2 }
      end
    end

    assert_includes error.message, '"bar"'
    assert_includes error.message, '"inactive?"'
  end
end

Could you try making a script like this that could reproduce the error?

skipkayhil avatar Apr 05 '22 03:04 skipkayhil

Hey @gabo-cs, I tried reproducing the error you are seeing but I was unable to (on 6.1, 7.0, or main):

# frozen_string_literal: true

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"

  git_source(:github) { |repo| "https://github.com/#{repo}.git" }

  gem "rails", "~> 6.1.0"
  gem "sqlite3"
end

require "active_record"
require "minitest/autorun"
require "logger"

# This connection will do for database-independent bug reports.
ActiveRecord::Base.establish_connection(adapter: "sqlite3", database: ":memory:")
ActiveRecord::Base.logger = Logger.new(STDOUT)

ActiveRecord::Schema.define do
  create_table :posts, force: true do |t|
  end
end

class Post < ActiveRecord::Base
end

class BugTest < Minitest::Test
  def test_association_stuff
    error = assert_raises(ArgumentError) do
      Post.class_eval do
        enum foo: { active: 0, inactive: 1 }
        enum bar: { on: 0, off: 1, inactive: 2 }
      end
    end

    assert_includes error.message, '"bar"'
    assert_includes error.message, '"inactive?"'
  end
end

Could you try making a script like this that could reproduce the error?

Hmmm, that's curious! Can you do it on a model and just do like a rails c or something and see what error you get? Doing it on this small tests gives me the expected result, but doing it on a rails model (brand new, existing, fat, skinny) gives me the wrong message, looking into what that might be due to.

gabo-cs avatar Apr 05 '22 03:04 gabo-cs

@gabo-cs we need a bug report script or trivial app that reproduces the problem in order to help. Otherwise it is likely other gems may be causing trouble on your app.

gmcgibbon avatar Apr 05 '22 22:04 gmcgibbon

@gmcgibbon You're right, it looks like something else is causing this, however, I do think it's because of something rails-related and not something external, but I'll try to look into it and confirm. In the meantime, would you mind taking a look at this?

image

I did the same thing (Post model with those two enums) on a brand new rails project, and check out what happens: the first time the error is fine but the second time and on, it isn't (This is in rails console). I'll try to put this together on a test script but wanted to first give it a whirl on a real rails app to double-check that there's indeed something "rails-y" going on, my gut feeling tells me this has to do with _enum_methods_module on this validation.

Thanks a lot for the help.

gabo-cs avatar Apr 06 '22 20:04 gabo-cs

Thanks for making the repo, I'm seeing the same error that you are now! I believe this is due to a weird edge case where Post is throwing an error as the class is loading, and because of this it ends up in a partially loaded state.

Reproduction (mostly the same as your repo):

rails new testing -J
cd testing
bin/rails g model Post
bin/rails g model Comment

modify post.rb:

class Post < ApplicationRecord
  enum foo: { active: 0, inactive: 1 }
  enum bar: { on: 0, off: 1, inactive: 2 }
end
bin/rails c
Loading development environment (Rails 7.0.2.3)
irb(main):001:0> Rails.autoloaders.main.log!
=> #<Proc:0x00007fe03d3e1bb8 /home/hartley/.cache/asdf/installs/ruby/3.1.1/lib/ruby/gems/3.1.0/gems/zeitwerk-2.5.4/lib/zeitwerk/loader/config.rb:265 (lambda)>
irb(main):002:0> Rails.autoloaders.main.to_unload
=> {}
irb(main):003:0> Post
[email protected]: constant ApplicationRecord loaded from file /home/hartley/test/rails44842/testing/app/models/application_record.rb
/home/hartley/.cache/asdf/installs/ruby/3.1.1/lib/ruby/gems/3.1.0/gems/activerecord-7.0.2.3/lib/active_record/enum.rb:301:in `raise_conflict_error': You tried to define an enum named "bar" on the model "Post", but this will generate a instance method "inactive?", which is already defined by another enum. (ArgumentError)
irb(main):004:0> Rails.autoloaders.main.to_unload
=>
{"ApplicationRecord"=>
  ["/home/hartley/test/rails44842/testing/app/models/application_record.rb", [Object, :ApplicationRecord]]}
irb(main):005:0> Comment
[email protected]: constant Comment loaded from file /home/hartley/test/rails44842/testing/app/models/comment.rb
=> Comment (call 'Comment.connection' to establish a connection)
irb(main):006:0> Rails.autoloaders.main.to_unload
=>
{"ApplicationRecord"=>
  ["/home/hartley/test/rails44842/testing/app/models/application_record.rb", [Object, :ApplicationRecord]],
 "Comment"=>["/home/hartley/test/rails44842/testing/app/models/comment.rb", [Object, :Comment]]}
irb(main):007:0> Post
/home/hartley/.cache/asdf/installs/ruby/3.1.1/lib/ruby/gems/3.1.0/gems/activerecord-7.0.2.3/lib/active_record/enum.rb:301:in `raise_conflict_error': You tried to define an enum named "foo" on the model "Post", but this will generate a instance method "active?", which is already defined by another enum. (ArgumentError)
irb(main):008:0>

What I'm noticing is that successfully loaded classes (ApplicationRecord and Comment) get added to the to_unload hash, but the unsuccessfully/partially loaded class (Post) does not. So I think what is happening is this:

  • The first time Post autoloads, the foo enum gets created, the bar enum throws an error
  • The second time Post tries to autoload again, it doesn't unload the previous definition, so the foo enum conflicts with the existing foo enum

I'm not sure if this is expected behavior or not since failing to autoload a class the first time does give the correct error.

cc @fxn because this is getting a little over my head 😄

skipkayhil avatar Apr 06 '22 21:04 skipkayhil

Nice! Glad you could reproduce it! Yeah that seems indeed related to the classes load, what's weird is that in another project I always get the wrong error message the first time/first autoload on all models, I haven't been able to identify what that's because of yet.

gabo-cs avatar Apr 06 '22 21:04 gabo-cs

Unfortunately this is how Module#autoload works in this edge case.

Let me explain with this snippet:

require 'tempfile'

Tempfile.create(['foo', '.rb']) do |file|
  file.write(<<~RUBY)
    class C
      p object_id
      raise
    end
  RUBY
  file.close

  autoload :C, file.path

  2.times do
    begin
      p !!Object.autoload?(:C)
      C
    rescue
      p :raised
    end
  end
end

That prints

true
60
:raised
true
60
:raised

As you see, if the file raises in the middle of the body, the autoload is preserved, but subsequent autoloads get the same class object, as if they were really reopening. That explains that if you ignore the exception, you'll double down on side-effects of the bad state.

Reloading reloads Post. The solution in this case is to fix the cause of the exception. File edition triggers reload, and all will be good when the enums are revised.

fxn avatar Apr 06 '22 21:04 fxn

Thanks for the really thorough explanation, Xavier!

Based on this and the other issue I just linked, I'll try exploring a way to see if a duplicate enum definition could just warn instead of raise. I think that should cover both these cases as well as something like:

model Post < ApplicationRecord
  enum foo: { active: 0, inactive: 1 }
  enum foo: { active: 0, inactive: 1 }
end

which I think should definitely not silently work.

skipkayhil avatar Apr 18 '22 06:04 skipkayhil

This issue has been automatically marked as stale because it has not been commented on for at least three months. The resources of the Rails team are limited, and so we are asking for your help. If you can still reproduce this error on the 7-0-stable branch or on main, please reply with all of the information you have about it in order to keep the issue open. Thank you for all your contributions.

rails-bot[bot] avatar Jul 17 '22 07:07 rails-bot[bot]

Btw, I think that the proper label for this issue should be autoloading, right?

gabo-cs avatar Jul 17 '22 13:07 gabo-cs

I came across this today when not recreating constants. Approximately I had:

    enum status: { pending: 0, active: 1, archived: 2, error: 3 }
    enum project_type: ['commercial', 'non-commercial']

Which loaded without failure, but when I added a default like so:

    enum status: { pending: 0, active: 1, archived: 2, error: 3 }
    enum project_type: ['commercial', 'non-commercial'], default: 'non-commercial'

This would error with the following:

You tried to define an enum named "status" on the model "ModelName", but this will generate a instance method "pending?", which is already defined by another enum.

The API docs for Rails 7 don't seem to include the enum: [] or enum: {} syntax anymore, instead specifying :enum, [] or :enum, {}, and when I changed to the "new" syntax I no longer had the error. Looks like this was introduced in ActiveRecord 7 with #41328. From this, we can assume that the default argument in the pre-7.0 syntax should have been _default, and testing proved this was correct. In short:

    enum project_type: ['commercial', 'non-commercial'], default: 'non-commercial' # BROKEN (mixed arguments)
    enum project_type: ['commercial', 'non-commercial'], _default: 'non-commercial' # FIXED (old arguments)
    enum :project_type, ['commercial', 'non-commercial'], default: 'non-commercial' # FIXED (new arguments)

Ruby: 3.0.3 ActiveRecord: 7.0.1 (upgraded from ActiveRecord 6 a while ago)

RossBarnie avatar Jul 20 '22 15:07 RossBarnie

@RossBarnie yes, I just ran into this and was going to write it up ... but you already did the hard work.

Basically a better error probably helps ... or break the old syntax and make people update (I’d probably just do the latter).

The current error is not great when accidentally adding a new-style argument (default: :foobalicious) at the end of old-style syntax (enum status: { coo_coo_coo: 0, foobalicious: 1 }):

undefined method `all?' for :foobalicious:Symbol

lordofthedanse avatar Aug 04 '22 02:08 lordofthedanse

Thanks @RossBarnie for the writeup I am adding one more example to this. I have this class Exercise(which puts an enum value none which is active record class method)

class Exercise < ApplicationRecord
  enum :category, { strength: 1, toning: 2, yoga: 3 }
  enum :equipment_type, { none: 1, dumbbell: 2, gym: 3 }
end

The error which I get is below

activerecord-7.0.4/lib/active_record/enum.rb:301:in `raise_conflict_error': You tried to define an enum named "category" on the model "Exercise", but this will generate a instance method "strength?", which is already defined by another enum. (ArgumentError)

The error complains about first value of category enum which is strength? instead of none

Ruby: 3.1.2 ActiveRecord: 7.0.4

ranupratapsingh avatar Oct 16 '22 10:10 ranupratapsingh

This issue has been automatically marked as stale because it has not been commented on for at least three months. The resources of the Rails team are limited, and so we are asking for your help. If you can still reproduce this error on the 7-0-stable branch or on main, please reply with all of the information you have about it in order to keep the issue open. Thank you for all your contributions.

rails-bot[bot] avatar Jan 14 '23 10:01 rails-bot[bot]

@ranupratapsingh

The error complains about first value of category enum which is strength? instead of none

I see this behavior as well. I have something like:

enum status: { pending: 0, confirmed: 1, canceled: 2 }

and I ended up adding a method called confirming? in the same model, and got the error referenced above:

ArgumentError (You tried to define an enum named "status" on the model "<Model>", but this will generate a instance method "pending?", which is already defined by another enum.):

I was confused not only because the referenced instance method was pending?, the first item in the enum and not the one "interfered with", but because to my knowledge no methods are auto-generated in the progressive tense of an enum value (though I could see this being a useful addition to the [ActiveModel::Dirty](https://api.rubyonrails.org/classes/ActiveModel/Dirty.html) API).

Changing to the "new" syntax @RossBarnie mentioned (enum :status, { pending: 0, confirmed: 1, canceled: 2 }) did not resolve the issue (though it did initially seem to, as I subsequently encountered a different unrelated error in the same model further down in the file, but resolving that issue just brought back the original error), renaming my (private) confirming? method did fix it. Strangely, despite undoing my changes to all the previous states of the file, I can't reproduce that error anymore. 🤷

My point in all this is I guess to corroborate your experience that the wrong enum item is reported on as having a naming conflict. Even though I can't explain why I had one at all, it definitely seems to have had something to do with the second enum value, since fiddling with that resolved my problem.

Ruby: 3.2.1 Rails/ActiveRecord: 7.0.8

darryn02 avatar Dec 27 '23 23:12 darryn02