Merge pull request #268 from labthings/handle-nullables

Handle fields that allow `null` values properly.

This merge fixes a longstanding issue that was causing invalid thing descriptions to be created if any fields had `allow_none` set to `True`.

Author: rwb27

Diff for: src/labthings/apispec/plugins.py

@@ -112,11 +112,10 @@ def spec_for_interaction(cls, interaction):
                 )
         return d
 
-    @classmethod
-    def spec_for_property(cls, prop):
-        class_schema = ensure_schema(prop.schema) or {}
+    def spec_for_property(self, prop):
+        class_schema = ensure_schema(self.spec, prop.schema) or {}
 
-        d = cls.spec_for_interaction(prop)
+        d = self.spec_for_interaction(prop)
 
         # Add in writeproperty methods
         for method in ("put", "post"):
@@ -155,9 +154,11 @@ def spec_for_property(cls, prop):
         return d
 
     def spec_for_action(self, action):
-        action_input = ensure_schema(action.args, name=f"{action.__name__}InputSchema")
+        action_input = ensure_schema(
+            self.spec, action.args, name=f"{action.__name__}InputSchema"
+        )
         action_output = ensure_schema(
-            action.schema, name=f"{action.__name__}OutputSchema"
+            self.spec, action.schema, name=f"{action.__name__}OutputSchema"
         )
         # We combine input/output parameters with ActionSchema using an
         # allOf directive, so we don't end up duplicating the schema

Diff for: src/labthings/apispec/utilities.py

@@ -1,21 +1,27 @@
 from inspect import isclass
 from typing import Dict, Type, Union, cast
 
+from apispec import APISpec
 from apispec.ext.marshmallow import MarshmallowPlugin
-from apispec.ext.marshmallow.field_converter import FieldConverterMixin
 from marshmallow import Schema
 
 from .. import fields
 
 
-def field2property(field):
-    """Convert a marshmallow Field to OpenAPI dictionary"""
-    converter = FieldConverterMixin()
-    converter.init_attribute_functions()
-    return converter.field2property(field)
+def field2property(spec: APISpec, field: fields.Field):
+    """Convert a marshmallow Field to OpenAPI dictionary
+
+    We require an initialised APISpec object to use its
+    converter function - in particular, this will depend
+    on the OpenAPI version defined in `spec`.  We also rely
+    on the spec having a `MarshmallowPlugin` attached.
+    """
+    plugin = get_marshmallow_plugin(spec)
+    return plugin.converter.field2property(field)
 
 
 def ensure_schema(
+    spec: APISpec,
     schema: Union[
         fields.Field,
         Type[fields.Field],
@@ -34,19 +40,22 @@ def ensure_schema(
 
     Other Schemas are returned as Marshmallow Schema instances, which will be
     converted to references by the plugin.
+
+    The first argument must be an initialised APISpec object, as the conversion
+    of single fields to dictionaries is version-dependent.
     """
     if schema is None:
         return None
     if isinstance(schema, fields.Field):
-        return field2property(schema)
+        return field2property(spec, schema)
     elif isinstance(schema, dict):
         return Schema.from_dict(schema, name=name)()
     elif isinstance(schema, Schema):
         return schema
     if isclass(schema):
         schema = cast(Type, schema)
         if issubclass(schema, fields.Field):
-            return field2property(schema())
+            return field2property(spec, schema())
         elif issubclass(schema, Schema):
             return schema()
     raise TypeError(

Diff for: src/labthings/json/marshmallow_jsonschema/README

@@ -3,4 +3,22 @@ This submodule is a modified version of fuhrysteve/marshmallow-jsonschema, with
 This will convert any Marshmallow schema into a JSON schema, without any schema references or additional properties. 
 It will create a basic, inline schema only.
 
+It has also been modified (with the addition of `.base.convert_type_list_to_oneof`) to avoid returning list values for
+the type of a schema (this is valid JSON schema, but is not permitted in Thing Description syntax).  Instead, we expand
+the list, by creating a copy of the schema for each type, and combining them using `oneOf`.  This means that
+`fields.String(allow_none=True)`, which would previously be rendered as:
+```json
+{"type": ["string", "null"]}
+```
+will be dumped as
+```json
+{
+    "oneOf": [
+        {"type": "string"},
+        {"type": "null"}
+    ]
+}
+```
+This is also valid JSONSchema, though clearly less elegant.  However, it's required by the thing description.
+
 https://github.com/fuhrysteve/marshmallow-jsonschema

Diff for: src/labthings/json/marshmallow_jsonschema/base.py

@@ -1,6 +1,7 @@
 import datetime
 import decimal
 import uuid
+from copy import deepcopy
 from inspect import isclass
 
 from marshmallow import Schema, fields, missing, validate
@@ -68,6 +69,29 @@
 }
 
 
+def convert_type_list_to_oneof(schema):
+    """Avoid setting `type` to a list, using `oneOf`
+
+    JSONSchema allows datatypes to be specified as lists, which
+    means the type may be any of the specified values.  This is
+    not permitted by W3C Thing Description.  This function will
+    use the oneOf mechanism in JSONSchema to convert schemas
+    that use a list of types into a series of schemas each with
+    one type.  Those schemas are then nested together using
+    OneOf.
+
+    Schemas that do not have a type that is a list are returned
+    unmodified.
+    """
+    if "type" not in schema or type(schema["type"]) != list:
+        return schema
+    subschemas = []
+    for t in schema["type"]:
+        subschemas.append(deepcopy(schema))
+        subschemas[-1]["type"] = t
+    return {"oneOf": subschemas}
+
+
 class JSONSchema(Schema):
     """Converts to JSONSchema as defined by http://json-schema.org/."""
 
@@ -202,6 +226,7 @@ def _get_schema_for_field(self, obj, field):
                 )
                 if base_class is not None and base_class in FIELD_VALIDATORS:
                     schema = FIELD_VALIDATORS[base_class](schema, field, validator, obj)
+        schema = convert_type_list_to_oneof(schema)
         return schema
 
     def _from_nested_schema(self, _, field):

Diff for: tests/conftest.py

@@ -213,6 +213,17 @@ def post(self, args):
 
     thing.add_view(TestFieldProperty, "/TestFieldProperty")
 
+    class TestNullableFieldProperty(PropertyView):
+        schema = fields.Integer(allow_none=True)
+
+        def get(self):
+            return "one"
+
+        def post(self, args):
+            pass
+
+    thing.add_view(TestNullableFieldProperty, "/TestNullableFieldProperty")
+
     class FailAction(ActionView):
         wait_for = 0.1
 

Diff for: tests/test_json_marshmallow_jsonschema.py

@@ -289,18 +289,23 @@ class TestSchema(Schema):
 
 
 def test_allow_none():
-    """A field with allow_none set to True should have type null as additional."""
+    """A field with allow_none set to True should be able to be null.
+
+    Note that this has been modified from JSONSchema behaviour: in
+    JSONSchema, "type" should be set to `["string", "null"]`.  This
+    is not allowed in Thing Descriptions, so instead we need to use
+    `oneOf`.  Our modified JSON schema library does this."""
 
     class TestSchema(Schema):
         id = fields.Integer(required=True)
-        readonly_fld = fields.String(allow_none=True)
+        nullable_fld = fields.String(allow_none=True)
 
     schema = TestSchema()
 
     dumped = validate_and_dump(schema)
 
-    assert dumped["properties"]["readonly_fld"] == {
-        "type": ["string", "null"],
+    assert dumped["properties"]["nullable_fld"] == {
+        "oneOf": [{"type": "string"}, {"type": "null"}],
     }
 
 

Diff for: tests/test_openapi.py

@@ -54,43 +54,49 @@ def post(self):
     assert original_input_schema != modified_input_schema
 
 
-def test_ensure_schema_field_instance():
-    ret = utilities.ensure_schema(fields.Integer())
+def test_ensure_schema_field_instance(spec):
+    ret = utilities.ensure_schema(spec, fields.Integer())
     assert ret == {"type": "integer"}
 
 
-def test_ensure_schema_field_class():
-    ret = utilities.ensure_schema(fields.Integer)
+def test_ensure_schema_nullable_field_instance(spec):
+    ret = utilities.ensure_schema(spec, fields.Integer(allow_none=True))
+    assert ret == {"type": "integer", "nullable": True}
+
+
+def test_ensure_schema_field_class(spec):
+    ret = utilities.ensure_schema(spec, fields.Integer)
     assert ret == {"type": "integer"}
 
 
-def test_ensure_schema_class():
-    ret = utilities.ensure_schema(LogRecordSchema)
+def test_ensure_schema_class(spec):
+    ret = utilities.ensure_schema(spec, LogRecordSchema)
     assert isinstance(ret, Schema)
 
 
-def test_ensure_schema_instance():
-    ret = utilities.ensure_schema(LogRecordSchema())
+def test_ensure_schema_instance(spec):
+    ret = utilities.ensure_schema(spec, LogRecordSchema())
     assert isinstance(ret, Schema)
 
 
-def test_ensure_schema_dict():
+def test_ensure_schema_dict(spec):
     ret = utilities.ensure_schema(
+        spec,
         {
             "count": fields.Integer(),
             "name": fields.String(),
-        }
+        },
     )
     assert isinstance(ret, Schema)
 
 
-def test_ensure_schema_none():
-    assert utilities.ensure_schema(None) is None
+def test_ensure_schema_none(spec):
+    assert utilities.ensure_schema(spec, None) is None
 
 
-def test_ensure_schema_error():
+def test_ensure_schema_error(spec):
     with pytest.raises(TypeError):
-        utilities.ensure_schema(Exception)
+        utilities.ensure_schema(spec, Exception)
 
 
 def test_get_marshmallow_plugin(spec):