Adds geo_distance query to opensearch #781

JPrevost · 2024-02-05T15:59:06Z

This has not yet been integrated with GraphQL, so the best way to validate this work is to use the rails console.

bin/rails console
q = {geopoint:{distance: "1mi", latitude: "45", longitude: "-112"}}
Opensearch.new.search(0, q, Timdex::OSClient)

You can also combine keyword (or filters) using our existing query syntax:

q = {geopoint:{distance: "1mi", latitude: "45", longitude: "-112"}, searchterm: "train stations"}

Relevant ticket(s):

https://mitlibraries.atlassian.net/browse/GDT-135

How does this address that need:

Adds an optional geo_distance search to our existing opensearch query builder

Document any side effects to this change:

rubocop config has been updated to the current version of ruby we are using
many opensearch tests were updated as the previous versions continued to pass even if I changed the expected values

Developer

All new ENV is documented in README
All new ENV has been added to Heroku Pipeline, Staging and Prod
ANDI or Wave has been run in accordance to
our guide and
all issues introduced by these changes have been resolved or opened as new
issues (link to those issues in the Pull Request details above)
Stakeholder approval has been confirmed (or is not needed)

Code Reviewer

The commit message is clear and follows our guidelines
(not just this pull request message)
There are appropriate tests covering any new functionality
The documentation has been updated or is unnecessary
The changes have been verified
New dependencies are appropriate or there were no changes

Requires database migrations?

NO

Includes new or updated dependencies?

NO

JPrevost · 2024-02-05T16:29:23Z

FYI: code climate is being weird. Running the latest rubocop locally does not show the same errors as in code climate. They seem to be behind in versions (again) in a way that feels hard to work around. I'd like to propose we address that separately from this PR though.

JPrevost · 2024-02-05T17:58:55Z

Latest commit seems to have CodeClimate's rubocop being less weird... although now it's complaining about things not part of this PR. I'm hoping it will stabilize-ish after this merges maybe?

JPrevost · 2024-02-05T19:16:59Z

app/models/opensearch.rb

@@ -109,9 +109,30 @@ def matches
    match_single_field_nested(:identifiers, m)
    match_single_field_nested(:locations, m)
    match_single_field_nested(:subjects, m)
+
+    match_geopoint(m) if @params[:geopoint].present?


@matt-bernhardt did we agree on geodistance for this parameter? If so I can update this. I believe my code is from before we met to discuss naming for this feature.

I remember talking about the name, and about there potentially being a value in avoiding a terminology clash with what's used in the field (i.e. we use geopoint with three arguments, when in the field a geopoint only has two attributes).

That said, I'm not super sure that this is actually a problem outside of my head. is it worth asking Daniel or any of the GIS staff whether they think this would cause confusion if someone tries to work with our API directly?

I think I want to make something work before we bring them in on naming. I think since OpenSearch calls this a geodistance search we had agreed to that and I just hadn't updated the code. I'll make this geodistance as the parameter name we agree to for now and once implemented fully in graphql we can get feedback. Thanks!

matt-bernhardt

I have a question about what seems like a piece of syntactic sugar that we're relying on now, just to confirm that this is what's happening - but I don't think this is a blocking concern.

The feature itself being introduced seems to function as intended - I've run a few different queries in the console, at different locations and different distances, and it seems like it works as intended.

As long as I'm right about the syntactic sugar, then I'm

matt-bernhardt · 2024-02-05T20:28:27Z

app/models/opensearch.rb

-      query: query,
-      aggregations: aggregations
+      query:,
+      aggregations:


I suspect that this is a syntactic sugar in Rails that I've forgotten - but the change in this hash is because the query and aggregations methods are called implicitly, and from is populated auto-magically?

Correct. When I changed the target ruby version in rubocop, this changed automatically when I save the file and seems to be the new preferred way of handling this. If we find we don't like it, we can decide to ignore the new rule. Other than being surprised it now works like this, I think I can learn to appreciate it.

matt-bernhardt · 2024-02-05T20:29:03Z

app/models/opensearch.rb

