Skip to content

drop TILED arg from wms printing queries (#7809)#7811

Closed
landryb wants to merge 1 commit intogeosolutions-it:masterfrom
landryb:fix/7809
Closed

drop TILED arg from wms printing queries (#7809)#7811
landryb wants to merge 1 commit intogeosolutions-it:masterfrom
landryb:fix/7809

Conversation

@landryb
Copy link
Copy Markdown
Contributor

@landryb landryb commented Feb 1, 2022

TILED is vendor-specific, and triggers exceptions with mapproxy
when requesting HEIGHT & WIDTH that dont match the cache tile size or format that doesnt match the cache format.

Description

Fix printing WMS layers coming from mapproxy

this can be tested before/after when adding a WMS layer:

  • from https://tiles.craig.fr/ortho/service - warning, single tile option needs to be used in layer properties otherwise layer wont load/display, server returns Invalid request: invalid tile format, use mixed because of default TILED=true behaviour - if removed as done by this PR, then printing the ortho layer works
  • from https://tiles.craig.fr/pci/service - triggers Invalid request: invalid tile size (use 256x256) exception serverside.

Please check if the PR fulfills these requirements

What kind of change does this PR introduce? (check one with "x", remove the others)

  • Bugfix

Issue

What is the current behavior?

#7809 - printing WMS layers coming from mapproxy fails because TILED=true triggers exceptions, either because the requested HEIGHT and WIDTH dont match the cache tile size or the requested format doesnt match the cache format.

What is the new behavior?
printing WMS layers coming from mapproxy succeeds, tested with both ortho and pci.

Breaking change

Does this PR introduce a breaking change? (check one with "x", remove the other)

  • Yes, and I documented them in migration notes
  • MAYBE
  • No

Maybe, because from my understanding TILED=true triggers a vendor-specific behaviour in geoserver/geowebcache ?

Ideally, TILED=true should only be added if the remote is geoserver/gwc, but at that point i have no idea if that's an information we can get (and trust) from capabilities document.

Other useful information

Interoperability is hard.

TILED is vendor-specific, and triggers exceptions with mapproxy
when requesting HEIGHT & WIDTH that dont match the cache tile size.
@tdipisa tdipisa requested a review from offtherailz February 1, 2022 10:52
@tdipisa tdipisa added this to the 2022.01.00 milestone Feb 1, 2022
@tdipisa tdipisa added the bug label Feb 1, 2022
landryb added a commit to georchestra/mapstore2-georchestra that referenced this pull request Feb 2, 2022
landryb added a commit to georchestra/mapstore2-georchestra that referenced this pull request Feb 2, 2022
Copy link
Copy Markdown
Member

@offtherailz offtherailz left a comment

Choose a reason for hiding this comment

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

Generally speaking, tiled option in GeoServer is used to get WMS requests from tile cache, if they match the gridset. Otherwise the response is provided anyway, with some special headers specifying the esit and reason of cache failure
I understood that MapProxy uses the same parameter name with a different behavior, and rises exception so it can be removed (it is only a cache support optimization).
Anyway the layers has also a tiled parameter, that is used on map.
If you had to disable it for the layer, maybe is a good idea to read it from the layer object and use it if present (and / or true).

@landryb
Copy link
Copy Markdown
Contributor Author

landryb commented Feb 2, 2022

Anyway the layers has also a tiled parameter, that is used on map. If you had to disable it for the layer, maybe is a good idea to read it from the layer object and use it if present (and / or true).

Well, the tiled parameter is on by default on all layers (and i havent found a way to disable that per server in the ms2 config), and it works fine when displaying layers that dont have a 'mixed' cache format (eg the layer from https://tiles.craig.fr/pci/service). TILED=true is used along HEIGHT=256 and HEIGHT=256 so mapproxy is fine with that, which isnt the case when querying the server for the image to print (which might be 'against' TILED spec, which is a vendor extension anyway..).

For layers that use a *mixed* cache format (eg layers from https://tiles.craig.fr/ortho/service), using TILED=true (so, the default) breaks layer display (and also printing).

@tdipisa tdipisa added tmp and removed tmp labels Feb 11, 2022
@tdipisa tdipisa self-requested a review February 22, 2022 08:46
Copy link
Copy Markdown
Member

@tdipisa tdipisa left a comment

Choose a reason for hiding this comment

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

I don't think we can approve this PR as it is @offtherailz. I completely understand the @landryb point of view and of course we can probably do something to improve the interoperability with MapProxy in this case.
In order to find an automatic way to set the TILED property to true/false depending on the layer source is something that need to be investigated. Therefore a dedicated issue should be opened for this. For the moment, and especially for 2022.01.00, what we can do is just to make it configurable globally (default for MapStore is true). What do you think @offtherailz?

@landryb
Copy link
Copy Markdown
Contributor Author

landryb commented Feb 22, 2022

well you already have a way to know if you're talking to geoserver in https://github.com/geosolutions-it/MapStore2/blob/master/web/client/utils/LayersUtils.js#L25 - though i think it'd be much nicer to check for vendor extensions in capabilities. Fwiw my geoserver is on https://ids.craig.fr/wxs/ for various reasons, it's a bit awkward to expect geoserver to be at /geoserver/...

As for TILED itself, making it globally configurable would be a valid workaround for now :)

@tdipisa
Copy link
Copy Markdown
Member

tdipisa commented Feb 22, 2022

well you already have a way to know if you're talking to geoserver in https://github.com/geosolutions-it/MapStore2/blob/master/web/client/utils/LayersUtils.js#L25 - though i think it'd be much nicer to check for vendor extensions in capabilities. Fwiw my geoserver is on https://ids.craig.fr/wxs/ for various reasons, it's a bit awkward to expect geoserver to be at /geoserver/...

I can understand your concern about this but unfortunately, as far as I know, there isn't a clean/stable way implemented so far in GeoServer (eg. a kind of declaration or something similar) to understand that the capabilities are effectively coming from a GeoServer. Therefore some development GeoServer side should be necessary for this (at least for new GeoServer versions).
One way could be also to delegate the user to declare this through the MS UI (for example when the user define a source in the Catalog tool). Long story short, we haven't made a decision on this yet.
Feel free anyway to contribute to this if you want/can (even if only with your ideas) and thank you so much in any case for your feedback.

As for TILED itself, making it globally configurable would be a valid workaround for now :)

Do you have in mind or do you have the possibility to contribute for this? Otherwise I'm going to close this PR for now. Thank you again.

@landryb
Copy link
Copy Markdown
Contributor Author

landryb commented Feb 22, 2022

As for TILED itself, making it globally configurable would be a valid workaround for now :)

Do you have in mind or do you have the possibility to contribute for this? Otherwise I'm going to close this PR for now. Thank you again.

I dont think i have enough knowledge of the platform right now to implement that in a short timeframe.. but that might be something we can fund.

@tdipisa
Copy link
Copy Markdown
Member

tdipisa commented Feb 22, 2022

As for TILED itself, making it globally configurable would be a valid workaround for now :)

Do you have in mind or do you have the possibility to contribute for this? Otherwise I'm going to close this PR for now. Thank you again.

I dont think i have enough knowledge of the platform right now to implement that in a short timeframe.. but that might be something we can fund.

@landryb ok, let me know then. Thank you. I'm going to close this for the moment.

@landryb
Copy link
Copy Markdown
Contributor Author

landryb commented Feb 22, 2022

there isn't a clean/stable way implemented so far in GeoServer (eg. a kind of declaration or something similar) to understand that the capabilities are effectively coming from a GeoServer.

another possibility would be to look for GEOSERVER under /Service/KeywordList/Keyword in GetCapabilities as it's a default config in geoserver and i doubt many service administrators change that..

@landryb
Copy link
Copy Markdown
Contributor Author

landryb commented Feb 24, 2022

there isn't a clean/stable way implemented so far in GeoServer (eg. a kind of declaration or something similar) to understand that the capabilities are effectively coming from a GeoServer.

another possibility would be to look for GEOSERVER under /Service/KeywordList/Keyword in GetCapabilities as it's a default config in geoserver and i doubt many service administrators change that..

replying to myself on a closed issue, also checking for the presence of updateSequence attribute to the toplevel WMS_Capabilities XML element would be another idea to figure out if the remote is geoserver.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants