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

[3.3.x][Fixes #8112] Dump and restore Geofence rules in case of requests or transaction failures #8113

Merged
merged 8 commits into from Sep 16, 2021

Conversation

italogsfernandes
Copy link
Contributor

@italogsfernandes italogsfernandes commented Sep 13, 2021

References: #8112

What was done

  • GeofenceAdapter and GeofenceLayerRulesUnitOfWork were created:
    • When inside the GeofenceLayerRulesUnitOfWork the geofence transactions were stacked to be executed at the end (__exit__).
  • GeofenceLayerRulesUnitOfWork were used as a context manager.
  • In case of failure:
    • geofence_adapter.rollback() restore the previous state, if needed.

How to test

  • Django Testing
./manage.py test geonode.security.tests.PermissionsTest.test_set_layer_permissions_unit_of_work --keepdb
./manage.py test geonode.security.tests.PermissionsTest.test_layer_set_default_permissions_unit_of_work --keepdb
./manage.py test geonode.security.tests.PermissionsTest.test_layer_permissions_geofence_rollback --keepd
  • Manual Testing:
    • Run the application.
    • Add a layer.
    • Change its permissions:
      image
    • During execution, check the layer rules count using the following geoserver request:
    # Change `localhost:8080` to your geoserver url, `Basic YWRtaW46Z2Vvc2VydmVy' to your geoserver authorization token and `san_andres_y_providencia_coastline0` to your layer name.
    curl --location --request GET 'http://localhost:8080/geoserver/rest/geofence/rules?workspace=geonode&layer=san_andres_y_providencia_coastline0' \
    --header 'Authorization: Basic YWRtaW46Z2Vvc2VydmVy'
    • It should reflect the changed you made using the geonode interface.
  • For manual testing the rollback:
    • Try to raise an Exception after the Unit of Work block (geonode/security/models.py#L422, PermissionLevelMixin.set_permissions).
                                  self.set_dirty_state()
                  raise Exception()
              except Exception as e:
                  geofence_adapter.rollback()
                  raise GeoNodeException(e)
    • Verify at the geoserver (same cURL as above) if the rules list has been modified during the execution and restored at the end.
    • At the geonode interface, you will have the following message:
      image

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.

@cla-bot cla-bot bot added the cla-signed CLA Bot: community license agreement signed label Sep 13, 2021
@italogsfernandes italogsfernandes changed the title [Fixes 8112] Dump and restore Geofence rules in case of requests or transaction failures [Fixes #8112] Dump and restore Geofence rules in case of requests or transaction failures Sep 13, 2021
@italogsfernandes italogsfernandes changed the title [Fixes #8112] Dump and restore Geofence rules in case of requests or transaction failures [Fixes GeoNode#8112] Dump and restore Geofence rules in case of requests or transaction failures Sep 13, 2021
@italogsfernandes italogsfernandes changed the title [Fixes GeoNode#8112] Dump and restore Geofence rules in case of requests or transaction failures [Fixes 8112] Dump and restore Geofence rules in case of requests or transaction failures Sep 13, 2021
@italogsfernandes italogsfernandes self-assigned this Sep 13, 2021
@italogsfernandes italogsfernandes added this to the 3.3.0 milestone Sep 13, 2021
@italogsfernandes italogsfernandes changed the title [Fixes 8112] Dump and restore Geofence rules in case of requests or transaction failures [Fixes #8112] Dump and restore Geofence rules in case of requests or transaction failures Sep 13, 2021
@italogsfernandes italogsfernandes marked this pull request as draft September 13, 2021 11:45
@lgtm-com
Copy link

lgtm-com bot commented Sep 13, 2021

This pull request introduces 1 alert when merging 1869a5a into d8f70e7 - view on LGTM.com

new alerts:

  • 1 for Property in old-style class

@italogsfernandes italogsfernandes marked this pull request as ready for review September 13, 2021 14:07
@codecov
Copy link

codecov bot commented Sep 13, 2021

Codecov Report

Merging #8113 (d1651bb) into 3.3.x (7528f97) will increase coverage by 0.03%.
The diff coverage is 77.74%.

❗ Current head d1651bb differs from pull request most recent head b1aa953. Consider uploading reports for the commit b1aa953 to get more accurate results

@@            Coverage Diff             @@
##            3.3.x    #8113      +/-   ##
==========================================
+ Coverage   55.60%   55.64%   +0.03%     
==========================================
  Files         592      592              
  Lines       42578    42715     +137     
  Branches     5564     5580      +16     
==========================================
+ Hits        23676    23768      +92     
- Misses      17503    17543      +40     
- Partials     1399     1404       +5     

@afabiani afabiani added the backport 3.2.x PR should be backported to target version label Sep 15, 2021
@afabiani
Copy link
Member

@italogsfernandes overall the PR is good and it works as expected. I just added a small commit for an exception on the remote services, not strictly related to this PR.

I would ask if it's possible to add few mocked tests for the classes you just introduced. Thanks.

@italogsfernandes
Copy link
Contributor Author

Ok, thanks @afabiani. I'll be adding the tests.

@italogsfernandes italogsfernandes changed the title [Fixes #8112] Dump and restore Geofence rules in case of requests or transaction failures [3.3.x][[Fixes #8112] Dump and restore Geofence rules in case of requests or transaction failures Sep 15, 2021
@italogsfernandes italogsfernandes changed the title [3.3.x][[Fixes #8112] Dump and restore Geofence rules in case of requests or transaction failures [3.3.x][Fixes #8112] Dump and restore Geofence rules in case of requests or transaction failures Sep 15, 2021
@afabiani afabiani merged commit 0b0cd42 into GeoNode:3.3.x Sep 16, 2021
afabiani pushed a commit that referenced this pull request Sep 16, 2021
…sts or transaction failures (#8113)

* [Fixes #8112] Dump and restore Geofence rules in case of requests or transaction failures

* fix lgtm

* - Making sure the name of a remote layer is correct when fetching the rules

* [CircleCI] Fix Test Cases

* - Fixing a logical issue with layer alternate: the name must not be equal to the alternate prefix

* Add tests and minor fixes

* [Pep8] Fix flake8 issues

Co-authored-by: afabiani <alessio.fabiani@geo-solutions.it>
(cherry picked from commit 0b0cd42)
afabiani pushed a commit that referenced this pull request Sep 16, 2021
…ase of reque… (#8130)

* [3.3.x][Fixes #8112] Dump and restore Geofence rules in case of requests or transaction failures (#8113)

* [Fixes #8112] Dump and restore Geofence rules in case of requests or transaction failures

* fix lgtm

* - Making sure the name of a remote layer is correct when fetching the rules

* [CircleCI] Fix Test Cases

* - Fixing a logical issue with layer alternate: the name must not be equal to the alternate prefix

* Add tests and minor fixes

* [Pep8] Fix flake8 issues

Co-authored-by: afabiani <alessio.fabiani@geo-solutions.it>
(cherry picked from commit 0b0cd42)

* [CircleCI] Fix tests

Co-authored-by: Ítalo Fernandes <italo@italogsfernandes.com>
italogsfernandes added a commit to italogsfernandes/geonode that referenced this pull request Sep 16, 2021
…n case of requests or transaction failures (GeoNode#8113)"

This reverts commit 0b0cd42.
afabiani pushed a commit to italogsfernandes/geonode that referenced this pull request Sep 16, 2021
…n case of requests or transaction failures (GeoNode#8113)"

This reverts commit 0b0cd42.
afabiani pushed a commit that referenced this pull request Sep 17, 2021
#8134)

* Revert "[3.3.x][Fixes #8112] Dump and restore Geofence rules in case of requests or transaction failures (#8113)"

This reverts commit 0b0cd42.

* Revert "[3.3.x][Fixes #8112] Dump and restore Geofence rules in case of requests or transaction failures (#8113)"

This reverts commit 0b0cd42.

* [CircleCI] Fix tests

Co-authored-by: afabiani <alessio.fabiani@geo-solutions.it>
afabiani pushed a commit that referenced this pull request Sep 17, 2021
…the selected ones (#8137)

* Revert "[3.3.x][Fixes #8112] Dump and restore Geofence rules in case of requests or transaction failures (#8113)"

This reverts commit 0b0cd42.

* Revert "[3.3.x][Fixes #8112] Dump and restore Geofence rules in case of requests or transaction failures (#8113)"

This reverts commit 0b0cd42.

* [CircleCI] Fix tests

* [Fixes #8125] [3.x] Initial upload GeoFence permissions do not match the selected ones

* [CircleCI] Fix tests

Co-authored-by: Ítalo Fernandes <italo@italogsfernandes.com>
afabiani pushed a commit that referenced this pull request Sep 17, 2021
…the selected ones (#8137)

* Revert "[3.3.x][Fixes #8112] Dump and restore Geofence rules in case of requests or transaction failures (#8113)"

This reverts commit 0b0cd42.

* Revert "[3.3.x][Fixes #8112] Dump and restore Geofence rules in case of requests or transaction failures (#8113)"

This reverts commit 0b0cd42.

* [CircleCI] Fix tests

* [Fixes #8125] [3.x] Initial upload GeoFence permissions do not match the selected ones

* [CircleCI] Fix tests

Co-authored-by: Ítalo Fernandes <italo@italogsfernandes.com>
(cherry picked from commit e1df8ac)

# Conflicts:
#	geonode/security/models.py
#	geonode/security/tests.py
afabiani pushed a commit that referenced this pull request Sep 18, 2021
…the selected ones (#8137) (#8139)

* Revert "[3.3.x][Fixes #8112] Dump and restore Geofence rules in case of requests or transaction failures (#8113)"

This reverts commit 0b0cd42.

* Revert "[3.3.x][Fixes #8112] Dump and restore Geofence rules in case of requests or transaction failures (#8113)"

This reverts commit 0b0cd42.

* [CircleCI] Fix tests

* [Fixes #8125] [3.x] Initial upload GeoFence permissions do not match the selected ones

* [CircleCI] Fix tests

Co-authored-by: Ítalo Fernandes <italo@italogsfernandes.com>
(cherry picked from commit e1df8ac)

# Conflicts:
#	geonode/security/models.py
#	geonode/security/tests.py
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 3.2.x PR should be backported to target version cla-signed CLA Bot: community license agreement signed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants