Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DOP-14025] Remove pathlib.PurePosixPath from API schemas #41

Merged
merged 1 commit into from
Apr 22, 2024
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
1 change: 1 addition & 0 deletions docs/changelog/next_release/41.bugfix.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix 500 error while creating HDFS connection.
2 changes: 1 addition & 1 deletion syncmaster/backend/api/v1/router.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
from syncmaster.backend.api.v1.connections import router as connection_router
from syncmaster.backend.api.v1.groups import router as group_router
from syncmaster.backend.api.v1.queue import router as queue_router
from syncmaster.backend.api.v1.transfers.router import router as transfer_router
from syncmaster.backend.api.v1.transfers import router as transfer_router
from syncmaster.backend.api.v1.users import router as user_router

router = APIRouter(prefix="/v1")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,6 @@
from kombu.exceptions import KombuError

from syncmaster.backend.api.deps import UnitOfWorkMarker
from syncmaster.backend.api.v1.transfers.utils import (
process_file_transfer_directory_path,
)
from syncmaster.backend.services import UnitOfWork, get_user
from syncmaster.db.models import Status, User
from syncmaster.db.utils import Permission
Expand Down Expand Up @@ -115,8 +112,6 @@ async def create_transfer(
if transfer_data.group_id != queue.group_id:
raise DifferentTransferAndQueueGroupError

transfer_data = process_file_transfer_directory_path(transfer_data) # type: ignore

async with unit_of_work:
transfer = await unit_of_work.transfer.create(
group_id=transfer_data.group_id,
Expand Down Expand Up @@ -316,8 +311,6 @@ async def update_transfer(
params_type=transfer_data.source_params.type,
)

transfer_data = process_file_transfer_directory_path(transfer_data) # type: ignore

async with unit_of_work:
transfer = await unit_of_work.transfer.update(
transfer=transfer,
Expand Down
2 changes: 0 additions & 2 deletions syncmaster/backend/api/v1/transfers/__init__.py

This file was deleted.

17 changes: 0 additions & 17 deletions syncmaster/backend/api/v1/transfers/utils.py

This file was deleted.

24 changes: 15 additions & 9 deletions syncmaster/schemas/v1/transfers/file/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,11 @@

from pathlib import PurePosixPath

from pydantic import BaseModel, Field, validator
from pydantic import BaseModel, Field, field_validator

from syncmaster.schemas.v1.transfers.file_format import CSV, JSON, JSONLine


def validate_directory_path(path: str) -> PurePosixPath:
return PurePosixPath(path)


# At the moment the ReadTransferSourceParams and ReadTransferTargetParams
# classes are identical but may change in the future
class ReadFileTransferSource(BaseModel):
Expand All @@ -28,20 +24,30 @@ class ReadFileTransferTarget(BaseModel):
# At the moment the CreateTransferSourceParams and CreateTransferTargetParams
# classes are identical but may change in the future
class CreateFileTransferSource(BaseModel):
directory_path: PurePosixPath
directory_path: str
file_format: CSV | JSONLine | JSON = Field(..., discriminator="type")

class Config:
arbitrary_types_allowed = True

_validate_dir_path = validator("directory_path", allow_reuse=True, pre=True)(validate_directory_path)
@field_validator("directory_path", mode="before")
@classmethod
def _directory_path_is_valid_path(cls, value):
if not PurePosixPath(value).is_absolute():
raise ValueError("Directory path must be absolute")
return value


class CreateFileTransferTarget(BaseModel):
directory_path: PurePosixPath
directory_path: str
file_format: CSV | JSONLine = Field(..., discriminator="type") # JSON FORMAT IS NOT SUPPORTED AS A TARGET !

class Config:
arbitrary_types_allowed = True

_validate_dir_path = validator("directory_path", allow_reuse=True, pre=True)(validate_directory_path)
@field_validator("directory_path", mode="before")
@classmethod
def _directory_path_is_valid_path(cls, value):
if not PurePosixPath(value).is_absolute():
raise ValueError("Directory path must be absolute")
return value
4 changes: 2 additions & 2 deletions tests/test_unit/test_transfers/test_create_transfer.py
Original file line number Diff line number Diff line change
Expand Up @@ -521,7 +521,7 @@ async def test_developer_plus_cannot_create_transfer_with_other_group_queue(
}


async def test_developer_plus_can_not_create_transfer_with_target_s3_json(
async def test_developer_plus_can_not_create_transfer_with_target_format_json(
client: AsyncClient,
two_group_connections: tuple[MockConnection, MockConnection],
session: AsyncSession,
Expand All @@ -548,7 +548,7 @@ async def test_developer_plus_can_not_create_transfer_with_target_s3_json(
"source_params": {"type": "postgres", "table_name": "source_table"},
"target_params": {
"type": "s3",
"directory_path": "some/dir",
"directory_path": "/some/dir",
"df_schema": {},
"options": {},
"file_format": {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -173,3 +173,81 @@ async def test_developer_plus_can_create_hdfs_transfer(
"strategy_params": transfer.strategy_params,
"queue_id": transfer.queue_id,
}


@pytest.mark.parametrize(
"create_connection_data",
[
{
"type": "s3",
"host": "localhost",
"port": 443,
},
],
indirect=True,
)
@pytest.mark.parametrize(
"target_source_params",
[
{
"type": "s3",
"directory_path": "some/path",
"file_format": {
"type": "csv",
},
},
],
)
async def test_cannot_create_file_transfer_with_relative_path(
client: AsyncClient,
two_group_connections: tuple[MockConnection, MockConnection],
group_queue: Queue,
mock_group: MockGroup,
target_source_params: dict,
create_connection_data: dict,
):
# Arrange
first_connection, second_connection = two_group_connections
user = mock_group.get_member_of_role(UserTestRoles.Developer)

# Act
result = await client.post(
"v1/transfers",
headers={"Authorization": f"Bearer {user.token}"},
json={
"group_id": mock_group.group.id,
"name": "new test transfer",
"description": "",
"is_scheduled": False,
"schedule": "",
"source_connection_id": first_connection.id,
"target_connection_id": second_connection.id,
"source_params": target_source_params,
"target_params": target_source_params,
"strategy_params": {"type": "full"},
"queue_id": group_queue.id,
},
)

# Assert
assert result.status_code == 422
assert result.json() == {
"detail": [
{
"ctx": {"error": {}},
"input": "some/path",
"loc": ["body", "source_params", "s3", "directory_path"],
"msg": "Value error, Directory path must be absolute",
"type": "value_error",
"url": "https://errors.pydantic.dev/2.7/v/value_error",
},
{
"ctx": {"error": {}},
"input": "some/path",
"loc": ["body", "target_params", "s3", "directory_path"],
"msg": "Value error, Directory path must be absolute",
"type": "value_error",
"url": "https://errors.pydantic.dev/2.7/v/value_error",
},
],
}