Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ENG-6820] Fix n+1 queries #201

Merged
merged 7 commits into from
Jan 10, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion addon_service/addon_operation_invocation/serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
)
Expand All @@ -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")
Expand Down
23 changes: 22 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,16 @@ 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()
)
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:
Expand All @@ -96,3 +105,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",
]
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
29 changes: 17 additions & 12 deletions addon_service/common/serializer_fields.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
7 changes: 6 additions & 1 deletion addon_service/configured_addon/storage/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
4 changes: 3 additions & 1 deletion addon_service/external_service/citation/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
4 changes: 3 additions & 1 deletion addon_service/external_service/computing/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,5 +5,7 @@


class ExternalComputingServiceViewSet(ReadOnlyModelViewSet):
queryset = ExternalComputingService.objects.all()
queryset = ExternalComputingService.objects.all().select_related(
"oauth2_client_config"
)
serializer_class = ExternalComputingServiceSerializer
4 changes: 3 additions & 1 deletion addon_service/external_service/storage/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,5 +5,7 @@


class ExternalStorageServiceViewSet(ReadOnlyModelViewSet):
queryset = ExternalStorageService.objects.all()
queryset = ExternalStorageService.objects.all().select_related(
"oauth2_client_config"
)
serializer_class = ExternalStorageServiceSerializer
26 changes: 22 additions & 4 deletions addon_service/resource_reference/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -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"
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)
12 changes: 9 additions & 3 deletions addon_service/user_reference/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
3 changes: 3 additions & 0 deletions app/env.py
Original file line number Diff line number Diff line change
Expand Up @@ -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(",")))
Expand All @@ -16,6 +18,7 @@
NEW_RELIC_ENVIRONMENT = os.environ.get("NEW_RELIC_ENVIRONMENT")
SESSION_COOKIE_DOMAIN = os.environ.get("SESSION_COOKIE_DOMAIN")


###
# databases

Expand Down
17 changes: 12 additions & 5 deletions app/settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -95,11 +95,6 @@
"addon_service",
]

if DEBUG:
# run under ASGI locally:
INSTALLED_APPS.insert(0, "daphne") # django's reference asgi server
ASGI_APPLICATION = "app.asgi.application"

MIDDLEWARE = [
"corsheaders.middleware.CorsMiddleware",
"django.middleware.security.SecurityMiddleware",
Expand All @@ -111,6 +106,17 @@
"django.middleware.clickjacking.XFrameOptionsMiddleware",
]


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"


ROOT_URLCONF = "app.urls"

TEMPLATES = [
Expand Down Expand Up @@ -168,6 +174,7 @@
},
}

SILKY_PYTHON_PROFILER = True
DATABASE_ROUTERS = ["addon_service.osf_models.db_router.OsfDatabaseRouter"]

REST_FRAMEWORK = {
Expand Down
4 changes: 4 additions & 0 deletions app/urls.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
from django.conf import settings
from django.contrib import admin
from django.urls import (
include,
Expand All @@ -15,3 +16,6 @@
name="docs-root",
),
]

if "silk" in settings.INSTALLED_APPS:
urlpatterns.append(path("silk/", include("silk.urls", namespace="silk")))
Loading
Loading