Add `InvalidDatetime` to represent a malformed protobuf `Timestamp` by llucax · Pull Request #278 · frequenz-floss/frequenz-client-common-python

llucax · 2026-08-31T07:38:13Z

This PR closes the last gap (hopefully) of _from_proto() functions that can raise on invalid protobuf data.

It adds a new wrapper type InvalidDatetime that can be used to represent invalid timestamps in protobuf messages, and adds a new function datetime_from_proto2() that will return an InvalidDatetime instead of raising an exception.

MetricSample adds a sample_time2: datetime | InvalidDatetime field that will be set to an InvalidDatetime if the protobuf message has a malformed timestamp and an accessor get_sample_time2() that will raise the new InvalidDatetimeError exception if the timestamp is invalid. The old sample_time field is still present as a read-only property (not a real dataclass field) but deprecated.

Microgrid and InvalidLifetime fields using datetime are updated to use datetime | InvalidDatetime without a deprecation path as they are unreleased.

The user guides are updated to include when NOT to use a base class more clearly.

Fixes #276.

Every other conversion module is named after the wrapper type it produces, not after the protobuf message it consumes: `_delivery_area`, `_location`, `_bounds`. This one converts to and from `datetime`, so `_timestamp` reads like the odd one out, and the tests already agree -- they have always lived in `tests/proto/test_datetime.py`. Rename it and leave the contents alone. Nothing public moves; the symbols are re-exported from `frequenz.client.common.proto` either way. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>

A protobuf `Timestamp` is two plain integers on the wire, so a decoded message can break the `Timestamp` contract in two ways: a `seconds` count outside the years 1 to 9999, or a `nanos` fraction outside `[0, 999999999]`. Today the first takes the whole conversion down with an exception, and the second is silently repaired into a wrong but plausible-looking `datetime`. Add the wrapper that will carry both instead. `InvalidDatetime` keeps `seconds` and `nanos` exactly as received, so callers can inspect, report or reinterpret what the server actually sent. `InvalidDatetimeError` is the error the semantic accessors will raise once wrapper fields start holding one. Both go in the top-level package rather than in `types`, next to the conversion function that will produce them in `proto/`. `types` wraps `frequenz-api-common` messages, and a timestamp is not one of them: it is a well-known protobuf type, which is exactly why its conversion function lives in the top-level `proto/` package and not in a versioned `proto/v1alphaN/` one. The wrapper follows the converter. Nothing uses it yet; the conversion function and the wrapper fields follow. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>

`datetime_from_proto()` mishandles both ways a decoded `Timestamp` can break its own contract. An out-of-range `seconds` count raises, so the whole conversion fails: datetime_from_proto(Timestamp(seconds=253402300800)) ValueError: year 10000 is out of range An out-of-range `nanos` fraction is worse, because it fails silently: `int(nanos / 1000)` truncates it into the microsecond field, so `nanos=1000000000` becomes a whole extra second and `nanos=-1` becomes zero, and the caller gets a `datetime` that looks fine. Protobuf's own `ToDatetime()` rejects both. `datetime_from_proto()` is released, so this needs a new name rather than a changed contract. Add `datetime_from_proto2()`, returning `datetime | InvalidDatetime`, which keeps the raw `seconds` and `nanos` in either case. It takes no `tz`. A `Timestamp` denotes a UTC instant, and the protobuf `seconds` range is exactly the range `datetime` covers -- both are the years 1 to 9999 -- so in UTC, well-formed and representable are the same thing and `InvalidDatetime` has a single meaning. Accepting a `tz` would break that: converting an in-range instant into another zone overflows within a day of either end, so the wrapper would also have to mean "valid, but not in the zone you asked for". Callers who need another zone call `.astimezone()`, and handle that case themselves. The conversion goes through `timedelta` rather than `datetime.fromtimestamp()`. A `float` timestamp has about 16 significant digits, while the far end of the range needs 18 to keep microsecond resolution, so the released function silently loses sub-second precision there. Building the offset from integers keeps the new one exact across the whole range. Nothing uses it yet; the callers move over one wrapper at a time, and `datetime_from_proto()` is deprecated once none is left. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>

