Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

`scope` and `enum` should raise an error if the generated methods conflict with an existing one #13389

Closed
chancancode opened this Issue · 14 comments

6 participants

@chancancode
Owner

It should cover these four categories:

Case 1 (class method conflicts):

class Lol < ActiveRecord::Base
  enum state_a: [:new]
end

>> Lol.new
  Lol Load (1.1ms)  SELECT "lols".* FROM "lols"  WHERE "lols"."state_a" = 0

Case 2 (conflict between enums):

class Model < ActiveRecord::Base
  enum state_a: [:conflict, :something]
  enum state_b: [:conflict, :else]
end

Case 3 (conflict with AR methods):

class Lol < ActiveRecord::Base
  enum state_a: [:valid]
end

>> Lol.new.save!
   (0.1ms)  begin transaction
   (0.0ms)  rollback transaction
ArgumentError: wrong number of arguments (1 for 0)

Case 4 (dynamic AR methods):

>> o = Order.first
  Order Load (0.2ms)  SELECT  "orders".* FROM "orders"   ORDER BY "orders"."id" ASC LIMIT 1
=> #<Order id: 1, shipping_address: nil, state: nil, created_at: "2013-12-19 00:14:45", updated_at: "2013-12-19 00:14:45">
>> o.shipping_address = "123 Some St"
=> "123 Some St"
>> o.shipping_address_changed?
=> false

As part of this, this should be disallowed as well:

class Model < ActiveRecord::Base
  scope :new, ->{ .... }
  scope :allocate, ->{ ... }
  scope :all, ->{ ... }
  scope :none, -> { ... }

  ...
end
@chancancode chancancode was assigned
@ronalchn

I think that the enum API should be changed, to add a layer of nesting.

Instead of Lol.new, lol.valid? etc, we should instead have:

Lol.state_a.new # scope for state_a = new
lol.state_a.valid? # lol.state_a == "valid"?

This would avoid any number of conflicts that could occur.

If in some cases, we want a more intuitive api without nesting, it should be specified in the options, eg:

class Task
  enum status: [:pending, :complete], nesting: :shallow
end

task.pending?

In no instance should the non-nested version be the default, since that is very likely to cause conflicts.

Note: perhaps a better name should be found instead of nesting: :shallow.

@allomov

+1 for @ronalchn.
but I'm not sure this should be activerecord functionality. as me for the better solution is to move enums to separate gem, such as enumerize.

@ronalchn

I'm actually looking forward to this functionality, because it only takes about 10 lines of code to implement something simple, and that is often what you need. However, having it in rails removes the need to maintain those 10 lines of code, or to work out which gem to use.

Enumerations appear in many programming languages, so it is often useful.

I think it is a great benefit to have a canonical implementation here.

For many simple uses, it also avoids poor workaround implementations (eg. string types, or multiple boolean fields).

@chancancode chancancode referenced this issue from a commit
@dhh dhh Added ActiveRecord::Base#enum for declaring enum attributes where the…
… values map to integers in the database, but can be queried by name
db41eb8
@allomov

@ronalchn "Perfection is achieved not when there is nothing more to add, but when there is nothing left to take away". In this case I'd rather add one row in Gemfile than additional simplicity inside activerecord.

@allomov

@ronalchn good example of consequences caused by adding such functionality is appearing of "dangerous" methods that @chancancode is talking about. good example imo.

@rafaelfranca

@allomov most part of these "dangerous" methods are also present in the associations and attributes, so it is not a problem of this new feature. You can't have a attribute called errors, neither valid.

@imanel

But reducing available method names even further is not best idea. I would say that we either need to remove adding scoped at all or make them unobtrusive like in @ronalchn comment

@chancancode
Owner

Yes, the only thing "dangerous" about this (hence in quotes) is method name collision, which is also present in scope, etc. I'll at least fix scope to also raise errors.

The examples are meant for tracking four different categories of bugs so I don't forget:

  1. generated scopes conflict with class methods
  2. conflict between enums on the same class (or superclass)
  3. conflict with AR methods like valid?
  4. conflict with dynamic methods added via method missing

