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

GetFeatureInfo exceptions parameter format configuration #9466

Closed
ale-cristofori opened this issue Sep 20, 2023 · 5 comments · Fixed by #9471 or #9562
Closed

GetFeatureInfo exceptions parameter format configuration #9466

ale-cristofori opened this issue Sep 20, 2023 · 5 comments · Fixed by #9471 or #9562

Comments

@ale-cristofori
Copy link
Contributor

ale-cristofori commented Sep 20, 2023

Hi

In MapStore we want to include webservices from "Digitaal Vlaanderen" (eg. https://geo.api.vlaanderen.be/dhmv/wms)

When MapStore sends a GetFeatureInfo request to those services, no result shows up in the identify panel. When taking a look at the response in the network tab of the browser, it is clear that an error occurred:

   <?xml version="1.0" encoding="UTF-8" standalone="no" ?>
  <!DOCTYPE ServiceExceptionReport SYSTEM "http://schemas.opengis.net/wms/1.1.1/exception_1_1_1.dtd">
  <ServiceExceptionReport version="1.1.1">
  <ServiceException code="InvalidFormat">
  Parameter 'exceptions' contains unacceptable value.
  </ServiceException>
  </ServiceExceptionReport>

I see that MapStore adds to every GetFeatureInfo request the following parameter: "exceptions=application%2Fjson"
This parameter seems to be hardcoded in the file MapStore2\web\client\utils\mapinfo\wms.js

When taking a look at the service https://geo.api.vlaanderen.be/dhmv/wms?service=WMS&version=1.1.1&request=GetCapabilities, it is clear the exceptions parameter value that is being send by MapStore is not supported by the service:

<Exception>
  <Format>application/vnd.ogc.se_xml</Format>
  <Format>application/vnd.ogc.se_inimage</Format>
  <Format>application/vnd.ogc.se_blank</Format>
</Exception>

For our own MapStore GeoServers, the format application/json is supported though, so we didn't experienced this problem in the past.

We had contact with the owner of the failing services and they are not planning to add support other exceptions parameters at short term.

In order to fix the issue of malfunctioning Identify, we think it is necessary to make some changes to MapStore to support other exceptions parameter and allowing to configure them (not hardcoded), or to be able to disable the exceptions parameter.
Or is there already a way to achieve this that I missed?

What is your option about this? Can you give an estimation on the work needed to be done for this feature request?

@ale-cristofori
Copy link
Contributor Author

Removing the exception=json parameter from here do not seems to cause any problem. https://github.com/geosolutions-it/MapStore2/blob/master/web/client/utils/mapinfo/wms.js#L62 In fact the exception type doesn't look to be used in any case.
We can check a little more around, but I think ti can be solved simply removing it at all, at least make it dependent on "Server Type", but it may be an unuseful complication.

We should:

test forcing an error from GFI (E.g. breaking up the freemarker template)
Verify that the exception json format is not necessary
If not necessary, remove it
IF necessary, find a fix that makes it work in all service types (e.g. use a common format and parse the exception, or make it vendor dependent).

@MV88
Copy link
Contributor

MV88 commented Sep 21, 2023

Hi @tdipisa @offtherailz

me and @mahmoudadel54 had a discussion over this task and we noticed that we can simply remove the exceptions parameter but then this has to be handled here differently

response.data.exceptions
? exceptionsFeatureInfo(reqId, response.data.exceptions, requestParams, lMetaData)
: loadFeatureInfo(reqId, response.data, requestParams, { ...lMetaData, features: response.features, featuresCrs: response.featuresCrs }, layer)
)
.catch((e) => Rx.Observable.of(errorFeatureInfo(reqId, e.data || e.statusText || e.status, requestParams, lMetaData)))

in particular from now on the gfi request will used the default value for exceptions when an exception happens.

Consider also that the error info is not even used and those requests that fail or have some errors are simply filtered out here

if (["exceptions", "error"].includes(type)) {
const fltRequests = state.requests.filter((_, index)=> index !== requestIndex);
const fltResponses = state.responses.filter((_, index)=> index !== requestIndex);
return {
...state, responses: fltResponses, requests: fltRequests
};
}

resulting in the feature info (given only one layer failing) to open and close, immediately

I was thinking that since we ignore the error exception we can show a notification message for each layer that fails with a message similar to this one: "The GFI request for the layer has failed and will be discarded"

getfeatureInfo.mp4

What do you think about it?


there is also a weird behaviour with arcgis that if you use text/plain and you clicks where there is no data or outside layer extent, it will give 400 but using text/xml will returns valid empty body response

@tdipisa
Copy link
Member

tdipisa commented Sep 21, 2023

@MV88

me and @mahmoudadel54 had a discussion over this task and we noticed that we can simply remove the exceptions parameter but then this has to be handled here differently

To be clarified with @offtherailz if necessary.

I was thinking that since we ignore the error exception we can show a notification message for each layer that fails with a message similar to this one: "The GFI request for the layer has failed and will be discarded"

Only if it does not imply big changes on the UI/UX (maybe you can clarify it better) and if we can strictly stay in the expected effort. Otherwise a dedicated issue must be opened for a future evolution.

there is also a weird behaviour with arcgis that if you use text/plain and you clicks where there is no data or outside layer extent, it will give 400 but using text/xml will returns valid empty body response

