From a81f0df215db8abeccde05a53422739d80060e9b Mon Sep 17 00:00:00 2001 From: Mattia Giupponi Date: Thu, 23 Jul 2026 15:20:51 +0200 Subject: [PATCH] Test performance improvements --- geonode/base/api/serializers.py | 53 ++++++++++++++++++++++++++------- geonode/base/api/tests.py | 31 +++++++++++++++++++ geonode/base/api/views.py | 8 ++++- geonode/base/models.py | 4 +-- geonode/security/registry.py | 4 +-- 5 files changed, 84 insertions(+), 16 deletions(-) diff --git a/geonode/base/api/serializers.py b/geonode/base/api/serializers.py index 6d8094518d0..28b6571d653 100644 --- a/geonode/base/api/serializers.py +++ b/geonode/base/api/serializers.py @@ -258,13 +258,20 @@ def get_attribute(self, instance): return build_absolute_uri(avatar_url(instance, self.avatar_size)) +def _real_instance(instance): + # memoize the polymorphic downcast: three fields call it per row + if not hasattr(instance, "_real_instance_cache"): + instance._real_instance_cache = instance.get_real_instance() + return instance._real_instance_cache + + class EmbedUrlField(DynamicComputedField): def __init__(self, **kwargs): super().__init__(**kwargs) def get_attribute(self, instance): try: - _instance = instance.get_real_instance() + _instance = _real_instance(instance) except Exception as e: logger.exception(e) _instance = None @@ -302,7 +309,7 @@ def get_attribute(self, instance): logger.info( f"Field {self.field_name} is deprecated and will be removed in the future GeoNode version. Please refer to download_urls" ) - _instance = instance.get_real_instance() + _instance = _real_instance(instance) return _instance.download_url if hasattr(_instance, "download_url") else None except Exception as e: logger.exception(e) @@ -315,7 +322,7 @@ def __init__(self, **kwargs): def get_attribute(self, instance): try: - _instance = instance.get_real_instance() + _instance = _real_instance(instance) except Exception as e: logger.exception(e) raise e @@ -339,14 +346,24 @@ def get_attribute(self, instance): elif _instance.resource_type in ["dataset"]: download_urls = [] + request = self.context.get("request") + # reuse the already-resolved instance instead of re-resolving the alternate per handler + # (_resolve_dataset); gate on the cached download perm to keep the same visibility + _can_download = bool(request) and permissions_registry.user_has_perm( + request.user, _instance, "download_resourcebase", include_virtual=True, use_cache=True + ) # lets get only the default one first to set it default_handler = get_default_dataset_download_handler() - obj = default_handler(self.context.get("request"), _instance.alternate) + obj = default_handler(request, _instance.alternate) + if _can_download: + obj._resource = _instance if obj.download_url: download_urls.append({"url": obj.download_url, "ajax_safe": obj.is_ajax_safe, "default": True}) # then let's prepare the payload with everything for handler in get_download_handlers(): - obj = handler(self.context.get("request"), _instance.alternate) + obj = handler(request, _instance.alternate) + if _can_download: + obj._resource = _instance if obj.download_url: download_urls.append({"url": obj.download_url, "ajax_safe": obj.is_ajax_safe, "default": False}) @@ -365,7 +382,12 @@ def __init__(self, **kwargs): def get_attribute(self, instance): _user = self.context.get("request") if _user and not _user.user.is_anonymous: - return Favorite.objects.filter(object_id=instance.pk, user=_user.user).exists() + # fetch the user favorites once per request, then membership check in python + fav_ids = self.context.get("_favorite_ids") + if fav_ids is None: + fav_ids = set(Favorite.objects.filter(user=_user.user).values_list("object_id", flat=True)) + self.context["_favorite_ids"] = fav_ids + return instance.pk in fav_ids return False @@ -556,9 +578,8 @@ class Meta: def to_representation(self, instance): ret = [] link_fields = ["extension", "link_type", "name", "mime", "url"] - links = Link.objects.filter( - resource_id=instance, # link_type__in=["OGC:WMS", "OGC:WFS", "OGC:WCS", "image", "metadata"] - ) + # select_related asset/resource so the per-link asset + resource lookups below don't each hit the db + links = Link.objects.filter(resource_id=instance).select_related("asset", "resource") request = self.context.get("request", None) for lnk in links: formatted_link = model_to_dict(lnk, fields=link_fields) @@ -571,7 +592,11 @@ def to_representation(self, instance): "content": model_to_dict(lnk.asset, ["title", "description", "type", "created"]), } if request and permissions_registry.user_has_perm( - request.user, lnk.resource.get_self_resource(), "download_resourcebase", include_virtual=True + request.user, + lnk.resource.get_self_resource(), + "download_resourcebase", + include_virtual=True, + use_cache=True, ): extras["content"]["download_url"] = asset_handler_registry.get_handler( lnk.asset @@ -845,8 +870,14 @@ def to_representation(self, instance): data = super().to_representation(instance) if not isinstance(data, int): try: + # the permission-filtered base queryset is identical for every facet row of the + # request, so build it once instead of re-running the whole pipeline per row + base_qs = self.context.get("_count_base_qs") + if base_qs is None: + base_qs = get_resources_with_perms(request.user, filter_options) + self.context["_count_base_qs"] = base_qs count_filter = {self.Meta.count_type: instance} - data["count"] = get_resources_with_perms(request.user, filter_options).filter(**count_filter).count() + data["count"] = base_qs.filter(**count_filter).count() except (TypeError, NoReverseMatch) as e: logger.exception(e) return data diff --git a/geonode/base/api/tests.py b/geonode/base/api/tests.py index 0d0be73cabd..d3867361b75 100644 --- a/geonode/base/api/tests.py +++ b/geonode/base/api/tests.py @@ -75,6 +75,7 @@ License, Group, LinkedResource, + Link, ) from geonode.layers.models import Dataset @@ -112,6 +113,36 @@ def setUpTestData(cls): create_models(b"map") create_models(b"dataset") + def test_download_urls_visibility_follows_download_permission(self): + """download_urls exposes the download link only to users allowed to download the dataset.""" + bobby = get_user_model().objects.get(username="bobby") + norman = get_user_model().objects.get(username="norman") + dataset = create_single_dataset("perm_download_ds", owner=bobby) + download = "http://example.org/perm_download_ds.zip" + try: + Link.objects.create( + resource=dataset.get_self_resource(), + link_type="original", + name="original", + extension="zip", + mime="application/zip", + url=download, + ) + dataset.set_permissions({"users": {norman: ["base.view_resourcebase"]}}) + url = f"{reverse('base-resources-list')}/{dataset.id}?filter{{metadata_only}}=false" + # owner can download: the link is present + self.assertTrue(self.client.login(username="bobby", password="bob")) + response = self.client.get(url, format="json") + self.assertEqual(response.status_code, 200) + self.assertIn(download, [_d["url"] for _d in response.data["resource"]["download_urls"]]) + # user with view but not download: the link is hidden + self.assertTrue(self.client.login(username="norman", password="norman")) + response = self.client.get(url, format="json") + self.assertEqual(response.status_code, 200) + self.assertNotIn(download, [_d["url"] for _d in response.data["resource"]["download_urls"]]) + finally: + dataset.delete() + def test_groups_list(self): """ Ensure we can access the gropus list. diff --git a/geonode/base/api/views.py b/geonode/base/api/views.py index cf6c6b839da..8797c6a3905 100644 --- a/geonode/base/api/views.py +++ b/geonode/base/api/views.py @@ -305,7 +305,13 @@ class ResourceBaseViewSet(ApiPresetsInitializer, MultiLangViewMixin, DeprecatedE ResourceBasePermissionsFilter, FavoriteFilter, ] - queryset = ResourceBase.objects.select_related("owner").order_by("-created") + queryset = ( + ResourceBase.objects.select_related( + "owner", "category", "license", "group", "restriction_code_type", "spatial_representation_type" + ) + # keywords/tkeywords/regions are prefetched by dynamic-rest; contactrole is a custom field, prefetch it here + .prefetch_related("contactrole_set__contact").order_by("-created") + ) serializer_class = ResourceBaseSerializer pagination_class = GeoNodeApiPagination diff --git a/geonode/base/models.py b/geonode/base/models.py index 90850fe7f16..106c93568a7 100644 --- a/geonode/base/models.py +++ b/geonode/base/models.py @@ -1745,8 +1745,8 @@ def __get_contact_role_elements__(self, role: str) -> Optional[List[settings.AUT Optional[List[settings.AUTH_USER_MODEL]]: returns the requested contact role from the database """ try: - contact_role = ContactRole.objects.filter(role=role, resource=self) - contacts = [cr.contact for cr in contact_role] + # single reverse-set read + python filter, instead of one query per role + contacts = [cr.contact for cr in self.contactrole_set.all() if cr.role == role] except ContactRole.DoesNotExist: contacts = None return contacts diff --git a/geonode/security/registry.py b/geonode/security/registry.py index 7a83de590d7..1d2a758e05a 100644 --- a/geonode/security/registry.py +++ b/geonode/security/registry.py @@ -70,7 +70,7 @@ def sanity_checks(self): for item in self.REGISTRY: self.__check_item(item) - def user_has_perm(self, user, instance=None, perm="", include_virtual=False): + def user_has_perm(self, user, instance=None, perm="", include_virtual=False, use_cache=False): """ Returns True if the user has the defined perm (permission) """ @@ -78,7 +78,7 @@ def user_has_perm(self, user, instance=None, perm="", include_virtual=False): return False resolved_perms = self.get_perms( - instance=instance, user=user, include_virtual=True, include_user_add_resource=True + instance=instance, user=user, include_virtual=True, include_user_add_resource=True, use_cache=use_cache ) if isinstance(perm, list):