`metric_sample_from_proto()` promises to report malformed input through the returned type, but its very first line called `datetime_from_proto()`, so a sample whose timestamp broke the `Timestamp` contract either raised or was silently repaired. When it raised, a single bad sample in a stream failed the whole conversion, and the caller never saw the metric, the value, the bounds or the connection that came with it. Move the converters to `datetime_from_proto2()`, so the rest of the sample survives and the raw seconds and nanoseconds stay available for diagnosis. `MetricSample.sample_time` is released as a plain `datetime`, so it cannot simply widen. Follow the scheme already used in this class for `bounds` / `bounds_set`: the field becomes `sample_time2`, typed `datetime | InvalidDatetime`, and `sample_time` stays as a deprecated read-only property returning a `datetime`. For a malformed timestamp it raises `InvalidDatetimeError`, which is a `ValueError` -- not the same exception as before, but the same kind, and better than returning a repaired value. `get_sample_time()` does the same without the warning, for code that has already migrated. Constructing with `sample_time=` is deliberately *not* deprecated, unlike `bounds=`. A `list[Bounds]` will never be a valid `bounds_set`, so that argument has to warn; a `datetime` will still be a valid `sample_time` after `sample_time2` is renamed back at the next minor release, so warning about it would only create work for callers. `sample_time2=` exists for building a sample from a malformed wire timestamp, and for `dataclasses.replace()`, which passes field names. The released `metric_sample_from_proto_with_issues()` keeps raising, now a plain `ValueError`. It is the loud path callers already depend on, and quietly returning a sample carrying an `InvalidDatetime` instead would let a malformed timestamp through a `major_issues` acceptance check unnoticed. Only the unreleased `metric_sample_from_proto()` keeps the value. Both `__init__` arguments that accept an older spelling are resolved in small static helpers, so the constructor stays readable and both can be deleted in one piece later. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>

`mkdocs-material` has no such admonition, so it renders as a generic note. Use `Warning: Deprecated` instead. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>

`microgrid_from_proto()` converts the creation timestamp before it has built anything else, so a `create_timestamp` outside the years 1 to 9999 discarded the whole microgrid -- its ID, name, delivery area and location included -- over a field most callers never read. Move it to `datetime_from_proto2()` and widen `Microgrid.create_time` to `datetime | InvalidDatetime`, plus a `get_create_time()` accessor alongside `get_delivery_area()` and `get_location()` for callers that do read it. `Microgrid` is unreleased, so the annotation widens in place; no compatibility property is needed here. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>

`lifetime_from_proto()` already returns `Lifetime | InvalidLifetime`, but it built both ends with `datetime_from_proto()`, so a malformed timestamp raised before the invariant check it feeds could ever run. Every electrical component and connection converter goes through this function, so one such lifetime failed the whole component. Move it to `datetime_from_proto2()` and let `InvalidLifetime` hold what it returns: its two fields become `datetime | InvalidDatetime | None`. `Lifetime` keeps `datetime | None`, because it always represents a valid lifetime. This makes `BaseLifetime` pointless. It existed only to hold two fields both subclasses shared, and they no longer share them: a subclass cannot narrow an inherited attribute, so keeping the base would mean declaring `Lifetime.start_time` as a union it never holds and adding accessors to unwrap a case it rejects. `Lifetime` and `InvalidLifetime` become two independent frozen dataclasses (like `BoundsSet` and `InvalidBoundsSet`, which are two independent classes for the same reason). `BaseLifetime` was never released so we can simply remove it. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>

The wrapping guide told when to add a base class, but it didn't explicitly mention when not to do it, so make it more explicit, including examples. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>

Nothing in the library calls it any more: metric samples, microgrids and lifetimes all went over to `datetime_from_proto2()`. Point the external callers there too via a deprecation warning. The warning mentions the replacement and its return type, and the docstring explains the three reasons to move: the new function keeps a malformed timestamp in the return type instead of raising or repairing it, it does not lose sub-second precision far from the epoch, and it returns UTC rather than taking a `tz`. The `Raises` section is new. The old function always raised on an out-of-range timestamp, but never said so, which is a large part of why callers were surprised by it. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>

The three guides were written against the wrapper surface as it was, so they describe `Location` as the only type with per-field invalid wrappers and list no invalid timestamp at all. Now that wrapper timestamps are unions, a reader following the User Guide would meet an `InvalidDatetime` the guide never mentions. Add it to the wrapper overview, and to the User Guide page on validity with a worked example of both ways to read such a field. Add it to the string-output page too, which enumerates the types using the `<invalid:...>` marker. The Wrapping Guide gains the rule the timestamp wrapper demonstrates: when the same field appears in several types, write one field wrapper and one error for all of them, and put it where the thing it wraps belongs. For a well-known protobuf type, that is the top-level package rather than a versioned domain. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>

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

Copilot

🟡 Changes recommended

The deprecated metric converter newly raises for malformed nanoseconds that its released implementation previously accepted.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds explicit handling for malformed protobuf timestamps without leaking protobuf types into the public API.

Changes:

Adds InvalidDatetime, its typed error, and safe timestamp conversion.
Propagates invalid timestamps through metrics, microgrids, and lifetimes.
Adds migration documentation and comprehensive tests.

File summaries

