From 8c6aea3ed34a48e2b7b1b320912db175a3510550 Mon Sep 17 00:00:00 2001 From: Oleh Paduchak Date: Mon, 6 Jan 2025 16:02:53 +0200 Subject: [PATCH 1/7] fixed n+1 queries --- .../addon_operation_invocation/serializers.py | 11 ++++++- .../addon_operation_invocation/views.py | 26 ++++++++++++++++- addon_service/authentication.py | 2 ++ addon_service/common/serializer_fields.py | 29 +++++++++++-------- .../configured_addon/storage/views.py | 7 ++++- .../external_service/citation/views.py | 4 ++- .../external_service/computing/views.py | 4 ++- .../external_service/storage/views.py | 4 ++- addon_service/resource_reference/models.py | 26 ++++++++++++++--- addon_service/user_reference/models.py | 12 ++++++-- 10 files changed, 100 insertions(+), 25 deletions(-) diff --git a/addon_service/addon_operation_invocation/serializers.py b/addon_service/addon_operation_invocation/serializers.py index 07460742..8aefc312 100644 --- a/addon_service/addon_operation_invocation/serializers.py +++ b/addon_service/addon_operation_invocation/serializers.py @@ -68,7 +68,12 @@ class Meta: thru_addon = CustomPolymorphicResourceRelatedField( many=False, required=False, - queryset=ConfiguredAddon.objects.active(), + queryset=ConfiguredAddon.objects.active().select_related( + "base_account__external_service", + "base_account__authorizedstorageaccount", + "base_account__authorizedcitationaccount", + "base_account__account_owner", + ), related_link_view_name=view_names.related_view(RESOURCE_TYPE), polymorphic_serializer=ConfiguredAddonPolymorphicSerializer, ) @@ -92,6 +97,10 @@ class Meta: "by_user": "addon_service.serializers.UserReferenceSerializer", } + def to_internal_value(self, data): + validated_data = super().to_internal_value(data) + return validated_data + def create(self, validated_data): _thru_addon = validated_data.get("thru_addon") _thru_account = validated_data.get("thru_account") diff --git a/addon_service/addon_operation_invocation/views.py b/addon_service/addon_operation_invocation/views.py index c58d6719..2f94a917 100644 --- a/addon_service/addon_operation_invocation/views.py +++ b/addon_service/addon_operation_invocation/views.py @@ -86,8 +86,31 @@ def retrieve_related(self, request, *args, **kwargs): def perform_create(self, serializer): super().perform_create(serializer) + narrowed_down_types = ( + [ + "thru_addon__configuredcitationaddon", + "thru_addon__base_account__authorizedcitationaccount", + ] + if "citation" + in serializer.initial_data.get( + "thru_addon", serializer.initial_data.get("thru_account") + )["type"] + else [ + "thru_addon__configuredstorageaddon", + "thru_addon__base_account__authorizedstorageaccount", + ] + ) # after creating the AddonOperationInvocation, look into invoking it - _invocation = serializer.instance + _invocation = ( + AddonOperationInvocation.objects.filter(pk=serializer.instance.pk) + .select_related( + "thru_addon__base_account__external_service", + *narrowed_down_types, + "thru_addon__base_account___credentials", + "thru_account___credentials", + ) + .first() + ) _operation_type = _invocation.operation.operation_type match _operation_type: case AddonOperationType.REDIRECT | AddonOperationType.IMMEDIATE: @@ -96,3 +119,4 @@ def perform_create(self, serializer): perform_invocation__celery.delay(_invocation.pk) case _: raise ValueError(f"unknown operation type: {_operation_type}") + serializer.instance = _invocation diff --git a/addon_service/authentication.py b/addon_service/authentication.py index 82d2cbb6..8d1c482c 100644 --- a/addon_service/authentication.py +++ b/addon_service/authentication.py @@ -9,6 +9,8 @@ class GVCombinedAuthentication(drf_authentication.BaseAuthentication): """Authentication supporting session, basic, and token methods.""" def authenticate(self, request: DrfRequest): + if request.session.get("user_reference_uri"): + return True, None _user_uri = osf.get_osf_user_uri(request) if _user_uri: UserReference.objects.get_or_create(user_uri=_user_uri) diff --git a/addon_service/common/serializer_fields.py b/addon_service/common/serializer_fields.py index a818a434..1d63488d 100644 --- a/addon_service/common/serializer_fields.py +++ b/addon_service/common/serializer_fields.py @@ -8,6 +8,9 @@ SkipDataMixin, ) +from addon_service.authorized_account.models import AuthorizedAccount +from addon_service.configured_addon.models import ConfiguredAddon + class ReadOnlyResourceRelatedField( json_api_serializers.ResourceRelatedField, drf_serializers.ReadOnlyField @@ -54,16 +57,18 @@ def get(self, *, pk): class CustomPolymorphicResourceRelatedField(PolymorphicResourceRelatedField): def to_representation(self, value): data = super().to_representation(value) - if hasattr(value, "authorizedcitationaccount"): - data["type"] = "authorized-citation-accounts" - elif hasattr(value, "authorizedcomputingaccount"): - data["type"] = "authorized-computing-accounts" - elif hasattr(value, "authorizedstorageaccount"): - data["type"] = "authorized-storage-accounts" - elif hasattr(value, "configuredcitationaddon"): - data["type"] = "configured-citation-addons" - elif hasattr(value, "configuredcomputingaddon"): - data["type"] = "configured-computing-addons" - elif hasattr(value, "configuredstorageaccount"): - data["type"] = "configured-storage-addons" + if isinstance(value, AuthorizedAccount): + if hasattr(value, "authorizedcitationaccount"): + data["type"] = "authorized-citation-accounts" + elif hasattr(value, "authorizedstorageaccount"): + data["type"] = "authorized-storage-accounts" + elif hasattr(value, "authorizedcomputingaccount"): + data["type"] = "authorized-computing-accounts" + elif isinstance(value, ConfiguredAddon): + if hasattr(value, "configuredcitationaddon"): + data["type"] = "configured-citation-addons" + elif hasattr(value, "configuredstorageaddon"): + data["type"] = "configured-storage-addons" + elif hasattr(value, "configuredcomputingaddon"): + data["type"] = "configured-computing-addons" return data diff --git a/addon_service/configured_addon/storage/views.py b/addon_service/configured_addon/storage/views.py index 10d4a521..722900e9 100644 --- a/addon_service/configured_addon/storage/views.py +++ b/addon_service/configured_addon/storage/views.py @@ -12,7 +12,12 @@ class ConfiguredStorageAddonViewSet(ConfiguredAddonViewSet): - queryset = ConfiguredStorageAddon.objects.active() + queryset = ConfiguredStorageAddon.objects.active().select_related( + "base_account__authorizedstorageaccount", + "base_account__account_owner", + "base_account__external_service__externalstorageservice", + "authorized_resource", + ) serializer_class = ConfiguredStorageAddonSerializer @action( diff --git a/addon_service/external_service/citation/views.py b/addon_service/external_service/citation/views.py index c267ce42..b8e9fa03 100644 --- a/addon_service/external_service/citation/views.py +++ b/addon_service/external_service/citation/views.py @@ -5,5 +5,7 @@ class ExternalCitationServiceViewSet(ReadOnlyModelViewSet): - queryset = ExternalCitationService.objects.all() + queryset = ExternalCitationService.objects.all().select_related( + "oauth2_client_config", "oauth1_client_config" + ) serializer_class = ExternalCitationServiceSerializer diff --git a/addon_service/external_service/computing/views.py b/addon_service/external_service/computing/views.py index 146b5adb..b52baf0e 100644 --- a/addon_service/external_service/computing/views.py +++ b/addon_service/external_service/computing/views.py @@ -5,5 +5,7 @@ class ExternalComputingServiceViewSet(ReadOnlyModelViewSet): - queryset = ExternalComputingService.objects.all() + queryset = ExternalComputingService.objects.all().select_related( + "oauth2_client_config" + ) serializer_class = ExternalComputingServiceSerializer diff --git a/addon_service/external_service/storage/views.py b/addon_service/external_service/storage/views.py index fe487475..703f48c2 100644 --- a/addon_service/external_service/storage/views.py +++ b/addon_service/external_service/storage/views.py @@ -5,5 +5,7 @@ class ExternalStorageServiceViewSet(ReadOnlyModelViewSet): - queryset = ExternalStorageService.objects.all() + queryset = ExternalStorageService.objects.all().select_related( + "oauth2_client_config" + ) serializer_class = ExternalStorageServiceSerializer diff --git a/addon_service/resource_reference/models.py b/addon_service/resource_reference/models.py index d8725d00..d9004be6 100644 --- a/addon_service/resource_reference/models.py +++ b/addon_service/resource_reference/models.py @@ -12,17 +12,35 @@ class ResourceReference(AddonsServiceBaseModel): @property def configured_storage_addons(self): - return ConfiguredStorageAddon.objects.filter(authorized_resource=self).order_by( - Lower("_display_name") + return ( + ConfiguredStorageAddon.objects.filter(authorized_resource=self) + .select_related( + "base_account__external_service__externalstorageservice", + "base_account__authorizedstorageaccount", + "base_account__account_owner", + ) + .order_by(Lower("_display_name")) ) @property def configured_citation_addons(self): - return ConfiguredCitationAddon.objects.filter(authorized_resource=self) + return ConfiguredCitationAddon.objects.filter( + authorized_resource=self + ).select_related( + "base_account__external_service__externalcitationservice", + "base_account__authorizedcitationaccount", + "base_account__account_owner", + ) @property def configured_computing_addons(self): - return ConfiguredComputingAddon.objects.filter(authorized_resource=self) + return ConfiguredComputingAddon.objects.filter( + authorized_resource=self + ).select_related( + "base_account__external_service__externalcomputingservice", + "base_account__authorizedcitationaccount", + "base_account__account_owner", + ) class Meta: verbose_name = "Resource Reference" diff --git a/addon_service/user_reference/models.py b/addon_service/user_reference/models.py index 05fc108e..e54ad38d 100644 --- a/addon_service/user_reference/models.py +++ b/addon_service/user_reference/models.py @@ -39,15 +39,21 @@ def configured_resources(self): @property def authorized_storage_accounts(self): - return AuthorizedStorageAccount.objects.filter(account_owner=self) + return AuthorizedStorageAccount.objects.filter( + account_owner=self + ).select_related("external_service") @property def authorized_citation_accounts(self): - return AuthorizedCitationAccount.objects.filter(account_owner=self) + return AuthorizedCitationAccount.objects.filter( + account_owner=self + ).select_related("external_service") @property def authorized_computing_accounts(self): - return AuthorizedComputingAccount.objects.filter(account_owner=self) + return AuthorizedComputingAccount.objects.filter( + account_owner=self + ).select_related("external_service") class Meta: verbose_name = "User Reference" From 3c1f84c1734821f10a134516528ebdeae281296f Mon Sep 17 00:00:00 2001 From: Oleh Paduchak Date: Mon, 6 Jan 2025 16:04:51 +0200 Subject: [PATCH 2/7] reverted changes to AddonOperationInvocation --- .../addon_operation_invocation/views.py | 26 +------------------ app/settings.py | 3 +++ app/urls.py | 1 + 3 files changed, 5 insertions(+), 25 deletions(-) diff --git a/addon_service/addon_operation_invocation/views.py b/addon_service/addon_operation_invocation/views.py index 2f94a917..c58d6719 100644 --- a/addon_service/addon_operation_invocation/views.py +++ b/addon_service/addon_operation_invocation/views.py @@ -86,31 +86,8 @@ def retrieve_related(self, request, *args, **kwargs): def perform_create(self, serializer): super().perform_create(serializer) - narrowed_down_types = ( - [ - "thru_addon__configuredcitationaddon", - "thru_addon__base_account__authorizedcitationaccount", - ] - if "citation" - in serializer.initial_data.get( - "thru_addon", serializer.initial_data.get("thru_account") - )["type"] - else [ - "thru_addon__configuredstorageaddon", - "thru_addon__base_account__authorizedstorageaccount", - ] - ) # after creating the AddonOperationInvocation, look into invoking it - _invocation = ( - AddonOperationInvocation.objects.filter(pk=serializer.instance.pk) - .select_related( - "thru_addon__base_account__external_service", - *narrowed_down_types, - "thru_addon__base_account___credentials", - "thru_account___credentials", - ) - .first() - ) + _invocation = serializer.instance _operation_type = _invocation.operation.operation_type match _operation_type: case AddonOperationType.REDIRECT | AddonOperationType.IMMEDIATE: @@ -119,4 +96,3 @@ def perform_create(self, serializer): perform_invocation__celery.delay(_invocation.pk) case _: raise ValueError(f"unknown operation type: {_operation_type}") - serializer.instance = _invocation diff --git a/app/settings.py b/app/settings.py index 636dc875..063e92b2 100644 --- a/app/settings.py +++ b/app/settings.py @@ -83,6 +83,7 @@ # Application definition INSTALLED_APPS = [ + "silk", "django.contrib.auth", "django.contrib.contenttypes", "django.contrib.sessions", @@ -101,6 +102,7 @@ ASGI_APPLICATION = "app.asgi.application" MIDDLEWARE = [ + "silk.middleware.SilkyMiddleware", "corsheaders.middleware.CorsMiddleware", "django.middleware.security.SecurityMiddleware", "django.contrib.sessions.middleware.SessionMiddleware", @@ -168,6 +170,7 @@ }, } +SILKY_PYTHON_PROFILER = True DATABASE_ROUTERS = ["addon_service.osf_models.db_router.OsfDatabaseRouter"] REST_FRAMEWORK = { diff --git a/app/urls.py b/app/urls.py index 9d8a7df2..f97bacad 100644 --- a/app/urls.py +++ b/app/urls.py @@ -14,4 +14,5 @@ RedirectView.as_view(url="/static/gravyvalet_code_docs/index.html"), name="docs-root", ), + path("silk/", include("silk.urls", namespace="silk")), ] From 755a28859a4ad890ca9719bda4c44a6f9d647d51 Mon Sep 17 00:00:00 2001 From: Oleh Paduchak Date: Tue, 7 Jan 2025 16:34:20 +0200 Subject: [PATCH 3/7] added django-silk as dev dependency --- app/settings.py | 17 ++++++++++------- app/urls.py | 5 ++++- poetry.lock | 44 +++++++++++++++++++++++++++++++++++++++++++- pyproject.toml | 1 + 4 files changed, 58 insertions(+), 9 deletions(-) diff --git a/app/settings.py b/app/settings.py index 063e92b2..eec43218 100644 --- a/app/settings.py +++ b/app/settings.py @@ -83,7 +83,6 @@ # Application definition INSTALLED_APPS = [ - "silk", "django.contrib.auth", "django.contrib.contenttypes", "django.contrib.sessions", @@ -96,13 +95,7 @@ "addon_service", ] -if DEBUG: - # run under ASGI locally: - INSTALLED_APPS.insert(0, "daphne") # django's reference asgi server - ASGI_APPLICATION = "app.asgi.application" - MIDDLEWARE = [ - "silk.middleware.SilkyMiddleware", "corsheaders.middleware.CorsMiddleware", "django.middleware.security.SecurityMiddleware", "django.contrib.sessions.middleware.SessionMiddleware", @@ -113,6 +106,16 @@ "django.middleware.clickjacking.XFrameOptionsMiddleware", ] + +if DEBUG: + # add django-silk to enable profiling + INSTALLED_APPS.append("silk") + MIDDLEWARE.insert(0, "silk.middleware.SilkyMiddleware") + # run under ASGI locally: + INSTALLED_APPS.insert(0, "daphne") # django's reference asgi server + ASGI_APPLICATION = "app.asgi.application" + + ROOT_URLCONF = "app.urls" TEMPLATES = [ diff --git a/app/urls.py b/app/urls.py index f97bacad..1e307e51 100644 --- a/app/urls.py +++ b/app/urls.py @@ -1,3 +1,4 @@ +from django.conf import settings from django.contrib import admin from django.urls import ( include, @@ -14,5 +15,7 @@ RedirectView.as_view(url="/static/gravyvalet_code_docs/index.html"), name="docs-root", ), - path("silk/", include("silk.urls", namespace="silk")), ] + +if "silk" in settings.INSTALLED_APPS: + urlpatterns.append(path("silk/", include("silk.urls", namespace="silk"))) diff --git a/poetry.lock b/poetry.lock index d195bc8f..dc30143b 100644 --- a/poetry.lock +++ b/poetry.lock @@ -199,6 +199,20 @@ files = [ [package.extras] visualize = ["Twisted (>=16.1.1)", "graphviz (>0.5.1)"] +[[package]] +name = "autopep8" +version = "2.3.1" +description = "A tool that automatically formats Python code to conform to the PEP 8 style guide" +optional = false +python-versions = ">=3.8" +files = [ + {file = "autopep8-2.3.1-py2.py3-none-any.whl", hash = "sha256:a203fe0fcad7939987422140ab17a930f684763bf7335bdb6709991dd7ef6c2d"}, + {file = "autopep8-2.3.1.tar.gz", hash = "sha256:8d6c87eba648fdcfc83e29b788910b8643171c395d9c4bcf115ece035b9c9dda"}, +] + +[package.dependencies] +pycodestyle = ">=2.12.0" + [[package]] name = "billiard" version = "4.2.0" @@ -733,6 +747,23 @@ files = [ [package.dependencies] Django = ">=4.2" +[[package]] +name = "django-silk" +version = "5.3.2" +description = "Silky smooth profiling for the Django Framework" +optional = false +python-versions = ">=3.9" +files = [ + {file = "django_silk-5.3.2-py3-none-any.whl", hash = "sha256:49f1caebfda28b1707f0cfef524e0476beb82b8c5e40f5ccff7f73a6b4f6d3ac"}, + {file = "django_silk-5.3.2.tar.gz", hash = "sha256:b0db54eebedb8d16f572321bd6daccac0bd3f547ae2618bb45d96fe8fc02229d"}, +] + +[package.dependencies] +autopep8 = "*" +Django = ">=4.2" +gprof2dot = ">=2017.09.19" +sqlparse = "*" + [[package]] name = "djangorestframework" version = "3.14.0" @@ -919,6 +950,17 @@ files = [ {file = "frozenlist-1.4.1.tar.gz", hash = "sha256:c037a86e8513059a2613aaba4d817bb90b9d9b6b69aace3ce9c877e8c8ed402b"}, ] +[[package]] +name = "gprof2dot" +version = "2024.6.6" +description = "Generate a dot graph from the output of several profilers." +optional = false +python-versions = ">=3.8" +files = [ + {file = "gprof2dot-2024.6.6-py2.py3-none-any.whl", hash = "sha256:45b14ad7ce64e299c8f526881007b9eb2c6b75505d5613e96e66ee4d5ab33696"}, + {file = "gprof2dot-2024.6.6.tar.gz", hash = "sha256:fa1420c60025a9eb7734f65225b4da02a10fc6dd741b37fa129bc6b41951e5ab"}, +] + [[package]] name = "hyperlink" version = "21.0.0" @@ -2209,4 +2251,4 @@ testing = ["coverage (>=5.0.3)", "zope.event", "zope.testing"] [metadata] lock-version = "2.0" python-versions = "^3.12" -content-hash = "f2198ba94898f4260705bc395c05b70dff5b019786c9c0011da0ba92852f92b4" +content-hash = "9a504ffb729f32f4824b2e07fa523c8bf40f460e3082a51edcf7d1424208e566" diff --git a/pyproject.toml b/pyproject.toml index 84cbfc1d..17e06d9e 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -32,6 +32,7 @@ flake8 = "^7.1.1" isort = "^5.13.2" pre-commit = "^3.8.0" pdoc = "14.5.1" +django-silk = "^5.3.2" [tool.poetry.group.release.dependencies] sentry-sdk = "2.7.1" From 97a17c87b05fea4194386b2e75a3fae50d5becee Mon Sep 17 00:00:00 2001 From: Oleh Paduchak Date: Tue, 7 Jan 2025 16:35:08 +0200 Subject: [PATCH 4/7] performance optimisations to addon operation invocations --- .../addon_operation_invocation/views.py | 22 +++++++- addon_service/common/network.py | 4 +- addon_service/common/osf.py | 1 + addon_service/credentials/models.py | 21 +++----- addon_service/tasks/invocation.py | 51 ++++++++++--------- 5 files changed, 59 insertions(+), 40 deletions(-) diff --git a/addon_service/addon_operation_invocation/views.py b/addon_service/addon_operation_invocation/views.py index c58d6719..6e2a86bc 100644 --- a/addon_service/addon_operation_invocation/views.py +++ b/addon_service/addon_operation_invocation/views.py @@ -87,7 +87,15 @@ def retrieve_related(self, request, *args, **kwargs): def perform_create(self, serializer): super().perform_create(serializer) # after creating the AddonOperationInvocation, look into invoking it - _invocation = serializer.instance + _invocation = ( + AddonOperationInvocation.objects.filter(pk=serializer.instance.pk) + .select_related( + *self._get_narrowed_down_selects(serializer), + "thru_account___credentials", + ) + .first() + ) + _invocation.thru_addon.base_account = _invocation.thru_account _operation_type = _invocation.operation.operation_type match _operation_type: case AddonOperationType.REDIRECT | AddonOperationType.IMMEDIATE: @@ -96,3 +104,15 @@ def perform_create(self, serializer): perform_invocation__celery.delay(_invocation.pk) case _: raise ValueError(f"unknown operation type: {_operation_type}") + serializer.instance = _invocation + + def _get_narrowed_down_selects(self, serializer): + addon_resource_name = serializer.initial_data.get( + "thru_addon", serializer.initial_data.get("thru_account") + )["type"] + addon_type = addon_resource_name.split("-")[1] + return [ + f"thru_addon__configured{addon_type}addon", + f"thru_account__authorized{addon_type}account", + f"thru_account__external_service__external{addon_type}service", + ] diff --git a/addon_service/common/network.py b/addon_service/common/network.py index 1e923a60..0e8d5f21 100644 --- a/addon_service/common/network.py +++ b/addon_service/common/network.py @@ -83,7 +83,9 @@ async def _do_send(self, request: HttpRequestInfo): async with self._try_send(request) as _response: yield _response except exceptions.ExpiredAccessToken: - await _PrivateNetworkInfo.get(self).account.refresh_oauth2_access_token() + await _PrivateNetworkInfo.get( + self + ).account.refresh_oauth_access_token__async() # if this one fails, don't try refreshing again async with self._try_send(request) as _response: yield _response diff --git a/addon_service/common/osf.py b/addon_service/common/osf.py index 5add3e58..10a7582b 100644 --- a/addon_service/common/osf.py +++ b/addon_service/common/osf.py @@ -91,6 +91,7 @@ async def has_osf_permission_on_resource( params={ "resolve": "f", # do not redirect to the referent "embed": "referent", # instead, include the referent in the response + "fields[nodes]": "current_user_permissions", }, headers=[ *_get_osf_auth_headers(request), diff --git a/addon_service/credentials/models.py b/addon_service/credentials/models.py index a0ebe128..627d1690 100644 --- a/addon_service/credentials/models.py +++ b/addon_service/credentials/models.py @@ -88,37 +88,32 @@ def _key_parameters(self, value: encryption.KeyParameters) -> None: ### @property - def authorized_accounts(self): + def authorized_account(self): """Returns the list of all accounts that point to this set of credentials. For now, this will just be a single AuthorizedAccount, but in the future other types of accounts for the same user could point to the same set of credentials """ try: - return [ - *filter( - bool, - [ - getattr(self, "authorized_account", None), - getattr(self, "temporary_authorized_account", None), - ], - ) - ] + if account := getattr(self, "authorized_account", None): + return account + else: + return getattr(self, "temporary_authorized_account", None) except ExternalCredentials.authorized_account.RelatedObjectDoesNotExist: return None @property def format(self): - if not self.authorized_accounts: + if not self.authorized_account: return None - return self.authorized_accounts[0].external_service.credentials_format + return self.authorized_account.external_service.credentials_format def clean_fields(self, *args, **kwargs): super().clean_fields(*args, **kwargs) self._validate_credentials() def _validate_credentials(self): - if not self.authorized_accounts: + if not self.authorized_account: return try: self.decrypted_credentials diff --git a/addon_service/tasks/invocation.py b/addon_service/tasks/invocation.py index a76cb5e3..430becad 100644 --- a/addon_service/tasks/invocation.py +++ b/addon_service/tasks/invocation.py @@ -17,33 +17,34 @@ def perform_invocation__blocking(invocation: AddonOperationInvocation) -> None: """perform the given invocation: run an operation thru an addon and handle any errors""" # implemented as a sync function for django transactions - with dibs(invocation): # TODO: handle dibs errors - try: - _imp = get_addon_instance__blocking( - invocation.imp_cls, # type: ignore[arg-type] #(TODO: generic impstantiation) - invocation.thru_account, - invocation.config, - ) - _operation = invocation.operation - # inner transaction to contain database errors, - # so status can be saved in the outer transaction (from `dibs`) - with transaction.atomic(): - _result = _imp.invoke_operation__blocking( - _operation.declaration, - invocation.operation_kwargs, - ) - invocation.operation_result = json_for_typed_value( - _operation.declaration.result_dataclass, - _result, + try: + _imp = get_addon_instance__blocking( + invocation.imp_cls, # type: ignore[arg-type] #(TODO: generic impstantiation) + invocation.thru_account, + invocation.config, + ) + _operation = invocation.operation + # inner transaction to contain database errors, + # so status can be saved in the outer transaction (from `dibs`) + with transaction.atomic(): + _result = _imp.invoke_operation__blocking( + _operation.declaration, + invocation.operation_kwargs, ) - invocation.invocation_status = InvocationStatus.SUCCESS - except BaseException as _e: - invocation.set_exception(_e) - raise # TODO: or swallow? - finally: - invocation.save() + invocation.operation_result = json_for_typed_value( + _operation.declaration.result_dataclass, + _result, + ) + invocation.invocation_status = InvocationStatus.SUCCESS + except BaseException as _e: + invocation.set_exception(_e) + raise # TODO: or swallow? + finally: + invocation.save() @celery.shared_task(acks_late=True) def perform_invocation__celery(invocation_pk: str) -> None: - perform_invocation__blocking(AddonOperationInvocation.objects.get(pk=invocation_pk)) + invocation = AddonOperationInvocation.objects.get(pk=invocation_pk) + with dibs(invocation): # TODO: handle dibs errors + perform_invocation__blocking(invocation) From 6865a6bc7a943e898a0d92bf55439e0a4e8b2b4b Mon Sep 17 00:00:00 2001 From: Oleh Paduchak Date: Tue, 7 Jan 2025 17:06:15 +0200 Subject: [PATCH 5/7] fixed tests --- .../addon_operation_invocation/views.py | 3 ++- addon_service/credentials/models.py | 21 ++++++++++++------- app/env.py | 3 +++ app/settings.py | 3 ++- 4 files changed, 20 insertions(+), 10 deletions(-) diff --git a/addon_service/addon_operation_invocation/views.py b/addon_service/addon_operation_invocation/views.py index 6e2a86bc..b1b10e32 100644 --- a/addon_service/addon_operation_invocation/views.py +++ b/addon_service/addon_operation_invocation/views.py @@ -95,7 +95,8 @@ def perform_create(self, serializer): ) .first() ) - _invocation.thru_addon.base_account = _invocation.thru_account + if _invocation.thru_addon: + _invocation.thru_addon.base_account = _invocation.thru_account _operation_type = _invocation.operation.operation_type match _operation_type: case AddonOperationType.REDIRECT | AddonOperationType.IMMEDIATE: diff --git a/addon_service/credentials/models.py b/addon_service/credentials/models.py index 627d1690..a0ebe128 100644 --- a/addon_service/credentials/models.py +++ b/addon_service/credentials/models.py @@ -88,32 +88,37 @@ def _key_parameters(self, value: encryption.KeyParameters) -> None: ### @property - def authorized_account(self): + def authorized_accounts(self): """Returns the list of all accounts that point to this set of credentials. For now, this will just be a single AuthorizedAccount, but in the future other types of accounts for the same user could point to the same set of credentials """ try: - if account := getattr(self, "authorized_account", None): - return account - else: - return getattr(self, "temporary_authorized_account", None) + return [ + *filter( + bool, + [ + getattr(self, "authorized_account", None), + getattr(self, "temporary_authorized_account", None), + ], + ) + ] except ExternalCredentials.authorized_account.RelatedObjectDoesNotExist: return None @property def format(self): - if not self.authorized_account: + if not self.authorized_accounts: return None - return self.authorized_account.external_service.credentials_format + return self.authorized_accounts[0].external_service.credentials_format def clean_fields(self, *args, **kwargs): super().clean_fields(*args, **kwargs) self._validate_credentials() def _validate_credentials(self): - if not self.authorized_account: + if not self.authorized_accounts: return try: self.decrypted_credentials diff --git a/app/env.py b/app/env.py index 1efaf036..25d60e49 100644 --- a/app/env.py +++ b/app/env.py @@ -2,8 +2,10 @@ """ import os +import sys +TESTING = "test" in sys.argv DEBUG = bool(os.environ.get("DEBUG")) # any non-empty value enables debug mode SECRET_KEY = os.environ.get("SECRET_KEY") # used by django for cryptographic signing ALLOWED_HOSTS = list(filter(bool, os.environ.get("ALLOWED_HOSTS", "").split(","))) @@ -16,6 +18,7 @@ NEW_RELIC_ENVIRONMENT = os.environ.get("NEW_RELIC_ENVIRONMENT") SESSION_COOKIE_DOMAIN = os.environ.get("SESSION_COOKIE_DOMAIN") + ### # databases diff --git a/app/settings.py b/app/settings.py index eec43218..1a6c6f6e 100644 --- a/app/settings.py +++ b/app/settings.py @@ -107,10 +107,11 @@ ] -if DEBUG: +if DEBUG and not env.TESTING: # add django-silk to enable profiling INSTALLED_APPS.append("silk") MIDDLEWARE.insert(0, "silk.middleware.SilkyMiddleware") +if DEBUG: # run under ASGI locally: INSTALLED_APPS.insert(0, "daphne") # django's reference asgi server ASGI_APPLICATION = "app.asgi.application" From 6e97e8d8a103ea82b3d8b7f8a2615d42e978db6c Mon Sep 17 00:00:00 2001 From: Oleh Paduchak Date: Thu, 9 Jan 2025 14:32:10 +0200 Subject: [PATCH 6/7] fixed token refresh for oauth2 --- addon_service/common/network.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/addon_service/common/network.py b/addon_service/common/network.py index 0e8d5f21..1e923a60 100644 --- a/addon_service/common/network.py +++ b/addon_service/common/network.py @@ -83,9 +83,7 @@ async def _do_send(self, request: HttpRequestInfo): async with self._try_send(request) as _response: yield _response except exceptions.ExpiredAccessToken: - await _PrivateNetworkInfo.get( - self - ).account.refresh_oauth_access_token__async() + await _PrivateNetworkInfo.get(self).account.refresh_oauth2_access_token() # if this one fails, don't try refreshing again async with self._try_send(request) as _response: yield _response From 12fcef86f1b64d81df7d41e0c88632184088c3c0 Mon Sep 17 00:00:00 2001 From: Oleh Paduchak Date: Fri, 10 Jan 2025 17:58:44 +0200 Subject: [PATCH 7/7] reverted user_reference_uri change --- addon_service/authentication.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/addon_service/authentication.py b/addon_service/authentication.py index 8d1c482c..82d2cbb6 100644 --- a/addon_service/authentication.py +++ b/addon_service/authentication.py @@ -9,8 +9,6 @@ class GVCombinedAuthentication(drf_authentication.BaseAuthentication): """Authentication supporting session, basic, and token methods.""" def authenticate(self, request: DrfRequest): - if request.session.get("user_reference_uri"): - return True, None _user_uri = osf.get_osf_user_uri(request) if _user_uri: UserReference.objects.get_or_create(user_uri=_user_uri)