fix: Reliably quote columns/tables #1400
Conversation
| def quote(field) | ||
| "\"#{field.to_s}\"" |
There was a problem hiding this comment.
in pg, is basically quote_ident. for values, should be single-quotes like quote_literal
|
Not sure what the current failure means. Work so far is an improvement, I think.. not sure what to do next |
| # :nocov: | ||
| else | ||
| "#{table.to_s}_#{field.to_s}" | ||
| end | ||
| end | ||
| end | ||
|
|
||
| def quote_table_name(table_name) | ||
| if _model_class.try(:connection) |
There was a problem hiding this comment.
could be _model_class&.connection
There was a problem hiding this comment.
Yes, this would be better now that we don't support ruby versions older than 2.3
There was a problem hiding this comment.
I was thinking 'what if _model_class exists but doesn't respond to connection' but then I was thinking 'this is the ar resource, it's fine', but then I was thinking, ... eh
bf4
force-pushed
the
properly_quote
branch
2 times, most recently
from
7d61c8a to
8820e54
Compare
test/controllers/controller_test.rb
Outdated
| @@ -494,15 +494,15 @@ def test_sorting_by_relationship_field | |||
|
|
|||
| assert_response :success | |||
| assert json_response['data'].length > 10, 'there are enough records to show sort' | |||
| expected = Post | |||
There was a problem hiding this comment.
@lgebhardt rather than assert different rules here based on the value of adapter_sorts_nulls_last and try to reproduce the db behavior, I thought it would be sufficient to assert that the query results on the AR model give the same result as the resource.
Thoughts? I briefly tried using defining adapter_sorts_nulls_last then writing a transform in Ruby but I didn't duplicate the pg/mysql/sqlite logic and then thought, "why am I even?"
def adapter_specific_sort_comparison(l,r, sort_type:)
if l && r
comparison = l <=> r
case sort_type
when :sort then comparison
when :minus_sort then -comparison
else fail ArgumentError, "Unhandled sort_type #{sort_type} in #{__callee__}"
end
elsif l.nil? && r.nil?
0
elsif l.nil?
adapter_sorts_nulls_last ? -1 : 1
else
adapter_sorts_nulls_last ? 1 : -1
end
endThere was a problem hiding this comment.
technically this is out of scope of quoting and is part of "make mysql" tests pass.
There was a problem hiding this comment.
I extracted this into #1402
test/controllers/controller_test.rb
Outdated
|
|
||
| # Postgres sorts nulls last, whereas sqlite and mysql sort nulls first | ||
| if ENV['DATABASE_URL'].starts_with?('postgres') |
There was a problem hiding this comment.
I replaced all "what adapter is this" cases I found with "what behavior do I expect"
There was a problem hiding this comment.
I extracted this into #1402
|
@lgebhardt I decided to merge this since the target is a dev branch and this meets the goals I set re: quoting and now it can be the basis for further work |
|
@bf4 sounds good to me. I'm not sure how to best handle the adapter based sorting differences, but we can definitely handle that in a separate pr. |
* refactor: easily quote table/column * refactor: extract table name when missing * fix: Reliably quote columns/tables * refactor: putting quoting methods together * Handle special case of * - tests * fix: hack mysql test query comparison
* refactor: easily quote table/column * refactor: extract table name when missing * fix: Reliably quote columns/tables * refactor: putting quoting methods together * Handle special case of * - tests * fix: hack mysql test query comparison
see https://github.com/rails/rails/blob/7-0-stable/activerecord/lib/active_record/connection_adapters/abstract/quoting.rb#L8-L146
Addresses #1369