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

5-0-stable: Fix select_all with legacy binds #28495

Merged
merged 4 commits into from
Apr 21, 2017

Conversation

kamipo
Copy link
Member

@kamipo kamipo commented Mar 20, 2017

Backport #27939.

r? @rafaelfranca

Because `type_cast` against `binds` always requires
`attr.value_for_database` and this pattern appears frequently.
@rails-bot
Copy link

warning Warning warning

  • Pull requests are usually filed against the master branch for this repo, but this one is against 5-0-stable. Please double check that you specified the right target!

Copy link
Member

@rafaelfranca rafaelfranca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

explain.rb and log_subscriber.rb also call bind.value_for_database. Should not them be fixed?

if binds.first.is_a?(Array)
binds.map { |column, value| type_cast(value, column) }
else
binds.map { |attr| type_cast(attr.value_for_database) }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have a method above: prepare_binds_for_database that does exactly what this does.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since 5.0, binds that generated in the internal is an array of QueryAttribute.
This fix only addresses passing legacy binds explictly case (e.g. conn.select_all("...", nil, [[column, value], ...])) and it is sufficient fix I think.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, I mean that we can reuse that method here.

@kamipo
Copy link
Member Author

kamipo commented Mar 20, 2017

hmm... we also need to pick f814585.

end
def render_bind(attr, type_casted_value)
value = if attr.type.binary? && attr.value
"<#{attr.value_for_database.to_s.bytesize} bytes of binary data>"
Copy link
Member Author

@kamipo kamipo Mar 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realized an issue. We cannot render legacy binds even if master branch.
If passed legacy binds, attr is an array, cannot call attr.type.binary?.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, we will need an conditional here.

@kamipo
Copy link
Member Author

kamipo commented Mar 23, 2017

Added #28526.

@kamipo
Copy link
Member Author

kamipo commented Apr 21, 2017

Any chance this fix could be merged in 5-0-stable? This has already been fixed in 5-1-stable.

@rafaelfranca rafaelfranca merged commit cb2da8a into rails:5-0-stable Apr 21, 2017
@kamipo kamipo deleted the backport-27939 branch April 21, 2017 01:16
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

4 participants