Add pagination_info_from_proto2 and deprecate the old converter

`pagination_info_from_proto()` calls the `PaginationInfo` constructor
directly, so it raises on a count the constructor rejects. A conversion
function must never raise on wire content: the caller loses the whole
message over one bad field. Add the replacement that returns
`PaginationInfo | InvalidPaginationInfo` and deprecate the old one,
following the path `delivery_area_from_proto` and `bounds_from_proto`
already took.

The warning text uses the form the deprecation guide prescribes, with
both fully qualified names written out. The six deprecations already in
the library use a shorter backticked form instead, so this one does not
match them; the guide is the normative source, and a caller who sees
only the warning can act on it without guessing the import path.

The new converter also reads `next_page_token` through `HasField()`
instead of testing it for truth. The field has presence, so the old
truthiness test collapses "the server sent an empty token" into "the
server sent no token"; only the second means there are no more pages.
Changing this in place would be a silent behavior change, which is
another reason it belongs in the new name rather than the old one.

Note that `total_items` is a `uint32`, so a decoded message cannot in
fact carry a negative count today, and the invalid branch is
unreachable through the generated bindings. It is there because the
converter's contract is not to raise regardless of what it is handed,
and because the wire type is not the wrapper's to assume; the test
exercises it with a message-like stand-in.

Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>

Author: llucax

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]. |
 

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",
 ]

src/frequenz/client/common/pagination/proto/v1alpha8/_pagination_info.py

@@ -6,25 +6,77 @@
 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,
         next_page_token=message.next_page_token if message.next_page_token else None,
     )
 
 
+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.
 

tests/pagination/proto/v1alpha8/test_pagination_info.py

@@ -3,42 +3,127 @@
 
 """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)
     assert proto.total_items == 100
     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=<invalid:-1>,next=None"