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.

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

Koop Roadmap for 2022-2023 #386

Closed
rgwozdz opened this issue Aug 8, 2022 · 7 comments
Closed

Koop Roadmap for 2022-2023 #386

rgwozdz opened this issue Aug 8, 2022 · 7 comments
Assignees

Comments

@rgwozdz
Copy link
Member

rgwozdz commented Aug 8, 2022

Hello Koop User Community! I'd like to shared some proposed changes with you. They are listed below in order of expected implementation. Please comment with any feedback or suggestions. It's our hope to start implementing these changes soon.

  1. Move Koop's core-dependencies to a monorepo

Packages will include:

koop-core
koop-logger
koop-cache-memory
koop-output-geoservices
feature-server
winnow

The monorepo approach helps alleviate existing pain points that include:

  • Deployment of dependency's version bumps
  • e2e tests for Koop that integrate lastest versions of all packages
  • location for FeatureServer conformance testing
  • easier tracking/tagging of versions

Note: Package versions will use individually-based version numbers, continuous with version numbers they currently use in independent repos. However, we plan to npm publish all packages under the @koopjs npm org.

  1. Support use of the koop-logger in Feature-Server and Winnow

These dependencies currently use console.log while other Koop core-dependencies and many plugins leverage the Koop logger. Using koop logger will allow better management of log messages (debug, warn, info, error), help ensure log visibility, and provide formatting required by some deployments.

  1. Remove legacy output-geoservices routes (routes without rest/services)

Early versions of output-geoservices did not include routes with the rest/services fragment of the URL path. However, some ArcGIS clients require service URLs to include it to work properly. An additional set of routes with rest/services were added to address this issue, but the old routes without that fragment were maintained to avoid a breaking change. While we avoided a breaking change, it has lead to confusion on which routes to use and question about why some routes won't work with clients like AGO.

Note: This change will break any existing implementations that leverage routes without the rest/services fragment.

  1. Add code coverage tooling to all monorepo packages

  2. Remove generic datasets provider and ship in separate plugin

Koop ships with a set of generic "datasets" provider with custom endpoints that allow users to add and retrieve ad hoc JSON to the koop-cache. It's likely not used very often, and could be a source of memory problems if abused. This provider should be separated and moved to its own plugin repository and therefore installed only when needed by Koop developers.

  1. Stretch goal: Remove "hosts" parameter from the provider specification and output-geoservices routes

The existing Koop-provider specification allows for the configuration of two route parameters, hosts and id. In the early development of Koop, hosts was meant to contain information to help target a remote API (maybe a service hostname or route), while id was meant to contain information the help target a specific resource on a remote API. However, such definitions are not always applicable to a given provider, so in reality these are just two optional and generic route parameters. In summary, having two generic route parameters is often unnecessary, creates route-building complications, and adds usage confusion (e.g., what do I use hosts for?). A single generic parameter is sufficient, as it can be a delimited string that hold multiple pieces of information.

We will:

  • remove support for providers with an enabled hosts parameter from koop-core
  • reject the registration of providers that enable the hosts parameter and log messages that provide a link to migration documents
  • update the Koop documentation so that the provider spec no longer includes hosts, and includes steps to migrate old provider to the new specification.
@rgwozdz rgwozdz self-assigned this Aug 8, 2022
@schmidtk
Copy link
Contributor

schmidtk commented Sep 8, 2022

For services with a large number of layers, support for arbitrary folder depth would be very useful. For example:

/es/rest/services/Parent/FeatureServer
/es/rest/services/Parent/Child/FeatureServer
/es/rest/services/Parent/Child/SubChild/FeatureServer

Currently with the :host/:id route format, the /Parent/Child path is possible (required?) but to my knowledge you can't arbitrarily nest services. When using the Elasticsearch provider with the current version of Koop, the third URL would error with:

Cannot GET /es/rest/services/Parent/Child/SubChild/FeatureServer

If you get rid of the :host parameter, will it be possible to support this arbitrary nesting or will both the second and third URL's report an error? Would the idea be to change the route something like this?

