From 8440eb2826f14e579e0892ed3566f43bd86a888c Mon Sep 17 00:00:00 2001 From: Ilya Etingof Date: Wed, 22 Jan 2020 16:44:19 +0100 Subject: [PATCH] Relax required Redfish fields handling Redfish defines some of the fields in its JSON schemas as mandatory. However, some implementations ignore this requirement and occasionally omit some required fields in the Redfish document tree they produce. Failing the whole Redfish interaction basing solely on the absence of a required (bit non-essential) field makes sushy perfect, but not exactly practical. This patch changes to semantics of the `default` parameter in ``Field` constructor in a way that it can inhibit otherwise fatal failure when a select of required attributes are missing. Along this mis/feature, some non-essential fields in Redfish message registry have been made required and defaulted effectively making them non-required. Change-Id: I637f11ff9ceab398077eae19db83db396356c8dc Story: 2006641 Task: 38362 --- sushy/resources/base.py | 38 ++++++++++--------- sushy/resources/registry/message_registry.py | 5 ++- .../registry/test_message_registry.py | 12 +++++- 3 files changed, 35 insertions(+), 20 deletions(-) diff --git a/sushy/resources/base.py b/sushy/resources/base.py index 02a8c019..16af69e6 100644 --- a/sushy/resources/base.py +++ b/sushy/resources/base.py @@ -40,10 +40,9 @@ class Field(object): :param path: JSON field to fetch the value from. Either a string, or a list of strings in case of a nested field. - :param required: whether this field is required. Missing required - fields result in MissingAttributeError. + :param required: whether this field is required. Missing required, + but not defaulted, fields result in MissingAttributeError. :param default: the default value to use when the field is missing. - Only has effect when the field is not required. :param adapter: a function to call to transform and/or validate the received value. UnicodeError, ValueError or TypeError from this call are reraised as MalformedAttributeError. @@ -80,7 +79,8 @@ class Field(object): :param resource: ResourceBase instance for which the field is loaded. :param nested_in: parent resource path (for error reporting only), must be a list of strings or None. - :raises: MissingAttributeError if a required field is missing. + :raises: MissingAttributeError if a required field is missing + and not defaulted. :raises: MalformedAttributeError on invalid field value or type. :returns: loaded and verified value """ @@ -94,12 +94,18 @@ class Field(object): except KeyError: if self._required: path = (nested_in or []) + self._path - raise exceptions.MissingAttributeError( - attribute='/'.join(path), - resource=resource.path) - else: - # Do not run the adapter on the default value - return self._default + + if self._default is None: + raise exceptions.MissingAttributeError( + attribute='/'.join(path), + resource=resource.path) + + logging.warning( + 'Applying default "%s" on required, but missing ' + 'attribute "%s"' % (self._default, path)) + + # Do not run the adapter on the default value + return self._default # NOTE(etingof): this is just to account for schema violation if item is None: @@ -251,11 +257,10 @@ class MappedField(Field): a string or a list of string. In the latter case, the value will be fetched from a nested object. :param mapping: a mapping to take values from. - :param required: whether this field is required. Missing required - fields result in MissingAttributeError. + :param required: whether this field is required. Missing required, + but not defaulted, fields result in MissingAttributeError. :param default: the default value to use when the field is missing. - Only has effect when the field is not required. This value is not - matched against the mapping. + This value is not matched against the mapping. """ if not isinstance(mapping, collections.abc.Mapping): raise TypeError("The mapping argument must be a mapping") @@ -278,10 +283,9 @@ class MappedListField(Field): :param field: JSON field to fetch the list of values from. :param mapping: a mapping for the list elements. - :param required: whether this field is required. Missing required - fields result in MissingAttributeError. + :param required: whether this field is required. Missing required, + but not defaulted, fields result in MissingAttributeError. :param default: the default value to use when the field is missing. - Only has effect when the field is not required. """ if not isinstance(mapping, collections.abc.Mapping): raise TypeError("The mapping argument must be a mapping") diff --git a/sushy/resources/registry/message_registry.py b/sushy/resources/registry/message_registry.py index 2b32edef..fc900222 100644 --- a/sushy/resources/registry/message_registry.py +++ b/sushy/resources/registry/message_registry.py @@ -21,7 +21,7 @@ from sushy.resources import mappings as res_maps class MessageDictionaryField(base.DictionaryField): - description = base.Field('Description', required=False) + description = base.Field('Description', required=True, default='') """Indicates how and when the message is returned by the Redfish service""" message = base.Field('Message', required=True) @@ -47,7 +47,8 @@ class MessageDictionaryField(base.DictionaryField): severity = base.MappedField('Severity', res_maps.SEVERITY_VALUE_MAP, - default=res_cons.SEVERITY_CRITICAL) + required=True, + default=res_cons.SEVERITY_WARNING) """Mapped severity of the message""" diff --git a/sushy/tests/unit/resources/registry/test_message_registry.py b/sushy/tests/unit/resources/registry/test_message_registry.py index e94c486e..93e8671e 100644 --- a/sushy/tests/unit/resources/registry/test_message_registry.py +++ b/sushy/tests/unit/resources/registry/test_message_registry.py @@ -63,7 +63,7 @@ class MessageRegistryTestCase(base.TestCase): self.assertEqual('Panic', self.registry.messages['Failed'].resolution) self.assertEqual( 2, len(self.registry.messages['MissingThings'].param_types)) - self.assertEqual(res_cons.SEVERITY_CRITICAL, + self.assertEqual(res_cons.SEVERITY_WARNING, self.registry.messages['MissingThings'].severity) self.assertEqual( res_cons.PARAMTYPE_STRING, @@ -74,6 +74,16 @@ class MessageRegistryTestCase(base.TestCase): self.assertEqual( 'Try Later', self.registry.messages['MissingThings'].resolution) + def test__parse_attributes_missing_msg_desc(self): + self.json_doc['Messages']['Success'].pop('Description') + self.registry._parse_attributes(self.json_doc) + self.assertEqual('', self.registry.messages['Success'].description) + + def test__parse_attributes_missing_msg_severity(self): + self.json_doc['Messages']['Success'].pop('Severity') + self.registry._parse_attributes(self.json_doc) + self.assertEqual('warning', self.registry.messages['Success'].severity) + def test__parse_attribtues_unknown_param_type(self): self.registry.json['Messages']['Failed']['ParamTypes'] = \ ['unknown_type']