From b6ee4ce69b57ed625237e1bd405f156db0227205 Mon Sep 17 00:00:00 2001 From: Erik Jaegervall Date: Tue, 16 Jan 2024 14:36:37 +0100 Subject: [PATCH] Fix LOW-6 to LOW-10 --- kuksa-client/kuksa_client/grpc/__init__.py | 83 +++++++++---- kuksa-client/tests/test_basevssclient.py | 129 +++++++++++++++++++++ kuksa-client/tests/test_datapoint.py | 47 +++++++- kuksa-client/tests/test_grpc.py | 92 ++++++++++++--- submodules/kuksa.val | 2 +- 5 files changed, 314 insertions(+), 39 deletions(-) create mode 100644 kuksa-client/tests/test_basevssclient.py diff --git a/kuksa-client/kuksa_client/grpc/__init__.py b/kuksa-client/kuksa_client/grpc/__init__.py index 7c8c887..7799592 100644 --- a/kuksa-client/kuksa_client/grpc/__init__.py +++ b/kuksa-client/kuksa_client/grpc/__init__.py @@ -20,7 +20,6 @@ import dataclasses import datetime import enum -import http import logging import re from typing import Any @@ -164,20 +163,29 @@ def from_message(cls, message: types_pb2.Metadata): if message.HasField(field): setattr(metadata, field, getattr(message, field)) if message.HasField('value_restriction'): - value_restriction = getattr( - message.value_restriction, message.value_restriction.WhichOneof('type')) - metadata.value_restriction = ValueRestriction() - for field in ('min', 'max'): - if value_restriction.HasField(field): - setattr(metadata.value_restriction, field, - getattr(value_restriction, field)) - if value_restriction.allowed_values: - metadata.value_restriction.allowed_values = list( - value_restriction.allowed_values) + restriction_type = message.value_restriction.WhichOneof('type') + # Make sure that a type actually is set + if restriction_type: + value_restriction = getattr( + message.value_restriction, restriction_type) + metadata.value_restriction = ValueRestriction() + # All types except string support min/max + if restriction_type != 'string': + for field in ('min', 'max'): + if value_restriction.HasField(field): + setattr(metadata.value_restriction, field, + getattr(value_restriction, field)) + if value_restriction.allowed_values: + metadata.value_restriction.allowed_values = list( + value_restriction.allowed_values) return metadata # pylint: disable=too-many-branches def to_message(self, value_type: DataType = DataType.UNSPECIFIED) -> types_pb2.Metadata: + """ + to_message/from_message aligned to use None rather than empty list for + representing allowed values in value restrictions + """ message = types_pb2.Metadata( data_type=self.data_type.value, entry_type=self.entry_type.value) for field in ('description', 'comment', 'deprecation', 'unit'): @@ -201,7 +209,7 @@ def to_message(self, value_type: DataType = DataType.UNSPECIFIED) -> types_pb2.M if self.value_restriction.max is not None: message.value_restriction.signed.max = int( self.value_restriction.max) - if self.value_restriction.allowed_values is not None: + if self.value_restriction.allowed_values: message.value_restriction.signed.allowed_values.extend( (int(value) for value in self.value_restriction.allowed_values), @@ -222,7 +230,7 @@ def to_message(self, value_type: DataType = DataType.UNSPECIFIED) -> types_pb2.M if self.value_restriction.max is not None: message.value_restriction.unsigned.max = int( self.value_restriction.max) - if self.value_restriction.allowed_values is not None: + if self.value_restriction.allowed_values: message.value_restriction.unsigned.allowed_values.extend( (int(value) for value in self.value_restriction.allowed_values), @@ -239,7 +247,7 @@ def to_message(self, value_type: DataType = DataType.UNSPECIFIED) -> types_pb2.M if self.value_restriction.max is not None: message.value_restriction.floating_point.max = float( self.value_restriction.max) - if self.value_restriction.allowed_values is not None: + if self.value_restriction.allowed_values: message.value_restriction.floating_point.allowed_values.extend( (float(value) for value in self.value_restriction.allowed_values), @@ -248,7 +256,7 @@ def to_message(self, value_type: DataType = DataType.UNSPECIFIED) -> types_pb2.M DataType.STRING, DataType.STRING_ARRAY, ): - if self.value_restriction.allowed_values is not None: + if self.value_restriction.allowed_values: message.value_restriction.string.allowed_values.extend( (str(value) for value in self.value_restriction.allowed_values), @@ -308,11 +316,32 @@ class Datapoint: @classmethod def from_message(cls, message: types_pb2.Datapoint): + """ + Return internal Datapoint representation or None on error + """ + if message.WhichOneof('value') is None: + logger.warning("No value provided in datapoint!") + return None + + if message.HasField('timestamp'): + # gRPC timestamp supports date up to including year 9999 + # If timestamp by any reason contains a larger number for seconds than supported + # you may get an overflow error + try: + timestamp = message.timestamp.ToDatetime( + tzinfo=datetime.timezone.utc, + ) + except OverflowError: + + logger.error("Timestamp %d out of accepted range, value ignored!", + message.timestamp.seconds) + return None + else: + timestamp = None + return cls( value=getattr(message, message.WhichOneof('value')), - timestamp=message.timestamp.ToDatetime( - tzinfo=datetime.timezone.utc, - ) if message.HasField('timestamp') else None, + timestamp=timestamp, ) def cast_array_values(cast, array): @@ -635,9 +664,21 @@ def _raise_if_invalid(self, response): err, preserving_proto_field_name=True) for err in response.errors] else: errors = [] - if (error and error['code'] is not http.HTTPStatus.OK) or any( - sub_error['error']['code'] is not http.HTTPStatus.OK for sub_error in errors - ): + + raise_error = False + if (error and error.get('code') != 200): + raise_error = True + else: + for sub_error in errors: + if 'error' in sub_error: + if sub_error['error'].get('code') != 200: + logger.debug("Sub-error %d but no top level error", sub_error['error'].get('code')) + raise_error = True + else: + logger.error("No error field for sub-error") + raise_error = True + + if raise_error: raise VSSClientError(error, errors) def get_authorization_header(self, token: str): diff --git a/kuksa-client/tests/test_basevssclient.py b/kuksa-client/tests/test_basevssclient.py new file mode 100644 index 0000000..66ec28f --- /dev/null +++ b/kuksa-client/tests/test_basevssclient.py @@ -0,0 +1,129 @@ +# /******************************************************************************** +# * Copyright (c) 2024 Contributors to the Eclipse Foundation +# * +# * See the NOTICE file(s) distributed with this work for additional +# * information regarding copyright ownership. +# * +# * This program and the accompanying materials are made available under the +# * terms of the Apache License 2.0 which is available at +# * http://www.apache.org/licenses/LICENSE-2.0 +# * +# * SPDX-License-Identifier: Apache-2.0 +# ********************************************************************************/ + +import pytest +import http +from kuksa_client.grpc import BaseVSSClient +from kuksa_client.grpc import VSSClientError +from kuksa.val.v1 import val_pb2 +from kuksa.val.v1 import types_pb2 + + +def test_response_no_error(): + """ + + """ + error = types_pb2.Error( + code=http.HTTPStatus.OK, reason='not_found', message="Does.Not.Exist not found") + errors = (types_pb2.DataEntryError( + path='Does.Not.Exist', error=error),) + resp = val_pb2.GetResponse( + error=error, errors=errors) + + base_vss_client = BaseVSSClient("hostname", 1234) + + # No exception expected on next line + base_vss_client._raise_if_invalid(resp) + + +def test_response_error_404(): + """ + + """ + error = types_pb2.Error( + code=404, reason='not_found', message="Does.Not.Exist not found") + errors = (types_pb2.DataEntryError( + path='Does.Not.Exist', error=error),) + resp = val_pb2.GetResponse( + error=error, errors=errors) + + base_vss_client = BaseVSSClient("hostname", 1234) + + with pytest.raises(VSSClientError): + base_vss_client._raise_if_invalid(resp) + + +def test_response_no_code(): + """ + To make sure that a proper is error is generated when code is missing in response + """ + error = types_pb2.Error( + reason='not_found', message="Does.Not.Exist not found") + errors = (types_pb2.DataEntryError( + path='Does.Not.Exist', error=error),) + resp = val_pb2.GetResponse( + error=error, errors=errors) + + base_vss_client = BaseVSSClient("hostname", 1234) + + with pytest.raises(VSSClientError): + base_vss_client._raise_if_invalid(resp) + + +def test_response_error_in_errors(): + """ + Logic for now is that we cannot always expect that "error" gives the aggregated state. + A command might be OK even if individual calls failed + """ + + no_error = types_pb2.Error( + code=http.HTTPStatus.OK, reason='', message="") + error = types_pb2.Error( + code=404, reason='not_found', message="Does.Not.Exist not found") + errors = (types_pb2.DataEntryError( + path='Does.Not.Exist', error=error),) + resp = val_pb2.GetResponse( + error=no_error, errors=errors) + + base_vss_client = BaseVSSClient("hostname", 1234) + + with pytest.raises(VSSClientError): + base_vss_client._raise_if_invalid(resp) + + +def test_response_no_code_in_error_in_errors(): + """ + To make sure that a proper is error is generated when code is missing in response + """ + + no_error = types_pb2.Error( + code=http.HTTPStatus.OK, reason='', message="") + error = types_pb2.Error( + reason='not_found', message="Does.Not.Exist not found") + errors = (types_pb2.DataEntryError( + path='Does.Not.Exist', error=error),) + resp = val_pb2.GetResponse( + error=no_error, errors=errors) + + base_vss_client = BaseVSSClient("hostname", 1234) + + with pytest.raises(VSSClientError): + base_vss_client._raise_if_invalid(resp) + + +def test_response_no_error_in_errors(): + """ + To make sure that a proper is error is generated when code is missing in response + """ + + no_error = types_pb2.Error( + code=http.HTTPStatus.OK, reason='', message="") + errors = (types_pb2.DataEntryError( + path='Does.Not.Exist'),) # Note no error given + resp = val_pb2.GetResponse( + error=no_error, errors=errors) + + base_vss_client = BaseVSSClient("hostname", 1234) + + with pytest.raises(VSSClientError): + base_vss_client._raise_if_invalid(resp) diff --git a/kuksa-client/tests/test_datapoint.py b/kuksa-client/tests/test_datapoint.py index 77a7eac..128fc30 100644 --- a/kuksa-client/tests/test_datapoint.py +++ b/kuksa-client/tests/test_datapoint.py @@ -13,6 +13,8 @@ import pytest from kuksa_client.grpc import Datapoint +from kuksa.val.v1 import types_pb2 +from google.protobuf import timestamp_pb2 # # Client rules: @@ -123,7 +125,7 @@ def test_quotes_in_string_values(): def test_quotes_in_string_values_2(): - """Doubee quotes in double quotes so in total three values""" + """Double quotes in double quotes so in total three values""" test_str = "['dtc1, dtc2', dtc3, \" dtc4, dtc4\"]" my_array = list(Datapoint.cast_array_values(Datapoint.cast_str, test_str)) assert len(my_array) == 3 @@ -179,3 +181,46 @@ def test_cast_bool(): assert Datapoint.cast_bool("Ja") is True assert Datapoint.cast_bool("Nein") is True assert Datapoint.cast_bool("Doch") is True + + +def test_from_message_none(): + """ + There shall always be a value + """ + msg = types_pb2.Datapoint() + datapoint = Datapoint.from_message(msg) + assert datapoint is None + + +def test_from_message_uint32(): + msg = types_pb2.Datapoint(uint32=456) + datapoint = Datapoint.from_message(msg) + assert datapoint.value == 456 + + +def test_from_message_time(): + """ + Make sure that we can handle values out of range (by discarding them) + gRPC supports year up to including 9999, so any date in year 10000 or later shall + result in that None is returned + """ + # Wed Jan 17 2024 10:02:27 GMT+0000 + timestamp = timestamp_pb2.Timestamp(seconds=1705485747) + msg = types_pb2.Datapoint(uint32=456, timestamp=timestamp) + datapoint = Datapoint.from_message(msg) + assert datapoint.timestamp.year == 2024 + + # Thu Dec 30 9999 07:13:22 GMT+0000 + timestamp = timestamp_pb2.Timestamp(seconds=253402154002) + msg = types_pb2.Datapoint(uint32=456, timestamp=timestamp) + datapoint = Datapoint.from_message(msg) + assert datapoint.timestamp.year == 9999 + + # Sat Jan 29 10000 07:13:22 GMT+0000 + # Currently the constructors does not check range + timestamp = timestamp_pb2.Timestamp(seconds=253404746002) + msg = types_pb2.Datapoint(uint32=456, timestamp=timestamp) + + # But the from_message method handle the checks + datapoint = Datapoint.from_message(msg) + assert datapoint is None diff --git a/kuksa-client/tests/test_grpc.py b/kuksa-client/tests/test_grpc.py index 91d06f5..7648aef 100644 --- a/kuksa-client/tests/test_grpc.py +++ b/kuksa-client/tests/test_grpc.py @@ -119,17 +119,29 @@ def test_to_message_value_restriction_without_value_type(self): (None, None, [-42, 0, 42]), (-42, 42, [-42, 0, 42]), ]) - def test_to_message_signed_value_restriction(self, value_type, min_value, max_value, allowed_values): + def test_to_from_message_signed_value_restriction(self, value_type, min_value, max_value, allowed_values): + + input_metadata = Metadata(value_restriction=ValueRestriction( + min=min_value, max=max_value, allowed_values=allowed_values, + )) + + if not allowed_values: + # Empty array treated as None by KUKSA Metadata class + allowed_values = None + if (min_value, max_value, allowed_values) == (None, None, None): expected_message = types_pb2.Metadata() + output_metadata = Metadata() else: expected_message = types_pb2.Metadata(value_restriction=types_pb2.ValueRestriction( signed=types_pb2.ValueRestrictionInt( min=min_value, max=max_value, allowed_values=allowed_values), )) - assert Metadata(value_restriction=ValueRestriction( - min=min_value, max=max_value, allowed_values=allowed_values, - )).to_message(value_type) == expected_message + output_metadata = Metadata(value_restriction=ValueRestriction( + min=min_value, max=max_value, allowed_values=allowed_values, + )) + assert input_metadata.to_message(value_type) == expected_message + assert Metadata.from_message(expected_message) == output_metadata @pytest.mark.parametrize('value_type', ( DataType.UINT8, @@ -150,17 +162,30 @@ def test_to_message_signed_value_restriction(self, value_type, min_value, max_va (None, None, [0, 12, 42]), (0, 42, [0, 12, 42]), ]) - def test_to_message_unsigned_value_restriction(self, value_type, min_value, max_value, allowed_values): + def test_to_from_message_unsigned_value_restriction(self, value_type, min_value, max_value, allowed_values): + + input_metadata = Metadata(value_restriction=ValueRestriction( + min=min_value, max=max_value, allowed_values=allowed_values, + )) + + if not allowed_values: + # Empty array treated as None by KUKSA Metadata class + allowed_values = None + if (min_value, max_value, allowed_values) == (None, None, None): expected_message = types_pb2.Metadata() + output_metadata = Metadata() else: expected_message = types_pb2.Metadata(value_restriction=types_pb2.ValueRestriction( unsigned=types_pb2.ValueRestrictionUint( min=min_value, max=max_value, allowed_values=allowed_values), )) - assert Metadata(value_restriction=ValueRestriction( - min=min_value, max=max_value, allowed_values=allowed_values, - )).to_message(value_type) == expected_message + output_metadata = Metadata(value_restriction=ValueRestriction( + min=min_value, max=max_value, allowed_values=allowed_values, + )) + + assert input_metadata.to_message(value_type) == expected_message + assert Metadata.from_message(expected_message) == output_metadata @pytest.mark.parametrize('value_type', ( DataType.FLOAT, @@ -178,31 +203,66 @@ def test_to_message_unsigned_value_restriction(self, value_type, min_value, max_ (None, None, [0.5, 12., 41.5]), (0.5, 41.5, [0.5, 12., 41.5]), ]) - def test_to_message_float_value_restriction(self, value_type, min_value, max_value, allowed_values): + def test_to_from_message_float_value_restriction(self, value_type, min_value, max_value, allowed_values): + + input_metadata = Metadata(value_restriction=ValueRestriction( + min=min_value, max=max_value, allowed_values=allowed_values, + )) + + if not allowed_values: + # Empty array treated as None by KUKSA Metadata class + allowed_values = None + if (min_value, max_value, allowed_values) == (None, None, None): expected_message = types_pb2.Metadata() + output_metadata = Metadata() else: expected_message = types_pb2.Metadata(value_restriction=types_pb2.ValueRestriction( floating_point=types_pb2.ValueRestrictionFloat( min=min_value, max=max_value, allowed_values=allowed_values), )) - assert Metadata(value_restriction=ValueRestriction( - min=min_value, max=max_value, allowed_values=allowed_values, - )).to_message(value_type) == expected_message + output_metadata = Metadata(value_restriction=ValueRestriction( + min=min_value, max=max_value, allowed_values=allowed_values, + )) + + assert input_metadata.to_message(value_type) == expected_message + assert Metadata.from_message(expected_message) == output_metadata @pytest.mark.parametrize('value_type', (DataType.STRING, DataType.STRING_ARRAY)) @pytest.mark.parametrize('allowed_values', [None, [], ['Hello', 'world']]) - def test_to_message_string_value_restriction(self, value_type, allowed_values): + def test_to_from_message_string_value_restriction(self, value_type, allowed_values): + + input_metadata = Metadata(value_restriction=ValueRestriction( + allowed_values=allowed_values, + )) + + if not allowed_values: + # Empty array treated as None by KUKSA Metadata class + allowed_values = None + if allowed_values is None: expected_message = types_pb2.Metadata() + output_metadata = Metadata() else: expected_message = types_pb2.Metadata(value_restriction=types_pb2.ValueRestriction( string=types_pb2.ValueRestrictionString( allowed_values=allowed_values), )) - assert Metadata(value_restriction=ValueRestriction( - allowed_values=allowed_values, - )).to_message(value_type) == expected_message + output_metadata = Metadata(value_restriction=ValueRestriction( + allowed_values=allowed_values, + )) + + assert input_metadata.to_message(value_type) == expected_message + assert Metadata.from_message(expected_message) == output_metadata + + def test_metadata_from_message_value_restriction_no_type(self): + """ + This intends to cover the case when the proto message has a value restriction, but + no contents (type not specified) + """ + input_message = types_pb2.Metadata(value_restriction=types_pb2.ValueRestriction()) + expected_metadata = Metadata() + assert Metadata.from_message(input_message) == expected_metadata @pytest.mark.parametrize('metadata_dict, init_kwargs', [ ({}, {}), diff --git a/submodules/kuksa.val b/submodules/kuksa.val index 6690ca9..df6dcb0 160000 --- a/submodules/kuksa.val +++ b/submodules/kuksa.val @@ -1 +1 @@ -Subproject commit 6690ca970a2f7dd8245bd00f6b10788eaad755b5 +Subproject commit df6dcb0fafd651d5e9bec037194c352a822cd3f9