diff --git a/docs/changelog/next_release/119.bugfix.rst b/docs/changelog/next_release/119.bugfix.rst new file mode 100644 index 00000000..8b871558 --- /dev/null +++ b/docs/changelog/next_release/119.bugfix.rst @@ -0,0 +1 @@ +Now `Queue.name` is made unique within `group_id` \ No newline at end of file diff --git a/syncmaster/db/migrations/versions/2023-11-23_0003_create_queue_table.py b/syncmaster/db/migrations/versions/2023-11-23_0003_create_queue_table.py index 77a0927c..8c3b7a12 100644 --- a/syncmaster/db/migrations/versions/2023-11-23_0003_create_queue_table.py +++ b/syncmaster/db/migrations/versions/2023-11-23_0003_create_queue_table.py @@ -21,8 +21,9 @@ def upgrade(): op.create_table( "queue", - sa.Column("name", sa.String(length=128), nullable=False), sa.Column("id", sa.BigInteger(), nullable=False), + sa.Column("name", sa.String(length=128), nullable=False), + sa.Column("slug", sa.String(length=256), nullable=False), sa.Column("group_id", sa.BigInteger(), nullable=False), sa.Column("description", sa.String(length=512), nullable=False), sa.Column("created_at", sa.DateTime(), server_default=sa.text("now()"), nullable=False), @@ -41,7 +42,7 @@ def upgrade(): ondelete="CASCADE", ), sa.PrimaryKeyConstraint("id", name=op.f("pk__queue")), - sa.UniqueConstraint("name", name=op.f("uq__queue__name")), + sa.UniqueConstraint("slug", name=op.f("uq__queue__slug")), ) op.create_index(op.f("ix__queue__group_id"), "queue", ["group_id"], unique=False) diff --git a/syncmaster/db/models/group.py b/syncmaster/db/models/group.py index 496e019f..bc5e4706 100644 --- a/syncmaster/db/models/group.py +++ b/syncmaster/db/models/group.py @@ -77,7 +77,7 @@ class Group(Base, TimestampMixin, DeletableMixin): owner: Mapped[User] = relationship(User) members: Mapped[list[User]] = relationship(User, secondary="user_group") - queue: Mapped[Queue] = relationship(back_populates="group") + queue: Mapped[Queue] = relationship(back_populates="group", cascade="all, delete-orphan") search_vector: Mapped[str] = mapped_column( TSVECTOR, diff --git a/syncmaster/db/models/queue.py b/syncmaster/db/models/queue.py index bb082b09..a71a841f 100644 --- a/syncmaster/db/models/queue.py +++ b/syncmaster/db/models/queue.py @@ -17,7 +17,8 @@ class Queue(Base, ResourceMixin, TimestampMixin, DeletableMixin): - name: Mapped[str] = mapped_column(String(128), nullable=False, unique=True) + name: Mapped[str] = mapped_column(String(128), nullable=False) + slug: Mapped[str] = mapped_column(String(256), nullable=False, unique=True) transfers: Mapped[list[Transfer]] = relationship(back_populates="queue") group: Mapped[Group] = relationship(back_populates="queue") @@ -31,4 +32,4 @@ class Queue(Base, ResourceMixin, TimestampMixin, DeletableMixin): ) def __repr__(self): - return f"" + return f"" diff --git a/syncmaster/db/repositories/queue.py b/syncmaster/db/repositories/queue.py index ea5edece..d6e961ef 100644 --- a/syncmaster/db/repositories/queue.py +++ b/syncmaster/db/repositories/queue.py @@ -197,7 +197,7 @@ async def get_resource_permission(self, user: User, resource_id: int) -> Permiss @staticmethod def _raise_error(err: DBAPIError) -> NoReturn: constraint = err.__cause__.__cause__.constraint_name - if constraint == "uq__queue__name": + if constraint == "uq__queue__slug": raise DuplicatedQueueNameError raise SyncmasterError from err diff --git a/syncmaster/schemas/v1/queue.py b/syncmaster/schemas/v1/queue.py index 7d5d4632..9ad99422 100644 --- a/syncmaster/schemas/v1/queue.py +++ b/syncmaster/schemas/v1/queue.py @@ -1,6 +1,6 @@ # SPDX-FileCopyrightText: 2023-2024 MTS PJSC # SPDX-License-Identifier: Apache-2.0 -from pydantic import BaseModel, Field, constr +from pydantic import BaseModel, Field, computed_field, constr from syncmaster.schemas.v1.page import PageSchema @@ -8,14 +8,20 @@ class CreateQueueSchema(BaseModel): name: constr(max_length=128, pattern=r"^[-_a-zA-Z0-9]+$") = Field( # noqa: F722 ..., - description="Queue name", + description="Queue name that allows letters, numbers, dashes, and underscores", ) group_id: int = Field(..., description="Queue owner group id") description: str = Field(default="", description="Additional description") + @computed_field + @property + def slug(self) -> str: + return f"{self.group_id}-{self.name}" + class ReadQueueSchema(BaseModel): name: str + slug: str description: str | None = None group_id: int id: int diff --git a/tests/test_unit/conftest.py b/tests/test_unit/conftest.py index b05d068b..94bc7b61 100644 --- a/tests/test_unit/conftest.py +++ b/tests/test_unit/conftest.py @@ -1,4 +1,5 @@ import secrets +from collections.abc import AsyncGenerator import pytest_asyncio from sqlalchemy.ext.asyncio import AsyncSession @@ -169,7 +170,7 @@ async def user_with_many_roles(session: AsyncSession, settings: Settings, simple @pytest_asyncio.fixture -async def empty_group(session: AsyncSession, settings) -> MockGroup: +async def empty_group(session: AsyncSession, settings) -> AsyncGenerator[MockGroup, None]: owner = await create_user( session=session, username="empty_group_owner", @@ -195,7 +196,7 @@ async def empty_group(session: AsyncSession, settings) -> MockGroup: @pytest_asyncio.fixture -async def group(session: AsyncSession, settings: Settings) -> MockGroup: +async def group(session: AsyncSession, settings: Settings) -> AsyncGenerator[MockGroup, None]: owner = await create_user( session=session, username="notempty_group_owner", @@ -239,7 +240,7 @@ async def group(session: AsyncSession, settings: Settings) -> MockGroup: async def mock_group( session: AsyncSession, settings: Settings, -): +) -> AsyncGenerator[MockGroup, None]: group_owner = await create_user( session=session, username=f"{secrets.token_hex(5)}_group_connection_owner", @@ -289,7 +290,7 @@ async def group_queue( session: AsyncSession, settings: Settings, mock_group: MockGroup, -) -> Queue: +) -> AsyncGenerator[Queue, None]: queue = await create_queue( session=session, name=f"{secrets.token_hex(5)}_test_queue", diff --git a/tests/test_unit/test_queue/test_create_queue.py b/tests/test_unit/test_queue/test_create_queue.py index 197330c8..c4abc690 100644 --- a/tests/test_unit/test_queue/test_create_queue.py +++ b/tests/test_unit/test_queue/test_create_queue.py @@ -35,6 +35,7 @@ async def test_maintainer_plus_can_create_queue( "name": "New_queue", "description": "Some interesting description", "group_id": mock_group.group.id, + "slug": f"{mock_group.group.id}-New_queue", } assert result.status_code == 200 queue = (await session.scalars(select(Queue).filter_by(id=result.json()["id"]))).one() @@ -43,6 +44,7 @@ async def test_maintainer_plus_can_create_queue( assert queue.group_id == mock_group.group.id assert queue.name == "New_queue" assert queue.description == "Some interesting description" + assert queue.slug == f"{mock_group.group.id}-New_queue" await session.delete(queue) await session.commit() @@ -71,6 +73,7 @@ async def test_superuser_can_create_queue( "name": "New_queue", "description": "Some interesting description", "group_id": mock_group.group.id, + "slug": f"{mock_group.group.id}-New_queue", } assert result.status_code == 200 queue = (await session.scalars(select(Queue).filter_by(id=result.json()["id"]))).one() @@ -79,6 +82,7 @@ async def test_superuser_can_create_queue( assert queue.group_id == mock_group.group.id assert queue.name == "New_queue" assert queue.description == "Some interesting description" + assert queue.slug == f"{mock_group.group.id}-New_queue" await session.delete(queue) await session.commit() @@ -395,3 +399,56 @@ async def test_maintainer_plus_can_not_create_queue_with_duplicate_name_error( "details": None, }, } + + +async def test_maintainer_plus_can_create_queues_with_the_same_name_but_diff_groups( + client: AsyncClient, + session: AsyncSession, + role_maintainer_plus: UserTestRoles, + mock_group: MockGroup, + group: MockGroup, +): + # Arrange + mock_group_user = mock_group.get_member_of_role(role_maintainer_plus) + group_user = group.get_member_of_role(role_maintainer_plus) + + # Act + queue_1 = await client.post( + "v1/queues", + headers={"Authorization": f"Bearer {mock_group_user.token}"}, + json={ + "name": "New_queue", + "description": "Some interesting description", + "group_id": mock_group.group.id, + }, + ) + queue_2 = await client.post( + "v1/queues", + headers={"Authorization": f"Bearer {group_user.token}"}, + json={ + "name": "New_queue", + "description": "Some interesting description", + "group_id": group.group.id, + }, + ) + queue_1_json = queue_1.json() + queue_2_json = queue_2.json() + + # Assert + assert queue_1.status_code == 200 + assert queue_1_json == { + "id": queue_1_json["id"], + "name": "New_queue", + "description": "Some interesting description", + "group_id": mock_group.group.id, + "slug": f"{mock_group.group.id}-New_queue", + } + assert queue_2.status_code == 200 + assert queue_2_json == { + "id": queue_2_json["id"], + "name": "New_queue", + "description": "Some interesting description", + "group_id": group.group.id, + "slug": f"{group.group.id}-New_queue", + } + assert queue_1_json["slug"] != queue_2_json["slug"] diff --git a/tests/test_unit/test_queue/test_read_queue.py b/tests/test_unit/test_queue/test_read_queue.py index 5b6e72cd..bf95e1d9 100644 --- a/tests/test_unit/test_queue/test_read_queue.py +++ b/tests/test_unit/test_queue/test_read_queue.py @@ -28,6 +28,7 @@ async def test_group_member_can_read_queue( "name": group_queue.name, "description": group_queue.description, "group_id": group_queue.group_id, + "slug": f"{group_queue.group.id}-{group_queue.name}", } assert result.status_code == 200 @@ -49,6 +50,7 @@ async def test_superuser_can_read_queue( "name": group_queue.name, "description": group_queue.description, "group_id": group_queue.group_id, + "slug": f"{group_queue.group.id}-{group_queue.name}", } assert result.status_code == 200 diff --git a/tests/test_unit/test_queue/test_read_queues.py b/tests/test_unit/test_queue/test_read_queues.py index b207ab92..1e066bfe 100644 --- a/tests/test_unit/test_queue/test_read_queues.py +++ b/tests/test_unit/test_queue/test_read_queues.py @@ -35,6 +35,7 @@ async def test_group_member_can_read_queues( "name": group_queue.name, "description": group_queue.description, "group_id": mock_group.id, + "slug": f"{mock_group.id}-{group_queue.name}", }, ], "meta": { @@ -73,6 +74,7 @@ async def test_superuser_can_read_queues( "name": group_queue.name, "description": group_queue.description, "group_id": mock_group.id, + "slug": f"{mock_group.id}-{group_queue.name}", }, ], "meta": { @@ -216,6 +218,7 @@ async def test_search_queues_with_query( "name": group_queue.name, "description": group_queue.description, "group_id": mock_group.id, + "slug": f"{mock_group.id}-{group_queue.name}", }, ], } diff --git a/tests/test_unit/test_queue/test_update_queue.py b/tests/test_unit/test_queue/test_update_queue.py index c07d1c70..2a00b834 100644 --- a/tests/test_unit/test_queue/test_update_queue.py +++ b/tests/test_unit/test_queue/test_update_queue.py @@ -33,6 +33,7 @@ async def test_maintainer_plus_can_update_queue( "name": group_queue.name, "description": "New description", "group_id": group_queue.group_id, + "slug": f"{group_queue.group_id}-{group_queue.name}", } assert result.status_code == 200 @@ -44,6 +45,7 @@ async def test_maintainer_plus_can_update_queue( "name": queue.name, "description": queue.description, "group_id": queue.group_id, + "slug": f"{queue.group_id}-{queue.name}", } @@ -70,6 +72,7 @@ async def test_superuser_can_update_queue( "name": group_queue.name, "description": "New description", "group_id": group_queue.group_id, + "slug": f"{group_queue.group_id}-{group_queue.name}", } queue = await session.get(Queue, group_queue.id) @@ -80,6 +83,7 @@ async def test_superuser_can_update_queue( "name": queue.name, "description": queue.description, "group_id": queue.group_id, + "slug": f"{queue.group_id}-{queue.name}", } diff --git a/tests/test_unit/utils.py b/tests/test_unit/utils.py index 2681eb5c..b3f8fde2 100644 --- a/tests/test_unit/utils.py +++ b/tests/test_unit/utils.py @@ -77,6 +77,7 @@ async def create_queue( name=name, description=description, group_id=group_id, + slug=f"{group_id}-{name}", ) session.add(queue) await session.commit()