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 to generate if-then-else schema for union types #1209

Closed
mdesousa opened this issue Apr 5, 2022 · 6 comments · Fixed by #1376
Closed

Option to generate if-then-else schema for union types #1209

mdesousa opened this issue Apr 5, 2022 · 6 comments · Fixed by #1376

Comments

@mdesousa
Copy link
Contributor

mdesousa commented Apr 5, 2022

Today (v1.0.0) a union type like the following:

export type Fish = {
  animal_type: 'fish';
  found_in: 'ocean' | 'river';
};

export type Bird = {
  animal_type: 'bird';
  can_fly: boolean;
};

export type Animal = Bird | Fish;

Generates the following json schema:

    "Animal": {
      "anyOf": [
        {
          "$ref": "#/definitions/Bird"
        },
        {
          "$ref": "#/definitions/Fish"
        }
      ]
    },

This fulfills the need to validate the schema. However, when there is an error schema validators like ajv produce errors for all schema paths. As an example, { animal_type: 'fish', found_in: 'lake' } results in the following error:

must have required property 'can_fly': can_fly [keyword: required, instancePath: , schemaPath: #/definitions/Bird/required]
must be equal to one of the allowed values: ocean,river [keyword: enum, instancePath: /found_in, schemaPath: #/definitions/Fish/properties/found_in/enum]
must match a schema in anyOf:  [keyword: anyOf, instancePath: , schemaPath: #/anyOf]

This first line in this error can be misleading since animal_type is fish, and can_fly is not a property of fish. The second line is the real error that we'd like to present.
The errors can get more confusing as you add more types to the union type... a lot of errors are shown for types that are irrelevant given the provide animal_type.

A solution to this problem would be to have the option to generate if-then-else json schemas. These provide more information to the validators to pick the appropriate schema. So if we generate the following:

    "Animal": {
        "type": "object",
        "properties": {
          "animal_type": {
            "type": "string",
            "enum": ["fish", "bird"]
          }
        },
    "allOf": [
        {
          "if": {
            "properties": { "animal_type": { "const": "fish" } }
          },
          "then": {
            "$ref": "#/definitions/Fish"
          }
        },
        {
          "if": {
            "properties": { "animal_type": { "const": "bird" } }
          },
          "then": {
            "$ref": "#/definitions/Bird"
          }
        }
      ]
    }

The same input results in the following, more concise error:

must be equal to one of the allowed values: ocean,river [keyword: enum, instancePath: /found_in, schemaPath: #/definitions/Fish/properties/found_in/enum]
@Jason3S
Copy link
Contributor

Jason3S commented Apr 6, 2022

@mdesousa,

Excellent description.

I would push back on the validators. The validators have all the same information that this tool does. It is nearly impossible to know which field / fields to use as the type pivot. It gets even more complicated when you add sub-types.

The validator should be able to do a "best-fit" and give the appropriate error.

@mdesousa
Copy link
Contributor Author

mdesousa commented Apr 6, 2022

Yes @Jason3S , you are right... validators do have the same information. However, validators actually did their part by supporting if-then-else and would say "Instead of using an anyOf json-schema, you should be using if-then-else". It's a more clear representation of what is being modeled, as shown in the example documented here.

I think one option to figure this out from TypeScript could be to have an annotation for a property that fulfills the role of kind for a given type. For example:

/**
 * @kind-prop animal_type
 */
export type Animal = Bird | Fish;

This could be used as a hint to generate the schema using if-then-else instead of anyOf, and would provide the name of the property to use in the if conditions. It could also serve to make this an opt-in feature... if not provided, the anyOf schema can be generated.

@domoritz
Copy link
Member

domoritz commented Apr 6, 2022

While it might be easier for validators, we have to consider other common use cases like generating types/classes as in http://github.com/altair-viz/altair, which is generated from the Vega-Lite schema. Since Vega-Lite is the main use case of this library, we need to be careful to support this use case well.

@mdesousa
Copy link
Contributor Author

mdesousa commented Apr 6, 2022

Definitely wouldn't propose to alter the generation in ways that could break existing implementations.
Using an opt-in annotation like the one proposed would mean that everything is generated the same way, unless the if-then-else schema is the desired output.

@domoritz
Copy link
Member

domoritz commented Apr 6, 2022

I tried to avoid having too many knobs so far and would only be willing to accept this change with comprehensive unit tests.

@github-actions
Copy link

🚀 Issue was released in v1.1.0 🚀

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

Successfully merging a pull request may close this issue.

3 participants