File	Description
`tests/test_datetime.py`	Tests timestamp wrapper and error.
`tests/proto/test_datetime.py`	Tests timestamp conversions and boundaries.
`tests/microgrid/test_microgrid.py`	Tests creation-time accessor.
`tests/microgrid/proto/v1alpha8/test_microgrid.py`	Tests malformed creation timestamps.
`tests/microgrid/proto/v1alpha8/test_lifetime.py`	Tests malformed lifetime timestamps.
`tests/microgrid/electrical_components/proto/v1alpha8/conftest.py`	Adjusts lifetime test serialization.
`tests/microgrid/_lifetime/test_lifetime.py`	Removes base-class assertion.
`tests/microgrid/_lifetime/test_invalid_lifetime.py`	Tests invalid timestamp endpoints.
`tests/microgrid/_lifetime/test_base_lifetime.py`	Removes obsolete base-class tests.
`tests/metrics/test_sample_metric_sample.py`	Tests timestamp compatibility and accessor.
`tests/metrics/proto/v1alpha8/test_sample_metric_sample.py`	Tests malformed sample conversion.
`src/frequenz/client/common/proto/_timestamp.py`	Removes superseded converter module.
`src/frequenz/client/common/proto/_datetime.py`	Implements safe and legacy conversions.
`src/frequenz/client/common/proto/__init__.py`	Exports the new converter.
`src/frequenz/client/common/microgrid/proto/v1alpha8/_microgrid.py`	Preserves malformed creation times.
`src/frequenz/client/common/microgrid/proto/v1alpha8/_lifetime.py`	Preserves malformed lifetime endpoints.
`src/frequenz/client/common/microgrid/electrical_components/proto/v1alpha8/_electrical_component.py`	Updates lifetime documentation.
`src/frequenz/client/common/microgrid/electrical_components/proto/v1alpha8/_electrical_component_connection.py`	Updates connection documentation.
`src/frequenz/client/common/microgrid/_microgrid.py`	Widens creation time and adds accessor.
`src/frequenz/client/common/microgrid/_lifetime.py`	Separates valid and invalid lifetime models.
`src/frequenz/client/common/microgrid/__init__.py`	Removes obsolete base export.
`src/frequenz/client/common/metrics/proto/v1alpha8/_sample.py`	Uses safe timestamp conversion.
`src/frequenz/client/common/metrics/_sample.py`	Adds widened timestamp field and compatibility property.
`src/frequenz/client/common/_datetime.py`	Defines timestamp wrapper and error.
`src/frequenz/client/common/__init__.py`	Exports new public types.
`RELEASE_NOTES.md`	Documents migration and APIs.
`docs/wrapping-guide/validity-in-the-type.md`	Clarifies wrapper design guidance.
`docs/user-guide/validity-in-the-type.md`	Documents malformed timestamp handling.
`docs/user-guide/reading-string-output.md`	Documents invalid timestamp formatting.
`docs/user-guide/overview.md`	Adds the timestamp utility overview.

Review details

Files reviewed: 30/30 changed files
Comments generated: 3
Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

+    * `frequenz.client.common.metrics.MetricSample.get_sample_time()`
    * `frequenz.client.common.microgrid.electrical_components.ElectricalComponent.get_metric_config_bounds()`


+    sample_time = datetime_from_proto2(message.sample_time)
+    if isinstance(sample_time, InvalidDatetime):
+        raise ValueError(f"malformed sample_time {sample_time}")


+produces it. When, it wraps no `frequenz-api-common` message at all, like a
+timestamp, put it directly in the top-level package or an utility-specific
+module, do not mix it with a domain-specific module.


llucax added 11 commits August 31, 2026 09:29

Fix the Deprecated: section header on bounds

d08f8dd

`mkdocs-material` has no such admonition, so it renders as a generic note. Use `Warning: Deprecated` instead. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>

Document when not to use a base class

8424832

The wrapping guide told when to add a base class, but it didn't explicitly mention when not to do it, so make it more explicit, including examples. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>

Update release notes

90897cc

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

llucax requested a review from a team as a code owner August 31, 2026 07:38

llucax requested review from shsms and removed request for a team August 31, 2026 07:38

llucax self-assigned this Aug 31, 2026

llucax requested a balanced review from Copilot August 31, 2026 07:38

Copilot started reviewing on behalf of llucax August 31, 2026 07:39 View session

Copilot AI reviewed Aug 31, 2026

View reviewed changes

Navigation Menu

Uh oh!

Add `InvalidDatetime` to represent a malformed protobuf `Timestamp` - #278

Add `InvalidDatetime` to represent a malformed protobuf `Timestamp`#278
llucax wants to merge 11 commits into
frequenz-floss:v0.x.xfrom
llucax:datetime-exc

llucax commented Aug 31, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		* `frequenz.client.common.metrics.MetricSample.get_sample_time()`
		* `frequenz.client.common.microgrid.electrical_components.ElectricalComponent.get_metric_config_bounds()`

Uh oh!

Conversation

llucax commented Aug 31, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants