Skip to content
This repository has been archived by the owner on Jun 14, 2024. It is now read-only.

feat: add uniqueness field validation for span questions #60

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/argilla_server/apis/v1/handlers/datasets/questions.py
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,9 @@ async def create_dataset_question(
question_create: QuestionCreate,
current_user: User = Security(auth.get_current_user),
):
dataset = await _get_dataset_or_raise(db, dataset_id, with_fields=True)
# TODO: Review this flow since we're putting logic here that will be used internally by the context
# Fields and questions are required to apply validations.
dataset = await _get_dataset_or_raise(db, dataset_id, with_fields=True, with_questions=True)

await authorize(current_user, DatasetPolicyV1.create_question(dataset))

Expand Down
4 changes: 4 additions & 0 deletions src/argilla_server/contexts/questions.py
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,10 @@ def _validate_span_question_settings_before_create(
if field not in field_names:
raise ValueError(f"'{field}' is not a valid field name.\nValid field names are {field_names!r}")

for question in dataset.questions:
if question.type == QuestionType.span and field == question.parsed_settings.field:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I already added in one commit a property to Question database model to check if if span or not. But after some changes I removed it because I was not using it.

Take a look to the commit here: 799743b#diff-b7efe64daa88a5d7c98088e2a66d38739605226e60d4ed967712ca2790c54931

If you want to apply that change then you can do:

Suggested change
if question.type == QuestionType.span and field == question.parsed_settings.field:
if question.is_span and field == question.parsed_settings.field:

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer to keep it as is. We already have other places using this kind of condition with settings directly and it can be confusing seeing is_span or settings.type == QuestionType.span depending on the context.

We can review it later when using question validators. However, migrating the project to Python 3.10 and using the pattern-matching switch would be a better option.

raise ValueError(f"'{field}' is already used by span question with id '{question.id}'")


async def create_question(db: AsyncSession, dataset: Dataset, question_create: QuestionCreate) -> Question:
if dataset.is_ready:
Expand Down
31 changes: 30 additions & 1 deletion tests/unit/api/v1/datasets/test_create_dataset_question.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
from sqlalchemy import func, select
from sqlalchemy.ext.asyncio import AsyncSession

from tests.factories import DatasetFactory, TextFieldFactory
from tests.factories import DatasetFactory, SpanQuestionFactory, TextFieldFactory


@pytest.mark.asyncio
Expand Down Expand Up @@ -109,3 +109,32 @@ async def test_create_dataset_span_question_with_non_existent_field(
}

assert (await db.execute(select(func.count(Question.id)))).scalar() == 0

async def test_create_dataset_question_with_other_span_question_using_the_same_field(
self, async_client: AsyncClient, db: AsyncSession, owner_auth_header: dict
):
dataset = await DatasetFactory.create()
question = await SpanQuestionFactory(dataset=dataset)

question_field = question.settings["field"]
await TextFieldFactory.create(name=question_field, dataset=dataset)

response = await async_client.post(
self.url(dataset.id),
headers=owner_auth_header,
json={
"name": "new-question",
"title": "Title",
"settings": {
"type": QuestionType.span,
"field": question_field,
"options": [
{"value": "label-c", "text": "Label C", "description": "Label C description"},
{"value": "label-d", "text": "Label D", "description": "Label D description"},
],
},
},
)

assert response.status_code == 422
assert response.json() == {"detail": f"'field-a' is already used by span question with id '{question.id}'"}
Loading