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

Support for OGC API Styles #43

Merged
merged 14 commits into from
Jul 16, 2024
Merged

Support for OGC API Styles #43

merged 14 commits into from
Jul 16, 2024

Conversation

LukasLohoff
Copy link
Contributor

@LukasLohoff LukasLohoff commented May 7, 2024

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:

  • not sure if the extra type OgcApiStyleDocument is needed - you could also use OgcApiDocument here if you add a styles subproperty. This would remove the need for casting sometimes
  • allStyles currently gets all its information from the styles endpoint. There might be a link to a json representation for each style which contains more information. Maybe another function such as getStyle({styleId}) is useful here?

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!

Copy link
Member

@jahow jahow left a comment

Choose a reason for hiding this comment

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

Thank you for this nice contribution and for your interest in the project!

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!

One thing I can say is that so far we used a principle of 1/ get a list of identifiers and 2/ get a full object from an identifier. So, kind of like what you suggested:

  • allStyles would return an array of ids
  • getStyle(styleId) would return a complete object

And then typically the application code can read stuff from the complete object, e.g. a list of supported formats, and then call getStylesheetUrl with a given format.

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

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!

@LukasLohoff
Copy link
Contributor Author

LukasLohoff commented May 15, 2024

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:

  • get a list of available styles and a description or title to display to the user
  • get a list of style formats for each style to check if the provided format is supported by the client
  • get a stylesheet URL for a given style

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
Copy link
Contributor Author

LukasLohoff commented May 22, 2024

  • 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:

LukasLohoff/ogc-client@b8664f6...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
Copy link
Member

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
Copy link
Contributor Author

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}

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?

  • adding an optional collectionId parameter to the existing allStyles and getStyle methods
    • would have to change allStyles to a function because getters do not allow parameters
  • adding two new methods to retrieve styles from collections

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):

  • rename the methods so they better describe the difference (i.e. listStyleIds or allStyleIds instead of listAllStyles); let the user decide
  • accept the additional requests and remove listAllStyles

What's your opinion on this?

@jahow
Copy link
Member

jahow commented Jun 10, 2024

@LukasLohoff here is what I would suggest:

  • listAllStyles can be dropped as it is redundant
  • allStyles should return a list of styles in brief (or summary) format; this should only contain informations available in the /styles?f=json endpoint, without any further request:
    • title
    • id
    • list of formats (obtained by getting all links with a rel of type "stylesheet" and getting their formats)
    • Let's call this model OgcStyleBrief for clarity
  • getStyle(id: string) should return a full style object; this object should contain information in the /styles/xx/metadata?f=json (i.e. "describedby" link) endpoint if available; otherwise it should still succeed but only return the same info as in allStyles
    • Let's call this model OgcStyleFull

As for collection-specific styles:

  • collections should be augmented with a styles property that is of type OgcStyleBrief[] and uses the style information present in the collection document (so id, title and formats)
  • getStyleForCollection(collectionId: string, styleId: string) should return a full style object (OgcStyleFull) which is specific to that collection; same logic as above, is there is a /collections/xx/styles/yy/metadata?f=json endpoint (i.e. "describedby" link) use it, otherwise return basic info from the collection document

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

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

  • if the user asks for something impossible (e.g. for a style that doesn't exist on a collection) throw an error
  • if the user asks for something that's unexpected but can still work (e.g. generate a URL with a format that is not advertised) print a console.warn and still do the task the best we can

Thank you for your commitment!

contains smaller improvements:
- remove redundant listAllStyles
- merge  getStyleMetadata into getStyle and handle fallback internally
@LukasLohoff
Copy link
Contributor Author

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'
);

Copy link
Member

@jahow jahow left a comment

Choose a reason for hiding this comment

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

Thank you very much! This looks very good. Thank you for committing to this valuable contribution!

@jahow jahow merged commit 52f33cd into camptocamp:main Jul 16, 2024
1 check passed
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

Successfully merging this pull request may close these issues.

3 participants