Add table name to search query #830
Conversation
Problem: When default scopes with joins are used, the generated search query can lead to ambiguity. For instance, the column `id` may exist in two tables, which will lead to a SQL error when it is impossible to decide which table to use. Solution: Add `resource_class`'s `table_name` to the generated search query. This ensures that the generated query produces the format `table_name.id` instead of `id` in the where clause generated for the search.
lib/administrate/search.rb
Outdated
| @@ -19,9 +19,10 @@ def run | |||
| private | |||
|
|
|||
| delegate :resource_class, to: :resolver | |||
| delegate :table_name, to: :resource_class | |||
There was a problem hiding this comment.
kind of thinking this was a bad idea, and that using resource_class.table_name might be a bit more clear. let me know what you think!
There was a problem hiding this comment.
I would use resource_class.table_name because you aren't' frequently changing data types as you chain and you don't expect resource_class to become possibly nil at any point.
lib/administrate/search.rb
Outdated
|
|
||
| def query | ||
| search_attributes.map { |attr| "lower(#{attr}) LIKE ?" }.join(" OR ") | ||
| search_attributes.map { |attr| "lower(#{table_name}.#{attr}) LIKE ?" }.join(" OR ") |
There was a problem hiding this comment.
@klaseskilson someone just opened a PR showing that this code doesn't align with the SQL standard in #829. Are you able to quote the table name and, just to leave things in better shape than they were before, the column name as well?
There was a problem hiding this comment.
Sure thing, on it.
spec/lib/administrate/search_spec.rb
Outdated
| resolver = double(resource_class: User, dashboard_class: MockDashboard) | ||
| search = Administrate::Search.new(resolver, "test") | ||
| expected_query = [ | ||
| "lower(name) LIKE ? OR lower(email) LIKE ?", | ||
| "lower(users.name) LIKE ? OR lower(users.email) LIKE ?", |
There was a problem hiding this comment.
The SQL should probably be:
lower("users"."name") LIKE ? OR lower("users"."email") LIKE ?
lib/administrate/search.rb
Outdated
| @@ -19,9 +19,10 @@ def run | |||
| private | |||
|
|
|||
| delegate :resource_class, to: :resolver | |||
| delegate :table_name, to: :resource_class | |||
There was a problem hiding this comment.
I would use resource_class.table_name because you aren't' frequently changing data types as you chain and you don't expect resource_class to become possibly nil at any point.
|
@BenMorganIO Pushed 63d01d0 with quoting. 👍 |
|
Note: fixes #829 |
|
Looks great! Thanks! |
* Add table name to search query Problem: When default scopes with joins are used, the generated search query can lead to ambiguity. For instance, the column `id` may exist in two tables, which will lead to a SQL error when it is impossible to decide which table to use. Solution: Add `resource_class`'s `table_name` to the generated search query. This ensures that the generated query produces the format `table_name.id` instead of `id` in the where clause generated for the search. * Quote table and column name in search Closes thoughtbot#829
Hi! As always, thank you for a great gem. Ran into a problem with a pretty straightforward fix. Hit me with your feedback! 👍
Problem
When default scopes with joins are used, the generated search query can lead to ambiguity. For instance, the column
idmay exist in two tables, which will lead to a SQL error when it is impossible to decide which table to use.Solution
Add
resource_class'stable_nameto the generated search query. This ensures that the generated query produces the formattable_name.idinstead ofidin the where clause generated for the search.