hey-api
Discriminators without mapping creates broken type
Description
Hi, wonderful project, thanks!
I have discovered a bug with schemas that include a discriminator, but do not include a mapping, which, according to the spec is optional.
With the following schema:
{
"openapi": "3.0.3",
"paths": {
"/whatever": {
"get": {
"summary": "Do the thing",
"parameters": [
{
"in": "query",
"name": "id",
"description": "The id of the thing",
"required": true,
"schema": {
"$ref": "#/components/schemas/_types.FooBar"
},
"style": "form"
}
]
}
}
},
"components": {
"schemas": {
"_types.Foo": {
"type": "object",
"properties": {
"type": {
"type": "string",
"enum": [
"foo"
]
},
"fooProp": {
"type": "string"
}
}
},
"_types.Bar": {
"type": "object",
"properties": {
"type": {
"type": "string",
"enum": [
"bar"
]
},
"barProp": {
"type": "string"
}
}
},
"_types.FooBar": {
"discriminator": {
"propertyName": "type"
},
"oneOf": [
{
"$ref": "#/components/schemas/_types.Foo"
},
{
"$ref": "#/components/schemas/_types.Bar"
}
]
}
}
}
}
The output type is:
export type TypesFoo = {
type?: 'foo';
fooProp?: string;
};
export type TypesBar = {
type?: 'bar';
barProp?: string;
};
export type TypesFooBar = ({
type: '_types.Foo';
} & TypesFoo) | ({
type: '_types.Bar';
} & TypesBar);
However, TypesFooBar is actually unusable as a type now as the type property for each of the constituents has been polluted with some metadata from the schema.
I believe this should have come out as simply:
export type TypesFooBar = TypesFoo | TypesBar;
or
export type TypesFooBar = ({
type: 'foo';
} & TypesFoo) | ({
type: 'bar';
} & TypesBar);
Hopefully this is enough information to recreate, thanks in advance!
Reproducible example or configuration
I have created a reproduction project here: https://github.com/JamesMcNee/hey-api-discrimination-example/blob/main/src/index.ts
OpenAPI specification (optional)
No response
System information (optional)
No response
@JamesMcNee my impression is this is an incorrect spec because type is optional (how can it discriminate anything if it's missing) and it doesn't constrain to its schema name. How did you generate this specification?
Hi @mrlubos, thanks for looking into this.
You are correct, that was my mistake, type should not be optional. I have pushed to my example with these fields required, however the issue with the generated type still remains.
export type TypesFoo = {
type: 'foo';
fooProp?: string;
};
export type TypesBar = {
type: 'bar';
barProp?: string;
};
export type TypesFooBar = ({
type: '_types.Foo';
} & TypesFoo) | ({
type: '_types.Bar';
} & TypesBar);
I came across this issue when generating a client from the Elasticsearch API spec, which can be found here. My example was manually cribbed from this to be a small reproduction.
I was curious to follow your work with Copilot in the PR, and did notice that you pointed out that if mapping is missing, the schemas name should match the discriminable value, which in both the elastic example and my reproduction it does not! However, I do lean on the side of not generating the broken type in this case and simply ignoring the discriminator, which will actually produce a discriminated type in the generation. Keen to hear your thoughts!
@JamesMcNee It comes down to whether this is a spec-compliant schema.
If not, the maintenance burden shouldn't be shifted downstream on tooling, and this is the stance I take on these issues.
If yes, I'd fix it, but so far that doesn't seem to be the case. What if there was another oneOf schema foo and it correctly constrained itself with type: 'foo'? The handling you propose would work for your case but not this one, which leads me to the conclusion this is a spec issue.
Assuming that's the case, short of pushing for the fix at the source, I'd suggest trying to patch it yourself https://heyapi.dev/openapi-ts/configuration/parser#patch. I understand this might not be feasible but that should motivate Elastic to fix their spec, not me to handle their interpretation of the discriminator keyword.
Makes sense. I’ll do some digging when I get back to my laptop. I have solved it for my use case by just recursively deleting the discriminator keys before passing it to client gen.
I agree that if the schema is not spec compliant then it’s not really the libraries fault. However I am not sure the library is doing the complete right thing as if it cannot find the schema at the top level, maybe it should ignore it.
Hi @mrlubos, okay I have done some more reading and experimenting and here are my thoughts.
When using the discriminator without a mapping, the discriminable value (propertyName) for each of the constituent schemas of the oneOf, anyOf or allOf MUST match the name of the constituent schema exactly, if not, the schema is not spec compliant.
Thus, both the elastic-search schema I linked, and the one in my example are not spec compliant. Therefore, I agree with you that this is not the job of this library to fix, the input is incorrect.
However, as this library is accepting non spec compliant schemas (and also interestingly validators such as ^1 do not seem to warn/error on this either, despite the spec being clear that it should fail^2 validation), I do think there is a case that its behaviour should be to act as if this key was not present, rather than attempting to understand it.
As I said in my previous message, I have worked around this by deleting all the discriminators before passing the schema to the library, which works for my use case. I think next steps for me will be to attempt to understand how elastic are generating their schema and raise an issue with them about it not being spec compliant as you suggest.
Thanks for the time you have spent on this issue, I will leave it with you on if the behaviour of the library is correct here, despite the invalid schema. If you think that it is, feel free to close the issue.
Hello! Elasticsearch specification maintainer here. For context, we don't think OpenAPI is flexible enough to model the Elasticsearch APIs, and we publish OpenAPI files mostly for documentation purposes. That said, we're happy to fix bugs when possible. Can you please clarify what is expected in mapping here? Would you mind modifying your example above to make it a valid OpenAPI spec?
@pquentin I think adding the mapping field to discriminators would be the cleanest diff in your spec (none of them have it).
Here's how the sample spec could be fixed, it's really just mapping the actual constrained type field value to the schema reference (edit: and ensuring type is required but I believe that's not an issue in your spec)
{
"openapi": "3.0.3",
"paths": {
"/whatever": {
"get": {
"summary": "Do the thing",
"parameters": [
{
"in": "query",
"name": "id",
"description": "The id of the thing",
"required": true,
"schema": {
"$ref": "#/components/schemas/_types.FooBar"
},
"style": "form"
}
]
}
}
},
"components": {
"schemas": {
"_types.Foo": {
"type": "object",
"properties": {
"type": {
"type": "string",
"enum": [
"foo"
]
},
"fooProp": {
"type": "string"
}
}
},
"_types.Bar": {
"type": "object",
"properties": {
"type": {
"type": "string",
"enum": [
"bar"
]
},
"barProp": {
"type": "string"
}
}
},
"_types.FooBar": {
"discriminator": {
"propertyName": "type",
"mapping": {
"foo": "#/components/schemas/_types.Foo",
"bar": "#/components/schemas/_types.Bar"
}
},
"oneOf": [
{
"$ref": "#/components/schemas/_types.Foo"
},
{
"$ref": "#/components/schemas/_types.Bar"
}
]
}
}
}
}
Hi @pquentin, thanks for reaching out. I had intended to reach out on the elastic project, however did not yet have chance to.
we don't think OpenAPI is flexible enough to model the Elasticsearch APIs, and we publish OpenAPI files mostly for documentation purposes
Understood, and appreciate this! The types that are produced from it using this library I find are significantly better than those in the officially provided client , so the spec is pretty good! Thanks for linking the blog post, interesting stuff.
Can you please clarify what is expected in mapping here? Would you mind modifying your example above to make it a valid OpenAPI spec?
@mrlubos has gracefully provided this, his note around ensuring type is required is correct, this was an issue in my original example, but not in the elastic schema. The important thing is that the key of the mapping must be the actual value from that field in the linked schema, then the mappings value must be the path to the schema.
As an actual example from the elastic spec, taking the smallest example snippet:
{
"_types.analysis.Normalizer": {
"discriminator": {
"propertyName": "type"
},
"oneOf": [
{
"$ref": "#/components/schemas/_types.analysis.LowercaseNormalizer"
},
{
"$ref": "#/components/schemas/_types.analysis.CustomNormalizer"
}
]
}
}
would become
{
"_types.analysis.Normalizer": {
"discriminator": {
"propertyName": "type",
"mapping": {
"lowercase": "#/components/schemas/_types.analysis.LowercaseNormalizer",
"custom": "#/components/schemas/_types.analysis.CustomNormalizer"
}
},
"oneOf": [
{
"$ref": "#/components/schemas/_types.analysis.LowercaseNormalizer"
},
{
"$ref": "#/components/schemas/_types.analysis.CustomNormalizer"
}
]
}
}
Thanks!
Thank you for the detailed explanations, that was very useful! We fixed the Elasticsearch OpenAPI spec to include mapping, this specific issue should be solved.
@JamesMcNee Let me know if you're happy with the output and we can close this issue!
Awesome @pquentin , thanks for picking it up!
@mrlubos, Yep can close, ultimately the spec was invalid, so the behaviour of the library is somewhat undefined in that case 👍
Thanks for your time on this both.