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

Quote colum_names when building select: #31403

Merged
merged 1 commit into from Dec 12, 2017

Conversation

@Edouard-chin
Copy link
Member

@Edouard-chin Edouard-chin commented Dec 11, 2017

Quote colum_names when building select:

@rails-bot
Copy link

@rails-bot rails-bot commented Dec 11, 2017

r? @kaspth

(@rails-bot has picked a reviewer for you, use r? to override)

@Edouard-chin Edouard-chin force-pushed the Edouard-chin:fix-quoted-columnname branch 3 times, most recently Dec 11, 2017
activerecord/test/cases/base_test.rb Outdated
query = Developer.from("`developers`").to_sql
quoted_id = Developer.connection.quote_table_name("id")

assert_match /SELECT #{quoted_id}.* FROM `developers`/, query

This comment has been minimized.

@rafaelfranca

rafaelfranca Dec 11, 2017
Member

This generates a ruby warning. We need a () around the arguments. Also can we hardcode quoted_id instead of using a variable? It will make the test easier to understand.

This comment has been minimized.

@Edouard-chin

Edouard-chin Dec 11, 2017
Author Member

Sounds good for the ()

I added the dynamic quoted_id because based on the adapter the quoting is different. MySQL uses backticks while other uses simple ". I can make a conditional if you prefer

This comment has been minimized.

@rafaelfranca

rafaelfranca Dec 11, 2017
Member

Oh. That is true. Let's only add the ()

- #30980 introcuded a change to not use `Arel.star` when model have ignored columns, a query used to look like `SELECT *. FROM developers` whereas now it would like `SELECT column1, column2 FROM developers`
  - If a column has the same name has a reserved database specific keyword (such as key, where ...) then the query would fail because the names aren't quoted
- Quoting almost always happen unless we use a `from` clause in the query https://github.com/rails/rails/blob/9965b98dc0d58a86e10b4343bb6e15e01661a8c3/activerecord/lib/active_record/relation/query_methods.rb#L1052
- This PR cast all columns name to symbols in order for the quoting logic to be picked up https://github.com/rails/rails/blob/9965b98dc0d58a86e10b4343bb6e15e01661a8c3/activerecord/lib/active_record/relation/query_methods.rb#L1054-L1055
- A reproduction script can be found here https://gist.github.com/Edouard-chin/f56d464a0adcb76962afc1a9134a1536
@Edouard-chin Edouard-chin force-pushed the Edouard-chin:fix-quoted-columnname branch to 2d78c66 Dec 11, 2017
@Edouard-chin
Copy link
Member Author

@Edouard-chin Edouard-chin commented Dec 12, 2017

Done thanks 🙇

@rafaelfranca rafaelfranca merged commit a681eaf into rails:master Dec 12, 2017
2 checks passed
2 checks passed
codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
rafaelfranca added a commit that referenced this pull request Dec 12, 2017
Quote colum_names when building select:
@Edouard-chin Edouard-chin deleted the Edouard-chin:fix-quoted-columnname branch Dec 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.