From ad50b594a81d7303323df9d2a1cfba02edb0bb67 Mon Sep 17 00:00:00 2001 From: Oleh Paduchak Date: Thu, 25 Jul 2024 14:37:44 +0300 Subject: [PATCH] fixed pr comments --- .../abstract/authorized_account/models.py | 30 -------------- .../authorized_account/serializers.py | 39 +++++++------------ addon_service/addon_imp/instantiation.py | 8 ++-- .../addon_operation_invocation/models.py | 3 +- .../authorized_storage_account/models.py | 34 +++++++++++++++- .../authorized_storage_account/serializers.py | 17 ++++++++ .../configured_storage_addon/models.py | 1 + addon_service/oauth2/models.py | 6 ++- addon_service/tasks/invocation.py | 2 +- addon_service/tests/_helpers.py | 4 +- 10 files changed, 81 insertions(+), 63 deletions(-) diff --git a/addon_service/abstract/authorized_account/models.py b/addon_service/abstract/authorized_account/models.py index 08203ea5..9e075fbc 100644 --- a/addon_service/abstract/authorized_account/models.py +++ b/addon_service/abstract/authorized_account/models.py @@ -54,36 +54,6 @@ class AuthorizedAccount(AddonsServiceBaseModel): ) _api_base_url = models.URLField(blank=True) - account_owner = models.ForeignKey( - "addon_service.UserReference", - on_delete=models.CASCADE, - related_name="authorized_storage_accounts", - ) - _credentials = models.OneToOneField( - "addon_service.ExternalCredentials", - on_delete=models.CASCADE, - primary_key=False, - null=True, - blank=True, - related_name="authorized_storage_account", - ) - _temporary_oauth1_credentials = models.OneToOneField( - "addon_service.ExternalCredentials", - on_delete=models.CASCADE, - primary_key=False, - null=True, - blank=True, - related_name="temporary_authorized_storage_account", - ) - oauth2_token_metadata = models.ForeignKey( - "addon_service.OAuth2TokenMetadata", - on_delete=models.CASCADE, # probs not - null=True, - blank=True, - related_name="authorized_storage_accounts", - related_query_name="%(class)s_authorized_storage_account", - ) - class Meta: abstract = True diff --git a/addon_service/abstract/authorized_account/serializers.py b/addon_service/abstract/authorized_account/serializers.py index 1c49e8ce..edf0aa24 100644 --- a/addon_service/abstract/authorized_account/serializers.py +++ b/addon_service/abstract/authorized_account/serializers.py @@ -1,42 +1,41 @@ +from __future__ import annotations + from abc import abstractmethod +from typing import TYPE_CHECKING from django.core.exceptions import ValidationError as ModelValidationError from rest_framework_json_api import serializers -from rest_framework_json_api.utils import get_resource_type_from_model -from addon_service.abstract.authorized_account.models import AuthorizedAccount -from addon_service.addon_operation.models import AddonOperationModel -from addon_service.common import view_names from addon_service.common.credentials_formats import CredentialsFormats -from addon_service.models import ( - AuthorizedStorageAccount, - ExternalStorageService, - UserReference, -) from addon_service.osf_models.fields import encrypt_string from addon_service.serializer_fields import ( CredentialsField, - DataclassRelatedLinkField, EnumNameMultipleChoiceField, - ReadOnlyResourceRelatedField, ) from addon_toolkit import AddonCapabilities -RESOURCE_TYPE = get_resource_type_from_model(AuthorizedStorageAccount) +if TYPE_CHECKING: + from addon_service.abstract.authorized_account.models import AuthorizedAccount + from addon_service.models import ExternalStorageService + +REQUIRED_FIELDS = frozenset(["url", "account_owner", "authorized_operations"]) class AuthorizedAccountSerializer(serializers.HyperlinkedModelSerializer): def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) + # Check whether subclasses declare all of required fields + if not REQUIRED_FIELDS.issubset(set(self.fields.keys())): + raise Exception( + f"{self.__class__.__name__} requires {self.REQUIRED_FIELDS} to be instantiated" + ) + # Check if it's a POST request and remove the field as it's not in our FE spec if "context" in kwargs and kwargs["context"]["request"].method == "POST": self.fields.pop("configured_storage_addons", None) - url = serializers.HyperlinkedIdentityField( - view_name=view_names.detail_view(RESOURCE_TYPE), required=False - ) authorized_capabilities = EnumNameMultipleChoiceField(enum_cls=AddonCapabilities) authorized_operation_names = serializers.ListField( child=serializers.CharField(), @@ -44,15 +43,7 @@ def __init__(self, *args, **kwargs): ) auth_url = serializers.CharField(read_only=True) api_base_url = serializers.URLField(allow_blank=True, required=False) - account_owner = ReadOnlyResourceRelatedField( - many=False, - queryset=UserReference.objects.all(), - related_link_view_name=view_names.related_view(RESOURCE_TYPE), - ) - authorized_operations = DataclassRelatedLinkField( - dataclass_model=AddonOperationModel, - related_link_view_name=view_names.related_view(RESOURCE_TYPE), - ) + credentials = CredentialsField(write_only=True, required=False) initiate_oauth = serializers.BooleanField(write_only=True, required=False) diff --git a/addon_service/addon_imp/instantiation.py b/addon_service/addon_imp/instantiation.py index 826abfbf..0f0bc3f5 100644 --- a/addon_service/addon_imp/instantiation.py +++ b/addon_service/addon_imp/instantiation.py @@ -1,3 +1,5 @@ +from __future__ import annotations + from typing import TYPE_CHECKING from asgiref.sync import async_to_sync @@ -15,9 +17,9 @@ async def get_storage_addon_instance( - imp_cls: "type[StorageAddonImp]", - account: "AuthorizedStorageAccount", - config: "StorageConfig", + imp_cls: type[StorageAddonImp], + account: AuthorizedStorageAccount, + config: StorageConfig, ) -> StorageAddonImp: """create an instance of a `StorageAddonImp` diff --git a/addon_service/addon_operation_invocation/models.py b/addon_service/addon_operation_invocation/models.py index 49180bd5..07ffd4fe 100644 --- a/addon_service/addon_operation_invocation/models.py +++ b/addon_service/addon_operation_invocation/models.py @@ -84,9 +84,10 @@ def clean_fields(self, *args, **kwargs): {"thru_addon": "thru_addon and thru_account must agree"} ) + @property def storage_imp_config(self) -> StorageConfig: if self.thru_addon: - return self.thru_addon.storage_imp_config() + return self.thru_addon.storage_imp_config return self.thru_account.storage_imp_config def set_exception(self, exception: BaseException) -> None: diff --git a/addon_service/authorized_storage_account/models.py b/addon_service/authorized_storage_account/models.py index 65d7a9c6..7ae72017 100644 --- a/addon_service/authorized_storage_account/models.py +++ b/addon_service/authorized_storage_account/models.py @@ -21,6 +21,36 @@ class AuthorizedStorageAccount(AuthorizedAccount): related_name="authorized_storage_accounts", ) + account_owner = models.ForeignKey( + "addon_service.UserReference", + on_delete=models.CASCADE, + related_name="authorized_storage_accounts", + ) + _credentials = models.OneToOneField( + "addon_service.ExternalCredentials", + on_delete=models.CASCADE, + primary_key=False, + null=True, + blank=True, + related_name="authorized_storage_account", + ) + _temporary_oauth1_credentials = models.OneToOneField( + "addon_service.ExternalCredentials", + on_delete=models.CASCADE, + primary_key=False, + null=True, + blank=True, + related_name="temporary_authorized_storage_account", + ) + oauth2_token_metadata = models.ForeignKey( + "addon_service.OAuth2TokenMetadata", + on_delete=models.CASCADE, # probs not + null=True, + blank=True, + related_name="authorized_storage_accounts", + related_query_name="%(class)s_authorized_storage_account", + ) + class Meta: verbose_name = "Authorized Storage Account" verbose_name_plural = "Authorized Storage Accounts" @@ -35,7 +65,9 @@ def external_service(self) -> ExternalStorageService: async def execute_post_auth_hook(self, auth_extras: dict | None = None): imp = await get_storage_addon_instance( - self.imp_cls, self, self.storage_imp_config + self.imp_cls, + self, + self.storage_imp_config, ) self.external_account_id = await imp.get_external_account_id(auth_extras or {}) await self.asave() diff --git a/addon_service/authorized_storage_account/serializers.py b/addon_service/authorized_storage_account/serializers.py index 3c31cf19..78a11d23 100644 --- a/addon_service/authorized_storage_account/serializers.py +++ b/addon_service/authorized_storage_account/serializers.py @@ -9,7 +9,12 @@ from addon_service.abstract.authorized_account.serializers import ( AuthorizedAccountSerializer, ) +from addon_service.addon_operation.models import AddonOperationModel from addon_service.common import view_names +from addon_service.common.serializer_fields import ( + DataclassRelatedLinkField, + ReadOnlyResourceRelatedField, +) from addon_service.models import ( AuthorizedStorageAccount, ConfiguredStorageAddon, @@ -33,6 +38,18 @@ class AuthorizedStorageAccountSerializer(AuthorizedAccountSerializer): related_link_view_name=view_names.related_view(RESOURCE_TYPE), required=False, ) + url = serializers.HyperlinkedIdentityField( + view_name=view_names.detail_view(RESOURCE_TYPE), required=False + ) + account_owner = ReadOnlyResourceRelatedField( + many=False, + queryset=UserReference.objects.all(), + related_link_view_name=view_names.related_view(RESOURCE_TYPE), + ) + authorized_operations = DataclassRelatedLinkField( + dataclass_model=AddonOperationModel, + related_link_view_name=view_names.related_view(RESOURCE_TYPE), + ) included_serializers = { "account_owner": "addon_service.serializers.UserReferenceSerializer", diff --git a/addon_service/configured_storage_addon/models.py b/addon_service/configured_storage_addon/models.py index fb545bee..c74428f1 100644 --- a/addon_service/configured_storage_addon/models.py +++ b/addon_service/configured_storage_addon/models.py @@ -117,6 +117,7 @@ def external_service(self): def imp_cls(self) -> type[AddonImp]: return self.base_account.imp_cls + @property def storage_imp_config(self) -> StorageConfig: return dataclasses.replace( self.base_account.storage_imp_config, diff --git a/addon_service/oauth2/models.py b/addon_service/oauth2/models.py index 6698873a..edf2e7f0 100644 --- a/addon_service/oauth2/models.py +++ b/addon_service/oauth2/models.py @@ -1,3 +1,5 @@ +from __future__ import annotations + from datetime import timedelta from typing import TYPE_CHECKING @@ -17,7 +19,7 @@ if TYPE_CHECKING: - from addon_service.abstract.authorized_account import AuthorizedAccount + from addon_service.abstract.authorized_account.models import AuthorizedAccount class OAuth2ClientConfig(AddonsServiceBaseModel): @@ -115,7 +117,7 @@ def validate_shared_client(self): @transaction.atomic def update_with_fresh_token( self, fresh_token_result: FreshTokenResult - ) -> "tuple[AuthorizedAccount]": + ) -> tuple[AuthorizedAccount]: # update this record's fields self.state_nonce = None # one-time-use, now used self.refresh_token = fresh_token_result.refresh_token diff --git a/addon_service/tasks/invocation.py b/addon_service/tasks/invocation.py index 5f2c5037..8991e305 100644 --- a/addon_service/tasks/invocation.py +++ b/addon_service/tasks/invocation.py @@ -22,7 +22,7 @@ def perform_invocation__blocking(invocation: AddonOperationInvocation) -> None: _imp = get_storage_addon_instance__blocking( invocation.imp_cls, # type: ignore[arg-type] #(TODO: generic impstantiation) invocation.thru_account, - invocation.storage_imp_config(), + invocation.storage_imp_config, ) _operation = invocation.operation # inner transaction to contain database errors, diff --git a/addon_service/tests/_helpers.py b/addon_service/tests/_helpers.py index cf157d85..a330169b 100644 --- a/addon_service/tests/_helpers.py +++ b/addon_service/tests/_helpers.py @@ -1,3 +1,5 @@ +from __future__ import annotations + import contextlib import dataclasses import secrets @@ -175,7 +177,7 @@ async def _route_post(self, url, *args, **kwargs): @dataclasses.dataclass class MockOAuth1ServiceProvider: - _external_service: "ExternalStorageService" + _external_service: ExternalStorageService _static_request_token: str _static_request_secret: str _static_verifier: str