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

Limitation: JSONSchemaType<T> does not support unions #1302

Closed
aiham opened this issue Oct 17, 2020 · 17 comments · Fixed by #1485
Closed

Limitation: JSONSchemaType<T> does not support unions #1302

aiham opened this issue Oct 17, 2020 · 17 comments · Fixed by #1485
Milestone

Comments

@aiham
Copy link

aiham commented Oct 17, 2020

What version of Ajv are you using? Does the issue happen if you use the latest version?

ajv@7.0.0-beta.1

Ajv options object

N/A (type error)

JSON Schema

See code below

Sample data

N/A (type error)

Your code

type VariableGroupValue = string | number | { query: string };

const variableGroupValueSchema: JSONSchemaType<VariableGroupValue> = {
  anyOf: [
    {
      type: 'string',
    },
    {
      type: 'number',
    },
    {
      type: 'object',
      properties: {
        query: {
          type: 'string',
        },
      },
      required: ['query'],
      additionalProperties: false,
    },
  ],
};

Validation result, data AFTER validation, error messages

const variableGroupValueSchema: JSONSchemaType<VariableGroupValue, false>
Type '{ anyOf: ({ type: "string"; } | { type: "number"; } | { type: "object"; properties: { query: { type: "string"; }; }; required: "query"[]; additionalProperties: false; })[]; }' is not assignable to type 'JSONSchemaType<VariableGroupValue, false>'.
  Type '{ anyOf: ({ type: "string"; } | { type: "number"; } | { type: "object"; properties: { query: { type: "string"; }; }; required: "query"[]; additionalProperties: false; })[]; }' is not assignable to type '{ type: "object"; required: "query"[]; additionalProperties: boolean; properties?: PropertiesSchema<{ query: string; }> | undefined; patternProperties?: { [pattern: string]: never; } | undefined; propertyNames?: JSONSchemaType<...> | undefined; dependencies?: { ...; } | undefined; minProperties?: number | undefined;...'.
    Type '{ anyOf: ({ type: "string"; } | { type: "number"; } | { type: "object"; properties: { query: { type: "string"; }; }; required: "query"[]; additionalProperties: false; })[]; }' is missing the following properties from type '{ type: "object"; required: "query"[]; additionalProperties: boolean; properties?: PropertiesSchema<{ query: string; }> | undefined; patternProperties?: { [pattern: string]: never; } | undefined; propertyNames?: JSONSchemaType<...> | undefined; dependencies?: { ...; } | undefined; minProperties?: number | undefined;...': type, required, additionalPropertiests(2322)
Peek Problem (Alt+F8)
No quick fixes available

image

It complains that the top level schema object doesn't have the keys: type, required, additionalPropertiests. I thought I could use anyOf on it's own. It doesn't really make sense to use type at the top level because it could be one of three different types but it looks like it expects it to be an object. I also tried oneOf with the same result.

Any suggestions or workarounds welcome, especially if my schema is wrong. Thanks

What results did you expect?

Expected no type error

Are you going to resolve the issue?

No.

@aiham aiham changed the title Typescript error when using oneOf with string|number|object Typescript error when using anyOf/oneOf with string|number|object Oct 17, 2020
@epoberezkin
Copy link
Member

epoberezkin commented Oct 17, 2020

This JSONSchemaType<T> type doesn’t support unions yet, so just use a generic Schema (or SchemaObject) type.

You can still compile schemas into type guards with type parameters

@epoberezkin
Copy link
Member

epoberezkin commented Oct 17, 2020

You could also use some type definition for generic JSON Schema, not parameterized on the data type - I’ve seen it in several places, so I don’t think they should be included here.

@epoberezkin epoberezkin changed the title Typescript error when using anyOf/oneOf with string|number|object Limitation: JSONSchemaType<T> does not support unions Oct 17, 2020
@aiham
Copy link
Author

aiham commented Oct 17, 2020

Thanks for the quick response. Yeah I added a type param to my type guard which worked post-validation (although it would be nice to validate the actual schema):

