Issue #78 start implementing update_service and improve tests

src/openeo_aggregator/backend.py

@@ -877,13 +877,13 @@ def create_service(self, user_id: str, process_graph: dict, service_type: str, a
 
         return service.service_id
 
-    def remove_service(self, user_id: str, service_id: str) -> None:
-        """https://openeo.org/documentation/1.0/developers/api/reference.html#operation/delete-service"""
+    def _find_connection_with_service_id(self, service_id: str) -> BackendConnection:
+        """Get connection for the backend that contains the service, return None if not found."""
+
         # Search all services on the backends.
-        service = None
         for con in self._backends:
             try:
-                service = con.get(f"/services/{service_id}")
+                _ = con.get(f"/services/{service_id}")
             except OpenEoApiError as e:
                 if e.http_status_code == 404:
                     # Expected error
@@ -895,23 +895,52 @@ def remove_service(self, user_id: str, service_id: str) -> None:
             except Exception as e:
                 _log.warning(f"Failed to get service {service_id!r} from {con.id}: {e!r}", exc_info=True)
                 raise e
+            else:
+                return con
 
-            try:
-                con.delete(f"/services/{service_id}", expected_status=204)
-            except (OpenEoApiError, OpenEOApiException) as e:
-                # TODO: maybe we should just let these exception straight go to the caller without logging it here.
-                # Logging it here seems prudent and more consistent with the handling of unexpected exceptions below.
-                _log.warning(f"Failed to delete service {service_id!r} from {con.id}: {e!r}", exc_info=True)
-                raise
-            except Exception as e:
-                _log.warning(f"Failed to delete service {service_id!r} from {con.id}: {e!r}", exc_info=True)
-                raise OpenEOApiException(
-                    f"Failed to delete secondary service with id {service_id!r} on backend {con.id!r}: {e!r}"
-                ) from e
+        return None
+
+    def remove_service(self, user_id: str, service_id: str) -> None:
+        """https://openeo.org/documentation/1.0/developers/api/reference.html#operation/delete-service"""
+        con = self._find_connection_with_service_id(service_id)
+        if not con:
+            raise ServiceNotFoundException(service_id)
 
-        if not service:
+        try:
+            con.delete(f"/services/{service_id}", expected_status=204)
+        except (OpenEoApiError, OpenEOApiException) as e:
+            # TODO: maybe we should just let these exception straight go to the caller without logging it here.
+            # Logging it here seems prudent and more consistent with the handling of unexpected exceptions below.
+            _log.warning(f"Failed to delete service {service_id!r} from {con.id}: {e!r}", exc_info=True)
+            raise
+        except Exception as e:
+            _log.warning(f"Failed to delete service {service_id!r} from {con.id}: {e!r}", exc_info=True)
+            raise OpenEOApiException(
+                f"Failed to delete secondary service with id {service_id!r} on backend {con.id!r}: {e!r}"
+            ) from e
+
+    def update_service(self, user_id: str, service_id: str, process_graph: dict) -> None:
+        """https://openeo.org/documentation/1.0/developers/api/reference.html#operation/update-service"""
+
+        con = self._find_connection_with_service_id(service_id)
+        if not con:
             raise ServiceNotFoundException(service_id)
 
+        api_version = self._backends.api_version
+        try:
+            key = "process_graph" if api_version < ComparableVersion((1, 0, 0)) else "process"
+            con.patch(f"/services/{service_id}", json={key: process_graph}, expected_status=204)
+        except (OpenEoApiError, OpenEOApiException) as e:
+            # TODO: maybe we should just let these exception straight go to the caller without logging it here.
+            # Logging it here seems prudent and more consistent with the handling of unexpected exceptions below.
+            _log.warning(f"Failed to delete service {service_id!r} from {con.id}: {e!r}", exc_info=True)
+            raise
+        except Exception as e:
+            _log.warning(f"Failed to delete service {service_id!r} from {con.id}: {e!r}", exc_info=True)
+            raise OpenEOApiException(
+                f"Failed to delete secondary service with id {service_id!r} on backend {con.id!r}: {e!r}"
+            ) from e
+
 
 class AggregatorBackendImplementation(OpenEoBackendImplementation):
     # No basic auth: OIDC auth is required (to get EGI Check-in eduperson_entitlement data)
@@ -1115,3 +1144,9 @@ def create_service(self, user_id: str, process_graph: dict, service_type: str, a
     def remove_service(self, user_id: str, service_id: str) -> None:
         """https://openeo.org/documentation/1.0/developers/api/reference.html#operation/delete-service"""
         self.secondary_services.remove_service(user_id=user_id, service_id=service_id)
+
+    def update_service(self, user_id: str, service_id: str, process_graph: dict) -> None:
+        """https://openeo.org/documentation/1.0/developers/api/reference.html#operation/update-service"""
+        self.secondary_services.update_service(
+            user_id=user_id, service_id=service_id, process_graph=process_graph
+        )

tests/conftest.py

@@ -13,7 +13,7 @@
 
 
 @pytest.fixture
-def backend1(requests_mock) -> str:
+def backend1(requests_mock,) -> str:
     domain = "https://b1.test/v1"
     # TODO: how to work with different API versions?
     requests_mock.get(domain + "/", json={"api_version": "1.0.0"})
@@ -33,6 +33,17 @@ def backend2(requests_mock) -> str:
     return domain
 
 
+# as "lib_requests_mock": make a distinction with the pytest fixture that has the same name 
+import requests_mock as lib_requests_mock
+
+def set_backend_to_api_version(requests_mock: lib_requests_mock.Mocker, domain: str, api_version: str) -> str:
+    """Helper function to make the backend connection use the expected API version."""
+
+    # TODO: would like a nicer solution to make the backend fixtures match the expected API version.
+    # Good enough for now tough, just have to remember to call it in your test.
+    return requests_mock.get(f"{domain}/", json={"api_version": api_version})
+
+
 @pytest.fixture
 def main_test_oidc_issuer() -> str:
     """
@@ -137,12 +148,12 @@ def backend_implementation(flask_app) -> AggregatorBackendImplementation:
 
 
 @pytest.fixture(params=["0.4.0", "1.0.0"])
-def api_version(request):
+def api_version_fixture(request):
     """To go through all relevant API versions"""
     return request.param
 
 
-def get_api_version(flask_app, api_version) -> ApiTester:
+def get_api_tester(flask_app, api_version) -> ApiTester:
     return ApiTester(api_version=api_version, client=flask_app.test_client())
 
 
@@ -155,13 +166,13 @@ def get_api100(flask_app: flask.Flask) -> ApiTester:
 
 
 @pytest.fixture
-def api(flask_app, api_version) -> ApiTester:
+def api_tester(flask_app, api_version_fixture) -> ApiTester:
     """Get an ApiTester for each version.
 
     Useful when it easy to test several API versions with (mostly) the same test code.
     But when the difference is too big, just keep it simple and write separate tests.
     """
-    return get_api_version(flask_app, api_version)
+    return get_api_tester(flask_app, api_version_fixture)
 
 
 @pytest.fixture

tests/test_backend.py

@@ -14,8 +14,8 @@
 from openeo_driver.users.oidc import OidcProvider
 from openeo_driver.errors import ProcessGraphMissingException, ProcessGraphInvalidException, ServiceUnsupportedException
 from openeo.rest import OpenEoApiError, OpenEoRestError
-from .conftest import DEFAULT_MEMOIZER_CONFIG
-
+from .conftest import DEFAULT_MEMOIZER_CONFIG, set_backend_to_api_version
+  
 
 TEST_USER = "Mr.Test"
 
@@ -363,10 +363,11 @@ def test_service_info_wrong_id(
         with pytest.raises(ServiceNotFoundException):
             abe_implementation.service_info(user_id=TEST_USER, service_id="doesnotexist")
 
-    def test_create_service(self, api, multi_backend_connection, config, backend1, requests_mock):
+    def test_create_service(self, api_tester, multi_backend_connection, config, backend1, requests_mock):
         """When it gets a correct params for a new service, it successfully creates it."""
 
         # Set up responses for creating the service in backend 1
+        set_backend_to_api_version(requests_mock, backend1, api_tester.api_version)
         expected_openeo_id = "wmts-foo"
         location_backend_1 = backend1 + "/services/" + expected_openeo_id
         process_graph = {"foo": {"process_id": "foo", "arguments": {}}}
@@ -385,20 +386,21 @@ def test_create_service(self, api, multi_backend_connection, config, backend1, r
             user_id=TEST_USER,
             process_graph=process_graph,
             service_type="WMTS",
-            api_version=api.api_version,
+            api_version=api_tester.api_version,
             configuration={}
         )
         assert actual_openeo_id == expected_openeo_id
 
-    @pytest.mark.parametrize("api_version", ["0.4.0", "1.0.0", "1.1.0"])
     @pytest.mark.parametrize("exception_class", [OpenEoApiError, OpenEoRestError])
     def test_create_service_backend_raises_openeoapiexception(
-        self, multi_backend_connection, config, backend1, requests_mock, api_version, exception_class
+        self, api_tester, multi_backend_connection, config, backend1, backend2, requests_mock, exception_class
     ):
         """When the backend raises a general exception the aggregator raises an OpenEOApiException."""
 
         # Set up responses for creating the service in backend 1:
         # This time the backend raises an error, one that will be reported as a OpenEOApiException. 
+        set_backend_to_api_version(requests_mock, backend1, api_tester.api_version)
+        set_backend_to_api_version(requests_mock, backend2, api_tester.api_version)
         process_graph = {"foo": {"process_id": "foo", "arguments": {}}}
         requests_mock.post(
             backend1 + "/services",
@@ -411,23 +413,23 @@ def test_create_service_backend_raises_openeoapiexception(
                 user_id=TEST_USER,
                 process_graph=process_graph,
                 service_type="WMTS",
-                api_version=api_version,
+                api_version=api_tester.api_version,
                 configuration={}
             )
 
-    @pytest.mark.parametrize("api_version", ["0.4.0", "1.0.0", "1.1.0"])
     @pytest.mark.parametrize("exception_class",
         [ProcessGraphMissingException, ProcessGraphInvalidException, ServiceUnsupportedException]
     )
     def test_create_service_backend_reraises(
-        self, multi_backend_connection, config, backend1, requests_mock, api_version, exception_class
+        self, api_tester, multi_backend_connection, config, backend1, requests_mock, exception_class
     ):
         """When the backend raises exception types that indicate client error / bad input data,
         the aggregator raises and OpenEOApiException.
         """
 
         # Set up responses for creating the service in backend 1
         # This time the backend raises an error, one that will simply be re-raised/passed on as it is.
+        set_backend_to_api_version(requests_mock, backend1, api_tester.api_version)
         process_graph = {"foo": {"process_id": "foo", "arguments": {}}}
         requests_mock.post(
             backend1 + "/services",
@@ -441,7 +443,7 @@ def test_create_service_backend_reraises(
                 user_id=TEST_USER,
                 process_graph=process_graph,
                 service_type="WMTS",
-                api_version=api_version,
+                api_version=api_tester.api_version,
                 configuration={}
             )
 
@@ -475,7 +477,7 @@ def test_remove_service(
 
     @pytest.mark.parametrize("backend_status_code", [400, 500])
     def test_remove_service_backend_response_is_an_error_status(
-            self, multi_backend_connection, config, backend1, backend2, requests_mock,
+            self, multi_backend_connection, config, backend1, requests_mock,
             service_metadata_wmts_foo, backend_status_code
     ):
         """When the backend response is an error HTTP 400/500 then the aggregator raises an OpenEoApiError."""
@@ -517,6 +519,49 @@ def test_remove_service_service_id_not_found(
         with pytest.raises(ServiceNotFoundException):
             abe_implementation.remove_service(user_id=TEST_USER, service_id="wmts-foo")
 
+    def test_update_service(
+        self, api_tester, multi_backend_connection, config, backend1, backend2, requests_mock, service_metadata_wmts_foo
+    ):
+        """When it gets a correct service ID, it returns the expected ServiceMetadata."""
+
+        # Also test that it can skip backends that don't have the service
+        set_backend_to_api_version(requests_mock, backend1, api_tester.api_version)
+        set_backend_to_api_version(requests_mock, backend2, api_tester.api_version)
+        mock_get1 = requests_mock.get(
+            backend1 + "/services/wmts-foo",
+            status_code=404
+        )
+        # Update should succeed in backend2 so service should be present first.
+        service_metadata_wmts_foo
+        mock_get2 = requests_mock.get(
+            backend2 + "/services/wmts-foo",
+            json=service_metadata_wmts_foo.prepare_for_json(),
+            status_code=200
+        )
+        mock_patch = requests_mock.patch(
+            backend2 + "/services/wmts-foo",
+            status_code=204,
+        )
+        abe_implementation = AggregatorBackendImplementation(backends=multi_backend_connection, config=config)
+        process_graph_after = {"bar": {"process_id": "bar", "arguments": {"arg1": "bar"}}}
+
+        abe_implementation.update_service(user_id=TEST_USER, service_id="wmts-foo", process_graph=process_graph_after)
+
+        # # Make sure the aggregator asked the backend to remove the service.
+        assert mock_patch.called
+        from openeo.capabilities import ComparableVersion
+        comp_api_version = ComparableVersion(api_tester.api_version)
+        if comp_api_version < ComparableVersion((1, 0, 0)):
+            expected_process = {"process_graph": process_graph_after}
+        else:
+            expected_process = {"process": process_graph_after}
+
+        assert mock_patch.last_request.json() == expected_process
+
+        # Check the other mocks were called too, just to be sure.
+        assert mock_get1.called
+        assert mock_get2.called
+
 
 class TestInternalCollectionMetadata:
 

tests/test_views.py

@@ -1214,7 +1214,7 @@ def service_metadata_wms_bar(self):
             # not setting "created": This is used to test creating a service.
         )
 
-    def test_service_types_simple(self, api, backend1, backend2, requests_mock):
+    def test_service_types_simple(self, api_tester, backend1, backend2, requests_mock):
         """Given 2 backends but only 1 backend has a single service, then the aggregator
             returns that 1 service's metadata.
         """
@@ -1242,10 +1242,10 @@ def test_service_types_simple(self, api, backend1, backend2, requests_mock):
         requests_mock.get(backend1 + "/service_types", json=single_service_type)
         requests_mock.get(backend2 + "/service_types", json=single_service_type)
 
-        resp = api.get('/service_types').assert_status_code(200)
+        resp = api_tester.get('/service_types').assert_status_code(200)
         assert resp.json == single_service_type
 
-    def test_service_types_merging(self, api, backend1, backend2, requests_mock):
+    def test_service_types_merging(self, api_tester, backend1, backend2, requests_mock):
         """Given 2 backends with each 1 service, then the aggregator lists both services."""
         service_type_1 = {
             "WMTS": {
@@ -1279,7 +1279,7 @@ def test_service_types_merging(self, api, backend1, backend2, requests_mock):
         requests_mock.get(backend1 + "/service_types", json=service_type_1)
         requests_mock.get(backend2 + "/service_types", json=service_type_2)
 
-        resp = api.get("/service_types").assert_status_code(200)
+        resp = api_tester.get("/service_types").assert_status_code(200)
         actual_service_types = resp.json
 
         expected_service_types = dict(service_type_1)
@@ -1353,15 +1353,15 @@ def test_service_info_api040(
         assert expected_service2.attributes == resp.json["attributes"]
 
     def test_service_info_wrong_id(
-        self, api, backend1, backend2, requests_mock, service_metadata_wmts_foo, service_metadata_wms_bar
+        self, api_tester, backend1, backend2, requests_mock, service_metadata_wmts_foo, service_metadata_wms_bar
     ):
         """When it gets a non-existent service ID, it returns HTTP Status 404, Not found."""
 
         requests_mock.get(backend1 + "/services/wmts-foo", json=service_metadata_wmts_foo.prepare_for_json())
         requests_mock.get(backend2 + "/services/wms-bar", json=service_metadata_wms_bar.prepare_for_json())
-        api.set_auth_bearer_token(token=TEST_USER_BEARER_TOKEN)
+        api_tester.set_auth_bearer_token(token=TEST_USER_BEARER_TOKEN)
 
-        api.get("/services/doesnotexist").assert_status_code(404)
+        api_tester.get("/services/doesnotexist").assert_status_code(404)
 
     def test_create_wmts_040(self, api040, requests_mock, backend1):
         api040.set_auth_bearer_token(TEST_USER_BEARER_TOKEN)