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

Deprecate using `#quoted_id` in quoting / type casting #27962

Merged
merged 3 commits into from Feb 23, 2017

Conversation

Projects
None yet
9 participants
@kamipo
Member

kamipo commented Feb 10, 2017

Originally quoted_id was used in legacy quoting mechanism. Now we use
type casting mechanism for that. Let's deprecate quoted_id.

@@ -17,7 +22,11 @@ def quote(value)
# SQLite does not understand dates, so this method will convert a Date
# to a String.
def type_cast(value, column = nil)
value = id_value_for_database(value) if value.is_a?(Base)

This comment has been minimized.

@rafaelfranca

rafaelfranca Feb 14, 2017

Member

Is this necessary here? quoted_id was not being called here.

@rafaelfranca

rafaelfranca Feb 14, 2017

Member

Is this necessary here? quoted_id was not being called here.

This comment has been minimized.

@kamipo

kamipo Feb 14, 2017

Member

If value is an AR::Base object, using value.id has potentially a bug.
If id: :datetime, precision: 3, typed cast value should be 2017-02-14 12:34:56.789000 by quoted_date, not 2017-02-14 12:34:56 +0000.

@kamipo

kamipo Feb 14, 2017

Member

If value is an AR::Base object, using value.id has potentially a bug.
If id: :datetime, precision: 3, typed cast value should be 2017-02-14 12:34:56.789000 by quoted_date, not 2017-02-14 12:34:56 +0000.

This comment has been minimized.

@rafaelfranca

rafaelfranca Feb 14, 2017

Member

If there is a potential bug in the current code we should add a test to it and fix before changing the behavior. Care to add the test?

@rafaelfranca

rafaelfranca Feb 14, 2017

Member

If there is a potential bug in the current code we should add a test to it and fix before changing the behavior. Care to add the test?

Show outdated Hide outdated activerecord/lib/active_record/connection_adapters/abstract/quoting.rb
if value.respond_to?(:quoted_id) && value.respond_to?(:id)
ActiveSupport::Deprecation.warn \

This comment has been minimized.

@rafaelfranca

rafaelfranca Feb 14, 2017

Member

We don't need to deprecate here, it is only being used to make sure it is a AR::Base object.

@rafaelfranca

rafaelfranca Feb 14, 2017

Member

We don't need to deprecate here, it is only being used to make sure it is a AR::Base object.

This comment has been minimized.

@kamipo

kamipo Feb 14, 2017

Member

Originally using only value.id (skipping cast_type.serialize and _type_cast) has potentially a bug.
Do we keep the return value.id without deprecating?

@kamipo

kamipo Feb 14, 2017

Member

Originally using only value.id (skipping cast_type.serialize and _type_cast) has potentially a bug.
Do we keep the return value.id without deprecating?

This comment has been minimized.

@rafaelfranca

rafaelfranca Feb 14, 2017

Member

I understand the problem but in this case here quoted_id was never called and the deprecation message say it was. If using value.id is a bug we should just fix the bug and remove the deprecation.

@rafaelfranca

rafaelfranca Feb 14, 2017

Member

I understand the problem but in this case here quoted_id was never called and the deprecation message say it was. If using value.id is a bug we should just fix the bug and remove the deprecation.

def test_type_cast_ar_object
value = DatetimePrimaryKey.new(id: @time)
assert_equal "2017-02-14 12:34:56.789000", @connection.type_cast(value)

This comment has been minimized.

@kamipo

kamipo Feb 14, 2017

Member

I added the test that type casting serialization and removed deprecation message.

@kamipo

kamipo Feb 14, 2017

Member

I added the test that type casting serialization and removed deprecation message.

kamipo added some commits Feb 14, 2017

Deprecate using `#quoted_id` in quoting
Originally `quoted_id` was used in legacy quoting mechanism. Now we use
type casting mechanism for that. Let's deprecate `quoted_id`.

@rafaelfranca rafaelfranca merged commit 230cf44 into rails:master Feb 23, 2017

2 checks passed

codeclimate no new or fixed issues
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@kamipo kamipo deleted the kamipo:deprecate_quoted_id branch Feb 24, 2017

@metaskills

This comment has been minimized.

Show comment
Hide comment
@metaskills

metaskills Feb 27, 2017

Contributor

Thanks for this change. The quoted_id was a technique promoted in the SQL Server adapter to bypass global national N'string' quoting in TSQL. Now that we have the type system, using our Type::Char implicitly via simple conditions or explicitly is the right way.

