feat: Add `raise_on_errors` parameter for strict parsing validation by eduardiazf · Pull Request #73 · frequenz-floss/frequenz-client-assets-python

eduardiazf · 2026-01-26T10:29:25Z

Summary

Add raise_on_errors: bool = False parameter to parsing functions. When True, raises ParsingError with major_issues, minor_issues, and raw_message attributes instead of logging.

Changes

Add ParsingError exception in exceptions.py
Add raise_on_errors parameter to:
- electrical_component_proto()
- component_connection_from_proto()
- microgrid_from_proto()
- AssetsApiClient methods

Resources

https://docs.python.org/3/library/exceptions.html

llucax

I wrote this having an exception hierarchy like the following in mind:

ValidationError <--+--- InvalidMicrogridError
                   |
                   +--- InvalidElectricalComponentError
                   |
                   +--- ValidationErrorGroup (inherits from ExceptionGroup too) <------ InvalidElectricalComponentErrorGroup

I know this could be a bit overkill for a first version, so for a first approach I think we could just have one simple ValidationError and raise that with the first issue we find.

What I think is more important is not to add the raise_on_errors to xxx_from_proto but instead using the existing xxx_from_proto_with_issues.

src/frequenz/client/assets/electrical_component/_connection_proto.py

src/frequenz/client/assets/_client.py

cwasicki

Thanks a lot, I think Luca's comments are reasonable, otherwise looks good to me.

llucax

Other than the one comment, LGTM.

src/frequenz/client/assets/exceptions.py

Copilot

Pull request overview

This pull request adds a raise_on_errors parameter to parsing functions in the Assets API client, enabling strict validation mode that raises exceptions instead of logging validation issues.

Changes:

Added new validation exception classes (ValidationError, InvalidMicrogridError, InvalidElectricalComponentError, InvalidConnectionError, and their error group variants) in exceptions.py
Refactored parsing functions to separate issue collection from logging (e.g., microgrid_from_proto_with_issues)
Added raise_on_errors parameter to AssetsApiClient methods (get_microgrid, list_microgrid_electrical_components, list_microgrid_electrical_component_connections)

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.

File	Description
src/frequenz/client/assets/exceptions.py	Defines new ValidationError exception hierarchy including base class, specific error types for different entities, and error group classes for batch validation
src/frequenz/client/assets/_microgrid_proto.py	Refactors microgrid parsing to extract microgrid_from_proto_with_issues function that collects validation issues without logging
src/frequenz/client/assets/_client.py	Implements raise_on_errors parameter in three API methods to raise validation exceptions when strict mode is enabled

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-19T16:52:50Z

src/frequenz/client/assets/_client.py

+        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


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 · 2026-02-19T16:52:50Z

src/frequenz/client/assets/_client.py

+        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],
+                    exceptions=exceptions,
+                )
+            return connections


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 · 2026-02-19T16:52:51Z

src/frequenz/client/assets/_client.py

+        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


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 · 2026-02-19T16:52:51Z

src/frequenz/client/assets/_client.py

+        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


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 · 2026-02-19T16:52:51Z

src/frequenz/client/assets/exceptions.py

