[Fixes #12124] GNIP 100: Assets #12179

etj · 2024-04-22T15:13:01Z

First implementation of assets.

Also these changes are needed: GeoNode/geonode-importer#235

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: black geonode && 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.

mattiagiupponi · 2024-04-23T13:58:46Z

geonode/assets/utils.py

+        return HttpResponse(status=401)
+
+
+def get_default_asset(resource: ResourceBase) -> Asset or None:


I would rename this. i suggest to call it get_resource_asset otherwise i would expect to have a the value of DEFAULT_ASSET_HANDLER

It returns an Asset, not an AssetHandler.

The first one is the default default bc at the moment we have only a single Asset.
When we'll have multiple assets per resource there will be a way to choose the default one (highest priority?, a pluggable selector?)
get_resource_asset would imply there is only one asset per resource

geonode/base/models.py

mattiagiupponi · 2024-04-23T14:07:35Z

geonode/base/models.py

+    asset_handler_registry.get_handler(instance).remove_data(instance)
+
+
+signals.post_delete.connect(cleanup_asset_data, sender=LocalAsset)


If you connect the signal to "Asset" it should work without defining (in the future) one for each asset

signals.post_delete.connect(cleanup_asset_data, sender=Asset)

mattiagiupponi · 2024-04-23T14:21:03Z

geonode/base/models.py

        if self.resource_type == "dataset":
-            allowed_file = select_relevant_files(get_allowed_extensions(), self.files)
+            from geonode.assets.utils import get_default_asset
+            from geonode.geoserver.helpers import select_relevant_files


Can we just move on top this imports? This API si widely called. Having this here means re-import them everytime. They are cached, yes but part of them can be still re-imported:
https://stackoverflow.com/a/46662587/7597536

that would create a circular reference error

mattiagiupponi · 2024-04-23T14:26:59Z

geonode/documents/api/views.py

+            if file:
+                manager = StorageManager(remote_files={"base_file": file})
+                manager.clone_remote_files()
+                create_asset_and_link(


Instead of cloning the remote files and then copy them again in the settings.ASSET_ROOT, whey dont we improve the clone_remote_files by giving a destination path? In this way we can avoid to perform 3 operations on the file.
In geonode deployment with a huge quanity of document, this file system work can be an issue

keeping the current behaviour so far

geonode/documents/tasks.py

mattiagiupponi · 2024-04-23T14:33:47Z

geonode/resource/manager.py

+                    links = self._copy_data(instance, target=_resource)
+                    files = []
+                    for link in links:
+                        files.extend(link.asset.location)


cant' we just return the location from the _copy_data function instead of the whole object? If yes, this for is not needed

adding as comment

# we're just merging all the files together: it won't work once we have multiple assets per resource # TODO: get the files from the proper Asset

mattiagiupponi · 2024-04-23T14:39:26Z

geonode/base/api/views.py

+
+    def _get_file(self, request, pk, attachment: bool):
+        asset = get_object_or_404(Asset, pk=pk)
+        if bad_response := get_perms_response(request, asset):


can't this be replaced by the IsOwnerOrAdmin permission class? If yes, let's improve the perms class (if needed)

nope: assets have their own permission semantic:
As described in the issue, step 0 is "visibile if owner or admin, or if user has read access on at least one resource attached to the asset"
Furthermore, we may want to make permission also more complex, for instance including group admin or permission directly attached to the Asset. The get_perms_response will encapsulate such logic.

…assets

codecov · 2024-05-07T16:00:46Z

Codecov Report

Attention: Patch coverage is 80.08721% with 137 lines in your changes are missing coverage. Please review.

Project coverage is 64.18%. Comparing base (3454ae5) to head (da90a2a).
Report is 12 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #12179      +/-   ##
==========================================
+ Coverage   64.03%   64.18%   +0.14%     
==========================================
  Files         870      882      +12     
  Lines       52221    52788     +567     
  Branches     6487     6530      +43     
==========================================
+ Hits        33438    33880     +442     
- Misses      17281    17400     +119     
- Partials     1502     1508       +6

etj requested review from giohappy and mattiagiupponi April 22, 2024 15:13

cla-bot bot added the cla-signed CLA Bot: community license agreement signed label Apr 22, 2024

etj force-pushed the 12124_assets branch 3 times, most recently from ef0d851 to c04e13a Compare April 23, 2024 11:40

mattiagiupponi requested changes Apr 23, 2024

View reviewed changes

etj force-pushed the 12124_assets branch 4 times, most recently from ce319e1 to 49d489b Compare April 24, 2024 14:19

[Fixes #12124] GNIP 100: Assets

535d580

etj force-pushed the 12124_assets branch from 49d489b to 535d580 Compare April 24, 2024 15:48

mattiagiupponi and others added 17 commits April 29, 2024 10:45

fix test

03c968d

[Fixes #12124] Fix broken tests

32327a9

[Fixes #12124] Fix broken tests

a6cf9f9

Merge branch 'master' of github.com:GeoNode/geonode into 12124_assets

aa13c5f

[Fixes #12124] Fix broken tests

7737447

[Fixes #12124] Fix broken tests

47c6c13

[Fixes #12124] Fix broken tests

80311a5

[Fixes #12124] Fix broken tests

1ec7613

[Fixes #12124] GNIP 100: Assets - improve model

9d1697b

[Fixes #12124] GNIP 100: Assets - fix resource manager field list

e7c8519

[Fixes #12124] GNIP 100: Assets - minor test "in" fix

c1028fb

Test removing handlerinfo

bdfa14e

Merge branch '12124_assets' of github.com:GeoNode/geonode into 12124_…

89722cb

…assets

Test add logger

bb03c9e

#12124 Remove keepdb from test.sh

1d023cc

#12124 Remove keepdb from test.sh

bb6c54e

Fix test

1aa69fc

Fix format

c50a11c

etj added 5 commits May 7, 2024 18:15

[Fixes #12124] GNIP 100: Assets - improvements in clone and delete

5806872

[Fixes #12124] GNIP 100: Assets - add tests and (un)managed files

291f93e

[Fixes #12124] GNIP 100: Assets - tests

64b4dee

[Fixes #12124] GNIP 100: Assets - improve file handling

1e3b401

[Fixes #12124] GNIP 100: Assets - cleanup tmp doc file

da90a2a

etj linked an issue May 10, 2024 that may be closed by this pull request

GNIP 100: Assets #12124

Open

5 tasks

etj mentioned this pull request May 13, 2024

Assets #12225

Draft

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Fixes #12124] GNIP 100: Assets #12179

[Fixes #12124] GNIP 100: Assets #12179

etj commented Apr 22, 2024 •

edited

mattiagiupponi Apr 23, 2024

etj Apr 23, 2024

mattiagiupponi Apr 23, 2024

mattiagiupponi Apr 23, 2024

etj Apr 23, 2024

mattiagiupponi Apr 23, 2024

etj Apr 23, 2024

mattiagiupponi Apr 23, 2024

etj Apr 23, 2024

mattiagiupponi Apr 23, 2024

etj Apr 23, 2024

codecov bot commented May 7, 2024 •

edited

		return HttpResponse(status=401)


		def get_default_asset(resource: ResourceBase) -> Asset or None:

		asset_handler_registry.get_handler(instance).remove_data(instance)


		signals.post_delete.connect(cleanup_asset_data, sender=LocalAsset)

[Fixes #12124] GNIP 100: Assets #12179

Are you sure you want to change the base?

[Fixes #12124] GNIP 100: Assets #12179

Conversation

etj commented Apr 22, 2024 • edited

Checklist

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 May 7, 2024 • edited

Codecov Report

etj commented Apr 22, 2024 •

edited

codecov bot commented May 7, 2024 •

edited