From 6ddeb1114246a541d699a0aa0b5ff4b1e3d50389 Mon Sep 17 00:00:00 2001 From: Andrew Davison Date: Wed, 16 Oct 2024 16:23:39 +0200 Subject: [PATCH] Support adding new model instances to already-released model projects. This basically involves using both user and service clients. Note that the service client needs at least read access to the private (collab) space (need to add model-curators group to the collab team with "viewer" role) --- .../validation_service/db.py | 28 +++++----- .../validation_service/resources/models.py | 43 ++++++++++----- .../validation_service/resources/tests.py | 2 +- .../validation_service/tests/fixtures.py | 8 +++ .../validation_service/tests/test_models.py | 52 ++++++++++++++++++- 5 files changed, 105 insertions(+), 28 deletions(-) diff --git a/validation_service_api/validation_service/db.py b/validation_service_api/validation_service/db.py index 670ef756..7186c1b5 100644 --- a/validation_service_api/validation_service/db.py +++ b/validation_service_api/validation_service/db.py @@ -9,6 +9,7 @@ from fairgraph.openminds.core import Model, ModelVersion, SoftwareVersion from fairgraph.openminds.computation import ValidationTest, ValidationTestVersion from . import settings +from .auth import get_kg_client_for_service_account RETRY_INTERVAL = 60 # seconds @@ -21,13 +22,13 @@ def _check_service_status(): ) -def _get_model_by_id_or_alias(model_id, kg_client, scope): +def _get_model_by_id_or_alias(model_id, kg_client, scope, use_cache=False): try: model_id = UUID(model_id) get_model = Model.from_uuid except ValueError: get_model = Model.from_alias - model_project = get_model(str(model_id), kg_client, scope=scope) + model_project = get_model(str(model_id), kg_client, scope=scope, use_cache=use_cache) if not model_project: raise HTTPException( status_code=status.HTTP_404_NOT_FOUND, @@ -45,21 +46,24 @@ def _get_model_instance_by_id(instance_id, kg_client, scope): detail=f"Model instance with ID '{instance_id}' not found.", ) - model_project = Model.list(kg_client, scope=scope, space=model_instance.space, - has_versions=model_instance) + model_project = Model.list(kg_client, scope=scope, has_versions=model_instance, use_cache=False) if not model_project: # we could get an empty response if the model_project has just been # updated and the KG is not consistent, so we wait and try again sleep(RETRY_INTERVAL) - model_project = Model.list(kg_client, scope=scope, space=model_instance.space, - has_versions=model_instance) + model_project = Model.list(kg_client, scope=scope, has_versions=model_instance, use_cache=False) if not model_project: - # in case of a dangling model instance, where the parent model_project - # has been deleted but the instance wasn't - raise HTTPException( - status_code=status.HTTP_404_NOT_FOUND, - detail=f"Model instance with ID '{instance_id}' no longer exists.", - ) + # try the service client + kg_service_client = get_kg_client_for_service_account() + model_project = Model.list(kg_service_client, scope="in progress", has_versions=model_instance, use_cache=False) + if not model_project: + raise Exception("foo") + # in case of a dangling model instance, where the parent model_project + # has been deleted but the instance wasn't + raise HTTPException( + status_code=status.HTTP_404_NOT_FOUND, + detail=f"Model instance with ID '{instance_id}' no longer exists.", + ) model_project = model_project[0] return model_instance, model_project.uuid diff --git a/validation_service_api/validation_service/resources/models.py b/validation_service_api/validation_service/resources/models.py index a1dd0913..4e33fe36 100644 --- a/validation_service_api/validation_service/resources/models.py +++ b/validation_service_api/validation_service/resources/models.py @@ -5,7 +5,7 @@ from fairgraph.utility import as_list import fairgraph.openminds.core as omcore import fairgraph.openminds.computation as omcmp -from fairgraph.errors import ResolutionFailure +from fairgraph.errors import ResolutionFailure, AuthorizationError, ResourceExistsError from fastapi import APIRouter, Depends, Query, Path, HTTPException, status from fastapi.security import HTTPBearer, HTTPAuthorizationCredentials @@ -26,6 +26,7 @@ ModelInstance, ModelInstancePatch, project_id_from_space, + space_from_project_id, special_spaces ) from ..queries import build_model_project_filters, model_alias_exists, expand_combinations @@ -393,7 +394,9 @@ async def create_model( ) assert space_name == kg_space - model_project.save(kg_user_client, space=kg_space, recursive=True) + # todo: we might want to save the tree explicitly rather than recursively, + # to have more control over when we ignore duplicates + model_project.save(kg_user_client, space=kg_space, recursive=True, ignore_duplicates=True) model_project.scope = "in progress" return ScientificModel.from_kg_object(model_project, kg_user_client) @@ -629,9 +632,15 @@ async def create_model_instance( detail=f"Another model instance with the same name already exists.", ) # otherwise save to KG - model_instance_kg.save(kg_user_client, space=model_project.space, recursive=True) + target_space = space_from_project_id(collab_id) + model_instance_kg.save(kg_user_client, space=target_space, recursive=True) model_project.has_versions = as_list(model_project.has_versions) + [model_instance_kg] - model_project.save(kg_user_client, recursive=False) + try: + model_project.save(kg_user_client, recursive=False) + except AuthorizationError: + # if the model project has already been published we may not be able + # to save with user client, so use service client + model_project.save(kg_service_client, recursive=False) return ModelInstance.from_kg_object(model_instance_kg, kg_user_client, model_project.uuid, scope="any") @@ -708,18 +717,18 @@ async def delete_model_instance_by_id( _check_service_status() user = User(token, allow_anonymous=False) kg_user_client = get_kg_client_for_user_account(token) + kg_service_client = get_kg_client_for_service_account() model_instance_kg, model_id = _get_model_instance_by_id(model_instance_id, kg_user_client, scope="any") - model_project = _get_model_by_id_or_alias(model_id, kg_user_client, scope="any") - collab_id = model_project.space[len("collab-"):] + collab_id = project_id_from_space(model_instance_kg.space) if not ( await user.can_edit_collab(collab_id) or await user.is_admin() ): raise HTTPException( status_code=status.HTTP_403_FORBIDDEN, - detail=f"Access to this model is restricted to members of Collab #{model_project.collab_id}", + detail=f"Access to this model is restricted to members of Collab #{model_instance_kg.space}", ) - await _delete_model_instance(model_instance_id, model_project, kg_user_client) + await _delete_model_instance(model_instance_id, model_id, kg_user_client, kg_service_client) @router.delete("/models/{model_id}/instances/{model_instance_id}", status_code=status.HTTP_200_OK) @@ -730,21 +739,27 @@ async def delete_model_instance( # todo: handle non-existent UUID user = User(token, allow_anonymous=False) kg_user_client = get_kg_client_for_user_account(token) + kg_service_client = get_kg_client_for_service_account() model_instance_kg, model_id = _get_model_instance_by_id(model_instance_id, kg_user_client, scope="any") - model_project = _get_model_by_id_or_alias(model_id, kg_user_client, scope="any") - collab_id = model_project.space[len("collab-"):] + collab_id = project_id_from_space(model_instance_kg.space) if not ( await user.can_edit_collab(collab_id) or await user.is_admin() ): raise HTTPException( status_code=status.HTTP_403_FORBIDDEN, - detail=f"Access to this model is restricted to members of Collab #{model_project.collab_id}", + detail=f"Access to this model is restricted to members of Collab #{model_instance_kg.space}", ) - await _delete_model_instance(model_instance_id, model_project, kg_user_client) + await _delete_model_instance(model_instance_id, model_id, kg_user_client, kg_service_client) -async def _delete_model_instance(model_instance_id, model_project, kg_user_client): +async def _delete_model_instance(model_instance_id, model_id, kg_user_client, kg_service_client): + try: + model_project = _get_model_by_id_or_alias(model_id, kg_user_client, scope="in progress", use_cache=False) + kg_client_for_model = kg_user_client + except HTTPException: + model_project = _get_model_by_id_or_alias(model_id, kg_service_client, scope="in progress", use_cache=False) + kg_client_for_model = kg_service_client model_instances = as_list(model_project.has_versions) n_start = len(model_instances) for model_instance in model_instances[:]: @@ -757,4 +772,4 @@ async def _delete_model_instance(model_instance_id, model_project, kg_user_clien if n_start > 0: assert len(model_instances) == n_start - 1 model_project.has_versions = model_instances - model_project.save(kg_user_client, recursive=False) + model_project.save(kg_client_for_model, recursive=False) diff --git a/validation_service_api/validation_service/resources/tests.py b/validation_service_api/validation_service/resources/tests.py index f3879a1e..b8e2d584 100644 --- a/validation_service_api/validation_service/resources/tests.py +++ b/validation_service_api/validation_service/resources/tests.py @@ -261,7 +261,7 @@ async def create_test(test: ValidationTest, token: HTTPAuthorizationCredentials assert space_name == kg_space try: - test_definition.save(kg_user_client, recursive=True, space=kg_space) + test_definition.save(kg_user_client, recursive=True, space=kg_space, ignore_duplicates=True) except AuthenticationError as err: user_info = await user.get_user_info() raise HTTPException( diff --git a/validation_service_api/validation_service/tests/fixtures.py b/validation_service_api/validation_service/tests/fixtures.py index 3589b4c2..4289c6da 100644 --- a/validation_service_api/validation_service/tests/fixtures.py +++ b/validation_service_api/validation_service/tests/fixtures.py @@ -17,6 +17,14 @@ credentials = os.environ["VF_TEST_TOKEN"] ) AUTH_HEADER = {"Authorization": f"{token.scheme} {token.credentials}"} +if "VF_ADMIN_TOKEN" in os.environ: + admin_token = HTTPAuthorizationCredentials( + scheme="Bearer", + credentials = os.environ["VF_ADMIN_TOKEN"] + ) + ADMIN_AUTH_HEADER = {"Authorization": f"{admin_token.scheme} {admin_token.credentials}"} +else: + ADMIN_AUTH_HEADER = None #TEST_SPACE = "myspace" #TEST_PROJECT = "myspace" TEST_PROJECT = "validation-framework-testing" diff --git a/validation_service_api/validation_service/tests/test_models.py b/validation_service_api/validation_service/tests/test_models.py index cdc10d1c..b821fcbf 100644 --- a/validation_service_api/validation_service/tests/test_models.py +++ b/validation_service_api/validation_service/tests/test_models.py @@ -9,7 +9,10 @@ import pytest from ..data_models import BrainRegion, Species -from .fixtures import _build_sample_model, private_model, released_model, client, token, AUTH_HEADER, TEST_PROJECT +from .fixtures import ( + _build_sample_model, private_model, released_model, client, token, + AUTH_HEADER, ADMIN_AUTH_HEADER, TEST_PROJECT +) def check_model(model): @@ -654,6 +657,53 @@ def test_delete_model_instance(caplog): assert response.status_code == 200 +def test_add_instance_to_published_model(caplog): + # We retrieve a model in the "model" space that has been released + # A normal user should be able to add an instance to this model + # but should not be able to change any other metadata. + published_model_uuid = "e919d175-b831-4295-b84c-9b00c944f0e3" # Olfactory bulb network model 2003 + response = client.get(f"/models/{published_model_uuid}") + assert response.status_code == 200 + published_model = response.json() + assert len(published_model["instances"]) == 3 # three published versions + assert published_model["project_id"] == "model" + now = datetime.now(timezone.utc) + new_instance = { + "version": f"test-{now.strftime('%Y-%m-%dT%H:%M:%S.%f')}", + "description": "This is fake data for testing", + "code_format": "application/vnd.neuron-simulator+hoc", + "source": "http://example.com/fake_olfactory_bulb_model_for_testing", + "license": "The MIT license", + "project_id": TEST_PROJECT + } + response = client.post(f"/models/{published_model_uuid}/instances/", json=new_instance, headers=AUTH_HEADER) + assert response.status_code == 201 + new_instance_uuid = response.json()["id"] + + # get the model project again without authentication - should still have 3 (published) instances + response = client.get(f"/models/{published_model_uuid}") + assert response.status_code == 200 + published_model_again = response.json() + assert len(published_model_again["instances"]) == 3 # three published versions + + # get the model project again with an admin account - should now have 4 instances - 3 published plus one unpublished + # note that this doesn't work with a normal user account, which can't get in-progress info from "model" space + response = client.get(f"/models/{published_model_uuid}", headers=ADMIN_AUTH_HEADER) + assert response.status_code == 200 + published_model_again = response.json() + assert len(published_model_again["instances"]) == 4 + + # cleanup: delete model instance again + response = client.delete(f"/models/{published_model_uuid}/instances/{new_instance_uuid}", headers=AUTH_HEADER) + assert response.status_code == 200 + + # get the model project one last time with admin account - should be back to the three published instances + response = client.get(f"/models/{published_model_uuid}", headers=ADMIN_AUTH_HEADER) + assert response.status_code == 200 + published_model_again = response.json() + assert len(published_model_again["instances"]) == 3 + + def test_hhnb_models(caplog): # list of models used by the HHNB app, to check hhnb_models = [