const validateVariableGroups = ajv.compile<VariableGroups>(variableGroupsSchema);

You could also use some type definition for generic JSON Schema, not parameterized on the data type - I’ve seen it in several places, so I don’t think they should be included here.

Could link to an example? I'm not sure what you mean by this, and I can't find too many alternative examples in the docs.

Thanks again.

@epoberezkin
Copy link
Member

although it would be nice to validate the actual schema

Ajv validates the schema itself it at run time, although not on the type level and not that it is correct, just that it is valid.

Could link to an example?

https://github.com/enriched/ts-json-schema/blob/master/src/JsonSchema.ts

I think that what I meant, but I’ve seen in other places - it’s easy to construct a type for a generic JSON schema, not linked to a particular data type

@aiham
Copy link
Author

aiham commented Oct 20, 2020

Thanks for the link.

Yep, it's good that it checks it's a valid schema but I meant it would be good if it matched my type definition at compile time. Since it doesn't it will have to be a unit test to ensure it's the correct schema in runtime.

Cheers

@epoberezkin
Copy link
Member

@erikbrinkman not sure you’ve seen it. Somebody on Gitter also just asked for it (not sure if you are there). I was planning to look at it some time in April, not sooner - if you could help improve JSONSchemaType before that lots of people would be happy (including myself obviously:)

@jrr
Copy link
Contributor

jrr commented Mar 8, 2021

It was me! I was hoping to use a schema to describe a property that's string | number.

For now I'm planning to apply schema validation to a subset of the object, and use some hand-written validation for troublesome bits like this, but perhaps in the future I'll be able to expand the schema-validated surface area to include it.

@epoberezkin
Copy link
Member

@jrr you can also do it without JSONSchemaType, rather than without schema - you can use type parameter explicitly without tying data type to schema.

@erikbrinkman
Copy link
Collaborator

I learned a bit about typescript transformation types when working on the JTDSchemaType. It might be possible to do something like validate oneOf: (Schema<A | B>)[]). This wouldn't prevent you from doing something like oneOf: [Schema]`. They would type check, but obviously not validate the union.

This isn't at the top of my priority list, as I generally much prefer JTD, and have used that for everything, but I'll try to take a stab at some point.

At some point I mentioned that there is one more complexity, which is the partial schema. e.g. in JSONSchema you can do { someProperties, anyOf: [{ restOfProperties }] } or { anyOf: [{ allProperties }] }. The former is more concise, but the latter will be much easier to type check. Is it worthless to only support the verbose method?

@aiham
Copy link
Author

aiham commented Mar 9, 2021

@erikbrinkman I don't think its worthless to only support the verbose method, especially as a first step since its easier to implement. It will cover some % of the cases (including my personal one above) but I've also used the partial schema form too so it's not perfect but you'll have to judge if the time/effort is worth implementing the whole thing. Thanks for looking into this again.

@aiham
Copy link
Author

aiham commented Mar 9, 2021

It also gives a workaround to people who really want type checking to restructure their schema to be verbose if they really want it and can tolerate the duplication of rules.

It would be good to keep the full implementation on a roadmap if it's not implemented straight away.

Thanks.

@erikbrinkman
Copy link
Collaborator

I've looked into this a bit, and there's a problem.

The current implementation takes the stance that JSONSchemaType<A | B> = JSONSchemaType<A> | JSONSchemaType<B>

This property is used to create the SomeJSONSchema type by calling JSONSchemaType<Known> where Known represents a union of all possible types. I don't see a way to address unions properly without breaking this.

Note, you can kind of specify unions, you just need to have a fully defined root object, and then define partial schema off of it. It won't work for everything, but there might be clever ways to make some things wok, e.g. you can't represent JSONSchemaType<string | number> but you can do:

type B = { b: number }
type C = { b: string }

const x: JSONSchemaType<B | C> = {
    type: "object",
    required: ["b"],
    anyOf: [
        { properties: { b: { type: "number" } } },
        { properties: { b: { type: "string" } } },
    ]
}

