Do not use Arel.star when ignored_columns
#27918
Conversation
activerecord/test/cases/base_test.rb
Outdated
|
|
||
| test "when #reload called, ignored columns' attribute methods are not defined" do | ||
| developer = Developer.create!(name: 'Developer') | ||
| refute developer.respond_to?(:first_name) |
There was a problem hiding this comment.
It looks like you could also use assert_not_respond_to developer, :first_name although I guess refute is used elsewhere in this file. Any reason to use one vs. the other?
There was a problem hiding this comment.
@composerinteralia No real reason, just went with what was used elsewhere.
| @@ -1042,7 +1042,11 @@ def build_select(arel) | |||
| if select_values.any? | |||
| arel.project(*arel_columns(select_values.uniq)) | |||
| else | |||
| arel.project(@klass.arel_table[Arel.star]) | |||
| if @klass.ignored_columns.any? | |||
| arel.project(*arel_columns(@klass.columns.map(&:name))) | |||
There was a problem hiding this comment.
There's a memoized column_names method you could use here.
| @@ -1042,7 +1042,11 @@ def build_select(arel) | |||
| if select_values.any? | |||
| arel.project(*arel_columns(select_values.uniq)) | |||
| else | |||
| arel.project(@klass.arel_table[Arel.star]) | |||
| if @klass.ignored_columns.any? | |||
There was a problem hiding this comment.
We don't need this conditional. Just always do column_names.
There was a problem hiding this comment.
We don't need it, but it does make for much nicer looking queries in the common case. 😕
maclover7
force-pushed
the
jm-fix-27914
branch
from
3823c47
to
84a7505
Compare
|
updated @eugeneius @sgrif @sgrif, once we're set on the new implementation in |
| arel.project(@klass.arel_table[Arel.star]) | ||
| end | ||
| columns_to_select = if select_values.any? | ||
| select_values.uniq |
There was a problem hiding this comment.
Can we just indent this 2 spaces rather than block indenting?
There was a problem hiding this comment.
Rubocop is asking this indentation. I believe because this rule is broken and needs to be fixed https://github.com/rails/rails/blob/master/.rubocop.yml#L116.
activerecord/test/cases/base_test.rb
Outdated
| refute developer.respond_to?(:first_name) | ||
| refute developer.respond_to?(:first_name=) | ||
|
|
||
| developer.reload |
There was a problem hiding this comment.
If we introduce changes to build_select, should we also assert that to_sql doesn't include the ignored column?
maclover7
force-pushed
the
jm-fix-27914
branch
from
84a7505
to
814e0d2
Compare
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. Re: the test I had to change, it should have never been passing in the first place, due to the ignored `first_name` column on `Developer.
maclover7
force-pushed
the
jm-fix-27914
branch
from
814e0d2
to
8717d9d
Compare
| @@ -1042,7 +1042,11 @@ def build_select(arel) | |||
| if select_values.any? | |||
| arel.project(*arel_columns(select_values.uniq)) | |||
| else | |||
|
@maclover7 can you take a look at @rafaelfranca's feedback? |
|
Providing some feedback, I'm using this patch in production for a couple weeks now and everything is working as expected 👍 |
Do not use `Arel.star` when `ignored_columns`
|
@sobrinho feel free to grab this commit, do the changes necessary, fix the tests and open a new PR. |
|
@rafaelfranca thanks, done in #30980, this one can be closed now. |
Summary
If there are any ignored columns, we will now list out all columns we
want to be returned from the database.
Other Information
first place, due to the ignored
first_namecolumn on `Developer.