feat(WMS): call getCapabilities during layer preprocessing #594

autra · 2017-12-06T15:50:27Z

Description

Add GetCapabilities query for WMS layers. This PR implement the fetching of capabilities and set or check the following informations:

projection
extent
format

BREAKING CHANGE:

WMS layer will now query getCapabilities. If you want to disable this behaviour (because you already provide all the informations in the json object), you need to set layer.disableGetCap to true.
previously we had layer.format and layer.options.mimetype coexisting, with no clear difference in role. This commit removes layer.options.mimetype to only keep layer.format. This is needed to reliably check configured format.

Motivation and Context

Supporting getCapabilities in our providers is a necessary step to present a usable SIG application for the outside world, let's start this.

Moreover, if we want itowns to be able to display a layer from its url only, we need to be able to query capabilities from these layers.

Work that would need to follow this PR:

support getCapabilities for WFS and WMTS
add the possibility to create an entire scene (view + layers) with just a WMS/WMTS/WFS url.

This is a first pass at fixing #285.

autra · 2017-12-06T15:50:43Z

@iTowns/reviewers please r?

gchoqueux

We need to valid the default value of layer.disableGetCap.

Why you change options.mimetype to format?
You have to support both if you make this type of change. (and tag to deprecated)

autra · 2017-12-06T17:25:17Z

Why you change options.mimetype to format?

From the description

previously we had layer.format and layer.options.mimetype coexisting, with no clear difference in role. This commit removes layer.options.mimetype to only keep layer.format. This is needed to reliably check configured format.

2 option for the same thing is not needed. I picked the better.

You have to support both if you make this type of change. (and tag to deprecated)

For such a simple thing? Deprecation process is necessary if the change will notably impact client, preventing them to work before extensive work is done to adapt themselves to a new version. It is not necessary to go into such a process for a simple renaming. As the commit is annotated with a BREAKING CHANGE, it'll appear automatically in the release note. I'll make sure it is clearly visible.

We need to valid the default value of layer.disableGetCap.

It is very surprising that itowns managed to do without getCap this far honestly :-) Every SIG app I'm aware of do that by default. Of course we'll keep a way to bypass it (hence the disableGetCap, the name can be changed btw). It is very useful in some case I agree (for instance for Lyon data, their getCap takes 17 sec :-/ )

I believe this will make iTowns a lot simpler to use, especially for WMTS layers.

autra · 2017-12-11T09:37:23Z

We need to valid the default value of layer.disableGetCap.

Ok let's validate it. What's your thoughts about it?

gchoqueux · 2017-12-11T09:13:41Z

src/Core/Geographic/Coordinates.js

@@ -10,6 +10,8 @@ import mE from '../Math/MathExtended';
 import Ellipsoid from '../Math/Ellipsoid';

 proj4.defs('EPSG:4978', '+proj=geocent +datum=WGS84 +units=m +no_defs');
+// used in some protocol
+proj4.defs('CRS:84', proj4.defs('EPSG:4326'));


can you add comment?

// CRS:84 is equivalent to EPSG:4326 - ie, basic WGS84 degrees.

I don't see the point of this comment. This line is pretty self-explanatory.

not for the WGS84

I see what you mean, but I'm not sure this is the place to explain CRS.

I had to watch on the internet for explanations CRS:84, so it's a good place to explain to the developers. Why it makes sense to add this equivalence because we try to support wgs84 in itowns. It's better to be generous in commentary and it'sn't much to add

gchoqueux · 2017-12-11T09:33:22Z

src/Core/Scheduler/Providers/WMS_Provider.js

+    let supportedCrs = [];
+    const crsPropName = version === '1.3.0' ? 'CRS' : 'SRS';
+
+    for (const childElem of xmlLayer.children) {


Searching for items by tag name is often used. It could be factoring.

Otherwise, it will not be easier to transform the xml object js?
I also found lib js that turns the capes into json.

Searching for items by tag name is often used. It could be factoring.

I've hesitated to do so, but each time it's a bit different. So I decided against it because:

It will only abstract the for loop in the general case (the if part is always a bit different)

it is less readable than the current code. And it's better to optimize for the reader of our code.

Otherwise, it will not be easier to transform the xml object js?

In my opinion, not worthy the extra lib. It won't save me the for loop anyway, and it adds a layer of abstraction that doesn't bring a lot of value here.

To elaborate, as we already have everything we need to query and parse xml in js, I think we can do without. It will avoid growing itowns.js.

gchoqueux · 2017-12-11T09:38:51Z

src/Core/Scheduler/Providers/WMS_Provider.js

+}
+
+function parseGeoBounds(version, xmlLayer) {
+    const geoTagName = version === '1.3.0' ? 'EX_GeographicBoundingBox' : 'LatLonBoundingBox';


you could group in private functions

getCrsPNByVersion(version)... getGeoPNByVersion(version)...

You want a function for a one-liner?

it's just to group the constants
you can suggest something else

Oh I see. Yes, a function should be better than constants here I guess.

gchoqueux · 2017-12-11T09:43:57Z

src/Core/Scheduler/Providers/WMS_Provider.js

+    });
+};
+
+WMS_Provider.prototype.findXmlLayer = function findXMLLayer(name, xml) {


Could you add documentation on new functions?

Oups I forgot! yes will do.

Actually I did better: I only expose them through a _testing export, which avoid polluting the prototype of the provider with private functions.

gchoqueux · 2017-12-11T09:48:05Z

src/Core/Scheduler/Providers/WMS_Provider.js

+    return new Set(_parseSupportedCrs(version, xmlLayer));
+};
+
+WMS_Provider.prototype.parseSupportedFormats = function parseSupportedFormats(version, xmlCapa) {


There's a difference between xmlLayer and xmlCapa

gchoqueux · 2017-12-11T09:56:30Z

src/Core/Scheduler/Providers/WMTS_Provider.js

        options.style = options.style || 'normal';
        options.projection = options.projection || 'EPSG:3857';
        let newBaseUrl = `${layer.url}` +
            `?LAYER=${options.name}` +
-            `&FORMAT=${options.mimetype}` +
+            `&FORMAT=${layer.format}` +


options.mimetype to layer.format to be in another PR, both must be supported with a deprecated message

I can put that in another PR if you want, but I still don't see the need for a depreciation message. I did give my reasons, but you didn't give yours.

I think it breaks the users cases

Yes, but what about my suggestion in my previous comment? If it's clearly documented in the release notes, as it is a trivial change to do, user won't have any difficulty right? They would upgrade their itowns lib, and in the same commit rename this property.

Anyway, I'll see if I can remove this from this PR.

As it breaks the user cases, it's better, we have to support both and put a deprecated message

Here it is: #597

gchoqueux · 2017-12-11T09:57:45Z

src/Core/View.js

+        // if we still haven't any validExtent after getCapabilities, we assume
+        // it is the same as the configured extent
+        if (!layer.validExtent) {
+            layer.validExtent = layer.extent;


and if layer.extent is undefined?

Normally it shouldn't happen: if disableGetCap is true, we check for it. Otherwise the parseExtent method has several fallbacks that cannot reasonably fail.

autra · 2017-12-11T12:05:55Z

@gchoqueux updated.

This now depends on #597

autra · 2018-03-07T14:35:03Z

@iTowns/reviewers updated and rebased :-)

peppsac · 2018-03-07T16:14:38Z

examples/wfs.js

@@ -31,6 +31,7 @@ view.addLayer({
    networkOptions: { crossOrigin: 'anonymous' },
    type: 'color',
    protocol: 'wms',
+    disableGetCap: true, // their getCap call is too slow


Can't this be implicit? If the user supplies all needed paramaters (format, version?) then you assume that the GetCapabilities query is not needed.
WDYT?

It's an interesting idea. The advantage of disableGetCap is that it explicitly ensure getCap won't be called. If it's implicit, it can still be called if you forget something. Maybe we can cover both use cases ? if we have all the parameters, we disable it. If we have disableGetCap, we check the needed parameters and throw if one is missing. WDYT?

Also, WDYT about disableGetCap naming? The more I see it, the more I prefer disableGetCapabilities.

Your suggestion seems fine.

And I prefer disableGetCapabilities (or noGetCapabilities?)

autra · 2018-03-09T13:31:18Z

@peppsac updated

WMS provider call get capabilities and set or check the following information: - projection - extent - format BREAKING CHANGE: WMS layer will now query getCapabilities. If you want to disable this behaviour (because you already provide all the informations in the json object), you need to set layer.disableGetCap to true.

gchoqueux requested changes Dec 6, 2017

View reviewed changes

gchoqueux requested changes Dec 11, 2017

View reviewed changes

autra force-pushed the get_cap_1 branch from 9479e3b to 5dfc5f1 Compare December 11, 2017 12:04

autra mentioned this pull request Dec 20, 2017

feature (protocols): force loading step of group strategy #538

Merged

peppsac mentioned this pull request Jan 24, 2018

fix(examples): wrong tile matrix limit #617

Merged

autra force-pushed the get_cap_1 branch 2 times, most recently from 1c9c4d1 to 6df47f8 Compare March 7, 2018 14:33

autra force-pushed the get_cap_1 branch from 6df47f8 to 127c9eb Compare March 7, 2018 15:12

peppsac reviewed Mar 7, 2018

View reviewed changes

autra force-pushed the get_cap_1 branch from 12c2fb6 to 617221c Compare March 9, 2018 13:42

autra added 3 commits July 13, 2018 18:48

disableGetCap -> disableGetCapabilities

0ed7ed9

skip getCap when all the info are already there

52d8d63

autra force-pushed the get_cap_1 branch from 1f08bfd to 52d8d63 Compare July 13, 2018 16:53

fix linter

ba9d589

gchoqueux added this to the 2.xx.x milestone Feb 1, 2019

autra closed this Jun 19, 2019

feat(WMS): call getCapabilities during layer preprocessing #594

feat(WMS): call getCapabilities during layer preprocessing #594

Conversation

autra commented Dec 6, 2017

Description

Motivation and Context

autra commented Dec 6, 2017

gchoqueux left a comment

Choose a reason for hiding this comment

autra commented Dec 6, 2017

autra commented Dec 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

autra commented Dec 11, 2017

autra commented Mar 7, 2018

peppsac Mar 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

autra commented Mar 9, 2018

peppsac Mar 7, 2018 •

edited

Loading