Contributor

metaskills commented Feb 27, 2017

Thanks for this change. The quoted_id was a technique promoted in the SQL Server adapter to bypass global national N'string' quoting in TSQL. Now that we have the type system, using our Type::Char implicitly via simple conditions or explicitly is the right way.

yahonda added a commit to yahonda/oracle-enhanced that referenced this pull request Mar 30, 2017

Handle `TIMESTAMP WITH TIMEZONE` separately from `TIMEZONE`
Fixes #1206

rails/rails#27962 moves `TypeCastingTest` from sqlite3 specific one to generic then some tests are failing.
because `type_cast` and `quote` method both call `quoted_date` for `Date` and `Time` class objects in abstract adapter
but Oracle enhanced adapter does not, using its own implementation.

This pull request lets generic `Date` and `Type` objects handled by abstract adapter implementation.
For non generic data types including timezone `ActiveRecord::OracleEnhanced::Type::TimestampTz` type has been created
and `TimestampTz` type is handled as it used to.

Oracle enhanced adpater now relies on `Time::DATE_FORMATS` defined in ActiveSupport by this change.
Therefore changing `NLS_DATE_FORMAT` and `NLS_TIMESTAMP_FORMAT` are not working and will not be supported.

yahonda added a commit to yahonda/oracle-enhanced that referenced this pull request Mar 30, 2017

Handle `TIMESTAMP WITH TIMEZONE` separately from `TIMEZONE`
Fixes #1206

rails/rails#27962 moves `TypeCastingTest` from sqlite3 specific one to generic then some tests are failing because `type_cast` and `quote` method both call `quoted_date` for `Date` and `Time` class objects in abstract adapter
but Oracle enhanced adapter does not, using its own implementation.

This pull request lets generic `Date` and `Type` objects handled by abstract adapter implementation.
For non-generic data types including timezone `ActiveRecord::OracleEnhanced::Type::TimestampTz` type has been created and `TimestampTz` type is handled as it used to.

Oracle enhanced adapter now relies on `Time::DATE_FORMATS` defined in ActiveSupport by this change.
Therefore changing `NLS_DATE_FORMAT` and `NLS_TIMESTAMP_FORMAT` are not working and will not be supported.

yahonda added a commit to yahonda/oracle-enhanced that referenced this pull request Mar 30, 2017

Handle `TIMESTAMP WITH TIMEZONE` separately from `TIMEZONE`
Fixes #1206

rails/rails#27962 moves `TypeCastingTest` from sqlite3 specific one to generic then some tests are failing because `type_cast` and `quote` method both call `quoted_date` for `Date` and `Time` class objects in abstract adapter
but Oracle enhanced adapter does not, using its own implementation.

This pull request lets generic `Date` and `Type` objects handled by abstract adapter implementation.
For non-generic data types including timezone `ActiveRecord::OracleEnhanced::Type::TimestampTz` type has been created and `TimestampTz` type is handled as it used to.

Oracle enhanced adapter now relies on `Time::DATE_FORMATS` defined in ActiveSupport by this change.
Therefore changing `NLS_DATE_FORMAT` and `NLS_TIMESTAMP_FORMAT` are not working and will not be supported.
@grosser

This comment has been minimized.

Show comment
Hide comment
@grosser

grosser May 2, 2017

Contributor

not very helpful error message ... would be great to point out the source_location ... just ran into this without having the slightest hint on where the quote_id is coming from ... had to open up the gem to debug ...

turns out to be active_hash ... but when I remove the quote_id support then it just blows up :/

class Role < ActiveHash::Base
  undef quoted_id

...

TypeError: can't quote Role
    activerecord-5.1.0/lib/active_record/connection_adapters/abstract/quoting.rb:189:in `_quote'

so had to go through all places that use the fake AR and change them to call .id and then pass that into queries ...

Contributor

grosser commented May 2, 2017

not very helpful error message ... would be great to point out the source_location ... just ran into this without having the slightest hint on where the quote_id is coming from ... had to open up the gem to debug ...

turns out to be active_hash ... but when I remove the quote_id support then it just blows up :/

class Role < ActiveHash::Base
  undef quoted_id

...

TypeError: can't quote Role
    activerecord-5.1.0/lib/active_record/connection_adapters/abstract/quoting.rb:189:in `_quote'

so had to go through all places that use the fake AR and change them to call .id and then pass that into queries ...

@sevenseacat

This comment has been minimized.

Show comment
Hide comment
@sevenseacat