@aiham
Copy link
Author

aiham commented Mar 10, 2021

Thanks for looking into this. That's an interesting workaround, wrapping the value in an object and have the options all use the same key. If the typing was absolutely necessary it would probably do the trick but be a bit ugly since the wrapper is mostly redundant.

erikbrinkman added a commit to erikbrinkman/ajv that referenced this issue Mar 11, 2021
erikbrinkman added a commit to erikbrinkman/ajv that referenced this issue Mar 11, 2021
erikbrinkman added a commit to erikbrinkman/ajv that referenced this issue Mar 11, 2021
@epoberezkin
Copy link
Member

This property is used to create the SomeJSONSchema type by calling JSONSchemaType where Known represents a union of all possible types. I don't see a way to address unions properly without breaking this.

I would not worry about it, SomeJSONSchema can be implemented separately (I don't think it is even documented at the moment).

e.g. you can't represent JSONSchemaType<string | number> but you can do:

The workaround is a bit verbose tbh...

For simple types JSON Schema supports type as array, so maybe the solution could be (if it is possible):

  • if all members of the union are simple types:
    • recursively chip off members one by one, requiring that the type is array of simple types (in the same order as the union members
  • else if some members are structured types:
    • use anyOf (and still use array of types on the top level)

Having array of types on the top level is beneficial for JSON Schema because without it, for example, type coercion would not work correctly.

It is a difficult problem, maybe it cannot be solved

@erikbrinkman
Copy link
Collaborator

I think something like you mention should be possible if we break the existing behavior. I'm happy to take a stab, but it'd be good to have things a little more fleshed out.

Two

Two questions I have:

  • not all types have the same extra parameters, correct? So for a native string | number you might do type: ["string", "number"] but you might use anyOf if you wanted only positive numbers. In this case, both should potentially type check?
  • what about enum support? How do you imagine this integrating? Should number | "a" | "b" be { anyOf: [{ type: "number" }, { type: "string", enum: ["a", "b"]}] }. I guess that's the only thing that makes sense, but it seems like we'll have to pick an opinionated way, as unions of all possible ways to divide them up seem hard.
    • as a side note, the more I think about how complex this is, the more I think a JSONDataType might actually be easier 😅

It's also important to note that because this isn't tagged, it's impossible to make sure that every element is specified, for the same reason that you can't convert "a" | "b" to ["a", "b"].

Thinking about this while I write, I think I have the following structure in mind (loosely as this isn't real typescript):

| { anyOf: (JSONSchemaType<S in T> | JSONSchemaType<T>)[] }
| { type: (JSONType<S extends primitive in T>)[] }

essentially you can have an anyOf of all of the types or the union of all the types, and the union can also be typed as a type: ...[] of the primitives. Since we can't guarantee that all elements of the union are matched anyway, this should make it possible to represent number | string | { ... } as { anyOf: [{ type: ["number", "string"] }, { type: "object", ... }] }, while number | string can just be { type: ["number", "string"] }.

Does that make sense and sound reasonable?

@epoberezkin
Copy link
Member

not all types have the same extra parameters, correct? So for a native string | number you might do type: ["string", "number"] but you might use anyOf if you wanted only positive numbers. In this case, both should potentially type check?

Yes

as unions of all possible ways to divide them up seem hard.

Yes, that's why I didn't do it :) There are plenty of edge cases and somewhere the line should be drawn.

JSONDataType might actually be easier

Interesting :)

Does that make sense and sound reasonable?

Yes. I am very happy with any limitations tbh - I would just add them to the docs. JSONSchemaType is not comprehensive as is anyway, it only highlights how difficult it is to map JSON Schema to types (in general case)

erikbrinkman added a commit to erikbrinkman/ajv that referenced this issue Mar 14, 2021
@epoberezkin
Copy link
Member

in v8

@epoberezkin epoberezkin added this to the 8.0.0 milestone Mar 15, 2021
andriyl pushed a commit to Redocly/ajv that referenced this issue Jun 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

4 participants