search

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

Error: syntax error near DESC #40

Closed nneal closed 11 years ago

nneal commented 11 years ago

Been trying to fix some test errors on the Rails4 branch. Just compiling some info here.

ActsAsMappableTest#test_scoped_distance_column_in_select
ActiveRecord::StatementInvalid: PG::SyntaxError: ERROR:  syntax error at or near "DESC"
LINE 1: ...ations"."company_id" = $1  ORDER BY (ACOS(least(1 DESC, COS(...

also occurs in:

ActsAsMappableTest#test_distance_column_in_select
ActsAsMappableTest#test_ip_geocoded_distance_column_in_select

Occuring on Postgres and MySQL

It appears that the problem is introduced when ActiveRecord splits strings with a comma to inject ASC/DESC here: https://github.com/rails/rails/blob/master/activerecord/lib/active_record/relation/query_methods.rb#L994

I'm not sure if it's truly an ActiveRecord error or if we are passing in a malformed order string.

The order passed in is: 

"\n          (ACOS(least(1,COS(0.5745378329739577)*COS(-1.692244085410541)*COS(RADIANS(locations.lat))*COS(RADIANS(locations.lng))+\n          COS(0.5745378329739577)*SIN(-1.692244085410541)*COS(RADIANS(locations.lat))*SIN(RADIANS(locations.lng))+\n          SIN(0.5745378329739577)*SIN(RADIANS(locations.lat))))*3963.19)\n          asc"

@mnoack can I assume that all the tests on Rails4 branch are up to date?

mnoack commented 11 years ago

@NealKemp The last build was https://travis-ci.org/geokit/geokit-rails/builds/10199966 (rails 3 works in that build but not rails 4, precisely because of this issue)

Thanks for going into the detail to find why rails 4 breaks this function. That'll help it get fixed.

I might have a look at it, but perhaps there's another way to send in our order clause so it doesn't have the double DESC statements, or maybe we could do a pull request on rails to fix this?, e.g.

I assume it doing it for this reason: 'first ASC, second DESC' -> 'first DESC, second ASC' or something like that

Maybe the rails order method needs to take into account brackets, etc. to deal with our more complex case.

nneal commented 11 years ago

I just looked at some of the tests and it looks like we're using a deprecated method all which may be the root of some of our problems.

If, in the tests, we switch to using to_a which is recommended, the tests pass. Also, it gets rid of the thousands of warnings I get when running the tests.

Should we make the assumption that people should be using the to_a and not the deprecated all or do you think we should make tests pass using all. I'd lean towards the former since this is a Rails 4 branch

mnoack commented 11 years ago

Looks like your PR has fixed this. Excellent work @NealKemp