Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Option<bool> gets required in schema #11

Closed
patriksvensson opened this issue Jan 24, 2020 · 6 comments · Fixed by #16
Closed

Option<bool> gets required in schema #11

patriksvensson opened this issue Jan 24, 2020 · 6 comments · Fixed by #16

Comments

@patriksvensson
Copy link

Hello, and thank you for a great project!

I suspect I've encountered a bug (or I'm holding it wrong), but for some reason optional bools are set as required in definitions.

The version I'm using is 0.7.0-alpha-1

struct MyStruct {
  pub foo: Option<bool>
}

pub fn get_schema() -> String {
    let settings = schemars::gen::SchemaSettings::draft07().with(|s| {
        s.option_nullable = false;
        s.option_add_null_type = false;
    });
    let gen = settings.into_generator();
    let schema = gen.into_root_schema_for::<config::MyStruct>();
    serde_json::to_string_pretty(&schema).unwrap()
}

Generates the following schema:

{
  "$schema": "http://json-schema.org/draft-07/schema#",
  "title": "MyStruct",
  "type": "object",
  "required": [
    "foo"
  ],
  "properties": {
    "foo": {
      "type": "boolean"
    }
  }
}

If I add #[serde(default)] to foo it removes the requirement.

Would it make sense to add an option to SchemaSettings that says "Don't make Options required". I realize that the logic for this probably is more complicated since we have to take option_nullable and option_add_null_type into account, but having some kind of sensible default for this would make adoption easier.

Again, thanks for your great work. Much appreciated!

@GREsau
Copy link
Owner

GREsau commented Jan 24, 2020

My understanding was that deserializing an Option<T> field still required a property with the correct name to be present (although that property could be set to null) - and if you want to allow omitting the property, then you need to add #[serde(default)].

However I just tested this and it's clear I was wrong - when deserializing Option<T> fields from JSON, a missing property is treated the same as the property being set to null:

use serde::{Deserialize, Serialize};

#[derive(Debug, Deserialize, Serialize)]
struct MyStruct {
  pub foo: Option<bool>
}

fn main() {
    let json = r#"{ "foo": false }"#;
    let s: MyStruct = serde_json::from_str(json).unwrap();
    assert_eq!(s.foo, Some(false));
    
    let json = r#"{ "foo": null }"#;
    let s: MyStruct = serde_json::from_str(json).unwrap();
    assert_eq!(s.foo, None);
    
    let json = "{ }";
    let s: MyStruct = serde_json::from_str(json).unwrap();
    assert_eq!(s.foo, None);
}

In other words, this is indeed a bug in schemars! I don't think this should be controlled by an option in SchemaSettings, I think it should just always be the case that Option fields are not required.

@GREsau
Copy link
Owner

GREsau commented Jan 24, 2020

Unfortunately I don't have a lot of time right now so this probably won't be fixed for a few weeks - but I'll try to get round to fixing this at some point...

@patriksvensson
Copy link
Author

No problem at all. Perhaps I could take a stab at it if you have any suggestion of how it might work?

@patriksvensson
Copy link
Author

Sorry, missed your previous response. I can take a look at it and see if I can fix it.

@GREsau
Copy link
Owner

GREsau commented Jan 24, 2020

Ideally, the fix for this should work for all paths to std::option::Option, e.g. foo, bar and baz should all be optional here:

use std::option::Option as Opt;

#[derive(Debug, Deserialize, Serialize, JsonSchema)]
struct MyStruct {
  pub foo: Option<bool>,
  pub bar: std::option::Option<bool>,
  pub baz: Opt<bool>,
}

Similarly, a field of a user-defined Option type should still be required:

#[derive(Debug, PartialEq, Deserialize, Serialize, JsonSchema)]
struct Option<T>(T);

#[derive(Debug, Deserialize, Serialize, JsonSchema)]
struct MyStruct {
  // foo is required
  pub foo: Option<bool>
}

Getting this behaviour from a proc-macro may be awkward though...

@GREsau
Copy link
Owner

GREsau commented Feb 29, 2020

Fixed in v0.7.0-alpha-2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants