search

koopjs / FeatureServer

An open source Geoservices Implementation (deprecated)
https://geoservices.github.io
Other
101 stars 32 forks source link

Support /FeatureServer/:layer/queryRelatedRecords route #209

Closed rsjones closed 2 years ago

rsjones commented 2 years ago

Querying related records is part of the ArcGIS REST APIs core functionality. Table or layers can be related by something called a relationship class. In such cases, the ArcGIS REST API allows you to query features in such a layer and can return their related records from another layer. See this doc and this doc for additional details.

This PR adds a handler for a query method called queryRelatedRecords. This handler expects a specialized type of JSON that is not valid GeoJSON; specifically, it is a Feature Collection of Feature Collection. This means that this new handler will only work for providers that inspect the query method and send this special JSON when the method's value is queryRelatedRecords. This is key - the Koop provider needs to have the logic and metadata to determine the relationships and build the proper JSON to pass onto Feature Server. Therefore, manually testing this new functionality with a live Koop instance will require such a provider.

In addition to the handler noted above, the PR adds a handler for queries requesting metadata on relationships "managed" by the service.

cc @rsjones

rgwozdz commented 2 years ago

@rsjones - thanks very much for this. Can you give some more background/context for the purpose in the PR description? Also, can you add a CHANGELOG.md entry with heading ## Unreleased?

rgwozdz commented 2 years ago

@rsjones - still would like to get a more detailed description of what functionality this PR is adding. Some of our reviewers are familiar with this aspect of ArcGIS server; do you have a link to ArcGIS docs about querying related records. I need some context and background on this functionality.

rsjones commented 2 years ago

https://developers.arcgis.com/rest/services-reference/enterprise/query-related-records-feature-service-.htm documents the ArcGIS REST API method to get the related records of a feature. This change to the FeatureServer returns the queryRelatedRecords output for features or table rows related to the objectIds passed into the /FeatureServer/:layer/queryRelatedRecords method.

rgwozdz commented 2 years ago

Just a few minor comments.

Is it possible for FeatureServer.queryRelatedRecords() to simply accept an array of FeatureCollection, instead of an invalid GeoJSON? After all, the geojson format for the input object is just a wrapper and does not seem to carry information.

I agree with this; if the object wrapper is just formatting, it is something that is the job for FeatureServer rather than the source provider. But I think an array of FeatureCollections might still be invalid GeoJSON.

haoliangyu commented 2 years ago

I agree with this; if the object wrapper is just formatting, it is something that is the job for FeatureServer rather than the source provider. But I think an array of FeatureCollections might still be invalid GeoJSON.

@rgwozdz should exposed functions in this library universally accept a GeoJSON formatted object due to Koop's architecture? If so, this would make sense.

rgwozdz commented 2 years ago

I agree with this; if the object wrapper is just formatting, it is something that is the job for FeatureServer rather than the source provider. But I think an array of FeatureCollections might still be invalid GeoJSON.

@rgwozdz should exposed functions in this library universally accept a GeoJSON formatted object due to Koop's architecture? If so, this would make sense.

Good question @haoliangyu. FeatureServer is a somewhat special case in that it isn't a plugin itself; rather, it is a dependency of the GeoServices plugin. That plugin pulls data from the model's pull method which in turn pulls data from the provider's getData method. The spec for getData states that it should return GeoJSON. Note that this GeoJSON does not fit the spec 100% - we expect providers to decorate it with properties like filtersApplied or metadata which violate the spec. However, the basic GeoJSON structure remains. The problem with this PR is that it expects getData to return JSON that departs from GeoJSON's overall structure (rather than just adding properties). It's a big departure from the Koop provider pattern for this method.

If we want to avoid breaking the getData spec, there is another approach . We could update the Geoservices plugin so that is has a route specifically for queryRelatedRecords with a handler that leverages a new model method,pullRelatedRecords (This is what you did to support catalog requests). pullRelatedRecords would in turn call a special provider method getRelatedRecords and the spec for the return would be an array of feature collections. In my opinion, this is the better way to add the functionality that this PR is attempting.

rsjones commented 2 years ago

I agree with this; if the object wrapper is just formatting, it is something that is the job for FeatureServer rather than the source provider. But I think an array of FeatureCollections might still be invalid GeoJSON.

@rgwozdz should exposed functions in this library universally accept a GeoJSON formatted object due to Koop's architecture? If so, this would make sense.

Good question @haoliangyu. FeatureServer is a somewhat special case in that it isn't a plugin itself; rather, it is a dependency of the GeoServices plugin. That plugin pulls data from the model's pull method which in turn pulls data from the provider's getData method. The spec for getData states that it should return GeoJSON. Note that this GeoJSON does not fit the spec 100% - we expect providers to decorate it with properties like filtersApplied or metadata which violate the spec. However, the basic GeoJSON structure remains. The problem with this PR is that it expects getData to return JSON that departs from GeoJSON's overall structure (rather than just adding properties). It's a big departure from the Koop provider pattern for this method.

