Improve Lifetime validation and error handling#249
Open
llucax wants to merge 11 commits into
Open
Conversation
The `model` is a required field in the protobuf message so it is better to keep the representation consistent. An empty string is preserved as is now. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
This class will be used as the base for both valid and invalid lifetime classes. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
`Lifetime`, the existing type, is retroactively made a subclass of `BaseLifetime`. Its field shape, construction API, validation, and operational checks are unchanged. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
github-actions
Bot
added
part:docs
There was a problem hiding this comment.
Pull request overview
This PR refines how microgrid “lifetime” values are represented and consumed across the client-common wrapper layer, aiming to preserve malformed wire data safely while providing explicit failure modes via semantic accessors (aligning with the direction in #239 to move away from fragile “issues as strings” patterns).
Changes:
- Introduce
BaseLifetime,Lifetime,InvalidLifetime, andInvalidLifetimeErrorunderfrequenz.client.common.microgrid, and update lifetime conversion to preserve malformed ranges asInvalidLifetime. - Add safe lifetime accessors (
get_operational_lifetime()) forElectricalComponentandElectricalComponentConnection, and make operational checks raiseInvalidLifetimeErrorwhen malformed data is present. - Update proto converters, tests, and release notes to reflect the new lifetime handling and imports (including moving lifetime conversion out of
frequenz.client.common.types.*).
Reviewed changes
Copilot reviewed 35 out of 35 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/microgrid/proto/v1alpha8/test_lifetime.py | Updates lifetime proto tests to expect InvalidLifetime preservation instead of raising. |
| tests/microgrid/electrical_components/test_simple_components.py | Adjusts component construction expectations (adds model). |
| tests/microgrid/electrical_components/test_problematic.py | Adjusts problematic component tests for new model requirement. |
| tests/microgrid/electrical_components/test_problematic_connection.py | Updates Lifetime import to microgrid module. |
| tests/microgrid/electrical_components/test_power_transformer.py | Updates construction to include model. |
| tests/microgrid/electrical_components/test_inverter.py | Updates construction to include model. |
| tests/microgrid/electrical_components/test_grid_connection_point.py | Updates construction to include model. |
| tests/microgrid/electrical_components/test_ev_charger.py | Updates construction to include model. |
| tests/microgrid/electrical_components/test_electrical_component_connection.py | Adds tests for safe lifetime accessors and invalid-lifetime raising behavior. |
| tests/microgrid/electrical_components/test_electrical_component_base.py | Adds component-level safe lifetime accessor tests and updates model expectations. |
| tests/microgrid/electrical_components/test_battery.py | Updates construction to include model. |
| tests/microgrid/electrical_components/proto/v1alpha8/test_raw_storage.py | Ensures model is preserved in raw/unrecognized component conversion cases. |
| tests/microgrid/electrical_components/proto/v1alpha8/test_electrical_component_connection.py | Updates connection proto conversion expectations: unbounded default and invalid lifetime preservation. |
| tests/microgrid/electrical_components/proto/v1alpha8/test_electrical_component_base.py | Updates base-data proto conversion expectations for lifetime handling and minor issues. |
| tests/microgrid/electrical_components/proto/v1alpha8/test_class.py | Updates test data dict to include model. |
| tests/microgrid/electrical_components/proto/v1alpha8/conftest.py | Moves Lifetime import to microgrid module and sets default lifetime fixture. |
| tests/microgrid/_lifetime/test_lifetime.py | Adjusts lifetime tests to new lifetime type hierarchy (BaseLifetime). |
| tests/microgrid/_lifetime/test_invalid_lifetime.py | Adds tests for InvalidLifetime preserving malformed ranges. |
| tests/microgrid/_lifetime/test_invalid_lifetime_error.py | Adds tests for InvalidLifetimeError behavior and inheritance. |
| tests/microgrid/_lifetime/test_base_lifetime.py | Adds tests ensuring BaseLifetime cannot be instantiated directly. |
| tests/microgrid/_lifetime/conftest.py | Adds shared time fixtures for lifetime-related tests. |
| tests/microgrid/_lifetime/init.py | Introduces package init for lifetime tests. |
| src/frequenz/client/common/types/proto/v1alpha8/_lifetime.py | Removes the old lifetime proto converter from types. |
| src/frequenz/client/common/types/proto/v1alpha8/init.py | Stops exporting lifetime_from_proto from types.proto.v1alpha8. |
| src/frequenz/client/common/types/_lifetime.py | Removes the old types.Lifetime implementation. |
| src/frequenz/client/common/types/init.py | Stops exporting Lifetime from frequenz.client.common.types. |
| src/frequenz/client/common/microgrid/proto/v1alpha8/_lifetime.py | Adds microgrid-scoped lifetime proto conversion that returns `Lifetime |
| src/frequenz/client/common/microgrid/proto/v1alpha8/init.py | Exports lifetime_from_proto from microgrid proto package. |
| src/frequenz/client/common/microgrid/electrical_components/proto/v1alpha8/_electrical_component.py | Updates component proto parsing to use new lifetime types and preserve malformed lifetimes. |
| src/frequenz/client/common/microgrid/electrical_components/proto/v1alpha8/_electrical_component_connection.py | Updates connection proto parsing to use new lifetime types; drops minor-issues collection for lifetime. |
| src/frequenz/client/common/microgrid/electrical_components/_electrical_component.py | Adds get_operational_lifetime() and makes operational checks raise on invalid lifetime. |
| src/frequenz/client/common/microgrid/electrical_components/_electrical_component_connection.py | Adds get_operational_lifetime() and makes operational checks raise on invalid lifetime. |
| src/frequenz/client/common/microgrid/_lifetime.py | Introduces BaseLifetime, Lifetime, InvalidLifetime, InvalidLifetimeError. |
| src/frequenz/client/common/microgrid/init.py | Re-exports new lifetime types at frequenz.client.common.microgrid.*. |
| RELEASE_NOTES.md | Updates release notes entry related to lifetime types/converters. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
`InvalidLifetime` represents malformed wire data. It carries the same timestamps as `BaseLifetime` without enforcing ordering invariants, so callers can inspect what the server actually sent without using it for operational checks. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
The dedicated `InvalidLifetimeError`, also a `ValueError` for convenience, carries the offending `InvalidLifetime` on `.lifetime`. Upcoming semantic accessors can therefore reject malformed lifetime data without preventing callers from inspecting the original timestamps. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
Now the conversion function returns `Lifetime | InvalidLifetime` instead of raising for malformed timestamp ordering. Invalid wire data becomes an `InvalidLifetime` carrying both timestamps, so callers can inspect or report what the server sent. The existing converter retains its current raising behavior for now. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
Loosen `ElectricalComponent.operational_lifetime` and `BaseElectricalComponentConnection.operational_lifetime` to `Lifetime | InvalidLifetime`, preserving malformed timestamp ordering received from protobuf instead of replacing it with an unbounded `Lifetime`. Add `get_operational_lifetime() -> Lifetime` to both class hierarchies so callers that require a valid lifetime receive either the valid value or an `InvalidLifetimeError` carrying the malformed one. The `is_operational_at()` and `is_operational_now()` shortcuts now use the safe accessor and raise the same error for malformed data. Missing and present-but-empty protobuf lifetimes continue to become `Lifetime()`, as both represent an unbounded lifetime in the upstream schema. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
Now that invalid lifetimes are encoded into the type system, we don't need to report issues with them via the major/minor issues side-channel. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
`src/.../types/_lifetime.py` now exposes four public symbols, so the test
file grew too large. Split it following the repository convention:
tests/types/test_lifetime.py
-> tests/types/_lifetime/test_<thing>.py
Test bodies and assertions are unchanged. Shared time fixtures move to the
subdirectory's `conftest.py`, and test names drop redundant type prefixes.
Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
This is to match where `Lifetime` is defined in the protobuf messages. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
We now also allow storing invalid values, but encoded via a new type
InvalidLifetime. This way users still can't read invalid values accidentally, but still allows users to inspect invalid values if they need to, instead of only reporting via logging.Add new safe accesors for
ElectricalComponentandElectricalComponentConnectionlifetimeattributes.Part of #239.