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.

Sign up for GitHub

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

Jump to bottom

Quote table_ and column_name for sorting #3652

Merged
merged 5 commits into from
Feb 26, 2024
Merged

Quote table_ and column_name for sorting #3652

merged 5 commits into from
Feb 26, 2024

Conversation

rnestler
Copy link
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
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)
Copy link
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?

Copy link
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
Copy link
Contributor Author

Any update on this?

mshibuya
mshibuya reviewed Oct 28, 2023
View reviewed changes
Copy link
Member

@mshibuya mshibuya left a comment

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.

lib/rails_admin/abstract_model.rb Outdated Show resolved Hide resolved
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)
Copy link
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
Copy link

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
19 checks passed
@mshibuya
Copy link
Member

Great job, thank you!

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

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

@mshibuya
Copy link
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