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

Use Arel for `IS NOT NULL` SQL condition #160

Closed jlestavel closed 1 year ago

jlestavel commented 1 year ago

This will properly format the SQL condition depending on the database

ryankopf commented 1 year ago

Ruby on Rail's arel is meant to be internal, it frequently contains breaking changes between Rails versions and it's not considered a best practice to use it in production code from my understanding.

Quoting from: https://blog.saeloun.com/2021/10/19/rails-arel-primer.html [..] reader be cautioned – Arel is still a private API provided by Rails. Meaning that future versions of Rails could be subject to changes in Arel. Fortunately, the changes have been limited to a minimum with the latest versions.

Does the current code not work on a particular database? Right now it does seem to produce the correctly quoted values.

jlestavel commented 1 year ago

Thanks for your comment. It seems that it does not "produce the correctly quoted values", at least for MySQL. If I execute the MySQL test suite, I have ActiveRecord::StatementInvalid errors:

bundle exec rake test_mysql

# Running:

E

Error:
ActsAsMappableTest#test_find_nearest:
ActiveRecord::StatementInvalid: Mysql2::Error: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near '.'lat' IS NOT NULL AND 'locations'.'lng' IS NOT NULL) ORDER BY 
          (ACOS(' at line 1
    ~/.rbenv/versions/3.1.0/lib/ruby/gems/3.1.0/gems/mysql2-0.5.5/lib/mysql2/client.rb:151:in `_query'
    ~/.rbenv/versions/3.1.0/lib/ruby/gems/3.1.0/gems/mysql2-0.5.5/lib/mysql2/client.rb:151:in `block in query'
    ~/.rbenv/versions/3.1.0/lib/ruby/gems/3.1.0/gems/mysql2-0.5.5/lib/mysql2/client.rb:150:in `handle_interrupt'
    ~/.rbenv/versions/3.1.0/lib/ruby/gems/3.1.0/gems/mysql2-0.5.5/lib/mysql2/client.rb:150:in `query'
    ~/.rbenv/versions/3.1.0/lib/ruby/gems/3.1.0/gems/activerecord-7.0.4.1/lib/active_record/connection_adapters/abstract_mysql_adapter.rb:632:in `block (2 levels) in raw_execute'
    ...
    ~/.rbenv/versions/3.1.0/lib/ruby/gems/3.1.0/gems/activerecord-7.0.4.1/lib/active_record/relation/finder_methods.rb:146:in `first'
    ~/Work/github/geokit-rails/test/acts_as_mappable_test.rb:177:in `test_find_nearest'

...

On my branch, the tests suite is 🟢

Ruby on Rail's arel is meant to be internal, it frequently contains breaking changes between Rails versions and it's not considered a best practice to use it in production code from my understanding.

In my understanding, Arel is a good abstraction for database agnosticism, so that sounds appropriate here where we want to support multiple databases.

It seems that commit 7c5be0ebe06b39f913d1dd57107a96b0fa87a68f introduced the issue.

ryankopf commented 1 year ago

I'm using MariaDB which seems to allow 'tablename'.'column' instead of using backticks. But since this change passes tests right now, I'm going to go ahead and approve it.