diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md index b022dc72..4d191f21 100644 --- a/RELEASE_NOTES.md +++ b/RELEASE_NOTES.md @@ -57,6 +57,14 @@ A well-formed `DeliveryArea` has a non-empty `code` and a specified (non-`UNSPECIFIED`) `code_type`. Constructing one with invalid data currently emits a `DeprecationWarning`; a future release will replace the warning with a hard `ValueError`. To opt into the upcoming behavior right now, pass `_raise_on_invalid=True` to the constructor. Prefer `delivery_area_from_proto2` to load delivery areas from the wire — malformed messages become `InvalidDeliveryArea` instances instead. +* `frequenz.client.common.pagination.proto.v1alpha8.pagination_info_from_proto` is now deprecated; use `pagination_info_from_proto2` instead. + + The new converter returns `PaginationInfo | InvalidPaginationInfo` and surfaces malformed wire data at the type level rather than raising a `ValueError` when `total_items` is negative. It also reads `next_page_token` through `HasField()`, so a token the server actually sent is preserved even when it is empty; only an unset token becomes `None`. The old converter continues to work but emits a `DeprecationWarning`. + +* `frequenz.client.common.pagination.PaginationInfo` now raises a `ValueError` when `total_items` is negative, which v0.4.0 accepted silently. + + A count of items can't be negative. Use `pagination_info_from_proto2` to load pagination information from the wire — malformed messages become `InvalidPaginationInfo` instances instead of raising. + * `frequenz.client.common.metrics.proto.v1alpha8.bounds_from_proto` is now deprecated; use `bounds_from_proto2` instead. The new converter returns `Bounds | InvalidBounds` and surfaces malformed wire data at the type level rather than raising a `ValueError` when `lower > upper`. The old converter continues to work but emits a `DeprecationWarning`. @@ -163,6 +171,14 @@ * Added `frequenz.client.common.grid.proto.v1alpha8.delivery_area_from_proto2` returning `DeliveryArea | InvalidDeliveryArea`. This is the replacement for the now-deprecated `delivery_area_from_proto`. +* Added a new pagination-info class hierarchy: + + * `frequenz.client.common.pagination.BasePaginationInfo` — abstract common supertype of the two concrete leaves; not directly instantiable. + * `frequenz.client.common.pagination.PaginationInfo` — well-formed pagination information (retroactively made a subclass of `BasePaginationInfo`), now with a compact `__str__` rendering as `items=100,next=token`. + * `frequenz.client.common.pagination.InvalidPaginationInfo` — malformed wire data; same fields as `PaginationInfo` with no invariants enforced, rendering a negative count as `items=`. + +* Added `frequenz.client.common.pagination.proto.v1alpha8.pagination_info_from_proto2` returning `PaginationInfo | InvalidPaginationInfo`. This is the replacement for the now-deprecated `pagination_info_from_proto`. + * Added a new `frequenz.client.common.microgrid.Lifetime` type together with the `frequenz.client.common.microgrid.proto.v1alpha8.lifetime_from_proto` conversion function. * Added `frequenz.client.common.metrics.proto.v1alpha8.bounds_from_proto2` returning `Bounds | InvalidBounds`. This is the replacement for the now-deprecated `bounds_from_proto`. diff --git a/docs/client-developer-guide/shipped-converters.md b/docs/client-developer-guide/shipped-converters.md index f81ec20c..80340b3c 100644 --- a/docs/client-developer-guide/shipped-converters.md +++ b/docs/client-developer-guide/shipped-converters.md @@ -17,7 +17,7 @@ Import the package that matches the messages your code receives. | [`metrics`][frequenz.client.common.metrics.proto.v1alpha8] | Metric samples ([`metrics_pb2.MetricSample`][frequenz.api.common.v1alpha8.metrics.metrics_pb2.MetricSample]), connections, aggregate values, bounds and bounds sets, plus metric and connection-category enum conversion. | | [`microgrid`][frequenz.client.common.microgrid.proto.v1alpha8] | Microgrids ([`microgrid_pb2.Microgrid`][frequenz.api.common.v1alpha8.microgrid.microgrid_pb2.Microgrid]) and lifetimes through [`microgrid_from_proto`][frequenz.client.common.microgrid.proto.v1alpha8.microgrid_from_proto] and [`lifetime_from_proto`][frequenz.client.common.microgrid.proto.v1alpha8.lifetime_from_proto]. | | [`microgrid.electrical_components`][frequenz.client.common.microgrid.electrical_components.proto.v1alpha8] | Electrical-component classes ([`electrical_components_pb2.ElectricalComponent`][frequenz.api.common.v1alpha8.microgrid.electrical_components.electrical_components_pb2.ElectricalComponent]), component instances and connections, plus category, diagnostic-code, and state-code enums. | -| [`pagination`][frequenz.client.common.pagination.proto.v1alpha8] | Pagination information ([`pagination_info_pb2.PaginationInfo`][frequenz.api.common.v1alpha8.pagination.pagination_info_pb2.PaginationInfo]) in both directions through [`pagination_info_from_proto`][frequenz.client.common.pagination.proto.v1alpha8.pagination_info_from_proto] and [`pagination_info_to_proto`][frequenz.client.common.pagination.proto.v1alpha8.pagination_info_to_proto]. | +| [`pagination`][frequenz.client.common.pagination.proto.v1alpha8] | Pagination information ([`pagination_info_pb2.PaginationInfo`][frequenz.api.common.v1alpha8.pagination.pagination_info_pb2.PaginationInfo]) in both directions through [`pagination_info_from_proto2`][frequenz.client.common.pagination.proto.v1alpha8.pagination_info_from_proto2] and [`pagination_info_to_proto`][frequenz.client.common.pagination.proto.v1alpha8.pagination_info_to_proto]. | | [`streaming`][frequenz.client.common.streaming.proto.v1alpha8] | Streaming events ([`event_pb2.Event`][frequenz.api.common.v1alpha8.streaming.event_pb2.Event]) in both directions through [`event_from_proto`][frequenz.client.common.streaming.proto.v1alpha8.event_from_proto] and [`event_to_proto`][frequenz.client.common.streaming.proto.v1alpha8.event_to_proto]. | | [`types`][frequenz.client.common.types.proto.v1alpha8] | Locations ([`location_pb2.Location`][frequenz.api.common.v1alpha8.types.location_pb2.Location]) through [`location_from_proto`][frequenz.client.common.types.proto.v1alpha8.location_from_proto]. | diff --git a/docs/user-guide/overview.md b/docs/user-guide/overview.md index ed238732..e57c4280 100644 --- a/docs/user-guide/overview.md +++ b/docs/user-guide/overview.md @@ -90,7 +90,9 @@ invalid fields. ## Pagination [`PaginationInfo`][frequenz.client.common.pagination.PaginationInfo] carries -pagination details. +pagination details; +[`InvalidPaginationInfo`][frequenz.client.common.pagination.InvalidPaginationInfo] +preserves a malformed item count. For every field, method, and remaining wrapper type, see the [API Reference](../reference/frequenz/client/common/index.md). The diff --git a/src/frequenz/client/common/pagination/__init__.py b/src/frequenz/client/common/pagination/__init__.py index b6bf160c..cb267d6a 100644 --- a/src/frequenz/client/common/pagination/__init__.py +++ b/src/frequenz/client/common/pagination/__init__.py @@ -3,6 +3,14 @@ """Pagination types used by common clients.""" -from ._pagination_info import PaginationInfo +from ._pagination_info import ( + BasePaginationInfo, + InvalidPaginationInfo, + PaginationInfo, +) -__all__ = ["PaginationInfo"] +__all__ = [ + "BasePaginationInfo", + "InvalidPaginationInfo", + "PaginationInfo", +] diff --git a/src/frequenz/client/common/pagination/_pagination_info.py b/src/frequenz/client/common/pagination/_pagination_info.py index 77978695..eadfab57 100644 --- a/src/frequenz/client/common/pagination/_pagination_info.py +++ b/src/frequenz/client/common/pagination/_pagination_info.py @@ -4,21 +4,85 @@ """Pagination information used by common clients.""" from dataclasses import dataclass +from typing import Any, Self @dataclass(frozen=True, kw_only=True) -class PaginationInfo: - """Information about the pagination of a list request.""" +class BasePaginationInfo: + """A base class for well-formed and malformed pagination information. + + This class cannot be instantiated directly. Use + [`PaginationInfo`][..PaginationInfo] for well-formed pagination + information or [`InvalidPaginationInfo`][..InvalidPaginationInfo] to + preserve malformed wire data. + """ total_items: int """The total number of items that match the request.""" next_page_token: str | None = None - """The token identifying the next page of results.""" + """The token identifying the next page of results. + + If `None`, the server did not send a token, so there are no more pages. + """ + + # pylint: disable-next=unused-argument + def __new__(cls, *args: Any, **kwargs: Any) -> Self: + """Prevent instantiation of this class.""" + if cls is BasePaginationInfo: + raise TypeError(f"Cannot instantiate {cls.__name__} directly") + return super().__new__(cls) + + +@dataclass(frozen=True, kw_only=True) +class PaginationInfo(BasePaginationInfo): + """Information about the pagination of a list request. + + The [`total_items`][.total_items] count is a number of items, so it can + never be negative. Pagination information built from a wire message that + breaks this rule is an + [`InvalidPaginationInfo`][..InvalidPaginationInfo] instead, so code + holding a `PaginationInfo` can use the count in arithmetic without + checking it first. + + Note: + Raises a `ValueError` if [`total_items`][.total_items] is negative. + Use [`InvalidPaginationInfo`][..InvalidPaginationInfo] to represent + malformed pagination data received from the wire. + """ def __post_init__(self) -> None: - """Validate pagination information.""" + """Validate pagination information. + + Raises: + ValueError: If [`total_items`][..total_items] is negative. + """ if self.total_items < 0: raise ValueError( f"total_items must be non-negative, not {self.total_items}" ) + + def __str__(self) -> str: + """Return a compact string representation of this pagination information.""" + return f"items={self.total_items},next={self.next_page_token}" + + +@dataclass(frozen=True, kw_only=True) +class InvalidPaginationInfo(BasePaginationInfo): + """Pagination information with malformed data received from the wire. + + This class preserves pagination data that fails the invariants required + for a well-formed [`PaginationInfo`][..PaginationInfo], allowing callers + to inspect the raw values without accidentally using them as a count. + + This class does not enforce any invariants on construction. + """ + + def __str__(self) -> str: + """Return a compact string representation of this invalid pagination info.""" + total_items = ( + f"" + if self.total_items < 0 + else str(self.total_items) + ) + return f"items={total_items},next={self.next_page_token}" diff --git a/src/frequenz/client/common/pagination/proto/v1alpha8/__init__.py b/src/frequenz/client/common/pagination/proto/v1alpha8/__init__.py index ab455c3e..b43a7c8a 100644 --- a/src/frequenz/client/common/pagination/proto/v1alpha8/__init__.py +++ b/src/frequenz/client/common/pagination/proto/v1alpha8/__init__.py @@ -5,10 +5,12 @@ from ._pagination_info import ( pagination_info_from_proto, + pagination_info_from_proto2, pagination_info_to_proto, ) __all__ = [ "pagination_info_from_proto", + "pagination_info_from_proto2", "pagination_info_to_proto", ] diff --git a/src/frequenz/client/common/pagination/proto/v1alpha8/_pagination_info.py b/src/frequenz/client/common/pagination/proto/v1alpha8/_pagination_info.py index 897c4489..f510b2ec 100644 --- a/src/frequenz/client/common/pagination/proto/v1alpha8/_pagination_info.py +++ b/src/frequenz/client/common/pagination/proto/v1alpha8/_pagination_info.py @@ -6,18 +6,36 @@ from frequenz.api.common.v1alpha8.pagination.pagination_info_pb2 import ( PaginationInfo as PaginationInfoPb, ) +from typing_extensions import deprecated -from ....pagination import PaginationInfo +from ....pagination import InvalidPaginationInfo, PaginationInfo -def pagination_info_from_proto(message: PaginationInfoPb) -> PaginationInfo: +@deprecated( + "frequenz.client.common.pagination.proto.v1alpha8.pagination_info_from_proto " + "is deprecated. Use " + "frequenz.client.common.pagination.proto.v1alpha8.pagination_info_from_proto2 " + "instead." +) +def pagination_info_from_proto( # noqa: DOC502 + message: PaginationInfoPb, +) -> PaginationInfo: """Convert a protobuf message to a [`PaginationInfo`][....PaginationInfo] object. + Warning: Deprecated + Use [`pagination_info_from_proto2`][..pagination_info_from_proto2] + instead. The new converter distinguishes well-formed from malformed + data at the type level (`PaginationInfo | InvalidPaginationInfo`) + rather than raising a `ValueError` when the invariant fires. + Args: message: The protobuf message to convert. Returns: The corresponding [`PaginationInfo`][....PaginationInfo] object. + + Raises: + ValueError: If the message carries a negative `total_items`. """ return PaginationInfo( total_items=message.total_items, @@ -25,6 +43,40 @@ def pagination_info_from_proto(message: PaginationInfoPb) -> PaginationInfo: ) +def pagination_info_from_proto2( + message: PaginationInfoPb, +) -> PaginationInfo | InvalidPaginationInfo: + """Convert a protobuf message to pagination information, preserving malformed data. + + Unlike [`pagination_info_from_proto`][..pagination_info_from_proto], the + token is read through + [`HasField()`][google.protobuf.message.Message.HasField], so a token the + server actually sent is preserved even when it is empty; only an unset + token becomes `None`. + + Args: + message: The protobuf message to convert. + + Returns: + A [`PaginationInfo`][....PaginationInfo] when the wire data is + well-formed, or an + [`InvalidPaginationInfo`][....InvalidPaginationInfo] preserving a + `total_items` count that violates `total_items >= 0`. + """ + next_page_token = ( + message.next_page_token if message.HasField("next_page_token") else None + ) + try: + return PaginationInfo( + total_items=message.total_items, next_page_token=next_page_token + ) + except ValueError: + pass + return InvalidPaginationInfo( + total_items=message.total_items, next_page_token=next_page_token + ) + + def pagination_info_to_proto(info: PaginationInfo) -> PaginationInfoPb: """Convert a [`PaginationInfo`][....PaginationInfo] object to a protobuf message. diff --git a/tests/pagination/proto/v1alpha8/test_pagination_info.py b/tests/pagination/proto/v1alpha8/test_pagination_info.py index 5d23d699..9230b803 100644 --- a/tests/pagination/proto/v1alpha8/test_pagination_info.py +++ b/tests/pagination/proto/v1alpha8/test_pagination_info.py @@ -3,32 +3,50 @@ """Tests for pagination info protobuf v1alpha8 conversions.""" +from typing import cast + +import pytest from frequenz.api.common.v1alpha8.pagination import pagination_info_pb2 -from frequenz.client.common.pagination import PaginationInfo +from frequenz.client.common.pagination import InvalidPaginationInfo, PaginationInfo from frequenz.client.common.pagination.proto.v1alpha8 import ( pagination_info_from_proto, + pagination_info_from_proto2, pagination_info_to_proto, ) -def test_pagination_info_from_proto_with_token() -> None: +def test_from_proto_with_token() -> None: """Test converting a protobuf PaginationInfo with a token.""" proto = pagination_info_pb2.PaginationInfo(total_items=100, next_page_token="token") - info = pagination_info_from_proto(proto) + with pytest.deprecated_call(match="pagination_info_from_proto2"): + info = pagination_info_from_proto(proto) assert info.total_items == 100 assert info.next_page_token == "token" -def test_pagination_info_from_proto_empty_token() -> None: +def test_from_proto_empty_token() -> None: """Test converting an empty protobuf token to None.""" proto = pagination_info_pb2.PaginationInfo(total_items=100, next_page_token="") - info = pagination_info_from_proto(proto) + with pytest.deprecated_call(match="pagination_info_from_proto2"): + info = pagination_info_from_proto(proto) assert info.total_items == 100 assert info.next_page_token is None -def test_pagination_info_to_proto_with_token() -> None: +def test_from_proto_emits_deprecation_warning() -> None: + """`pagination_info_from_proto` itself is deprecated and warns on call.""" + proto = pagination_info_pb2.PaginationInfo(total_items=1) + with pytest.deprecated_call( + match=r"^frequenz\.client\.common\.pagination\.proto\.v1alpha8\." + r"pagination_info_from_proto is deprecated\. Use " + r"frequenz\.client\.common\.pagination\.proto\.v1alpha8\." + r"pagination_info_from_proto2 instead\.$" + ): + pagination_info_from_proto(proto) + + +def test_to_proto_with_token() -> None: """Test converting a PaginationInfo with a token to protobuf.""" info = PaginationInfo(total_items=100, next_page_token="token") proto = pagination_info_to_proto(info) @@ -36,9 +54,76 @@ def test_pagination_info_to_proto_with_token() -> None: assert proto.next_page_token == "token" -def test_pagination_info_roundtrip() -> None: +def test_roundtrip() -> None: """Test round-tripping PaginationInfo to protobuf and back.""" info = PaginationInfo(total_items=100, next_page_token="token") proto = pagination_info_to_proto(info) - roundtripped_info = pagination_info_from_proto(proto) + roundtripped_info = pagination_info_from_proto2(proto) assert roundtripped_info == info + + +def test_from_proto2_with_token() -> None: + """A well-formed message with a token becomes a `PaginationInfo`.""" + proto = pagination_info_pb2.PaginationInfo(total_items=100, next_page_token="token") + info = pagination_info_from_proto2(proto) + assert isinstance(info, PaginationInfo) + assert info.total_items == 100 + assert info.next_page_token == "token" + + +def test_from_proto2_unset_token() -> None: + """An unset token becomes `None`.""" + proto = pagination_info_pb2.PaginationInfo(total_items=100) + info = pagination_info_from_proto2(proto) + assert isinstance(info, PaginationInfo) + assert info.total_items == 100 + assert info.next_page_token is None + + +def test_from_proto2_keeps_explicitly_empty_token() -> None: + """A token the server sent is kept even when it is empty.""" + proto = pagination_info_pb2.PaginationInfo(total_items=100, next_page_token="") + info = pagination_info_from_proto2(proto) + assert isinstance(info, PaginationInfo) + assert info.next_page_token == "" + + +def test_from_proto2_zero_total_items() -> None: + """A zero count is well-formed.""" + info = pagination_info_from_proto2(pagination_info_pb2.PaginationInfo()) + assert isinstance(info, PaginationInfo) + assert info.total_items == 0 + + +class _NegativeTotalItemsPb: + """A message-like object whose `total_items` is negative. + + The `total_items` field is a protobuf `uint32`, so a decoded message can + never carry a negative count. This stand-in exercises the invalid branch + of the converter, which must not raise regardless of what it is handed. + """ + + total_items = -1 + next_page_token = "" + + # pylint: disable-next=invalid-name,unused-argument + def HasField(self, field_name: str) -> bool: + """Report every field as unset. + + Args: + field_name: The name of the field to check. + + Returns: + Always `False`. + """ + return False + + +def test_from_proto2_negative_total_items() -> None: + """A negative count becomes an `InvalidPaginationInfo` instead of raising.""" + proto = cast(pagination_info_pb2.PaginationInfo, _NegativeTotalItemsPb()) + info = pagination_info_from_proto2(proto) + assert isinstance(info, InvalidPaginationInfo) + assert info.total_items == -1 + assert info.next_page_token is None + assert str(info) == "items=,next=None" diff --git a/tests/pagination/test_pagination_info.py b/tests/pagination/test_pagination_info.py index f2d580b7..8447b91e 100644 --- a/tests/pagination/test_pagination_info.py +++ b/tests/pagination/test_pagination_info.py @@ -5,25 +5,100 @@ import pytest -from frequenz.client.common.pagination import PaginationInfo +from frequenz.client.common.pagination import ( + BasePaginationInfo, + InvalidPaginationInfo, + PaginationInfo, +) -def test_pagination_info_accepts_zero_total_items() -> None: +def test_base_cannot_be_instantiated_directly() -> None: + """`BasePaginationInfo` refuses direct instantiation.""" + with pytest.raises(TypeError, match="Cannot instantiate BasePaginationInfo"): + BasePaginationInfo(total_items=0) + + +def test_leaves_are_base_subclasses() -> None: + """Both concrete types share the `BasePaginationInfo` supertype.""" + assert issubclass(PaginationInfo, BasePaginationInfo) + assert issubclass(InvalidPaginationInfo, BasePaginationInfo) + + +def test_accepts_zero_total_items() -> None: """Zero total items should be accepted.""" info = PaginationInfo(total_items=0) assert info.total_items == 0 -def test_pagination_info_accepts_positive_total_items() -> None: +def test_accepts_positive_total_items() -> None: """Positive total items should be accepted.""" info = PaginationInfo(total_items=1) assert info.total_items == 1 -def test_pagination_info_rejects_negative_total_items() -> None: +def test_rejects_negative_total_items() -> None: """Negative total items should be rejected with the exact message.""" with pytest.raises( ValueError, match=r"^total_items must be non-negative, not -1$", ): PaginationInfo(total_items=-1) + + +def test_token_defaults_to_none() -> None: + """The next page token is optional.""" + assert PaginationInfo(total_items=0).next_page_token is None + + +@pytest.mark.parametrize( + ("total_items", "next_page_token", "expected"), + [ + (0, None, "items=0,next=None"), + (100, "token", "items=100,next=token"), + ], +) +def test_str(total_items: int, next_page_token: str | None, expected: str) -> None: + """`PaginationInfo` renders compactly and without an invalid marker.""" + info = PaginationInfo(total_items=total_items, next_page_token=next_page_token) + assert str(info) == expected + + +def test_invalid_accepts_negative_total_items() -> None: + """`InvalidPaginationInfo` enforces no invariants on construction.""" + info = InvalidPaginationInfo(total_items=-1, next_page_token="token") + assert info.total_items == -1 + assert info.next_page_token == "token" + + +@pytest.mark.parametrize( + ("total_items", "next_page_token", "expected"), + [ + (-1, None, "items=,next=None"), + (-10, "token", "items=,next=token"), + (0, None, "items=0,next=None"), + ], +) +def test_invalid_str( + total_items: int, next_page_token: str | None, expected: str +) -> None: + """Only a negative count is marked as invalid.""" + info = InvalidPaginationInfo( + total_items=total_items, next_page_token=next_page_token + ) + assert str(info) == expected + + +def test_invalid_equality() -> None: + """Two `InvalidPaginationInfo` instances with the same data are equal.""" + info1 = InvalidPaginationInfo(total_items=-1) + info2 = InvalidPaginationInfo(total_items=-1) + info3 = InvalidPaginationInfo(total_items=-2) + assert info1 == info2 + assert info1 != info3 + + +def test_valid_and_invalid_are_distinct() -> None: + """A `PaginationInfo` and an `InvalidPaginationInfo` with same fields differ.""" + valid = PaginationInfo(total_items=1) + invalid = InvalidPaginationInfo(total_items=1) + assert valid != invalid # type: ignore[comparison-overlap]