Fixed PG::InvalidColumnReference with DISTINCT & by_distance #127
Conversation
…ther with 'by_distance' scope
|
|
||
| distance_formula = distance_sql(origin, units, formula).gsub(/\s+/, '') | ||
| with_latlng.select("#{arel.quoted_table_name}.*", "#{distance_formula} AS #{distance_column_name}") | ||
| .order("#{distance_column_name} #{options[:reverse] ? 'DESC' : 'ASC'}") |
There was a problem hiding this comment.
Place the . on the previous line, together with the method call receiver.
Line is too long. [90/80]
| arel = self.is_a?(ActiveRecord::Relation) ? self : self.all | ||
|
|
||
| distance_formula = distance_sql(origin, units, formula).gsub(/\s+/, '') | ||
| with_latlng.select("#{arel.quoted_table_name}.*", "#{distance_formula} AS #{distance_column_name}") |
|
|
||
| arel = self.is_a?(ActiveRecord::Relation) ? self : self.all | ||
|
|
||
| distance_formula = distance_sql(origin, units, formula).gsub(/\s+/, '') |
There was a problem hiding this comment.
Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.
| distance_column_name = distance_sql(origin, units, formula) | ||
| with_latlng.order("#{distance_column_name} #{options[:reverse] ? 'DESC' : 'ASC'}") | ||
|
|
||
| arel = self.is_a?(ActiveRecord::Relation) ? self : self.all |
1 similar comment
|
Looks like |
|
I'm closing until we fix this. But I'm absolutely happy to merge Pull Requests which pass the tests, maintains (or improves test coverage) and the code itself seems reasonable. |
|
It's been a long time since I created this so I forgot much of the context. Your main objection, in that case, is that it decreased the test coverage, right? |
|
Yes, I generally have 2 rules, it must pass the tests, and it should test the code that is changing in most cases. Given coverage has dropped I presume the entire method is untested. I would at least like people proposing changing code to attempt to test their change. Because in future someone might change this code and break your setup and we won't know until we do a release and then it's too late. Finally the gsub doesn't appear to be explained as to why it's needed. It would be great to explain this (it's good to do things in separate commits if they are different). The other question would be should we fix the gsub within the distance_sql method because if we call it in other places it would be wrong then wouldn't it? (because other callers will forget to gsub) |
I am using PG and Rails 5.1.
I had a pretty complex statement that used both
.distinctand the.by_distancescope, and I was getting this error:PG::InvalidColumnReference: ERROR: for SELECT DISTINCT, ORDER BY expressions must appear in select list
I dug down a bit and found that it was happening because the formula was right in the ORDER BY clause, and Postgres was having none of it.
This PR is a fix that worked for me. Whether it's something should be merged in, I don't know. But it did fix a bug for me, and it may for others, as well. If there is some further refactoring I could do to get this merged, let me know.
Basically, I am looking for feedback.