The resulting Pull Request should raise on all four cases. (AR already check these cases for attributes, so the bug is that enum and scope doesn't currently do the same checks. And they should.)

Also worth noting, the introduction of enum doesn't actually impose any further restrictions. If you don't use it, it doesn't affect you (well, unless you currently uses a scope called new or alloc :trollface:). If you do choose to use it, you'll just have to watch out on what names you cannot use (similar to how you can't have model attributes called errors and valid), the resulting PR intends to help you discover these mistakes early on rather than down the road.

@imanel

Is there possibility to structure it in a way that will allow "new" as a state? I'm pretty sure it will be very often case and if it's not absolutely necessary it would be nice to allow it ;)

@chancancode
Owner

@imanel The problem with new is that it will attempt to introduce a scope called new, which makes it impossible to allocate instances from the model class :trollface:

Suppose that, instead of raising an error, this skips creating the new scope for you. Then that'd let you define a new scope with a different name. But if you have to use a different name for the scope anyway, why not just use that name in the enum in the first place? Not to mention this would make a very confusing/surprising API :(

Just use unreviewed or something instead of new to make it easy on yourself. Reserved words are nothing new. Naming things are hard, this happens all the time. (klass anyone?)

Another option is to namespace these like @ronalchn and another commenter suggested in a different thread. However, that has been rejected by DHH because it makes the common (and far more likely) cases more cumbersome, which I agree.

If you are okay with name-spacing though, you can already do this:

class Lol < ActiveRecord::Base
  enum state: [:state_new, :state_closed, :state_rejected, ...]
end

Lol.state_new
lol.state_new?

etc

So, despite the scary looking examples, things are not that bad. Most of the time this won't be a problem for you in the real world. I think the current implantation already give you everything you need – it's "shallow" by default, but if for some reason you have conflicts AND you think name-spacing is a good solution, then you can do that explicitly.

:+1:?

@imanel

Hmm... I get your point - but I have other issue: this will not solve (2)

How about idea:

class Lol < ActiveRecord::Base
  enum state: [:new, :closed, :rejected, ...]
end

Lol.state_new
lol.state_new?

Combine name of enum with state name - that way you will avoid collisions of states between enums and reduce chance for using state that is already marked as reserved word.

:+1: ? :smile:

@chancancode
Owner

That's what I meant by "name spacing". It has been proposed and rejected here, also similar to @ronalchn's proposal above.

I agree with DHH that if you look at this at a macroscopic level, this is not a very common case, and making the common case worse for a corner case doesn't seem ideal.

But as I said before, you can already do this explicitly if you prefer it. I still think the "shallow" version is much nicer to work with though. If I were in the situation to choose between not using new (and use unreviewed or something) vs prefixing everything, I'll definitely go for the former.

@ronalchn

I disagree that namespacing should be done manually, because it makes other uses more verbose. Here are the cases where manual namespacing to avoid conflicts is unnecessary verbose:

bug.status = :status_new

class Bug < ActiveRecord::Base
  scope :open, -> { where('status <> ? OR status <> ?', STATUS[:status_resolved], STATUS[:status_rejected]) }
end

There should be an option to namespace, but only on the methods, and not on the symbols.

@imanel

@chancancode I was thinking over night about what you said and I believe we should have option to enable namespacing, but as option and not default. Because of that I tried to implement it in #13433 - please check if this is acceptable and share feedback. Thanks!

@chancancode chancancode referenced this issue from a commit in chancancode/rails
@chancancode chancancode `scope` now forbids names that conflict with existing class methods
The following examples now cause an ArgumentError to be raised:

    class Model < ActiveRecord::Base
      scope :new, ...
      scope :create, ...
      scope :all, ...
      scope :an_existing_scope, ...
      scope :find_by_some_attribute, ...
    end

See also #13389.
09256ed
@chancancode chancancode referenced this issue from a commit in chancancode/rails
@chancancode chancancode `scope` now forbids names that conflict with existing class methods
The following examples now cause an ArgumentError to be raised:

    class Model < ActiveRecord::Base
      scope :new, ...
      scope :create, ...
      scope :all, ...
      scope :an_existing_scope, ...
      scope :find_by_some_attribute, ...
    end

See also #13389.
9bfc5a6
@chancancode chancancode referenced this issue from a commit in chancancode/rails
@chancancode chancancode `enum` now forbids values that conflict with existing methods
`enum` generates a few dynamic methods. If any of these methods
conflict with an existing one, an ArgumentError will be raised. For
example:

    class Model < ActiveRecord::Base
      enum :good, [:existing, :enum, ...]

      enum :bad1, [:existing, ...] # conflict with another enum
      enum :bad2, [:new, ...]      # conflict with a class method
      enum :bad3, [:valid, ...]    # conflict with valid?
      enum :bad4, [:save, ...]     # conflict with save!
    end

Fixes #13389.
167a88f
@chancancode chancancode referenced this issue from a commit in chancancode/rails
@chancancode chancancode `scope` now forbids names that conflict with existing class methods
The following examples now cause an ArgumentError to be raised:

    class Model < ActiveRecord::Base
      scope :new, ...
      scope :create, ...
      scope :all, ...
      scope :an_existing_scope, ...
      scope :find_by_some_attribute, ...
    end

See also #13389.
41883ef
@chancancode chancancode referenced this issue from a commit in chancancode/rails
@chancancode chancancode `enum` now forbids values that conflict with existing methods
`enum` generates a few dynamic methods. If any of these methods
conflict with an existing one, an ArgumentError will be raised. For
example:

    class Model < ActiveRecord::Base
      enum :good, [:existing, :enum, ...]

      enum :bad1, [:existing, ...] # conflict with another enum
      enum :bad2, [:new, ...]      # conflict with a class method
      enum :bad3, [:valid, ...]    # conflict with valid?
      enum :bad4, [:save, ...]     # conflict with save!
    end

Fixes #13389.
387f45a
@chancancode chancancode referenced this issue from a commit in chancancode/rails
@chancancode chancancode `scope` now forbids names that conflict with existing class methods
The following examples now cause an ArgumentError to be raised:

    class Model < ActiveRecord::Base
      scope :new, ...
      scope :create, ...
      scope :all, ...
      scope :an_existing_scope, ...
      scope :find_by_some_attribute, ...
    end

See also #13389.
6d80dc7
@chancancode chancancode referenced this issue from a commit in chancancode/rails
@chancancode chancancode `enum` now forbids values that conflict with existing methods
`enum` generates a few dynamic methods. If any of these methods
conflict with an existing one, an ArgumentError will be raised. For
example:

    class Model < ActiveRecord::Base
      enum :good, [:existing, :enum, ...]

      enum :bad1, [:existing, ...] # conflict with another enum
      enum :bad2, [:new, ...]      # conflict with a class method
      enum :bad3, [:valid, ...]    # conflict with valid?
      enum :bad4, [:save, ...]     # conflict with save!
    end

Fixes #13389.
1bcaa42
@chancancode chancancode referenced this issue from a commit in chancancode/rails
@chancancode chancancode `scope` now raises on "dangerous" name conflict
Similar to dangerous attribute methods, a scope name conflict is
dangerous if it conflicts with an existing class method defined within
`ActiveRecord::Base` but not its ancestors.

See also #13389.

*Godfrey Chan*, *Philippe Creux*
6b68f15
@chancancode chancancode referenced this issue from a commit in chancancode/rails
@chancancode chancancode `scope` now raises on "dangerous" name conflicts
Similar to dangerous attribute methods, a scope name conflict is
dangerous if it conflicts with an existing class method defined within
`ActiveRecord::Base` but not its ancestors.

See also #13389.

*Godfrey Chan*, *Philippe Creux*
74bf1ba
@chancancode chancancode referenced this issue from a commit in chancancode/rails
@chancancode chancancode `enum` now raises on "dangerous" name conflicts
Dangerous name conflicts includes instance or class method conflicts
with methods defined within `ActiveRecord::Base` but not its ancestors,
as well as conflicts with methods generated by other enums on the same
class.

Fixes #13389.
a3c7660
@chancancode chancancode referenced this issue from a commit in chancancode/rails
@chancancode chancancode `enum` now forbids values that conflict with existing methods
`enum` generates a few dynamic methods. If any of these methods
conflict with an existing one, an ArgumentError will be raised. For
example:

    class Model < ActiveRecord::Base
      enum :good, [:existing, :enum, ...]

      enum :bad1, [:existing, ...] # conflict with another enum
      enum :bad2, [:new, ...]      # conflict with a class method
      enum :bad3, [:valid, ...]    # conflict with valid?
      enum :bad4, [:save, ...]     # conflict with save!
    end

Fixes #13389.
659cb8c
@chancancode chancancode referenced this issue from a commit in chancancode/rails
@chancancode chancancode `scope` now raises on "dangerous" name conflicts
Similar to dangerous attribute methods, a scope name conflict is
dangerous if it conflicts with an existing class method defined within
`ActiveRecord::Base` but not its ancestors.

See also #13389.

*Godfrey Chan*, *Philippe Creux*
7e8e91c
@chancancode chancancode closed this issue from a commit
@chancancode chancancode `enum` now raises on "dangerous" name conflicts
Dangerous name conflicts includes instance or class method conflicts
with methods defined within `ActiveRecord::Base` but not its ancestors,
as well as conflicts with methods generated by other enums on the same
class.

Fixes #13389.
40f0257
@senny senny added the enum label
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.