Map viewer - support to add layers from ESRI Rest services #5931

josegar74 · 2021-08-24T13:18:50Z

Extend the Add a Layer from > Services with ESRI Rest services:

matself · 2021-08-24T13:28:31Z

Good idea. See how this may improve the REST harvester as well. Beacuse REST harvesting from ESRI does not work. The Portal I mean.

jahow

Haven't tested it but I made some comments on the code, hope that helps! cheers

jahow · 2021-08-24T14:05:30Z

web-ui/src/main/resources/catalog/components/common/map/mapService.js

          addEsriRestFromScratch: function(map, url, name, createOnly, md) {
            if (url === '') {
              var error = "Trying to add an ESRI layer with no service URL. Layer name is " + name + ". Check the metadata or the map.";
              console.warn(error);
              return $q.reject(error);
            }
            var serviceUrl = url.replace(/(.*\/MapServer).*/, '$1');
-            var layer = !!name && parseInt(name).toString() === name
+            var layer = angular.isNumber(name)


That does not work with the comment block above, which is stating that name is a string

name is not a string, at least from my tests. Check https://land.discomap.eea.europa.eu/arcgis/rest/services/Agriculture/agri_extensification_on_HNVfarmland06/MapServer?f=pjson

The name is taken from the layer ids, also id=0 is valid, and caused issues with !!name that was evaluated to false

"layers": [ { "id": 0,

jahow · 2021-08-24T14:06:54Z

web-ui/src/main/resources/catalog/components/common/map/mapService.js

+           * If the md object is given, we add it to the layer, or we try
+           * to retrieve it in the catalog


This contradicts the name of the methods wich ends with FromScratch; I think it makes sense to rename this method addEsriRestLayer for example

This method already existed, it's not from this pull request. Can you explain why contradicts the name of the methods which ends with FromScratch?

This method already existed, it's not from this pull request.

I know, but this doesn't mean we cannot rename it.

"From scratch" means, in the context of the GN map service, that it does not need to be fed a capabilities document; besides, there is usually another "from capabilities" function that comes with it. For ESRI services it does not work like that so I suggest we remove the "from scratch" from the method name as it is misleading.

Ok, I don't see that much of an issue to use the FromScratch suffix (the method was already there, hasn't been added in this pull request), as the method doesn't use a capabilities document. But if it makes the code more clear will update it.

I understand, but I also disagree: while it may appear minor, an incorrect/misleading function name can significantly diminish the readability of the code. A widely accepted good practice is that whichever piece of code you alter should be cleaner after your changes than prior; this can include minor refactorings like this.

That said, I was simply making a comment about something that striked me as incorrect, not specifically requesting changes on your behalf.

It's already updated

jahow · 2021-08-24T14:09:47Z

web-ui/src/main/resources/catalog/components/common/map/mapService.js

+           *
+           * @param {ol.Map} map to add the layer
+           * @param {string} url of the service
+           * @param {string} name of the layer (identifier)


Although this was admittedly not clear at all before, the name parameter is optional I think. If not present, the name will be inferred from the URL. Why this is like that, I'm not sure... The logic around name and layer in this method is unclear to me.

For me was quite confusing the usage of that parameters, just tried to document it. Please check as at least part of the original code was added by you, maybe you can remember the logic for that variables.

Ok so I looked in more details and the name parameter is basically a stringified number used to identify a layer in a map. So 0, 1, 2 etc. The weird thing is that this variable is reused later on to store an arbitrary "layer name" in the form of mapId layerNumber, so for example my-map 0. On the other hand, the layer variable is used to store the layer name instead...

The confusing part is that a service url can contain a layer name (e.g. my-map/MapServer/0) and as such we can infer the layer name from the service url in case the provided layer name argument is incorrect (i.e. not a valid number in string format). This is all unnecessarily complicated, but I understand that this was out of your PR scope. I guess the bare minimum would be to make sure that the function is properly documented (especially the name and url arguments).

jahow · 2021-08-24T14:10:22Z

web-ui/src/main/resources/catalog/components/common/map/mapService.js

              ? name
              : url.replace(/.*\/([^\/]*)\/MapServer\/?(.*)/, '$2');
            name = url.replace(/.*\/([^\/]*)\/MapServer\/?(.*)/, '$1 $2');

-            var olLayer = getTheLayerFromMap(map, name, url);
+            // Use the url and the layer identifier to check if the layer exists
+            var olLayer = getTheLayerFromMap(map, layer, url);


You're switching from layer to name here, is that intentional? It looks like this could break things.

getTheLayerFromMap was not supporting ESRI Rest layers, so that code was useless and should be used the layer that is the idenfifier, not the string value that is also unclear why is implemented like this:

name = url.replace(/.*\/([^\/]*)\/MapServer\/?(.*)/, '$1 $2');

For example for https://land.discomap.eea.europa.eu/arcgis/rest/services/Agriculture/agri_extensification_on_HNVfarmland06/MapServer

sets name to agri_extensification_on_HNVfarmland06

For https://land.discomap.eea.europa.eu/arcgis/rest/services/Agriculture/agri_extensification_on_HNVfarmland06/MapServer/0

sets name to agri_extensification_on_HNVfarmland06 0

it's totally unclear the reason for that, but that can't be used to identify the layer in the map.

jahow · 2021-08-24T14:13:01Z

web-ui/src/main/resources/catalog/components/viewer/wmsimport/partials/esrilayer.html

+      <div class="flex-col flex-grow">
+        <a href=""
+           class="truncate"
+           data-ng-click="addLayer(null, $event)"


addLayer() would be enough here I think

ok, will update, copied from similar existing code for WMS services.

jahow · 2021-08-24T14:13:12Z

web-ui/src/main/resources/catalog/components/viewer/wmsimport/WmsImportDirective.js

+            evt.stopPropagation();
+          };
+
+          scope.addLayer = function(c) {


Unused function argument

jahow · 2021-08-24T14:16:40Z

web-ui/src/main/resources/catalog/components/viewer/wmsimport/WmsImportDirective.js

+    '$translate',
+    function($translate) {
+
+      var label= $translate.instant('filter');


This should go on the scope and be used as {{ ::label }} in the template (might as well use angularjs for templating 😉 )

also this would remove the dependency to $translate

This was copied for similar code in other existing directive...

jahow · 2021-08-24T14:18:59Z

web-ui/src/main/resources/catalog/components/viewer/wmsimport/WmsImportDirective.js

+            controller.addEsriRestLayer(scope.member);
+          };
+
+          scope.isParentNode = false;


This is weird, looks like this directive is built to handle nested layers, but does not actually support it. IIRC layers in ESRI map services are not organized in a tree like in the WMS, so I guess you could get rid of everything related to layer groups and simplify the whole thing.

Future maintainers will thank you.

It was copied from other directive for WMS, will cleanup this as I was not sure if nested was supported. I leaved in case that is supported to extend it later.

jahow · 2021-08-24T14:20:38Z

web-ui/src/main/resources/catalog/components/common/map/esriService.js

+          if (url) {
+            url = url + '?f=json';
+
+            if (gnUrlUtils.isValid(url)) {


If the URL is not valid the returned promise will never resolve nor reject; I think you can remove this check and let $http fail like it should on a malformed URL.

ok, will update.

jahow · 2021-08-24T14:21:28Z

web-ui/src/main/resources/catalog/components/common/map/esriService.js

+                timeout: timeout
+              })
+                .success(function(data, status, headers, config) {
+                  if (!!data.mapName) {


What does this check mean? This might require a comment, because it looks like you received a valid response and still decided to throw a "capabilities failed" error.

Checks if the response has a property mapName, that is expected to be in the ESRI Rest service document, to avoid you configure any url like https://github.com

Ok, then it would make sense to return another error, like "The response was not a valid ESRI Rest document" or something like that. I'm not sure this is what happens with other services, we probably just let them fail once they try to parse a non existent property or because of the content-type.

…rovements

…nMap.addEsriRestFromScratch method

jahow

Thanks for the improvements! The AddEsriLayer method is still very confusing in its logic, but at least it is better documented now.

matself · 2021-09-02T17:08:06Z

Great addition - I believe. I have just refreshed and built a brand new GN 4 instance. Do I need some configuration setting to get the ESRI option under Services?
Second, as asked above: Will it be possible to harvest REST services as well?

josegar74 · 2021-09-03T06:15:50Z

@matself the pull request is not yet merged in the main branch. I hope is merged soon to be available in the main branch (4.0.x version).

About the harvester, that's another feature, not the scope of this pull request. I recommend to open a new issue for this topic. Generally, contributions to GeoNetwork are made through user pull requests or if don't have the skills by hiring a developer who can develop the feature and make a pull request. At https://geonetwork-opensource.org/ you can find a list of some of the companies (among others) that work on GeoNetwork.

…into esrirest_services

Map viewer - support to add layers from ESRI Rest services

18ee700

josegar74 added this to the 4.0.6 milestone Aug 24, 2021

josegar74 requested review from fxprunayre and jahow August 24, 2021 13:18

jahow reviewed Aug 24, 2021

View reviewed changes

josegar74 added 2 commits August 25, 2021 09:02

Map viewer - support to add layers from ESRI Rest services - code imp…

63d8163

…rovements

Map viewer - support to add layers from ESRI Rest services - rename g…

03bff3a

…nMap.addEsriRestFromScratch method

jahow approved these changes Sep 2, 2021

View reviewed changes

Merge branch 'main' of https://github.com/geonetwork/core-geonetwork …

8b14098

…into esrirest_services

josegar74 merged commit 695c20e into geonetwork:main Nov 8, 2021

josegar74 added the backport 3.12.x label Nov 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Map viewer - support to add layers from ESRI Rest services #5931

Map viewer - support to add layers from ESRI Rest services #5931

josegar74 commented Aug 24, 2021

matself commented Aug 24, 2021

jahow left a comment

jahow Aug 24, 2021

josegar74 Aug 24, 2021

jahow Aug 24, 2021

josegar74 Aug 25, 2021

jahow Aug 25, 2021

josegar74 Aug 25, 2021 •

edited

Loading

jahow Aug 25, 2021

josegar74 Aug 25, 2021

jahow Aug 24, 2021

josegar74 Aug 24, 2021

jahow Sep 2, 2021

jahow Aug 24, 2021

josegar74 Aug 24, 2021

jahow Aug 24, 2021

josegar74 Aug 24, 2021

jahow Aug 24, 2021

jahow Aug 24, 2021

jahow Aug 24, 2021

josegar74 Aug 24, 2021

jahow Aug 24, 2021

josegar74 Aug 24, 2021

jahow Aug 24, 2021

josegar74 Aug 24, 2021

jahow Aug 24, 2021

josegar74 Aug 24, 2021

jahow Aug 24, 2021

jahow left a comment

matself commented Sep 2, 2021

josegar74 commented Sep 3, 2021

		* If the md object is given, we add it to the layer, or we try
		* to retrieve it in the catalog

Map viewer - support to add layers from ESRI Rest services #5931

Map viewer - support to add layers from ESRI Rest services #5931

Conversation

josegar74 commented Aug 24, 2021

matself commented Aug 24, 2021

jahow left a 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

josegar74 Aug 25, 2021 • edited Loading

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

jahow left a comment

Choose a reason for hiding this comment

matself commented Sep 2, 2021

josegar74 commented Sep 3, 2021

josegar74 Aug 25, 2021 •

edited

Loading