@@ -177,7 +198,7 @@ def source_array(param)
    param.each do |source|
      sources << {
        term: {
-          source: source
+          source:


Same question here as above - this is now being called implicitly?

matt-bernhardt · 2024-02-05T20:33:28Z

test/models/opensearch_test.rb

@@ -4,59 +4,66 @@ class OpensearchTest < ActiveSupport::TestCase
  test 'matches citation' do
    os = Opensearch.new
    os.instance_variable_set(:@params, { citation: 'foo' })
-    assert os.matches.select { |m| m[:citation] == 'foo' }
+
+    assert(os.matches.to_json.include?('{"match":{"citation":"foo"}}'))


I've confirmed that changing something in this test causes it to fail, but not that it was broken before. I'm happy to know that this test syntax is now fail-able, though.

JPrevost · 2024-02-05T21:06:28Z

Thanks for the review @matt-bernhardt . I'm going to change the name from geopoint to geodistance as a new commit now

JPrevost · 2024-02-05T21:13:21Z

@matt-bernhardt I've made the name changes if you would mind confirming the final commit makes sense to you before I squashy squash. Thanks!

matt-bernhardt

Yep, that looks good to me. I've run the new search name in the console and saw results come back.

Thanks!

Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/GDT-135 How does this address that need: * Adds an optional geo_distance search to our existing opensearch query builder Document any side effects to this change: * rubocop config has been updated to a more recent version of ruby * codeclimate rubocop channel has been updated to the latest available. It is still a version behind rubocop itself. https://github.com/codeclimate/codeclimate-rubocop/branches/all?utf8=✓&query=channel%2Frubocop * many opensearch tests were updated as the previous versions continued to pass even if I changed the expected values

mitlib temporarily deployed to timdex-pr-781 February 5, 2024 16:03 Inactive

JPrevost temporarily deployed to timdex-pr-781 February 5, 2024 16:04 Inactive

JPrevost requested review from matt-bernhardt and jazairi February 5, 2024 16:29

JPrevost temporarily deployed to timdex-pr-781 February 5, 2024 17:57 Inactive

JPrevost commented Feb 5, 2024

View reviewed changes

matt-bernhardt self-assigned this Feb 5, 2024

matt-bernhardt approved these changes Feb 5, 2024

View reviewed changes

JPrevost temporarily deployed to timdex-pr-781 February 5, 2024 21:08 Inactive

matt-bernhardt approved these changes Feb 6, 2024

View reviewed changes

JPrevost force-pushed the gdt-135-geodistance branch from 0c70302 to 726b2d7 Compare February 6, 2024 15:17

JPrevost temporarily deployed to timdex-pr-781 February 6, 2024 15:17 Inactive

JPrevost merged commit d267e10 into main Feb 8, 2024
3 of 4 checks passed

JPrevost deleted the gdt-135-geodistance branch February 8, 2024 17:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds geo_distance query to opensearch #781

Adds geo_distance query to opensearch #781

JPrevost commented Feb 5, 2024 •

edited by matt-bernhardt

Loading

JPrevost commented Feb 5, 2024

JPrevost commented Feb 5, 2024 •

edited

Loading

JPrevost Feb 5, 2024

matt-bernhardt Feb 5, 2024

JPrevost Feb 5, 2024

matt-bernhardt left a comment

matt-bernhardt Feb 5, 2024

JPrevost Feb 5, 2024

matt-bernhardt Feb 5, 2024

JPrevost Feb 5, 2024

matt-bernhardt Feb 5, 2024

JPrevost commented Feb 5, 2024

JPrevost commented Feb 5, 2024

matt-bernhardt left a comment

Adds geo_distance query to opensearch #781

Adds geo_distance query to opensearch #781

Conversation

JPrevost commented Feb 5, 2024 • edited by matt-bernhardt Loading

Developer

Code Reviewer

Requires database migrations?

Includes new or updated dependencies?

JPrevost commented Feb 5, 2024

JPrevost commented Feb 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matt-bernhardt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JPrevost commented Feb 5, 2024

JPrevost commented Feb 5, 2024

matt-bernhardt left a comment

Choose a reason for hiding this comment

JPrevost commented Feb 5, 2024 •

edited by matt-bernhardt

Loading

JPrevost commented Feb 5, 2024 •

edited

Loading