Fixing Audit issues LOW-6 to LOW-10

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):

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)

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