Skip to content

Quote table_ and column_name for sorting#3652

Merged
mshibuya merged 5 commits into
railsadminteam:masterfrom
rnestler:fix/1631-quote-column-names
Feb 26, 2024
Merged

Quote table_ and column_name for sorting#3652
mshibuya merged 5 commits into
railsadminteam:masterfrom
rnestler:fix/1631-quote-column-names

Conversation

@rnestler

@rnestler rnestler commented Oct 13, 2023

Copy link
Copy Markdown
Contributor

Fixes #1631.

@rnestler rnestler force-pushed the fix/1631-quote-column-names branch 4 times, most recently from f5f7d8a to 51cf5a4 Compare October 13, 2023 13:52
rnestler
rnestler commented Oct 13, 2023
View reviewed changes
Comment thread spec/controllers/rails_admin/main_controller_spec.rb
Comment on lines -76 to +83
expect(controller.send(:get_sort_hash, RailsAdmin.config(Player))).to eq(sort: 'players.team_id', sort_reverse: true)
expect(controller.send(:get_sort_hash, RailsAdmin.config(Player))).to match(sort: /.?players.?\..?team_id.?/, sort_reverse: true)
end
end

context 'using active_record, supporting joins', active_record: true do
it 'gives back the local column' do
controller.params = {sort: 'team', model_name: 'players'}
expect(controller.send(:get_sort_hash, RailsAdmin.config(Player))).to eq(sort: 'teams.name', sort_reverse: true)
expect(controller.send(:get_sort_hash, RailsAdmin.config(Player))).to match(sort: /.?teams.?\..?name.?/, sort_reverse: true)

@rnestler rnestler Oct 13, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Maybe I should also add a regression spec with a model with capitalized fields? Where would I put it?

@mshibuya mshibuya Oct 28, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Somewhere around here will be good, as this fix is about #get_sort_hash.
Also you can create RDBMS-specific test cases like:

rails_admin/spec/rails_admin/support/csv_converter_spec.rb

Lines 69 to 75 in 0e12e5b

case connection_config[:adapter]
when 'postgresql'
@connection = ActiveRecord::Base.connection.instance_variable_get(:@connection)
@connection.set_client_encoding('latin1')
when 'mysql2'
ActiveRecord::Base.connection.execute('SET NAMES latin1;')
end

@rnestler

rnestler commented Oct 27, 2023

Copy link
Copy Markdown
Contributor Author

Any update on this?

mshibuya
mshibuya reviewed Oct 28, 2023
View reviewed changes

@mshibuya mshibuya left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for the PR! I've made some comments.

Comment thread lib/rails_admin/abstract_model.rb Outdated
Comment thread spec/controllers/rails_admin/main_controller_spec.rb
Comment on lines -76 to +83
expect(controller.send(:get_sort_hash, RailsAdmin.config(Player))).to eq(sort: 'players.team_id', sort_reverse: true)
expect(controller.send(:get_sort_hash, RailsAdmin.config(Player))).to match(sort: /.?players.?\..?team_id.?/, sort_reverse: true)
end
end

context 'using active_record, supporting joins', active_record: true do
it 'gives back the local column' do
controller.params = {sort: 'team', model_name: 'players'}
expect(controller.send(:get_sort_hash, RailsAdmin.config(Player))).to eq(sort: 'teams.name', sort_reverse: true)
expect(controller.send(:get_sort_hash, RailsAdmin.config(Player))).to match(sort: /.?teams.?\..?name.?/, sort_reverse: true)

@mshibuya mshibuya Oct 28, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Somewhere around here will be good, as this fix is about #get_sort_hash.
Also you can create RDBMS-specific test cases like:

rails_admin/spec/rails_admin/support/csv_converter_spec.rb

Lines 69 to 75 in 0e12e5b

case connection_config[:adapter]
when 'postgresql'
@connection = ActiveRecord::Base.connection.instance_variable_get(:@connection)
@connection.set_client_encoding('latin1')
when 'mysql2'
ActiveRecord::Base.connection.execute('SET NAMES latin1;')
end

@rnestler rnestler force-pushed the fix/1631-quote-column-names branch from 51cf5a4 to 68299c0 Compare October 30, 2023 17:00
@coveralls

coveralls commented Oct 30, 2023

Copy link
Copy Markdown

Coverage Status

coverage: 96.021% (-0.02%) from 96.037% when pulling 68299c0 on rnestler:fix/1631-quote-column-names into 13c90fd on railsadminteam:master.

@mshibuya mshibuya merged commit b8155f8 into railsadminteam:master Feb 26, 2024
@mshibuya

mshibuya commented Feb 26, 2024

Copy link
Copy Markdown
Member

Great job, thank you!

@rnestler rnestler deleted the fix/1631-quote-column-names branch February 26, 2024 09:51
@rnestler

rnestler commented Feb 26, 2024

Copy link
Copy Markdown
Contributor Author

Ah sorry I forgot about that. I think I didn't add the regression test or RDBMS-specific test cases.

@mshibuya

mshibuya commented Feb 26, 2024

Copy link
Copy Markdown
Member

I'll handle that when I have time 👍

mshibuya added a commit that referenced this pull request Mar 2, 2024
@mshibuya
Follow-up for #3652
d1ae021 
Add a test case to ensure the table and column names are quoted
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mshibuya mshibuya mshibuya left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Table names don't always get properly quoted

3 participants

@rnestler @coveralls @mshibuya
⚡