/$providerNs/rest/services/:id*/FeatureServer

Then using the third URL as an example, req.params.id would be Parent and req.params.0 would be /Child/SubChild and the provider would be responsible for handling that appropriately.

Thanks for providing this roadmap, this is all helpful planning our project moving forward.

@rgwozdz
Copy link
Member Author

rgwozdz commented Sep 8, 2022

Hi @schmidtk! Thanks very much posting here. I'm pretty sure you can't delimit a param with /, as it's probably a reserved character. Curious if you can elaborate on the reason you need a route with all of additional fragments. Also, are these fragments static or are they actual parameters? i.e.,

/es/rest/services/:id/Child/SubChild/FeatureServer

vs

/es/rest/services/:id/:child/:subChild/FeatureServer

@schmidtk
Copy link
Contributor

schmidtk commented Sep 8, 2022

This would be for organizational purposes, so we can structure our services in folders.

Here's an example from the ArcGIS Online sample server:
https://sampleserver6.arcgisonline.com/arcgis/rest/services
https://sampleserver6.arcgisonline.com/arcgis/rest/services/LocalGovernment/Recreation/FeatureServer

The base service has a mix of folders and services to better organize all of the layers. If the URL pattern was changed to /es/rest/services/:id/FeatureServer it would not be possible to mimic this sort of structure. You would be limited to listing out a flat list of layers, which is not desirable for servers with a lot of layers.

If the URL pattern was instead /es/rest/services/:id*/FeatureServer, arbitrary nesting could be supported. Using the above URL as an example, the req.params object would look like this:

{
  id: "LocalGovernment",
  0: "/Recreation"
}

The id property will contain everything up to a /, and the 0 property will contain everything else up to /FeatureServer (including slashes). The provider would be responsible for interpreting that, but this would allow any depth in the URL structure.

@rgwozdz
Copy link
Member Author

rgwozdz commented Sep 8, 2022

@schmidtk - thanks very much for elaborating. This is an interesting idea. I'm not quite sure what it would mean for the way Koop handles routing, I'll need to explore the internals and try it out. But it could be great! Looking forward to trying this out.

FYI, changing the route params is a stretch goal for the next year, so it might be a bit before any changes on it (at least from our side) move forward.

@schmidtk
Copy link
Contributor

schmidtk commented Sep 8, 2022

Great, thanks for your consideration on this.

I'm not sure what impacts this might have in the core Koop code, but I did do a quick smoke test with the Elasticsearch provider to see how the current URL's could be maintained with the suggested /:id* URL pattern.

The route is handled in this block in the provider model:
https://github.com/koopjs/koop-provider-elasticsearch/blob/d0c4213c150d6e5de6bd4cd15c18b5b2f295fece/src/model.js#L42-L45

With the new pattern, the param assignments would change to:

// This was previously the :host param.
const esId = req.params.id;
// This value includes the leading slash, stripping it for parity with the old code.
const serviceName = req.params[0].replace('/^\//', ''); 

That assumes the current two level structure (/Parent/Child/FeatureServer). If the structure was nested further, the provider would want to split req.params[0] on / and handle the request based on those path parts. If the path was only one level (/Parent/FeatureServer), you would simply use the req.params.id value.

I tested this out with our current URL's and everything worked great with the FeatureServer/Info/Layers/Layer Info/Query endpoints.

@efbenson
Copy link

@rgwozdz as I play around with getting the related layers working with a custom provider it feels like returning massive metadata and other "configuration" data on the geojson object is getting to be a bit cludgy. Maybe it would be possible to add some additional functions on the Model class for that. Something like getLayersConfiguration(host, id) or something like that where we could return the static configuration about the layers. IMO that would also provide a cleaner way of generating the data for the /layers rest endpoint

@rgwozdz
Copy link
Member Author

rgwozdz commented Mar 24, 2023

Closing this as we move past this period. I have moved comments to their own issues and tagged them as roadmap candidates. Any enhancements for consideration on the coming year's roadmap should be added as enhancement issues.

@rgwozdz rgwozdz closed this as completed Mar 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants