Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update GraphQL to better support Locations #784

Merged
merged 2 commits into from
Feb 13, 2024
Merged

Update GraphQL to better support Locations #784

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c00a14a
Replace GeoPoint with GeoShape as attribute of LocationType
matt-bernhardt Feb 5, 2024
cdf20f7
Address code review feedback
matt-bernhardt Feb 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion app/graphql/types/record_type.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,9 +108,14 @@ class AlternateTitleType < Types::BaseObject

# Warning: related_place was supposed to be an array but was incorrectly a string in graphql for v1
class LocationType < Types::BaseObject
field :geopoint, String, description: 'GeoPoint data for the location, if applicable'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we discussed on zoom, let's try to deprecate geopoint as a matter of best practice even though it is very unlikely anyone is using it.

So the we'd mark the field as deprecated and suggest using the new field while still mapping the new field value to the old field so people still get what they asked for either way. See https://github.com/MITLibraries/timdex/blob/main/app/graphql/types/record_type.rb#L34-L36 for an example of how we have done that on other fields that got renamed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure thing - I've switched from a replacement to a deprecation for geopoint now. The test has been updated to include it as well. I see the deprecation announcement in the help docs, but don't see it in the actual API response, so I don't yet have a way to test for the deprecation itself.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need an explicit test for the deprecation at this point. Manually confirm it is flagged as deprecated along with the tests that both fields return the expected data should be solid. I'll finish up the review on this now thanks!

field :geopoint, String, deprecation_reason: 'Use `geoshape`'
field :geoshape, String, description: 'GeoShape data for the location, if applicable'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may want to provide more details on what is in this field. I believe dataeng is consistently (we should confirm) using the BBOX option for the envelope as described here:
https://opensearch.org/docs/latest/field-types/supported-field-types/geo-shape/#envelope

If that turns out to be true, noting something like this for description would probably be helpful.
'GeoShape data for the location when available in WKT format. i.e. BBOX (minLon, maxLon, maxLat, minLat)'

field :kind, String, description: 'Type of location'
field :value, String, description: 'Name of location'

def geopoint
@object['geoshape']
end
end

class HighlightType < Types::BaseObject
Expand Down
25 changes: 25 additions & 0 deletions test/controllers/graphql_controller_v2_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -454,6 +454,31 @@ def setup
end
end

test 'graphqlv2 can retrieve geolocation information' do
VCR.use_cassette('graphql v2 geolocation') do
post '/graphql', params: { query: '{
search(searchterm: "train stations") {
records {
locations {
geopoint
geoshape
kind
value
}
}
}
}' }
assert_equal(200, response.status)
json = JSON.parse(response.body)

assert_not json['errors'].present?
assert_equal(
json['data']['search']['records'].first['locations'][0].keys.sort,
["geopoint", "geoshape", "value", "kind"].sort
)
end
end

test 'graphqlv2 retrieve invalid field' do
post '/graphql', params: { query: 'recordId(id: "stuff") {
stuff
Expand Down
34 changes: 34 additions & 0 deletions test/vcr_cassettes/graphql_v2_geolocation.yml
View file
Open in desktop

Large diffs are not rendered by default.

Loading