enforce WMS layer limit

[sidneygijzen]

What this PR does

This PR proposes an approach for implementing the WMS layer limit. One of the goals is trying to mitigate the resource-intensive requests - as reported in #6866 and #7320 - by enabling a WMS server administrator to make use of the WMS layer limit.

My initial thought was to modify the WebMapServiceCatalogItem validLayers getter, but during manual testing the styles URL param went awry. From what I can tell it is because the WebMapServiceCatalogItem layersArray is used in the WebMapServiceCapabilitiesStratum legends getter. Therefore I chose to check for a layer limit in the layersArray.

A note on the modified XML test files; during the implementation a couple of unrelated tests errored out. A small number of xml files contained a <LayerLimit>1</LayerLimit>. I have opted to remove the <LayerLimit>1</LayerLimit> elements from the XML file instead of adjusting the tests.

Finally, I am not sure if it fully addresses #5267, because I haven't looked at the MagdaReference model.

Test me

Aside from the added unit test, I tested this manually by configuring a wms-group as a wms. Please see this gist for a test config. With this "wrong" config I try to simulate what is happening in #7320, which is that the CswCatalogGroup currently has no knowledge of the concept of a WMS-group. The WMS below has a WMS layer limit defined, so only one wms layer is requested in this case.

Checklist

• There are unit tests to verify my changes are correct or unit tests aren't applicable (if so, write quick reason why unit tests don't exist)
  - [ ] I've updated relevant documentation in doc/.
• I've updated CHANGES.md with what I changed.
• I've provided instructions in the PR description on how to test this PR.

[sidneygijzen]

@pjonsson Thanks for the review! I've updated the PR.

[nf-s]

Thanks @sidneygijzen !

Just one code comment, and can you please add a warning message in shortReport if the layer limit has been hit? Just so users know if a WMS item has been configured with too many layers

https://github.com/sidneygijzen/terriajs/blob/c22bf3f09c9f0d190c02599598856e13d2e3d43e/lib/Models/Catalog/Ows/WebMapServiceCatalogItem.ts#L185-L194

Thanks 🙂

[nf-s]

Instead of pulling layerLimit out of the WebMapServiceCapabilitiesStratum, it would be better to create a layerLimit trait in WebMapServiceCatalogItemTraits.

This would allow the user to configure it (in a JSON catalog), and then you can remove this get layerLimit in WebMapServiceCatalogItem.

While there are a few instances of using the WebMapServiceCapabilitiesStratum directly in WebMapServiceCatalogItem, this is not ideal, and should be considered an anti-pattern, at some point we'll refactor it 🙂

[sidneygijzen]

@nf-s thank you for the review! I appreciate it that you're explaining why something should be in a specific spot in the codebase.

I will look into adding a warning message in shortReport about the WMS layer limit.

Concerning the code comment, I am afraid I am a bit confused.

A WMS LayerLimit is configured - and visible in the WMS GetCapabilities - by the organisation operating the WMS server. For example: see this section about wms_layerlimit in the MapServer docs. This PR aims to read the layer limit configured by the organisation operating the WMS server and use it while making WMS GetMap requests.

Where it gets confusing for me, is that you refer to "allow the user to configure it (in a JSON catalog)". I am assuming that by "the user" you are referring to a person who is deploying TerriaJS. In my honest opinion there is no configuration to be made in this regard by the organisation/person deploying TerriaJS. It is not possible to change the wms_layerlimit of the WMS server by configuring it in TerriaJS. If the person deploying TerriaJS would configure a higher value for the layer limit than the WMS server allows, it results in WMS server error messages (see #5267). What would be use cases for setting a lower value than what the WMS server allows?

The aforementioned is the reason why I chose to pull the layer limit out of the WebMapServiceCapabilitiesStratum. From my point of view it is simply a constant found in the WMS GetCapabilities. What would be the right place for reading config from the WMS GetCapabilities and using it when making WMS GetMap requests?

What do you think?

[pvgenuchten]

In my view both approaches are valid and could both be implemented:

• any software should respect the layerlimit if it is advertised in capabilities, considering fair use of the service
• if a terria admin aims to limit the number of layers requested from a service, it could only be done in a terria configuration (if the service is out of their control)

My suggestion would be to start with this, and then implement the second scenario in a separate PR, unless the approaches are conflicting...

[zoran995]

@sidneygijzen, @pvgenuchten, one approach we can try here is to introduce a trait for LayerLimit, allowing users to define their value as part of the JSON configuration. And if the layerLimit is defined as part of WMS Capabilities, use that as the default value if not supplied by the user. But let me check with @nf-s what he thinks about this and get back to this

[nf-s]

Sorry for the late reply @sidneygijzen

It is less about being able to manually configure the layerLimit - it is more about following existing patterns that are in place. Defining a layerLimit trait also provides some documentation (as you will need to specify a description).

Also, if you define a layerLimit trait, then the value set in WebMapServiceCapabilitiesStratum will automatically be available in WebMapServiceCatalogItem (so you can delete your get layerLimit accessor in WebMapServiceCatalogItem_.

You can look at isGeoServer as an example trait.

If you are unsure how the model layer/traits system works - we have some documentation available here

• https://docs.terria.io/guide/contributing/model-layer/
• https://docs.terria.io/guide/contributing/traits-in-depth/
• https://docs.terria.io/guide/contributing/strata-examples/

In short, you should define a layerLimit trait in WebMapServiceCatalogItemTraits like so

  @primitiveTrait({
    type: "number",
    name: "Layer limit",
    description: "The maximum number of layers that can be requested simultaneously."
  })
  layerLimit?: number;

And then you should delete the get layerLimit accessor in WebMapServiceCatalogItem

https://github.com/sidneygijzen/terriajs/blob/df7e04cc70e87b2eda71e8fd4c35f28b4b6eb52d/lib/Models/Catalog/Ows/WebMapServiceCatalogItem.ts#L249-L256

After both of those things, you will be able to use this.layerLimit in WebMapServiceCatalogItem - and the value from WebMapServiceCapabilitiesStratum will automatically flow into the WebMapServiceCatalogItem model

Thanks!

[sidneygijzen]

Thank you for the detailed response @nf-s. I appreciate it a lot!

I will have a closer look in the coming weeks and update this PR accordingly.

[sidneygijzen]

I've thinking about this PR a while now, but the more I work on it, I'm not convinced the current implementation is correct. The current implementation limits the layers array, however I think the limitation is set too early in the process. A better implementation would be when a user would like to view multiple layers simultaneously, not when listing the available layers.

So, following that line of thought, I am thinking of the following: 1) reverting the change to the layers array and 2) checking the layer limit when the user actually wants to add a layer to the map, e.g by checking the layerlimit in the Workbench and/or DataCatalogItem component.

Because of the above, I've converted this PR to draft state.