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

Schemars overwrites declarations if there are structs with the same name #62

Closed
NeoLegends opened this issue Nov 19, 2020 · 6 comments · Fixed by #247
Closed

Schemars overwrites declarations if there are structs with the same name #62

NeoLegends opened this issue Nov 19, 2020 · 6 comments · Fixed by #247

Comments

@NeoLegends
Copy link

mod a {
    use super::*;

    #[derive(JsonSchema)]
    pub struct Config {
        test: String,
    }
}
mod b {
    use super::*;

    #[derive(JsonSchema)]
    pub struct Config {
        test2: String,
    }
}

#[derive(JsonSchema)]
pub struct Config2 {
    a_cfg: a::Config,
    b_cfg: b::Config,
}

generates the following, invalid schema:

{
  "$schema": "http://json-schema.org/draft-07/schema#",
  "title": "Config2",
  "type": "object",
  "required": [
    "a_cfg",
    "b_cfg"
  ],
  "properties": {
    "a_cfg": {
      "$ref": "#/definitions/Config"
    },
    "b_cfg": {
      "$ref": "#/definitions/Config"
    }
  },
  "definitions": {
    "Config": {
      "type": "object",
      "required": [
        "test"
      ],
      "properties": {
        "test": {
          "type": "string"
        }
      }
    }
  }
}

This is because there are two member structs with the same name, although they sit at different paths. This use case occurs in our systems because some (more top-level modules) export their own Config structs which we then aggregate into one, global config for which we want to generate a schema for.

Is this expected behavior? What can we do to fix this? Would it make sense to extend schemars with support for this use case? It could automatically incorporate, e. g. the parent module name / the module path into the name of the definitions, such that these clashes can be avoided.

@NeoLegends
Copy link
Author

Note that turning on inlining is a possible workaround. This may make the schema more verbose though.

https://docs.rs/schemars/0.8.0/schemars/gen/struct.SchemaSettings.html#structfield.inline_subschemas

@pashadia
Copy link

pashadia commented Feb 3, 2021

I have found and successfully used a workaround: Using #[schemars(rename = "......")] on one of the structs with duplicated definitions.

@GREsau
Copy link
Owner

GREsau commented Mar 22, 2021

This is a tricky one - schemars somewhat naively treats the schema_name (set to the type name by schemars_derive) as a unique identifier for a type/schema.

I've considered trying to find a better way to do it that properly differentiates types with the same name. Perhaps we could use TypeId, although that gets tricky with borrowed types, since I think for our purposes we would want T and &T to be considered the same type.

@GREsau
Copy link
Owner

GREsau commented Mar 22, 2021

Currently, the suggestions from @NeoLegends and @pashadia are the best way to work around this. However, rename = '...' relies on one of the structs being in your crate, which may not always be the case.

I can think of two potential ways we could fix this, both rely on adding a new attribute:

Option 1: rename_type

#[derive(JsonSchema)]
pub struct Config2 {
    a_cfg: a::Config,
    #[schemars(rename_type = "ConfigB")]
    b_cfg: b::Config,
}

Which would produce:

{
  "properties": {
    "a_cfg": { "$ref": "#/definitions/Config" },
    "b_cfg": { "$ref": "#/definitions/ConfigB" }
  },
  "definitions": {
    "Config": { /* ... */ },
    "ConfigB": { /* ... */ }
  },
  /* ... */
}

Option 2: inline

#[derive(JsonSchema)]
pub struct Config2 {
    a_cfg: a::Config,
    #[schemars(inline)]
    b_cfg: b::Config,
}

Which would produce:

{
  "properties": {
    "a_cfg": {
      "$ref": "#/definitions/Config"
    },
    "b_cfg": {
        "type": "object",
        "required": [
          "test2"
        ],
        "properties": {
          "test2": {
            "type": "string"
          }
        }
      }
  },
  "definitions": {
    "Config": { /* ... */ }
  },
  /* ... */
}

@GREsau
Copy link
Owner

GREsau commented Apr 25, 2021

A possible alternative fix would be to just treat schema_name() as a "friendly" name (used as the top-level schema title and key within definitions, as it is today), and have a separate function to return a unique name/ID which doesn't appear in the resulting schema. Then schemars can detect when two types with the same friendly name are added in the same schema, and fix it by altering one of their friendly names, e.g. just append a "2" to the end

A simple implementation of the unique ID would be to concatenate the the type's module path with the friendly name, although we'd need to be careful re generic types, e.g. ensure that HashMap<a::Config> and HashMap<b::Config> have different IDs.

BrynCooke pushed a commit to apollographql/router that referenced this issue Mar 14, 2022
… with the json schema.

Previously if two config structs had the same name they would be resolved to the same definition.
GREsau/schemars#62
BrynCooke added a commit to apollographql/router that referenced this issue Mar 15, 2022
* Separate out apollo plugins from general plugins section.

* Enable inline subschemas to prevent name clashes from causeing issues with the json schema.
Previously if two config structs had the same name they would be resolved to the same definition.
GREsau/schemars#62

* Rename plugins to user_plugins.
Now we have `apollo_plugins` and `user_plugins`

* Move logic around defaulting and ordering to configuration.
Reporting comes first, rhai comes last.

* Fix tests.
We can't enable the reporting plugin for unit testing, so check to see if the global subscriber is set before adding it.

* Changelog
Clippy

* Test fixes

* Review fixes

Co-authored-by: bryn <bryn@apollographql.com>
@GREsau
Copy link
Owner

GREsau commented Sep 17, 2023

This is finally fixed in v0.8.14 😄

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.

3 participants