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

Enforce strict key values for BTreeMap with enum key #128

Open
MicaiahReid opened this issue Jul 7, 2023 · 5 comments · May be fixed by GREsau/schemars#231
Open

Enforce strict key values for BTreeMap with enum key #128

MicaiahReid opened this issue Jul 7, 2023 · 5 comments · May be fixed by GREsau/schemars#231
Labels
schemars This is affected by the schemars crate

Comments

@MicaiahReid
Copy link

MicaiahReid commented Jul 7, 2023

Whenever a schema is generated for a BTreeMap with an enum value as the key, the result allows any string for the key. For example:

#[derive(serde::Serialize, JsonSchema, Debug, PartialEq, Eq, Clone, PartialOrd, Ord)]
enum Thing {
    Option1,
    Option2,
}
#[derive(serde::Serialize, JsonSchema)]
struct Response {
    reply: BTreeMap<Thing, String>,
}

generates:

 "components": {
    "schemas": {
      "Response": {
        "type": "object",
        "required": [
          "reply"
        ],
        "properties": {
          "reply": {
            "type": "object",
            "additionalProperties": {
              "type": "string"
            }
          }
        }
      }
    }
  }

This allows the reply object to have any key, when it should really be limited to the enum values. Something like:

 "components": {
    "schemas": {
      "Response": {
        "type": "object",
        "required": [
          "reply"
        ],
        "properties": {
          "reply": {
            "oneOf": [
              {
                "type": "object",
                "required": [
                  "Option1"
                ],
                "properties": {
                  "Option1": {
                    "type": "string"
                  }
                }
              },
              {
                "type": "object",
                "required": [
                  "Option2"
                ],
                "properties": {
                  "Option2": {
                    "type": "string"
                  }
                },
                "maxProperties": 1
              }
            ]
          }
        }
      }
    }
  }

I don't know if there's any other way to limit the possible key values for an object, other than using the oneOf property, but being able to limit potential keys of a spec using a BTreeMap would be really helpful

@ralpha
Copy link
Collaborator

ralpha commented Jul 7, 2023

Would have to look more into this. But in any case the IndexMap or more specifically the Schemars::Map should be preferred.
This is because BTreeMap does not guarantee order or objects and can be different each time it is compiled.
(What would make build result less reliable)

I also think this part is handled by Schemars and not by Okapi. As everything under "schemas": is created there.
So also considering opening an issue there, or look if there already is an existing issue.

@ralpha
Copy link
Collaborator

ralpha commented Jul 7, 2023

If you want to have a look at the issue the BTreeMap implementation is here: https://github.com/GREsau/schemars/blob/master/schemars/src/json_schema_impls/maps.rs
IndexMap uses the same implementation as HashMap and BTreeMap for sub component generation. (So will have same problem for all, but also 1 fix for all)
For the internal Enum you need to look here: https://github.com/GREsau/schemars/blob/master/schemars_derive/src/schema_exprs.rs#L123

@MicaiahReid
Copy link
Author

Great, thanks for the info, @ralpha! I'll take a look to see if I can get a PR in for a fix.

@MicaiahReid
Copy link
Author

I've started a PR, but I don't think I'll have the time to properly grok this to get it working properly. If anyone wants to pick up where I left off, that would be rad, but I understand there are other priorities.

@MicaiahReid
Copy link
Author

@ralpha I've opened a PR for this, I'd appreciate a review! GREsau/schemars#231

@ralpha ralpha added the schemars This is affected by the schemars crate label Dec 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
schemars This is affected by the schemars crate
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants