Use a distinct error for requests with invalid identifiers (#612)

* Use a distinct error for requests with invalid identifiers

This is more informative than 'not found', and as a bonus avoids
doing database roundtrips.

* Improve the error message if the id might be of another type

Author: PGijsbers

src/routers/resource_router.py

@@ -30,6 +30,7 @@
 from dependencies.pagination import Pagination, PaginationParams
 from error_handling import as_http_exception
 from database.model.ai_asset.distribution import Distribution
+from routers.helper_functions import get_asset_type_by_abbreviation
 from versioning import Version, VersionedResource
 
 from http import HTTPStatus
@@ -412,6 +413,7 @@ def get_resource(
             schema: self._possible_schemas_type = "aiod",  # type: ignore
             user: KeycloakUser | None = Depends(get_user_or_none),
         ):
+            self._raise_if_identifier_is_wrong_type(identifier)
             resource = self.get_resource(
                 identifier=identifier, schema=schema, user=user, platform=None
             )
@@ -420,6 +422,20 @@ def get_resource(
 
         return get_resource
 
+    def _raise_if_identifier_is_wrong_type(self, identifier: str):
+        if not identifier.startswith(self.resource_class.__abbreviation__):
+            hint = ""
+            if other_type := get_asset_type_by_abbreviation().get(identifier.split("_")[0]):
+                hint = f" Did you mean to request a {other_type.__tablename__!r} instead?"
+            raise HTTPException(
+                status_code=HTTPStatus.UNPROCESSABLE_ENTITY,
+                detail=(
+                    f"{identifier!r} is not a valid {self.resource_name} identifier, "
+                    f"valid {self.resource_name} identifiers start with "
+                    f"{self.resource_class.__abbreviation__!r}." + hint
+                ),
+            )
+
     def get_platform_resource_func(self):
         """
         Return a function that can be used to retrieve a single resource of a platform.
@@ -541,6 +557,7 @@ def put_resource(
             resource_create_instance: clz_create,  # type: ignore
             user: KeycloakUser = Depends(get_user_or_raise),
         ):
+            self._raise_if_identifier_is_wrong_type(identifier)
             with DbSession() as session:
                 try:
                     resource: Any = self._retrieve_resource(session, identifier)
@@ -592,6 +609,7 @@ def delete_resource(
             identifier: str,
             user: KeycloakUser = Depends(get_user_or_raise),
         ):
+            self._raise_if_identifier_is_wrong_type(identifier)
             with DbSession() as session:
                 try:
                     # Raise error if it does not exist
@@ -627,6 +645,7 @@ def submit_resource(
             submission: SubmissionCreateV2 | None = None,
             user: KeycloakUser = Depends(get_user_or_raise),
         ):
+            self._raise_if_identifier_is_wrong_type(identifier)
             with DbSession() as session:
                 resource = self._retrieve_resource(identifier=identifier, session=session)  # type: ignore
 

src/tests/routers/generic/test_router_delete.py

@@ -64,27 +64,26 @@ def test_delete_requires_admin(
         assert try_delete().status_code == HTTPStatus.OK
 
 
-@pytest.mark.parametrize("identifier", [3, 4])
 def test_non_existent(
     client_test_resource: TestClient,
-    identifier: int,
     mocked_privileged_token: Mock,
 ):
-    with DbSession() as session:
-        session.add_all(
-            [
-                factory_test_resource(title="my_test_resource", status=EntryStatus.DRAFT,
-                                      platform="example", platform_resource_identifier=1),
-                factory_test_resource(title="second_test_resource", status=EntryStatus.DRAFT,
-                                      platform="example", platform_resource_identifier=2),
-            ]
-        )
-        session.commit()
     response = client_test_resource.delete(
-        f"/test_resources/{identifier}", headers={"Authorization": "Fake token"}
+        f"/test_resources/test_44", headers={"Authorization": "Fake token"}
+    )
+    assert response.status_code == HTTPStatus.NOT_FOUND, response.json()
+    assert response.json()["detail"] == f"Test_resource 'test_44' not found in the database."
+
+
+def test_not_valid(
+        client_test_resource: TestClient,
+        mocked_privileged_token: Mock,
+):
+    response = client_test_resource.delete(
+        f"/test_resources/data_44", headers={"Authorization": "Fake token"}
     )
-    assert response.status_code == 404, response.json()
-    assert response.json()["detail"] == f"Test_resource '{identifier}' not found in the database."
+    assert response.status_code == HTTPStatus.UNPROCESSABLE_ENTITY, response.json()
+    assert "is not a valid" in response.json()["detail"]
 
 
 def test_add_after_deletion(

src/tests/routers/generic/test_router_get.py

@@ -19,9 +19,15 @@ def test_get_happy_path(client_test_resource: TestClient, engine_test_resource_f
 
 
 def test_not_found(client_test_resource: TestClient, engine_test_resource_filled: str):
-    response = client_test_resource.get("/test_resources/99")
+    response = client_test_resource.get("/test_resources/test_99")
     assert response.status_code == HTTPStatus.NOT_FOUND, response.json()
-    assert response.json()["detail"] == "Test_resource '99' not found in the database."
+    assert response.json()["detail"] == "Test_resource 'test_99' not found in the database."
+
+
+def test_not_valid(client_test_resource: TestClient, engine_test_resource_filled: str):
+    response = client_test_resource.get("/test_resources/99")
+    assert response.status_code == HTTPStatus.UNPROCESSABLE_ENTITY, response.json()
+    assert "is not a valid" in response.json()["detail"]
 
 
 def test_get_draft_unauthenticated_not_allowed(client_test_resource: TestClient):

src/tests/routers/generic/test_router_put.py

@@ -1,3 +1,4 @@
+from http import HTTPStatus
 from unittest.mock import Mock
 
 import pytest
@@ -37,13 +38,28 @@ def test_non_existent(
     mocked_privileged_token: Mock,
 ):
     response = client_test_resource.put(
-        "/test_resources/2",
+        "/test_resources/test_2",
         json={"title": "new_title", "platform": "other", "platform_resource_identifier": "2"},
         headers={"Authorization": "Fake token"},
     )
     assert response.status_code == 404, response.json()
     response_json = response.json()
-    assert response_json["detail"] == "Test_resource '2' not found in the database."
+    assert response_json["detail"] == "Test_resource 'test_2' not found in the database."
+
+
+def test_wrong_identifier_type(
+        client_test_resource: TestClient,
+        engine_test_resource_filled: Engine,
+        mocked_privileged_token: Mock,
+):
+    response = client_test_resource.put(
+        "/test_resources/data_2",
+        json={"title": "new_title", "platform": "other", "platform_resource_identifier": "2"},
+        headers={"Authorization": "Fake token"},
+    )
+    assert response.status_code == HTTPStatus.UNPROCESSABLE_ENTITY, response.json()
+    response_json = response.json()
+    assert "is not a valid" in response_json["detail"]
 
 
 def test_too_long_name(