Skip to content
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 passing a column to type_cast #39106

Merged
merged 1 commit into from May 2, 2020

Conversation

kamipo
Copy link
Member

@kamipo kamipo commented May 1, 2020

The type information for type casting is entirely separated to type
object, so if anyone does passing a column to type_cast in Rails 6,
they are likely doing something wrong. See the comment for more details:

# If you are having to call this function, you are likely doing something
# wrong. The column does not have sufficient type information if the user
# provided a custom type on the class level either explicitly (via
# Attributes::ClassMethods#attribute) or implicitly (via
# AttributeMethods::Serialization::ClassMethods#serialize, +time_zone_aware_attributes+).
# In almost all cases, the sql type should only be used to change quoting behavior, when the primitive to
# represent the type doesn't sufficiently reflect the differences
# (varchar vs binary) for example. The type used to get this primitive
# should have been provided before reaching the connection adapter.
def type_cast_from_column(column, value) # :nodoc:

This also deprecates passing legacy binds (an array of [column, value]
which is 4.2 style format) to query methods on connection. That legacy
format was kept for backward compatibility, instead of that, I've
supported casted binds format (an array of casted values), it is easier
to construct binds than existing two binds format.

The type information for type casting is entirely separated to type
object, so if anyone does passing a column to `type_cast` in Rails 6,
they are likely doing something wrong. See the comment for more details:

https://github.com/rails/rails/blob/28d815b89487ce4001a3f6f0ab684e6f9c017ed0/activerecord/lib/active_record/connection_adapters/abstract/quoting.rb#L33-L42

This also deprecates passing legacy binds (an array of `[column, value]`
which is 4.2 style format) to query methods on connection. That legacy
format was kept for backward compatibility, instead of that, I've
supported casted binds format (an array of casted values), it is easier
to construct binds than existing two binds format.
@kamipo kamipo force-pushed the deprecate_passing_column_to_type_cast branch from aeafd59 to 92360e9 Compare May 1, 2020 22:18
@kamipo kamipo merged commit cfa5aa7 into rails:master May 2, 2020
@kamipo kamipo deleted the deprecate_passing_column_to_type_cast branch May 2, 2020 06:11
koic added a commit to koic/oracle-enhanced that referenced this pull request May 2, 2020
koic added a commit to koic/oracle-enhanced that referenced this pull request May 2, 2020
huacnlee added a commit to huacnlee/bulk_insert that referenced this pull request Sep 28, 2020
In Rails 6.1 type_cast_from_column API has been removed:
rails/rails#39106

lookup_cast_type_from_column was exists in Rails 5.0
kamipo added a commit to kamipo/rails that referenced this pull request Jan 16, 2021
Related issue rails#41133.

In rails#39106, I've allowed binds of casted values as an alternative of the
legacy binds (an array of `[column || nil, value]`) to deprecate and
remove the legacy binds support in a future version of Rails.

At that time, I've respected the existing logging format for binds.

i.e.

```ruby
# legacy binds
conn.select_all("SELECT * FROM events WHERE id IN (?, ?)", nil, [[nil, 1], [nil, 2]])
# (0.1ms)  SELECT * FROM events WHERE id IN (?, ?)  [[nil, 1], [nil, 2]]

# casted binds
conn.select_all("SELECT * FROM events WHERE id IN (?, ?)", nil, [1, 2])
# (0.1ms)  SELECT * FROM events WHERE id IN (?, ?)  [[nil, 1], [nil, 2]]
```

To improve the performance of generating IN clause, 72fd0ba avoids
`build_bind_attribute` for each values, so now binds has become casted
values.

```ruby
conn.select_all(Event.where(id: [1, 2]))
# (0.1ms)  SELECT * FROM events WHERE id IN (?, ?)  [[nil, 1], [nil, 2]]
```

Regardless of whether 72fd0ba avoids `build_bind_attribute` or not, the
logging format for the binds of casted values is odd (at least not
pretty to me).

I'd like to concise the logging format to just use casted values.

```ruby
conn.select_all("SELECT * FROM events WHERE id IN (?, ?)", nil, [1, 2])
# (0.1ms)  SELECT * FROM events WHERE id IN (?, ?)  [1, 2]

conn.select_all(Event.where(id: [1, 2]))
# (0.1ms)  SELECT * FROM events WHERE id IN (?, ?)  [1, 2]
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant