From ea7a6e3ab92c4a8e1bb0c9b7af1f730ebb2dc806 Mon Sep 17 00:00:00 2001 From: Julien Lestavel Date: Sat, 2 Jun 2018 20:16:18 +0200 Subject: [PATCH 1/2] Use Arel when it's possible --- lib/geokit-rails/acts_as_mappable.rb | 41 ++++++++++++++++++---------- test/acts_as_mappable_test.rb | 6 ++++ test/fixtures/companies.yml | 4 ++- test/schema.rb | 1 + 4 files changed, 37 insertions(+), 15 deletions(-) diff --git a/lib/geokit-rails/acts_as_mappable.rb b/lib/geokit-rails/acts_as_mappable.rb index 29649e3..40d3c1b 100644 --- a/lib/geokit-rails/acts_as_mappable.rb +++ b/lib/geokit-rails/acts_as_mappable.rb @@ -153,13 +153,16 @@ def by_distance(options = {}) units = extract_units_from_options(options) formula = extract_formula_from_options(options) distance_column_name = distance_sql(origin, units, formula) - with_latlng.order(Arel.sql( - "#{distance_column_name} #{options[:reverse] ? 'DESC' : 'ASC'}" - )) + with_latlng.order( + Arel.sql(distance_column_name).send(options[:reverse] ? 'desc' : 'asc') + ) end def with_latlng - where("#{qualified_lat_column_name} IS NOT NULL AND #{qualified_lng_column_name} IS NOT NULL") + where( + Arel.sql(qualified_lat_column_name).not_eq(nil) + .and(Arel.sql(qualified_lng_column_name).not_eq(nil)) + ) end def closest(options = {}) @@ -233,7 +236,7 @@ def distance_sql(origin, units=default_units, formula=default_formula) # If it's a :within query, add a bounding box to improve performance. # This only gets called if a :bounds argument is not otherwise supplied. def formulate_bounds_from_distance(options, origin, units) - distance = options[:within] if options.has_key?(:within) + distance = options[:within] if options.has_key?(:within) && options[:within].is_a?(Numeric) distance = options[:range].last-(options[:range].exclude_end?? 1 : 0) if options.has_key?(:range) if distance Geokit::Bounds.from_point_and_radius(origin,distance,:units=>units) @@ -248,22 +251,32 @@ def distance_conditions(options) formula = extract_formula_from_options(options) distance_column_name = distance_sql(origin, units, formula) - res = if options.has_key?(:within) - "#{distance_column_name} <= #{options[:within]}" + if options.has_key?(:within) + Arel.sql(distance_column_name).lteq(options[:within]) elsif options.has_key?(:beyond) - "#{distance_column_name} > #{options[:beyond]}" + Arel.sql(distance_column_name).gt(options[:beyond]) elsif options.has_key?(:range) - "#{distance_column_name} >= #{options[:range].first} AND #{distance_column_name} <#{'=' unless options[:range].exclude_end?} #{options[:range].last}" + min_condition = Arel.sql(distance_column_name).gteq(options[:range].begin) + max_condition = if options[:range].exclude_end? + Arel.sql(distance_column_name).lt(options[:range].end) + else + Arel.sql(distance_column_name).lteq(options[:range].end) + end + min_condition.and(max_condition) end - Arel::Nodes::SqlLiteral.new("(#{res})") if res.present? end def bound_conditions(bounds) + return nil unless bounds sw,ne = bounds.sw, bounds.ne - lng_sql = bounds.crosses_meridian? ? "(#{qualified_lng_column_name}<#{ne.lng} OR #{qualified_lng_column_name}>#{sw.lng})" : "#{qualified_lng_column_name}>#{sw.lng} AND #{qualified_lng_column_name}<#{ne.lng}" - res = "#{qualified_lat_column_name}>#{sw.lat} AND #{qualified_lat_column_name}<#{ne.lat} AND #{lng_sql}" - #Arel::Nodes::SqlLiteral.new("(#{res})") if res.present? - res if res.present? + lat, lng = Arel.sql(qualified_lat_column_name), Arel.sql(qualified_lng_column_name) + lat.gt(sw.lat).and(lat.lt(ne.lat)).and( + if bounds.crosses_meridian? + lng.lt(ne.lng).or(lng.gt(sw.lng)) + else + lng.gt(sw.lng).and(lng.lt(ne.lng)) + end + ) end # Extracts the origin instance out of the options if it exists and returns diff --git a/test/acts_as_mappable_test.rb b/test/acts_as_mappable_test.rb index 8dcbd8b..4167638 100644 --- a/test/acts_as_mappable_test.rb +++ b/test/acts_as_mappable_test.rb @@ -108,6 +108,12 @@ def test_find_within_with_coordinates assert_equal 5, locations.count end + def test_find_within_with_column_as_distance + locations = Location.joins(:company).within(Company.arel_table[:max_distance], origin: @loc_a) + assert_equal 4, locations.to_a.size + assert_equal 4, locations.count + end + def test_find_with_compound_condition #locations = Location.geo_scope(:origin => @loc_a).where("distance < 5 and city = 'Coppell'") locations = Location.within(5, :origin => @loc_a).where("city = 'Coppell'") diff --git a/test/fixtures/companies.yml b/test/fixtures/companies.yml index d48a2e8..6ecd654 100644 --- a/test/fixtures/companies.yml +++ b/test/fixtures/companies.yml @@ -1,7 +1,9 @@ starbucks: id: 1 name: Starbucks + max_distance: 1.7 barnes_and_noble: id: 2 - name: Barnes & Noble \ No newline at end of file + name: Barnes & Noble + max_distance: 2.0 diff --git a/test/schema.rb b/test/schema.rb index be3de5d..4e6bef5 100644 --- a/test/schema.rb +++ b/test/schema.rb @@ -1,6 +1,7 @@ ActiveRecord::Schema.define(:version => 0) do create_table :companies, :force => true do |t| t.column :name, :string + t.column :max_distance, :float end create_table :locations, :force => true do |t| From bc8f89df5928a5aba1f20dfb0dda556e0f2da2a0 Mon Sep 17 00:00:00 2001 From: Julien Lestavel Date: Sun, 3 Jun 2018 12:46:33 +0200 Subject: [PATCH 2/2] Do not use Arel `.not_eq(nil)` because of a bug in Arel 6 cf https://github.com/rails/arel/issues/354 --- lib/geokit-rails/acts_as_mappable.rb | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/lib/geokit-rails/acts_as_mappable.rb b/lib/geokit-rails/acts_as_mappable.rb index 40d3c1b..a31b0a6 100644 --- a/lib/geokit-rails/acts_as_mappable.rb +++ b/lib/geokit-rails/acts_as_mappable.rb @@ -159,10 +159,7 @@ def by_distance(options = {}) end def with_latlng - where( - Arel.sql(qualified_lat_column_name).not_eq(nil) - .and(Arel.sql(qualified_lng_column_name).not_eq(nil)) - ) + where("#{qualified_lat_column_name} IS NOT NULL AND #{qualified_lng_column_name} IS NOT NULL") end def closest(options = {})