camptocamp / ogc-client

A TypeScript library for interacting with geospatial services
https://camptocamp.github.io/ogc-client/
BSD 3-Clause "New" or "Revised" License
66 stars 12 forks source link

Support for OGC API Styles #43

Closed LukasLohoff closed 4 months ago

LukasLohoff commented 7 months ago

This attempts to add basic support for styles. For now just retrieving styles is supported.

Examples:

Get a list of available style identifiers:

const endpoint = new OgcApiEndpoint('https://demo.ldproxy.net/zoomstack');
await endpoint.listAllStyles;

Get a list of styles (includes the supported formats, which you need for getStylesheetUrl):

await endpoint.allStyles;

Get style metadata:

await endpoint.getStyleMetadata('Deuteranopia');

Get stylesheet URL from metadata (or style document):

await endpoint.getStylesheetUrl('Deuteranopia', 'application/vnd.esri.lyr');

Some questions / notes:

For testing I mainly used the ldproxy demos at https://demo.ldproxy.net/ and https://test.cubewerx.com/cubewerx/cubeserv/demo/ogcapi

Happy for any suggestions or feedback!

LukasLohoff commented 6 months ago

First, the way we have designed APIs so far was mostly driven by usage. If you have a sample of application code that can demonstrate how these new features will be leveraged, that would help a lot with deciding which way to go!

It's a client which allows to import styles from an OGC API. Right now I don't have a code sample unfortunately. Maybe later. The basic requirements would be:

Your approach matches most of this process, which is great! One thing that can make things easier to use would be to:

  • rename getStyleMetadata into getStyle for clarity
  • only read style metadata from the metadata.json document, instead of parsing it both from the endpoint and the metadata documents (i.e. the parseStyles and parseBaseStyleMetadata functions)
  • do not include the links in the returned Style object; links are usually not given back to the application code (IIRC), they only have a technical role and ogc-client should allow consumers to not care about links

Sounds very reasonable, I'll change that part!

I hope this is clear. I'm not super familiar with the Styles spec which means I might have misunderstood some things. But again, this looks like a very solid start already!

Thanks!

LukasLohoff commented 6 months ago
  • rename getStyleMetadata into getStyle for clarity
  • only read style metadata from the metadata.json document, instead of parsing it both from the endpoint and the metadata documents (i.e. the parseStyles and parseBaseStyleMetadata functions)
  • do not include the links in the returned Style object; links are usually not given back to the application code (IIRC), they only have a technical role and ogc-client should allow consumers to not care about links

Had some time today to implement the changes. I also rebased the branch, so here is the diff:

https://github.com/LukasLohoff/ogc-client/compare/b8664f6e6daf348018a8ef31b3aee2387da3cd23...LukasLohoff:ogc-client:ogc-styles

As suggested, allStyles now gets its information from the metadata.json documents. This should also be more robust because the supported formats do not have to be specified in the endpoint (I think, not sure what the standard actually says). It however means that there will be one GET request per style.

ronitjadhav commented 6 months ago

I have tested the updates, and everything works!!

I do have a few suggestions regarding the styles. Perhaps we could consider implementing the following endpoints for more granular access to styles based on collections:

  1. Retrieve a list of styles for the specified collection: /collections/{collectionId}/styles
  2. Retrieve the specified style for a particular collection: /collections/{collectionId}/styles/{styleId}

These changes could also help us get styles from this OGC API service: https://maps.gnosis.earth/ogcapi/

Additionally, I'm wondering about the necessity of the listAllStyles method. Since we can obtain the style IDs from the allStyles method, it might be worth considering whether listAllStyles is redundant.

Looking forward to your thoughts on this!

LukasLohoff commented 6 months ago

I have tested the updates, and everything works!!

:+1:

I do have a few suggestions regarding the styles. Perhaps we could consider implementing the following endpoints for more granular access to styles based on collections:

  1. Retrieve a list of styles for the specified collection: /collections/{collectionId}/styles
  2. Retrieve the specified style for a particular collection: /collections/{collectionId}/styles/{styleId}

Definitely sounds useful! I totally forgot about the collections in combination with styles, as the services I used for testing didn't have them. I think this is something we will also need at some point for our application.

What would you prefer?

Additionally, I'm wondering about the necessity of the listAllStyles method. Since we can obtain the style IDs from the allStyles method, it might be worth considering whether listAllStyles is redundant.

Correct, all information returned by listAllStyles is already part of the allStyles response. listAllStyles has the advantage that it only needs a single request to fetch the list of ids - in comparison to one request per style for the allStyles method. I see a few options here (which all are some sort of compromise):

What's your opinion on this?

jahow commented 5 months ago

@LukasLohoff here is what I would suggest:

As for collection-specific styles:

Collection-specific styles can be done in a followup PR if needed, no problem :slightly_smiling_face:

As for error handling, this is the policy we use:

Thank you for your commitment!

LukasLohoff commented 4 months ago

Hi @jahow

first of all, sorry for the delay. Implemented your suggestions and also added support for collections. I didn't add the styles property to the collection (for caching?) yet, that could be added later.

Instead of adding additional methods for collections I chose to add an optional parameter instead. If you want to change the API design, that's of course also fine. It just made more sense for me when implementing it. Another way would be switching to an options parameter approach, because having multiple parameters can be confusing. e.g.:

await endpoint.getStylesheetUrl({
  id: 'Tritanopia',
  mimeType: 'application/vnd.ogc.sld+xml;version=1.0',
  collectionId: 'airports'
});

Let me know what you think.

Here are updated example snippets (note that allStyles is now a function):

Get a list of styles:

await endpoint.allStyles();

Get a list of styles for a collection:

await endpoint.allStyles('airports');

Get style metadata:

await endpoint.getStyle('Deuteranopia');

Get style metadata for a collection:

await endpoint.getStyle('Deuteranopia', 'airports');

Get stylesheet URL for a collection:

await endpoint.getStylesheetUrl(
  'Tritanopia',
  'application/vnd.ogc.sld+xml;version=1.0',
  'airports'
);