geosolutions-it
improve metadata display interoperability with mapserver/mapproxy
Description
Rather small PR but fixing many different bugs in the original implementation in #5190 (which was probably only tested against geoserver), cf georchestra/mapstore2-georchestra#300
Please check if the PR fulfills these requirements
- [ ] The commit message follows our guidelines: https://github.com/geosolutions-it/MapStore2/blob/master/CONTRIBUTING.md
- [ ] Tests for the changes have been added (for bug fixes / features)
- [ ] Docs have been added / updated (for bug fixes / features)
What kind of change does this PR introduce? (check one with "x", remove the others)
- [x] Bugfix
- [x] Feature
Issue
What is the current behavior?
georchestra/mapstore2-georchestra#300
What is the new behavior?
with this (and #7851), i'm able to display metadatas (provided CORS is properly configured in mapstore and geonetwork, still working on that on my production setup) from various geonetworks via metadataUrl entries from GetCapabilities documents returned https://wms.craig.fr/ortho (mapserver), https://tiles.craig.fr/ortho/service (mapproxy, multiple layers) and https://tiles.craig.fr/pci/service (mapproxy, one layer) - all those 3 were previously failing with various different problems described in the issue.
Breaking change
Does this PR introduce a breaking change? (check one with "x", remove the other)
- [ ] Yes, and I documented them in migration notes
- [x] No
loading metadata pointed at by geoserver getcapabilities should still work.
Other useful information
locally i have this diff to web/client/epics/catalog.js for printing exceptions, dunno if acceptable:
@@ -408,7 +408,8 @@ export default (API) => ({
.startWith(
showLayerMetadata({}, true)
)
- .catch(() => {
+ .catch((e) => {
+ console.log(e);
return Rx.Observable.of(error({
title: "notification.warning",
message: "toc.layerMetadata.notification.warnigGetMetadataRecordById",
to be probably discussed (feedback from developers more than welcome):
- i havent added tests because i dont have the setup to run them and i wouldnt know where to start
- dunno if reusing
removeWorkspacefrom an epic is a good idea, maybe the function should be moved elsewhere ? - dunno if
web/client/utils/ogc/is the right directory for the mapserver schema - i've relaxed the
iso19115:2003type being hardcoded to also acceptTC211, because even if the WMS 1.3.0 spec says:
The “type” attribute indicates the standard to which the metadata complies.
Two “type” attribute values are defined by this International Standard:
- the value “ISO 19115:2003” refers to ISO 19115:2003
- the value “FGDC:1998” refers to FGDC-STD-001-1998
it also says An information community may define meanings for other “type” attribute values. , and mapserver hardcodes/defaults to TC211 in its generated GetMetadata links, cf https://github.com/MapServer/MapServer/blob/main/mapmetadata.c#L928 and https://mapserver.org/development/rfc/ms-rfc-82.html - that's also one of the values pushed by geonetwork to geoserver, cf https://github.com/georchestra/geonetwork/blob/georchestra-gn4-4.x/services/src/main/java/org/fao/geonet/api/mapservers/GeoServerRest.java#L573 (im trying to get that fixed cf georchestra/geonetwork#197)
as for tests failing, yes i know eslint chokes on web/client/utils/ogc/MS.js but that's a generated file, and i'm not going to try to understand how to reformat it with npm tooling before i have more feedback on the PR.
i think the comment in MS.js clearly says how it has been generated and that it's to support mapserver: https://github.com/geosolutions-it/MapStore2/blob/35f31b22745fc83217303afccb2867de390b9921/web/client/utils/ogc/MS.js#L9
i'll move removeWorkspace and add the eslint comments to the PR.
as for tests i'm not sure how to do that since i dont have the setup to actually run them, and it's not only about TC211 its about processing capabilities coming from non-geoserver :)
Good. only a test for your functionality is sufficient. It is a proof that it retrieves the capabilities and the part you need at the same time.
grm. shouldnt have rebased. @offtherailz what do you think of https://github.com/geosolutions-it/MapStore2/pull/7865/commits/cb374933533b15f1e2411023b2f5d702790514ab ? that takes care of multiple nesting levels that can be possible with mapserver getcapabilities.. i'm sure there's a better way to write this.
- So if you want this feature to be maintained for the future, you may add a test in
catalog-test.jswhere you can check the expected result from a sample Capabilities withTC211that you can add totest-resourcesyou provide.
I'm sorry, but as i said i dont know how to run a single test (npm test just blows here complaining about needing chrome or X or whatever, im on a remote machine) so even if i could write some kind of test (well, i'd need some kind of guidance/example for that, there are bazillions of tests and i dont really understand the ES6 syntax used) i wouldnt manage to run it.
npm run test -- -t getMetadataRecordById
...
....
WARNING in ./node_modules/proj4/lib/version.js 1:0-51
Should not import the named export 'version' (reexported as 'default') from default-exporting module (only default export is available soon)
@ ./node_modules/proj4/lib/index.js 8:0-32 19:16-23
@ ./MapStore2/web/client/utils/ConfigUtils.js 1:42250-42277 1:42728-42735 1:42766-42778 1:49777-49794 1:49799-49811
@ ./MapStore2/web/client/selectors/context.js 1:14658-14704 1:16058-16083
@ ./js/epics/usersession.js 1:9084-9168 1:9461-9480 1:13051-13073
@ ./js/epics/__tests__/usersession-test.js 9:0-105 68:13-37 74:13-35 79:13-34
@ ./js/ sync -test\.jsx?$ ./epics/__tests__/usersession-test.js
@ ./tests.webpack.js 1:14-59
ERROR in ./MapStore2/web/client/utils/ConfigUtils.js 1:42641-42690
Module not found: Error: Can't resolve '@mapstore/patcher' in '/data/src/georchestra/mapstore2-georchestra/MapStore2/web/client/utils'
@ ./MapStore2/web/client/selectors/context.js 1:14658-14704 1:16058-16083
@ ./js/epics/usersession.js 1:9084-9168 1:9461-9480 1:13051-13073
@ ./js/epics/__tests__/usersession-test.js 9:0-105 68:13-37 74:13-35 79:13-34
@ ./js/ sync -test\.jsx?$ ./epics/__tests__/usersession-test.js
@ ./tests.webpack.js 1:14-59
webpack 5.9.0 compiled with 1 error and 1 warning in 16735 ms
24 02 2022 12:39:42.533:WARN [filelist]: Pattern "/data/src/georchestra/mapstore2-georchestra/js/test-resources/**/*" does not match any file.
24 02 2022 12:39:42.682:INFO [karma-server]: Karma v5.2.3 server started at http://10.0.7.20 :9876/
24 02 2022 12:39:42.682:INFO [launcher]: Launching browsers ChromeHeadless with concurrency unlimited
24 02 2022 12:39:42.686:INFO [launcher]: Starting browser ChromeHeadless
24 02 2022 12:39:42.719:ERROR [launcher]: No binary for ChromeHeadless browser on your platform.
Please, set "CHROME_BIN" env variable.
if possible id like to avoid installing chrome just to run a single test.
as for my last commit 240abbb the goal is to fix a buglet i that was in the previous implem:
- if you have a toplevel layer with a metadataurl, but nested layers, the toplevel layer wouldnt be found (since the previous code looked for
capability.layer.layerfirst, and only if not found forcapability.layer) - iterating on 4 levels like this is gross, but we make sure to flatten all the layers found in the GetCapabilities this way
- and i've tested it working fine with layers at 3rd or 4th level in GetCapabilities, which would not have been found with the previous implementation
on the several things i learnt so far wrt running tests:
- not really possible to run ms2 tests from the MapStore2 submodule of ms2-georchestra
- much better to separately build the ms2 project/repo
-
npm test -- --grep web/client/epics/__tests__/catalog-test.jsapparently is supposed to allow passing arguments to karma, but whatever i pass all tests are run
Edit: to run a single testfile, i've resorted to this:
--- a/build/tests-travis.webpack.js
+++ b/build/tests-travis.webpack.js
@@ -1,3 +1,3 @@
-var context = require.context('../web', true, /-test\.jsx?$/);
+var context = require.context('../web/client/epics/', true, /catalog-test\.jsx?$/);
context.keys().forEach(context);
module.exports = context;
@offtherailz i'm trying to add two testcases, but they're failing because they timeout. Should the tests be self-contained, eg they're forbidden to reach outside and fetch XML files, and the metadata XML files to be parsed should be provided as test resources too - and referenced via a relative path instead of a fullblown URL ?
it('getMetadataRecordById-mapserver-TC211', (done) => {
testEpic(getMetadataRecordById, 1, initAction(), (actions) => {
actions.filter( ({type}) => type === SHOW_NOTIFICATION).map(({message}) => {
// eslint-disable-next-line no-console
console.log(message);
expect(1);
done();
});
}, {
layers: {
selected: ["opendata_raw"],
flat: [{
id: "opendata_raw",
catalogURL: "base/web/client/test-resources/wms/getCapabilities-mapserver.xml"
}]
}
});
});
it('getMetadataRecordById-mapproxy-single', (done) => {
testEpic(getMetadataRecordById, 1, initAction(), (actions) => {
actions.filter( ({type}) => type === SHOW_NOTIFICATION).map(({message}) => {
// eslint-disable-next-line no-console
console.log(message);
expect(1);
done();
});
}, {
layers: {
selected: ["cadastre"],
flat: [{
id: "cadastre",
catalogURL: "base/web/client/test-resources/wms/getCapabilities-mapproxy-singlelayer.xml"
}]
}
});
});
(the two capabilities files come from http://wms.craig.fr/ortho?request=getcapabilities&service=wms and http://tiles.craig.fr/pci/service?request=getcapabilities&service=wms)
Hi @landryb,
To speed up test development I suggest npm run continuoustest.
This allows you to re-execute tests on every file save.
You can reduce the number of tests executed with npm run continuoustest by editing the file tests.webpack.js.
You can edit the path and the regex of the files to execute in this file.
Moreover you can click on the debug button in the opened chrome window to have the test enviroment debuggable via chrome-developer-tools.
https://user-images.githubusercontent.com/1279510/155554908-7df452ec-d9de-4b30-ba56-a2ae2f2fbec6.mp4
Should the tests be self-contained, eg they're forbidden to reach outside and fetch XML files, and the metadata XML files to be parsed should be provided as test resources too - and referenced via a relative path instead of a fullblown URL ?
yes, they should be self-contained. Saving the xml in the proper directory (as the other tests) should do the job. If you have a timeout is because the test failed before done or because you had some exception. I suggest to put some break-points in order to understand what is failing.
Hope it helps
yeah, that helps thanks - in the meantime i'm resorting to good'ol console.log debugging, since i dont build on localhost nor use chrome.
extra tip: for console.log output to show up, this is needed in build/testConfig.js:
browserConsoleLogOptions: {
terminal: true,
- level: 'DISABLE'
+ level: ''
},
your feedback on 240abbb is welcome :) i know its gross but it works for me...
yeah, that helps thanks - in the meantime i'm resorting to good'ol console.log debugging, since i dont build on localhost nor use chrome.
extra tip: for console.log output to show up, this is needed in build/testConfig.js:
browserConsoleLogOptions: {
terminal: true,
- level: 'DISABLE'
+ level: ''
},
your feedback on 240abbb is welcome :) i know its gross but it works for me...
well, whatever i'm doing is probably wrong, because with this test:
it('getMetadataRecordById-mapserver-TC211', (done) => {
testEpic(getMetadataRecordById, 1, initAction(), (actions) => {
actions.filter( ({type}) => type === "LAYERS:SHOW_LAYER_METADATA").map(({metadataRecord}) => {
// eslint-disable-next-line no-console
console.log(metadataRecord);
done();
});
}, {
layers: {
selected: ["opendata_raw"],
flat: [{
id: "opendata_raw",
catalogURL: "base/web/client/test-resources/wms/getCapabilities-mapserver.xml"
}]
}
});
});
i can see in the debugging that the LAYERS:SHOW_LAYER_METADATA action is triggered, but metadataRecord is an empty object.
I also figured out that the existing getMetadataRecordById test only checked for the function to return an error message if provided wrong stuff, it doesnt test at all that the function properly returns valid values when given valid input.....
my understanding of the test failure is that regardless of having a local url for the GetCapabilities,
it('getMetadataRecordById-mapserver-TC211', (done) => {
testEpic(getMetadataRecordById, 1, initAction(), (actions) => {
actions.filter( ({type}) => type === "LAYERS:SHOW_LAYER_METADATA").map(({metadataRecord}) => {
// eslint-disable-next-line no-console
console.log(JSON.stringify(metadataRecord));
// eslint-disable-next-line no-console
console.log("pouet");
done();
});
}, {
layers: {
selected: ["opendata_raw"],
flat: [{
id: "opendata_raw",
type: "wms",
url: "base/web/client/test-resources/wms/getCapabilities-mapserver.xml",
catalogURL: "base/web/client/test-resources/wms/getCapabilities-mapserver.xml"
}]
}
});
});
the Promise isnt called in https://github.com/geosolutions-it/MapStore2/blob/master/web/client/api/WMS.js#L123
@offtherailz please look at 77e4046 and tell me what i'm doing wrong. from my understanding of what the debugger shows, the codepaths parsing the capabilities in the method aren't taken.
Hi: I fixed the test. The only issue with the test was that you was doing expect({}).toBe({}) that is like an ===. You had to use toEqual instead, for deep object comparison.
https://github.com/geosolutions-it/MapStore2/pull/7865/commits/35bbb7aacc90a6dfd16cbde0e363796611fc1ba3
right, thanks for that. Anyway, the test itself sucks, since it shouldnt check for {} but for a real returned value, and i dont understand why the methods/promises aren't properly called. At that point that's out of my expertise area...
what im trying to explain is that at runtime, the various codepaths added are properly taken/tested, but from the testing environment something bails out/exits early/something else, and the method doesn't seem called (or exits very early). At that point, im no expert in nodejs/test debugging, so i'd be glad if you could help me with debugging the test itself.
I think in the past month, i've spent a full week on this PR. It would be a win for everyone if geosolutions could spend some time on it for the sake of improved interoperability :)
note that the existing getMetadataRecordById test is also a bogus test, it only checks that the method properly bails out if given garbage....
@landryb, thank you for your contribution. If you cannot provide tests as requested by @offtherailz, we cannot merge this PR now. We will do that for you as soon as possible according to our schedule. Thank you again.
I've backported 3cebff0 on top of my local 2022.01 branch, and tested it in my dev instance:
-
loaded layer
opendata_raw(nested 4 levels of Layers in the capabilities tree) from https://wms.craig.fr/ortho (mapserver), it found the TC211 metadataURL, loaded the automatically generated metadata by mapserver (eg https://wms.craig.fr/ortho?language=fra&request=GetMetadata&layer=opendata_raw) and displayed title and abstract
-
loaded a layer from the local GS, which has two ISO19115:2003 metadataurl
<MetadataURL type="ISO19115:2003">
<Format>text/xml</Format>
<OnlineResource xlink:type="simple" xlink:href="https://xxx/geonetwork/srv/api/records/0274c8e6-60c0-43f7-a27e-50d73e7ac883/formatters/xml"/>
</MetadataURL>
<MetadataURL type="ISO19115:2003">
<Format>text/html</Format>
<OnlineResource xlink:type="simple" xlink:href="https://xxx/geonetwork/srv/eng/catalog.search#/metadata/0274c8e6-60c0-43f7-a27e-50d73e7ac883"/>
</MetadataURL>
- it displayed both the link to the html version and parsed the xml version:
-
loaded
planignlayer from https://wms.craig.fr/ign (mapserver), it found the TC211 metadataurl and fetched and parsed the xml from geonetwork (https://ids.craig.fr/geocat/srv/api/records/d6756eb4-daeb-4d51-a4ce-9756e474a61e/formatters/xml)
-
loaded
ortholayer from https://tiles.craig.fr/ortho (mapproxy) which has several ISO19115:2003text/xmllinks, it fetched the first one from geonetwork (https://ids.craig.fr/geocat/srv/api/records/a03705d7-db7c-446f-af5e-d57b8f80e149/formatters/xml) and properly parsed its xml too
To me, that covers all cases i was testing, and shows it works as expected. Thanks @allyoucanmap for polishing the PR and fixing the tests !
oh, and one last test done, loading a layer from geoserver using a workspace-specific url (eg https://fqdn/geoserver/ws/ows) -> it also finds the ISO19115:2003 metadataurls.
@tdipisa @allyoucanmap should be ready for merging ?
@landryb merged, thanks for the contribution!
@ElenaGallo please test this improvement on dev and let us know when we can backport on 2023.01.xx, thanks
Hi @landryb,
to test this PR and to create functional tests on the metadata tool to be used over time, we need layers, like the ones mentioned in the comment above, to be used on our MapStore as well.
Can you kindly provide a service with layers that has ISO 19115:2003 metadataurl and the ability to display both the html version and the xml version link (like the layers of points 2 and 3)?
I would also need another layer from mapproxy that has several ISO19115:2003 text/xml links, like the ortho layer from point 5.
(the layer above doesn't currently work in the Mapstore because it has a map configuration issue)
Hi @landryb,
to test this PR and to create functional tests on the metadata tool to be used over time, we need layers, like the ones mentioned in the comment above, to be used on our MapStore as well.
Can you kindly provide a service with layers that has ISO 19115:2003 metadataurl and the ability to display both the html version and the xml version link (like the layers of points 2 and 3)?
for that you should be able to use any layer from https://ids.craig.fr/wxs/public/ows?service=wms&version=1.3.0&request=GetCapabilities most of them have the a text/xml entries with ISO 19115:2003 and TC211 and a text/html entry with TC211. https://www.geo2france.fr/geoserver/ows?service=WMS&version=1.3.0&request=GetCapabilities also provides all type/variants of time/format for their wms layers. You can scan for those type keywords in the GetCapabilities response to find the corresponding layer name/title..
Or to avoid relying on external sources that might go away (some layers might be removed over time) you can replicate the setup in any of the geoserver instances managed by geosolutions ?
I would also need another layer from mapproxy that has several ISO19115:2003 text/xml links, like the
ortholayer from point 5. (the layer above doesn't currently work in the Mapstore because it has a map configuration issue)
if it doesn't work, i guess that's because of #8167 ? you should be able to make that layer work by ticking 'singletile' in the layer properties - i can use it without issues with that.
@ElenaGallo
Or to avoid relying on external sources that might go away (some layers might be removed over time) you can replicate the setup in any of the geoserver instances managed by geosolutions ?
I agree with that. If it is possible to setup a test layer in our GS-STABLE with the same output in the WMS getcapabilities it would be better to have full control over time.