Make Option<T> fields optional in generated schemas (#16)

2020-02-29 19:37:20 +00:00 · 2020-02-29 19:37:20 +00:00 · 4ad5000232
commit 4ad5000232
parent 60284fdf93
14 changed files with 269 additions and 148 deletions
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@ -1,5 +1,9 @@
 # Changelog

+## Current changes (version TBC)
+### Fixed:
+- When deriving `JsonSchema` on structs, `Option<T>` struct fields are no longer included in the list of required properties in the schema (https://github.com/GREsau/schemars/issues/11)
+
 ## [0.7.0-alpha-1] - 2019-12-29
 ### Changed:
 - **BREAKING CHANGE** - `SchemaSettings` can no longer be created using struct initialization syntax. Instead, if you need to use custom schema settings, you can use a constructor function and either:
--- a/schemars/src/flatten.rs
+++ b/schemars/src/flatten.rs
@ -15,7 +15,7 @@ impl Schema {
    }
 }

-trait Merge: Sized {
+pub(crate) trait Merge: Sized {
    fn merge(self, other: Self) -> Self;
 }

--- a/schemars/src/gen.rs
+++ b/schemars/src/gen.rs
@ -1,3 +1,4 @@
+use crate::flatten::Merge;
 use crate::schema::*;
 use crate::{JsonSchema, Map};

@ -169,9 +170,9 @@ impl SchemaGenerator {
        &self.settings
    }

-    /// Returns a `SchemaObject` equivalent to the given `schema` which may have validation, metadata or other properties set on it.
+    /// Modifies the given `SchemaObject` so that it may have validation, metadata or other properties set on it.
    ///
-    /// If `schema` is not a `$ref` schema, then this returns `schema` unmodified. Otherwise, depending on this generator's settings,
+    /// If `schema` is not a `$ref` schema, then this does not modify `schema`. Otherwise, depending on this generator's settings,
    /// this may wrap the `$ref` in another schema. This is required because in many JSON Schema implementations, a schema with `$ref`
    /// set may not include other properties.
    ///
@ -184,24 +185,19 @@ impl SchemaGenerator {
    /// let ref_schema = SchemaObject::new_ref("foo".to_owned());
    /// assert!(ref_schema.is_ref());
    ///
-    /// let extensible_schema = gen.make_extensible(ref_schema.clone());
+    /// let mut extensible_schema = ref_schema.clone();
+    /// gen.make_extensible(&mut extensible_schema);
    /// assert_ne!(ref_schema, extensible_schema);
    /// assert!(!extensible_schema.is_ref());
    ///
-    /// let extensible_schema2 = gen.make_extensible(extensible_schema.clone());
+    /// let mut extensible_schema2 = extensible_schema.clone();
+    /// gen.make_extensible(&mut extensible_schema);
    /// assert_eq!(extensible_schema, extensible_schema2);
    /// ```
-    pub fn make_extensible(&self, schema: SchemaObject) -> SchemaObject {
+    pub fn make_extensible(&self, schema: &mut SchemaObject) {
        if schema.is_ref() && !self.settings().allow_ref_siblings {
-            SchemaObject {
-                subschemas: Some(Box::new(SubschemaValidation {
-                    all_of: Some(vec![schema.into()]),
-                    ..Default::default()
-                })),
-                ..Default::default()
-            }
-        } else {
-            schema
+            let original = std::mem::replace(schema, SchemaObject::default());
+            schema.subschemas().all_of = Some(vec![original.into()]);
        }
    }

@ -279,8 +275,8 @@ impl SchemaGenerator {
    /// add them to the `SchemaGenerator`'s schema definitions and include them in the returned `SchemaObject`'s
    /// [`definitions`](../schema/struct.Metadata.html#structfield.definitions)
    pub fn root_schema_for<T: ?Sized + JsonSchema>(&mut self) -> RootSchema {
-        let schema = T::json_schema(self);
-        let mut schema: SchemaObject = self.make_extensible(schema.into());
+        let mut schema = T::json_schema(self).into();
+        self.make_extensible(&mut schema);
        schema.metadata().title.get_or_insert_with(T::schema_name);
        RootSchema {
            meta_schema: self.settings.meta_schema.clone(),
@ -294,8 +290,8 @@ impl SchemaGenerator {
    /// If `T`'s schema depends on any [referenceable](JsonSchema::is_referenceable) schemas, then this method will
    /// include them in the returned `SchemaObject`'s [`definitions`](../schema/struct.Metadata.html#structfield.definitions)
    pub fn into_root_schema_for<T: ?Sized + JsonSchema>(mut self) -> RootSchema {
-        let schema = T::json_schema(&mut self);
-        let mut schema: SchemaObject = self.make_extensible(schema.into());
+        let mut schema = T::json_schema(&mut self).into();
+        self.make_extensible(&mut schema);
        schema.metadata().title.get_or_insert_with(T::schema_name);
        RootSchema {
            meta_schema: self.settings.meta_schema,
@ -346,4 +342,11 @@ impl SchemaGenerator {
            _ => None,
        }
    }
+
+    // TODO should this take a Schema instead of SchemaObject?
+    pub(crate) fn apply_metadata(&self, schema: &mut SchemaObject, metadata: Metadata) {
+        self.make_extensible(schema);
+        // TODO get rid of the clone
+        schema.metadata = Some(Box::new(metadata)).merge(schema.metadata.clone());
+    }
 }
--- a/schemars/src/json_schema_impls/core.rs
+++ b/schemars/src/json_schema_impls/core.rs
@ -35,7 +35,8 @@ impl<T: JsonSchema> JsonSchema for Option<T> {
            }
        }
        if gen.settings().option_nullable {
-            let mut schema_obj = gen.make_extensible(schema.into());
+            let mut schema_obj = schema.into();
+            gen.make_extensible(&mut schema_obj);
            schema_obj
                .extensions
                .insert("nullable".to_owned(), json!(true));
@ -44,8 +45,8 @@ impl<T: JsonSchema> JsonSchema for Option<T> {
        schema
    }

-    fn json_schema_optional(gen: &mut SchemaGenerator) -> Schema {
-        let mut schema = T::json_schema_optional(gen);
+    fn json_schema_for_flatten(gen: &mut SchemaGenerator) -> Schema {
+        let mut schema = T::json_schema_for_flatten(gen);
        if let Schema::Object(SchemaObject {
            object: Some(ref mut object_validation),
            ..
@ -55,6 +56,25 @@ impl<T: JsonSchema> JsonSchema for Option<T> {
        }
        schema
    }
+
+    fn add_schema_as_property(
+        gen: &mut SchemaGenerator,
+        parent: &mut SchemaObject,
+        name: String,
+        metadata: Option<Metadata>,
+        _required: bool,
+    ) {
+        let mut schema = gen.subschema_for::<Self>();
+
+        if let Some(metadata) = metadata {
+            let mut schema_obj = schema.into();
+            gen.apply_metadata(&mut schema_obj, metadata);
+            schema = Schema::Object(schema_obj);
+        }
+
+        let object = parent.object();
+        object.properties.insert(name, schema);
+    }
 }

 fn add_null_type(instance_type: &mut SingleOrVec<InstanceType>) {
--- a/schemars/src/json_schema_impls/mod.rs
+++ b/schemars/src/json_schema_impls/mod.rs
@ -21,8 +21,18 @@ macro_rules! forward_impl {
                <$target>::json_schema(gen)
            }

-            fn json_schema_optional(gen: &mut SchemaGenerator) -> Schema {
-                <$target>::json_schema_optional(gen)
+            fn json_schema_for_flatten(gen: &mut SchemaGenerator) -> Schema {
+                <$target>::json_schema_for_flatten(gen)
+            }
+
+            fn add_schema_as_property(
+                gen: &mut SchemaGenerator,
+                parent: &mut crate::schema::SchemaObject,
+                name: String,
+                metadata: Option<crate::schema::Metadata>,
+                required: bool,
+            ) {
+                <$target>::add_schema_as_property(gen, parent, name, metadata, required)
            }
        }
    };
--- a/schemars/src/lib.rs
+++ b/schemars/src/lib.rs
@ -236,7 +236,7 @@ pub use schemars_derive::*;
 #[doc(hidden)]
 pub use serde_json as _serde_json;

-use schema::Schema;
+use schema::{Schema, SchemaObject};

 /// A type which can be described as a JSON Schema document.
 ///
@ -281,11 +281,39 @@ pub trait JsonSchema {

    /// Helper for generating schemas for flattened `Option` fields.
    ///
-    /// This should not need to be called or implemented by code outside of `schemars`.
+    /// This should not need to be called or implemented by code outside of `schemars`,
+    /// and should not be considered part of the public API.
    #[doc(hidden)]
-    fn json_schema_optional(gen: &mut gen::SchemaGenerator) -> Schema {
+    fn json_schema_for_flatten(gen: &mut gen::SchemaGenerator) -> Schema {
        Self::json_schema(gen)
    }
+
+    /// Helper for generating schemas for `Option` fields.
+    ///
+    /// This should not need to be called or implemented by code outside of `schemars`,
+    /// and should not be considered part of the public API.
+    #[doc(hidden)]
+    fn add_schema_as_property(
+        gen: &mut gen::SchemaGenerator,
+        parent: &mut SchemaObject,
+        name: String,
+        metadata: Option<schema::Metadata>,
+        required: bool,
+    ) {
+        let mut schema = gen.subschema_for::<Self>();
+
+        if let Some(metadata) = metadata {
+            let mut schema_obj = schema.into();
+            gen.apply_metadata(&mut schema_obj, metadata);
+            schema = Schema::Object(schema_obj);
+        }
+
+        let object = parent.object();
+        if required {
+            object.required.insert(name.clone());
+        }
+        object.properties.insert(name, schema);
+    }
 }

 #[cfg(test)]
--- a/schemars/tests/expected/doc_comments_enum.json
+++ b/schemars/tests/expected/doc_comments_enum.json
@ -19,9 +19,6 @@
      "properties": {
        "Complex": {
          "type": "object",
-          "required": [
-            "my_nullable_string"
-          ],
          "properties": {
            "my_nullable_string": {
              "title": "A nullable string",
--- a/schemars/tests/expected/flatten.json
+++ b/schemars/tests/expected/flatten.json
@ -0,0 +1,34 @@
+{
+  "$schema": "http://json-schema.org/draft-07/schema#",
+  "title": "Flat",
+  "type": "object",
+  "required": [
+    "b",
+    "f",
+    "s",
+    "v"
+  ],
+  "properties": {
+    "b": {
+      "type": "boolean"
+    },
+    "f": {
+      "type": "number",
+      "format": "float"
+    },
+    "os": {
+      "default": "",
+      "type": "string"
+    },
+    "s": {
+      "type": "string"
+    },
+    "v": {
+      "type": "array",
+      "items": {
+        "type": "integer",
+        "format": "int32"
+      }
+    }
+  }
+}
--- a/schemars/tests/expected/struct-normal.json
+++ b/schemars/tests/expected/struct-normal.json
@ -10,6 +10,12 @@
    "bar": {
      "type": "boolean"
    },
+    "baz": {
+      "type": [
+        "string",
+        "null"
+      ]
+    },
    "foo": {
      "type": "integer",
      "format": "int32"
--- a/schemars/tests/expected/struct-tuple.json
+++ b/schemars/tests/expected/struct-tuple.json
@ -9,8 +9,14 @@
    },
    {
      "type": "boolean"
+    },
+    {
+      "type": [
+        "string",
+        "null"
+      ]
    }
  ],
-  "maxItems": 2,
-  "minItems": 2
+  "maxItems": 3,
+  "minItems": 3
 }
--- a/schemars/tests/flatten.rs
+++ b/schemars/tests/flatten.rs
@ -1,6 +1,6 @@
 mod util;
-use pretty_assertions::assert_eq;
-use schemars::{schema_for, JsonSchema};
+use schemars::JsonSchema;
+use util::*;

 #[derive(Debug, JsonSchema)]
 struct Flat {
@ -43,8 +43,11 @@ struct Deep4 {
 }

 #[test]
-fn flatten_schema() {
-    let flat = schema_for!(Flat);
-    let deep = schema_for!(Deep1);
-    assert_eq!(flat, deep);
+fn test_flat_schema() -> TestResult {
+    test_default_generated_schema::<Flat>("flatten")
+}
+
+#[test]
+fn test_flattened_schema() -> TestResult {
+    test_default_generated_schema::<Deep1>("flatten")
 }
--- a/schemars/tests/struct.rs
+++ b/schemars/tests/struct.rs
@ -6,6 +6,7 @@ use util::*;
 pub struct Struct {
    foo: i32,
    bar: bool,
+    baz: Option<String>,
 }

 #[test]
@ -14,7 +15,7 @@ fn struct_normal() -> TestResult {
 }

 #[derive(Debug, JsonSchema)]
-pub struct Tuple(i32, bool);
+pub struct Tuple(i32, bool, Option<String>);

 #[test]
 fn struct_tuple() -> TestResult {
--- a/schemars_derive/src/lib.rs
+++ b/schemars_derive/src/lib.rs
@ -38,7 +38,8 @@ pub fn derive_json_schema(input: proc_macro::TokenStream) -> proc_macro::TokenSt
        Data::Struct(Style::Struct, ref fields) => schema_for_struct(fields, Some(&cont.attrs)),
        Data::Enum(ref variants) => schema_for_enum(variants, &cont.attrs),
    };
-    let schema_expr = set_metadata_on_schema_from_docs(schema_expr, &cont.original.attrs);
+    let doc_metadata = SchemaMetadata::from_doc_attrs(&cont.original.attrs);
+    let schema_expr = doc_metadata.apply_to_schema(schema_expr);

    let type_name = cont.ident;
    let type_params: Vec<_> = cont.generics.type_params().map(|ty| &ty.ident).collect();
@ -176,7 +177,8 @@ fn schema_for_external_tagged_enum<'a>(
                ..Default::default()
            })),
        });
-        set_metadata_on_schema_from_docs(schema_expr, &variant.original.attrs)
+        let doc_metadata = SchemaMetadata::from_doc_attrs(&variant.original.attrs);
+        doc_metadata.apply_to_schema(schema_expr)
    }));

    wrap_schema_fields(quote! {
@ -214,7 +216,8 @@ fn schema_for_internal_tagged_enum<'a>(
                ..Default::default()
            })),
        });
-        let tag_schema = set_metadata_on_schema_from_docs(tag_schema, &variant.original.attrs);
+        let doc_metadata = SchemaMetadata::from_doc_attrs(&variant.original.attrs);
+        let tag_schema = doc_metadata.apply_to_schema(tag_schema);

        let variant_schema = match variant.style {
            Style::Unit => return tag_schema,
@ -244,7 +247,8 @@ fn schema_for_internal_tagged_enum<'a>(
 fn schema_for_untagged_enum<'a>(variants: impl Iterator<Item = &'a Variant<'a>>) -> TokenStream {
    let schemas = variants.map(|variant| {
        let schema_expr = schema_for_untagged_enum_variant(variant);
-        set_metadata_on_schema_from_docs(schema_expr, &variant.original.attrs)
+        let doc_metadata = SchemaMetadata::from_doc_attrs(&variant.original.attrs);
+        doc_metadata.apply_to_schema(schema_expr)
    });

    wrap_schema_fields(quote! {
@ -288,7 +292,7 @@ fn schema_for_tuple_struct(fields: &[Field]) -> TokenStream {
 }

 fn schema_for_struct(fields: &[Field], cattrs: Option<&serde_attr::Container>) -> TokenStream {
-    let (flat, nested): (Vec<_>, Vec<_>) = fields
+    let (flattened_fields, property_fields): (Vec<_>, Vec<_>) = fields
        .iter()
        .filter(|f| !f.attrs.skip_deserializing() || !f.attrs.skip_serializing())
        .partition(|f| f.attrs.flatten());
@ -299,63 +303,50 @@ fn schema_for_struct(fields: &[Field], cattrs: Option<&serde_attr::Container>) -
        SerdeDefault::Path(path) => Some(quote!(let container_default = #path();)),
    };

-    let mut required = Vec::new();
-    let recurse = nested.iter().map(|field| {
+    let properties = property_fields.iter().map(|field| {
        let name = field.attrs.name().deserialize_name();
        let default = field_default_expr(field, set_container_default.is_some());

-        if default.is_none() {
-            required.push(name.clone());
-        }
-
-        let ty = get_json_schema_type(field);
-        let span = field.original.span();
-        let schema_expr = quote_spanned! {span=>
-            gen.subschema_for::<#ty>()
+        let required = match default {
+            Some(_) => quote!(false),
+            None => quote!(true),
        };

-        let metadata = SchemaMetadata {
+        let metadata = &SchemaMetadata {
            read_only: field.attrs.skip_deserializing(),
            write_only: field.attrs.skip_serializing(),
            default,
            skip_default_if: field.attrs.skip_serializing_if().cloned(),
-            ..get_metadata_from_docs(&field.original.attrs)
+            ..SchemaMetadata::from_doc_attrs(&field.original.attrs)
        };
-        let schema_expr = set_metadata_on_schema(schema_expr, &metadata);
+
+        let ty = get_json_schema_type(field);
+        let span = field.original.span();

        quote_spanned! {span=>
-            props.insert(#name.to_owned(), #schema_expr);
+            <#ty>::add_schema_as_property(gen, &mut schema_object, #name.to_owned(), #metadata, #required);
        }
    });

-    let schema = wrap_schema_fields(quote! {
-        instance_type: Some(schemars::schema::InstanceType::Object.into()),
-        object: Some(Box::new(schemars::schema::ObjectValidation {
-            properties: {
-                let mut props = schemars::Map::new();
-                #(#recurse)*
-                props
-            },
-            required: {
-                let mut required = schemars::Set::new();
-                #(required.insert(#required.to_owned());)*
-                required
-            },
-            ..Default::default()
-        })),
-    });
-
-    let flattens = flat.iter().map(|field| {
+    let flattens = flattened_fields.iter().map(|field| {
        let ty = get_json_schema_type(field);
-        quote_spanned! {field.original.span()=>
-            .flatten(<#ty>::json_schema_optional(gen))
+        let span = field.original.span();
+
+        quote_spanned! {span=>
+            .flatten(<#ty>::json_schema_for_flatten(gen))
        }
    });

    quote! {
        {
            #set_container_default
-            #schema #(#flattens)*
+            let mut schema_object = schemars::schema::SchemaObject {
+                instance_type: Some(schemars::schema::InstanceType::Object.into()),
+                ..Default::default()
+            };
+            #(#properties)*
+            schemars::schema::Schema::Object(schema_object)
+            #(#flattens)*
        }
    }
 }
--- a/schemars_derive/src/metadata.rs
+++ b/schemars_derive/src/metadata.rs
@ -1,5 +1,6 @@
 use crate::attr;
-use proc_macro2::TokenStream;
+use proc_macro2::{Ident, Span, TokenStream};
+use quote::{ToTokens, TokenStreamExt};
 use syn::{Attribute, ExprPath};

 #[derive(Debug, Clone, Default)]
@ -12,49 +13,77 @@ pub struct SchemaMetadata {
    pub skip_default_if: Option<ExprPath>,
 }

-pub fn set_metadata_on_schema_from_docs(
-    schema_expr: TokenStream,
-    attrs: &[Attribute],
-) -> TokenStream {
-    let metadata = get_metadata_from_docs(attrs);
-    set_metadata_on_schema(schema_expr, &metadata)
+impl ToTokens for SchemaMetadata {
+    fn to_tokens(&self, tokens: &mut TokenStream) {
+        let setters = self.make_setters();
+        if setters.is_empty() {
+            tokens.append(Ident::new("None", Span::call_site()))
+        } else {
+            tokens.extend(quote! {
+                Some({
+                    let mut metadata = schemars::schema::Metadata::default();
+                    #(#setters)*
+                    metadata
+                })
+            })
+        }
+    }
 }

-pub fn get_metadata_from_docs(attrs: &[Attribute]) -> SchemaMetadata {
+impl SchemaMetadata {
+    pub fn from_doc_attrs(attrs: &[Attribute]) -> SchemaMetadata {
        let (title, description) = attr::get_title_and_desc_from_doc(attrs);
        SchemaMetadata {
            title,
            description,
            ..Default::default()
        }
-}
+    }

-pub fn set_metadata_on_schema(schema_expr: TokenStream, metadata: &SchemaMetadata) -> TokenStream {
+    pub fn apply_to_schema(&self, schema_expr: TokenStream) -> TokenStream {
+        let setters = self.make_setters();
+
+        if setters.is_empty() {
+            return schema_expr;
+        }
+
+        quote! {
+            {
+                let mut schema = #schema_expr.into();
+                gen.make_extensible(&mut schema);
+                let mut metadata = schema.metadata();
+                #(#setters)*
+                schemars::schema::Schema::Object(schema)
+            }
+        }
+    }
+
+    fn make_setters(self: &SchemaMetadata) -> Vec<TokenStream> {
        let mut setters = Vec::<TokenStream>::new();

-    if let Some(title) = &metadata.title {
+        if let Some(title) = &self.title {
            setters.push(quote! {
                metadata.title = Some(#title.to_owned());
            });
        }
-    if let Some(description) = &metadata.description {
+        if let Some(description) = &self.description {
            setters.push(quote! {
                metadata.description = Some(#description.to_owned());
            });
        }

-    if metadata.read_only {
+        if self.read_only {
            setters.push(quote! {
                metadata.read_only = true;
            });
        }
-    if metadata.write_only {
+        if self.write_only {
            setters.push(quote! {
                metadata.write_only = true;
            });
        }

-    match (&metadata.default, &metadata.skip_default_if) {
+        match (&self.default, &self.skip_default_if) {
            (Some(default), Some(skip_if)) => setters.push(quote! {
                {
                    let default = #default;
@ -69,17 +98,6 @@ pub fn set_metadata_on_schema(schema_expr: TokenStream, metadata: &SchemaMetadat
            _ => {}
        }

-    if setters.is_empty() {
-        return schema_expr;
-    }
-
-    quote! {
-        {
-            let schema = #schema_expr.into();
-            let mut schema_obj = gen.make_extensible(schema);
-            let mut metadata = schema_obj.metadata();
-            #(#setters)*
-            schemars::schema::Schema::Object(schema_obj)
-        }
+        setters
    }
 }