+            raw_message: The protobuf message that failed validation.
+        """
+        issues_summary = ", ".join(major_issues)
+        super().__init__(f"Validation failed: {issues_summary}")


The error message will be "Validation failed: " (with trailing space and no details) if major_issues is empty. While the current usage in the codebase always checks for non-empty major_issues before raising this exception, this creates a fragile API. Consider either adding validation to reject empty major_issues, or improving the message format to handle this edge case, such as "Validation failed" without the colon when there are no issues to list.

Suggested change

super().__init__(f"Validation failed: {issues_summary}")

message = (

f"Validation failed: {issues_summary}"

if issues_summary

else "Validation failed"

)

super().__init__(message)

Copilot · 2026-02-19T16:52:52Z

src/frequenz/client/assets/_client.py

+                    connections.append(connection)
+            if exceptions:
+                raise InvalidConnectionErrorGroup(
+                    valid_connections=[c for c in connections if c is not None],


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 · 2026-02-19T16:52:52Z

src/frequenz/client/assets/_client.py

+        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


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.

llucax

Sorry, my suggestion to inherit from ExceptionGroup and our own singular exception was flawed and could break exception groups handling.

On the plus side, I think using plain ExceptionGroups simplifies the exception hierarchy and code quite a bit.

src/frequenz/client/assets/_client.py

src/frequenz/client/assets/exceptions.py

src/frequenz/client/assets/_client.py

eduardiazf · 2026-03-11T09:46:41Z

Sorry, my suggestion to inherit from ExceptionGroup and our own singular exception was flawed and could break exception groups handling.

On the plus side, I think using plain ExceptionGroups simplifies the exception hierarchy and code quite a bit.

Agreed, makes sense. Removing all ExceptionGroup subclasses and using plain ExceptionGroup directly will simplify things quite a bit. Will update the code accordingly.

llucax

Except for this small typing issue, LGTM!

I would consider reversing the raise_on_errors default to True. It is a breaking change, but I think it is the safest default. It looks to me like not raising is the niche use case.

src/frequenz/client/assets/_client.py

eduardiazf · 2026-03-12T22:25:28Z

Except for this small typing issue, LGTM!

I would consider reversing the raise_on_errors default to True. It is a breaking change, but I think it is the safest default. It looks to me like not raising is the niche use case.

I think we can change the default value to True in a separate PR so we can merge this one without extending it by having to modify the tests in this PR.

Signed-off-by: eduardiazf <eduardiazf@gmail.com>

…hierarchy and move validation logic to client level Signed-off-by: eduardiazf <eduardiazf@gmail.com>

Signed-off-by: eduardiazf <eduardiazf@gmail.com>

llucax

Approving as it is a very minor thing and can be also be fixed in a follow-up PR, so we can get this one finally merged.

llucax · 2026-03-13T07:56:33Z

src/frequenz/client/assets/_client.py

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


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

]

eduardiazf requested review from a team as code owners January 26, 2026 10:29

eduardiazf requested a review from ela-kotulska-frequenz January 26, 2026 10:29

eduardiazf force-pushed the fix/parsing-component-connection-errors branch from 9052e0b to 01f668d Compare January 26, 2026 10:29

eduardiazf added the cmd:skip-release-notes It is not necessary to update release notes for this PR label Jan 26, 2026

llucax reviewed Jan 26, 2026

View reviewed changes

src/frequenz/client/assets/electrical_component/_connection_proto.py Outdated Show resolved Hide resolved

src/frequenz/client/assets/_client.py Outdated Show resolved Hide resolved

src/frequenz/client/assets/_client.py Show resolved Hide resolved

cwasicki reviewed Jan 27, 2026

View reviewed changes

eduardiazf force-pushed the fix/parsing-component-connection-errors branch 2 times, most recently from 6cbf873 to 86debfc Compare January 29, 2026 12:03

eduardiazf requested a review from llucax February 19, 2026 10:33

llucax requested a review from Copilot February 19, 2026 16:46

Copilot started reviewing on behalf of llucax February 19, 2026 16:47 View session

llucax reviewed Feb 19, 2026

View reviewed changes

src/frequenz/client/assets/exceptions.py Outdated Show resolved Hide resolved

Copilot AI reviewed Feb 19, 2026

View reviewed changes

eduardiazf force-pushed the fix/parsing-component-connection-errors branch 2 times, most recently from 2a67212 to d024111 Compare March 5, 2026 13:11

github-actions bot added part:docs Affects the documentation part:tests Affects the unit, integration and performance (benchmarks) tests labels Mar 5, 2026

eduardiazf force-pushed the fix/parsing-component-connection-errors branch from d024111 to f6ff8c5 Compare March 5, 2026 13:22

eduardiazf requested a review from llucax March 5, 2026 13:25

cwasicki previously approved these changes Mar 9, 2026

View reviewed changes

llucax reviewed Mar 10, 2026

View reviewed changes

eduardiazf dismissed cwasicki’s stale review via 85d2357 March 11, 2026 10:19

eduardiazf force-pushed the fix/parsing-component-connection-errors branch from 85d2357 to 24bc406 Compare March 11, 2026 10:20

eduardiazf requested a review from llucax March 11, 2026 10:23

llucax reviewed Mar 11, 2026

View reviewed changes

src/frequenz/client/assets/_client.py Outdated Show resolved Hide resolved

eduardiazf force-pushed the fix/parsing-component-connection-errors branch from fb48aa6 to 1b6ab4b Compare March 12, 2026 22:27

Add raise_on_errors parameter to parsing functions

0525e6b

Signed-off-by: eduardiazf <eduardiazf@gmail.com>

eduardiazf added 5 commits March 12, 2026 23:29

refactor(raise_on_errors): replace ParsingError with ValidationError …

ad38047

…hierarchy and move validation logic to client level Signed-off-by: eduardiazf <eduardiazf@gmail.com>

fix: Handle empty major_issues in ValidationError message formatting

f8570e1

Signed-off-by: eduardiazf <eduardiazf@gmail.com>

fix: Remove redundant None filter in valid_connections list

d3acea6

Signed-off-by: eduardiazf <eduardiazf@gmail.com>

refactor: Replace ExceptionGroup subclasses with plain ExceptionGroup

fcaeedb

Signed-off-by: eduardiazf <eduardiazf@gmail.com>

fix: Fix return type

9fad9ab

Signed-off-by: eduardiazf <eduardiazf@gmail.com>

eduardiazf force-pushed the fix/parsing-component-connection-errors branch from 1b6ab4b to 9fad9ab Compare March 12, 2026 22:29

eduardiazf requested a review from llucax March 12, 2026 22:30

llucax approved these changes Mar 13, 2026

View reviewed changes

-        super().__init__(f"Validation failed: {issues_summary}")
+        message = (
+            f"Validation failed: {issues_summary}"
+            if issues_summary
+            else "Validation failed"
+        )
+        super().__init__(message)

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

Conversation

eduardiazf commented Jan 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Resources

Uh oh!

llucax left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cwasicki left a comment

Choose a reason for hiding this comment

Uh oh!

llucax left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 19, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 19, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 19, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 19, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 19, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 19, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 19, 2026

Choose a reason for hiding this comment

Uh oh!

llucax left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

eduardiazf commented Mar 11, 2026

Uh oh!

llucax left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

eduardiazf commented Mar 12, 2026

Uh oh!

llucax left a comment

Choose a reason for hiding this comment

Uh oh!

llucax Mar 13, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

eduardiazf commented Jan 26, 2026 •

edited

Loading

llucax left a comment •

edited

Loading