feat: Add raise_on_errors parameter for strict parsing validation

[eduardiazf]

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.

[llucax]

I wouldn't add this here, I would add this at the client level.

[llucax]

Do the error handling here instead:

if raise_on_errors:
    major_issues = []
    minor_issues = []
    microgrid = microgrid_from_proto_with_issues(response.microgrid, major_issues, minor_issues)
    # For now I would ignore minor_issues, I have the feeling we should only have
    # "errors" in the future, this minor/minor distinction is not really useful
    if major_issues:
        raise InvalidMicrogridError(microgrid, response.microgrid, major_errors, minor_errors)
    return microgrid
else:
    microgrid_from_proto(response.microgrid, major_issues)

EDIT: Updated example to only call xxx_with_issues() if `raise_on_errors is true.

I would use InvalidMicrogridError rather than ParseError as we are not really parsing anything here, protobuf messages are already parsed and have no problem at the protobuf-level, is just some validations we are doing that don't pass.

In the future we might even change the argument for something like on_validation_error=LOG|RAISE_ONCE|RAISE_ALL|IGNORE (for now I think we can keep it simple and leave the raise_on_errors).

[llucax]

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

[cwasicki]

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

[llucax]

Other than the one comment, LGTM.

[llucax]

Is it really necessary to override the __new__() to pass extra arguments to a ExceptionGroup? 🤔

[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]

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]

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]

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]

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]

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]

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]

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.