New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rails where query should see value is an enum and convert a string #17226

Closed
justin808 opened this Issue Oct 10, 2014 · 18 comments

Comments

Projects
None yet
9 participants
@justin808

justin808 commented Oct 10, 2014

Suppose you have this situation:

MyModel.where(some_enum: "enum_value")

Well, that currently doesn't work. You have to do this:

MyModel.where(some_enum: some_enums["enum_value"])

That is so that "enum_value" is converted to the proper integer value.

Even worse, the first query currently does not throw an error and converts the string (any string) to an integer value of zero!

Here's an actual console trace showing this:

[17] (pry) main: 0> Manager.where(dominant_product_type: "hedge_fund").count
   (8.3ms)  SELECT COUNT(*) FROM "managers"  WHERE "managers"."dominant_product_type" = 0
3980
[18] (pry) main: 0> Manager.where(dominant_product_type: "liquidity_fund").count
   (8.4ms)  SELECT COUNT(*) FROM "managers"  WHERE "managers"."dominant_product_type" = 0
3980
[20] (pry) main: 0> Manager.where(dominant_product_type: Manager.dominant_product_types["liquidity_fund"]).count
   (7.5ms)  SELECT COUNT(*) FROM "managers"  WHERE "managers"."dominant_product_type" = 1
22

Notice, zero is used in the first two queries. The third query is correct.

A related issue is this, using the example from the docs:

Should this code give the same result or different:

statuses = Conversation.select(:status).where.not(status: nil).distinct.map(&:status)

as compared to:

statuses = Conversation.distinct.where.not(status: nil).pluck(:status)

In the first case, you get back an Array with 2 strings: =["active",
"archived"]=. In the second case, you get back an Array with 2 integers: [0, 1].

If the core team deems this worthy, I could try my hand at a PR to implement this.

At the minimum, this seems worthy for the documentation.

@Bounga

This comment has been minimized.

Contributor

Bounga commented Oct 11, 2014

@justin808 It would have a performance cost to check for enums automatically and convert the string to the corresponding integer. But rather of doing:

MyModel.where(some_enum: some_enums["enum_value"])

you should use the provided scopes generated along with enums:

MyModel.enum_value

which according to the doc is the recommended way of doing that.

About the 0 "bug" it is because AR uses #to_i on the provided value maybe we could use Integer to throw an exception when it's not a valid value but I tend to think we should stick to using provided scopes.

For sure the enum related API could be changed to be less permissive but I think it would introduce code difficult to maintain for near no gain in confort for developers.

About the map vs pluck, it seems to be an expected behavior to me. pluck return the raw value from the db, map calls the status method which will convert the integer to its human readable value.

@rafaelfranca

This comment has been minimized.

Member

rafaelfranca commented Oct 11, 2014

which according to the doc is the recommended way of doing that.

Yes.

@justin808

This comment has been minimized.

justin808 commented Oct 11, 2014

@Bounga and @rafaelfranca, When you want to match objects such that the value is coming from another object of the same type, which would you recommend and why?

MyModel.send(another_instance_of_my_model.some_enum)

or

MyModel.where(some_enum: some_enums[another_instance_of_my_model.some_enum])

or

MyModel.where(some_enum: another_instance_of_my_model.attributes["some_enum"])
@justin808

This comment has been minimized.

justin808 commented Oct 11, 2014

Would it make sense to have a gem extension allow something like:

MyModel.where_enum(some_enum: another_instance_of_my_model.some_enum)

and

MyModel.where_enum(some_enum: enum_value)
@justin808

This comment has been minimized.

justin808 commented Oct 11, 2014

@Bounga I see little downside and significant upside to only making the change of using

Integer("some_string", 10)

Rather than:

someString.to_i

In terms of the principle of least surprise, it's very surprising to have this sort of behavior shown below. Thus, stricter type conversion would save developers meaningful time in terms of debugging queries. The use of zero is especially problematic, as Enum users often use zero to mean the default value, so it's hard to distinguish that zero is incorrect.

