Skip to content

feat: Add raise_on_errors parameter for strict parsing validation #73

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
eduardiazf wants to merge 6 commits into
base: v0.x.x
Choose a base branch
from
Open

feat: Add raise_on_errors parameter for strict parsing validation #73

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0525e6b
Add raise_on_errors parameter to parsing functions
eduardiazf Jan 26, 2026
ad38047
refactor(raise_on_errors): replace ParsingError with ValidationError …
eduardiazf Jan 29, 2026
f8570e1
fix: Handle empty major_issues in ValidationError message formatting
eduardiazf Mar 5, 2026
d3acea6
fix: Remove redundant None filter in valid_connections list
eduardiazf Mar 5, 2026
fcaeedb
refactor: Replace ExceptionGroup subclasses with plain ExceptionGroup
eduardiazf Mar 11, 2026
9fad9ab
fix: Fix return type
eduardiazf Mar 12, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
139 changes: 128 additions & 11 deletions src/frequenz/client/assets/_client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,23 @@
from frequenz.client.common.microgrid.electrical_components import ElectricalComponentId

from ._microgrid import Microgrid
from ._microgrid_proto import microgrid_from_proto
from ._microgrid_proto import microgrid_from_proto, microgrid_from_proto_with_issues
from .electrical_component._connection import ComponentConnection
from .electrical_component._connection_proto import component_connection_from_proto
from .electrical_component._connection_proto import (
component_connection_from_proto,
component_connection_from_proto_with_issues,
)
from .electrical_component._electrical_component import ElectricalComponent
from .electrical_component._electrical_component_proto import electrical_component_proto
from .exceptions import ClientNotConnected
from .electrical_component._electrical_component_proto import (
electrical_component_from_proto_with_issues,
electrical_component_proto,
)
from .exceptions import (
ClientNotConnected,
InvalidConnectionError,
InvalidElectricalComponentError,
InvalidMicrogridError,
)

DEFAULT_GRPC_CALL_TIMEOUT = 60.0
"""The default timeout for gRPC calls made by this client (in seconds)."""
Expand Down Expand Up @@ -88,21 +99,30 @@ def stub(self) -> assets_pb2_grpc.PlatformAssetsAsyncStub:
# use the async stub, so we cast the sync stub to the async stub.
return self._stub # type: ignore

async def get_microgrid( # noqa: DOC502 (raises ApiClientError indirectly)
self, microgrid_id: MicrogridId
async def get_microgrid( # noqa: DOC502,DOC503 (raises indirectly)
self,
microgrid_id: MicrogridId,
*,
raise_on_errors: bool = False,
) -> Microgrid:
"""
Get the details of a microgrid.

Args:
microgrid_id: The ID of the microgrid to get the details of.
raise_on_errors: If True, raise an
[InvalidMicrogridError][frequenz.client.assets.exceptions.InvalidMicrogridError]
when major validation issues are found instead of just
logging them.

Returns:
The details of the microgrid.

Raises:
ApiClientError: If there are any errors communicating with the Assets API,
most likely a subclass of [GrpcError][frequenz.client.base.exception.GrpcError].
InvalidMicrogridError: If `raise_on_errors` is True and major
validation issues are found.
"""
response = await call_stub_method(
self,
Expand All @@ -113,19 +133,48 @@ async def get_microgrid( # noqa: DOC502 (raises ApiClientError indirectly)
method_name="GetMicrogrid",
)

if raise_on_errors:
major_issues: list[str] = []
minor_issues: list[str] = []
microgrid = microgrid_from_proto_with_issues(
response.microgrid,
major_issues=major_issues,
minor_issues=minor_issues,
)
if major_issues:
raise InvalidMicrogridError(
microgrid=microgrid,
major_issues=major_issues,
minor_issues=minor_issues,
raw_message=response.microgrid,
)
return microgrid
Comment on lines +136 to +151
Copy link

Copilot AI Feb 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When raise_on_errors=True and there are only minor issues (no major issues), those minor issues are not logged. This creates a behavioral inconsistency where minor issues are silently discarded when strict validation is enabled but no major issues are found. Consider adding logging for minor issues even when raise_on_errors=True and no exception is raised, to maintain consistency with the default behavior.

Copilot uses AI. Check for mistakes.
Comment on lines +136 to +151
Copy link

Copilot AI Feb 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new raise_on_errors parameter and the associated exception raising behavior lack test coverage. No tests were added to verify that InvalidMicrogridError is raised correctly when major validation issues are found with raise_on_errors=True, or that the exception contains the expected microgrid, major_issues, minor_issues, and raw_message attributes. Consider adding comprehensive test cases covering: 1) successful validation with raise_on_errors=True, 2) validation failure with major issues, 3) validation with only minor issues (to test that they're not logged), and 4) edge cases like empty issues lists.

Copilot uses AI. Check for mistakes.

return microgrid_from_proto(response.microgrid)

async def list_microgrid_electrical_components(
self, microgrid_id: MicrogridId
self,
microgrid_id: MicrogridId,
*,
raise_on_errors: bool = False,
) -> list[ElectricalComponent]:
"""
Get the electrical components of a microgrid.

Args:
microgrid_id: The ID of the microgrid to get the electrical components of.
raise_on_errors: If True, raise an
`ExceptionGroup[InvalidElectricalComponentError]`
when major validation issues are found in any component instead
of just logging them.

Returns:
The electrical components of the microgrid.

Raises:
ExceptionGroup: If `raise_on_errors` is True and major validation
issues are found. All exceptions in the group are
[InvalidElectricalComponentError][frequenz.client.assets.exceptions.InvalidElectricalComponentError].
"""
response = await call_stub_method(
self,
Expand All @@ -138,6 +187,35 @@ async def list_microgrid_electrical_components(
method_name="ListMicrogridElectricalComponents",
)

if raise_on_errors:
components: list[ElectricalComponent] = []
exceptions: list[InvalidElectricalComponentError] = []
for component_pb in response.components:
major_issues: list[str] = []
minor_issues: list[str] = []
component = electrical_component_from_proto_with_issues(
component_pb,
major_issues=major_issues,
minor_issues=minor_issues,
)
if major_issues:
exceptions.append(
InvalidElectricalComponentError(
component=component,
major_issues=major_issues,
minor_issues=minor_issues,
raw_message=component_pb,
)
)
else:
components.append(component)
if exceptions:
raise ExceptionGroup(
f"{len(exceptions)} electrical component(s) failed validation",
exceptions,
)
return components
Comment on lines +190 to +217
Copy link

Copilot AI Feb 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new raise_on_errors parameter and the associated exception raising behavior lack test coverage. No tests were added to verify that InvalidElectricalComponentErrorGroup is raised correctly when major validation issues are found with raise_on_errors=True, or that the exception contains the expected valid_components and exceptions. Consider adding comprehensive test cases covering: 1) all components valid with raise_on_errors=True, 2) some components with validation failures, 3) all components failing validation, and 4) edge cases like empty component lists.

Copilot uses AI. Check for mistakes.
Comment on lines +190 to +217
Copy link

Copilot AI Feb 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When raise_on_errors=True and there are only minor issues (no major issues), those minor issues are not logged. This creates a behavioral inconsistency where minor issues are silently discarded when strict validation is enabled but no major issues are found. Consider adding logging for minor issues even when raise_on_errors=True and no exception is raised, to maintain consistency with the default behavior.

Copilot uses AI. Check for mistakes.

return [
electrical_component_proto(component) for component in response.components
]
Expand All @@ -147,7 +225,9 @@ async def list_microgrid_electrical_component_connections(
microgrid_id: MicrogridId,
source_component_ids: Iterable[ElectricalComponentId] = (),
destination_component_ids: Iterable[ElectricalComponentId] = (),
) -> list[ComponentConnection | None]:
*,
raise_on_errors: bool = False,
) -> list[ComponentConnection]:
"""
Get the electrical component connections of a microgrid.

Expand All @@ -158,9 +238,18 @@ async def list_microgrid_electrical_component_connections(
these component IDs. If None or empty, no filtering is applied.
destination_component_ids: Only return connections that terminate at
these component IDs. If None or empty, no filtering is applied.
raise_on_errors: If True, raise an
`ExceptionGroup[InvalidConnectionError]`
when major validation issues are found in any connection instead
of just logging them.

Returns:
The electrical component connections of the microgrid.

Raises:
ExceptionGroup: If `raise_on_errors` is True and major validation
issues are found. All exceptions in the group are
[InvalidConnectionError][frequenz.client.assets.exceptions.InvalidConnectionError].
"""
request = assets_pb2.ListMicrogridElectricalComponentConnectionsRequest(
microgrid_id=int(microgrid_id),
Expand All @@ -177,9 +266,37 @@ async def list_microgrid_electrical_component_connections(
method_name="ListMicrogridElectricalComponentConnections",
)

return list(
map(
if raise_on_errors:
valid_connections: list[ComponentConnection] = []
exceptions: list[InvalidConnectionError] = []
for conn_pb in filter(bool, response.connections):
major_issues: list[str] = []
connection = component_connection_from_proto_with_issues(
conn_pb, major_issues=major_issues
)
if major_issues:
exceptions.append(
InvalidConnectionError(
connection=connection,
major_issues=major_issues,
minor_issues=[],
raw_message=conn_pb,
)
)
elif connection is not None:
valid_connections.append(connection)
if exceptions:
raise ExceptionGroup(
f"{len(exceptions)} connection(s) failed validation",
exceptions,
)
return valid_connections

return [
c
for c in map(
component_connection_from_proto,
filter(bool, response.connections),
)
)
if c is not None
]
Comment on lines +295 to +302
Copy link
Contributor

@llucax llucax Mar 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works, but you are filtering twice now. You are filtering at the response.connections level that can't really be None or falsy, and then at the converted connection level (which can be None), so you can just get rid of the filter().

Suggested change
return [
c
for c in map(
component_connection_from_proto,
filter(bool, response.connections),
)
)
if c is not None
]
return [
for c in map(component_connection_from_proto, response.connections)
if c is not None
]

54 changes: 40 additions & 14 deletions src/frequenz/client/assets/_microgrid_proto.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,46 @@ def microgrid_from_proto(message: microgrid_pb2.Microgrid) -> Microgrid:
major_issues: list[str] = []
minor_issues: list[str] = []

microgrid = microgrid_from_proto_with_issues(
message, major_issues=major_issues, minor_issues=minor_issues
)

if major_issues:
_logger.warning(
"Found issues in microgrid: %s | Protobuf message:\n%s",
", ".join(major_issues),
message,
)

if minor_issues:
_logger.debug(
"Found minor issues in microgrid: %s | Protobuf message:\n%s",
", ".join(minor_issues),
message,
)

return microgrid


def microgrid_from_proto_with_issues(
message: microgrid_pb2.Microgrid,
*,
major_issues: list[str],
minor_issues: list[str],
) -> Microgrid:
"""Convert a protobuf microgrid message to a microgrid object, collecting issues.

This function is useful when you want to collect issues during parsing
rather than logging them immediately.

Args:
message: The protobuf message to convert.
major_issues: A list to collect major issues found during validation.
minor_issues: A list to collect minor issues found during validation.

Returns:
The resulting microgrid object.
"""
delivery_area: DeliveryArea | None = None
if message.HasField("delivery_area"):
delivery_area = delivery_area_from_proto(message.delivery_area)
Expand All @@ -54,20 +94,6 @@ def microgrid_from_proto(message: microgrid_pb2.Microgrid) -> Microgrid:
elif isinstance(status, int):
major_issues.append("status is unrecognized")

if major_issues:
_logger.warning(
"Found issues in microgrid: %s | Protobuf message:\n%s",
", ".join(major_issues),
message,
)

if minor_issues:
_logger.debug(
"Found minor issues in microgrid: %s | Protobuf message:\n%s",
", ".join(minor_issues),
message,
)

return Microgrid(
id=MicrogridId(message.id),
enterprise_id=EnterpriseId(message.enterprise_id),
Expand Down
Loading
Loading