Skip to content

Commit

Permalink
[Refactor] remove name validations for dataset workspaces and usernam…
Browse files Browse the repository at this point in the history
…es (#5575)

# Description
<!-- Please include a summary of the changes and the related issue.
Please also include relevant motivation and context. List any
dependencies that are required for this change. -->

This PR removes name pattern validations for datasets, workspaces, and
users.

**Type of change**
<!-- Please delete options that are not relevant. Remember to title the
PR according to the type of change -->

- Refactor (change restructuring the codebase without changing
functionality)
- Improvement (change adding some improvement to an existing
functionality)

**How Has This Been Tested**
<!-- Please add some reference about how your feature has been tested.
-->

**Checklist**
<!-- Please go over the list and make sure you've taken everything into
account -->

- I added relevant documentation
- I followed the style guidelines of this project
- I did a self-review of my code
- I made corresponding changes to the documentation
- I confirm My changes generate no new warnings
- I have added tests that prove my fix is effective or that my feature
works
- I have added relevant notes to the CHANGELOG.md file (See
https://keepachangelog.com/)
  • Loading branch information
frascuchon authored Oct 7, 2024
1 parent 83996fe commit 485f3ff
Show file tree
Hide file tree
Showing 12 changed files with 15 additions and 126 deletions.
4 changes: 4 additions & 0 deletions argilla-server/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,10 @@ These are the section headers that we use:

## [Unreleased]()

### Removed

- Removed name pattern validation for Workspaces, Datasets, and Users. ([#5575](https://github.com/argilla-io/argilla/pull/5575))

## [2.3.0](https://github.com/argilla-io/argilla/compare/v2.2.0...v2.3.0)

### Added
Expand Down
6 changes: 4 additions & 2 deletions argilla-server/src/argilla_server/api/schemas/v1/datasets.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,14 +25,16 @@
except ImportError:
from typing_extensions import Annotated

DATASET_NAME_REGEX = r"^(?!-|_)[a-zA-Z0-9-_ ]+$"
DATASET_NAME_MIN_LENGTH = 1
DATASET_NAME_MAX_LENGTH = 200
DATASET_GUIDELINES_MIN_LENGTH = 1
DATASET_GUIDELINES_MAX_LENGTH = 10000

DatasetName = Annotated[
constr(regex=DATASET_NAME_REGEX, min_length=DATASET_NAME_MIN_LENGTH, max_length=DATASET_NAME_MAX_LENGTH),
constr(
min_length=DATASET_NAME_MIN_LENGTH,
max_length=DATASET_NAME_MAX_LENGTH,
),
Field(..., description="Dataset name"),
]

Expand Down
3 changes: 1 addition & 2 deletions argilla-server/src/argilla_server/api/schemas/v1/users.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
from argilla_server.enums import UserRole
from argilla_server.pydantic_v1 import BaseModel, Field, constr

USER_USERNAME_REGEX = "^(?!-|_)[A-za-z0-9-_]+$"
USER_PASSWORD_MIN_LENGTH = 8
USER_PASSWORD_MAX_LENGTH = 100

Expand All @@ -43,7 +42,7 @@ class Config:
class UserCreate(BaseModel):
first_name: constr(min_length=1, strip_whitespace=True)
last_name: Optional[constr(min_length=1, strip_whitespace=True)]
username: str = Field(regex=USER_USERNAME_REGEX, min_length=1)
username: str = Field(..., min_length=1)
role: Optional[UserRole]
password: str = Field(min_length=USER_PASSWORD_MIN_LENGTH, max_length=USER_PASSWORD_MAX_LENGTH)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,8 @@
from typing import List
from uuid import UUID

from argilla_server.constants import ES_INDEX_REGEX_PATTERN
from argilla_server.pydantic_v1 import BaseModel, Field

WORKSPACE_NAME_REGEX = ES_INDEX_REGEX_PATTERN


class Workspace(BaseModel):
id: UUID
Expand All @@ -33,7 +30,7 @@ class Config:


class WorkspaceCreate(BaseModel):
name: str = Field(regex=WORKSPACE_NAME_REGEX, min_length=1)
name: str = Field(min_length=1)


class Workspaces(BaseModel):
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,6 @@
import typer
import yaml

from argilla_server.api.schemas.v1.users import USER_USERNAME_REGEX
from argilla_server.api.schemas.v1.workspaces import WORKSPACE_NAME_REGEX
from argilla_server.database import AsyncSessionLocal
from argilla_server.models import User, UserRole
from argilla_server.pydantic_v1 import BaseModel, Field, constr
Expand All @@ -31,12 +29,12 @@


class WorkspaceCreate(BaseModel):
name: str = Field(..., regex=WORKSPACE_NAME_REGEX, min_length=1)
name: str = Field(..., min_length=1)


class UserCreate(BaseModel):
first_name: constr(strip_whitespace=True)
username: str = Field(..., regex=USER_USERNAME_REGEX, min_length=1)
username: str = Field(..., min_length=1)
role: UserRole
api_key: constr(min_length=1)
password_hash: constr(min_length=1)
Expand Down
2 changes: 0 additions & 2 deletions argilla-server/src/argilla_server/constants.py
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,4 @@
# The metadata field name prefix defined for protected (non-searchable) values
PROTECTED_METADATA_FIELD_PREFIX = "_"

ES_INDEX_REGEX_PATTERN = r"^(?!-|_)[a-z0-9-_]+$"

JS_MAX_SAFE_INTEGER = 9007199254740991
8 changes: 0 additions & 8 deletions argilla-server/tests/unit/api/handlers/v1/test_datasets.py
Original file line number Diff line number Diff line change
Expand Up @@ -916,10 +916,6 @@ async def test_create_dataset_with_invalid_length_guidelines(
"dataset_json",
[
{"name": ""},
{"name": "123$abc"},
{"name": "unit@test"},
{"name": "-test-dataset"},
{"name": "_test-dataset"},
{"name": "a" * (DATASET_NAME_MAX_LENGTH + 1)},
{"name": "test-dataset", "guidelines": ""},
{"name": "test-dataset", "guidelines": "a" * (DATASET_GUIDELINES_MAX_LENGTH + 1)},
Expand Down Expand Up @@ -4777,10 +4773,6 @@ async def test_update_dataset(self, async_client: "AsyncClient", db: "AsyncSessi
[
{"name": None},
{"name": ""},
{"name": "123$abc"},
{"name": "unit@test"},
{"name": "-test-dataset"},
{"name": "_test-dataset"},
{"name": "a" * (DATASET_NAME_MAX_LENGTH + 1)},
{"name": "test-dataset", "guidelines": ""},
{"name": "test-dataset", "guidelines": "a" * (DATASET_GUIDELINES_MAX_LENGTH + 1)},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -201,23 +201,6 @@ async def test_create_user_with_existent_username(

assert (await db.execute(select(func.count(User.id)))).scalar() == 2

async def test_create_user_with_invalid_username(
self, db: AsyncSession, async_client: AsyncClient, owner_auth_header: dict
):
response = await async_client.post(
self.url(),
headers=owner_auth_header,
json={
"first_name": "First name",
"last_name": "Last name",
"username": "invalid username",
"password": "12345678",
},
)

assert response.status_code == 422
assert (await db.execute(select(func.count(User.id)))).scalar() == 1

async def test_create_user_with_invalid_min_length_first_name(
self, db: AsyncSession, async_client: AsyncClient, owner_auth_header: dict
):
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -87,18 +87,6 @@ async def test_create_workspace_with_existent_name(

assert (await db.execute(select(func.count(Workspace.id)))).scalar() == 1

async def test_create_workspace_with_invalid_name(
self, db: AsyncSession, async_client: AsyncClient, owner_auth_header: dict
):
response = await async_client.post(
self.url(),
headers=owner_auth_header,
json={"name": "invalid name"},
)

assert response.status_code == 422
assert (await db.execute(select(func.count(Workspace.id)))).scalar() == 0

async def test_create_workspace_with_invalid_min_length_name(
self, db: AsyncSession, async_client: AsyncClient, owner_auth_header: dict
):
Expand Down
27 changes: 2 additions & 25 deletions argilla-server/tests/unit/cli/database/users/test_create.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,11 @@
from typing import TYPE_CHECKING

import pytest
from argilla_server.contexts import accounts
from argilla_server.models import User, UserRole, Workspace
from click.testing import CliRunner
from typer import Typer

from argilla_server.contexts import accounts
from argilla_server.models import User, UserRole, Workspace
from tests.factories import UserSyncFactory, WorkspaceSyncFactory

if TYPE_CHECKING:
Expand Down Expand Up @@ -131,17 +131,6 @@ def test_create_with_input_username(sync_db: "Session", cli_runner: CliRunner, c
assert sync_db.query(User).filter_by(username="username").first()


def test_create_with_invalid_username(sync_db: "Session", cli_runner: CliRunner, cli: Typer):
result = cli_runner.invoke(
cli,
"database users create --first-name first-name --username -Invalid-Username --password 12345678 --role owner",
)

assert result.exit_code == 1
assert sync_db.query(User).count() == 0
assert sync_db.query(Workspace).count() == 0


def test_create_with_existing_username(sync_db: "Session", cli_runner: CliRunner, cli: Typer):
UserSyncFactory.create(username="username")

Expand Down Expand Up @@ -243,15 +232,3 @@ def test_create_with_existent_workspaces(sync_db: "Session", cli_runner: CliRunn
user = sync_db.query(User).filter_by(username="username").first()
assert user
assert [ws.name for ws in user.workspaces] == ["workspace-a", "workspace-b", "workspace-c"]


def test_create_with_invalid_workspaces(sync_db: "Session", cli_runner: CliRunner, cli: Typer):
result = cli_runner.invoke(
cli,
"database users create --first-name first-name --username username --role owner --password 12345678 "
"--workspace workspace-a --workspace 'invalid workspace' --workspace workspace-c",
)

assert result.exit_code == 1
assert sync_db.query(User).count() == 0
assert sync_db.query(Workspace).count() == 0
24 changes: 0 additions & 24 deletions argilla-server/tests/unit/cli/database/users/test_migrate.py
Original file line number Diff line number Diff line change
Expand Up @@ -96,30 +96,6 @@ def test_migrate_with_one_user_file(monkeypatch, sync_db: "Session", cli_runner:
assert [ws.name for ws in user.workspaces] == ["john", "argilla", "team"]


def test_migrate_with_invalid_user(monkeypatch, sync_db: "Session", cli_runner: CliRunner, cli: Typer):
mock_users_file = os.path.join(os.path.dirname(__file__), "test_user_files", "users_invalid_user.yml")

with mock.patch.dict(os.environ, {"ARGILLA_LOCAL_AUTH_USERS_DB_FILE": mock_users_file}):
result = cli_runner.invoke(cli, "database users migrate")

assert result.exit_code == 1
assert sync_db.query(User).count() == 0
assert sync_db.query(Workspace).count() == 0
assert sync_db.query(WorkspaceUser).count() == 0


def test_migrate_with_invalid_workspace(monkeypatch, sync_db: "Session", cli_runner: CliRunner, cli: Typer):
mock_users_file = os.path.join(os.path.dirname(__file__), "test_user_files", "users_invalid_workspace.yml")

with mock.patch.dict(os.environ, {"ARGILLA_LOCAL_AUTH_USERS_DB_FILE": mock_users_file}):
result = cli_runner.invoke(cli, "database users migrate")

assert result.exit_code == 1
assert sync_db.query(User).count() == 0
assert sync_db.query(Workspace).count() == 0
assert sync_db.query(WorkspaceUser).count() == 0


def test_migrate_with_nonexistent_file(monkeypatch, sync_db: "Session", cli_runner: CliRunner, cli: Typer):
with mock.patch.dict(os.environ, {"ARGILLA_LOCAL_AUTH_USERS_DB_FILE": "nonexistent.yml"}):
result = cli_runner.invoke(cli, "database users migrate")
Expand Down
27 changes: 1 addition & 26 deletions argilla-server/tests/unit/security/test_model.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,19 +14,13 @@


import pytest

from argilla_server.api.schemas.v1.users import User, UserCreate
from argilla_server.api.schemas.v1.workspaces import WorkspaceCreate

from tests.factories import UserFactory
from tests.pydantic_v1 import ValidationError


@pytest.mark.parametrize("invalid_name", ["work space", "work/space", "work.space", "_", "-"])
def test_workspace_create_invalid_name(invalid_name: str):
with pytest.raises(ValidationError):
WorkspaceCreate(name=invalid_name)


@pytest.mark.parametrize(
"username",
[
Expand All @@ -49,25 +43,6 @@ def test_user_create(username: str):
assert UserCreate(first_name="first-name", username=username, password="12345678")


@pytest.mark.parametrize(
"invalid_username",
[
"user name",
"user/name",
"user.name",
"_",
"-",
"-1234",
"_1234",
"_mark",
"-mark",
],
)
def test_user_create_invalid_username(invalid_username: str):
with pytest.raises(ValidationError):
UserCreate(first_name="first-name", username=invalid_username, password="12345678")


@pytest.mark.asyncio
async def test_user_first_name():
user = await UserFactory.create(first_name="first-name", workspaces=[])
Expand Down

0 comments on commit 485f3ff

Please sign in to comment.