Do not use `Arel.star` when `ignored_columns` #30980

sobrinho · 2017-10-24T23:02:05Z

rails-bot · 2017-10-24T23:02:08Z

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @kaspth (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review.

Please see the contribution instructions for more information.

sobrinho · 2017-10-24T23:02:45Z

cc @rafaelfranca

rafaelfranca · 2017-10-25T15:24:41Z

Let me know when the tests are green

sobrinho · 2017-10-25T18:22:20Z

@rafaelfranca the failures aren't related to this change anymore (not sure why they are happening at all now, maybe an intermittent failure).

I'm not sure if this is going to the right direction, can you please review the test changes and confirm this is the way to go?

rafaelfranca

Changes looks good to me. Only one question. Restarted the jobs to see if they now pass.

rafaelfranca · 2017-10-25T20:02:16Z

activerecord/test/cases/scoping/default_scoping_test.rb

@@ -118,76 +118,76 @@ def test_unscope_after_reordering_and_combining
  end

  def test_unscope_with_where_attributes
-    expected = Developer.order("salary DESC").collect(&:name)
-    received = DeveloperOrderedBySalary.where(name: "David").unscope(where: :name).collect(&:name)
+    expected = Developer.order("salary DESC").order(:name).collect(&:name)


Do you remember why we need to add a new order here?

The build was resulting in a different order between expected and received, I normalized both by using a predictable order.

Not sure why it started to fail in this PR but I have assumed that Postgres changed the order of the results for some reason (there is no guarantee in the order of the results in Postgres unless specified, in that case, if the salary is the same we do not have a guaranteed order of the records [there is but it is the order of the record on the disk that may vary from time to time]).

In the other hand, I'm not sure if this is the real problem but since this PR change should not impact on this test at all I assumed it was that.

might be better to do assert_equal(expected.sort, received.sort)

Yeah, we've used sort and sort_by for that. 75cfb3d 93c65ae 625bdb1

Pretty much the same, should I change it to sort in ruby instead of the database?

I prefer to because of the already existing order that we have on those tests

sobrinho · 2017-10-25T22:10:19Z

@rafaelfranca the build is green now 🤘

kamipo · 2017-10-26T04:37:48Z

activerecord/test/cases/adapters/mysql2/explain_test.rb

-    assert_match %(EXPLAIN for: SELECT `developers`.* FROM `developers` WHERE `developers`.`id` = 1), explain
-    assert_match %r(developers |.* const), explain
+    explain = Author.where(id: 1).explain
+    assert_match %(EXPLAIN for: SELECT `authors`.* FROM `authors` WHERE `authors`.`id` = 1), explain


Why are you changing mysql2's tests only from Developer to Author?

The original author wanted to keep the EXPLAIN with the STAR and for that, the model had to be changed.

I can update the EXPLAIN test to keep the Developer model and update the regexp to match against the new query.

What do you think?

I think it is better to fix all other explain tests to use Author to not have a fragile test that will have to change every time the list of ignored column at Developers change.

I'm thinking of creating a specific model for that feature, in that way the model will be exclusive for that feature and won't break anything around that.

Or even use something like that:

test '...' do begin Developer.ignored_columns = [:first_name, :last_name] assertions here ensure Developer.ignored_columns = [] end end

What do you think?

I think changing to author is a good solution already that doesn't require adding any new code.

So change the other specs to use author too?

kamipo · 2017-10-26T04:40:41Z

activerecord/test/cases/scoping/default_scoping_test.rb

    assert_equal 1, scope.where_clause.ast.children.length
-    assert_equal Developer.where(name: "David"), scope
+    assert_equal DeveloperCalledJamis.where("salary < 10000").to_a, scope.to_a


Is this change necessary?

I have no idea, I left as the original author wrote, I will try to revert that and see what happen.

sobrinho · 2017-11-12T18:06:37Z

@rafaelfranca can you take a look again?

rafaelfranca

👍 Can you squash your commits?

sobrinho · 2017-11-13T19:04:33Z

@rafaelfranca like this?

rafaelfranca · 2017-11-13T19:12:29Z

This works, but you could at least three commits in your PR because they were atomic.

If there are any ignored columns, we will now list out all columns we want to be returned from the database. Includes a regression test.

sobrinho · 2017-11-13T19:43:03Z

@rafaelfranca done!

rafaelfranca · 2017-11-13T20:22:43Z

Thanks! I'll merge in a minute

…lumns Do not use `Arel.star` when `ignored_columns`

- rails#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

rails-bot assigned kaspth Oct 24, 2017

sobrinho mentioned this pull request Oct 24, 2017

Do not use Arel.star when ignored_columns #27918

Closed

sobrinho force-pushed the sobrinho/arel-star-ignored-columns branch 4 times, most recently from e956302 to f8bd7e1 Compare October 25, 2017 12:11

rafaelfranca assigned rafaelfranca and unassigned kaspth Oct 25, 2017

rafaelfranca added the activerecord label Oct 25, 2017

sobrinho force-pushed the sobrinho/arel-star-ignored-columns branch 2 times, most recently from ab9fd5b to fe42e50 Compare October 25, 2017 17:55

rafaelfranca reviewed Oct 25, 2017

View reviewed changes

kamipo reviewed Oct 26, 2017

View reviewed changes

sobrinho mentioned this pull request Nov 13, 2017

AR::Base#reload and ignored_columns #27914

Closed

rafaelfranca requested changes Nov 13, 2017

View reviewed changes

sobrinho force-pushed the sobrinho/arel-star-ignored-columns branch from ab25bad to 736cbea Compare November 13, 2017 19:04

maclover7 and others added 3 commits November 13, 2017 17:41

Do not use Arel.star when ignored_columns

6acde95

If there are any ignored columns, we will now list out all columns we want to be returned from the database. Includes a regression test.

Change tests to use models which don't ignore any columns

54e1018

Fix postgres ordering issue on default scoping test

f8627df

sobrinho force-pushed the sobrinho/arel-star-ignored-columns branch from 736cbea to f8627df Compare November 13, 2017 19:42

rafaelfranca approved these changes Nov 13, 2017

View reviewed changes

rafaelfranca merged commit f49d594 into rails:master Nov 13, 2017

rafaelfranca added a commit that referenced this pull request Nov 13, 2017

Merge pull request #30980 from sobrinho/sobrinho/arel-star-ignored-co…

ce0787f

…lumns Do not use `Arel.star` when `ignored_columns`

sobrinho deleted the sobrinho/arel-star-ignored-columns branch November 13, 2017 21:44

yahonda mentioned this pull request Nov 15, 2017

Allow test_ignored_columns_not_included_in_SELECT column names case insensitive #31160

Merged

Edouard-chin mentioned this pull request Dec 11, 2017

Quote colum_names when building select: #31403

Merged

kamipo mentioned this pull request Apr 26, 2018

ignored_columns does not apply to ActiveRecord::Base#attributes #32729

Closed

sircpl mentioned this pull request Mar 21, 2022

[R5U-2250] Request for Implementation: Support for Rails >= 5.1.7 ignored_columns zendesk/kasket#65

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not use `Arel.star` when `ignored_columns` #30980

Do not use `Arel.star` when `ignored_columns` #30980

sobrinho commented Oct 24, 2017

rails-bot commented Oct 24, 2017

sobrinho commented Oct 24, 2017

rafaelfranca commented Oct 25, 2017

sobrinho commented Oct 25, 2017

rafaelfranca left a comment

rafaelfranca Oct 25, 2017

sobrinho Oct 25, 2017

joevandyk Oct 26, 2017

kamipo Oct 26, 2017

sobrinho Oct 26, 2017

rafaelfranca Oct 26, 2017

sobrinho commented Oct 25, 2017

kamipo Oct 26, 2017

sobrinho Oct 26, 2017

rafaelfranca Oct 26, 2017

sobrinho Oct 26, 2017 •

edited

rafaelfranca Nov 6, 2017

sobrinho Nov 7, 2017

rafaelfranca Nov 7, 2017

kamipo Oct 26, 2017

sobrinho Oct 26, 2017

sobrinho commented Nov 12, 2017

rafaelfranca left a comment

sobrinho commented Nov 13, 2017

rafaelfranca commented Nov 13, 2017

sobrinho commented Nov 13, 2017

rafaelfranca commented Nov 13, 2017

Do not use Arel.star when ignored_columns #30980

Do not use Arel.star when ignored_columns #30980

Conversation

sobrinho commented Oct 24, 2017

rails-bot commented Oct 24, 2017

sobrinho commented Oct 24, 2017

rafaelfranca commented Oct 25, 2017

sobrinho commented Oct 25, 2017

rafaelfranca left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sobrinho commented Oct 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sobrinho Oct 26, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sobrinho commented Nov 12, 2017

rafaelfranca left a comment

Choose a reason for hiding this comment

sobrinho commented Nov 13, 2017

rafaelfranca commented Nov 13, 2017

sobrinho commented Nov 13, 2017

rafaelfranca commented Nov 13, 2017

Do not use `Arel.star` when `ignored_columns` #30980

Do not use `Arel.star` when `ignored_columns` #30980

sobrinho Oct 26, 2017 •

edited