If we want to avoid breaking the getData spec, there is another approach . We could update the Geoservices plugin so that is has a route specifically for queryRelatedRecords with a handler that leverages a new model method,pullRelatedRecords (This is what you did to support catalog requests). pullRelatedRecords would in turn call a special provider method getRelatedRecords and the spec for the return would be an array of feature collections. In my opinion, this is the better way to add the functionality that this PR is attempting.

The way Koop asks providers to add attributes to the GeoJSON fits within the spec as "Foreign Members". There is risk of clients not understanding these parameters, but they should still work with the GeoJSON returned and parsers ignoring the foreign members.

There are some clients/libraries that do support the nested FeatureCollection e.g. gdal. It its true that the FeatureCollections within the features array of the parent FeatureCollection make them "Foreign Members" limiting supported clients if the GeoJSON is returned. An array of FeatureCollections isn't within the spec and structurally might make more sense, however it has a potentially more impactful result to clients not knowing how to parse the data structure if its ever returned in a similar fashion to how query can return GeoJSON. If the possibility of returning this as GeoJSON is no concern then either structure works based on other Koop considerations.

rgwozdz commented 2 years ago

There are some clients/libraries that do support the nested FeatureCollection e.g. gdal.

I'm not talking about clients/libraries in general; I'm talking about Koop Output Plugins that leverage the model.pull method which expects GeoJSON that is not a nested Feature Collection.

rsjones commented 2 years ago

Koop Output Plugins that leverage the model.pull method which expects GeoJSON

Wouldn't Output Plugins that don't understand the foreign members in the features array just treat it as having no GeoJSON Features?

rgwozdz commented 2 years ago

Koop Output Plugins that leverage the model.pull method which expects GeoJSON

Wouldn't Output Plugins that don't understand the foreign members in the features array just treat it as having no GeoJSON Features?

I don't think so. The problem is not that the features you have in the features array (a feature collection) have foreign members. It is that features in the features array are expected to have type Feature and have geometry and properties keys. The FeatureCollections you are putting in the features are type FeatureCollection and they have no geometry key/value. Take a look at https://github.com/koopjs/FeatureServer/blob/20956746e16ffb3fdddeff94e243142a63a9b910/test/integration/fixtures/relatedData.json, the map view won't load because it's invalid GeoJSON.

I appreciate you articulating the point about foreign-members. Currently the spec for getData is that it should return GeoJSON with or without foreign members. But the JSON you're planning to return deviates from that.

I'd like to wait for @haoliangyu to weigh in.

rsjones commented 2 years ago

the map view won't load because it's invalid GeoJSON

Just another data point as @haoliangyu weights in is that this GeoJSON with a NULL geometry to be in spec behaves the same as the relatedData.json you reference. https://github.com/koopjs/FeatureServer/blob/20956746e16ffb3fdddeff94e243142a63a9b910/test/integration/fixtures/no-geometry.json

rgwozdz commented 2 years ago

Just another data point as @haoliangyu weights in is that this GeoJSON with a NULL geometry to be in spec behaves the same as the relatedData.json you reference. https://github.com/koopjs/FeatureServer/blob/20956746e16ffb3fdddeff94e243142a63a9b910/test/integration/fixtures/no-geometry.json

yup, true, good point. More important is what that spec says:

A Feature object has a "type" member with the value "Feature". A Feature object has a member with the name "geometry".

I guess once way forward is set the type as Feature instead of FeatureCollection and add a geometry: null.

haoliangyu commented 2 years ago

For this library, I don't think it has to follow the GeoJSON data format restriction as it is technically a separate library from the Koop family. Therefore, using a structurally simple data format, like an array of FeatureCollection, makes sense to me and provides a nicer user interface. Any Koop-produced data object can be translated within the GeoServices output plugin. Anyway, I don't hold a very strong opinion on this. It is just about user interface.

@rgwozdz Your concern on the model.pull is valid and I totally agree, but it is out-of-scope in this PR. We should plan the new pull method but separately.

@rsjones No matter which solution is chosen, you will need to customize the provider to pull related record groups. Do you have a plan to update any provider plugin?

rsjones commented 2 years ago

Do you have a plan to update any provider plugin?

The provider that drove this work already returns related records in the GeoJSON formation outlined in this PR. https://github.com/EsriPS/koop-provider-knowledge

rgwozdz commented 2 years ago

@haoliangyu - thanks for your input here.

@rsjones - in the future, please consider opening a feature enhancement issue for work of this scope so we can have an architecture discussion prior to commencement of development work.