Update GraphQL to better support Locations #784
Conversation
matt-bernhardt
force-pushed
the
gdt-134
branch
from
9535130 to
99599c4
Compare
matt-bernhardt
force-pushed
the
gdt-134
branch
2 times, most recently
from
f26fff7 to
3241148
Compare
| @@ -108,7 +108,7 @@ 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' | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
I think this is good now but I am not approving until we coordinate with DataEng on when they would like to update production TIMDEX data to use the new field (i.e. I want to keep main deployable and I'm pretty sure this would break things if someone requested either Locations.geopoint or locations.geoshape once we merge until the index mappings are updated. I don't know if this will require a full reharvest or just an update to the mappings on the existing indexes since there is currently no data in the changing field.
| @@ -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, deprecation_reason: 'Use `geoshape`' | |||
| field :geoshape, String, description: 'GeoShape data for the location, if applicable' | |||
There was a problem hiding this comment.
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)'
|
I'd love to write a more helpful description field for |
matt-bernhardt
force-pushed
the
gdt-134
branch
2 times, most recently
from
6c1035e to
a059699
Compare
There was a problem hiding this comment.
@matt-bernhardt I realize now that we have a bunch of other things that aren't really ready for prod right now until the indexes are updated so holding this one back doesn't make sense. We can merge, and we'll need to carefully plan a release with all of the new geospatial features soon in conjunction with data being released or at least indexes being updated to support the data.
** Why are these changes being introduced: * Now that we're adding geospatial records to TIMDEX, we have changed the data model slightly. Initially we had wanted to use a GeoPoint field in the LocationType construct, but that is now ending up as GeoShape. As a result, we need to update the application code to match our actual use of these records. ** Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/gdt-134 ** How does this address that need: * This replaces the GeoPoint field with a GeoShape field. * In parallel to the data model change, we also add a test to confirm that all three fields are coming back from the GraphQL endpoint. ** Document any side effects to this change: * I'm not sure whether it is a problem to be testing the extents of the data model in this way, but I'm also not sure whether there's a better way to confirm this change in code. It feels worse to do a test of a specific record, because then our tests are fragile to changes in library holdings - which feels worse.
** Why are these changes being introduced: * Rather than simply replacing geopoint with geoshape, we are going to keep geopoint in the code, but mark it as deprecated. This is the recommended workflow for making changes in GraphQL (which we are following here to reinforce the practice, even though this particular field has never been populated) ** How does this address that need: * This brings back the geopoint field in the LocationType definition, but markes it deprecated in the schema. * We add a method to the Location type that populates the geopoint field with the contents of the geoshape field. Ironically, this is likely the first time that the geopoint field has been able to respond with any values at all. * We also update the test to include reference to this now-deprecated field. ** Document any side effects to this change: * Is it a side effect to be, effectively, adding content to the geopoint field only as we mark it deprecated?
matt-bernhardt
force-pushed
the
gdt-134
branch
from
a059699 to
cdf20f7
Compare
This replaces the GeoPoint field in LocationType with a GeoShape. I also take a crack at writing a test for this, but I'm honestly still not sure it is needed. I don't think we have any similar tests like this for the other types defined in
record_type.rb.Developer
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)
Code Reviewer
(not just this pull request message)
Requires database migrations?
NO
Includes new or updated dependencies?
NO