update custom field definition uniqueness criteria

CHANGELOG.md

@@ -26,6 +26,7 @@ The types of changes are:
 ### Changed
 
 - Merge instances of RTK `createApi` into one instance for better cache invalidation [#3059](https://github.com/ethyca/fides/pull/3059)
+- Update custom field definition uniqueness to be case insensitive name per resource type [#3215](https://github.com/ethyca/fides/pull/3215)
 - Restrict where privacy notices of certain consent mechanisms must be displayed [#3195](https://github.com/ethyca/fides/pull/3195)
 
 ## [2.12.1](https://github.com/ethyca/fides/compare/2.12.0...2.12.1)

src/fides/api/ctl/database/session.py

@@ -2,9 +2,10 @@
 
 from sqlalchemy import create_engine
 from sqlalchemy.ext.asyncio import AsyncSession, create_async_engine
-from sqlalchemy.orm import Session, sessionmaker
+from sqlalchemy.orm import sessionmaker
 
 from fides.core.config import CONFIG
+from fides.lib.db.session import ExtendedSession
 
 engine = create_async_engine(
     CONFIG.database.async_database_uri,
@@ -15,7 +16,7 @@
 sync_engine = create_engine(CONFIG.database.sync_database_uri, echo=False)
 sync_session = sessionmaker(
     sync_engine,
-    class_=Session,
+    class_=ExtendedSession,
     expire_on_commit=False,
     autocommit=False,
 )

src/fides/api/ctl/migrations/versions/15a3e7483249_make_customfielddefinition_uniqueness_.py

@@ -0,0 +1,53 @@
+"""make customfielddefinition uniqueness case insensitive
+
+Revision ID: 15a3e7483249
+Revises: e92da354691e
+Create Date: 2023-05-02 15:03:56.256982
+
+"""
+import sqlalchemy as sa
+from alembic import op
+from sqlalchemy.exc import IntegrityError
+
+# revision identifiers, used by Alembic.
+revision = "15a3e7483249"
+down_revision = "e92da354691e"
+branch_labels = None
+depends_on = None
+
+
+def upgrade():
+
+    try:
+        op.execute(
+            """ CREATE UNIQUE INDEX ix_plus_custom_field_definition_unique_lowername_resourcetype
+                ON plus_custom_field_definition
+                (lower(name), resource_type)
+            """
+        )
+    except IntegrityError as exc:
+        raise Exception(
+            f"Fides attempted to create new custom field definition unique index but got error: {exc}. "
+            f"Adjust custom field names to avoid case-insensitive name overlaps for the same resource type."
+        )
+
+    # remove unnecessary index of unused field
+    op.drop_index(
+        "ix_plus_custom_field_definition_field_definition",
+        table_name="plus_custom_field_definition",
+    )
+
+
+def downgrade():
+    op.drop_index(
+        op.f("ix_plus_custom_field_definition_unique_lowername_resourcetype"),
+        table_name="plus_custom_field_definition",
+    )
+
+    # re-add unnecessray index of unused field in downgrade, for consistency
+    op.create_index(
+        op.f("ix_plus_custom_field_definition_field_definition"),
+        "plus_custom_field_definition",
+        ["field_definition"],
+        unique=False,
+    )

src/fides/api/ctl/sql_models.py

@@ -16,6 +16,7 @@
 from sqlalchemy import Enum as EnumColumn
 from sqlalchemy import (
     ForeignKey,
+    Index,
     Integer,
     String,
     Text,
@@ -25,6 +26,7 @@
     type_coerce,
 )
 from sqlalchemy.dialects.postgresql import BYTEA
+from sqlalchemy.exc import IntegrityError
 from sqlalchemy.orm import Session, relationship
 from sqlalchemy.sql import func
 from sqlalchemy.sql.sqltypes import DateTime
@@ -37,6 +39,7 @@
     FidesUserPermissions,
 )
 from fides.lib.db.base_class import FidesBase as FideslibBase
+from fides.lib.exceptions import KeyOrNameAlreadyExists
 
 
 class FidesBase(FideslibBase):
@@ -514,7 +517,50 @@ class CustomFieldDefinition(Base):
     )
     active = Column(BOOLEAN, nullable=False, default=True)
 
-    UniqueConstraint("name", "resource_type")
+    @classmethod
+    def create(
+        cls: Type[PrivacyDeclaration],
+        db: Session,
+        *,
+        data: dict[str, Any],
+        check_name: bool = False,  # this is the reason for the override
+    ) -> PrivacyDeclaration:
+        """
+        Overrides base create to avoid unique check on `name` column
+        and to cleanly handle uniqueness constraint on name/resource_type
+        """
+        try:
+            return super().create(db=db, data=data, check_name=check_name)
+        except IntegrityError as e:
+            if cls.name_resource_index in str(e):
+                raise KeyOrNameAlreadyExists(
+                    "Custom field definitions must have unique names for a given resource type"
+                )
+            raise e
+
+    def update(self, db: Session, *, data: Dict[str, Any]) -> FidesBase:
+        """Overrides base update to cleanly handle uniqueness constraint on name/resource type"""
+        try:
+            return super().update(db=db, data=data)
+        except IntegrityError as e:
+            if CustomFieldDefinition.name_resource_index in str(e):
+                raise KeyOrNameAlreadyExists(
+                    "Custom field definitions must have unique names for a given resource type"
+                )
+            raise e
+
+    # unique index on the lowername/resource type for case-insensitive name checking per resource type
+    name_resource_index = (
+        "ix_plus_custom_field_definition_unique_lowername_resourcetype"
+    )
+    __table_args__ = (
+        Index(
+            name_resource_index,
+            resource_type,
+            func.lower(name),
+            unique=True,
+        ),
+    )
 
 
 class CustomField(Base):

tests/ctl/core/test_custom_field_models.py

@@ -0,0 +1,144 @@
+# pylint: disable=missing-docstring, redefined-outer-name
+
+import pytest
+from sqlalchemy.exc import IntegrityError
+
+from fides.api.ctl.sql_models import CustomFieldDefinition
+from fides.lib.exceptions import KeyOrNameAlreadyExists
+
+
+@pytest.fixture(autouse=True)
+def clear_custom_metadata_resources(db):
+    """
+    Fixture run on each test to clear custom field DB tables,
+    to ensure each test runs with a clean slate.
+    """
+
+    def delete_data(tables):
+        for table in tables:
+            table.query(db).delete()
+        db.commit()
+
+    tables = [
+        CustomFieldDefinition,
+    ]
+
+    delete_data(tables)
+
+    yield
+    delete_data(tables)
+
+
+def test_custom_field_definition_duplicate_name_rejected_create(db):
+    """Assert case-insensitive unique checks on name/resource type upon creation"""
+    definition1 = CustomFieldDefinition.create(
+        db=db,
+        data={
+            "name": "test1",
+            "description": "test",
+            "field_type": "string",
+            "resource_type": "system",
+            "field_definition": "string",
+        },
+    )
+    with pytest.raises(KeyOrNameAlreadyExists):
+        CustomFieldDefinition.create(
+            db=db,
+            data={
+                "name": "Test1",
+                "description": "test",
+                "field_type": "string",
+                "resource_type": "system",
+                "field_definition": "string",
+            },
+        )
+
+    # assert the second record didn't get created
+    assert len(CustomFieldDefinition.all(db)) == 1
+
+    # with a different resource type, we should allow creation
+    definition2 = CustomFieldDefinition.create(
+        db=db,
+        data={
+            "name": "Test1",
+            "description": "test",
+            "field_type": "string",
+            "resource_type": "privacy_declaration",
+            "field_definition": "string",
+        },
+    )
+
+    # assert we've got two different records created successfully
+    assert len(CustomFieldDefinition.all(db)) == 2
+    assert definition1.id != definition2.id
+
+
+def test_custom_field_definition_duplicate_name_different_resource_type_accepted(db):
+    """Assert that we can create custom field definition with same name on a different resource type"""
+    definition1 = CustomFieldDefinition.create(
+        db=db,
+        data={
+            "name": "test1",
+            "description": "test",
+            "field_type": "string",
+            "resource_type": "system",
+            "field_definition": "string",
+        },
+    )
+
+    definition2 = CustomFieldDefinition.create(
+        db=db,
+        data={
+            "name": "test1",
+            "description": "test",
+            "field_type": "string",
+            "resource_type": "privacy_declaration",  # different resource type, so this should be allowed
+            "field_definition": "string",
+        },
+    )
+
+    # assert we've got two different records created successfully
+    assert len(CustomFieldDefinition.all(db)) == 2
+    assert definition1.id != definition2.id
+
+
+def test_custom_field_definition_duplicate_name_rejected_update(db):
+    """Assert case-insensitive unique checks on name/resource type upon update"""
+
+    definition1 = CustomFieldDefinition.create(
+        db=db,
+        data={
+            "name": "test1",
+            "description": "test",
+            "field_type": "string",
+            "resource_type": "system",
+            "field_definition": "string",
+        },
+    )
+
+    definition2 = CustomFieldDefinition.create(
+        db=db,
+        data={
+            "name": "Test 1",  # space in name should allow creation, considered unique name
+            "description": "test",
+            "field_type": "string",
+            "resource_type": "system",
+            "field_definition": "string",
+        },
+    )
+
+    # assert we've got two different records created successfully
+    assert len(CustomFieldDefinition.all(db)) == 2
+    assert definition1.id != definition2.id
+
+    with pytest.raises(KeyOrNameAlreadyExists) as e:
+        definition2 = definition2.update(
+            db=db,
+            data={
+                "name": "Test1",  # if we try to update name to remove space, we should hit uniqueness error
+            },
+        )
+
+    # assert update did not go through
+    db.refresh(definition2)
+    assert definition2.name == "Test 1"