[1] (pry) main: 0> "$1,234".to_i
0
[2] (pry) main: 0> Integer("$1,234")
ArgumentError: invalid value for Integer(): "$1,234"
from (pry):2:in `Integer'

If this is desirable, I could take a crack at making this change and submit a PR.

@Bounga

This comment has been minimized.

Contributor

Bounga commented Oct 12, 2014

@Bounga and @rafaelfranca, When you want to match objects such that the value is coming from another object of the same type, which would you recommend and why?

I personally would use #send if there's no high performance concerns. IMO it's the cleanest way to write this.

The gem may help some people but I don't think it will get a lot of attention of the community and for sure it will never make it into the core. But if you do need this, implement it as a gem so you can re-use it easily and share it with the community.

About the enhancement of parameter conversion (#to_i vs Integer) it may help to avoid surprises as you said. You should explain this possible confusion with 0 value on the rails-core mailing list to know what core developers think about this change. If you can provide a PR (with tests) it would have more impact since people will be able to test the change.

@justin808

This comment has been minimized.

justin808 commented Oct 15, 2014

@Bounga and @rafaelfranca What if we added this to the default methods created:

Conversations.with_status(string_or_num_status)

Thus, have the enums generate an additional scope per enum attribute.

The problem with the Conversations.active scope is that one has to use send for use when the value is dynamic, and if the value is invalid, the error message will be obscure.

I personally like seeing the attribute name with the enum value in the queries. Some of my models have many enum values and just seeing something like Conversation.active does not tell me what attribute is being queried on.

A somewhat related issue is having multiple attributes that share enum values. Has the core team ever gave any recommendation for this use case?

@Bounga

This comment has been minimized.

Contributor

Bounga commented Oct 15, 2014

@justin808 This may be interesting. I understand your point of view. You should discuss this feature on Rails-core mailing list to get feedback from core team.

@justin808

This comment has been minimized.

justin808 commented Nov 10, 2014

I wrote up a detailed article on this topic: Enums and Queries in Rails 4.1, and Understanding Ruby. I'd be happy to create a PR once there is consensus on what direction to take.

  1. @Bounga and @rafaelfranca on Github suggest that we can't automatically
    convert enum string values in queries. I think that is true for converting
    cases of a ? or a named param, but I suspect that a quick map lookup to see
    that the attribute is an enum, and a string is passed, and then converting
    the string value to an integer is the right thing to do for 2 reasons:
  2. This is the sort of "magic" that I expect from Rails.
  3. Existing methods find_or_initialize_by and find_or_create_by will
    result in obscure bugs when string params are passed for enums.

However, it's worth considering if all default accessor methods (setters)
should be consistently be called for purposes of passing values in a map to
such methods. I would venture that Rails enums are some Rails provided magic,
and thus they should have a special case. If this shouldn't go into Rails,
then possibly a gem extension could provide a method like
Model.where_with_enum which would convert a String into the proper
numerical value for the enum. I'm not a huge fan of the generated Model
scopes for enums, as I like to see what database field is being queried
against.

2. Aside from putting automatic conversion of the enum hash attributes, I
recommend we change the automatic conversion of Strings to integers to use
the stricter Integer(some_string) rather than some_string.to_i. The
difference is considerable, String#to_i is extremely permissive. Try it in
a console. With the to_i method, any number characters at the beginning of
the String are converted to an Integer. If the first character is not a
number, 0 is returned, which is almost certainly a default enum value.
Thus, this simple change would make it extremely clear when an enum string
is improperly used. I would guess that this would make some existing code
crash, but in all circumstances for a valid reason. As to whether this change
should be done for all integer attributes is a different discussion, as that
could have backwards compatibility ramifications. This change would require changing the tests in ActiveRecord::ConnectionAdapters::TypesTest. For example, this test:

assert_equal 0, type.type_cast_from_user('bad') 

would change to throw an exception, unless the cases are restricted to using
Integer.new() for enums. It is inconsistent that some type conversions throw
exceptions, such as converting a symbol to an integer. Whether or not they
should is much larger issue. In the case of enums, I definitely believe that proper enum string value should not silently convert to zero every time.

@fabn

This comment has been minimized.

fabn commented Feb 18, 2015

I saw that this has been implemented in master with this commit c51f9b6

However the commit is only in master branch, will that commit backported in 4.2.x or it's just for rails 5.x?

@rafaelfranca

This comment has been minimized.

Member

rafaelfranca commented Feb 18, 2015

@fabn only for 5.x. That implementation requires a lot of refactorings to work.

@fabn

This comment has been minimized.

fabn commented Feb 18, 2015

@rafaelfranca I was hoping for the opposite answer, in any case thanks for the prompt answer.

@Hamdan85

This comment has been minimized.

Hamdan85 commented Sep 25, 2015

is this solved? wheres the solution!?

@abdalaklinch

This comment has been minimized.

abdalaklinch commented Nov 9, 2016

In conclusion, what is the recommended way for performing a mysql query with the .where method, while using Rails 4?

@yskkin

This comment has been minimized.

Contributor

yskkin commented Nov 9, 2016

Does this mean the following document is invalid?
http://api.rubyonrails.org/classes/ActiveRecord/Enum.html

Of course, you can also query them directly if the scopes don't fit your needs:

Conversation.where(status: [:active, :archived])
Conversation.where.not(status: :active)
@abdalaklinch

This comment has been minimized.

abdalaklinch commented Nov 9, 2016

As a hack solution, I am doing this now:
Requests.where(status: Request.statuses[params[:status]])

@pirj

This comment has been minimized.

pirj commented Jan 9, 2017

@yskkin The document you mention should be valid for Rails 5, while where is not mentioned in 4.2 docs: http://api.rubyonrails.org/v4.2/classes/ActiveRecord/Enum.html

@stormit-vn

This comment has been minimized.

stormit-vn commented Aug 1, 2018

Sorry, I knew that the issue has been closed, but I would like to post my question related to this issue.

Does anyone know how to customize the query based on the filter query string and enum of the record?

Here is my requirement

In the Record, we have this filter as below

enum stage: { requested: 0, processing: 10, submitted: 20, approved: 30, open: 45, expired: 50, closed: 60, pending: 61, denied: 62 }

Basically, it works properly when using stage filter such as ?filter[stage]=expired&page[limit]=10

However, we have a requirement that we need to customize the filter/query to add another field such as if the stage filter is expired, we have to check the stage column in database as well as another column (e.g. expired_date). Here is the sample output query for the above URI

SELECT * FROM table WHERE stage=50 OR expired_date < current time

Also, when returning the response, if any record matched above filter, we want to change the stage field to expired (or 50 as its value), it is because of some row with expired_date < current time might have different stage value. This is an optional requirement

I am new to RoR, so I would like to know your advice. So could you please take a look and let me know your idea

Thank in advance!

Regards,
Hoang

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment