Skip to content

Bugfix for discriminators using allOf #1578

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Apr 3, 2024
Merged

Bugfix for discriminators using allOf #1578

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1f6dea4
Fixed base class discriminators to only be handled in allOfs
mzronek Mar 5, 2024
b645047
Improved Windows compatibility for update:examples script
mzronek Mar 5, 2024
0d8c81c
Support multiple discriminator values pointing to the same refs in al…
mzronek Mar 5, 2024
ffbe547
Updated examples for improved discriminator allOf support
mzronek Mar 5, 2024
ce292e7
Added jsdoc for new patchDiscriminatorEnum function
mzronek Mar 6, 2024
df9e8b2
Readded composition test for oneOf inside an object as its own unit test
mzronek Mar 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions packages/openapi-typescript/examples/digital-ocean-api.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19323,7 +19323,7 @@ export interface operations {
* */
requestBody?: {
content: {
"application/json": Omit<components["schemas"]["floating_ip_action_unassign"], "type"> | Omit<components["schemas"]["floating_ip_action_assign"], "type">;
"application/json": components["schemas"]["floating_ip_action_unassign"] | components["schemas"]["floating_ip_action_assign"];
};
};
responses: {
Expand Down Expand Up @@ -22367,7 +22367,7 @@ export interface operations {
* */
requestBody?: {
content: {
"application/json": Omit<components["schemas"]["reserved_ip_action_unassign"], "type"> | Omit<components["schemas"]["reserved_ip_action_assign"], "type">;
"application/json": components["schemas"]["reserved_ip_action_unassign"] | components["schemas"]["reserved_ip_action_assign"];
};
};
responses: {
Expand Down
37 changes: 25 additions & 12 deletions packages/openapi-typescript/scripts/update-examples.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,18 +15,31 @@ async function generateSchemas() {
const updateSchema = async (name: string, ext: string) => {
const start = performance.now();

await execa(
"./bin/cli.js",
[`./examples/${name}${ext}`, "-o", `./examples/${name}.ts`],
{ cwd },
);

schemasDoneCount++;
const timeMs = Math.round(performance.now() - start);

console.log(
`✔︎ [${schemasDoneCount}/${schemaTotalCount}] Updated ${name} (${timeMs}ms)`,
);
try {
await execa(
"./bin/cli.js",
[`./examples/${name}${ext}`, "-o", `./examples/${name}.ts`],
{
cwd:
process.platform === "win32"
? // execa/cross-spawn can not handle URL objects on Windows, so convert it to string and cut away the protocol
cwd.toString().slice("file:///".length)
: cwd,
},
);

schemasDoneCount++;
const timeMs = Math.round(performance.now() - start);

console.log(
`✔︎ [${schemasDoneCount}/${schemaTotalCount}] Updated ${name} (${timeMs}ms)`,
);
} catch (error) {
console.error(
`✘ [${schemasDoneCount}/${schemaTotalCount}] Failed to update ${name}`,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice addition 👍

{ error: error instanceof Error ? error.message : error },
);
}
};

console.log("Updating examples...");
Expand Down
2 changes: 1 addition & 1 deletion packages/openapi-typescript/src/lib/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -326,7 +326,7 @@ export function scanDiscriminators(
// (sometimes this mapping is implicit, so it can’t be done until we know
// about every discriminator in the document)
walk(schema, (obj, path) => {
for (const key of ["oneOf", "anyOf", "allOf"] as const) {
for (const key of ["allOf"] as const) {
if (obj && Array.isArray(obj[key])) {
for (const item of (obj as any)[key]) {
if ("$ref" in item) {
Expand Down
29 changes: 18 additions & 11 deletions packages/openapi-typescript/src/transform/schema-object.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,8 +138,18 @@ export function transformSchemaObjectWithComposition(
* Object + composition (anyOf/allOf/oneOf) types
*/

/** Collect oneOf/allOf/anyOf with Omit<> for discriminators */
function collectCompositions(
/** Collect oneOf/anyOf */
function collectUnionCompositions(items: (SchemaObject | ReferenceObject)[]) {
const output: ts.TypeNode[] = [];
for (const item of items) {
output.push(transformSchemaObject(item, options));
}

return output;
}

/** Collect allOf with Omit<> for discriminators */
function collectAllOfCompositions(
items: (SchemaObject | ReferenceObject)[],
required?: string[],
): ts.TypeNode[] {
Expand Down Expand Up @@ -184,6 +194,7 @@ export function transformSchemaObjectWithComposition(
options,
);
}

const discriminator =
("$ref" in item && options.ctx.discriminators.objects[item.$ref]) ||
(item as any).discriminator; // eslint-disable-line @typescript-eslint/no-explicit-any
Expand All @@ -201,7 +212,7 @@ export function transformSchemaObjectWithComposition(

// core + allOf: intersect
const coreObjectType = transformSchemaObjectCore(schemaObject, options);
const allOfType = collectCompositions(
const allOfType = collectAllOfCompositions(
schemaObject.allOf ?? [],
schemaObject.required,
);
Expand All @@ -216,21 +227,17 @@ export function transformSchemaObjectWithComposition(
}
// anyOf: union
// (note: this may seem counterintuitive, but as TypeScript’s unions are not true XORs, they mimic behavior closer to anyOf than oneOf)
const anyOfType = collectCompositions(
schemaObject.anyOf ?? [],
schemaObject.required,
);
const anyOfType = collectUnionCompositions(schemaObject.anyOf ?? []);
if (anyOfType.length) {
finalType = tsUnion([...(finalType ? [finalType] : []), ...anyOfType]);
}
// oneOf: union (within intersection with other types, if any)
const oneOfType = collectCompositions(
const oneOfType = collectUnionCompositions(
schemaObject.oneOf ||
("type" in schemaObject &&
schemaObject.type === "object" &&
(schemaObject.enum as (SchemaObject | ReferenceObject)[])) ||
[],
schemaObject.required,
);
if (oneOfType.length) {
// note: oneOf is the only type that may include primitives
Expand Down Expand Up @@ -408,8 +415,8 @@ function transformSchemaObjectCore(
// type: object
const coreObjectType: ts.TypeElement[] = [];

// discriminatorss: explicit mapping on schema object
for (const k of ["oneOf", "allOf", "anyOf"] as const) {
// discriminators: explicit mapping on schema object
for (const k of ["allOf", "anyOf"] as const) {
if (!schemaObject[k]) {
continue;
}
Expand Down
37 changes: 37 additions & 0 deletions packages/openapi-typescript/test/discriminators.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,38 @@ describe("3.1 discriminators", () => {
},
allOf: [{ $ref: "#/components/schemas/Pet" }],
},
LizardDog: {
allOf: [
{ $ref: "#/components/schemas/Dog" },
{ $ref: "#/components/schemas/Lizard" },
],
},
AnimalSighting: {
oneOf: [
{
$ref: "#/components/schemas/Cat",
},
{
$ref: "#/components/schemas/Dog",
},
{
$ref: "#/components/schemas/Lizard",
},
],
},
Beast: {
anyOf: [
{
$ref: "#/components/schemas/Cat",
},
{
$ref: "#/components/schemas/Dog",
},
{
$ref: "#/components/schemas/Lizard",
},
],
},
},
},
},
Expand All @@ -65,6 +97,11 @@ export interface components {
petType: "Lizard";
lovesRocks?: boolean;
} & Omit<components["schemas"]["Pet"], "petType">;
LizardDog: {
petType: "LizardDog";
} & (Omit<components["schemas"]["Dog"], "petType"> & Omit<components["schemas"]["Lizard"], "petType">);
AnimalSighting: components["schemas"]["Cat"] | components["schemas"]["Dog"] | components["schemas"]["Lizard"];
Beast: components["schemas"]["Cat"] | components["schemas"]["Dog"] | components["schemas"]["Lizard"];
};
responses: never;
parameters: never;
Expand Down
14 changes: 2 additions & 12 deletions packages/openapi-typescript/test/transform/schema-object/composition.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,20 +258,12 @@ describe("composition", () => {
"discriminator > oneOf",
{
given: {
type: "object",
Copy link
Contributor Author

@mzronek mzronek Mar 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@drwpow I was unsure why there is already an object here for the test of a oneOf discriminator, so I removed it. Maybe the implicit allOf merging here should be in a separate unit test?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes this actually should be its own test, you’re right, but it’s important we keep this “wrong” testcase. We still get so many bug reports and code examples of people combining type: "object" with its own properties + oneOf (even though in the docs it suggests not doing that). You’re right—it shouldn’t be the ideal case, but we should keep it as a test case so we can just be sure that it generates something (logical collisions are the responsibility of the schema author, but openapi-typescript should still attempt to generate something that reflects what they’ve authored)

required: ["name"],
properties: {
name: { type: "string" },
},
oneOf: [
{ $ref: "#/components/schemas/Cat" },
{ $ref: "#/components/schemas/Dog" },
],
},
want: `{
petType: "Pet";
name: string;
} & (Omit<components["schemas"]["Cat"], "petType"> | Omit<components["schemas"]["Dog"], "petType">)`,
want: `components["schemas"]["Cat"] | components["schemas"]["Dog"]`,
options: {
path: "#/components/schemas/Pet",
ctx: {
Expand Down Expand Up @@ -313,9 +305,7 @@ describe("composition", () => {
given: {
oneOf: [{ $ref: "#/components/schemas/parent" }, { type: "null" }],
},
want: `{
operation: "schema-object";
} & (Omit<components["schemas"]["parent"], "operation"> | null)`,
want: `components["schemas"]["parent"] | null`,
options: {
...DEFAULT_OPTIONS,
ctx: {
Expand Down