Skip to content

Commit

Permalink
performance optimisations to addon operation invocations
Browse files Browse the repository at this point in the history
  • Loading branch information
opaduchak committed Jan 7, 2025
1 parent 755a288 commit 97a17c8
Show file tree
Hide file tree
Showing 5 changed files with 59 additions and 40 deletions.
22 changes: 21 additions & 1 deletion addon_service/addon_operation_invocation/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -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",
]
4 changes: 3 additions & 1 deletion addon_service/common/network.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions addon_service/common/osf.py
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down
21 changes: 8 additions & 13 deletions addon_service/credentials/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
51 changes: 26 additions & 25 deletions addon_service/tasks/invocation.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)

0 comments on commit 97a17c8

Please sign in to comment.