That's another issue, not related to the subject of this one. Therefore, nothing to do for the moment.

@MV88
Copy link
Contributor

MV88 commented Sep 21, 2023

following a discussion with @offtherailz we suggest to something little different from above.

@mahmoudadel54 you can do this

Observable.defer(() => axios.get(basePath, { params }))
        .let(interceptOGCError)

  • in the identify you can remove the ternary and simply so something similar
.map((response) => loadFeatureInfo(reqId, response.data, requestParams, { ...lMetaData, features: response.features, featuresCrs: response.featuresCrs }, layer)
                            )
                            .catch((e) => Rx.Observable.of(errorFeatureInfo(reqId, e.data || e.statusText || e.status, requestParams, lMetaData)))
  • check that places where the usage of EXCEPTIONS_FEATURE_INFO are still intercepting it, if needed dispatch it from in the catch
  • ignore adding a notification for now
  • remove the unnecessary ternary (now exceptions are managed only by catch clausole.
  • remove the unncecessary duplicated action. (now only one handles error).

for any questions you can ask me or at @offtherailz

@offtherailz
Copy link
Member

offtherailz commented Sep 21, 2023

I discussed with @MV88 about the solution proposed above, so I'm aware of it.
Please contact me @mahmoudadel54 if @MV88 if you are blocked and @MV88 is OOO.
Thank you.

mahmoudadel54 added a commit to mahmoudadel54/MapStore2 that referenced this issue Sep 21, 2023
…iguration

Desription: - put a default format for GFI exceptions - add getIdentifyFlow to catch GFI errors and exceptions
mahmoudadel54 added a commit to mahmoudadel54/MapStore2 that referenced this issue Sep 22, 2023
Description: fix unit test failing results, create new unit test for getIdentifyFlow for wms
mahmoudadel54 added a commit to mahmoudadel54/MapStore2 that referenced this issue Sep 26, 2023
edit in wms-test of mapInfo file by adding a unit test for exception
mahmoudadel54 added a commit to mahmoudadel54/MapStore2 that referenced this issue Sep 26, 2023
mahmoudadel54 added a commit to mahmoudadel54/MapStore2 that referenced this issue Sep 28, 2023
MV88 pushed a commit that referenced this issue Sep 28, 2023
…ion (#9471)

* #9466: GetFeatureInfo exceptions parameter format configuration

Desription: - put a default format for GFI exceptions - add getIdentifyFlow to catch GFI errors and exceptions
* #9466: resolve reviewer comments
Description: fix unit test failing results, create new unit test for getIdentifyFlow for wms
* resolve review comments
add test cases for getIdentifyFlow in wms-test
* #9466: resolve review comments
edit in wms-test of mapInfo file by adding a unit test for exception
* #9466: resolve eslint errors in unit test
* #9466: add unit test for a exception response in string format for mapInfo/wms-test
@ElenaGallo ElenaGallo self-assigned this Oct 4, 2023
mahmoudadel54 added a commit to mahmoudadel54/MapStore2 that referenced this issue Oct 5, 2023
…nfiguration

Desription: - put a default format for GFI exceptions - add getIdentifyFlow to catch GFI errors and exceptions
* geosolutions-it#9466: resolve reviewer comments
Description: fix unit test failing results, create new unit test for getIdentifyFlow for wms
* resolve review comments
add test cases for getIdentifyFlow in wms-test
* geosolutions-it#9466: resolve review comments
edit in wms-test of mapInfo file by adding a unit test for exception
* geosolutions-it#9466: resolve eslint errors in unit test
* geosolutions-it#9466: add unit test for a exception response in string format for mapInfo/wms-test
mahmoudadel54 added a commit to mahmoudadel54/MapStore2 that referenced this issue Oct 5, 2023
…nfiguration

Desription: - put a default format for GFI exceptions - add getIdentifyFlow to catch GFI errors and exceptions
* geosolutions-it#9466: resolve reviewer comments
Description: fix unit test failing results, create new unit test for getIdentifyFlow for wms
* resolve review comments
add test cases for getIdentifyFlow in wms-test
* geosolutions-it#9466: resolve review comments
edit in wms-test of mapInfo file by adding a unit test for exception
* geosolutions-it#9466: resolve eslint errors in unit test
* geosolutions-it#9466: add unit test for a exception response in string format for mapInfo/wms-test
@MV88 MV88 added the BackportNeeded Commits provided for an issue need to be backported to the milestone's stable branch label Oct 5, 2023
MV88 pushed a commit that referenced this issue Oct 5, 2023
)

Desription: - put a default format for GFI exceptions - add getIdentifyFlow to catch GFI errors and exceptions
* #9466: resolve reviewer comments
Description: fix unit test failing results, create new unit test for getIdentifyFlow for wms
* resolve review comments
add test cases for getIdentifyFlow in wms-test
* #9466: resolve review comments
edit in wms-test of mapInfo file by adding a unit test for exception
* #9466: resolve eslint errors in unit test
* #9466: add unit test for a exception response in string format for mapInfo/wms-test
@ElenaGallo ElenaGallo added Accepted and removed BackportNeeded Commits provided for an issue need to be backported to the milestone's stable branch labels Oct 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment