feat: add new endpoint to delete metadata properties

CHANGELOG.md

@@ -26,6 +26,7 @@ These are the section headers that we use:
 - Added a validation layer for both `metadata_properties` and `metadata_filters` in their schemas and as part of the `add_records` and `filter_by` methods, respectively ([#3860](https://github.com/argilla-io/argilla/pull/3860)).
 - Added `sort_by` query parameter to listing records endpoints that allows to sort the records by `inserted_at`, `updated_at` or metadata property ([#3843](https://github.com/argilla-io/argilla/pull/3843)).
 - Added fields `inserted_at` and `updated_at` in `RemoteResponseSchema` ([#3822](https://github.com/argilla-io/argilla/pull/3822)).
+- New `DELETE /api/v1/metadata-properties/:metadata_property_id` endpoint allowing the deletion of a specific metadata property. ([#3911](https://github.com/argilla-io/argilla/pull/3911)).
 
 ### Changed
 

src/argilla/server/apis/v1/handlers/metadata_properties.py

@@ -21,7 +21,7 @@
 from argilla.server.database import get_async_db
 from argilla.server.models import MetadataProperty, User
 from argilla.server.policies import MetadataPropertyPolicyV1, authorize
-from argilla.server.schemas.v1.metadata_properties import MetadataMetrics
+from argilla.server.schemas.v1.metadata_properties import MetadataMetrics, MetadataProperty
 from argilla.server.search_engine import SearchEngine, get_search_engine
 from argilla.server.security import auth
 
@@ -35,6 +35,7 @@ async def _get_metadata_property(db: "AsyncSession", metadata_property_id: UUID)
             status_code=status.HTTP_404_NOT_FOUND,
             detail=f"Metadata property with id `{metadata_property_id}` not found",
         )
+
     return metadata_property
 
 
@@ -51,3 +52,24 @@ async def get_metadata_property_metrics(
     await authorize(current_user, MetadataPropertyPolicyV1.compute_metrics(metadata_property))
 
     return await search_engine.compute_metrics_for(metadata_property)
+
+
+@router.delete("/metadata-properties/{metadata_property_id}", response_model=MetadataProperty)
+async def delete_metadata_property(
+    *,
+    db: AsyncSession = Depends(get_async_db),
+    metadata_property_id: UUID,
+    current_user: User = Security(auth.get_current_user),
+):
+    metadata_property = await _get_metadata_property(db, metadata_property_id)
+
+    await authorize(current_user, MetadataPropertyPolicyV1.delete(metadata_property))
+
+    # TODO: We should split API v1 into different FastAPI apps so we can customize error management.
+    # After mapping ValueError to 422 errors for API v1 then we can remove this try except.
+    try:
+        await datasets.delete_metadata_property(db, metadata_property)
+    except ValueError as err:
+        raise HTTPException(status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, detail=str(err))
+
+    return metadata_property

src/argilla/server/contexts/datasets.py

@@ -197,13 +197,21 @@ async def get_question_by_name_and_dataset_id(db: "AsyncSession", name: str, dat
     return result.scalar_one_or_none()
 
 
+async def get_metadata_property_by_id(db: "AsyncSession", metadata_property_id: UUID) -> Union[MetadataProperty, None]:
+    return await MetadataProperty.read(db, id=metadata_property_id)
+
+
 async def get_metadata_property_by_name_and_dataset_id(
     db: "AsyncSession", name: str, dataset_id: UUID
 ) -> Union[MetadataProperty, None]:
     result = await db.execute(select(MetadataProperty).filter_by(name=name, dataset_id=dataset_id))
     return result.scalar_one_or_none()
 
 
+async def delete_metadata_property(db: "AsyncSession", metadata_property: MetadataProperty) -> MetadataProperty:
+    return await metadata_property.delete(db)
+
+
 async def create_question(db: "AsyncSession", dataset: Dataset, question_create: QuestionCreate) -> Question:
     if dataset.is_ready:
         raise ValueError("Question cannot be created for a published dataset")

src/argilla/server/policies.py

@@ -368,6 +368,34 @@ async def is_allowed(actor: User) -> bool:
         return is_allowed
 
 
+class MetadataPropertyPolicyV1:
+    @classmethod
+    def compute_metrics(cls, metadata_property: MetadataProperty) -> PolicyAction:
+        async def is_allowed(actor: User) -> bool:
+            if actor.is_owner:
+                return True
+
+            exists_workspace_user = await _exists_workspace_user_by_user_and_workspace_id(
+                actor, metadata_property.dataset.workspace_id
+            )
+
+            return (actor.is_admin and exists_workspace_user and metadata_property.is_visible) or (
+                actor.is_annotator and exists_workspace_user and metadata_property.is_visible_for_annotators
+            )
+
+        return is_allowed
+
+    @classmethod
+    def delete(cls, metadata_property: MetadataProperty) -> PolicyAction:
+        async def is_allowed(actor: User) -> bool:
+            return actor.is_owner or (
+                actor.is_admin
+                and await _exists_workspace_user_by_user_and_workspace_id(actor, metadata_property.dataset.workspace_id)
+            )
+
+        return is_allowed
+
+
 class RecordPolicyV1:
     @classmethod
     def delete(cls, record: Record) -> PolicyAction:
@@ -464,24 +492,6 @@ async def is_allowed(actor: User) -> bool:
         return is_allowed
 
 
-class MetadataPropertyPolicyV1:
-    @classmethod
-    def compute_metrics(cls, metadata_property: MetadataProperty) -> PolicyAction:
-        async def is_allowed(actor: User) -> bool:
-            if actor.is_owner:
-                return True
-
-            exists_workspace_user = await _exists_workspace_user_by_user_and_workspace_id(
-                actor, metadata_property.dataset.workspace_id
-            )
-
-            return (actor.is_admin and exists_workspace_user and metadata_property.is_visible) or (
-                actor.is_annotator and exists_workspace_user and metadata_property.is_visible_for_annotators
-            )
-
-        return is_allowed
-
-
 class DatasetSettingsPolicy:
     @classmethod
     def list(cls, dataset: DatasetDB) -> PolicyAction:

src/argilla/server/routes.py

@@ -68,9 +68,9 @@
 api_router.include_router(datasets_v1.router, prefix="/v1")
 api_router.include_router(fields_v1.router, prefix="/v1")
 api_router.include_router(questions_v1.router, prefix="/v1")
+api_router.include_router(metadata_properties_v1.router, prefix="/v1")
 api_router.include_router(records_v1.router, prefix="/v1")
 api_router.include_router(responses_v1.router, prefix="/v1")
 api_router.include_router(users_v1.router, prefix="/v1")
 api_router.include_router(workspaces_v1.router, prefix="/v1")
 api_router.include_router(suggestions_v1.router, prefix="/v1")
-api_router.include_router(metadata_properties_v1.router, prefix="/v1")

src/argilla/server/schemas/v1/metadata_properties.py

@@ -12,12 +12,15 @@
 #  See the License for the specific language governing permissions and
 #  limitations under the License.
 
+from datetime import datetime
 from typing import Generic, List, Literal, Optional, TypeVar, Union
+from uuid import UUID
 
 from pydantic import BaseModel, Field, validator
 from pydantic.generics import GenericModel
 
 from argilla.server.enums import MetadataPropertyType
+from argilla.server.schemas.v1.datasets import MetadataPropertySettings
 
 FLOAT_METADATA_METRICS_PRECISION = 5
 
@@ -62,3 +65,16 @@ def round_result(cls, v: float):
 MetadataMetrics = Annotated[
     Union[TermsMetadataMetrics, IntegerMetadataMetrics, FloatMetadataMetrics], Field(..., discriminator="type")
 ]
+
+
+class MetadataProperty(BaseModel):
+    id: UUID
+    name: str
+    description: Optional[str] = None
+    settings: MetadataPropertySettings
+    dataset_id: UUID
+    inserted_at: datetime
+    updated_at: datetime
+
+    class Config:
+        orm_mode = True

tests/factories.py

@@ -243,13 +243,15 @@ class MetadataPropertyFactory(BaseFactory):
     class Meta:
         model = MetadataProperty
 
+    # TODO: Remove this method and fix possible failing tests
     @classmethod
     def _create(cls, model_class, *args, **kwargs):
         default_settings = getattr(cls, "settings", {})
         settings = kwargs.get("settings", {})
         if settings:
-            default_settings.update(settings)
-            kwargs["settings"] = default_settings
+            new_settings = default_settings.copy()
+            new_settings.update(settings)
+            kwargs["settings"] = new_settings
         return super()._create(model_class, *args, **kwargs)
 
     name = factory.Sequence(lambda n: f"metadata-property-{n}")
@@ -276,13 +278,15 @@ class QuestionFactory(BaseFactory):
     class Meta:
         model = Question
 
+    # TODO: Remove this method and fix possible failing tests
     @classmethod
     def _create(cls, model_class, *args, **kwargs):
         default_settings = cls.settings.copy()
         settings = kwargs.get("settings", {})
         if settings:
-            default_settings.update(settings)
-            kwargs["settings"] = default_settings
+            new_settings = default_settings.copy()
+            new_settings.update(settings)
+            kwargs["settings"] = new_settings
         return super()._create(model_class, *args, **kwargs)
 
     name = factory.Sequence(lambda n: f"question-{n}")

tests/unit/server/api/v1/test_metadata_properties.py

@@ -14,14 +14,17 @@
 
 import uuid
 from typing import TYPE_CHECKING, Type
+from uuid import uuid4
 
 import pytest
 from argilla._constants import API_KEY_HEADER_NAME
-from argilla.server.enums import UserRole
+from argilla.server.enums import MetadataPropertyType, UserRole
+from argilla.server.models import MetadataProperty, UserRole
 from argilla.server.search_engine import FloatMetadataMetrics, IntegerMetadataMetrics, TermsMetadataMetrics
+from sqlalchemy import func, select
 
 from tests.factories import (
-    AdminFactory,
+    AnnotatorFactory,
     BaseFactory,
     FloatMetadataPropertyFactory,
     IntegerMetadataPropertyFactory,
@@ -130,3 +133,81 @@ async def test_compute_metrics_for_unauthorized_user(
             f"/api/v1/metadata-properties/{metadata_property.id}/metrics", headers={API_KEY_HEADER_NAME: user.api_key}
         )
         assert response.status_code == 403
+
+
+@pytest.mark.parametrize("user_role", [UserRole.owner, UserRole.admin])
+@pytest.mark.asyncio
+async def test_delete_metadata_property(async_client: "AsyncClient", db: "AsyncSession", user_role: UserRole):
+    metadata_property = await IntegerMetadataPropertyFactory.create(
+        name="name",
+        description="description",
+    )
+    user = await UserFactory.create(role=user_role, workspaces=[metadata_property.dataset.workspace])
+
+    response = await async_client.delete(
+        f"/api/v1/metadata-properties/{metadata_property.id}", headers={API_KEY_HEADER_NAME: user.api_key}
+    )
+
+    assert response.status_code == 200
+    assert (await db.execute(select(func.count(MetadataProperty.id)))).scalar() == 0
+
+    response_body = response.json()
+    assert response_body == {
+        "id": str(metadata_property.id),
+        "name": "name",
+        "description": "description",
+        "settings": {"type": MetadataPropertyType.integer, "min": None, "max": None},
+        "dataset_id": str(metadata_property.dataset_id),
+        "inserted_at": metadata_property.inserted_at.isoformat(),
+        "updated_at": metadata_property.updated_at.isoformat(),
+    }
+
+
+@pytest.mark.asyncio
+async def test_delete_metadata_property_without_authentication(async_client: "AsyncClient", db: "AsyncSession"):
+    metadata_property = await IntegerMetadataPropertyFactory.create()
+
+    response = await async_client.delete(f"/api/v1/metadata-properties/{metadata_property.id}")
+
+    assert response.status_code == 401
+    assert (await db.execute(select(func.count(MetadataProperty.id)))).scalar() == 1
+
+
+@pytest.mark.asyncio
+async def test_delete_metadata_property_as_admin_from_different_workspace(
+    async_client: "AsyncClient", db: "AsyncSession"
+):
+    admin = await UserFactory.create(role=UserRole.admin)
+    metadata_property = await IntegerMetadataPropertyFactory.create()
+
+    response = await async_client.delete(
+        f"/api/v1/metadata-properties/{metadata_property.id}", headers={API_KEY_HEADER_NAME: admin.api_key}
+    )
+
+    assert response.status_code == 403
+    assert (await db.execute(select(func.count(MetadataProperty.id)))).scalar() == 1
+
+
+@pytest.mark.asyncio
+async def test_delete_metadata_property_as_annotator(async_client: "AsyncClient", db: "AsyncSession"):
+    annotator = await AnnotatorFactory.create()
+    metadata_property = await IntegerMetadataPropertyFactory.create()
+
+    response = await async_client.delete(
+        f"/api/v1/metadata-properties/{metadata_property.id}", headers={API_KEY_HEADER_NAME: annotator.api_key}
+    )
+
+    assert response.status_code == 403
+    assert (await db.execute(select(func.count(MetadataProperty.id)))).scalar() == 1
+
+
+@pytest.mark.asyncio
+async def test_delete_metadata_property_with_nonexistent_metadata_property_id(
+    async_client: "AsyncClient", db: "AsyncSession", owner_auth_header: dict
+):
+    await IntegerMetadataPropertyFactory.create()
+
+    response = await async_client.delete(f"/api/v1/metadata-properties/{uuid4()}", headers=owner_auth_header)
+
+    assert response.status_code == 404
+    assert (await db.execute(select(func.count(MetadataProperty.id)))).scalar() == 1

tests/unit/server/api/v1/test_questions.py

@@ -244,7 +244,7 @@ async def test_update_question_as_annotator(async_client: "AsyncClient"):
     assert response.status_code == 403
 
 
-@pytest.mark.parametrize("role", [UserRole.admin, UserRole.owner])
+@pytest.mark.parametrize("role", [UserRole.owner, UserRole.admin])
 @pytest.mark.asyncio
 async def test_delete_question(async_client: "AsyncClient", db: "AsyncSession", role: UserRole):
     question = await TextQuestionFactory.create(name="name", title="title", description="description")