sevenseacat May 2, 2017

Contributor

Sounds like you should file a bug with ActiveHash if the problem is there...

Contributor

sevenseacat commented May 2, 2017

Sounds like you should file a bug with ActiveHash if the problem is there...

@rainchen

This comment has been minimized.

Show comment
Hide comment
@rainchen

rainchen Jul 20, 2017

DEPRECATION WARNING: quoted_id is deprecated and will be removed from Rails 5.2
What is the replacement method for quoted_id? the deprecation warning message is not helpful enough.

rainchen commented Jul 20, 2017

DEPRECATION WARNING: quoted_id is deprecated and will be removed from Rails 5.2
What is the replacement method for quoted_id? the deprecation warning message is not helpful enough.

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Jul 21, 2017

Member

There is no replacement for that method. If you need to quote you id differently you need to define a different type for that column.

Member

rafaelfranca commented Jul 21, 2017

There is no replacement for that method. If you need to quote you id differently you need to define a different type for that column.

@pjrebsch

This comment has been minimized.

Show comment
Hide comment
@pjrebsch

pjrebsch Apr 13, 2018

@rainchen

The replacement appears to be defining a value_for_database instance method:

# Quotes the column value to help prevent
# {SQL injection attacks}[https://en.wikipedia.org/wiki/SQL_injection].
def quote(value)
value = id_value_for_database(value) if value.is_a?(Base)
if value.respond_to?(:value_for_database)
value = value.value_for_database
end
_quote(value)
end

pjrebsch commented Apr 13, 2018

@rainchen

The replacement appears to be defining a value_for_database instance method:

# Quotes the column value to help prevent
# {SQL injection attacks}[https://en.wikipedia.org/wiki/SQL_injection].
def quote(value)
value = id_value_for_database(value) if value.is_a?(Base)
if value.respond_to?(:value_for_database)
value = value.value_for_database
end
_quote(value)
end

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Apr 13, 2018

Member

No, that is not the replacement, the value_for_database is called on the column type, not in the Active Record instance. So, like I said above, the replacement is defining a different type for the column.

Member

rafaelfranca commented Apr 13, 2018

No, that is not the replacement, the value_for_database is called on the column type, not in the Active Record instance. So, like I said above, the replacement is defining a different type for the column.

@pjrebsch

This comment has been minimized.

Show comment
Hide comment
@pjrebsch

pjrebsch Apr 13, 2018

Sorry, then I misunderstood the intent here.

My use case for quoted_id was to be able to pass a custom class instance which acts like an integer into an AR query without having to bother manually calling a method on it to convert it into a type that _quote expected.

Overly simplified example:

class CustomClass
  def initialize( val )
    @val = val
  end
  
  # for Rails >= 5.2
  def value_for_database
    @val.to_i
  end

  # for Rails < 5.2
  alias_method :quoted_id, :value_for_database
end

obj = CustomClass.new('2')
SomeModel.where( some_integer_column: obj )

Implementing quoted_id / value_for_database as an instance method on my custom class allows this to work without raising a "cannot quote" exception.

pjrebsch commented Apr 13, 2018

Sorry, then I misunderstood the intent here.

My use case for quoted_id was to be able to pass a custom class instance which acts like an integer into an AR query without having to bother manually calling a method on it to convert it into a type that _quote expected.

Overly simplified example:

class CustomClass
  def initialize( val )
    @val = val
  end
  
  # for Rails >= 5.2
  def value_for_database
    @val.to_i
  end

  # for Rails < 5.2
  alias_method :quoted_id, :value_for_database
end

obj = CustomClass.new('2')
SomeModel.where( some_integer_column: obj )

Implementing quoted_id / value_for_database as an instance method on my custom class allows this to work without raising a "cannot quote" exception.

@leifcr

This comment has been minimized.

Show comment
Hide comment
@leifcr

leifcr May 28, 2018

If you need to quote you id differently you need to define a different type for that column.

@rafaelfranca Can you point in a direction for looking into definining a different type for a column? I see some AR addons are 'stuck' on 5.1 due to the quoted_id change.

leifcr commented May 28, 2018

If you need to quote you id differently you need to define a different type for that column.

@rafaelfranca Can you point in a direction for looking into definining a different type for a column? I see some AR addons are 'stuck' on 5.1 due to the quoted_id change.

@leifcr

This comment has been minimized.

Show comment
Hide comment
@leifcr

leifcr commented May 29, 2018

@rafaelfranca Thanks!

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