From 212fae37e3687744873c813fb2ea8435c9d39400 Mon Sep 17 00:00:00 2001 From: chiku Date: Wed, 6 Aug 2025 14:55:34 +0900 Subject: [PATCH 1/3] Fix typos in comments. --- addon_service/authorized_account/link/models.py | 2 +- addon_service/authorized_account/storage/models.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/addon_service/authorized_account/link/models.py b/addon_service/authorized_account/link/models.py index 6ac6880a..9fe0d869 100644 --- a/addon_service/authorized_account/link/models.py +++ b/addon_service/authorized_account/link/models.py @@ -4,7 +4,7 @@ class AuthorizedLinkAccount(AuthorizedAccount): - """Model for describing a user's account on an ExternalService. + """Model for describing a user's account on an ExternalLinkService. This model collects all of the information required to actually perform remote operations against the service and to aggregate accounts under a known user. diff --git a/addon_service/authorized_account/storage/models.py b/addon_service/authorized_account/storage/models.py index e13f211d..441e5465 100644 --- a/addon_service/authorized_account/storage/models.py +++ b/addon_service/authorized_account/storage/models.py @@ -6,7 +6,7 @@ class AuthorizedStorageAccount(AuthorizedAccount): - """Model for describing a user's account on an ExternalService. + """Model for describing a user's account on an ExternalStorageService. This model collects all of the information required to actually perform remote operations against the service and to aggregate accounts under a known user. From 8fd6d61fc93ed5f1f9fdfa3cf862a9d784d670ce Mon Sep 17 00:00:00 2001 From: chiku Date: Wed, 6 Aug 2025 14:56:55 +0900 Subject: [PATCH 2/3] Avoid `None` check of enum_*_fields by prohibiting `None` type. --- addon_service/admin/_base.py | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/addon_service/admin/_base.py b/addon_service/admin/_base.py index c1454aca..9d287573 100644 --- a/addon_service/admin/_base.py +++ b/addon_service/admin/_base.py @@ -46,11 +46,11 @@ def has_changed(self, initial, data): class GravyvaletModelAdmin(admin.ModelAdmin): - enum_choice_fields: dict[str, type[enum.Enum]] | None = None - enum_multiple_choice_fields: dict[str, type[enum.Enum]] | None = None + enum_choice_fields: dict[str, type[enum.Enum]] = {} + enum_multiple_choice_fields: dict[str, type[enum.Enum]] = {} def formfield_for_dbfield(self, db_field, request, **kwargs): - if self.enum_choice_fields and db_field.name in self.enum_choice_fields: + if db_field.name in self.enum_choice_fields: _enum = self.enum_choice_fields[db_field.name] return forms.ChoiceField( label=db_field.verbose_name, @@ -59,10 +59,7 @@ def formfield_for_dbfield(self, db_field, request, **kwargs): *self._list_enum_members(_enum), ], ) - if ( - self.enum_multiple_choice_fields - and db_field.name in self.enum_multiple_choice_fields - ): + if db_field.name in self.enum_multiple_choice_fields: _enum = self.enum_multiple_choice_fields[db_field.name] return EnumNameMultipleChoiceField( choices=self._list_enum_members(_enum), From 4976efe61591da47d2d230aaa106ba868d67e60a Mon Sep 17 00:00:00 2001 From: chiku Date: Wed, 6 Aug 2025 14:57:48 +0900 Subject: [PATCH 3/3] Introduce a dynamic addon imp registry to support Foreign Addon Imps A Foreign Addon Imp is an addon implementation that is put outside of gravyvalet codebase offered as a Django application. A collection of Foreign Addon Imps is distributed as a Django project. AddonRegistry class is the dynamic addon imp registry that holds the addon imps in use. Even the built-in addon implementation needs to be registered to be used, by adding the name and imp number pair to `app.settings.ADDON_IMPS` configuration. `AddonImpNumbers` enum is discarded. In addition, AddonImpRegistry class replaces the public interface of `addon_service.common.known_imps` module. Foreign Addon Imps needs to be registered to `ADDON_IMPS` too. Moreover, they needs to be registered to `INSTALLED_APPS`. --- FOREIGN_ADDON_IMP_DEVELOPMENT.md | 167 ++++++++ README.md | 50 +++ addon_service/addon_imp/models.py | 10 +- addon_service/admin/__init__.py | 22 +- addon_service/admin/_base.py | 47 +++ addon_service/apps.py | 4 + addon_service/common/known_imps.py | 197 +++++---- addon_service/common/validators.py | 4 +- .../configured_addon/computing/models.py | 4 +- addon_service/configured_addon/models.py | 4 +- addon_service/external_service/models.py | 29 +- addon_service/external_service/serializers.py | 4 +- .../management/commands/do_box_test.py | 6 +- .../management/commands/fill_garbage.py | 4 +- .../0017_change_icon_name_to_charfield.py | 26 ++ addon_service/tests/_factories.py | 10 +- addon_service/tests/test_addon_registry.py | 212 ++++++++++ .../test_foreign_addon_imp_integration.py | 394 ++++++++++++++++++ addon_service/tests/test_hmac_api_auth.py | 5 + .../tests/test_static_dataclasses_api.py | 154 ++++++- .../interfaces/foreign_addon_imp_config.py | 27 ++ app/settings.py | 27 ++ 22 files changed, 1275 insertions(+), 132 deletions(-) create mode 100644 FOREIGN_ADDON_IMP_DEVELOPMENT.md create mode 100644 addon_service/migrations/0017_change_icon_name_to_charfield.py create mode 100644 addon_service/tests/test_addon_registry.py create mode 100644 addon_service/tests/test_foreign_addon_imp_integration.py create mode 100644 addon_toolkit/interfaces/foreign_addon_imp_config.py diff --git a/FOREIGN_ADDON_IMP_DEVELOPMENT.md b/FOREIGN_ADDON_IMP_DEVELOPMENT.md new file mode 100644 index 00000000..e0cfa1b8 --- /dev/null +++ b/FOREIGN_ADDON_IMP_DEVELOPMENT.md @@ -0,0 +1,167 @@ +# Foreign Addon Imp Development Guide + +This guide explains how to develop Foreign Addon Imps for gravyvalet, allowing you to create custom integrations that can be distributed as standalone Python packages. + +## Overview + +Foreign Addon Imps are Django apps that extend gravyvalet's functionality by implementing new external service integrations. They can be developed independently and distributed as regular Python packages. + +## Requirements + +- Python 3.9+ +- Django 4.2+ +- gravyvalet + +## Development Steps + +### Create Your Package Structure + +Create a standard Python package structure: + +``` +your_addon_imp_package/ +├── setup.py +├── README.md +├── your_addon_imp_app/ +│ ├── __init__.py +│ ├── apps.py +│ ├── your_imp.py # your AddonImp implementation +│ └── static/ +│ └── {AppConfig.name}/ # typically, `your_addon_imp_package.your_addon_imp_app` +│ └── icons/ # Place your addon's icon files here +│ └── your_icon.svg +``` + +A foreign addon imp package can include multiple foreign addon imps. To do so, +just include multiple Django apps that behave as foreign addon imps. + +### Implement Your Django App Config + +Create `apps.py` with a class that inherits from `ForeignAddonImpConfig`: + +```python +from addon_toolkit.interfaces.foreign_addon_imp_config import ForeignAddonImpConfig +from .your_imp import YourServiceImp + +class YourAddonImpConfig(ForeignAddonImpConfig): + name = "your_addon_imp_package.your_addon_imp_app" + + @property + def imp(self): + """Return your AddonImp implementation class.""" + return YourServiceImp + + @property + def addon_imp_name(self): + """Return the unique name for your addon imp. + + IMPORTANT: This name MUST be unique across all addon imps in + gravyvalet installation. Users will reference this name in their + ADDON_IMPS configuration. + """ + return "YOUR_ADDON_IMP_APP_NAME" +``` + +### Implement Your AddonImp + +Create your AddonImp implementation based on the type of service: + +```python +from addon_toolkit.imp import AddonImp +from addon_toolkit.imp_subclasses.storage import StorageAddonImp + +class YourServiceImp(StorageAddonImp): + # Implement required methods and properties + # See addon_toolkit documentation for details + pass +``` + +The modules under the `addon_imps` package are good examples to +implement this part. + +### Choose a Unique Addon Imp Name and Document the name + +**CRITICAL**: Your `addon_imp_name` must be unique to avoid conflicts. + +Before choosing a name, check built-in addon imp names in +`addon_service.common.known_imps.KnownAddonImps` and avoid to use the +names enumerated. + +Document the name clearly so users know exactly what to use. Since users +can use the package name of the addon imp application instaed of +`addon_imp_name` value, document the package name too is a recommended +manner. + +### Adding Icons for Your Addon Imp + +Foreign addon imps can provide custom icons that will be available in +the gravyvalet admin interface. + +#### Icon Directory Convention + +Place your icon files in the `static/{AppConfig.name}/icons/` directory +within your addon imp app: + +``` +your_addon_imp_app/ +├── static/ +│ └── {AppConfig.name} # typically, your_addon_imp_package.your_addon_imp_app/ +│ └── icons/ +│ ├── your_service.svg +│ ├── your_service.png +│ └── your_service_alt.jpg +``` + +#### Supported Formats + +- SVG (recommended for scalability) +- PNG +- JPG/JPEG + +### Package and Distribute + +Create a `setup.py` for your package: + +```python +from setuptools import setup, find_packages + +setup( + name="your-addon-imp-package", + version="1.0.0", + packages=find_packages(), + include_package_data=True, # Important for including static files + install_requires=[ + "django>=4.2", + ], + package_data={ + 'your_addon_imp_app': [ + 'static/your_addon_imp_package.your_addon_imp_app/icons/*', # Include icon files + ], + }, +) +``` + +## Installation and Usage + +Users can install and use your foreign addon imps by: + +1. Installing your package: +```bash +pip install your-addon-imp-package +``` + +2. Adding it to Django's `INSTALLED_APPS`: +```python +INSTALLED_APPS = [ + # ... other apps + "your_addon_imp_package.your_addon_imp_app", +] +``` + +3. Registering it in gravyvalet's `ADDON_IMPS`: +```python +ADDON_IMPS= { + # ... other addons + "YOUR_ADDON_IMP_APP_NAME": 5000, # Use a unique (eg. ID >= 5000) +} +``` diff --git a/README.md b/README.md index c5995a56..7bced80c 100644 --- a/README.md +++ b/README.md @@ -56,6 +56,56 @@ To configure OAuth addons: 4. There fill your client id and client secret (instructions to obtain them are [here](./services_setup_doc/README.md)) 5. Now you should be able to connect these addons according to existing user flows (in ordinary osf app) +## ...use foreign addon imps + +Foreign addon imps allow you to extend gravyvalet with additional integrations +without modifying the core code. + +To use foreign addon imps: + +1. Install the foreign addon imp package(s): +```bash +pip install foreign-addon-imp-package-you-want +``` + +2. Add the foreign addon imp(s) to `INSTALLED_APPS` in your Django settings: +```python +INSTALLED_APPS = [ + # ... existing apps ... + 'foreign_addon_imp_package_you_want.app_name', + # ... + 'addon_service', + # ... +] +``` + +3. Register each foreign addon imp to `ADDON_IMPS` with a unique ID number: +```python +ADDON_IMPS = { + # ... other addons ... + "YOUR_ADDON_IMP_NAME": 5001, # Use a unique number not used by other addons +} +``` + +The name of each addon imp must be documented in the document of the foreign +addon imp package. If 2 addon imp applications you want to use adopted identical +names, use the package name instaed: + +```python +ADDON_IMPS = { + # ... other addons ... + 'foreign_addon_imp_package_you_want.app_name': 5001, +} +``` + +The ID numbers must be: +- Unique across all addon imps +- Never changed once assigned (changing would break existing configurations) + +4. Restart gravyvalet to load the new foreign addon imps + +After these steps, the foreign addon imps will be available. + ## ...configure a good environment see `app/env.py` for details on all environment variables used. diff --git a/addon_service/addon_imp/models.py b/addon_service/addon_imp/models.py index 2992052d..856e359b 100644 --- a/addon_service/addon_imp/models.py +++ b/addon_service/addon_imp/models.py @@ -3,7 +3,7 @@ from functools import cached_property from addon_service.addon_operation.models import AddonOperationModel -from addon_service.common import known_imps +from addon_service.common.known_imps import AddonImpRegistry from addon_service.common.static_dataclass_model import StaticDataclassModel from addon_toolkit import AddonImp @@ -19,12 +19,12 @@ class AddonImpModel(StaticDataclassModel): @classmethod def init_args_from_static_key(cls, static_key: str) -> tuple: - return (known_imps.get_imp_by_name(static_key),) + return (AddonImpRegistry.get_imp_by_name(static_key),) @classmethod def iter_all(cls) -> typing.Iterator[typing.Self]: - for _imp in known_imps.KnownAddonImps: - yield cls(_imp.value) + for _imp in AddonImpRegistry.get_all_addon_imps(): + yield cls(_imp) @property def static_key(self) -> str: @@ -35,7 +35,7 @@ def static_key(self) -> str: @cached_property def name(self) -> str: - return known_imps.get_imp_name(self.imp_cls) + return AddonImpRegistry.get_imp_name(self.imp_cls) @cached_property def imp_docstring(self) -> str: diff --git a/addon_service/admin/__init__.py b/addon_service/admin/__init__.py index 10389538..06231f09 100644 --- a/addon_service/admin/__init__.py +++ b/addon_service/admin/__init__.py @@ -1,11 +1,15 @@ from django.contrib import admin from addon_service import models -from addon_service.common import known_imps from addon_service.common.credentials_formats import CredentialsFormats +from addon_service.common.known_imps import AddonImpRegistry from addon_service.common.service_types import ServiceTypes from addon_service.external_service.computing.models import ComputingSupportedFeatures from addon_service.external_service.storage.models import StorageSupportedFeatures +from addon_toolkit.interfaces.citation import CitationAddonImp +from addon_toolkit.interfaces.computing import ComputingAddonImp +from addon_toolkit.interfaces.link import LinkAddonImp +from addon_toolkit.interfaces.storage import StorageAddonImp from ..external_service.citation.models import CitationSupportedFeatures from ..external_service.link.models import ( @@ -26,13 +30,15 @@ class ExternalStorageServiceAdmin(GravyvaletModelAdmin): ) raw_id_fields = ("oauth2_client_config", "oauth1_client_config") enum_choice_fields = { - "int_addon_imp": known_imps.StorageAddonImpNumbers, "int_credentials_format": CredentialsFormats, "int_service_type": ServiceTypes, } enum_multiple_choice_fields = { "int_supported_features": StorageSupportedFeatures, } + dynamic_choice_fields = { + "int_addon_imp": lambda: AddonImpRegistry.iter_by_type(StorageAddonImp), + } @admin.register(models.ExternalCitationService) @@ -45,13 +51,15 @@ class ExternalCitationServiceAdmin(GravyvaletModelAdmin): ) raw_id_fields = ("oauth2_client_config", "oauth1_client_config") enum_choice_fields = { - "int_addon_imp": known_imps.CitationAddonImpNumbers, "int_credentials_format": CredentialsFormats, "int_service_type": ServiceTypes, } enum_multiple_choice_fields = { "int_supported_features": CitationSupportedFeatures, } + dynamic_choice_fields = { + "int_addon_imp": lambda: AddonImpRegistry.iter_by_type(CitationAddonImp), + } @admin.register(models.ExternalLinkService) @@ -64,7 +72,6 @@ class ExternalLinkServiceAdmin(GravyvaletModelAdmin): ) raw_id_fields = ("oauth2_client_config", "oauth1_client_config") enum_choice_fields = { - "int_addon_imp": known_imps.LinkAddonImpNumbers, "int_credentials_format": CredentialsFormats, "int_service_type": ServiceTypes, } @@ -72,6 +79,9 @@ class ExternalLinkServiceAdmin(GravyvaletModelAdmin): "int_supported_features": LinkSupportedFeatures, "int_supported_resource_types": SupportedResourceTypes, } + dynamic_choice_fields = { + "int_addon_imp": lambda: AddonImpRegistry.iter_by_type(LinkAddonImp), + } @admin.register(models.ExternalComputingService) @@ -84,13 +94,15 @@ class ExternalComputingServiceAdmin(GravyvaletModelAdmin): ) raw_id_fields = ("oauth2_client_config",) enum_choice_fields = { - "int_addon_imp": known_imps.ComputingAddonImpNumbers, "int_credentials_format": CredentialsFormats, "int_service_type": ServiceTypes, } enum_multiple_choice_fields = { "int_supported_features": ComputingSupportedFeatures, } + dynamic_choice_fields = { + "int_addon_imp": lambda: AddonImpRegistry.iter_by_type(ComputingAddonImp), + } @admin.register(models.OAuth2ClientConfig) diff --git a/addon_service/admin/_base.py b/addon_service/admin/_base.py index 9d287573..01d40a8b 100644 --- a/addon_service/admin/_base.py +++ b/addon_service/admin/_base.py @@ -1,10 +1,14 @@ import enum +from collections.abc import Callable +from pathlib import Path from django import forms +from django.apps import apps from django.contrib import admin from django.core.exceptions import ValidationError from addon_service.common.enum_utils import combine_flags +from addon_toolkit.interfaces.foreign_addon_imp_config import ForeignAddonImpConfig __all__ = ("GravyvaletModelAdmin",) @@ -48,8 +52,43 @@ def has_changed(self, initial, data): class GravyvaletModelAdmin(admin.ModelAdmin): enum_choice_fields: dict[str, type[enum.Enum]] = {} enum_multiple_choice_fields: dict[str, type[enum.Enum]] = {} + dynamic_choice_fields: dict[str, Callable[[], set[tuple[str, int]]]] = {} + + def get_icon_choices(self): + """Get icon choices from multiple directories.""" + from django.conf import settings + + icon_files = {} # absolute path -> display name + extensions = ["jpg", "jpeg", "png", "svg"] + + static_icon_dir = settings.PROVIDER_ICONS_DIR + if static_icon_dir.exists(): + for ext in extensions: + for icon_file in static_icon_dir.glob(f"*.{ext}"): + icon_files[str(icon_file.resolve())] = icon_file.name + + # Get icons from foreign addon imps + for app_config in apps.get_app_configs(): + if isinstance(app_config, ForeignAddonImpConfig): + icon_dir = Path(app_config.path) / "static" / app_config.name / "icons" + if icon_dir.exists(): + for ext in extensions: + for icon_file in icon_dir.glob(f"*.{ext}"): + icon_files[str(icon_file.resolve())] = ( + f"{app_config.verbose_name} - {icon_file.name}" + ) + + return [("", "---------")] + list(icon_files.items()) def formfield_for_dbfield(self, db_field, request, **kwargs): + # Handle icon_name field specially + if db_field.name == "icon_name": + return forms.ChoiceField( + choices=self.get_icon_choices(), + required=False, + help_text="Select an icon from available icon directories", + ) + if db_field.name in self.enum_choice_fields: _enum = self.enum_choice_fields[db_field.name] return forms.ChoiceField( @@ -66,6 +105,14 @@ def formfield_for_dbfield(self, db_field, request, **kwargs): widget=forms.CheckboxSelectMultiple, enum_cls=_enum, ) + if db_field.name in self.dynamic_choice_fields: + return forms.ChoiceField( + label=db_field.verbose_name, + choices=[ + (None, ""), + *self.dynamic_choice_fields[db_field.name](), + ], + ) return super().formfield_for_dbfield(db_field, request, **kwargs) diff --git a/addon_service/apps.py b/addon_service/apps.py index c2916203..d3e132b6 100644 --- a/addon_service/apps.py +++ b/addon_service/apps.py @@ -1,4 +1,5 @@ from django.apps import AppConfig +from django.conf import settings class AddonServiceConfig(AppConfig): @@ -7,3 +8,6 @@ class AddonServiceConfig(AppConfig): def ready(self): # need to import openapi extensions here for them to be registered import addon_service.common.openapi_extensions # noqa: F401 + from addon_service.common.known_imps import AddonImpRegistry + + AddonImpRegistry.register_addon_imps(settings.ADDON_IMPS) diff --git a/addon_service/common/known_imps.py b/addon_service/common/known_imps.py index edb3b7b0..568482d3 100644 --- a/addon_service/common/known_imps.py +++ b/addon_service/common/known_imps.py @@ -1,9 +1,18 @@ -"""the single static source of truth for addon implementations known to the addon service +"""Addon Implementation Registry for the addon service -import and add new implementations here to make them available in the api +supports both static built-in addons and dynamically registered addons. +import and add new static implementations to KnownAddonImps in order to +make them available. """ import enum +import logging +from collections.abc import ( + Iterable, + Iterator, +) + +from django.apps import apps from addon_imps.citations import ( mendeley, @@ -25,57 +34,123 @@ owncloud, s3, ) -from addon_service.common.enum_decorators import enum_names_same_as from addon_toolkit import AddonImp -from addon_toolkit.interfaces.citation import CitationAddonImp -from addon_toolkit.interfaces.computing import ComputingAddonImp -from addon_toolkit.interfaces.link import LinkAddonImp -from addon_toolkit.interfaces.storage import StorageAddonImp +from addon_toolkit.interfaces.foreign_addon_imp_config import ForeignAddonImpConfig + +logger = logging.getLogger(__name__) if __debug__: from addon_imps.storage import my_blarg -__all__ = ( - "AddonImpNumbers", - "KnownAddonImps", - "get_imp_by_name", - "get_imp_by_number", - "get_imp_name", -) - - -### -# Public interface for accessing concrete AddonImps via their API-facing name or integer ID (and vice-versa) - - -def get_imp_by_name(imp_name: str) -> type[AddonImp]: - return KnownAddonImps[imp_name].value +__all__ = ("AddonImpRegistry",) -def get_imp_name(imp: type[AddonImp]) -> str: - return KnownAddonImps(imp).name - - -def get_imp_by_number(imp_number: int) -> type[AddonImp]: - _imp_name = AddonImpNumbers(imp_number).name - return get_imp_by_name(_imp_name) +class AddonImpRegistry: + """ + Registry for addon imps in use. + """ + _name_imp_map: dict[str, type[AddonImp]] = {} + _number_name_map: dict[int, str] = {} -def get_imp_number(imp: type[AddonImp]) -> int: - _imp_name = get_imp_name(imp) - return AddonImpNumbers[_imp_name].value + @classmethod + def register_addon_imps(cls, addon_imps: dict[str, int]) -> None: + foreign_addon_imps = { + ks: cfg.imp + for cfg in apps.get_app_configs() + if isinstance(cfg, ForeignAddonImpConfig) + for ks in (cfg.addon_imp_name, cfg.name) + } + for name, number in addon_imps.items(): + if name in KnownAddonImps.__members__: + cls.register(name, number, KnownAddonImps[name].value) + elif name in foreign_addon_imps: + cls.register(name, number, foreign_addon_imps[name]) + else: + logger.warning( + f"No addon imp has name {name}. " + "Forgot to add an addon imp to INSTALLED_APPS?" + ) + + @classmethod + def register(cls, addon_imp_name: str, imp_number: int, imp: AddonImp) -> None: + if imp_number in cls._number_name_map: + if ( + addon_imp_name in cls._name_imp_map + and addon_imp_name == cls._number_name_map.get(imp_number) + ): + logger.info( + f"Addon Imp {addon_imp_name} has already been registered correctly." + ) + return + else: + logger.error( + f"imp number {imp_number} is specified for 2 addon imps -- " + f"{addon_imp_name} and {cls._number_name_map[imp_number]}" + ) + raise ValueError("imp number conflict") + + cls._name_imp_map[addon_imp_name] = imp + cls._number_name_map[imp_number] = addon_imp_name + + @classmethod + def get_all_addon_imps(cls) -> Iterable[type[AddonImp]]: + return cls._name_imp_map.values() + + @classmethod + def iter_by_type(cls, addon_imp_type: type[AddonImp]) -> Iterator[tuple[int, str]]: + return ( + (number, name) + for number, name in cls._number_name_map.items() + if issubclass(cls._name_imp_map[name], addon_imp_type) + ) + + @classmethod + def get_imp_by_name(cls, imp_name: str) -> type[AddonImp]: + return cls._name_imp_map[imp_name] + + @classmethod + def get_imp_name(cls, imp: type[AddonImp]) -> str: + for name, registered_imp in cls._name_imp_map.items(): + if registered_imp == imp: + return name + raise ValueError(f"Unknown addon imp: {imp}") + + @classmethod + def get_imp_by_number(cls, imp_number: int) -> type[AddonImp]: + return cls.get_imp_by_name(cls.get_name_by_number(imp_number)) + + @classmethod + def get_imp_number(cls, imp: type[AddonImp]) -> int: + imp_name = cls.get_imp_name(imp) + for number, name in cls._number_name_map.items(): + if name == imp_name: + return number + raise ValueError(f"Unknown addon imp : {imp}") + + @classmethod + def get_name_by_number(cls, imp_number: int) -> str: + return cls._number_name_map[imp_number] + + @classmethod + def clear(cls) -> None: + """Clear all registrations.""" + cls._name_imp_map.clear() + cls._number_name_map.clear() ### -# Static registry of known addon implementations -- add new imps to the enums below +# Static registry of known addon implementations -- add new static addon +# imps to KnownAddonImps below. @enum.unique class KnownAddonImps(enum.Enum): """Static mapping from API-facing name for an AddonImp to the Imp itself. - Note: Grouped by type and then ordered by respective AddonImpNumbers. + Note: Grouped by type and then ordered by respective imp numbers assigned + in apps.settings. """ # Type: Storage @@ -104,57 +179,3 @@ class KnownAddonImps(enum.Enum): if __debug__: BLARG = my_blarg.MyBlargStorage - - -@enum_names_same_as(KnownAddonImps) -@enum.unique -class AddonImpNumbers(enum.Enum): - """Static mapping from each AddonImp name to a unique integer (for database use) - - Note: Ideally, we should "prefix" the number by type and take future scalability into account. - e.g. Storage type uses 10xx, Citation type uses 11xx, Cloud Computing type uses 12xx and LINK type uses 13xx. - Consider this a future improvement when we run out of 1001~1019 for both storage and citation addons. - """ - - # Type: Storage - BOX = 1001 - S3 = 1003 - GOOGLEDRIVE = 1005 - DROPBOX = 1006 - FIGSHARE = 1007 - ONEDRIVE = 1008 - OWNCLOUD = 1009 - DATAVERSE = 1010 - GITLAB = 1011 - BITBUCKET = 1012 - GITHUB = 1013 - AZUREBLOBSTORAGE = 1014 - - # Type: Citation - ZOTERO = 1002 - MENDELEY = 1004 - - # Type: Cloud Computing - BOA = 1020 - - # Type: Link - LINK_DATAVERSE = 1030 - - if __debug__: - BLARG = -7 - - -def filter_addons_by_type(addon_type): - return frozenset( - { - AddonImpNumbers[item.name] - for item in KnownAddonImps - if issubclass(item.value, addon_type) - } - ) - - -StorageAddonImpNumbers = filter_addons_by_type(StorageAddonImp) -CitationAddonImpNumbers = filter_addons_by_type(CitationAddonImp) -ComputingAddonImpNumbers = filter_addons_by_type(ComputingAddonImp) -LinkAddonImpNumbers = filter_addons_by_type(LinkAddonImp) diff --git a/addon_service/common/validators.py b/addon_service/common/validators.py index be760a95..0f358140 100644 --- a/addon_service/common/validators.py +++ b/addon_service/common/validators.py @@ -13,9 +13,9 @@ from addon_toolkit.interfaces.link import LinkAddonImp from addon_toolkit.interfaces.storage import StorageAddonImp -from . import known_imps from .credentials_formats import CredentialsFormats from .invocation_status import InvocationStatus +from .known_imps import AddonImpRegistry from .service_types import ServiceTypes @@ -48,7 +48,7 @@ def validate_credentials_format(value): def _validate_imp_number(value, cls): """validator for `AddonImpNumbers` integer values""" try: - _imp_cls = known_imps.get_imp_by_number(value) + _imp_cls = AddonImpRegistry.get_imp_by_number(value) except KeyError: raise ValidationError(f"invalid imp number: {value}") diff --git a/addon_service/configured_addon/computing/models.py b/addon_service/configured_addon/computing/models.py index 88e2d848..5ea321cd 100644 --- a/addon_service/configured_addon/computing/models.py +++ b/addon_service/configured_addon/computing/models.py @@ -1,4 +1,4 @@ -from addon_service.common.known_imps import AddonImpNumbers +from addon_service.common.known_imps import AddonImpRegistry from addon_service.configured_addon.models import ConfiguredAddon from addon_toolkit.interfaces.computing import ComputingConfig @@ -20,4 +20,4 @@ def config(self) -> ComputingConfig: @property def external_service_name(self) -> str: number = self.base_account.external_service.int_addon_imp - return AddonImpNumbers(number).name.lower() + return AddonImpRegistry.get_name_by_number(number).lower() diff --git a/addon_service/configured_addon/models.py b/addon_service/configured_addon/models.py index 9b6e759c..75a7e446 100644 --- a/addon_service/configured_addon/models.py +++ b/addon_service/configured_addon/models.py @@ -5,7 +5,7 @@ from addon_service.addon_operation.models import AddonOperationModel from addon_service.common.base_model import AddonsServiceBaseModel -from addon_service.common.known_imps import AddonImpNumbers +from addon_service.common.known_imps import AddonImpRegistry from addon_service.common.validators import validate_addon_capability from addon_toolkit import ( AddonCapabilities, @@ -140,7 +140,7 @@ def imp_cls(self) -> type[AddonImp]: @property def external_service_name(self) -> str: number = self.base_account.external_service.int_addon_imp - return AddonImpNumbers(number).name.lower() + return AddonImpRegistry.get_name_by_number(number).lower() def clean_fields(self, *args, **kwargs): super().clean_fields(*args, **kwargs) diff --git a/addon_service/external_service/models.py b/addon_service/external_service/models.py index e52e1cea..e68ac9c6 100644 --- a/addon_service/external_service/models.py +++ b/addon_service/external_service/models.py @@ -1,11 +1,13 @@ +from pathlib import Path + from django.contrib.postgres.fields import ArrayField from django.core.exceptions import ValidationError from django.db import models from addon_service.addon_imp.models import AddonImpModel -from addon_service.common import known_imps from addon_service.common.base_model import AddonsServiceBaseModel from addon_service.common.credentials_formats import CredentialsFormats +from addon_service.common.known_imps import AddonImpRegistry from addon_service.common.service_types import ServiceTypes from addon_service.common.validators import ( validate_credentials_format, @@ -27,12 +29,11 @@ class ExternalService(AddonsServiceBaseModel): validators=[validate_service_type], verbose_name="Service type", ) - icon_name = models.FilePathField( - path=settings.PROVIDER_ICONS_DIR, - recursive=False, - match=r".*\.(jpg|png|svg)$", + icon_name = models.CharField( + max_length=255, blank=True, null=True, + help_text="Relative path to icon file within configured icon directories", ) int_addon_imp = models.IntegerField( null=False, @@ -65,11 +66,11 @@ def __repr__(self): @property def addon_imp(self) -> AddonImpModel: - return AddonImpModel(known_imps.get_imp_by_number(self.int_addon_imp)) + return AddonImpModel(AddonImpRegistry.get_imp_by_number(self.int_addon_imp)) @addon_imp.setter def addon_imp(self, value: AddonImpModel): - self.int_addon_imp = known_imps.get_imp_number(value.imp_cls) + self.int_addon_imp = AddonImpRegistry.get_imp_number(value.imp_cls) @property def auth_uri(self): @@ -92,7 +93,19 @@ def configurable_api_root(self) -> bool: @property def external_service_name(self) -> str: number = self.int_addon_imp - return known_imps.AddonImpNumbers(number).name.lower() + return AddonImpRegistry.get_name_by_number(number).lower() + + @property + def icon_url(self) -> str | None: + if self.icon_name: + icon_path = Path(self.icon_name) + if self.icon_name.startswith(str(settings.PROVIDER_ICONS_DIR)): + return f"/static/provider_icons/{icon_path.name}" + else: + # the icon offered by a Foreign Addon Imp. + return f"/{Path(*icon_path.parts[-4:])}" + + return None def clean_fields(self, *args, **kwargs): super().clean_fields(*args, **kwargs) diff --git a/addon_service/external_service/serializers.py b/addon_service/external_service/serializers.py index f824774f..fefad5f0 100644 --- a/addon_service/external_service/serializers.py +++ b/addon_service/external_service/serializers.py @@ -27,8 +27,8 @@ def __init__(self, *args, **kwargs): def get_icon_url(self, obj: ExternalService): request = self.context.get("request") - if request and obj.icon_name: - return f"{request.build_absolute_uri('/')}static/provider_icons/{obj.icon_name.split('/')[-1]}" + if request and obj.icon_url: + return request.build_absolute_uri(obj.icon_url) return None external_service_name = serializers.CharField(read_only=True) diff --git a/addon_service/management/commands/do_box_test.py b/addon_service/management/commands/do_box_test.py index 40dd4f9c..67a3b2c0 100644 --- a/addon_service/management/commands/do_box_test.py +++ b/addon_service/management/commands/do_box_test.py @@ -9,12 +9,12 @@ from django.core.management.base import BaseCommand from addon_service import models as db -from addon_service.common import known_imps from addon_service.common.aiohttp_session import ( close_singleton_client_session__blocking, ) from addon_service.common.credentials_formats import CredentialsFormats from addon_service.common.invocation_status import InvocationStatus +from addon_service.common.known_imps import AddonImpRegistry from addon_service.common.service_types import ServiceTypes from addon_service.tasks.invocation import perform_invocation__blocking from addon_toolkit import AddonCapabilities @@ -75,8 +75,8 @@ def _setup_oauth(self, user_uri: str, client_id, client_secret): client_secret=client_secret, ) _box_service, _ = db.ExternalStorageService.objects.update_or_create( - int_addon_imp=known_imps.get_imp_number( - known_imps.get_imp_by_name("BOX_DOT_COM") + int_addon_imp=AddonImpRegistry.get_imp_number( + AddonImpRegistry.get_imp_by_name("BOX_DOT_COM") ), defaults=dict( name="my-box-dot-com", diff --git a/addon_service/management/commands/fill_garbage.py b/addon_service/management/commands/fill_garbage.py index 30a53630..586c0fec 100644 --- a/addon_service/management/commands/fill_garbage.py +++ b/addon_service/management/commands/fill_garbage.py @@ -2,8 +2,8 @@ from django.core.management.base import LabelCommand from addon_service import models as db -from addon_service.common import known_imps from addon_service.common.credentials_formats import CredentialsFormats +from addon_service.common.known_imps import AddonImpRegistry from addon_toolkit import AddonCapabilities @@ -16,7 +16,7 @@ class Command(LabelCommand): def handle_label(self, label, **options): if not settings.DEBUG: raise Exception("must have DEBUG set to eat garbage") - _blarg_imp = known_imps.get_imp_by_name("BLARG") + _blarg_imp = AddonImpRegistry.get_imp_by_name("BLARG") _ess = db.ExternalStorageService.objects.create( service_name=label, addon_imp=db.AddonImpModel(_blarg_imp), diff --git a/addon_service/migrations/0017_change_icon_name_to_charfield.py b/addon_service/migrations/0017_change_icon_name_to_charfield.py new file mode 100644 index 00000000..27093c56 --- /dev/null +++ b/addon_service/migrations/0017_change_icon_name_to_charfield.py @@ -0,0 +1,26 @@ +# Generated by Django 4.2.20 on 2025-08-05 07:42 + +from django.db import ( + migrations, + models, +) + + +class Migration(migrations.Migration): + + dependencies = [ + ("addon_service", "0016_externallinkservice_int_supported_features_and_more"), + ] + + operations = [ + migrations.AlterField( + model_name="externalservice", + name="icon_name", + field=models.CharField( + blank=True, + help_text="Relative path to icon file within configured icon directories", + max_length=255, + null=True, + ), + ), + ] diff --git a/addon_service/tests/_factories.py b/addon_service/tests/_factories.py index 3aeea18f..5b2143d0 100644 --- a/addon_service/tests/_factories.py +++ b/addon_service/tests/_factories.py @@ -3,8 +3,8 @@ from factory.django import DjangoModelFactory from addon_service import models as db -from addon_service.common import known_imps from addon_service.common.credentials_formats import CredentialsFormats +from addon_service.common.known_imps import AddonImpRegistry from addon_service.common.service_types import ServiceTypes from addon_service.external_service.storage.models import StorageSupportedFeatures from addon_toolkit import AddonCapabilities @@ -76,7 +76,9 @@ class Meta: display_name = factory.Faker("word") max_concurrent_downloads = factory.Faker("pyint") max_upload_mb = factory.Faker("pyint") - int_addon_imp = known_imps.get_imp_number(known_imps.get_imp_by_name("BLARG")) + int_addon_imp = AddonImpRegistry.get_imp_number( + AddonImpRegistry.get_imp_by_name("BLARG") + ) supported_scopes = ["service.url/grant_all"] @classmethod @@ -198,8 +200,8 @@ class Meta: model = db.ExternalLinkService display_name = factory.Faker("word") - int_addon_imp = known_imps.get_imp_number( - known_imps.get_imp_by_name("LINK_DATAVERSE") + int_addon_imp = AddonImpRegistry.get_imp_number( + AddonImpRegistry.get_imp_by_name("LINK_DATAVERSE") ) supported_scopes = ["service.url/grant_all"] diff --git a/addon_service/tests/test_addon_registry.py b/addon_service/tests/test_addon_registry.py new file mode 100644 index 00000000..81713f9b --- /dev/null +++ b/addon_service/tests/test_addon_registry.py @@ -0,0 +1,212 @@ +"""Tests for the AddonImpRegistry class.""" + +from unittest.mock import patch + +from django.test import TestCase + +from addon_service.common.known_imps import ( + AddonImpRegistry, + KnownAddonImps, +) +from addon_toolkit import AddonImp +from addon_toolkit.interfaces.citation import CitationAddonImp +from addon_toolkit.interfaces.storage import StorageAddonImp + + +class MockStorageImp(StorageAddonImp): + """Mock storage addon imp for testing.""" + + +class MockCitationImp(CitationAddonImp): + """Mock citation addon imp for testing.""" + + +class TestAddonImpRegistry(TestCase): + """Test cases for AddonImpRegistry class.""" + + def setUp(self): + """Save and Clear registry before each test.""" + self._original_name_imp_map = AddonImpRegistry._name_imp_map.copy() + self._original_number_name_map = AddonImpRegistry._number_name_map.copy() + AddonImpRegistry.clear() + + def tearDown(self): + """Restore original registry state after each test.""" + AddonImpRegistry.clear() + AddonImpRegistry._name_imp_map.update(self._original_name_imp_map) + AddonImpRegistry._number_name_map.update(self._original_number_name_map) + + # Test 1: Registration Mechanics + def test_register_addon_imp_success(self): + """Test successful registration of an addon imp.""" + AddonImpRegistry.register("TEST_ADDON_IMP", 9999, MockStorageImp) + + self.assertEqual( + AddonImpRegistry.get_imp_by_name("TEST_ADDON_IMP"), MockStorageImp + ) + self.assertEqual(AddonImpRegistry.get_name_by_number(9999), "TEST_ADDON_IMP") + self.assertEqual(AddonImpRegistry.get_imp_by_number(9999), MockStorageImp) + + def test_register_duplicate_number_conflict(self): + """Test that duplicate imp numbers raise an error.""" + AddonImpRegistry.register("ADDON_IMP1", 1000, MockStorageImp) + + with self.assertRaises(ValueError) as context: + AddonImpRegistry.register("ADDON_IMP2", 1000, MockCitationImp) + self.assertIn("imp number conflict", str(context.exception)) + + def test_register_same_addon_imp_multiple_times(self): + """Test that registering the same addon imp multiple times is allowed.""" + AddonImpRegistry.register("TEST_ADDON_IMP", 9999, MockStorageImp) + # Should not raise an error + AddonImpRegistry.register("TEST_ADDON_IMP", 9999, MockStorageImp) + + self.assertEqual( + AddonImpRegistry.get_imp_by_name("TEST_ADDON_IMP"), MockStorageImp + ) + + def test_register_different_addon_imp_same_number_error(self): + """Test that different addon imps with the same number raise error.""" + AddonImpRegistry.register("ADDON_IMP1", 1000, MockStorageImp) + + with self.assertRaises(ValueError): + AddonImpRegistry.register("ADDON_IMP2", 1000, MockStorageImp) + + # Test 2: Retrieval Methods + def test_get_imp_by_name_valid(self): + """Test retrieving addon imp by a valid name.""" + AddonImpRegistry.register("TEST_ADDON_IMP", 9999, MockStorageImp) + result = AddonImpRegistry.get_imp_by_name("TEST_ADDON_IMP") + self.assertEqual(result, MockStorageImp) + + def test_get_imp_by_name_invalid(self): + """Test retrieving addon imp by an invalid name raises KeyError.""" + with self.assertRaises(KeyError): + AddonImpRegistry.get_imp_by_name("NONEXISTENT") + + def test_get_imp_by_number_valid(self): + """Test retrieving addon imp by a valid number.""" + AddonImpRegistry.register("TEST_ADDON_IMP", 9999, MockStorageImp) + result = AddonImpRegistry.get_imp_by_number(9999) + self.assertEqual(result, MockStorageImp) + + def test_get_imp_by_number_invalid(self): + """Test retrieving addon imp by an invalid number raises KeyError.""" + with self.assertRaises(KeyError): + AddonImpRegistry.get_imp_by_number(99999) + + def test_get_imp_name(self): + """Test getting name of registered imp.""" + AddonImpRegistry.register("TEST_ADDON_IMP", 9999, MockStorageImp) + name = AddonImpRegistry.get_imp_name(MockStorageImp) + self.assertEqual(name, "TEST_ADDON_IMP") + + def test_get_imp_name_unregistered(self): + """Test getting the name of an unregistered imp raises ValueError.""" + with self.assertRaises(ValueError) as context: + AddonImpRegistry.get_imp_name(MockCitationImp) + self.assertIn("Unknown addon imp", str(context.exception)) + + def test_get_imp_number(self): + """Test getting the number of a registered imp.""" + AddonImpRegistry.register("TEST_ADDON_IMP", 9999, MockStorageImp) + number = AddonImpRegistry.get_imp_number(MockStorageImp) + self.assertEqual(number, 9999) + + def test_get_imp_number_unregistered(self): + """Test getting the number of an unregistered imp raises ValueError.""" + with self.assertRaises(ValueError) as context: + AddonImpRegistry.get_imp_number(MockCitationImp) + self.assertIn("Unknown addon imp", str(context.exception)) + + def test_get_all_addon_imps(self): + """Test getting all registered addon imps.""" + AddonImpRegistry.register("ADDON_IMP1", 1001, MockStorageImp) + AddonImpRegistry.register("ADDON_IMP2", 1002, MockCitationImp) + + all_imps = list(AddonImpRegistry.get_all_addon_imps()) + self.assertEqual(len(all_imps), 2) + self.assertIn(MockStorageImp, all_imps) + self.assertIn(MockCitationImp, all_imps) + + def test_iter_by_type_storage(self): + """Test iterating addon imps by storage type.""" + AddonImpRegistry.register("STORAGE1", 1001, MockStorageImp) + AddonImpRegistry.register("CITATION1", 1002, MockCitationImp) + + storage_addon_imps = list(AddonImpRegistry.iter_by_type(StorageAddonImp)) + self.assertEqual(len(storage_addon_imps), 1) + self.assertEqual(storage_addon_imps[0], (1001, "STORAGE1")) + + def test_iter_by_type_citation(self): + """Test iterating addon imps by citation type.""" + AddonImpRegistry.register("STORAGE1", 1001, MockStorageImp) + AddonImpRegistry.register("CITATION1", 1002, MockCitationImp) + + citation_addon_imps = list(AddonImpRegistry.iter_by_type(CitationAddonImp)) + self.assertEqual(len(citation_addon_imps), 1) + self.assertEqual(citation_addon_imps[0], (1002, "CITATION1")) + + def test_iter_by_type_no_filter(self): + """Test iterating all addon imps.""" + AddonImpRegistry.register("STORAGE1", 1001, MockStorageImp) + AddonImpRegistry.register("CITATION1", 1002, MockCitationImp) + + addon_imps = list(AddonImpRegistry.iter_by_type(AddonImp)) + self.assertEqual(len(addon_imps), 2) + self.assertEqual(addon_imps[0], (1001, "STORAGE1")) + self.assertEqual(addon_imps[1], (1002, "CITATION1")) + + # Test 3: Edge Cases & Error Handling + def test_clear_registry(self): + """Test clearing the registry.""" + AddonImpRegistry.register("TEST_ADDON_IMP", 9999, MockStorageImp) + self.assertEqual(len(list(AddonImpRegistry.get_all_addon_imps())), 1) + + AddonImpRegistry.clear() + self.assertEqual(len(list(AddonImpRegistry.get_all_addon_imps())), 0) + + with self.assertRaises(KeyError): + AddonImpRegistry.get_imp_by_name("TEST_ADDON_IMP") + + def test_get_name_by_number_nonexistent(self): + """Test getting names by a nonexistent number raises KeyError.""" + with self.assertRaises(KeyError): + AddonImpRegistry.get_name_by_number(99999) + + @patch("addon_service.common.known_imps.logger") + def test_register_addon_imps_with_warning(self, mock_logger): + """Test warning when an addon imp is in ADDON_IMPS but not found.""" + ADDON_IMPS = { + "NONEXISTENT_ADDON_IMP": 5000, + } + + with patch( + "addon_service.common.known_imps.apps.get_app_configs", return_value=[] + ): + AddonImpRegistry.register_addon_imps(ADDON_IMPS) + + mock_logger.warning.assert_called_once() + self.assertIn( + "No addon imp has name NONEXISTENT_ADDON_IMP", + mock_logger.warning.call_args[0][0], + ) + + def test_register_addon_imps_with_known_addon_imps(self): + """Test registering known built-in addon imps through register_addon_imps.""" + addon_imps = { + "BOX": 1001, + "DROPBOX": 1006, + } + + with patch( + "addon_service.common.known_imps.apps.get_app_configs", return_value=[] + ): + AddonImpRegistry.register_addon_imps(addon_imps) + + self.assertEqual( + AddonImpRegistry.get_imp_by_number(1001), KnownAddonImps.BOX.value + ) + self.assertEqual( + AddonImpRegistry.get_imp_by_number(1006), KnownAddonImps.DROPBOX.value + ) diff --git a/addon_service/tests/test_foreign_addon_imp_integration.py b/addon_service/tests/test_foreign_addon_imp_integration.py new file mode 100644 index 00000000..17f0683d --- /dev/null +++ b/addon_service/tests/test_foreign_addon_imp_integration.py @@ -0,0 +1,394 @@ +"""Integration tests for Foreign Addon Imps feature.""" + +from unittest.mock import patch + +from django.test import TestCase + +from addon_service.addon_imp.models import AddonImpModel +from addon_service.common.known_imps import AddonImpRegistry +from addon_toolkit import AddonImp +from addon_toolkit.interfaces.citation import CitationAddonImp +from addon_toolkit.interfaces.foreign_addon_imp_config import ForeignAddonImpConfig +from addon_toolkit.interfaces.storage import StorageAddonImp + + +# Mock Foreign Addon Imp Implementation +class MockForeignStorageImp(StorageAddonImp): + """Mock foreign storage addon imp implementation.""" + + +class AltMockForeignStorageImp(StorageAddonImp): + pass + + +class MockForeignCitationImp(CitationAddonImp): + """Mock foreign citation addon imp implementation.""" + + pass + + +class MockForeignAddonImpConfig(ForeignAddonImpConfig): + """Mock foreign addon imp Django app config.""" + + name = "foreign_addon_imps.mock_storage" + verbose_name = "Mock Foreign Addon Imp" + path = "/fake/path" + + @property + def imp(self): + """Return the AddonImp subclass.""" + return MockForeignStorageImp + + @property + def addon_imp_name(self): + """Return the addon imp name for registration.""" + return "MOCK_STORAGE" + + +class AltMockForeignAddonImpConfig(ForeignAddonImpConfig): + name = "alt_foreign_addon_imps.alt_mock_storage" + verbose_name = "Mock Foreign Addon Imp Alternative" + path = "/fake/path" + + @property + def imp(self): + return AltMockForeignStorageImp + + @property + def addon_imp_name(self): + """Return the addon imp name for registration.""" + return "ALT_MOCK_STORAGE" + + +class MockForeignCitationAddonImpConfig(ForeignAddonImpConfig): + name = "foreign_addon_imps.mock_citation" + verbose_name = "Mock Foreign Citation Addon Imp" + path = "/fake/path" + + @property + def imp(self): + return MockForeignCitationImp + + @property + def addon_imp_name(self): + return "MOCK_FOREIGN_CITATION" + + +class TestForeignAddonImpDiscovery(TestCase): + """Test foreign addon imp discovery and loading.""" + + def setUp(self): + """Save and Clear registry before each test.""" + self._original_name_imp_map = AddonImpRegistry._name_imp_map.copy() + self._original_number_name_map = AddonImpRegistry._number_name_map.copy() + AddonImpRegistry.clear() + + def tearDown(self): + """Restore original registry state after each test.""" + AddonImpRegistry.clear() + AddonImpRegistry._name_imp_map.update(self._original_name_imp_map) + AddonImpRegistry._number_name_map.update(self._original_number_name_map) + + def test_foreign_addon_imp_discovery(self): + """Test that foreign addon imps are discovered from app configs.""" + mock_app_config = MockForeignAddonImpConfig( + "foreign_addon_imps.mock_storage", None + ) + + ADDON_IMPS = { + "MOCK_STORAGE": 5001, + } + + with patch( + "addon_service.common.known_imps.apps.get_app_configs", + return_value=[mock_app_config], + ): + AddonImpRegistry.register_addon_imps(ADDON_IMPS) + + self.assertEqual( + AddonImpRegistry.get_imp_by_name("MOCK_STORAGE"), MockForeignStorageImp + ) + self.assertEqual( + AddonImpRegistry.get_imp_by_number(5001), MockForeignStorageImp + ) + + def test_foreign_addon_imp_with_app_name_fallback(self): + """Test that foreign addon imp can be registered using app.name if addon_imp_name is not in ADDON_IMPS.""" + mock_app_config = MockForeignAddonImpConfig( + "foreign_addon_imps.mock_storage", None + ) + + # Use the app's name instead of addon_imp_name + ADDON_IMPS = { + "foreign_addon_imps.mock_storage": 5001, + } + + with patch( + "addon_service.common.known_imps.apps.get_app_configs", + return_value=[mock_app_config], + ): + AddonImpRegistry.register_addon_imps(ADDON_IMPS) + + # Should be registered under the app name + self.assertEqual( + AddonImpRegistry.get_imp_by_name("foreign_addon_imps.mock_storage"), + MockForeignStorageImp, + ) + self.assertEqual( + AddonImpRegistry.get_imp_by_number(5001), MockForeignStorageImp + ) + + def test_multiple_foreign_addon_imps(self): + """Test registering multiple foreign addon imps.""" + + ADDON_IMPS = { + "MOCK_STORAGE": 5001, + "ALT_MOCK_STORAGE": 5002, + } + + with patch( + "addon_service.common.known_imps.apps.get_app_configs", + return_value=[ + MockForeignAddonImpConfig("foreign_addon_imps.mock_storage", None), + AltMockForeignAddonImpConfig( + "alt_foreign_addon_imps.alt_mock_storage", None + ), + ], + ): + AddonImpRegistry.register_addon_imps(ADDON_IMPS) + + self.assertEqual( + AddonImpRegistry.get_imp_by_number(5001), MockForeignStorageImp + ) + self.assertEqual( + AddonImpRegistry.get_imp_by_number(5002), AltMockForeignStorageImp + ) + + def test_mixing_foreign_and_builtin_addon_imps(self): + """Test that foreign and built-in addon imps can coexist.""" + mock_app_config = MockForeignAddonImpConfig( + "foreign_addon_imps.mock_storage", None + ) + + ADDON_IMPS = { + "BOX": 1001, # Built-in addon + "MOCK_STORAGE": 5001, # Foreign addon imp + } + + with patch( + "addon_service.common.known_imps.apps.get_app_configs", + return_value=[mock_app_config], + ): + AddonImpRegistry.register_addon_imps(ADDON_IMPS) + + # Both should be registered + from addon_service.common.known_imps import KnownAddonImps + + self.assertEqual( + AddonImpRegistry.get_imp_by_number(1001), KnownAddonImps.BOX.value + ) + self.assertEqual( + AddonImpRegistry.get_imp_by_number(5001), MockForeignStorageImp + ) + + @patch("addon_service.common.known_imps.logger") + def test_foreign_addon_imp_not_in_installed_apps(self, mock_logger): + """Test warning when foreign addon imp in ADDON_IMPS but not in INSTALLED_APPS.""" + ADDON_IMPS = { + "MISSING_FOREIGN": 5001, + } + + # No app configs returned (simulating addon imp app not in INSTALLED_APPS) + with patch( + "addon_service.common.known_imps.apps.get_app_configs", return_value=[] + ): + AddonImpRegistry.register_addon_imps(ADDON_IMPS) + + mock_logger.warning.assert_called_once() + self.assertIn( + "No addon imp has name MISSING_FOREIGN", mock_logger.warning.call_args[0][0] + ) + + +class TestForeignAddonImpInterfaceValidation(TestCase): + """Test foreign addon imp interface requirements.""" + + def test_foreign_addon_imp_operations(self): + """Test that foreign addon imp operations are properly exposed.""" + operations = MockForeignStorageImp.all_implemented_operations() + + self.assertEqual(len(operations), 0) + + +class TestForeignAddonImpAPIIntegration(TestCase): + """Test that foreign addon imps integrate with existing API.""" + + def setUp(self): + """Set up test with registered foreign addon imps.""" + self._original_name_imp_map = AddonImpRegistry._name_imp_map.copy() + self._original_number_name_map = AddonImpRegistry._number_name_map.copy() + AddonImpRegistry.clear() + with patch( + "addon_service.common.known_imps.apps.get_app_configs", + return_value=[ + MockForeignAddonImpConfig("foreign_addon_imps.mock_storage", None) + ], + ): + # Register built-in and foreign addon imps for testing + ADDON_IMPS = { + # Type: Storage + "BOX": 1001, + "S3": 1003, + "GOOGLEDRIVE": 1005, + "DROPBOX": 1006, + "FIGSHARE": 1007, + "ONEDRIVE": 1008, + "OWNCLOUD": 1009, + "DATAVERSE": 1010, + "GITLAB": 1011, + "BITBUCKET": 1012, + "GITHUB": 1013, + "AZUREBLOBSTORAGE": 1014, + # Type: Citation + "ZOTERO": 1002, + "MENDELEY": 1004, + # Type: Cloud Computing + "BOA": 1020, + # Type: Link + "LINK_DATAVERSE": 1030, + # Foreign Addon Imps + "MOCK_STORAGE": 5001, + # For testing + "BLARG": -7, + } + AddonImpRegistry.register_addon_imps(ADDON_IMPS) + + def tearDown(self): + """Restore original registry state after each test.""" + AddonImpRegistry.clear() + AddonImpRegistry._name_imp_map.update(self._original_name_imp_map) + AddonImpRegistry._number_name_map.update(self._original_number_name_map) + + def test_foreign_addon_imp_in_addon_imp_model(self): + """Test that foreign addon imps appear in AddonImpModel.iter_all().""" + all_imps = list(AddonImpModel.iter_all()) + + # Should include our mock foreign addon imp + imp_classes = [imp.imp_cls for imp in all_imps] + self.assertIn(MockForeignStorageImp, imp_classes) + + def test_foreign_addon_imp_model_properties(self): + """Test that AddonImpModel works correctly with foreign addon imps.""" + imp_model = AddonImpModel(MockForeignStorageImp) + + self.assertEqual(imp_model.name, "MOCK_STORAGE") + self.assertEqual(imp_model.static_key, "MOCK_STORAGE") + self.assertEqual(imp_model.imp_cls, MockForeignStorageImp) + + def test_foreign_addon_imp_operations_in_model(self): + """Test that foreign addon imp operations are accessible through AddonImpModel.""" + imp_model = AddonImpModel(MockForeignStorageImp) + operations = imp_model.implemented_operations + + # Should have no operations for our mock implementation + self.assertEqual(len(operations), 0) + + def test_foreign_addon_imp_init_from_static_key(self): + """Test that AddonImpModel can be initialized from foreign addon imp name.""" + imp_model = AddonImpModel.init_args_from_static_key("MOCK_STORAGE") + self.assertEqual(imp_model, (MockForeignStorageImp,)) + + def test_foreign_addon_imp_serialization_compatibility(self): + """Test that foreign addon imps work with existing serializers.""" + from rest_framework.test import APIRequestFactory + + from addon_service.addon_imp.serializers import AddonImpSerializer + + imp_model = AddonImpModel(MockForeignStorageImp) + + # Create a mock request for the serializer context + factory = APIRequestFactory() + request = factory.get("/api/addon-imps/") + + serializer = AddonImpSerializer(imp_model, context={"request": request}) + + data = serializer.data + self.assertEqual(data["name"], "MOCK_STORAGE") + self.assertIn("docstring", data) + self.assertIn("interface_docstring", data) + + +class TestForeignAddonImpRegistryPersistence(TestCase): + """Test that foreign addon imp registration persists correctly.""" + + def test_registry_state_after_multiple_registrations(self): + """Test registry state remains consistent after multiple operations.""" + AddonImpRegistry.clear() + + AddonImpRegistry.register("MOCK_STORAGE", 5001, MockForeignStorageImp) + AddonImpRegistry.register("ALT_MOCK_STORAGE", 5002, AltMockForeignStorageImp) + + # Clear and re-register with different config + AddonImpRegistry.clear() + ADDON_IMPS = { + "MOCK_FOREIGN_CITATION": 5003, + } + with patch( + "addon_service.common.known_imps.apps.get_app_configs", + return_value=[ + MockForeignCitationAddonImpConfig( + "foreign_addon_imps.mock_citation", None + ), + ], + ): + AddonImpRegistry.register_addon_imps(ADDON_IMPS) + + self.assertEqual(len(list(AddonImpRegistry.get_all_addon_imps())), 1) + self.assertEqual( + AddonImpRegistry.get_imp_by_number(5003), MockForeignCitationImp + ) + + with self.assertRaises(KeyError): + AddonImpRegistry.get_imp_by_number(5001) + + def test_foreign_addon_imp_type_filtering(self): + """Test that foreign addon imps are correctly filtered by type.""" + AddonImpRegistry.clear() + + ADDON_IMPS = { + "MOCK_STORAGE": 5001, + "ALT_MOCK_STORAGE": 5002, + "MOCK_FOREIGN_CITATION": 5003, + } + + with patch( + "addon_service.common.known_imps.apps.get_app_configs", + return_value=[ + MockForeignAddonImpConfig("foreign_addon_imps.mock_storage", None), + AltMockForeignAddonImpConfig( + "alt_foreign_addon_imps.alt_mock_storage", None + ), + MockForeignCitationAddonImpConfig( + "foreign_addon_imps.mock_citation", None + ), + ], + ): + AddonImpRegistry.register_addon_imps(ADDON_IMPS) + + # Filter by storage type + storage_addons = list(AddonImpRegistry.iter_by_type(StorageAddonImp)) + self.assertEqual(len(storage_addons), 2) + self.assertEqual(storage_addons[0], (5001, "MOCK_STORAGE")) + self.assertEqual(storage_addons[1], (5002, "ALT_MOCK_STORAGE")) + + # Filter by citation type + citation_addons = list(AddonImpRegistry.iter_by_type(CitationAddonImp)) + self.assertEqual(len(citation_addons), 1) + self.assertEqual(citation_addons[0], (5003, "MOCK_FOREIGN_CITATION")) + + # Filter Nothing + all_addons = list(AddonImpRegistry.iter_by_type(AddonImp)) + self.assertEqual(len(all_addons), 3) + self.assertEqual(all_addons[0], (5001, "MOCK_STORAGE")) + self.assertEqual(all_addons[1], (5002, "ALT_MOCK_STORAGE")) + self.assertEqual(all_addons[2], (5003, "MOCK_FOREIGN_CITATION")) diff --git a/addon_service/tests/test_hmac_api_auth.py b/addon_service/tests/test_hmac_api_auth.py index 936ee3f5..5c43725e 100644 --- a/addon_service/tests/test_hmac_api_auth.py +++ b/addon_service/tests/test_hmac_api_auth.py @@ -169,6 +169,11 @@ def test_has_osf_permission_on_resource__wrong_key(self): class TestHmacApiAuth(APITestCase): @classmethod def setUpTestData(cls): + """Reset registry after test.""" + from addon_service.common.known_imps import AddonImpRegistry + + AddonImpRegistry.clear() + AddonImpRegistry.register_addon_imps({"BLARG": -7}) cls._user = _factories.UserReferenceFactory() cls._resource = _factories.ResourceReferenceFactory() cls._service = _factories.ExternalStorageOAuth2ServiceFactory() diff --git a/addon_service/tests/test_static_dataclasses_api.py b/addon_service/tests/test_static_dataclasses_api.py index 2e3de1aa..56efe21b 100644 --- a/addon_service/tests/test_static_dataclasses_api.py +++ b/addon_service/tests/test_static_dataclasses_api.py @@ -1,28 +1,164 @@ +from unittest.mock import patch + from django.urls import reverse from rest_framework.test import APITestCase -from addon_service.common import known_imps +from addon_service.common.known_imps import AddonImpRegistry +from addon_toolkit.interfaces.foreign_addon_imp_config import ForeignAddonImpConfig +from addon_toolkit.interfaces.storage import StorageAddonImp + + +class MockForeignStorageImp(StorageAddonImp): + """Mock foreign addon for API testing.""" + + +class MockForeignAddonConfig(ForeignAddonImpConfig): + """Mock foreign addon config for API testing.""" + + name = "foreign_addons.mock_storage" + path = "/fake/path" # Add a fake path to satisfy Django's requirements + + @property + def imp(self): + return MockForeignStorageImp + + @property + def addon_imp_name(self): + return "MOCK_STORAGE" class TestAddonImpsView(APITestCase): + def setUp(self): + """Set up test with clean registry.""" + self._original_name_imp_map = AddonImpRegistry._name_imp_map.copy() + self._original_number_name_map = AddonImpRegistry._number_name_map.copy() + AddonImpRegistry.clear() + with patch( + "addon_service.common.known_imps.apps.get_app_configs", + return_value=[MockForeignAddonConfig("foreign_addons.mock_storage", None)], + ): + # Register built-in and foreign addon imps for testing + ADDON_IMPS = { + # Type: Storage + "BOX": 1001, + "S3": 1003, + "GOOGLEDRIVE": 1005, + "DROPBOX": 1006, + "FIGSHARE": 1007, + "ONEDRIVE": 1008, + "OWNCLOUD": 1009, + "DATAVERSE": 1010, + "GITLAB": 1011, + "BITBUCKET": 1012, + "GITHUB": 1013, + "AZUREBLOBSTORAGE": 1014, + # Type: Citation + "ZOTERO": 1002, + "MENDELEY": 1004, + # Type: Cloud Computing + "BOA": 1020, + # Type: Link + "LINK_DATAVERSE": 1030, + # Foreign Addon Imps + "MOCK_STORAGE": 5001, + # For testing + "BLARG": -7, + } + AddonImpRegistry.register_addon_imps(ADDON_IMPS) + + def tearDown(self): + """Restore original registry state after each test.""" + AddonImpRegistry.clear() + AddonImpRegistry._name_imp_map.update(self._original_name_imp_map) + AddonImpRegistry._number_name_map.update(self._original_number_name_map) + def test_get(self): _resp = self.client.get(reverse("addon-imps-list")) _data = _resp.json()["data"] - _expected_names = {_imp.name for _imp in known_imps.KnownAddonImps} - _actual_names = {_datum["attributes"]["name"] for _datum in _data} - self.assertEqual(_expected_names, _actual_names) + _registered_names = { + AddonImpRegistry.get_imp_name(imp) + for imp in AddonImpRegistry.get_all_addon_imps() + } + _retrieved_names = {_datum["attributes"]["name"] for _datum in _data} + self.assertEqual(_registered_names, _retrieved_names) def test_unallowed_methods(self): ... class TestAddonOperationsView(APITestCase): + def setUp(self): + """Set up test with clean registry.""" + self._original_name_imp_map = AddonImpRegistry._name_imp_map.copy() + self._original_number_name_map = AddonImpRegistry._number_name_map.copy() + AddonImpRegistry.clear() + with patch( + "addon_service.common.known_imps.apps.get_app_configs", + return_value=[MockForeignAddonConfig("foreign_addons.mock_storage", None)], + ): + # Register built-in and foreign addon imps for testing + ADDON_IMPS = { + # Type: Storage + "BOX": 1001, + "S3": 1003, + "GOOGLEDRIVE": 1005, + "DROPBOX": 1006, + "FIGSHARE": 1007, + "ONEDRIVE": 1008, + "OWNCLOUD": 1009, + "DATAVERSE": 1010, + "GITLAB": 1011, + "BITBUCKET": 1012, + "GITHUB": 1013, + "AZUREBLOBSTORAGE": 1014, + # Type: Citation + "ZOTERO": 1002, + "MENDELEY": 1004, + # Type: Cloud Computing + "BOA": 1020, + # Type: Link + "LINK_DATAVERSE": 1030, + # Foreign Addon Imps + "MOCK_STORAGE": 5001, + # For testing + "BLARG": -7, + } + AddonImpRegistry.register_addon_imps(ADDON_IMPS) + + def tearDown(self): + """Restore original registry state after each test.""" + AddonImpRegistry.clear() + AddonImpRegistry._name_imp_map.update(self._original_name_imp_map) + AddonImpRegistry._number_name_map.update(self._original_number_name_map) + def test_get(self): _resp = self.client.get(reverse("addon-operations-list")) _data = _resp.json()["data"] - _expected_names = { + _implemented_names = { + _op.name + for _imp in AddonImpRegistry.get_all_addon_imps() + for _op in _imp.all_implemented_operations() + } + _declared_names = {_datum["attributes"]["name"] for _datum in _data} + self.assertEqual(_implemented_names, _declared_names) + self.assertTrue( + _implemented_names.issubset(_declared_names), + f"Expected operations {_implemented_names - _declared_names} not found in API", + ) + + def test_get_with_foreign_addon_operations(self): + """Test that foreign addon operations appear in the API response.""" + AddonImpRegistry.clear() + AddonImpRegistry.register("MOCK_STORAGE", 5002, MockForeignStorageImp) + + _resp = self.client.get(reverse("addon-operations-list")) + _data = _resp.json()["data"] + _implemented_names = { _op.name - for _imp in known_imps.KnownAddonImps - for _op in _imp.value.all_implemented_operations() + for _imp in AddonImpRegistry.get_all_addon_imps() + for _op in _imp.all_implemented_operations() } - _actual_names = {_datum["attributes"]["name"] for _datum in _data} - self.assertEqual(_expected_names, _actual_names) + _declared_names = {_datum["attributes"]["name"] for _datum in _data} + self.assertTrue( + _implemented_names.issubset(_declared_names), + f"Expected operations {_implemented_names - _declared_names} not found in API", + ) diff --git a/addon_toolkit/interfaces/foreign_addon_imp_config.py b/addon_toolkit/interfaces/foreign_addon_imp_config.py new file mode 100644 index 00000000..e7a8fdd4 --- /dev/null +++ b/addon_toolkit/interfaces/foreign_addon_imp_config.py @@ -0,0 +1,27 @@ +from abc import ( + ABC, + abstractmethod, +) + +from django.apps import AppConfig + +from ..imp import AddonImp + + +class ForeignAddonImpConfig(AppConfig, ABC): + """Abstract Base Class for Foreign Addon Imps""" + + @property + @abstractmethod + def imp(self) -> AddonImp: + """Return the AddonImp subclass of this Foreign Addon Imp.""" + pass + + @property + @abstractmethod + def addon_imp_name(self) -> str: + """ + Return the unique name identifying this addon imp app on the + gravyvalet system. + """ + pass diff --git a/app/settings.py b/app/settings.py index c471057d..b2067e9e 100644 --- a/app/settings.py +++ b/app/settings.py @@ -114,6 +114,33 @@ "drf_spectacular", ] +ADDON_IMPS = { + # Type: Storage + "BOX": 1001, + "S3": 1003, + "GOOGLEDRIVE": 1005, + "DROPBOX": 1006, + "FIGSHARE": 1007, + "ONEDRIVE": 1008, + "OWNCLOUD": 1009, + "DATAVERSE": 1010, + "GITLAB": 1011, + "BITBUCKET": 1012, + "GITHUB": 1013, + "AZUREBLOBSTORAGE": 1014, + # Type: Citation + "ZOTERO": 1002, + "MENDELEY": 1004, + # Type: Cloud Computing + "BOA": 1020, + # Type: Link + "LINK_DATAVERSE": 1030, + # Foreign Addon Imps +} + +if __debug__: + ADDON_IMPS["BLARG"] = -7 + MIDDLEWARE = [ "corsheaders.middleware.CorsMiddleware", "django.middleware.security.SecurityMiddleware",