From 77a3770e502259f42b507e82e97544c108f3984f Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Tue, 28 May 2024 12:06:57 +0200 Subject: [PATCH 1/9] Remove old commented out code from test Signed-off-by: Leandro Lucarella --- tests/test_client.py | 21 --------------------- 1 file changed, 21 deletions(-) diff --git a/tests/test_client.py b/tests/test_client.py index a693cfc..3bb9025 100644 --- a/tests/test_client.py +++ b/tests/test_client.py @@ -33,27 +33,6 @@ ) from frequenz.client.microgrid._connection import Connection -# @contextlib.asynccontextmanager -# async def _gprc_server( -# servicer: mock_api.MockMicrogridServicer | None = None, -# ) -> AsyncIterator[tuple[mock_api.MockMicrogridServicer, ApiClient]]: -# global _CURRENT_PORT # pylint: disable=global-statement -# port = _CURRENT_PORT -# _CURRENT_PORT += 1 -# if servicer is None: -# servicer = mock_api.MockMicrogridServicer() -# server = mock_api.MockGrpcServer(servicer, port=port) -# client = ApiClient( -# grpc.aio.insecure_channel(f"[::]:{port}"), -# f"[::]:{port}", -# retry_strategy=LinearBackoff(interval=0.0, jitter=0.05), -# ) -# await server.start() -# try: -# yield servicer, client -# finally: -# assert await server.graceful_shutdown() - class _TestClient(ApiClient): def __init__(self, *, retry_strategy: retry.Strategy | None = None) -> None: From 999dbae5dd60fc9d316e8b28b5ca323975cda169 Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Fri, 17 May 2024 09:47:11 +0200 Subject: [PATCH 2/9] Add specific errors for different gRPC statuses This makes error handling more pythonic, as one can now just catch the exception type one is interested in, without having to do a second-level matching using the status. It also helps avoiding to expose the grpclib classes to the user. Signed-off-by: Leandro Lucarella --- src/frequenz/client/microgrid/__init__.py | 38 +- src/frequenz/client/microgrid/_exception.py | 523 ++++++++++++++++++++ 2 files changed, 560 insertions(+), 1 deletion(-) diff --git a/src/frequenz/client/microgrid/__init__.py b/src/frequenz/client/microgrid/__init__.py index 85819df..c65a12b 100644 --- a/src/frequenz/client/microgrid/__init__.py +++ b/src/frequenz/client/microgrid/__init__.py @@ -27,7 +27,26 @@ ) from ._component_states import EVChargerCableState, EVChargerComponentState from ._connection import Connection -from ._exception import ClientError +from ._exception import ( + ClientError, + DataLoss, + EntityAlreadyExists, + EntityNotFound, + GrpcStatusError, + InternalError, + InvalidArgument, + OperationAborted, + OperationCancelled, + OperationNotImplemented, + OperationOutOfRange, + OperationPreconditionFailed, + OperationTimedOut, + OperationUnauthenticated, + PermissionDenied, + ResourceExhausted, + ServiceUnavailable, + UnknownError, +) from ._metadata import Location, Metadata __all__ = [ @@ -41,14 +60,31 @@ "ComponentMetricId", "ComponentType", "Connection", + "DataLoss", "EVChargerCableState", "EVChargerComponentState", "EVChargerData", + "EntityAlreadyExists", + "EntityNotFound", "Fuse", "GridMetadata", + "GrpcStatusError", + "InternalError", + "InvalidArgument", "InverterData", "InverterType", "Location", "Metadata", "MeterData", + "OperationAborted", + "OperationCancelled", + "OperationNotImplemented", + "OperationOutOfRange", + "OperationPreconditionFailed", + "OperationTimedOut", + "OperationUnauthenticated", + "PermissionDenied", + "ResourceExhausted", + "ServiceUnavailable", + "UnknownError", ] diff --git a/src/frequenz/client/microgrid/_exception.py b/src/frequenz/client/microgrid/_exception.py index 89d4386..46904d4 100644 --- a/src/frequenz/client/microgrid/_exception.py +++ b/src/frequenz/client/microgrid/_exception.py @@ -3,6 +3,529 @@ """Exceptions raised by the microgrid API client.""" +from __future__ import annotations + +from typing import Protocol + +import grpclib + class ClientError(Exception): """There was an error in the microgrid API client.""" + + def __init__( + self, + *, + server_url: str, + operation: str, + description: str, + ) -> None: + """Create a new instance. + + Args: + server_url: The URL of the server that returned the error. + operation: The operation that caused the error. + description: A human-readable description of the error. + """ + super().__init__( + f"Failed calling {operation!r} on {server_url!r}: {description}" + ) + + self.server_url = server_url + """The URL of the server that returned the error.""" + + self.operation = operation + """The operation that caused the error.""" + + self.description = description + """The human-readable description of the error.""" + + @classmethod + def from_grpc_error( + cls, + *, + server_url: str, + operation: str, + grpc_error: grpclib.GRPCError, + retryable: bool = True, + ) -> GrpcStatusError: + """Create an instance of the appropriate subclass from a gRPC error. + + Args: + server_url: The URL of the server that returned the error. + operation: The operation that caused the error. + grpc_error: The gRPC error to convert. + + Returns: + An instance of + [GrpcStatusError][frequenz.client.microgrid.GrpcStatusError] if + the gRPC status is not recognized, or an appropriate subclass if it is. + """ + + class Ctor(Protocol): + """A protocol for the constructor of a subclass of `GrpcStatusError`.""" + + def __call__( + self, *, server_url: str, operation: str, grpc_error: grpclib.GRPCError + ) -> GrpcStatusError: ... + + status_map: dict[grpclib.Status, Ctor] = { + grpclib.Status.CANCELLED: OperationCancelled, + grpclib.Status.UNKNOWN: UnknownError, + grpclib.Status.INVALID_ARGUMENT: InvalidArgument, + grpclib.Status.DEADLINE_EXCEEDED: OperationTimedOut, + grpclib.Status.NOT_FOUND: EntityNotFound, + grpclib.Status.ALREADY_EXISTS: EntityAlreadyExists, + grpclib.Status.PERMISSION_DENIED: PermissionDenied, + grpclib.Status.RESOURCE_EXHAUSTED: ResourceExhausted, + grpclib.Status.FAILED_PRECONDITION: OperationPreconditionFailed, + grpclib.Status.ABORTED: OperationAborted, + grpclib.Status.OUT_OF_RANGE: OperationOutOfRange, + grpclib.Status.UNIMPLEMENTED: OperationNotImplemented, + grpclib.Status.INTERNAL: InternalError, + grpclib.Status.UNAVAILABLE: ServiceUnavailable, + grpclib.Status.DATA_LOSS: DataLoss, + grpclib.Status.UNAUTHENTICATED: OperationUnauthenticated, + } + + if ctor := status_map.get(grpc_error.status): + return ctor( + server_url=server_url, operation=operation, grpc_error=grpc_error + ) + return GrpcStatusError( + server_url=server_url, + operation=operation, + description="Got an unrecognized status code", + grpc_error=grpc_error, + ) + + +class GrpcStatusError(ClientError): + """The gRPC server returned an error status code. + + These errors are specific to gRPC. If you want to use the client in + a protocol-independent way, you should avoid catching this exception. + + References: + * [gRPC status + codes](https://github.com/grpc/grpc/blob/master/doc/statuscodes.md) + """ + + def __init__( + self, + *, + server_url: str, + operation: str, + description: str, + grpc_error: grpclib.GRPCError, + ) -> None: + """Create a new instance. + + Args: + server_url: The URL of the server that returned the error. + operation: The operation that caused the error. + description: A human-readable description of the error. + grpc_error: The gRPC error originating this exception. + """ + message = f": {grpc_error.message}" if grpc_error.message else "" + details = f" ({grpc_error.details})" if grpc_error.details else "" + super().__init__( + server_url=server_url, + operation=operation, + description=f"{description} {message}{details}", + ) + self.description = description + + self.grpc_error = grpc_error + """The original gRPC error.""" + + +class OperationCancelled(GrpcStatusError): + """The operation was cancelled.""" + + def __init__( + self, *, server_url: str, operation: str, grpc_error: grpclib.GRPCError + ) -> None: + """Create a new instance. + + Args: + server_url: The URL of the server that returned the error. + operation: The operation that caused the error. + grpc_error: The gRPC error originating this exception. + """ + super().__init__( + server_url=server_url, + operation=operation, + description="The operation was cancelled", + grpc_error=grpc_error, + ) + + +class UnknownError(GrpcStatusError): + """There was an error that can't be described using other statuses.""" + + def __init__( + self, *, server_url: str, operation: str, grpc_error: grpclib.GRPCError + ) -> None: + """Create a new instance. + + Args: + server_url: The URL of the server that returned the error. + operation: The operation that caused the error. + grpc_error: The gRPC error originating this exception. + """ + super().__init__( + server_url=server_url, + operation=operation, + description="There was an error that can't be described using other statuses", + grpc_error=grpc_error, + ) + + +class InvalidArgument(GrpcStatusError): + """The client specified an invalid argument. + + Note that this error differs from + [OperationPreconditionFailed][frequenz.client.microgrid.OperationPreconditionFailed]. + This error indicates arguments that are problematic regardless of the state of the + system (e.g., a malformed file name). + """ + + def __init__( + self, *, server_url: str, operation: str, grpc_error: grpclib.GRPCError + ) -> None: + """Create a new instance. + + Args: + server_url: The URL of the server that returned the error. + operation: The operation that caused the error. + grpc_error: The gRPC error originating this exception. + """ + super().__init__( + server_url=server_url, + operation=operation, + description="The client specified an invalid argument", + grpc_error=grpc_error, + ) + + +class OperationTimedOut(GrpcStatusError): + """The time limit was exceeded while waiting for the operationt o complete. + + For operations that change the state of the system, this error may be returned even + if the operation has completed successfully. For example, a successful response from + a server could have been delayed long. + """ + + def __init__( + self, *, server_url: str, operation: str, grpc_error: grpclib.GRPCError + ) -> None: + """Create a new instance. + + Args: + server_url: The URL of the server that returned the error. + operation: The operation that caused the error. + grpc_error: The gRPC error originating this exception. + """ + super().__init__( + server_url=server_url, + operation=operation, + description="The time limit was exceeded while waiting for the operation " + "to complete", + grpc_error=grpc_error, + ) + + +class EntityNotFound(GrpcStatusError): + """The requested entity was not found. + + Note that this error differs from + [PermissionDenied][frequenz.client.microgrid.PermissionDenied]. This error is + used when the requested entity is not found, regardless of the user's permissions. + """ + + def __init__( + self, *, server_url: str, operation: str, grpc_error: grpclib.GRPCError + ) -> None: + """Create a new instance. + + Args: + server_url: The URL of the server that returned the error. + operation: The operation that caused the error. + grpc_error: The gRPC error originating this exception. + """ + super().__init__( + server_url=server_url, + operation=operation, + description="The requested entity was not found", + grpc_error=grpc_error, + ) + + +class EntityAlreadyExists(GrpcStatusError): + """The entity that we attempted to create already exists.""" + + def __init__( + self, *, server_url: str, operation: str, grpc_error: grpclib.GRPCError + ) -> None: + """Create a new instance. + + Args: + server_url: The URL of the server that returned the error. + operation: The operation that caused the error. + grpc_error: The gRPC error originating this exception. + """ + super().__init__( + server_url=server_url, + operation=operation, + description="The entity that we attempted to create already exists", + grpc_error=grpc_error, + ) + + +class PermissionDenied(GrpcStatusError): + """The caller does not have permission to execute the specified operation. + + Note that when the operation is rejected due to other reasons, such as the resources + being exhausted or the user not being authenticated at all, different errors should + be catched instead + ([ResourceExhausted][frequenz.client.microgrid.ResourceExhausted] and + [OperationUnauthenticated][frequenz.client.microgrid.OperationUnauthenticated] + respectively). + """ + + def __init__( + self, *, server_url: str, operation: str, grpc_error: grpclib.GRPCError + ) -> None: + """Create a new instance. + + Args: + server_url: The URL of the server that returned the error. + operation: The operation that caused the error. + grpc_error: The gRPC error originating this exception. + """ + super().__init__( + server_url=server_url, + operation=operation, + description="The caller does not have permission to execute the specified " + "operation", + grpc_error=grpc_error, + ) + + +class ResourceExhausted(GrpcStatusError): + """Some resource has been exhausted (for example per-user quota, disk space, etc.).""" + + def __init__( + self, *, server_url: str, operation: str, grpc_error: grpclib.GRPCError + ) -> None: + """Create a new instance. + + Args: + server_url: The URL of the server that returned the error. + operation: The operation that caused the error. + grpc_error: The gRPC error originating this exception. + """ + super().__init__( + server_url=server_url, + operation=operation, + description="Some resource has been exhausted (for example per-user quota, " + "disk space, etc.)", + grpc_error=grpc_error, + ) + + +class OperationPreconditionFailed(GrpcStatusError): + """The operation was rejected because the system is not in a required state. + + For example, the directory to be deleted is non-empty, an rmdir operation is applied + to a non-directory, etc. The user should perform some corrective action before + retrying the operation. + """ + + def __init__( + self, *, server_url: str, operation: str, grpc_error: grpclib.GRPCError + ) -> None: + """Create a new instance. + + Args: + server_url: The URL of the server that returned the error. + operation: The operation that caused the error. + grpc_error: The gRPC error originating this exception. + """ + super().__init__( + server_url=server_url, + operation=operation, + description="The operation was rejected because the system is not in a " + "required state", + grpc_error=grpc_error, + ) + + +class OperationAborted(GrpcStatusError): + """The operation was aborted. + + Typically due to a concurrency issue or transaction abort. + """ + + def __init__( + self, *, server_url: str, operation: str, grpc_error: grpclib.GRPCError + ) -> None: + """Create a new instance. + + Args: + server_url: The URL of the server that returned the error. + operation: The operation that caused the error. + grpc_error: The gRPC error originating this exception. + """ + super().__init__( + server_url=server_url, + operation=operation, + description="The operation was aborted", + grpc_error=grpc_error, + ) + + +class OperationOutOfRange(GrpcStatusError): + """The operation was attempted past the valid range. + + Unlike [InvalidArgument][frequenz.client.microgrid.InvalidArgument], this + error indicates a problem that may be fixed if the system state changes. + + There is a fair bit of overlap with + [OperationPreconditionFailed][frequenz.client.microgrid.OperationPreconditionFailed], + this error is just a more specific version of that error and could be the result of + an operation that doesn't even take any arguments. + """ + + def __init__( + self, *, server_url: str, operation: str, grpc_error: grpclib.GRPCError + ) -> None: + """Create a new instance. + + Args: + server_url: The URL of the server that returned the error. + operation: The operation that caused the error. + grpc_error: The gRPC error originating this exception. + """ + super().__init__( + server_url=server_url, + operation=operation, + description="The operation was attempted past the valid range", + grpc_error=grpc_error, + ) + + +class OperationNotImplemented(GrpcStatusError): + """The operation is not implemented or not supported/enabled in this service.""" + + def __init__( + self, *, server_url: str, operation: str, grpc_error: grpclib.GRPCError + ) -> None: + """Create a new instance. + + Args: + server_url: The URL of the server that returned the error. + operation: The operation that caused the error. + grpc_error: The gRPC error originating this exception. + """ + super().__init__( + server_url=server_url, + operation=operation, + description="The operation is not implemented or not supported/enabled in " + "this service", + grpc_error=grpc_error, + ) + + +class InternalError(GrpcStatusError): + """Some invariants expected by the underlying system have been broken. + + This error code is reserved for serious errors. + """ + + def __init__( + self, *, server_url: str, operation: str, grpc_error: grpclib.GRPCError + ) -> None: + """Create a new instance. + + Args: + server_url: The URL of the server that returned the error. + operation: The operation that caused the error. + grpc_error: The gRPC error originating this exception. + """ + super().__init__( + server_url=server_url, + operation=operation, + description="Some invariants expected by the underlying system have been " + "broken", + grpc_error=grpc_error, + ) + + +class ServiceUnavailable(GrpcStatusError): + """The service is currently unavailable. + + This is most likely a transient condition, which can be corrected by retrying with + a backoff. Note that it is not always safe to retry non-idempotent operations. + """ + + def __init__( + self, *, server_url: str, operation: str, grpc_error: grpclib.GRPCError + ) -> None: + """Create a new instance. + + Args: + server_url: The URL of the server that returned the error. + operation: The operation that caused the error. + grpc_error: The gRPC error originating this exception. + """ + super().__init__( + server_url=server_url, + operation=operation, + description="The service is currently unavailable", + grpc_error=grpc_error, + ) + + +class DataLoss(GrpcStatusError): + """Unrecoverable data loss or corruption.""" + + def __init__( + self, *, server_url: str, operation: str, grpc_error: grpclib.GRPCError + ) -> None: + """Create a new instance. + + Args: + server_url: The URL of the server that returned the error. + operation: The operation that caused the error. + grpc_error: The gRPC error originating this exception. + """ + super().__init__( + server_url=server_url, + operation=operation, + description="Unrecoverable data loss or corruption", + grpc_error=grpc_error, + ) + + +class OperationUnauthenticated(GrpcStatusError): + """The request does not have valid authentication credentials for the operation.""" + + def __init__( + self, *, server_url: str, operation: str, grpc_error: grpclib.GRPCError + ) -> None: + """Create a new instance. + + Args: + server_url: The URL of the server that returned the error. + operation: The operation that caused the error. + grpc_error: The gRPC error originating this exception. + """ + super().__init__( + server_url=server_url, + operation=operation, + description="The request does not have valid authentication credentials " + "for the operation", + grpc_error=grpc_error, + ) From 89964c919af93b10c1012ffaa23d6872006c6b4f Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Mon, 27 May 2024 12:05:52 +0200 Subject: [PATCH 3/9] Raise the new exceptions in the client Signed-off-by: Leandro Lucarella --- src/frequenz/client/microgrid/_client.py | 70 ++++++++++++------------ tests/test_client.py | 28 ++++++---- 2 files changed, 52 insertions(+), 46 deletions(-) diff --git a/src/frequenz/client/microgrid/_client.py b/src/frequenz/client/microgrid/_client.py index 35d37cf..c1397e4 100644 --- a/src/frequenz/client/microgrid/_client.py +++ b/src/frequenz/client/microgrid/_client.py @@ -90,19 +90,21 @@ async def components(self) -> Iterable[Component]: Iterator whose elements are all the components in the microgrid. Raises: - ClientError: If the connection to the Microgrid API cannot be established or - when the api call exceeded the timeout. + ClientError: If the are any errors communicating with the Microgrid API, + most likely a subclass of + [GrpcStatusError][frequenz.client.microgrid.GrpcStatusError]. """ try: component_list = await self.api.list_components( pb_microgrid.ComponentFilter(), timeout=int(DEFAULT_GRPC_CALL_TIMEOUT), ) - - except grpclib.GRPCError as err: - raise ClientError( - f"Failed to list components. Microgrid API: {self._server_url}. Err: {err}" - ) from err + except grpclib.GRPCError as grpc_error: + raise ClientError.from_grpc_error( + server_url=self._server_url, + operation="list_components", + grpc_error=grpc_error, + ) from grpc_error components_only = filter( lambda c: c.category @@ -168,8 +170,9 @@ async def connections( Microgrid connections matching the provided start and end filters. Raises: - ClientError: If the connection to the Microgrid API cannot be established or - when the api call exceeded the timeout. + ClientError: If the are any errors communicating with the Microgrid API, + most likely a subclass of + [GrpcStatusError][frequenz.client.microgrid.GrpcStatusError]. """ connection_filter = pb_microgrid.ConnectionFilter( starts=list(starts), ends=list(ends) @@ -182,10 +185,12 @@ async def connections( timeout=int(DEFAULT_GRPC_CALL_TIMEOUT), ), ) - except grpclib.GRPCError as err: - raise ClientError( - f"Failed to list connections. Microgrid API: {self._server_url}. Err: {err}" - ) from err + except grpclib.GRPCError as grpc_error: + raise ClientError.from_grpc_error( + server_url=self._server_url, + operation="list_connections", + grpc_error=grpc_error, + ) from grpc_error # Filter out the components filtered in `components` method. # id=0 is an exception indicating grid component. valid_ids = {c.component_id for c in valid_components} @@ -384,8 +389,9 @@ async def set_power(self, component_id: int, power_w: float) -> None: power_w: power to set for the component. Raises: - ClientError: If the connection to the Microgrid API cannot be established or - when the api call exceeded the timeout. + ClientError: If the are any errors communicating with the Microgrid API, + most likely a subclass of + [GrpcStatusError][frequenz.client.microgrid.GrpcStatusError]. """ try: await self.api.set_power_active( @@ -394,10 +400,12 @@ async def set_power(self, component_id: int, power_w: float) -> None: ), timeout=int(DEFAULT_GRPC_CALL_TIMEOUT), ) - except grpclib.GRPCError as err: - raise ClientError( - f"Failed to set power. Microgrid API: {self._server_url}. Err: {err}" - ) from err + except grpclib.GRPCError as grpc_error: + raise ClientError.from_grpc_error( + server_url=self._server_url, + operation="set_power_active", + grpc_error=grpc_error, + ) from grpc_error async def set_bounds( self, @@ -415,10 +423,10 @@ async def set_bounds( Raises: ValueError: when upper bound is less than 0, or when lower bound is greater than 0. - ClientError: If the connection to the Microgrid API cannot be established or - when the api call exceeded the timeout. + ClientError: If the are any errors communicating with the Microgrid API, + most likely a subclass of + [GrpcStatusError][frequenz.client.microgrid.GrpcStatusError]. """ - api_details = f"Microgrid API: {self._server_url}." if upper < 0: raise ValueError(f"Upper bound {upper} must be greater than or equal to 0.") if lower > 0: @@ -436,15 +444,9 @@ async def set_bounds( ), timeout=int(DEFAULT_GRPC_CALL_TIMEOUT), ) - except grpclib.GRPCError as err: - _logger.error( - "set_bounds write failed: %s, for message: %s, api: %s. Err: %s", - err, - next, - api_details, - err, - ) - raise ClientError( - f"Failed to set inclusion bounds. Microgrid API: {self._server_url}. " - f"Err: {err}" - ) from err + except grpclib.GRPCError as grpc_error: + raise ClientError.from_grpc_error( + server_url=self._server_url, + operation="add_inclusion_bounds", + grpc_error=grpc_error, + ) from grpc_error diff --git a/tests/test_client.py b/tests/test_client.py index 3bb9025..26054ce 100644 --- a/tests/test_client.py +++ b/tests/test_client.py @@ -187,12 +187,13 @@ async def test_components_grpc_error() -> None: """Test the components() method when the gRPC call fails.""" client = _TestClient() client.mock_stub.list_components.side_effect = grpclib.GRPCError( - mock.MagicMock(name="mock status"), "fake grpc error" + mock.MagicMock(name="mock_status"), "fake grpc error", "fake details" ) with pytest.raises( ClientError, - match="Failed to list components. Microgrid API: grpc://mock_host:1234. " - "Err: .*fake grpc error", + match=r"Failed calling 'list_components' on 'grpc://mock_host:1234': .* " + r">: fake grpc error " + r"\(fake details\)", ): await client.components() @@ -327,12 +328,13 @@ async def test_connections_grpc_error() -> None: """Test the components() method when the gRPC call fails.""" client = _TestClient() client.mock_stub.list_connections.side_effect = grpclib.GRPCError( - mock.MagicMock(name="mock status"), "fake grpc error" + mock.MagicMock(name="mock_status"), "fake grpc error", "fake details" ) with pytest.raises( ClientError, - match="Failed to list connections. Microgrid API: grpc://mock_host:1234. " - "Err: .*fake grpc error", + match=r"Failed calling 'list_connections' on 'grpc://mock_host:1234': .* " + r">: fake grpc error " + r"\(fake details\)", ): await client.connections() @@ -543,12 +545,13 @@ async def test_set_power_grpc_error() -> None: """Test set_power() raises ClientError when the gRPC call fails.""" client = _TestClient() client.mock_stub.set_power_active.side_effect = grpclib.GRPCError( - mock.MagicMock(name="mock status"), "fake grpc error" + mock.MagicMock(name="mock_status"), "fake grpc error", "fake details" ) with pytest.raises( ClientError, - match="Failed to set power. Microgrid API: grpc://mock_host:1234. " - "Err: .*fake grpc error", + match=r"Failed calling 'set_power_active' on 'grpc://mock_host:1234': .* " + r">: fake grpc error " + r"\(fake details\)", ): await client.set_power(component_id=83, power_w=100.0) @@ -609,11 +612,12 @@ async def test_set_bounds_grpc_error() -> None: """Test the components() method when the gRPC call fails.""" client = _TestClient() client.mock_stub.add_inclusion_bounds.side_effect = grpclib.GRPCError( - mock.MagicMock(name="mock status"), "fake grpc error" + mock.MagicMock(name="mock_status"), "fake grpc error", "fake details" ) with pytest.raises( ClientError, - match="Failed to set inclusion bounds. Microgrid API: grpc://mock_host:1234. " - "Err: .*fake grpc error", + match=r"Failed calling 'add_inclusion_bounds' on 'grpc://mock_host:1234': .* " + r">: fake grpc error " + r"\(fake details\)", ): await client.set_bounds(99, 0.0, 100.0) From 33681cf4f9661d87f59b26be73f1b4fc6b617f62 Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Mon, 27 May 2024 13:06:48 +0200 Subject: [PATCH 4/9] Add a retryable flag to errors This flag can simplify deciding if an operation returning an error can be blindly retried. Some errors might need some intervention to change the system's state, but another actor might do that, so a bling retry might still succeed. Retrying is still largely missing, but it will be solved separately, see: https://github.com/frequenz-floss/frequenz-client-microgrid-python/issues/52 Signed-off-by: Leandro Lucarella --- src/frequenz/client/microgrid/_exception.py | 36 +++++++++++++++++++-- 1 file changed, 34 insertions(+), 2 deletions(-) diff --git a/src/frequenz/client/microgrid/_exception.py b/src/frequenz/client/microgrid/_exception.py index 46904d4..a0b5975 100644 --- a/src/frequenz/client/microgrid/_exception.py +++ b/src/frequenz/client/microgrid/_exception.py @@ -11,7 +11,13 @@ class ClientError(Exception): - """There was an error in the microgrid API client.""" + """There was an error in the microgrid API client. + + To simplify retrying, errors are classified as + [retryable][frequenz.client.microgrid.ClientError.is_retryable], or not. Retryable + errors might succeed if retried, while permanent errors won't. When uncertain, + errors are assumed to be retryable. + """ def __init__( self, @@ -19,6 +25,7 @@ def __init__( server_url: str, operation: str, description: str, + retryable: bool, ) -> None: """Create a new instance. @@ -26,6 +33,7 @@ def __init__( server_url: The URL of the server that returned the error. operation: The operation that caused the error. description: A human-readable description of the error. + retryable: Whether retrying the operation might succeed. """ super().__init__( f"Failed calling {operation!r} on {server_url!r}: {description}" @@ -40,6 +48,9 @@ def __init__( self.description = description """The human-readable description of the error.""" + self.is_retryable = retryable + """Whether retrying the operation might succeed.""" + @classmethod def from_grpc_error( cls, @@ -55,6 +66,7 @@ def from_grpc_error( server_url: The URL of the server that returned the error. operation: The operation that caused the error. grpc_error: The gRPC error to convert. + retryable: Whether retrying the operation might succeed. Returns: An instance of @@ -97,6 +109,7 @@ def __call__( operation=operation, description="Got an unrecognized status code", grpc_error=grpc_error, + retryable=retryable, ) @@ -111,13 +124,14 @@ class GrpcStatusError(ClientError): codes](https://github.com/grpc/grpc/blob/master/doc/statuscodes.md) """ - def __init__( + def __init__( # pylint: disable=too-many-arguments self, *, server_url: str, operation: str, description: str, grpc_error: grpclib.GRPCError, + retryable: bool, ) -> None: """Create a new instance. @@ -126,6 +140,7 @@ def __init__( operation: The operation that caused the error. description: A human-readable description of the error. grpc_error: The gRPC error originating this exception. + retryable: Whether retrying the operation might succeed. """ message = f": {grpc_error.message}" if grpc_error.message else "" details = f" ({grpc_error.details})" if grpc_error.details else "" @@ -133,6 +148,7 @@ def __init__( server_url=server_url, operation=operation, description=f"{description} {message}{details}", + retryable=retryable, ) self.description = description @@ -158,6 +174,7 @@ def __init__( operation=operation, description="The operation was cancelled", grpc_error=grpc_error, + retryable=True, ) @@ -179,6 +196,7 @@ def __init__( operation=operation, description="There was an error that can't be described using other statuses", grpc_error=grpc_error, + retryable=True, # We don't know so we assume it's retryable ) @@ -206,6 +224,7 @@ def __init__( operation=operation, description="The client specified an invalid argument", grpc_error=grpc_error, + retryable=False, ) @@ -233,6 +252,7 @@ def __init__( description="The time limit was exceeded while waiting for the operation " "to complete", grpc_error=grpc_error, + retryable=True, ) @@ -259,6 +279,7 @@ def __init__( operation=operation, description="The requested entity was not found", grpc_error=grpc_error, + retryable=True, # If the entity is added later it might succeed ) @@ -280,6 +301,7 @@ def __init__( operation=operation, description="The entity that we attempted to create already exists", grpc_error=grpc_error, + retryable=True, # If the entity is deleted later it might succeed ) @@ -310,6 +332,7 @@ def __init__( description="The caller does not have permission to execute the specified " "operation", grpc_error=grpc_error, + retryable=True, # If the user is granted permission it might succeed ) @@ -332,6 +355,7 @@ def __init__( description="Some resource has been exhausted (for example per-user quota, " "disk space, etc.)", grpc_error=grpc_error, + retryable=True, # If the resource is freed it might succeed ) @@ -359,6 +383,7 @@ def __init__( description="The operation was rejected because the system is not in a " "required state", grpc_error=grpc_error, + retryable=True, # If the system state changes it might succeed ) @@ -383,6 +408,7 @@ def __init__( operation=operation, description="The operation was aborted", grpc_error=grpc_error, + retryable=True, ) @@ -413,6 +439,7 @@ def __init__( operation=operation, description="The operation was attempted past the valid range", grpc_error=grpc_error, + retryable=True, # If the system state changes it might succeed ) @@ -435,6 +462,7 @@ def __init__( description="The operation is not implemented or not supported/enabled in " "this service", grpc_error=grpc_error, + retryable=False, ) @@ -460,6 +488,7 @@ def __init__( description="Some invariants expected by the underlying system have been " "broken", grpc_error=grpc_error, + retryable=True, # If the system state changes it might succeed ) @@ -485,6 +514,7 @@ def __init__( operation=operation, description="The service is currently unavailable", grpc_error=grpc_error, + retryable=True, # If the service becomes available it might succeed ) @@ -506,6 +536,7 @@ def __init__( operation=operation, description="Unrecoverable data loss or corruption", grpc_error=grpc_error, + retryable=False, ) @@ -528,4 +559,5 @@ def __init__( description="The request does not have valid authentication credentials " "for the operation", grpc_error=grpc_error, + retryable=False, ) From c9f783c0b23da0380bc35ccdac84cb92064086ee Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Mon, 27 May 2024 13:10:00 +0200 Subject: [PATCH 5/9] Make `InvalidArgument` a `ValueError` Signed-off-by: Leandro Lucarella --- src/frequenz/client/microgrid/_exception.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/frequenz/client/microgrid/_exception.py b/src/frequenz/client/microgrid/_exception.py index a0b5975..c23d184 100644 --- a/src/frequenz/client/microgrid/_exception.py +++ b/src/frequenz/client/microgrid/_exception.py @@ -200,7 +200,7 @@ def __init__( ) -class InvalidArgument(GrpcStatusError): +class InvalidArgument(GrpcStatusError, ValueError): """The client specified an invalid argument. Note that this error differs from From fb46558dc386622e16d8960152dde230a3ceb8a0 Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Tue, 28 May 2024 12:07:07 +0200 Subject: [PATCH 6/9] Add tests for the new exceptions Signed-off-by: Leandro Lucarella --- tests/test_exception.py | 243 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 243 insertions(+) create mode 100644 tests/test_exception.py diff --git a/tests/test_exception.py b/tests/test_exception.py new file mode 100644 index 0000000..3391c18 --- /dev/null +++ b/tests/test_exception.py @@ -0,0 +1,243 @@ +# License: MIT +# Copyright © 2024 Frequenz Energy-as-a-Service GmbH + +"""Tests for the microgrid client exceptions.""" + +import re +from typing import Protocol +from unittest import mock + +import grpclib +import pytest + +from frequenz.client.microgrid import ( + ClientError, + DataLoss, + EntityAlreadyExists, + EntityNotFound, + GrpcStatusError, + InternalError, + InvalidArgument, + OperationAborted, + OperationCancelled, + OperationNotImplemented, + OperationOutOfRange, + OperationPreconditionFailed, + OperationTimedOut, + OperationUnauthenticated, + PermissionDenied, + ResourceExhausted, + ServiceUnavailable, + UnknownError, +) + + +class _GrpcStatusErrorCtor(Protocol): + """A protocol for the constructor of a subclass of `GrpcStatusError`.""" + + def __call__( + self, *, server_url: str, operation: str, grpc_error: grpclib.GRPCError + ) -> GrpcStatusError: ... + + +ERROR_TUPLES: list[tuple[type[GrpcStatusError], grpclib.Status, str, bool]] = [ + ( + OperationCancelled, + grpclib.Status.CANCELLED, + "The operation was cancelled", + True, + ), + ( + UnknownError, + grpclib.Status.UNKNOWN, + "There was an error that can't be described using other statuses", + True, + ), + ( + InvalidArgument, + grpclib.Status.INVALID_ARGUMENT, + "The client specified an invalid argument", + False, + ), + ( + OperationTimedOut, + grpclib.Status.DEADLINE_EXCEEDED, + "The time limit was exceeded while waiting for the operation to complete", + True, + ), + ( + EntityNotFound, + grpclib.Status.NOT_FOUND, + "The requested entity was not found", + True, + ), + ( + EntityAlreadyExists, + grpclib.Status.ALREADY_EXISTS, + "The entity that we attempted to create already exists", + True, + ), + ( + PermissionDenied, + grpclib.Status.PERMISSION_DENIED, + "The caller does not have permission to execute the specified operation", + True, + ), + ( + ResourceExhausted, + grpclib.Status.RESOURCE_EXHAUSTED, + "Some resource has been exhausted (for example per-user quota, disk space, etc.)", + True, + ), + ( + OperationPreconditionFailed, + grpclib.Status.FAILED_PRECONDITION, + "The operation was rejected because the system is not in a required state", + True, + ), + (OperationAborted, grpclib.Status.ABORTED, "The operation was aborted", True), + ( + OperationOutOfRange, + grpclib.Status.OUT_OF_RANGE, + "The operation was attempted past the valid range", + True, + ), + ( + OperationNotImplemented, + grpclib.Status.UNIMPLEMENTED, + "The operation is not implemented or not supported/enabled in this service", + False, + ), + ( + InternalError, + grpclib.Status.INTERNAL, + "Some invariants expected by the underlying system have been broken", + True, + ), + ( + ServiceUnavailable, + grpclib.Status.UNAVAILABLE, + "The service is currently unavailable", + True, + ), + ( + DataLoss, + grpclib.Status.DATA_LOSS, + "Unrecoverable data loss or corruption", + False, + ), + ( + OperationUnauthenticated, + grpclib.Status.UNAUTHENTICATED, + "The request does not have valid authentication credentials for the operation", + False, + ), +] + + +@pytest.mark.parametrize( + "exception_class, grpc_status, expected_description, retryable", ERROR_TUPLES +) +def test_grpc_status_error( + exception_class: _GrpcStatusErrorCtor, + grpc_status: grpclib.Status, + expected_description: str, + retryable: bool, +) -> None: + """Test gRPC status errors are correctly created from gRPC errors.""" + grpc_error = grpclib.GRPCError( + grpc_status, "grpc error message", "grpc error details" + ) + exception = exception_class( + server_url="http://testserver", + operation="test_operation", + grpc_error=grpc_error, + ) + + assert exception.server_url == "http://testserver" + assert exception.operation == "test_operation" + assert expected_description == exception.description + assert exception.grpc_error == grpc_error + assert exception.is_retryable == retryable + + +def test_grpc_unknown_status_error() -> None: + """Test that an UnknownError is created for an unknown gRPC status.""" + expected_description = "Test error" + grpc_error = grpclib.GRPCError( + mock.MagicMock(name="unknown_status"), + "grpc error message", + "grpc error details", + ) + exception = GrpcStatusError( + server_url="http://testserver", + operation="test_operation", + description=expected_description, + grpc_error=grpc_error, + retryable=True, + ) + + assert exception.server_url == "http://testserver" + assert exception.operation == "test_operation" + assert expected_description in exception.description + assert exception.grpc_error == grpc_error + assert exception.is_retryable is True + + +def test_client_error() -> None: + """Test the ClientError class.""" + error = ClientError( + server_url="http://testserver", + operation="test_operation", + description="An error occurred", + retryable=True, + ) + + assert error.server_url == "http://testserver" + assert error.operation == "test_operation" + assert error.description == "An error occurred" + assert error.is_retryable is True + + +@pytest.mark.parametrize( + "exception_class, grpc_status, expected_description, retryable", + ERROR_TUPLES + + [ + ( + GrpcStatusError, + mock.MagicMock(name="unknown_status"), + "Got an unrecognized status code", + True, + ) + ], +) +def test_from_grpc_error( + exception_class: type[GrpcStatusError], + grpc_status: grpclib.Status, + expected_description: str, + retryable: bool, +) -> None: + """Test that the from_grpc_error method creates the correct exception.""" + grpc_error = grpclib.GRPCError( + grpc_status, "grpc error message", "grpc error details" + ) + with pytest.raises( + exception_class, + match=r"Failed calling 'test_operation' on 'http://testserver': " + rf"{re.escape(expected_description)} " + rf": " + r"grpc error message \(grpc error details\)", + ) as exc_info: + raise ClientError.from_grpc_error( + server_url="http://testserver", + operation="test_operation", + grpc_error=grpc_error, + ) + + exception = exc_info.value + assert isinstance(exception, exception_class) + assert exception.server_url == "http://testserver" + assert exception.operation == "test_operation" + assert exception.grpc_error == grpc_error + assert expected_description == exception.description + assert exception.is_retryable == retryable From afc135c0cd8c5f15459cbcd24e3273ff1ed95dec Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Wed, 29 May 2024 10:55:55 +0200 Subject: [PATCH 7/9] Add a `GrpcStatusError` subclass for unrecognized status codes When we receive a gRPC status code that we don't recognize, we raise a `GrpcStatusError` instead of the base class for all gRPC status errors. This will allow users to more easily differentiate between known and unknown status codes. Signed-off-by: Leandro Lucarella --- src/frequenz/client/microgrid/__init__.py | 2 ++ src/frequenz/client/microgrid/_exception.py | 28 +++++++++++++++++---- tests/test_exception.py | 18 ++++++------- 3 files changed, 33 insertions(+), 15 deletions(-) diff --git a/src/frequenz/client/microgrid/__init__.py b/src/frequenz/client/microgrid/__init__.py index c65a12b..596a9e9 100644 --- a/src/frequenz/client/microgrid/__init__.py +++ b/src/frequenz/client/microgrid/__init__.py @@ -46,6 +46,7 @@ ResourceExhausted, ServiceUnavailable, UnknownError, + UnrecognizedGrpcStatus, ) from ._metadata import Location, Metadata @@ -87,4 +88,5 @@ "ResourceExhausted", "ServiceUnavailable", "UnknownError", + "UnrecognizedGrpcStatus", ] diff --git a/src/frequenz/client/microgrid/_exception.py b/src/frequenz/client/microgrid/_exception.py index c23d184..13f0fdb 100644 --- a/src/frequenz/client/microgrid/_exception.py +++ b/src/frequenz/client/microgrid/_exception.py @@ -58,7 +58,6 @@ def from_grpc_error( server_url: str, operation: str, grpc_error: grpclib.GRPCError, - retryable: bool = True, ) -> GrpcStatusError: """Create an instance of the appropriate subclass from a gRPC error. @@ -66,7 +65,6 @@ def from_grpc_error( server_url: The URL of the server that returned the error. operation: The operation that caused the error. grpc_error: The gRPC error to convert. - retryable: Whether retrying the operation might succeed. Returns: An instance of @@ -104,12 +102,10 @@ def __call__( return ctor( server_url=server_url, operation=operation, grpc_error=grpc_error ) - return GrpcStatusError( + return UnrecognizedGrpcStatus( server_url=server_url, operation=operation, - description="Got an unrecognized status code", grpc_error=grpc_error, - retryable=retryable, ) @@ -156,6 +152,28 @@ def __init__( # pylint: disable=too-many-arguments """The original gRPC error.""" +class UnrecognizedGrpcStatus(GrpcStatusError): + """The gRPC server returned an unrecognized status code.""" + + def __init__( + self, *, server_url: str, operation: str, grpc_error: grpclib.GRPCError + ) -> None: + """Create a new instance. + + Args: + server_url: The URL of the server that returned the error. + operation: The operation that caused the error. + grpc_error: The gRPC error originating this exception. + """ + super().__init__( + server_url=server_url, + operation=operation, + description="Got an unrecognized status code", + grpc_error=grpc_error, + retryable=True, # We don't know so we assume it's retryable + ) + + class OperationCancelled(GrpcStatusError): """The operation was cancelled.""" diff --git a/tests/test_exception.py b/tests/test_exception.py index 3391c18..eaf960a 100644 --- a/tests/test_exception.py +++ b/tests/test_exception.py @@ -29,6 +29,7 @@ ResourceExhausted, ServiceUnavailable, UnknownError, + UnrecognizedGrpcStatus, ) @@ -41,6 +42,12 @@ def __call__( ERROR_TUPLES: list[tuple[type[GrpcStatusError], grpclib.Status, str, bool]] = [ + ( + UnrecognizedGrpcStatus, + mock.MagicMock(name="unknown_status"), + "Got an unrecognized status code", + True, + ), ( OperationCancelled, grpclib.Status.CANCELLED, @@ -200,16 +207,7 @@ def test_client_error() -> None: @pytest.mark.parametrize( - "exception_class, grpc_status, expected_description, retryable", - ERROR_TUPLES - + [ - ( - GrpcStatusError, - mock.MagicMock(name="unknown_status"), - "Got an unrecognized status code", - True, - ) - ], + "exception_class, grpc_status, expected_description, retryable", ERROR_TUPLES ) def test_from_grpc_error( exception_class: type[GrpcStatusError], From bcdb8c1a9b6b5c8eec68fdd5a8c133a2dce15199 Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Wed, 29 May 2024 11:50:42 +0200 Subject: [PATCH 8/9] Rename `GrpcStatusError` to `GrpcError` Also improve the docs for the class slightly to be more clear about which errors should be more protocol-independent and safer to catch. Signed-off-by: Leandro Lucarella --- src/frequenz/client/microgrid/__init__.py | 4 +- src/frequenz/client/microgrid/_client.py | 8 ++-- src/frequenz/client/microgrid/_exception.py | 49 +++++++++++---------- tests/test_exception.py | 16 +++---- 4 files changed, 39 insertions(+), 38 deletions(-) diff --git a/src/frequenz/client/microgrid/__init__.py b/src/frequenz/client/microgrid/__init__.py index 596a9e9..27e7656 100644 --- a/src/frequenz/client/microgrid/__init__.py +++ b/src/frequenz/client/microgrid/__init__.py @@ -32,7 +32,7 @@ DataLoss, EntityAlreadyExists, EntityNotFound, - GrpcStatusError, + GrpcError, InternalError, InvalidArgument, OperationAborted, @@ -69,7 +69,7 @@ "EntityNotFound", "Fuse", "GridMetadata", - "GrpcStatusError", + "GrpcError", "InternalError", "InvalidArgument", "InverterData", diff --git a/src/frequenz/client/microgrid/_client.py b/src/frequenz/client/microgrid/_client.py index c1397e4..c2b9796 100644 --- a/src/frequenz/client/microgrid/_client.py +++ b/src/frequenz/client/microgrid/_client.py @@ -92,7 +92,7 @@ async def components(self) -> Iterable[Component]: Raises: ClientError: If the are any errors communicating with the Microgrid API, most likely a subclass of - [GrpcStatusError][frequenz.client.microgrid.GrpcStatusError]. + [GrpcError][frequenz.client.microgrid.GrpcError]. """ try: component_list = await self.api.list_components( @@ -172,7 +172,7 @@ async def connections( Raises: ClientError: If the are any errors communicating with the Microgrid API, most likely a subclass of - [GrpcStatusError][frequenz.client.microgrid.GrpcStatusError]. + [GrpcError][frequenz.client.microgrid.GrpcError]. """ connection_filter = pb_microgrid.ConnectionFilter( starts=list(starts), ends=list(ends) @@ -391,7 +391,7 @@ async def set_power(self, component_id: int, power_w: float) -> None: Raises: ClientError: If the are any errors communicating with the Microgrid API, most likely a subclass of - [GrpcStatusError][frequenz.client.microgrid.GrpcStatusError]. + [GrpcError][frequenz.client.microgrid.GrpcError]. """ try: await self.api.set_power_active( @@ -425,7 +425,7 @@ async def set_bounds( greater than 0. ClientError: If the are any errors communicating with the Microgrid API, most likely a subclass of - [GrpcStatusError][frequenz.client.microgrid.GrpcStatusError]. + [GrpcError][frequenz.client.microgrid.GrpcError]. """ if upper < 0: raise ValueError(f"Upper bound {upper} must be greater than or equal to 0.") diff --git a/src/frequenz/client/microgrid/_exception.py b/src/frequenz/client/microgrid/_exception.py index 13f0fdb..c116266 100644 --- a/src/frequenz/client/microgrid/_exception.py +++ b/src/frequenz/client/microgrid/_exception.py @@ -58,7 +58,7 @@ def from_grpc_error( server_url: str, operation: str, grpc_error: grpclib.GRPCError, - ) -> GrpcStatusError: + ) -> GrpcError: """Create an instance of the appropriate subclass from a gRPC error. Args: @@ -68,16 +68,16 @@ def from_grpc_error( Returns: An instance of - [GrpcStatusError][frequenz.client.microgrid.GrpcStatusError] if + [GrpcError][frequenz.client.microgrid.GrpcError] if the gRPC status is not recognized, or an appropriate subclass if it is. """ class Ctor(Protocol): - """A protocol for the constructor of a subclass of `GrpcStatusError`.""" + """A protocol for the constructor of a subclass of `GrpcError`.""" def __call__( self, *, server_url: str, operation: str, grpc_error: grpclib.GRPCError - ) -> GrpcStatusError: ... + ) -> GrpcError: ... status_map: dict[grpclib.Status, Ctor] = { grpclib.Status.CANCELLED: OperationCancelled, @@ -109,11 +109,12 @@ def __call__( ) -class GrpcStatusError(ClientError): - """The gRPC server returned an error status code. +class GrpcError(ClientError): + """The gRPC server returned an error with a status code. These errors are specific to gRPC. If you want to use the client in - a protocol-independent way, you should avoid catching this exception. + a protocol-independent way, you should avoid catching this exception. Catching + subclasses that don't have *grpc* in their name should be protocol-independent. References: * [gRPC status @@ -152,7 +153,7 @@ def __init__( # pylint: disable=too-many-arguments """The original gRPC error.""" -class UnrecognizedGrpcStatus(GrpcStatusError): +class UnrecognizedGrpcStatus(GrpcError): """The gRPC server returned an unrecognized status code.""" def __init__( @@ -174,7 +175,7 @@ def __init__( ) -class OperationCancelled(GrpcStatusError): +class OperationCancelled(GrpcError): """The operation was cancelled.""" def __init__( @@ -196,7 +197,7 @@ def __init__( ) -class UnknownError(GrpcStatusError): +class UnknownError(GrpcError): """There was an error that can't be described using other statuses.""" def __init__( @@ -218,7 +219,7 @@ def __init__( ) -class InvalidArgument(GrpcStatusError, ValueError): +class InvalidArgument(GrpcError, ValueError): """The client specified an invalid argument. Note that this error differs from @@ -246,7 +247,7 @@ def __init__( ) -class OperationTimedOut(GrpcStatusError): +class OperationTimedOut(GrpcError): """The time limit was exceeded while waiting for the operationt o complete. For operations that change the state of the system, this error may be returned even @@ -274,7 +275,7 @@ def __init__( ) -class EntityNotFound(GrpcStatusError): +class EntityNotFound(GrpcError): """The requested entity was not found. Note that this error differs from @@ -301,7 +302,7 @@ def __init__( ) -class EntityAlreadyExists(GrpcStatusError): +class EntityAlreadyExists(GrpcError): """The entity that we attempted to create already exists.""" def __init__( @@ -323,7 +324,7 @@ def __init__( ) -class PermissionDenied(GrpcStatusError): +class PermissionDenied(GrpcError): """The caller does not have permission to execute the specified operation. Note that when the operation is rejected due to other reasons, such as the resources @@ -354,7 +355,7 @@ def __init__( ) -class ResourceExhausted(GrpcStatusError): +class ResourceExhausted(GrpcError): """Some resource has been exhausted (for example per-user quota, disk space, etc.).""" def __init__( @@ -377,7 +378,7 @@ def __init__( ) -class OperationPreconditionFailed(GrpcStatusError): +class OperationPreconditionFailed(GrpcError): """The operation was rejected because the system is not in a required state. For example, the directory to be deleted is non-empty, an rmdir operation is applied @@ -405,7 +406,7 @@ def __init__( ) -class OperationAborted(GrpcStatusError): +class OperationAborted(GrpcError): """The operation was aborted. Typically due to a concurrency issue or transaction abort. @@ -430,7 +431,7 @@ def __init__( ) -class OperationOutOfRange(GrpcStatusError): +class OperationOutOfRange(GrpcError): """The operation was attempted past the valid range. Unlike [InvalidArgument][frequenz.client.microgrid.InvalidArgument], this @@ -461,7 +462,7 @@ def __init__( ) -class OperationNotImplemented(GrpcStatusError): +class OperationNotImplemented(GrpcError): """The operation is not implemented or not supported/enabled in this service.""" def __init__( @@ -484,7 +485,7 @@ def __init__( ) -class InternalError(GrpcStatusError): +class InternalError(GrpcError): """Some invariants expected by the underlying system have been broken. This error code is reserved for serious errors. @@ -510,7 +511,7 @@ def __init__( ) -class ServiceUnavailable(GrpcStatusError): +class ServiceUnavailable(GrpcError): """The service is currently unavailable. This is most likely a transient condition, which can be corrected by retrying with @@ -536,7 +537,7 @@ def __init__( ) -class DataLoss(GrpcStatusError): +class DataLoss(GrpcError): """Unrecoverable data loss or corruption.""" def __init__( @@ -558,7 +559,7 @@ def __init__( ) -class OperationUnauthenticated(GrpcStatusError): +class OperationUnauthenticated(GrpcError): """The request does not have valid authentication credentials for the operation.""" def __init__( diff --git a/tests/test_exception.py b/tests/test_exception.py index eaf960a..fbf9460 100644 --- a/tests/test_exception.py +++ b/tests/test_exception.py @@ -15,7 +15,7 @@ DataLoss, EntityAlreadyExists, EntityNotFound, - GrpcStatusError, + GrpcError, InternalError, InvalidArgument, OperationAborted, @@ -33,15 +33,15 @@ ) -class _GrpcStatusErrorCtor(Protocol): - """A protocol for the constructor of a subclass of `GrpcStatusError`.""" +class _GrpcErrorCtor(Protocol): + """A protocol for the constructor of a subclass of `GrpcErrorCtor`.""" def __call__( self, *, server_url: str, operation: str, grpc_error: grpclib.GRPCError - ) -> GrpcStatusError: ... + ) -> GrpcError: ... -ERROR_TUPLES: list[tuple[type[GrpcStatusError], grpclib.Status, str, bool]] = [ +ERROR_TUPLES: list[tuple[type[GrpcError], grpclib.Status, str, bool]] = [ ( UnrecognizedGrpcStatus, mock.MagicMock(name="unknown_status"), @@ -146,7 +146,7 @@ def __call__( "exception_class, grpc_status, expected_description, retryable", ERROR_TUPLES ) def test_grpc_status_error( - exception_class: _GrpcStatusErrorCtor, + exception_class: _GrpcErrorCtor, grpc_status: grpclib.Status, expected_description: str, retryable: bool, @@ -176,7 +176,7 @@ def test_grpc_unknown_status_error() -> None: "grpc error message", "grpc error details", ) - exception = GrpcStatusError( + exception = GrpcError( server_url="http://testserver", operation="test_operation", description=expected_description, @@ -210,7 +210,7 @@ def test_client_error() -> None: "exception_class, grpc_status, expected_description, retryable", ERROR_TUPLES ) def test_from_grpc_error( - exception_class: type[GrpcStatusError], + exception_class: type[GrpcError], grpc_status: grpclib.Status, expected_description: str, retryable: bool, From 642c62b10759209a028535684291905a68584371 Mon Sep 17 00:00:00 2001 From: Leandro Lucarella Date: Mon, 27 May 2024 15:09:06 +0200 Subject: [PATCH 9/9] Update release notes Signed-off-by: Leandro Lucarella --- RELEASE_NOTES.md | 28 ++++++++++++++++++++++++++-- 1 file changed, 26 insertions(+), 2 deletions(-) diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md index 0fc34b7..e696307 100644 --- a/RELEASE_NOTES.md +++ b/RELEASE_NOTES.md @@ -7,13 +7,37 @@ ## Upgrading - The client now uses a string URL to connect to the server, the `grpc_channel` and `target` arguments are now replaced by `server_url`. The current accepted format is `grpc://hostname[:][?ssl=]`, meaning that the `port` and `ssl` are optional and default to 9090 and `false` respectively. You will have to adapt the way you connect to the server in your code. + - The client is now using [`grpclib`](https://pypi.org/project/grpclib/) to connect to the server instead of [`grpcio`](https://pypi.org/project/grpcio/). You might need to adapt your code if you are using `grpcio` directly. -- The client now doesn't raise `grpc.aio.RpcError` exceptions anymore. Instead, it raises `ClientError` exceptions that have the `grpclib.GRPCError` as their `__cause__`. You might need to adapt your error handling code to catch `ClientError` exceptions instead of `grpc.aio.RpcError` exceptions. + +- The client now doesn't raise `grpc.aio.RpcError` exceptions anymore. Instead, it raises its own exceptions, one per gRPC error status code, all inheriting from `GrpcError`, which in turn inherits from `ClientError` (as any other exception raised by this library in the future). `GrpcError`s have the `grpclib.GRPCError` as their `__cause__`. You might need to adapt your error handling code to catch these specific exceptions instead of `grpc.aio.RpcError`. + + You can also access the underlying `grpclib.GRPCError` using the `grpc_error` attribute for `GrpStatusError` exceptions, but it is discouraged because it makes downstream projects dependant on `grpclib` too + - The client now uses protobuf/grpc bindings generated [betterproto](https://github.com/danielgtaylor/python-betterproto) ([frequenz-microgrid-betterproto](https://github.com/frequenz-floss/frequenz-microgrid-betterproto-python)) instead of [grpcio](https://pypi.org/project/grpcio/) ([frequenz-api-microgrid](https://github.com/frequenz-floss/frequenz-api-microgrid)). If you were using the bindings directly, you might need to do some minor adjustments to your code. ## New Features - +- The client now raises more specific exceptions based on the gRPC status code, so you can more easily handle different types of errors. + + For example: + + ```python + try: + connections = await client.connections() + except OperationTimedOut: + ... + ``` + + instead of: + + ```python + try: + connections = await client.connections() + except grpc.aio.RpcError as e: + if e.code() == grpc.StatusCode.DEADLINE_EXCEEDED: + ... + ``` ## Bug Fixes