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

ActiveRecord::QueryMethods#from no longer escapes mysql attributes #20360

Closed
k0nserv opened this issue May 29, 2015 · 2 comments
Closed

ActiveRecord::QueryMethods#from no longer escapes mysql attributes #20360

k0nserv opened this issue May 29, 2015 · 2 comments
Assignees
Milestone

Comments

@k0nserv
Copy link

k0nserv commented May 29, 2015

The release of Rails 4.2.1 breaks subqueries with columns that match reserved keywords.

Script to reproduce

unless File.exist?('Gemfile')
  File.write('Gemfile', <<-GEMFILE)
    source 'https://rubygems.org'
    gem 'rails', '4.2.1' # works with '4.2.0'
    gem 'mysql2'
  GEMFILE

  system 'bundle'
end

require 'bundler'
Bundler.setup(:default)

require 'active_record'
require 'minitest/autorun'
require 'logger'

# This connection will do for database-independent bug reports.
ActiveRecord::Base.establish_connection(adapter: 'mysql2',
                                        host: 'localhost',
                                        username: 'user',
                                        password: 'password',
                                        database: 'test')
ActiveRecord::Base.logger = Logger.new(STDOUT)

ActiveRecord::Schema.define do
  create_table :test, force: true  do |t|
    t.integer :desc
  end
end

class Test < ActiveRecord::Base
  self.table_name = :test

  default_scope { select(:desc) }
end

class BugTest < Minitest::Test
  def test_from_escaping_attributes
    Test.create!(desc: 10)
    Test.create!(desc: 11)

    result = Test.from(Test.where(desc: 10), Test.table_name)

    assert_equal 1, result.to_a.size
  end
end

Rails 4.2.0

In rails 4.2.0 this will generate a query that looks like this

SELECT `test`.`desc` FROM (SELECT `test`.`desc` FROM `test` WHERE `test`.`desc` = 10) test

Notice that the value in the select are properly escaped with backticks and fully qualified with the name of the subquery

Rails 4.2.1

In rails 4.2.1 it will generate a query that looks like this

SELECT desc FROM (SELECT `test`.`desc` FROM `test` WHERE `test`.`desc` = 10) test

Notice how both the escaping and the qualifying of the attribute with the name of the subquery is not present. Because the desc column is not escaped this will cause a SQL error

You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'desc FROM (SELECT `test`.`desc` FROM `test` WHERE `test`.`desc` = 10) test' at line 1: SELECT desc FROM (SELECT `test`.`desc` FROM `test` WHERE `test`.`desc` = 10) test

Suggested fix

Revert this behaviour back to escaping and fully qualifying the attributes when selecting from a subquery

@vipulnsward
Copy link
Member

This is related to #18743 (comment)
AFAIK, based on the discussion we agreed for this corner case?

@k0nserv
Copy link
Author

k0nserv commented May 30, 2015

That seems like it's probably it. Maybe just making sure the value is quoted, but skipping the full qualification of the column would be a good middle road. AFAIK that should allow both this behaviour and the behaviour discussed in #18743 to continue working

@sgrif sgrif self-assigned this May 30, 2015
@sgrif sgrif added this to the 4.2.2 milestone May 30, 2015
@sgrif sgrif closed this as completed in 0ef7e73 May 30, 2015
sgrif added a commit that referenced this issue May 30, 2015
Our general contract in Active Record is that strings are assumed to be
SQL literals, and symbols are assumed to reference a column. If a from
clause is given, we shouldn't include the table name, but we should
still quote the value as if it were a column.

Upon fixing this, the tests were still failing on SQLite. This was
because the column name being returned by the query was `"\"join\""`
instead of `"join"`. This is actually a bug in SQLite that was fixed a
long time ago, but I was using the version of SQLite included by OS X
which has this bug. Since I'm guessing this will be a common case for
contributors, I also added an explicit check with a more helpful error
message.

Fixes #20360
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants