diff --git a/.github/instructions/python.instructions.md b/.github/instructions/python.instructions.md index 0527fbec2234..a7f008830277 100644 --- a/.github/instructions/python.instructions.md +++ b/.github/instructions/python.instructions.md @@ -1,28 +1,65 @@ --- applyTo: '**/*.py' --- -Provide project context and coding guidelines that AI should follow when generating code, answering questions, or reviewing changes. ## 🛠️Coding Instructions for Python in This Repository Follow these rules **strictly** when generating Python code: -### 1. Python Version +### Python Version * Use Python 3.13: Ensure all code uses features and syntax compatible with Python 3.13. -### 2. **Type Annotations** +### Type Annotations * Always use full type annotations for all functions and class attributes. * ❗ **Exception**: Do **not** add return type annotations in `test_*` functions. -### 3. **Code Style & Formatting** +### Documentation with Annotated Types + +* Use `annotated_types.doc()` for parameter and return type documentation instead of traditional docstring Args/Returns sections +* **Apply documentation only for non-obvious parameters/returns**: + - Document complex behaviors that can't be deduced from parameter name and type + - Document validation rules, side effects, or special handling + - Skip documentation for self-explanatory parameters (e.g., `engine: AsyncEngine`, `product_name: ProductName`) +* **Import**: Always add `from annotated_types import doc` when using documentation annotations + +**Examples:** +```python +from typing import Annotated +from annotated_types import doc + +async def process_users( + engine: AsyncEngine, # No doc needed - self-explanatory + filter_statuses: Annotated[ + list[Status] | None, + doc("Only returns users with these statuses") + ] = None, + limit: int = 50, # No doc needed - obvious +) -> Annotated[ + tuple[list[dict], int], + doc("(user records, total count)") +]: + """Process users with filtering. + + Raises: + ValueError: If no filters provided + """ +``` + +* **Docstring conventions**: + - Keep docstrings **concise**, focusing on overall function purpose + - Include `Raises:` section for exceptions + - Avoid repeating information already captured in type annotations + - Most information should be deducible from function name, parameter names, types, and annotations + +### Code Style & Formatting * Follow [Python Coding Conventions](../../docs/coding-conventions.md) **strictly**. * Format code with `black` and `ruff`. * Lint code with `ruff` and `pylint`. -### 4. **Library Compatibility** +### Library Compatibility Ensure compatibility with the following library versions: @@ -30,8 +67,7 @@ Ensure compatibility with the following library versions: * `pydantic` ≥ 2.x * `fastapi` ≥ 0.100 - -### 5. **Code Practices** +### Code Practices * Use `f-string` formatting for all string interpolation except for logging message strings. * Use **relative imports** within the same package/module. @@ -40,13 +76,12 @@ Ensure compatibility with the following library versions: * Place **all imports at the top** of the file. * Document functions when the code is not self-explanatory or if asked explicitly. - -### 6. **JSON Serialization** +### JSON Serialization * Prefer `json_dumps` / `json_loads` from `common_library.json_serialization` instead of the built-in `json.dumps` / `json.loads`. * When using Pydantic models, prefer methods like `model.model_dump_json()` for serialization. -### 7. **aiohttp Framework** +### aiohttp Framework * **Application Keys**: Always use `web.AppKey` for type-safe application storage instead of string keys - Define keys with specific types: `APP_MY_KEY: Final = web.AppKey("APP_MY_KEY", MySpecificType)` @@ -58,6 +93,6 @@ Ensure compatibility with the following library versions: * **Error Handling**: Use the established exception handling decorators and patterns * **Route Definitions**: Use `web.RouteTableDef()` and organize routes logically within modules -### 8. **Running tests** +### Running tests * Use `--keep-docker-up` flag when testing to keep docker containers up between sessions. * Always activate the python virtual environment before running pytest. diff --git a/api/specs/web-server/_common.py b/api/specs/web-server/_common.py index 4ca4ef69e16b..bc74e765f6a5 100644 --- a/api/specs/web-server/_common.py +++ b/api/specs/web-server/_common.py @@ -60,7 +60,7 @@ def as_query(model_class: type[BaseModel]) -> type[BaseModel]: for field_name, field_info in model_class.model_fields.items(): field_default = field_info.default - assert not field_info.default_factory # nosec + assert not field_info.default_factory, f"got {field_info=}" # nosec query_kwargs = { "alias": field_info.alias, "title": field_info.title, diff --git a/api/specs/web-server/_users_admin.py b/api/specs/web-server/_users_admin.py index 2e4ff647c3cc..fba3a8df3d68 100644 --- a/api/specs/web-server/_users_admin.py +++ b/api/specs/web-server/_users_admin.py @@ -7,14 +7,14 @@ from enum import Enum from typing import Annotated -from _common import as_query -from fastapi import APIRouter, Depends, status +from fastapi import APIRouter, Depends, Query, status from models_library.api_schemas_webserver.users import ( + PageQueryParameters, UserAccountApprove, UserAccountGet, UserAccountReject, UserAccountSearchQueryParams, - UsersAccountListQueryParams, + UsersForAdminListFilter, ) from models_library.generics import Envelope from models_library.rest_pagination import Page @@ -26,13 +26,26 @@ _extra_tags: list[str | Enum] = ["admin"] +# NOTE: I still do not have a clean solution for this +# +class _Q(UsersForAdminListFilter, PageQueryParameters): ... + + @router.get( "/admin/user-accounts", response_model=Page[UserAccountGet], tags=_extra_tags, ) async def list_users_accounts( - _query: Annotated[as_query(UsersAccountListQueryParams), Depends()], + _query: Annotated[_Q, Depends()], + order_by: Annotated[ + str, + Query( + title="Order By", + description="Comma-separated list of fields for ordering (prefix with '-' for descending).", + example="-created_at,name", + ), + ] = "", ): ... diff --git a/packages/models-library/src/models_library/api_schemas_webserver/users.py b/packages/models-library/src/models_library/api_schemas_webserver/users.py index 7412e2b8df71..a4f45a07c4e3 100644 --- a/packages/models-library/src/models_library/api_schemas_webserver/users.py +++ b/packages/models-library/src/models_library/api_schemas_webserver/users.py @@ -1,7 +1,7 @@ import re from datetime import date, datetime from enum import Enum -from typing import Annotated, Any, Literal, Self +from typing import Annotated, Any, Literal, Self, TypeAlias import annotated_types from common_library.basic_types import DEFAULT_FACTORY @@ -27,6 +27,7 @@ from ..groups import AccessRightsDict, Group, GroupID, GroupsByTypeTuple, PrimaryGroupID from ..products import ProductName from ..rest_base import RequestParameters +from ..rest_ordering import OrderingQueryParams from ..string_types import ( GlobPatternSafeStr, SearchPatternSafeStr, @@ -317,7 +318,14 @@ class UsersForAdminListFilter(Filters): model_config = ConfigDict(extra="forbid") -class UsersAccountListQueryParams(UsersForAdminListFilter, PageQueryParameters): ... +UserAccountOrderFields: TypeAlias = Literal["email", "created_at"] + + +class UsersAccountListQueryParams( + UsersForAdminListFilter, + PageQueryParameters, + OrderingQueryParams[UserAccountOrderFields], +): ... class _InvitationDetails(InputSchema): @@ -338,7 +346,7 @@ class UserAccountSearchQueryParams(RequestParameters): email: Annotated[ GlobPatternSafeStr | None, Field( - description="complete or glob pattern for an email", + description="complete or glob pattern for an email (case insensitive)", ), ] = None primary_group_id: Annotated[ @@ -350,7 +358,7 @@ class UserAccountSearchQueryParams(RequestParameters): user_name: Annotated[ GlobPatternSafeStr | None, Field( - description="complete or glob pattern for a username", + description="complete or glob pattern for a username (case insensitive)", ), ] = None diff --git a/packages/models-library/src/models_library/invitations.py b/packages/models-library/src/models_library/invitations.py index adb1545f0b6d..cadc4667b5ec 100644 --- a/packages/models-library/src/models_library/invitations.py +++ b/packages/models-library/src/models_library/invitations.py @@ -1,7 +1,14 @@ -from datetime import datetime, timezone -from typing import Final +from datetime import UTC, datetime +from typing import Annotated, Final -from pydantic import BaseModel, EmailStr, Field, PositiveInt, field_validator +from pydantic import ( + AfterValidator, + BaseModel, + EmailStr, + Field, + PositiveInt, + field_validator, +) from .products import ProductName @@ -11,29 +18,40 @@ class InvitationInputs(BaseModel): """Input data necessary to create an invitation""" - issuer: str = Field( - ..., - description="Identifies who issued the invitation. E.g. an email, a service name etc. NOTE: it will be trimmed if exceeds maximum", - min_length=1, - max_length=_MAX_LEN, - ) - guest: EmailStr = Field( - ..., - description="Invitee's email. Note that the registration can ONLY be used with this email", - ) - trial_account_days: PositiveInt | None = Field( - default=None, - description="If set, this invitation will activate a trial account." - "Sets the number of days from creation until the account expires", - ) - extra_credits_in_usd: PositiveInt | None = Field( - default=None, - description="If set, the account's primary wallet will add extra credits corresponding to this ammount in USD", - ) - product: ProductName | None = Field( - default=None, - description="If None, it will use INVITATIONS_DEFAULT_PRODUCT", - ) + issuer: Annotated[ + str, + Field( + description="Identifies who issued the invitation. E.g. an email, a service name etc. NOTE: it will be trimmed if exceeds maximum", + min_length=1, + max_length=_MAX_LEN, + ), + ] + guest: Annotated[ + EmailStr, + AfterValidator(lambda v: v.lower()), + Field( + description="Invitee's email. Note that the registration can ONLY be used with this email", + ), + ] + trial_account_days: Annotated[ + PositiveInt | None, + Field( + description="If set, this invitation will activate a trial account." + "Sets the number of days from creation until the account expires", + ), + ] = None + extra_credits_in_usd: Annotated[ + PositiveInt | None, + Field( + description="If set, the account's primary wallet will add extra credits corresponding to this ammount in USD", + ), + ] = None + product: Annotated[ + ProductName | None, + Field( + description="If None, it will use INVITATIONS_DEFAULT_PRODUCT", + ), + ] = None @field_validator("issuer", mode="before") @classmethod @@ -44,10 +62,10 @@ def trim_long_issuers_to_max_length(cls, v): class InvitationContent(InvitationInputs): - """Data in an invitation""" + """Data within an invitation""" # avoid using default to mark exactly the time - created: datetime = Field(..., description="Timestamp for creation") + created: Annotated[datetime, Field(description="Timestamp for creation")] def as_invitation_inputs(self) -> InvitationInputs: return self.model_validate( @@ -62,6 +80,6 @@ def create_from_inputs( kwargs = invitation_inputs.model_dump(exclude_none=True) kwargs.setdefault("product", default_product) return cls( - created=datetime.now(tz=timezone.utc), + created=datetime.now(tz=UTC), **kwargs, ) diff --git a/packages/models-library/src/models_library/list_operations.py b/packages/models-library/src/models_library/list_operations.py new file mode 100644 index 000000000000..263cc129ccaa --- /dev/null +++ b/packages/models-library/src/models_library/list_operations.py @@ -0,0 +1,70 @@ +"""List operation models and helpers + +- Ordering: https://google.aip.dev/132#ordering + + +""" + +from enum import Enum +from typing import TYPE_CHECKING, Annotated, Generic, TypeVar + +from annotated_types import doc +from pydantic import BaseModel + + +class OrderDirection(str, Enum): + ASC = "asc" + DESC = "desc" + + +if TYPE_CHECKING: + from typing import Protocol + + class LiteralField(Protocol): + """Protocol for Literal string types""" + + def __str__(self) -> str: ... + + TField = TypeVar("TField", bound=LiteralField) +else: + TField = TypeVar("TField", bound=str) + + +class OrderClause(BaseModel, Generic[TField]): + field: TField + direction: OrderDirection = OrderDirection.ASC + + +def check_ordering_list( + order_by: Annotated[ + list[tuple[TField, OrderDirection]], + doc( + "Duplicates with same direction dropped, conflicting directions raise ValueError" + ), + ], +) -> Annotated[ + list[tuple[TField, OrderDirection]], + doc("Duplicates removed, preserving first occurrence order"), +]: + """Validates ordering list and removes duplicate entries. + + Raises: + ValueError: If a field appears with conflicting directions + """ + seen_fields: dict[TField, OrderDirection] = {} + unique_order_by = [] + + for field, direction in order_by: + if field in seen_fields: + # Field already seen - check if direction matches + if seen_fields[field] != direction: + msg = f"Field '{field}' appears with conflicting directions: {seen_fields[field].value} and {direction.value}" + raise ValueError(msg) + # Same field and direction - skip duplicate + continue + + # First time seeing this field + seen_fields[field] = direction + unique_order_by.append((field, direction)) + + return unique_order_by diff --git a/packages/models-library/src/models_library/rest_ordering.py b/packages/models-library/src/models_library/rest_ordering.py index 1fdd9d6cfed4..7e2d829d926c 100644 --- a/packages/models-library/src/models_library/rest_ordering.py +++ b/packages/models-library/src/models_library/rest_ordering.py @@ -1,32 +1,41 @@ -from enum import Enum -from typing import Annotated +from typing import Annotated, Generic +from common_library.basic_types import DEFAULT_FACTORY from common_library.json_serialization import json_dumps -from pydantic import BaseModel, BeforeValidator, ConfigDict, Field, field_validator +from pydantic import ( + BaseModel, + BeforeValidator, + ConfigDict, + Field, + field_validator, +) from .basic_types import IDStr +from .list_operations import OrderClause, OrderDirection, TField, check_ordering_list from .rest_base import RequestParameters -from .utils.common_validators import parse_json_pre_validator +from .utils.common_validators import ( + parse_json_pre_validator, +) - -class OrderDirection(str, Enum): - ASC = "asc" - DESC = "desc" +__all__: tuple[str, ...] = ("OrderDirection",) class OrderBy(BaseModel): - # Based on https://google.aip.dev/132#ordering - field: IDStr = Field(..., description="field name identifier") - direction: OrderDirection = Field( - default=OrderDirection.ASC, - description=( - f"As [A,B,C,...] if `{OrderDirection.ASC.value}`" - f" or [Z,Y,X, ...] if `{OrderDirection.DESC.value}`" + # NOTE: use instead OrderClause[TField] where TField is Literal of valid fields + field: Annotated[IDStr, Field(description="field name identifier")] + direction: Annotated[ + OrderDirection, + Field( + description=( + f"As [A,B,C,...] if `{OrderDirection.ASC.value}`" + f" or [Z,Y,X, ...] if `{OrderDirection.DESC.value}`" + ) ), - ) + ] = OrderDirection.ASC class _BaseOrderQueryParams(RequestParameters): + # Use OrderingQueryParams instead for more flexible ordering order_by: OrderBy @@ -91,12 +100,10 @@ def _check_ordering_field_and_map(cls, v): return _ordering_fields_api_to_column_map.get(v) or v assert "json_schema_extra" in _OrderBy.model_config # nosec - assert isinstance(_OrderBy.model_config["json_schema_extra"], dict) # nosec - assert isinstance( # nosec - _OrderBy.model_config["json_schema_extra"]["examples"], list - ) - order_by_example = _OrderBy.model_config["json_schema_extra"]["examples"][0] + + order_by_example = _OrderBy.model_json_schema()["examples"][0] order_by_example_json = json_dumps(order_by_example) + assert _OrderBy.model_validate(order_by_example), "Example is invalid" # nosec converted_default = _OrderBy.model_validate( @@ -104,17 +111,82 @@ def _check_ordering_field_and_map(cls, v): default.model_dump() ) - class _OrderQueryParams(_BaseOrderQueryParams): - order_by: Annotated[_OrderBy, BeforeValidator(parse_json_pre_validator)] = ( + class _OrderJsonQueryParams(_BaseOrderQueryParams): + order_by: Annotated[ + _OrderBy, + BeforeValidator(parse_json_pre_validator), Field( - default=converted_default, description=( f"Order by field (`{msg_field_options}`) and direction (`{msg_direction_options}`). " f"The default sorting order is `{json_dumps(default)}`." ), examples=[order_by_example], json_schema_extra={"example_json": order_by_example_json}, - ) - ) + ), + ] = converted_default + + return _OrderJsonQueryParams + + +def _parse_order_by(v): + if not v: + return [] + + if isinstance(v, list): + v = ",".join(v) + + if not isinstance(v, str): + msg = "order_by must be a string" + raise TypeError(msg) - return _OrderQueryParams + # 1. from comma-separated string to list of OrderClause + clauses = [] + for t in v.split(","): + token = t.strip() + if not token: + continue + if token.startswith("-"): + clauses.append((token[1:], OrderDirection.DESC)) + elif token.startswith("+"): + clauses.append((token[1:], OrderDirection.ASC)) + else: + clauses.append((token, OrderDirection.ASC)) + + # 2. check for duplicates and conflicting directions + return [ + {"field": field, "direction": direction} + for field, direction in check_ordering_list(clauses) + ] + + +class OrderingQueryParams(BaseModel, Generic[TField]): + """ + This class is designed to parse query parameters for ordering results in an API request. + + It supports multiple ordering clauses and allows for flexible sorting options. + + NOTE: It only parses strings and validates into list[OrderClause[TField]] + where TField is a type variable representing valid field names. + + + For example: + + /my/path?order_by=field1,-field2,+field3 + + would sort by field1 ascending, field2 descending, and field3 ascending. + """ + + order_by: Annotated[ + list[OrderClause[TField]], + BeforeValidator(_parse_order_by), + Field(default_factory=list), + ] = DEFAULT_FACTORY + + model_config = ConfigDict( + json_schema_extra={ + "examples": [ + {"order_by": "-created_at,name,+gender"}, + {"order_by": ""}, + ], + } + ) diff --git a/packages/models-library/src/models_library/string_types.py b/packages/models-library/src/models_library/string_types.py index cc7a2b4dcc5d..05d4c4f75c6d 100644 --- a/packages/models-library/src/models_library/string_types.py +++ b/packages/models-library/src/models_library/string_types.py @@ -203,6 +203,7 @@ def validate_input_xss_safety(value: str) -> str: max_length=200, strip_whitespace=True, pattern=r"^[A-Za-z0-9 ._\*@-]*$", # Allow alphanumeric, spaces, dots, underscores, hyphens, asterisks and at signs + to_lower=True # make case-insensitive ), AfterValidator(validate_input_xss_safety), ] @@ -215,6 +216,7 @@ def validate_input_xss_safety(value: str) -> str: min_length=1, max_length=200, pattern=r"^[A-Za-z0-9 ._@-]*$", # Allow alphanumeric, spaces, dots, underscores, hyphens, and at signs + to_lower=True # make case-insensitive ), AfterValidator(validate_input_xss_safety), annotated_types.doc( diff --git a/packages/models-library/tests/test_list_operations.py b/packages/models-library/tests/test_list_operations.py new file mode 100644 index 000000000000..61ff91aa5d4d --- /dev/null +++ b/packages/models-library/tests/test_list_operations.py @@ -0,0 +1,70 @@ +import pytest +from models_library.list_operations import ( + OrderDirection, + check_ordering_list, +) + + +def test_check_ordering_list_drops_duplicates_silently(): + """Test that check_ordering_list silently drops duplicate entries with same field and direction""" + + # Input with duplicates (same field and direction) + order_by = [ + ("email", OrderDirection.ASC), + ("created", OrderDirection.DESC), + ("email", OrderDirection.ASC), # Duplicate - should be dropped + ("name", OrderDirection.ASC), + ("created", OrderDirection.DESC), # Duplicate - should be dropped + ] + + result = check_ordering_list(order_by) + + # Should return unique entries preserving order of first occurrence + expected = [ + ("email", OrderDirection.ASC), + ("created", OrderDirection.DESC), + ("name", OrderDirection.ASC), + ] + + assert result == expected + + +def test_check_ordering_list_raises_for_conflicting_directions(): + """Test that check_ordering_list raises ValueError when same field has different directions""" + + # Input with same field but different directions + order_by = [ + ("email", OrderDirection.ASC), + ("created", OrderDirection.DESC), + ("email", OrderDirection.DESC), # Conflict! Same field, different direction + ] + + with pytest.raises(ValueError, match="conflicting directions") as exc_info: + check_ordering_list(order_by) + + error_msg = str(exc_info.value) + assert "Field 'email' appears with conflicting directions" in error_msg + assert "asc" in error_msg + assert "desc" in error_msg + + +def test_check_ordering_list_empty_input(): + """Test that check_ordering_list handles empty input correctly""" + + result = check_ordering_list([]) + assert result == [] + + +def test_check_ordering_list_no_duplicates(): + """Test that check_ordering_list works correctly when there are no duplicates""" + + order_by = [ + ("email", OrderDirection.ASC), + ("created", OrderDirection.DESC), + ("name", OrderDirection.ASC), + ] + + result = check_ordering_list(order_by) + + # Should return the same list + assert result == order_by diff --git a/packages/models-library/tests/test_rest_ordering.py b/packages/models-library/tests/test_rest_ordering.py index 33e1ad654d10..6feda4731df6 100644 --- a/packages/models-library/tests/test_rest_ordering.py +++ b/packages/models-library/tests/test_rest_ordering.py @@ -1,11 +1,14 @@ import pickle +from typing import Literal import pytest from common_library.json_serialization import json_dumps from models_library.basic_types import IDStr from models_library.rest_ordering import ( OrderBy, + OrderClause, OrderDirection, + OrderingQueryParams, create_ordering_query_model_class, ) from pydantic import ( @@ -236,3 +239,47 @@ def test_ordering_query_parse_json_pre_validator(): assert error["loc"] == ("order_by",) assert error["type"] == "value_error" assert error["input"] == bad_json_value + + +def test_ordering_query_params_parsing(): + """Test OrderingQueryParams parsing from URL query format like ?order_by=-created_at,name,+gender""" + + # Define allowed fields using Literal type + ValidField = Literal["created_at", "name", "gender"] + + class TestOrderingParams(OrderingQueryParams[ValidField]): + pass + + # Test parsing from comma-separated string + params = TestOrderingParams.model_validate({"order_by": "-created_at,name,+gender"}) + + assert params.order_by == [ + OrderClause[ValidField](field="created_at", direction=OrderDirection.DESC), + OrderClause[ValidField](field="name", direction=OrderDirection.ASC), + OrderClause[ValidField](field="gender", direction=OrderDirection.ASC), + ] + + +def test_ordering_query_params_validation_error_with_invalid_fields(): + """Test that OrderingQueryParams raises ValidationError when invalid fields are used""" + + # Define allowed fields using Literal type + ValidField = Literal["created_at", "name"] + + class TestOrderingParams(OrderingQueryParams[ValidField]): + pass + + # Test with invalid field should raise ValidationError + with pytest.raises(ValidationError) as err_info: + TestOrderingParams.model_validate( + {"order_by": "-created_at,invalid_field,name"} + ) + + # Verify the validation error details + exc = err_info.value + assert exc.error_count() == 1 + + error = exc.errors()[0] + assert error["loc"] == ("order_by", 1, "field") + assert error["type"] == "literal_error" + assert error["input"] == "invalid_field" diff --git a/packages/notifications-library/src/notifications_library/templates/on_account_requested.email.content.html b/packages/notifications-library/src/notifications_library/templates/on_account_requested.email.content.html index 5a317574c958..d922a71ad6c2 100644 --- a/packages/notifications-library/src/notifications_library/templates/on_account_requested.email.content.html +++ b/packages/notifications-library/src/notifications_library/templates/on_account_requested.email.content.html @@ -1,13 +1,14 @@ {% extends 'base.html' %} {% block title %} {% include 'on_account_requested.email.subject.txt' %} {% endblock %} {% block content %} -
Dear Support team +
Dear Support team
We have received the following request form for an account in :