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

#9031 GetMap format list in GetCapabilities not respected #9054

Merged
merged 9 commits into from
Mar 29, 2023

Conversation

allyoucanmap
Copy link
Contributor

@allyoucanmap allyoucanmap commented Mar 24, 2023

Description

This PR includes following improvements:

  • refactor of WMS api removing the JSONX parser
  • review how the format are included inside the layer object created from a WMS service
  • review interface of getLayerFromRecord method of catalog api
  • include a refresh button for the format selector of service advanced settings

For Testers

we need to cover functional test for following components

  • visibility settings of a layer
  • format settings of a layer
  • export of a WMC file
  • style editor
  • WMS/WMTS/CSW catalog
  • add WMS background layer from UI (check if available styles are correctly displayed)
  • layer metadata

Please check if the PR fulfills these requirements

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

  • Refactoring

Issue

What is the current behavior?

#9031
#8906

What is the new behavior?

The format for the WMS layer are extracted directly from the capabilities even if the advanced setting are not changed, in addition the filtering of the supported format types has been improved to allow only the supported ones listed in the capabilities

Breaking change

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

  • No

Other useful information

Copy link
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.

General thing looks good, I love we are removing ogc-schemas one piece at time ❤️

I asked for some fixes and doc/naming improvements. See my comments inline and contact me if something is unclear.

