[Fixes #10537] Improve rules creation using GeoFence batch - more imp… #10585

etj · 2023-01-25T18:39:20Z

A continuation of #10538.
Replaces #10556 since it had too many conflicts.

Batch creation have been moved to caller methods (wrt previous refactoring), in order to have a single Batch containing all the operations needed for a full perm syncronization.

Checklist

Reviewing is a process done by project maintainers, mostly on a volunteer basis. We try to keep the overhead as small as possible and appreciate if you help us to do so by completing the following items. Feel free to ask in a comment if you have troubles with any of them.

For all pull requests:

Confirm you have read the contribution guidelines
You have sent a Contribution Licence Agreement (CLA) as necessary (not required for small changes, e.g., fixing typos in the documentation)
Make sure the first PR targets the master branch, eventual backports will be managed later. This can be ignored if the PR is fixing an issue that only happens in a specific branch, but not in newer ones.

The following are required only for core and extension modules (they are welcomed, but not required, for contrib modules):

There is a ticket in https://github.com/GeoNode/geonode/issues describing the issue/improvement/feature (a notable exemption is, changes not visible to end-users)
The issue connected to the PR must have Labels and Milestone assigned
PR for bug fixes and small new features are presented as a single commit
Commit message must be in the form "[Fixes #<issue_number>] Title of the Issue"
New unit tests have been added covering the changes, unless there is an explanation on why the tests are not necessary/implemented
This PR passes all existing unit tests (test results will be reported by travis-ci after opening this PR)
This PR passes the QA checks: flake8 geonode
Commits changing the settings, UI, existing user workflows, or adding new functionality, need to include documentation updates
Commits adding new texts do use gettext and have updated .po / .mo files (without location infos)

Submitting the PR does not require you to check all items, but by the time it gets merged, they should be either satisfied or inapplicable.

codecov · 2023-01-26T12:20:17Z

Codecov Report

Merging #10585 (a2c766b) into master (e440111) will increase coverage by 0.01%.
The diff coverage is 84.25%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #10585      +/-   ##
==========================================
+ Coverage   62.11%   62.13%   +0.01%     
==========================================
  Files         828      828              
  Lines       51144    51174      +30     
  Branches     6554     6554              
==========================================
+ Hits        31768    31795      +27     
+ Misses      17695    17694       -1     
- Partials     1681     1685       +4

…rovements

mattiagiupponi · 2023-01-26T13:15:19Z

geonode/api/tests.py

-        self.api_client.client.login(username=self.user, password=self.passwd)
-        resp = self.api_client.get(f"{list_url + str(layer.id)}/")
-        self.assertValidJSONResponse(resp)
+        resp = self.client.get(list_url)


We are not performing anymore the login, is something wanted?

The login is on line 157, these are some more tests as unauth user

mattiagiupponi · 2023-01-26T13:20:10Z

geonode/geoserver/geofence.py

                verify=False,
            )

            if r.status_code not in (200, 201):
-                logger.debug(f"Could not insert rule: [{r.status_code}] - {r.content}")
-                raise GeofenceException(f"Could not insert rule: [{r.status_code}]")
+                logger.debug(f"Could not insert rule: [{r.status_code}] - {r.text}")


Since its raising an exception, it might be better to increase this log level from debug or remove it since is raising the exception

Usually the detailed information should be logged when debugging.
It's not said that the exception message is displayed at higher level.

mattiagiupponi · 2023-01-26T13:22:01Z

geonode/geoserver/geofence.py

@@ -203,38 +207,64 @@ def add_insert_rule(self, rule: Rule):
        operation.update(rule.get_object())
        self.operations.append(operation)

-    def get_batch_length(self):
+    def length(self) -> int:


Isn't this a property more than a method?

By leaving it as a method, it can be extended by filtering only certain types of operations

mattiagiupponi · 2023-01-26T13:23:24Z

geonode/geoserver/geofence.py

                verify=False,
            )

            if r.status_code != 200:
