Fixed PG::InvalidColumnReference with DISTINCT & by_distance

geokit / geokit-rails

Official Geokit plugin for Rails/ActiveRecord. Provides location-based goodness for your Rails app. Requires the Geokit gem.

MIT License

1.57k stars 245 forks source link

Fixed PG::InvalidColumnReference with DISTINCT & by_distance #127

Closed elsurudo closed 6 years ago

elsurudo commented 6 years ago

I am using PG and Rails 5.1.

I had a pretty complex statement that used both .distinct and the .by_distance scope, 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.

coveralls commented 6 years ago

Coverage decreased (-0.2%) to 48.617% when pulling b63b3d95444d9671a6c420c02d574876bec4c075 on elsurudo:order-by-fix into 9e5251afb76f14a5a2cd975023f0a0fab1e0cc80 on geokit:master.

coveralls commented 6 years ago

Coverage decreased (-0.2%) to 48.617% when pulling b63b3d95444d9671a6c420c02d574876bec4c075 on elsurudo:order-by-fix into 9e5251afb76f14a5a2cd975023f0a0fab1e0cc80 on geokit:master.

elsurudo commented 6 years ago

Looks like self.all is a problem because it returns an array instead of a relation. Confused because scoped was removed from Rails. I'm sure there is a way to fix this.

mnoack commented 6 years ago

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.

elsurudo commented 6 years ago

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?

mnoack commented 6 years ago

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)

onwardmk commented 2 weeks ago

I am experiencing this issue. Is there a workaround?