web/client/api/WMS.js Outdated Show resolved Hide resolved
web/client/api/WMS.js Outdated Show resolved Hide resolved
web/client/api/WMS.js Outdated Show resolved Hide resolved
web/client/api/WMS.js Outdated Show resolved Hide resolved
web/client/api/WMS.js Outdated Show resolved Hide resolved
web/client/api/WMS.js Outdated Show resolved Hide resolved
web/client/api/WMS.js Outdated Show resolved Hide resolved
web/client/api/WMS.js Show resolved Hide resolved
@@ -1483,7 +1483,8 @@
},
"format": {
"noOption": "Nessuna opzione",
"loading": "Caricamento in corso..."
"loading": "Caricamento in corso...",
"refresh": "Recupera il formato supportato"
Copy link
Member

Choose a reason for hiding this comment

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

I think it should be plural also on the other translations.

Suggested change
"refresh": "Recupera il formato supportato"
"refresh": "Recupera i formati supportati"

Copy link
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.

  • Please fix the translations

The rest is ok.

Copy link
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.

As exchanged in our call I would like to remove the support for supportedGetMapFormats (the name can be also misunderstood) in favor of a fixed list of a well known and tested formats if we cannot find a reliable heuristic that automatically interpret it from the wms getcapa and generate that list. I wouldn't like to stratify config properties like this in favor of a more linear behavior: if we will need to include a new format we will provide a new PR to update the list of explicitly supported ones.

@offtherailz offtherailz enabled auto-merge (squash) March 27, 2023 13:28
@landryb
Copy link
Collaborator

landryb commented Mar 27, 2023

As exchanged in our call I would like to remove the support for supportedGetMapFormats (the name can be also misunderstood) in favor of a fixed list of a well known and tested formats if we cannot find a reliable heuristic that automatically interpret it from the wms getcapa and generate that list. I wouldn't like to stratify config properties like this in favor of a more linear behavior: if we will need to include a new format we will provide a new PR to update the list of explicitly supported ones.

I'm a bit lost.. supportedGetMapFormats allowed one to 'override' the hardcoded list of formats supported by mapstore ? The newly hardcoded list after ea42e62 is a bit short.. in my opinion it should be whatever formats the browser is able to display, no ? or only the list of formats supported by OL/leaflet/cesium ? Should that only be 'official' mimetypes ?

what about image/tiff ? i also have servers with image/png; mode=32bit which .. is a valide mimetype (24-bits png with alpha channel). im not so sure too, but there might be servers out there with image/jpg without the e in jpeg..

I've tested image/png; mode=8bit from https://wms.craig.fr/ign (reported in #8906) and its now correctly available, and can be used as a default for the layers added from the service, be it in the layers display properties or the service advanced properties.

fetching available formats from https://tiles.craig.fr/ortho/service correctly only lists image/png & image/jpeg so georchestra/mapstore2-georchestra/#301 is properly handled.

will test with more services from https://geoservices.ign.fr/services-web-essentiels & https://geoservices.ign.fr/services-web-experts and others we might use.

@landryb
Copy link
Collaborator

landryb commented Mar 27, 2023

https://wxs-gpu.mongeoportail.ign.fr/externe/vkd1evhid6jdj5h4hkhyzjto/wms/v?service=WMS&version=1.3.0&request=GetCapabilities provides a large list, from which image/tiff, image/geotiff, image/tiff8 & image/geotiff8 might appear valid in addition to the already supported ones ?

https://wxs.ign.fr/administratif/geoportail/r/wms?service=WMS&version=1.3.0&request=GetCapabilities provides:

<Format>image/jpeg</Format>
<Format>image/png</Format>
<Format>image/tiff</Format>
<Format>image/geotiff</Format>
<Format>image/x-bil;bits=32</Format>
<Format>text/asc</Format>

from this list, ms2 only displays jpeg & png. So, tiff and geotiff are somewhat popular ? are those supported by OL ?

Will do more tests tmrw, including GFI formats (eg #9007 / georchestra/mapstore2-georchestra#302)

@landryb
Copy link
Collaborator

landryb commented Mar 28, 2023

for GFI formats testing, first some tests with mapserver:

  • i've added continents layer https://demo.mapserver.org/cgi-bin/wms (mapserver 8 official demo server) which only advertises text/html, application/vnd.ogc.gml, text/plain - the feature info pane of the layer properties correctly lists plain and html.
    • querying with info_format=text/plain triggers an error:
      msWMSLoadGetMapParams(): WMS server error. Missing required parameter STYLES. Note to service administrators: defining the "wms_allow_getmap_without_styles" "true" MAP.WEB.METADATA item will disable this check (backward compatibility with behaviour of MapServer < 8.0)
      it's the same error reported in [interoperability] WMS GetMap queries for elevation layer lack the mandatory STYLES parameter #8599. the STYLES parameter is also mandatory for getfeatureinfo requests. Adding an empty STYLES= to the query triggers another strange error:
msWMSFeatureInfo(): WMS server error. Layer(s) specified in QUERY_LAYERS parameter is not offered by the service instance.
  • this last one doesnt make sense as the query correctly specifies the same layer in both params: layers=continents&query_layers=continents&
  • adding layers from https://wms.craig.fr/ign (mapserver 8), same issue, missing STYLES parameter. But the correct list of GetFeatureInfo formats (text/html/properties/template) is proposed in the feature info pane
  • adding layers from https://wms.craig.fr/mnt (mapserver 8, this one sets wms_allow_getmap_without_styles):
    • the available formats are correctly listed (text/html/properties/template), i can correctly query a vector layer via html, properties or plain text.
    • For template format, i havent been able to edit it, clicking on the pencil button doesnt show a template editor, and i have this error in the console:
[React Intl] Cannot format message: "layerProperties.templateFormatInfoAlertExample", using message source as fallback. 

this bit is another issue ?

  • adding wms_allow_getmap_without_styles to https://wms.craig.fr/ign and removing extra GFI formats:
    • the defaults advertised in getcapabilities are only application/vnd.ogc.gml and text/plain.
    • Loading a layer from that entrypoint, only text is available in the feature info pane. That's expected.

some tests with mapproxy:

  • loading layers from https://tiles.craig.fr/ign (which advertises text/plain, text/html, text/xml & application/json), i can correctly query layers with the available formats (still no template working)
  • removing extra formats from the mapproxy config, the defaults advertised are text/plain, text/html, text/xml. As expected, only text and html are available in the feature info pane.

some tests with IGN national services:

<ServiceExceptionReport xmlns="http://www.opengis.net/ogc">
<ServiceException code="MissingParameterValue" >
  Parametre FORMAT absent.
</ServiceException>
</ServiceExceptionReport>

either FORMAT is mandatory in the protocol and should be the same value as INFO_FORMAT, or IGN service is too picky/broken.

and a last test with the GPU national service:

I hope this testing covers as much cases as possible. To me there are two issues to worry about:

  • the missing mandatory STYLES parameter, i'll eventually get around tackling it
  • the broken templates format, unsure if its been broken by this PR or something else. edit: it works

@landryb
Copy link
Collaborator

landryb commented Mar 28, 2023

trying to understand more the template issue, i'm still seeing the react intl error msgs in the error console on master, but i now realized where the 'edit template' button was. The template editor indeed works, and querying with a template format works with most layers i've tested coming from mapserver or mapproxy, but in a particular case i've been able to crash ms2:

STR:

Uncaught TypeError: response.features is undefined
    __WEBPACK_DEFAULT_EXPORT__ webpack://mapstore2/./web/client/components/data/identify/viewers/TemplateViewer.jsx?:32
    React 8
mapstore2.js line 7179 > eval:32:1

Some error handling missing in https://github.com/geosolutions-it/MapStore2/blob/master/web/client/components/data/identify/viewers/TemplateViewer.jsx#L18 (or somewhere else in the call stack) for that particular case, but other than that the template format seems to work fine when application/json is made available as a GFI format by mapserver/mapproxy.

@allyoucanmap
Copy link
Contributor Author

I'm a bit lost.. supportedGetMapFormats allowed one to 'override' the hardcoded list of formats supported by mapstore ? The newly hardcoded list after ea42e62 is a bit short.. in my opinion it should be whatever formats the browser is able to display, no ? or only the list of formats supported by OL/leaflet/cesium ? Should that only be 'official' mimetypes ?

what about image/tiff ? i also have servers with image/png; mode=32bit which .. is a valide mimetype (24-bits png with alpha channel). im not so sure too, but there might be servers out there with image/jpg without the e in jpeg..

@landryb We removed the proposed implementation of supportedGetMapFormats because every new format we will include in the list must be tested and we need to ensure the browser and map libraries can properly handle it. So we could add the image/png; mode=32bit mime type to the list if you have a server that could be tested

https://wxs-gpu.mongeoportail.ign.fr/externe/vkd1evhid6jdj5h4hkhyzjto/wms/v?service=WMS&version=1.3.0&request=GetCapabilities provides a large list, from which image/tiff, image/geotiff, image/tiff8 & image/geotiff8 might appear valid in addition to the already supported ones ?

https://wxs.ign.fr/administratif/geoportail/r/wms?service=WMS&version=1.3.0&request=GetCapabilities provides:

<Format>image/jpeg</Format>
<Format>image/png</Format>
<Format>image/tiff</Format>
<Format>image/geotiff</Format>
<Format>image/x-bil;bits=32</Format>
<Format>text/asc</Format>

from this list, ms2 only displays jpeg & png. So, tiff and geotiff are somewhat popular ? are those supported by OL ?

We tested tiff, tiff8, geotiff and geotiff8 from a GeoServer but they are not rendered by the browser and OL. The map library is using the Image class that is equivalent to the <img/> tag and the most common supported formats do not include tiff nor geotiff.
The default list of supported format in MapStore could include also "unofficial" mime type but we need at least a server with the implementation that could be properly tested, so if you have one with image/jpg we could add also this to the list, thanks.
In general the idea is this #9054 (review):

if we will need to include a new format we will provide a new PR to update the list of explicitly supported ones.

@allyoucanmap
Copy link
Contributor Author

@landryb About this two comments #9054 (comment) #9054 (comment) . This PR focus mainly on the Get Map request and it does not change the behavior of the Get Feature Info even if there is an improvement in the parsing. Issues related to the Get Feature Info needs to be added to this other issue #9007.

Copy link
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.

Thank you @allyoucanmap and @offtherailz for the review.
@landryb, according to #9054 (comment) if you have further testable formats to add to the list, just let us know and we will raise a new PR for them.

@allyoucanmap, I think the only things that are still missing are:

@offtherailz offtherailz merged commit e8bec47 into geosolutions-it:master Mar 29, 2023
@tdipisa
Copy link
Member

tdipisa commented Mar 29, 2023

@ElenaGallo please test this in DEV following indications provided by @allyoucanmap in PR description. Connected issues are two. No backport needed for this since it is for 2023.02.00

@landryb
Copy link
Collaborator

landryb commented Mar 29, 2023

So we could add the image/png; mode=32bit mime type to the list if you have a server that could be tested

sorry, that server isnt public.

We tested tiff, tiff8, geotiff and geotiff8 from a GeoServer but they are not rendered by the browser and OL. The map library is using the Image class that is equivalent to the <img/> tag and the most common supported formats do not include tiff nor geotiff.

Right, then it makes sense to not add them. Thanks for the explanation.

@ElenaGallo
Copy link
Contributor

ElenaGallo commented Mar 29, 2023

@allyoucanmap Once the layer is saved, the format of the layer is no longer visible in the level settings.
Sample map

metadata.save.error.mov

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants