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 2 commits into
base: v0.x.x
Choose a base branch
from
+443 −22
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
2 commits
Select commit Hold shift + click to select a range
939af90
Add raise_on_errors parameter to parsing functions
eduardiazf Jan 26, 2026
86debfc
refactor(raise_on_errors): replace ParsingError with ValidationError …
eduardiazf Jan 29, 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
129 changes: 122 additions & 7 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,25 @@
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,
InvalidConnectionErrorGroup,
InvalidElectricalComponentError,
InvalidElectricalComponentErrorGroup,
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 +101,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 +135,47 @@ 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 +138 to +153
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 +138 to +153
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
[InvalidElectricalComponentErrorGroup][frequenz.client.assets.exceptions.InvalidElectricalComponentErrorGroup]
when major validation issues are found in any component instead
of just logging them.

Returns:
The electrical components of the microgrid.

Raises:
InvalidElectricalComponentErrorGroup: If `raise_on_errors` is True
and major validation issues are found.
"""
response = await call_stub_method(
self,
Expand All @@ -138,6 +188,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 InvalidElectricalComponentErrorGroup(
valid_components=components,
exceptions=exceptions,
)
return components
Comment on lines +191 to +218
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 +191 to +218
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
]
Comment on lines 220 to 222
Copy link
Contributor

@llucax llucax Jan 26, 2026

Choose a reason for hiding this comment

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

Same, but we could use a ExceptionGroup to raise many exceptions:

components = []
exceptions = []
for component_pb in response.components:
    major_issues = []  # minor_issues...
    component = electrical_component_from_proto_with_issues(component_pb, major_issues, ...)
    if major_issues:
        exceptions.append(
            InvalidElectricalComponentError(component, component_pb, major_issues, ...)
        )
    else:
        components.append(component)

if exceptions:
    raise InvalidElectricalComponentErrorGroup(valid_components=components, exceptions)

return components

Expand All @@ -147,6 +226,8 @@ async def list_microgrid_electrical_component_connections(
microgrid_id: MicrogridId,
source_component_ids: Iterable[ElectricalComponentId] = (),
destination_component_ids: Iterable[ElectricalComponentId] = (),
*,
raise_on_errors: bool = False,
) -> list[ComponentConnection | None]:
"""
Get the electrical component connections of a microgrid.
Expand All @@ -158,9 +239,17 @@ 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
[InvalidConnectionErrorGroup][frequenz.client.assets.exceptions.InvalidConnectionErrorGroup]
when major validation issues are found in any connection instead
of just logging them.

Returns:
The electrical component connections of the microgrid.

Raises:
InvalidConnectionErrorGroup: If `raise_on_errors` is True and
major validation issues are found.
"""
request = assets_pb2.ListMicrogridElectricalComponentConnectionsRequest(
microgrid_id=int(microgrid_id),
Expand All @@ -177,6 +266,32 @@ async def list_microgrid_electrical_component_connections(
method_name="ListMicrogridElectricalComponentConnections",
)

if raise_on_errors:
connections: list[ComponentConnection | None] = []
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:
connections.append(connection)
if exceptions:
raise InvalidConnectionErrorGroup(
valid_connections=[c for c in connections if c is not None],
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 list comprehension filtering out None values is redundant. At line 286, connections are only appended when connection is not None, so the connections list will never contain None values at this point. You can simplify this to just valid_connections=connections.

Suggested change
valid_connections=[c for c in connections if c is not None],
valid_connections=connections,

Copilot uses AI. Check for mistakes.
exceptions=exceptions,
)
return connections
Comment on lines +269 to +293
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 InvalidConnectionErrorGroup is raised correctly when major validation issues are found with raise_on_errors=True, or that the exception contains the expected valid_connections and exceptions. Consider adding comprehensive test cases covering: 1) all connections valid with raise_on_errors=True, 2) some connections with validation failures, 3) all connections failing validation, and 4) edge cases like empty connection lists.

Copilot uses AI. Check for mistakes.

return list(
map(
component_connection_from_proto,
Expand Down
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