Merge pull request #258 from crim-ca/fix-not-keyword-literal-input

fix invalid Not keyword resolution

Author: fmigneault

CHANGES.rst

@@ -24,6 +24,10 @@ Fixes:
 - Fix default execution mode specification in process job control options
   (fixes `#182 <https://github.com/opengeospatial/ogcapi-processes/pull/182>`_).
 - Fix old OGC-API WPS REST bindings link in landing page for the more recent `OGC-API Processes` specification.
+- Fix invalid deserialization of schemas using ``not`` keyword that would result in all fields returned instead of
+  limiting them to the expected fields from the schema definitions for ``LiteralInputType`` in process description.
+- Adjust ``InputType`` and ``OutputType`` schemas to use ``allOf`` instead of ``anyOf`` definition since all sub-schemas
+  that define them must be combined, with their respectively required or optional fields.
 
 `3.1.0 <https://github.com/crim-ca/weaver/tree/3.1.0>`_ (2021-04-23)
 ========================================================================

tests/functional/test_wps_package.py

@@ -48,8 +48,15 @@
 IANA_ZIP = IANA_NAMESPACE + ":" + CONTENT_TYPE_APP_ZIP  # noqa # pylint: disable=unused-variable
 
 KNOWN_PROCESS_DESCRIPTION_FIELDS = {
-    "id", "title", "abstract", "inputs", "outputs", "executeEndpoint", "keywords", "metadata", "visibility"
+    "id", "title", "abstract", "keywords", "metadata", "inputs", "outputs", "executeEndpoint", "visibility"
 }
+# intersection of fields in InputType and specific sub-schema LiteralInputType
+KNOWN_PROCESS_DESCRIPTION_INPUT_DATA_FIELDS = {
+    "id", "title", "abstract", "keywords", "metadata", "links", "literalDataDomains", "additionalParameters",
+    "minOccurs", "maxOccurs"
+}
+# corresponding schemas of input, but min/max occurs not expected
+KNOWN_PROCESS_DESCRIPTION_OUTPUT_DATA_FIELDS = KNOWN_PROCESS_DESCRIPTION_INPUT_DATA_FIELDS - {"minOccurs", "maxOccurs"}
 
 LOGGER = logging.getLogger(__name__)
 
@@ -134,6 +141,11 @@ def test_literal_io_from_package(self):
         assert "maxOccurs" not in desc["process"]["outputs"][0]
         assert "format" not in desc["process"]["outputs"][0]
         assert len(set(desc["process"].keys()) - KNOWN_PROCESS_DESCRIPTION_FIELDS) == 0
+        # make sure that deserialization of literal fields did not produce over-verbose metadata
+        for p_input in desc["process"]["inputs"]:
+            assert len(set(p_input) - KNOWN_PROCESS_DESCRIPTION_INPUT_DATA_FIELDS) == 0
+        for p_output in desc["process"]["outputs"]:
+            assert len(set(p_output) - KNOWN_PROCESS_DESCRIPTION_OUTPUT_DATA_FIELDS) == 0
 
     def test_literal_io_from_package_and_offering(self):
         """

tests/wps_restapi/test_colander_extras.py

@@ -105,6 +105,49 @@ class ObjOneOf(ce.OneOfKeywordSchema):
                                  .format(ce._get_node_name(test_schema), test_value, result))
 
 
+def test_not_keyword_extra_fields_handling():
+    """
+    Using ``not`` keyword without any other schemas must return an empty mapping with additional fields dropped.
+    When providing other schemas, only fields in those inherited definitions should remain.
+    In should raise when matching the ``not`` conditions regardless.
+    """
+
+    class RequiredItem(ce.ExtendedMappingSchema):
+        item = ce.ExtendedSchemaNode(colander.String())
+
+    class MappingWithType(ce.ExtendedMappingSchema):
+        type = ce.ExtendedSchemaNode(colander.String())
+
+    class MappingWithoutType(ce.NotKeywordSchema, RequiredItem):
+        _not = [MappingWithType()]
+
+    class MappingOnlyNotType(ce.NotKeywordSchema):
+        _not = [MappingWithType()]
+
+    value = {"type": "invalid", "item": "valid"}
+    try:
+        result = MappingWithoutType().deserialize(value)
+    except colander.Invalid:
+        pass
+    except Exception:
+        raise AssertionError("Incorrect exception raised from deserialize of '{!s}' with {!s}"
+                             .format(ce._get_node_name(MappingWithoutType), value))
+    else:
+        raise AssertionError("Should have raised invalid schema from deserialize of '{!s}' with {!s}, but got {!s}"
+                             .format(ce._get_node_name(MappingWithoutType), value, result))
+
+    test_cases = [
+        (MappingWithoutType, {"item": "valid", "value": "ignore"}, {"item": "valid"}),
+        (MappingOnlyNotType, {"item": "valid", "value": "ignore"}, {})
+    ]
+    for test_schema, test_value, test_expect in test_cases:
+        try:
+            result = test_schema().deserialize(test_value)
+            assert result == test_expect, "Bad result from [{}] with [{}]".format(test_schema.__name__, test_value)
+        except colander.Invalid:
+            pytest.fail("Expected valid format from [{}] with [{}]".format(test_schema.__name__, test_value))
+
+
 class FieldTestString(ce.ExtendedSchemaNode):
     schema_type = colander.String
 

weaver/wps_restapi/colander_extras.py

@@ -1422,7 +1422,9 @@ class NotKeywordSchema(KeywordMapper):
     """
     Allows specifying specific schema conditions that fails underlying schema definition validation if present.
 
-    This is equivalent to OpenAPI object mapping with ``additionalProperties: false``, but is more explicit in
+    Corresponds to the ``not`` specifier of `OpenAPI` specification.
+
+    This is equivalent to `OpenAPI` object mapping with ``additionalProperties: false``, but is more explicit in
     the definition of invalid or conflicting field names with explicit definitions during deserialization.
 
     Example::
@@ -1436,9 +1438,21 @@ class MappingWithType(ExtendedMappingSchema):
         class MappingWithoutType(NotKeywordSchema, RequiredItem):
             _not = [MappingWithType()]
 
+        class MappingOnlyNotType(NotKeywordSchema):
+            _not = [MappingWithType()]
+
         # following will raise invalid error even if 'item' is valid because 'type' is also present
         MappingWithoutType().deserialize({"type": "invalid", "item": "valid"})
 
+        # following will return successfully with only 'item' because 'type' was not present
+        MappingWithoutType().deserialize({"item": "valid", "value": "ignore"})
+        # result: {"item": "valid"}
+
+        # following will return an empty mapping dropping 'item' since it only needs to ensure 'type' was not present,
+        # but did not provide any additional fields requirement from other class inheritances
+        MappingOnlyNotType().deserialize({"item": "valid"})
+        # result: {}
+
     .. seealso::
         - :class:`OneOfKeywordSchema`
         - :class:`AllOfKeywordSchema`
@@ -1457,7 +1471,7 @@ def _not(cls):
 
     def _deserialize_keyword(self, cstruct):
         """
-        Test each possible case, raise if any corresponding schema was successfully validated.
+        Raise if any sub-node schema that should NOT be present was successfully validated.
         """
         invalid_not = dict()
         for schema_class in self._not:  # noqa
@@ -1473,7 +1487,10 @@ def _deserialize_keyword(self, cstruct):
         if invalid_not:
             message = "Value contains not allowed fields from schema conditions: {}".format(invalid_not)
             raise colander.Invalid(node=self, msg=message, value=cstruct)
-        return cstruct
+        # If schema was a plain NotKeywordSchema, the result will be empty as it serves only to validate
+        # that the subnodes are not present. Otherwise, if it derives from other mapping classes, apply them.
+        # If deserialization was not applied here, everything in the original cstruct would bubble up.
+        return ExtendedMappingSchema.deserialize(self, cstruct)
 
 
 class KeywordTypeConverter(TypeConverter):

weaver/wps_restapi/swagger_definitions.py

@@ -768,8 +768,8 @@ class InputTypeDefinition(OneOfKeywordSchema):
     ]
 
 
-class InputType(AnyOfKeywordSchema):
-    _any_of = [
+class InputType(AllOfKeywordSchema):
+    _all_of = [
         InputDescriptionType(),
         InputTypeDefinition(),
         WithMinMaxOccurs(),
@@ -803,8 +803,8 @@ class OutputTypeDefinition(OneOfKeywordSchema):
     ]
 
 
-class OutputType(AnyOfKeywordSchema):
-    _any_of = [
+class OutputType(AllOfKeywordSchema):
+    _all_of = [
         OutputTypeDefinition(),
         OutputDescriptionType(),
     ]