From eeab6e8309b91967f25dce46b197fb68be25b77d Mon Sep 17 00:00:00 2001 From: Yuri Chiucconi Date: Wed, 30 Oct 2024 13:57:48 +0100 Subject: [PATCH 01/24] test reusing id --- tests/no_version/test_db.py | 44 +++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/tests/no_version/test_db.py b/tests/no_version/test_db.py index 0382b832de..604a38a9a6 100644 --- a/tests/no_version/test_db.py +++ b/tests/no_version/test_db.py @@ -1,5 +1,6 @@ import pytest from devtools import debug +from sqlmodel import delete from sqlmodel import select from fractal_server.app.db import DB @@ -114,3 +115,46 @@ def test_DB_class_sync(): assert DB._engine_sync assert DB._sync_session_maker + + +async def test_reusing_id(db): + + num_users = 10 + + # Create `num_users` new users + user_list = [ + UserOAuth(email=f"{x}@y.z", hashed_password="xxx") + for x in range(num_users) + ] + + for user in user_list: + db.add(user) + await db.commit() + for user in user_list: + await db.refresh(user) + + # Extract list of IDs + user_id_list = [user.id for user in user_list] + + # Remove all users + await db.execute(delete(UserOAuth)) + res = await db.execute(select(UserOAuth)) + users = res.scalars().unique().all() + assert len(users) == 0 + + # Create `num_users + 10` new users + new_user_list = [ + UserOAuth(email=f"{x}@y.z", hashed_password="xxx") + for x in range(num_users + 10) + ] + for user in new_user_list: + db.add(user) + await db.commit() + for user in new_user_list: + await db.refresh(user) + + # Extract list of IDs + new_user_id_list = [user.id for user in new_user_list] + + # Assert IDs lists are disjoined + assert set(new_user_id_list).isdisjoint(set(user_id_list)) From acbe909df68a48fc6bae4c1fb5d21662bfb583a4 Mon Sep 17 00:00:00 2001 From: Yuri Chiucconi Date: Wed, 30 Oct 2024 14:38:34 +0100 Subject: [PATCH 02/24] user settings project dir ref #1986 --- fractal_server/app/models/user_settings.py | 1 + fractal_server/app/schemas/user_settings.py | 10 +++++ .../19eca0dd47a9_user_settings_project_dir.py | 39 +++++++++++++++++++ 3 files changed, 50 insertions(+) create mode 100644 fractal_server/migrations/versions/19eca0dd47a9_user_settings_project_dir.py diff --git a/fractal_server/app/models/user_settings.py b/fractal_server/app/models/user_settings.py index acff62a8ca..c779d94df6 100644 --- a/fractal_server/app/models/user_settings.py +++ b/fractal_server/app/models/user_settings.py @@ -36,3 +36,4 @@ class UserSettings(SQLModel, table=True): ssh_jobs_dir: Optional[str] = None slurm_user: Optional[str] = None cache_dir: Optional[str] = None + project_dir: Optional[str] = None diff --git a/fractal_server/app/schemas/user_settings.py b/fractal_server/app/schemas/user_settings.py index edd3686bb2..99fcf1f29c 100644 --- a/fractal_server/app/schemas/user_settings.py +++ b/fractal_server/app/schemas/user_settings.py @@ -28,6 +28,7 @@ class UserSettingsRead(BaseModel): slurm_user: Optional[str] = None slurm_accounts: list[str] cache_dir: Optional[str] = None + project_dir: Optional[str] = None class UserSettingsReadStrict(BaseModel): @@ -35,6 +36,7 @@ class UserSettingsReadStrict(BaseModel): slurm_accounts: list[str] cache_dir: Optional[str] = None ssh_username: Optional[str] = None + project_dir: Optional[str] = None class UserSettingsUpdate(BaseModel, extra=Extra.forbid): @@ -46,6 +48,7 @@ class UserSettingsUpdate(BaseModel, extra=Extra.forbid): slurm_user: Optional[str] = None slurm_accounts: Optional[list[StrictStr]] = None cache_dir: Optional[str] = None + project_dir: Optional[str] = None _ssh_host = validator("ssh_host", allow_reuse=True)( valstr("ssh_host", accept_none=True) @@ -83,6 +86,13 @@ def cache_dir_validator(cls, value): validate_cmd(value) return val_absolute_path("cache_dir")(value) + @validator("project_dir") + def project_dir_validator(cls, value): + if value is None: + return None + validate_cmd(value) + return val_absolute_path("project_dir")(value) + class UserSettingsUpdateStrict(BaseModel, extra=Extra.forbid): slurm_accounts: Optional[list[StrictStr]] = None diff --git a/fractal_server/migrations/versions/19eca0dd47a9_user_settings_project_dir.py b/fractal_server/migrations/versions/19eca0dd47a9_user_settings_project_dir.py new file mode 100644 index 0000000000..652039a45f --- /dev/null +++ b/fractal_server/migrations/versions/19eca0dd47a9_user_settings_project_dir.py @@ -0,0 +1,39 @@ +"""user settings project dir + +Revision ID: 19eca0dd47a9 +Revises: 8e8f227a3e36 +Create Date: 2024-10-30 14:34:28.219355 + +""" +import sqlalchemy as sa +import sqlmodel +from alembic import op + + +# revision identifiers, used by Alembic. +revision = "19eca0dd47a9" +down_revision = "8e8f227a3e36" +branch_labels = None +depends_on = None + + +def upgrade() -> None: + # ### commands auto generated by Alembic - please adjust! ### + with op.batch_alter_table("user_settings", schema=None) as batch_op: + batch_op.add_column( + sa.Column( + "project_dir", + sqlmodel.sql.sqltypes.AutoString(), + nullable=True, + ) + ) + + # ### end Alembic commands ### + + +def downgrade() -> None: + # ### commands auto generated by Alembic - please adjust! ### + with op.batch_alter_table("user_settings", schema=None) as batch_op: + batch_op.drop_column("project_dir") + + # ### end Alembic commands ### From 8bbbc08e4246abaf5c154d3cb8ddd80210711de6 Mon Sep 17 00:00:00 2001 From: Yuri Chiucconi Date: Wed, 30 Oct 2024 14:39:03 +0100 Subject: [PATCH 03/24] dataset zarr dir optional --- fractal_server/app/routes/api/v2/dataset.py | 25 +++++++++++++++++++-- fractal_server/app/schemas/v2/dataset.py | 16 ++++++++----- 2 files changed, 33 insertions(+), 8 deletions(-) diff --git a/fractal_server/app/routes/api/v2/dataset.py b/fractal_server/app/routes/api/v2/dataset.py index fb10699842..439ab174a9 100644 --- a/fractal_server/app/routes/api/v2/dataset.py +++ b/fractal_server/app/routes/api/v2/dataset.py @@ -40,15 +40,36 @@ async def create_dataset( """ Add new dataset to current project """ - await _get_project_check_owner( + project = await _get_project_check_owner( project_id=project_id, user_id=user.id, db=db ) db_dataset = DatasetV2(project_id=project_id, **dataset.dict()) + + if dataset.zarr_dir is None: + if user.settings.project_dir is None: + raise HTTPException( + status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, + detail=( + "Both 'dataset.zarr_dir' and 'user.settings.project_dir' " + "are null" + ), + ) + else: + db_dataset.zarr_dir = "__PLACEHOLDER__" + db.add(db_dataset) + await db.commit() + await db.refresh(db_dataset) + db_dataset.zarr_dir = ( + f"{user.settings.project_dir}/fractal/" + f"{project_id}_{project.name}/" + f"{db_dataset.id}_{db_dataset.name}" + ) + db.add(db_dataset) await db.commit() await db.refresh(db_dataset) - await db.close() + await db.close() return db_dataset diff --git a/fractal_server/app/schemas/v2/dataset.py b/fractal_server/app/schemas/v2/dataset.py index 3bb318446b..76eb1d90b7 100644 --- a/fractal_server/app/schemas/v2/dataset.py +++ b/fractal_server/app/schemas/v2/dataset.py @@ -33,14 +33,16 @@ class DatasetCreateV2(BaseModel, extra=Extra.forbid): name: str - zarr_dir: str + zarr_dir: Optional[str] = None filters: Filters = Field(default_factory=Filters) # Validators @validator("zarr_dir") def normalize_zarr_dir(cls, v: str) -> str: - return normalize_url(v) + if v is not None: + return normalize_url(v) + return v _name = validator("name", allow_reuse=True)(valstr("name")) @@ -57,7 +59,7 @@ class DatasetReadV2(BaseModel): timestamp_created: datetime - zarr_dir: str + zarr_dir: Optional[str] = None filters: Filters = Field(default_factory=Filters) # Validators @@ -94,14 +96,16 @@ class DatasetImportV2(BaseModel, extra=Extra.forbid): """ name: str - zarr_dir: str + zarr_dir: Optional[str] = None images: list[SingleImage] = Field(default_factory=[]) filters: Filters = Field(default_factory=Filters) # Validators @validator("zarr_dir") def normalize_zarr_dir(cls, v: str) -> str: - return normalize_url(v) + if v is not None: + return normalize_url(v) + return v class DatasetExportV2(BaseModel): @@ -116,6 +120,6 @@ class DatasetExportV2(BaseModel): """ name: str - zarr_dir: str + zarr_dir: Optional[str] = None images: list[SingleImage] filters: Filters From 7da666f54f838e22cdd01c162ce44b48d47cbd68 Mon Sep 17 00:00:00 2001 From: Yuri Chiucconi Date: Wed, 30 Oct 2024 14:40:36 +0100 Subject: [PATCH 04/24] skip test if sqlite --- tests/no_version/test_db.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/no_version/test_db.py b/tests/no_version/test_db.py index 604a38a9a6..3166079ec9 100644 --- a/tests/no_version/test_db.py +++ b/tests/no_version/test_db.py @@ -117,6 +117,9 @@ def test_DB_class_sync(): assert DB._sync_session_maker +@pytest.mark.skipif( + DB_ENGINE == "sqlite", reason="Skip if DB is SQLite, pass if it's Postgres" +) async def test_reusing_id(db): num_users = 10 From 6ca8db70d8162b12743a29ddbbaad63a634a9b0b Mon Sep 17 00:00:00 2001 From: Yuri Chiucconi Date: Wed, 30 Oct 2024 16:07:54 +0100 Subject: [PATCH 05/24] remove some optionals and slugify names --- fractal_server/app/routes/api/v2/dataset.py | 11 +++++++++-- fractal_server/app/schemas/v2/dataset.py | 6 +++--- 2 files changed, 12 insertions(+), 5 deletions(-) diff --git a/fractal_server/app/routes/api/v2/dataset.py b/fractal_server/app/routes/api/v2/dataset.py index 439ab174a9..b9fc46a6b6 100644 --- a/fractal_server/app/routes/api/v2/dataset.py +++ b/fractal_server/app/routes/api/v2/dataset.py @@ -26,6 +26,13 @@ router = APIRouter() +def _slugify_name(_name: str) -> str: + _new_name = _name + for char in (" ", ".", "/", "\\"): + _new_name = _new_name.replace(char, "_") + return _new_name + + @router.post( "/project/{project_id}/dataset/", response_model=DatasetReadV2, @@ -61,8 +68,8 @@ async def create_dataset( await db.refresh(db_dataset) db_dataset.zarr_dir = ( f"{user.settings.project_dir}/fractal/" - f"{project_id}_{project.name}/" - f"{db_dataset.id}_{db_dataset.name}" + f"{project_id}_{_slugify_name(project.name)}/" + f"{db_dataset.id}_{_slugify_name(db_dataset.name)}" ) db.add(db_dataset) diff --git a/fractal_server/app/schemas/v2/dataset.py b/fractal_server/app/schemas/v2/dataset.py index 76eb1d90b7..8c8aef718b 100644 --- a/fractal_server/app/schemas/v2/dataset.py +++ b/fractal_server/app/schemas/v2/dataset.py @@ -59,7 +59,7 @@ class DatasetReadV2(BaseModel): timestamp_created: datetime - zarr_dir: Optional[str] = None + zarr_dir: str filters: Filters = Field(default_factory=Filters) # Validators @@ -96,7 +96,7 @@ class DatasetImportV2(BaseModel, extra=Extra.forbid): """ name: str - zarr_dir: Optional[str] = None + zarr_dir: str images: list[SingleImage] = Field(default_factory=[]) filters: Filters = Field(default_factory=Filters) @@ -120,6 +120,6 @@ class DatasetExportV2(BaseModel): """ name: str - zarr_dir: Optional[str] = None + zarr_dir: str images: list[SingleImage] filters: Filters From c1a55ff4f3b89895c92fd44fa68748af564d547d Mon Sep 17 00:00:00 2001 From: Yuri Chiucconi Date: Wed, 30 Oct 2024 16:21:24 +0100 Subject: [PATCH 06/24] change endpoint if-else branching --- fractal_server/app/routes/api/v2/dataset.py | 45 +++++++++++++++------ 1 file changed, 32 insertions(+), 13 deletions(-) diff --git a/fractal_server/app/routes/api/v2/dataset.py b/fractal_server/app/routes/api/v2/dataset.py index b9fc46a6b6..dea7f324ea 100644 --- a/fractal_server/app/routes/api/v2/dataset.py +++ b/fractal_server/app/routes/api/v2/dataset.py @@ -22,6 +22,8 @@ from ._aux_functions import _get_submitted_jobs_statement from fractal_server.app.models import UserOAuth from fractal_server.app.routes.auth import current_active_user +from fractal_server.config import get_settings +from fractal_server.syringe import Inject router = APIRouter() @@ -50,9 +52,15 @@ async def create_dataset( project = await _get_project_check_owner( project_id=project_id, user_id=user.id, db=db ) - db_dataset = DatasetV2(project_id=project_id, **dataset.dict()) + settings = Inject(get_settings) if dataset.zarr_dir is None: + + if settings.DB_ENGINE == "sqlite": + raise HTTPException( + status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, + detail=f"{dataset.zarr_dir=} but {settings.DB_ENGINE=}", + ) if user.settings.project_dir is None: raise HTTPException( status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, @@ -61,22 +69,33 @@ async def create_dataset( "are null" ), ) - else: - db_dataset.zarr_dir = "__PLACEHOLDER__" - db.add(db_dataset) - await db.commit() - await db.refresh(db_dataset) - db_dataset.zarr_dir = ( + + db_dataset = DatasetV2( + project_id=project_id, + zarr_dir="__PLACEHOLDER__", + **dataset.dict(), + ) + db.add(db_dataset) + await db.commit() + await db.refresh(db_dataset) + setattr( + db_dataset, + "zarr_dir", + ( f"{user.settings.project_dir}/fractal/" f"{project_id}_{_slugify_name(project.name)}/" f"{db_dataset.id}_{_slugify_name(db_dataset.name)}" - ) - - db.add(db_dataset) - await db.commit() - await db.refresh(db_dataset) + ), + ) + db.add(db_dataset) + await db.commit() + await db.refresh(db_dataset) + else: + db_dataset = DatasetV2(project_id=project_id, **dataset.dict()) + db.add(db_dataset) + await db.commit() + await db.refresh(db_dataset) - await db.close() return db_dataset From fa2221672f0d9261c4cfb9ce3c8dfb8538fbc368 Mon Sep 17 00:00:00 2001 From: Yuri Chiucconi Date: Wed, 30 Oct 2024 16:27:47 +0100 Subject: [PATCH 07/24] use string tool --- fractal_server/app/routes/api/v2/dataset.py | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/fractal_server/app/routes/api/v2/dataset.py b/fractal_server/app/routes/api/v2/dataset.py index dea7f324ea..d0000e96b9 100644 --- a/fractal_server/app/routes/api/v2/dataset.py +++ b/fractal_server/app/routes/api/v2/dataset.py @@ -23,18 +23,12 @@ from fractal_server.app.models import UserOAuth from fractal_server.app.routes.auth import current_active_user from fractal_server.config import get_settings +from fractal_server.string_tools import sanitize_string from fractal_server.syringe import Inject router = APIRouter() -def _slugify_name(_name: str) -> str: - _new_name = _name - for char in (" ", ".", "/", "\\"): - _new_name = _new_name.replace(char, "_") - return _new_name - - @router.post( "/project/{project_id}/dataset/", response_model=DatasetReadV2, @@ -83,8 +77,8 @@ async def create_dataset( "zarr_dir", ( f"{user.settings.project_dir}/fractal/" - f"{project_id}_{_slugify_name(project.name)}/" - f"{db_dataset.id}_{_slugify_name(db_dataset.name)}" + f"{project_id}_{sanitize_string(project.name)}/" + f"{db_dataset.id}_{sanitize_string(db_dataset.name)}" ), ) db.add(db_dataset) From b8d7ca5a5dea62a0f2ee4296ccfd41670f4ff806 Mon Sep 17 00:00:00 2001 From: Yuri Chiucconi Date: Wed, 30 Oct 2024 16:41:21 +0100 Subject: [PATCH 08/24] fix test --- tests/no_version/test_api_user_groups.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/no_version/test_api_user_groups.py b/tests/no_version/test_api_user_groups.py index b891d2d43f..1032c8c26e 100644 --- a/tests/no_version/test_api_user_groups.py +++ b/tests/no_version/test_api_user_groups.py @@ -331,6 +331,7 @@ async def test_patch_user_settings_bulk( slurm_user="test01", slurm_accounts=[], cache_dir=None, + project_dir=None, ) == user.settings.dict(exclude={"id"}) # remove user4 from default user group From 5861ebb3909620993b6f87af958c52deb9f92397 Mon Sep 17 00:00:00 2001 From: Yuri Chiucconi Date: Wed, 30 Oct 2024 16:59:40 +0100 Subject: [PATCH 09/24] exclude none and test --- fractal_server/app/routes/api/v2/dataset.py | 6 ++- tests/v2/03_api/test_api_dataset.py | 41 ++++++++++++++------- 2 files changed, 32 insertions(+), 15 deletions(-) diff --git a/fractal_server/app/routes/api/v2/dataset.py b/fractal_server/app/routes/api/v2/dataset.py index d0000e96b9..78ef4a376b 100644 --- a/fractal_server/app/routes/api/v2/dataset.py +++ b/fractal_server/app/routes/api/v2/dataset.py @@ -67,7 +67,7 @@ async def create_dataset( db_dataset = DatasetV2( project_id=project_id, zarr_dir="__PLACEHOLDER__", - **dataset.dict(), + **dataset.dict(exclude_none=True), ) db.add(db_dataset) await db.commit() @@ -85,7 +85,9 @@ async def create_dataset( await db.commit() await db.refresh(db_dataset) else: - db_dataset = DatasetV2(project_id=project_id, **dataset.dict()) + db_dataset = DatasetV2( + project_id=project_id, **dataset.dict(exclude_none=True) + ) db.add(db_dataset) await db.commit() await db.refresh(db_dataset) diff --git a/tests/v2/03_api/test_api_dataset.py b/tests/v2/03_api/test_api_dataset.py index c49b418640..c293ab16e6 100644 --- a/tests/v2/03_api/test_api_dataset.py +++ b/tests/v2/03_api/test_api_dataset.py @@ -7,7 +7,9 @@ _workflow_insert_task, ) from fractal_server.app.schemas.v2 import JobStatusTypeV2 +from fractal_server.config import get_settings from fractal_server.images import SingleImage +from fractal_server.syringe import Inject PREFIX = "api/v2" @@ -194,33 +196,26 @@ async def test_get_user_datasets( assert len(ds["history"]) == 0 -async def test_post_dataset(client, MockCurrentUser): - async with MockCurrentUser(): - # CREATE A PROJECT - res = await client.post( - f"{PREFIX}/project/", - json=dict(name="test project"), - ) - assert res.status_code == 201 - project = res.json() - project_id = project["id"] +async def test_post_dataset(client, MockCurrentUser, project_factory_v2): + async with MockCurrentUser() as user: + prj = await project_factory_v2(user) # ADD DATASET payload = dict(name="new dataset", zarr_dir="/tmp/zarr") res = await client.post( - f"{PREFIX}/project/{project_id}/dataset/", + f"{PREFIX}/project/{prj.id}/dataset/", json=payload, ) debug(res.json()) assert res.status_code == 201 dataset = res.json() assert dataset["name"] == payload["name"] - assert dataset["project_id"] == project_id + assert dataset["project_id"] == prj.id # EDIT DATASET payload1 = dict(name="new dataset name") res = await client.patch( - f"{PREFIX}/project/{project_id}/dataset/{dataset['id']}/", + f"{PREFIX}/project/{prj.id}/dataset/{dataset['id']}/", json=payload1, ) patched_dataset = res.json() @@ -229,6 +224,26 @@ async def test_post_dataset(client, MockCurrentUser): for k, v in payload1.items(): assert patched_dataset[k] == v + # Test POST dataset without zarr_dir + async with MockCurrentUser( + user_settings_dict={"project_dir": "/some/dir"} + ) as user: + prj = await project_factory_v2(user) + res = await client.post( + f"{PREFIX}/project/{prj.id}/dataset/", json=dict(name="DSName") + ) + settings = Inject(get_settings) + if settings.DB_ENGINE == "sqlite": + assert res.status_code == 422 + else: + assert res.status_code == 201 + async with MockCurrentUser() as user: + prj = await project_factory_v2(user) + res = await client.post( + f"{PREFIX}/project/{prj.id}/dataset/", json=dict(name="DSName2") + ) + assert res.status_code == 422 + async def test_delete_dataset( client, MockCurrentUser, project_factory_v2, dataset_factory_v2 From b1cb677e034623011304ea83e06b028fb9a90901 Mon Sep 17 00:00:00 2001 From: Yuri Chiucconi Date: Thu, 31 Oct 2024 16:45:44 +0100 Subject: [PATCH 10/24] fix tests --- tests/no_version/test_api_user_groups.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tests/no_version/test_api_user_groups.py b/tests/no_version/test_api_user_groups.py index 1032c8c26e..bd01672eac 100644 --- a/tests/no_version/test_api_user_groups.py +++ b/tests/no_version/test_api_user_groups.py @@ -361,7 +361,9 @@ async def test_patch_user_settings_bulk( # assert user1, user2 and user3 has been updated for user in [user1, user2, user3]: await db.refresh(user) - assert patch == user.settings.dict(exclude={"id", "slurm_user"}) + assert patch == user.settings.dict( + exclude={"id", "slurm_user", "project_dir"} + ) assert user.settings.slurm_user == "test01" # `slurm_user` not patched # assert user4 has old settings await db.refresh(user4) @@ -374,4 +376,4 @@ async def test_patch_user_settings_bulk( slurm_user="test01", slurm_accounts=[], cache_dir=None, - ) == user4.settings.dict(exclude={"id"}) + ) == user4.settings.dict(exclude={"id", "project_dir"}) From a77af50750332d3c3fedcaec208c529a645d587f Mon Sep 17 00:00:00 2001 From: Yuri Chiucconi Date: Mon, 4 Nov 2024 09:45:26 +0100 Subject: [PATCH 11/24] accept the edge-case of mistakenly reusing folders in sqlite --- fractal_server/app/routes/api/v2/dataset.py | 8 -------- tests/no_version/test_db.py | 3 --- 2 files changed, 11 deletions(-) diff --git a/fractal_server/app/routes/api/v2/dataset.py b/fractal_server/app/routes/api/v2/dataset.py index 78ef4a376b..ebd0761936 100644 --- a/fractal_server/app/routes/api/v2/dataset.py +++ b/fractal_server/app/routes/api/v2/dataset.py @@ -22,9 +22,7 @@ from ._aux_functions import _get_submitted_jobs_statement from fractal_server.app.models import UserOAuth from fractal_server.app.routes.auth import current_active_user -from fractal_server.config import get_settings from fractal_server.string_tools import sanitize_string -from fractal_server.syringe import Inject router = APIRouter() @@ -46,15 +44,9 @@ async def create_dataset( project = await _get_project_check_owner( project_id=project_id, user_id=user.id, db=db ) - settings = Inject(get_settings) if dataset.zarr_dir is None: - if settings.DB_ENGINE == "sqlite": - raise HTTPException( - status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, - detail=f"{dataset.zarr_dir=} but {settings.DB_ENGINE=}", - ) if user.settings.project_dir is None: raise HTTPException( status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, diff --git a/tests/no_version/test_db.py b/tests/no_version/test_db.py index 3166079ec9..604a38a9a6 100644 --- a/tests/no_version/test_db.py +++ b/tests/no_version/test_db.py @@ -117,9 +117,6 @@ def test_DB_class_sync(): assert DB._sync_session_maker -@pytest.mark.skipif( - DB_ENGINE == "sqlite", reason="Skip if DB is SQLite, pass if it's Postgres" -) async def test_reusing_id(db): num_users = 10 From db3cec4f0f5ac27e467f7add375dcb08979b1c3f Mon Sep 17 00:00:00 2001 From: Yuri Chiucconi Date: Mon, 4 Nov 2024 09:51:11 +0100 Subject: [PATCH 12/24] correctly remove sqlite check --- tests/no_version/test_db.py | 3 +++ tests/v2/03_api/test_api_dataset.py | 8 +------- 2 files changed, 4 insertions(+), 7 deletions(-) diff --git a/tests/no_version/test_db.py b/tests/no_version/test_db.py index 604a38a9a6..3166079ec9 100644 --- a/tests/no_version/test_db.py +++ b/tests/no_version/test_db.py @@ -117,6 +117,9 @@ def test_DB_class_sync(): assert DB._sync_session_maker +@pytest.mark.skipif( + DB_ENGINE == "sqlite", reason="Skip if DB is SQLite, pass if it's Postgres" +) async def test_reusing_id(db): num_users = 10 diff --git a/tests/v2/03_api/test_api_dataset.py b/tests/v2/03_api/test_api_dataset.py index c293ab16e6..081f30da7e 100644 --- a/tests/v2/03_api/test_api_dataset.py +++ b/tests/v2/03_api/test_api_dataset.py @@ -7,9 +7,7 @@ _workflow_insert_task, ) from fractal_server.app.schemas.v2 import JobStatusTypeV2 -from fractal_server.config import get_settings from fractal_server.images import SingleImage -from fractal_server.syringe import Inject PREFIX = "api/v2" @@ -232,11 +230,7 @@ async def test_post_dataset(client, MockCurrentUser, project_factory_v2): res = await client.post( f"{PREFIX}/project/{prj.id}/dataset/", json=dict(name="DSName") ) - settings = Inject(get_settings) - if settings.DB_ENGINE == "sqlite": - assert res.status_code == 422 - else: - assert res.status_code == 201 + assert res.status_code == 201 async with MockCurrentUser() as user: prj = await project_factory_v2(user) res = await client.post( From a48fee1bbc6ed5ad5e4acd149c8898f63b5a78da Mon Sep 17 00:00:00 2001 From: Yuri Chiucconi Date: Mon, 4 Nov 2024 10:02:49 +0100 Subject: [PATCH 13/24] changelog --- CHANGELOG.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3f39a65766..ef3db8047c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,11 @@ **Note**: Numbers like (\#1234) point to closed Pull Requests on the fractal-server repository. +# 2.7.2 (Unreleased) + +* API: + * Add `project_dir` attribute to `UserSettings` (\#1990). + * Set a default for `DatasetV2.zarr_dir` (\#1990). + # 2.7.1 > WARNING: As of this version, all extras for `pip install` are deprecated and From d81c75a54c016ec328ba9f0ad24ceec81ed2e847 Mon Sep 17 00:00:00 2001 From: Yuri Chiucconi Date: Mon, 4 Nov 2024 10:28:54 +0100 Subject: [PATCH 14/24] DatasetImportV2.zarr_dir made required --- fractal_server/app/schemas/v2/dataset.py | 4 +--- tests/no_version/test_api_user_groups.py | 14 ++++++++++---- tests/v2/01_schemas/test_schemas_dataset.py | 7 +++++++ 3 files changed, 18 insertions(+), 7 deletions(-) diff --git a/fractal_server/app/schemas/v2/dataset.py b/fractal_server/app/schemas/v2/dataset.py index 8c8aef718b..ed94765238 100644 --- a/fractal_server/app/schemas/v2/dataset.py +++ b/fractal_server/app/schemas/v2/dataset.py @@ -103,9 +103,7 @@ class DatasetImportV2(BaseModel, extra=Extra.forbid): # Validators @validator("zarr_dir") def normalize_zarr_dir(cls, v: str) -> str: - if v is not None: - return normalize_url(v) - return v + return normalize_url(v) class DatasetExportV2(BaseModel): diff --git a/tests/no_version/test_api_user_groups.py b/tests/no_version/test_api_user_groups.py index bd01672eac..724cafc535 100644 --- a/tests/no_version/test_api_user_groups.py +++ b/tests/no_version/test_api_user_groups.py @@ -352,6 +352,7 @@ async def test_patch_user_settings_bulk( # missing `slurm_user` slurm_accounts=["foo", "bar"], cache_dir="/tmp/cache", + project_dir="/foo", ) res = await registered_superuser_client.patch( f"{PREFIX}/group/{default_user_group.id}/user-settings/", json=patch @@ -361,9 +362,7 @@ async def test_patch_user_settings_bulk( # assert user1, user2 and user3 has been updated for user in [user1, user2, user3]: await db.refresh(user) - assert patch == user.settings.dict( - exclude={"id", "slurm_user", "project_dir"} - ) + assert patch == user.settings.dict(exclude={"id", "slurm_user"}) assert user.settings.slurm_user == "test01" # `slurm_user` not patched # assert user4 has old settings await db.refresh(user4) @@ -376,4 +375,11 @@ async def test_patch_user_settings_bulk( slurm_user="test01", slurm_accounts=[], cache_dir=None, - ) == user4.settings.dict(exclude={"id", "project_dir"}) + project_dir=None, + ) == user4.settings.dict(exclude={"id"}) + + res = await registered_superuser_client.patch( + f"{PREFIX}/group/{default_user_group.id}/user-settings/", + json=dict(project_dir="not/an/absolute/path"), + ) + assert res.status_code == 422 diff --git a/tests/v2/01_schemas/test_schemas_dataset.py b/tests/v2/01_schemas/test_schemas_dataset.py index 379ff5c378..446c060bc1 100644 --- a/tests/v2/01_schemas/test_schemas_dataset.py +++ b/tests/v2/01_schemas/test_schemas_dataset.py @@ -35,6 +35,13 @@ async def test_schemas_dataset_v2(): ) assert dataset_create.zarr_dir == normalize_url(dataset_create.zarr_dir) + # unit test `zarr_dir==None` + DatasetImportV2( + name="name", + filters={"attributes": {"x": 10}}, + zarr_dir="/tmp/", + images=[{"zarr_url": "/tmp/image/"}], + ) dataset_import = DatasetImportV2( name="name", filters={"attributes": {"x": 10}}, From 810ca16a845bfded2b52b2db78ebbbc82f10c730 Mon Sep 17 00:00:00 2001 From: Yuri Chiucconi Date: Mon, 4 Nov 2024 11:52:22 +0100 Subject: [PATCH 15/24] remove exclude_none and setattr --- fractal_server/app/routes/api/v2/dataset.py | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/fractal_server/app/routes/api/v2/dataset.py b/fractal_server/app/routes/api/v2/dataset.py index ebd0761936..6ecaa5f8f8 100644 --- a/fractal_server/app/routes/api/v2/dataset.py +++ b/fractal_server/app/routes/api/v2/dataset.py @@ -59,27 +59,21 @@ async def create_dataset( db_dataset = DatasetV2( project_id=project_id, zarr_dir="__PLACEHOLDER__", - **dataset.dict(exclude_none=True), + **dataset.dict(exclude={"zarr_dir"}), ) db.add(db_dataset) await db.commit() await db.refresh(db_dataset) - setattr( - db_dataset, - "zarr_dir", - ( - f"{user.settings.project_dir}/fractal/" - f"{project_id}_{sanitize_string(project.name)}/" - f"{db_dataset.id}_{sanitize_string(db_dataset.name)}" - ), + db_dataset.zarr_dir = ( + f"{user.settings.project_dir}/fractal/" + f"{project_id}_{sanitize_string(project.name)}/" + f"{db_dataset.id}_{sanitize_string(db_dataset.name)}" ) db.add(db_dataset) await db.commit() await db.refresh(db_dataset) else: - db_dataset = DatasetV2( - project_id=project_id, **dataset.dict(exclude_none=True) - ) + db_dataset = DatasetV2(project_id=project_id, **dataset.dict()) db.add(db_dataset) await db.commit() await db.refresh(db_dataset) From d612e4b2762dc110e9c996fd516f37330a3804d9 Mon Sep 17 00:00:00 2001 From: Yuri Chiucconi Date: Mon, 4 Nov 2024 11:56:38 +0100 Subject: [PATCH 16/24] docstrin for superuser restricted schemas --- fractal_server/app/schemas/user_settings.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/fractal_server/app/schemas/user_settings.py b/fractal_server/app/schemas/user_settings.py index 99fcf1f29c..0158cb7d1c 100644 --- a/fractal_server/app/schemas/user_settings.py +++ b/fractal_server/app/schemas/user_settings.py @@ -19,6 +19,10 @@ class UserSettingsRead(BaseModel): + """ + Schema reserved for superusers + """ + id: int ssh_host: Optional[str] = None ssh_username: Optional[str] = None @@ -40,6 +44,10 @@ class UserSettingsReadStrict(BaseModel): class UserSettingsUpdate(BaseModel, extra=Extra.forbid): + """ + Schema reserved for superusers + """ + ssh_host: Optional[str] = None ssh_username: Optional[str] = None ssh_private_key_path: Optional[str] = None From 4d9eb33d7d484ef634c3b6c4f1b558a133080262 Mon Sep 17 00:00:00 2001 From: Yuri Chiucconi Date: Mon, 4 Nov 2024 12:00:14 +0100 Subject: [PATCH 17/24] normalize path --- fractal_server/app/routes/api/v2/dataset.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/fractal_server/app/routes/api/v2/dataset.py b/fractal_server/app/routes/api/v2/dataset.py index 6ecaa5f8f8..d47d69bd90 100644 --- a/fractal_server/app/routes/api/v2/dataset.py +++ b/fractal_server/app/routes/api/v2/dataset.py @@ -23,6 +23,7 @@ from fractal_server.app.models import UserOAuth from fractal_server.app.routes.auth import current_active_user from fractal_server.string_tools import sanitize_string +from fractal_server.urls import normalize_url router = APIRouter() @@ -64,11 +65,14 @@ async def create_dataset( db.add(db_dataset) await db.commit() await db.refresh(db_dataset) - db_dataset.zarr_dir = ( + path = ( f"{user.settings.project_dir}/fractal/" f"{project_id}_{sanitize_string(project.name)}/" f"{db_dataset.id}_{sanitize_string(db_dataset.name)}" ) + normalized_path = normalize_url(path) + db_dataset.zarr_dir = normalized_path + db.add(db_dataset) await db.commit() await db.refresh(db_dataset) From d72a049986be03a3e36370a688b2c4f2691389d3 Mon Sep 17 00:00:00 2001 From: Yuri Chiucconi Date: Mon, 4 Nov 2024 12:05:34 +0100 Subject: [PATCH 18/24] replace skip with if-else --- tests/no_version/test_db.py | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/tests/no_version/test_db.py b/tests/no_version/test_db.py index 3166079ec9..7262a9bb9d 100644 --- a/tests/no_version/test_db.py +++ b/tests/no_version/test_db.py @@ -117,10 +117,12 @@ def test_DB_class_sync(): assert DB._sync_session_maker -@pytest.mark.skipif( - DB_ENGINE == "sqlite", reason="Skip if DB is SQLite, pass if it's Postgres" -) async def test_reusing_id(db): + """ + Tests different database behaviors with incremental IDs. + + https://github.com/fractal-analytics-platform/fractal-server/issues/1991 + """ num_users = 10 @@ -159,5 +161,9 @@ async def test_reusing_id(db): # Extract list of IDs new_user_id_list = [user.id for user in new_user_list] - # Assert IDs lists are disjoined - assert set(new_user_id_list).isdisjoint(set(user_id_list)) + if DB_ENGINE == "sqlite": + # Assert IDs lists are overlapping + assert not set(new_user_id_list).isdisjoint(set(user_id_list)) + else: + # Assert IDs lists are disjoined + assert set(new_user_id_list).isdisjoint(set(user_id_list)) From 12580d9e67fd0cdb6edbec214f30ce77f2f94b3e Mon Sep 17 00:00:00 2001 From: Yuri Chiucconi Date: Mon, 4 Nov 2024 12:10:18 +0100 Subject: [PATCH 19/24] fix default factory --- fractal_server/app/schemas/v2/dataset.py | 2 +- tests/v2/01_schemas/test_schemas_dataset.py | 10 +++------- 2 files changed, 4 insertions(+), 8 deletions(-) diff --git a/fractal_server/app/schemas/v2/dataset.py b/fractal_server/app/schemas/v2/dataset.py index ed94765238..41678e8336 100644 --- a/fractal_server/app/schemas/v2/dataset.py +++ b/fractal_server/app/schemas/v2/dataset.py @@ -97,7 +97,7 @@ class DatasetImportV2(BaseModel, extra=Extra.forbid): name: str zarr_dir: str - images: list[SingleImage] = Field(default_factory=[]) + images: list[SingleImage] = Field(default_factory=list) filters: Filters = Field(default_factory=Filters) # Validators diff --git a/tests/v2/01_schemas/test_schemas_dataset.py b/tests/v2/01_schemas/test_schemas_dataset.py index 446c060bc1..4be7d78648 100644 --- a/tests/v2/01_schemas/test_schemas_dataset.py +++ b/tests/v2/01_schemas/test_schemas_dataset.py @@ -35,13 +35,9 @@ async def test_schemas_dataset_v2(): ) assert dataset_create.zarr_dir == normalize_url(dataset_create.zarr_dir) - # unit test `zarr_dir==None` - DatasetImportV2( - name="name", - filters={"attributes": {"x": 10}}, - zarr_dir="/tmp/", - images=[{"zarr_url": "/tmp/image/"}], - ) + with pytest.raises(ValidationError): + DatasetImportV2(name="name", zarr_dir="None") + dataset_import = DatasetImportV2( name="name", filters={"attributes": {"x": 10}}, From 9845208b0f4f695dc9e0e9bbfefce018b5e96f07 Mon Sep 17 00:00:00 2001 From: Yuri Chiucconi Date: Mon, 4 Nov 2024 12:17:27 +0100 Subject: [PATCH 20/24] assert zarr_dir --- tests/v2/03_api/test_api_dataset.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/tests/v2/03_api/test_api_dataset.py b/tests/v2/03_api/test_api_dataset.py index 081f30da7e..2e135391af 100644 --- a/tests/v2/03_api/test_api_dataset.py +++ b/tests/v2/03_api/test_api_dataset.py @@ -8,6 +8,8 @@ ) from fractal_server.app.schemas.v2 import JobStatusTypeV2 from fractal_server.images import SingleImage +from fractal_server.string_tools import sanitize_string +from fractal_server.urls import normalize_url PREFIX = "api/v2" @@ -230,7 +232,13 @@ async def test_post_dataset(client, MockCurrentUser, project_factory_v2): res = await client.post( f"{PREFIX}/project/{prj.id}/dataset/", json=dict(name="DSName") ) + assert res.json()["zarr_dir"] == normalize_url( + f"{user.settings.project_dir}/fractal/" + f"{prj.id}_{sanitize_string(prj.name)}/" + f"{res.json()['id']}_{sanitize_string(res.json()['name'])}" + ) assert res.status_code == 201 + async with MockCurrentUser() as user: prj = await project_factory_v2(user) res = await client.post( From fc5e5add1530e0f5033a986f4467a5f1d4eefdf3 Mon Sep 17 00:00:00 2001 From: Yuri Chiucconi Date: Mon, 4 Nov 2024 12:27:42 +0100 Subject: [PATCH 21/24] TO REVERT verify #1987 --- tests/fixtures_server_v2.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/fixtures_server_v2.py b/tests/fixtures_server_v2.py index 5d22947188..ca71db026c 100644 --- a/tests/fixtures_server_v2.py +++ b/tests/fixtures_server_v2.py @@ -49,7 +49,7 @@ async def dataset_factory_v2(db: AsyncSession, tmp_path): async def __dataset_factory_v2(db: AsyncSession = db, **kwargs): defaults = dict( - name="My Dataset", project_id=1, zarr_dir=f"{tmp_path}/zarr" + name="My Dataset", project_id=1, zarr_dir=f"{tmp_path}/z/a/r/r" ) args = dict(**defaults) args.update(kwargs) From 2104143f068f25591b1d81417f6e86fa5b1e4f1e Mon Sep 17 00:00:00 2001 From: Yuri Chiucconi Date: Mon, 4 Nov 2024 12:31:56 +0100 Subject: [PATCH 22/24] Revert "TO REVERT" This reverts commit fc5e5add1530e0f5033a986f4467a5f1d4eefdf3. --- tests/fixtures_server_v2.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/fixtures_server_v2.py b/tests/fixtures_server_v2.py index ca71db026c..5d22947188 100644 --- a/tests/fixtures_server_v2.py +++ b/tests/fixtures_server_v2.py @@ -49,7 +49,7 @@ async def dataset_factory_v2(db: AsyncSession, tmp_path): async def __dataset_factory_v2(db: AsyncSession = db, **kwargs): defaults = dict( - name="My Dataset", project_id=1, zarr_dir=f"{tmp_path}/z/a/r/r" + name="My Dataset", project_id=1, zarr_dir=f"{tmp_path}/zarr" ) args = dict(**defaults) args.update(kwargs) From ee59118c089c16bc7d7e1d908e5f75089ef2bf3e Mon Sep 17 00:00:00 2001 From: Yuri Chiucconi Date: Mon, 4 Nov 2024 12:45:23 +0100 Subject: [PATCH 23/24] tests that `None` is a valid `project_dir` --- tests/no_version/test_api_user_groups.py | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/tests/no_version/test_api_user_groups.py b/tests/no_version/test_api_user_groups.py index 724cafc535..8a8fb864a7 100644 --- a/tests/no_version/test_api_user_groups.py +++ b/tests/no_version/test_api_user_groups.py @@ -383,3 +383,20 @@ async def test_patch_user_settings_bulk( json=dict(project_dir="not/an/absolute/path"), ) assert res.status_code == 422 + + # `None` is a valid `project_dir` + res = await registered_superuser_client.patch( + f"{PREFIX}/group/{default_user_group.id}/user-settings/", + json=dict(project_dir="/fancy/dir"), + ) + assert res.status_code == 200 + for user in [user1, user2, user3]: + await db.refresh(user) + assert user.settings.project_dir == "/fancy/dir" + res = await registered_superuser_client.patch( + f"{PREFIX}/group/{default_user_group.id}/user-settings/", + json=dict(project_dir=None), + ) + for user in [user1, user2, user3]: + await db.refresh(user) + assert user.settings.project_dir is None From 5d797ddea8b6e49450d1dcb94bdb179bc626557f Mon Sep 17 00:00:00 2001 From: Yuri Chiucconi Date: Mon, 4 Nov 2024 12:52:17 +0100 Subject: [PATCH 24/24] test missing case --- tests/v2/01_schemas/test_schemas_dataset.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/v2/01_schemas/test_schemas_dataset.py b/tests/v2/01_schemas/test_schemas_dataset.py index 4be7d78648..87f32df041 100644 --- a/tests/v2/01_schemas/test_schemas_dataset.py +++ b/tests/v2/01_schemas/test_schemas_dataset.py @@ -27,6 +27,8 @@ async def test_schemas_dataset_v2(): DatasetCreateV2( name="name", zarr_dir="/zarr", filters={"types": {"a": "b"}} ) + # Test zarr_dir=None is valid + DatasetCreateV2(name="name", zarr_dir=None) dataset_create = DatasetCreateV2( name="name", @@ -36,7 +38,7 @@ async def test_schemas_dataset_v2(): assert dataset_create.zarr_dir == normalize_url(dataset_create.zarr_dir) with pytest.raises(ValidationError): - DatasetImportV2(name="name", zarr_dir="None") + DatasetImportV2(name="name", zarr_dir=None) dataset_import = DatasetImportV2( name="name",