-                logger.debug(f"Error while running batch {batch.log_name}: [{r.status_code}] - {r.content}")
+                logger.debug(


It might be useful to increase this log level if it shows additional information than the Exception.

Usually the detailed information should be logged only when debugging

mattiagiupponi · 2023-01-26T13:23:55Z

geonode/geoserver/geofence.py


        except Exception as e:
-            logger.debug(f"Error while requesting batch execution {batch.log_name}", exc_info=e)
+            logger.info(f"Error while requesting batch exec {batch.log_name}")
+            logger.debug(f"Error while requesting batch exec {batch.log_name} --> {batch.get_object()}", exc_info=e)


It might be useful to increase this log level if it shows additional information than the Exception.

Usually the detailed information should be logged only when debugging

mattiagiupponi · 2023-01-26T13:27:21Z

geonode/geoserver/geofence.py

+                for r in gs_rules["rules"]:
+                    if r["layer"] and r["layer"] == layer_name:
+                        batch.add_delete_rule(r["id"])
+                        cnt += 1


Isn't the count provided by the batch.length() as soon as all the rules are added?

No: if the delete operations are collected in an existing batch, lenght would be > than the delete operations inserted

mattiagiupponi · 2023-01-26T13:31:21Z

geonode/geoserver/manager.py

                if isinstance(_resource, Dataset):
-                    if settings.OGC_SERVER["default"].get("GEOFENCE_SECURITY_ENABLED", False):
+                    if settings.OGC_SERVER["default"].get("GEOFENCE_SECURITY_ENABLED", False) or getattr(
+                        settings, "GEOFENCE_SECURITY_ENABLED", False


This is redundant. The GEOFENCE_SECURITY_ENABLED in the settings is used to populate the OGC_SERVER["default"]["GEOFENCE_SECURITY_ENABLED"]

Nope, tests are only setting the GEOFENCE_SECURITY_ENABLED entry, not the other one

mattiagiupponi · 2023-01-26T13:33:31Z

geonode/geoserver/manager.py

+                            if not created:
+                                gf_utils.collect_delete_layer_rules(workspace, _resource.name, batch)
+
+                            exist_geolimits = None


i would suggest to have it as "False" and not None since the name say "exists"

We need the 3 different values:

true -> limit exists

false -> limit does not exist

None -> not computed

#10585) * [Code Formatting] Revert formatting on conflicting files * [Fixes #10537] Improve rules creation using GeoFence batch - more improvements * [#10574] Align code formatting with black Co-authored-by: afabiani <alessio.fabiani@geosolutionsgroup.com> (cherry picked from commit 36f414d)

#10585) (#10588) * [Code Formatting] Revert formatting on conflicting files * [Fixes #10537] Improve rules creation using GeoFence batch - more improvements * [#10574] Align code formatting with black Co-authored-by: afabiani <alessio.fabiani@geosolutionsgroup.com> (cherry picked from commit 36f414d) Co-authored-by: Emanuele Tajariol <etj@geo-solutions.it>

…ore imp… (GeoNode#10585) (GeoNode#10588) * [Code Formatting] Revert formatting on conflicting files * [Fixes GeoNode#10537] Improve rules creation using GeoFence batch - more improvements * [GeoNode#10574] Align code formatting with black Co-authored-by: afabiani <alessio.fabiani@geosolutionsgroup.com> (cherry picked from commit 36f414d) Co-authored-by: Emanuele Tajariol <etj@geo-solutions.it>

cla-bot bot added the cla-signed CLA Bot: community license agreement signed label Jan 25, 2023

etj self-assigned this Jan 25, 2023

etj requested a review from mattiagiupponi January 25, 2023 18:39

etj force-pushed the 10537-geofence-batch-advanced-2 branch from a172525 to 1263da5 Compare January 26, 2023 10:01

etj mentioned this pull request Jan 26, 2023

[Fixes #10537] Improve rules creation using GeoFence batch - further improvements #10556

Closed

12 tasks

[Code Formatting] Revert formatting on conflicting files

b25fbdc

etj force-pushed the 10537-geofence-batch-advanced-2 branch 2 times, most recently from 83f00ff to f18221d Compare January 26, 2023 11:43

etj added 2 commits January 26, 2023 13:37

[Fixes #10537] Improve rules creation using GeoFence batch - more imp…

4b75a3d

…rovements

[#10574] Align code formatting with black

a2c766b

etj force-pushed the 10537-geofence-batch-advanced-2 branch from f18221d to a2c766b Compare January 26, 2023 12:43

mattiagiupponi requested changes Jan 26, 2023

View reviewed changes

etj marked this pull request as ready for review January 26, 2023 15:07

afabiani merged commit 36f414d into master Jan 26, 2023

afabiani deleted the 10537-geofence-batch-advanced-2 branch January 26, 2023 15:32

afabiani added the backport 4.1.x label Jan 26, 2023

afabiani added this to the 4.1.0 milestone Jan 26, 2023

github-actions bot mentioned this pull request Jan 26, 2023

[Backport 4.1.x] [Fixes #10537] Improve rules creation using GeoFence batch - more imp… #10588

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Fixes #10537] Improve rules creation using GeoFence batch - more imp… #10585

[Fixes #10537] Improve rules creation using GeoFence batch - more imp… #10585

etj commented Jan 25, 2023

codecov bot commented Jan 26, 2023 •

edited

Loading

mattiagiupponi Jan 26, 2023

etj Jan 26, 2023

mattiagiupponi Jan 26, 2023

etj Jan 26, 2023

mattiagiupponi Jan 26, 2023

etj Jan 26, 2023

mattiagiupponi Jan 26, 2023

etj Jan 26, 2023

mattiagiupponi Jan 26, 2023

etj Jan 26, 2023

mattiagiupponi Jan 26, 2023

etj Jan 26, 2023

mattiagiupponi Jan 26, 2023

etj Jan 26, 2023

mattiagiupponi Jan 26, 2023

etj Jan 26, 2023

[Fixes #10537] Improve rules creation using GeoFence batch - more imp… #10585

[Fixes #10537] Improve rules creation using GeoFence batch - more imp… #10585

Conversation

etj commented Jan 25, 2023

Checklist

codecov bot commented Jan 26, 2023 • edited Loading

Codecov Report

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

codecov bot commented Jan 26, 2023 •

edited

Loading