From 862814e7e475f4c9f17069e632487a9e74b217e5 Mon Sep 17 00:00:00 2001 From: Christopher Radek <14189820+chrisradek@users.noreply.github.com> Date: Thu, 29 Aug 2024 14:54:45 -0700 Subject: [PATCH] tsp-openapi3 - improve model generation of schemas using allOf (#4232) Fixes https://github.com/microsoft/typespec/issues/4152 Depends on https://github.com/microsoft/typespec/pull/4216 This PR updates how tsp-openapi3 handles generating models for schemas that use `allOf`. Currently `allOf` is ignored unless there is only 1 member and that member is a schema reference. In this scenario, the model extends the single member. This update now takes all of the schema `allOf` members into consideration when generating a model. - inline-schemas have their properties merged into the model's properties - schema references without a discriminator defined are spread into the model - if only 1 schema reference contains a discriminator, then the model extends it, otherwise these schema references are spread as well. --------- Co-authored-by: Christopher Radek --- ...enapi3-improve-allof-2024-7-21-14-39-30.md | 7 + .../convert/generators/generate-model.ts | 11 +- .../src/cli/actions/convert/interfaces.ts | 2 + .../transforms/transform-component-schemas.ts | 64 ++++-- .../src/cli/actions/convert/utils/context.ts | 6 + .../test/tsp-openapi3/data-types.test.ts | 187 ++++++++++++++++++ .../tsp-openapi3/output/one-any-all/main.tsp | 6 +- 7 files changed, 260 insertions(+), 23 deletions(-) create mode 100644 .chronus/changes/tsp-openapi3-improve-allof-2024-7-21-14-39-30.md diff --git a/.chronus/changes/tsp-openapi3-improve-allof-2024-7-21-14-39-30.md b/.chronus/changes/tsp-openapi3-improve-allof-2024-7-21-14-39-30.md new file mode 100644 index 0000000000..9e1536b515 --- /dev/null +++ b/.chronus/changes/tsp-openapi3-improve-allof-2024-7-21-14-39-30.md @@ -0,0 +1,7 @@ +--- +changeKind: fix +packages: + - "@typespec/openapi3" +--- + +Improves tsp-openapi3 model generation from schemas utilizing allOf. Models will now extend an allOf member if it is a schema reference and the only member with a discriminator. Other members will be spread into the model if defined as a schema reference, or have their properties treated as top-level properties if they are an inline-schema. \ No newline at end of file diff --git a/packages/openapi3/src/cli/actions/convert/generators/generate-model.ts b/packages/openapi3/src/cli/actions/convert/generators/generate-model.ts index 95e079f3cb..97ef77e414 100644 --- a/packages/openapi3/src/cli/actions/convert/generators/generate-model.ts +++ b/packages/openapi3/src/cli/actions/convert/generators/generate-model.ts @@ -122,6 +122,10 @@ function generateModel(model: TypeSpecModel, context: Context): string { definitions.push(...generateDecorators(model.decorators)); definitions.push(modelDeclaration.open); + if (model.spread?.length) { + definitions.push(...model.spread.map((spread) => `...${spread};`)); + } + definitions.push( ...model.properties.map((prop) => { // Decorators will be a combination of top-level (parameters) and @@ -152,17 +156,12 @@ type ModelDeclarationOutput = { open: string; close?: string }; function generateModelDeclaration(model: TypeSpecModel): ModelDeclarationOutput { const modelName = model.name; - const modelType = model.type ?? "object"; if (model.is) { return { open: `model ${modelName} is ${model.is};` }; } - if (!model.extends) { - return { open: `model ${modelName} {`, close: "}" }; - } - - if (modelType === "object") { + if (model.extends) { return { open: `model ${modelName} extends ${model.extends} {`, close: "}" }; } diff --git a/packages/openapi3/src/cli/actions/convert/interfaces.ts b/packages/openapi3/src/cli/actions/convert/interfaces.ts index 9925e8fe07..eeb04eb2c3 100644 --- a/packages/openapi3/src/cli/actions/convert/interfaces.ts +++ b/packages/openapi3/src/cli/actions/convert/interfaces.ts @@ -65,6 +65,8 @@ export interface TypeSpecModel extends TypeSpecDeclaration { * Defaults to 'object' */ type?: OpenAPI3Schema["type"]; + + spread?: string[]; } export interface TypeSpecAlias extends Pick { diff --git a/packages/openapi3/src/cli/actions/convert/transforms/transform-component-schemas.ts b/packages/openapi3/src/cli/actions/convert/transforms/transform-component-schemas.ts index 97b2a389a6..775f0915ea 100644 --- a/packages/openapi3/src/cli/actions/convert/transforms/transform-component-schemas.ts +++ b/packages/openapi3/src/cli/actions/convert/transforms/transform-component-schemas.ts @@ -86,7 +86,7 @@ export function transformComponentSchemas(context: Context, models: TypeSpecMode schema: OpenAPI3Schema ): void { const { name, scope } = getScopeAndName(rawName); - const extendsParent = getModelExtends(schema, scope); + const allOfDetails = getAllOfDetails(schema, scope); const isParent = getModelIs(schema, scope); types.push({ kind: "model", @@ -94,12 +94,13 @@ export function transformComponentSchemas(context: Context, models: TypeSpecMode scope, decorators: [...getDecoratorsForSchema(schema)], doc: schema.description, - properties: getModelPropertiesFromObjectSchema(schema), + properties: [...getModelPropertiesFromObjectSchema(schema), ...allOfDetails.properties], additionalProperties: typeof schema.additionalProperties === "object" ? schema.additionalProperties : undefined, - extends: extendsParent, + extends: allOfDetails.extends, is: isParent, type: schema.type, + spread: allOfDetails.spread, }); } @@ -125,23 +126,56 @@ export function transformComponentSchemas(context: Context, models: TypeSpecMode }); } - function getModelExtends(schema: OpenAPI3Schema, callingScope: string[]): string | undefined { - if (schema.type !== "object" || !schema.allOf) { - return; - } + interface AllOfDetails { + extends?: string; + properties: TypeSpecModelProperty[]; + spread: string[]; + } + function getAllOfDetails(schema: OpenAPI3Schema, callingScope: string[]): AllOfDetails { + const details: AllOfDetails = { + spread: [], + properties: [], + }; - if (schema.allOf.length !== 1) { - // TODO: Emit warning - can't extend more than 1 model - return; + if (!schema.allOf) { + return details; } - const parent = schema.allOf[0]; - if (!parent || !("$ref" in parent)) { - // TODO: Error getting parent - must be a reference, not expression - return; + let foundParentWithDiscriminator = false; + + for (const member of schema.allOf) { + // inline-schemas treated as normal objects with properties + if (!("$ref" in member)) { + details.properties.push(...getModelPropertiesFromObjectSchema(member)); + continue; + } + + const refSchema = context.getSchemaByRef(member.$ref); + + // Inheritance only supported if parent has a discriminator defined, otherwise prefer + // composition via spreading. + if (!refSchema?.discriminator) { + details.spread.push(context.getRefName(member.$ref, callingScope)); + continue; + } + + if (!foundParentWithDiscriminator) { + details.extends = context.getRefName(member.$ref, callingScope); + foundParentWithDiscriminator = true; + continue; + } + + // can only extend once, so if we have multiple potential parents, spread them all + // user will need to resolve TypeSpec errors (e.g. duplicate fields) manually + if (details.extends) { + details.spread.push(details.extends); + details.extends = undefined; + } + + details.spread.push(context.getRefName(member.$ref, callingScope)); } - return context.getRefName(parent.$ref, callingScope); + return details; } function getModelIs(schema: OpenAPI3Schema, callingScope: string[]): string | undefined { diff --git a/packages/openapi3/src/cli/actions/convert/utils/context.ts b/packages/openapi3/src/cli/actions/convert/utils/context.ts index 805e1466d6..14e8cec0e0 100644 --- a/packages/openapi3/src/cli/actions/convert/utils/context.ts +++ b/packages/openapi3/src/cli/actions/convert/utils/context.ts @@ -8,6 +8,7 @@ export interface Context { generateTypeFromRefableSchema(schema: Refable, callingScope: string[]): string; getRefName(ref: string, callingScope: string[]): string; + getSchemaByRef(ref: string): OpenAPI3Schema | undefined; } export function createContext(openApi3Doc: OpenAPI3Document): Context { @@ -23,6 +24,11 @@ export function createContext(openApi3Doc: OpenAPI3Document): Context { generateTypeFromRefableSchema(schema: Refable, callingScope: string[]) { return schemaExpressionGenerator.generateTypeFromRefableSchema(schema, callingScope); }, + getSchemaByRef(ref) { + const schemaName = ref.replace("#/components/schemas/", ""); + const schema = openApi3Doc.components?.schemas?.[schemaName]; + return schema; + }, }; return context; diff --git a/packages/openapi3/test/tsp-openapi3/data-types.test.ts b/packages/openapi3/test/tsp-openapi3/data-types.test.ts index 8bdfba4ca5..978a530f64 100644 --- a/packages/openapi3/test/tsp-openapi3/data-types.test.ts +++ b/packages/openapi3/test/tsp-openapi3/data-types.test.ts @@ -336,5 +336,192 @@ describe("converts top-level schemas", () => { expect(FooAlias?.sourceModels[0].usage).toBe("is"); expect(FooAlias?.sourceModels[0].model).toBe(Foo); }); + + it("allOf with parent discriminator", async () => { + const serviceNamespace = await tspForOpenAPI3({ + schemas: { + Pet: { + type: "object", + required: ["kind"], + properties: { + kind: { type: "string" }, + }, + discriminator: { propertyName: "kind" }, + }, + Cat: { + allOf: [ + { $ref: "#/components/schemas/Pet" }, + { + type: "object", + required: ["kind", "meow"], + properties: { kind: { type: "string", enum: ["cat"] }, meow: { type: "string" } }, + }, + ], + }, + }, + }); + + /* @discriminator("kind") model Pet { kind: string, } */ + const Pet = serviceNamespace.models.get("Pet"); + assert(Pet, "Pet model not found"); + + /* model Cat extends Pet { kind: "cat", meow: string, } */ + const Cat = serviceNamespace.models.get("Cat"); + assert(Cat, "Cat model not found"); + expect(Cat.baseModel).toBe(Pet); + expect(Cat.properties.size).toBe(2); + expect(Cat.properties.get("kind")).toMatchObject({ + optional: false, + type: { kind: "String", value: "cat" }, + }); + expect(Cat.properties.get("meow")).toMatchObject({ + optional: false, + type: { kind: "Scalar", name: "string" }, + }); + }); + + it("allOf without parent discriminator", async () => { + const serviceNamespace = await tspForOpenAPI3({ + schemas: { + Pet: { + type: "object", + required: ["name"], + properties: { + name: { type: "string" }, + }, + }, + Cat: { + allOf: [ + { $ref: "#/components/schemas/Pet" }, + { + type: "object", + required: ["kind", "meow"], + properties: { kind: { type: "string", enum: ["cat"] }, meow: { type: "string" } }, + }, + ], + }, + }, + }); + + /* model Pet { name: string, } */ + const Pet = serviceNamespace.models.get("Pet"); + assert(Pet, "Pet model not found"); + + /* model Cat { ...Pet, kind: "cat", meow: string, } */ + const Cat = serviceNamespace.models.get("Cat"); + assert(Cat, "Cat model not found"); + expect(Cat.baseModel).toBeUndefined(); + expect(Cat.properties.size).toBe(3); + expect(Cat.properties.get("name")).toMatchObject({ + optional: false, + type: { kind: "Scalar", name: "string" }, + }); + expect(Cat.properties.get("kind")).toMatchObject({ + optional: false, + type: { kind: "String", value: "cat" }, + }); + expect(Cat.properties.get("meow")).toMatchObject({ + optional: false, + type: { kind: "Scalar", name: "string" }, + }); + }); + + it("allOf with props", async () => { + const serviceNamespace = await tspForOpenAPI3({ + schemas: { + Pet: { + type: "object", + required: ["kind"], + properties: { + kind: { type: "string" }, + }, + discriminator: { propertyName: "kind" }, + }, + Cat: { + type: "object", + properties: { + paws: { type: "integer", format: "int8" }, + }, + allOf: [ + { $ref: "#/components/schemas/Pet" }, + { + type: "object", + required: ["kind", "meow"], + properties: { kind: { type: "string", enum: ["cat"] }, meow: { type: "string" } }, + }, + ], + }, + }, + }); + + /* @discriminator("kind") model Pet { kind: string, } */ + const Pet = serviceNamespace.models.get("Pet"); + assert(Pet, "Pet model not found"); + + /* model Cat extends Pet { kind: "cat", meow: string, paws?: int8 } */ + const Cat = serviceNamespace.models.get("Cat"); + assert(Cat, "Cat model not found"); + expect(Cat.baseModel).toBe(Pet); + expect(Cat.properties.size).toBe(3); + expect(Cat.properties.get("kind")).toMatchObject({ + optional: false, + type: { kind: "String", value: "cat" }, + }); + expect(Cat.properties.get("meow")).toMatchObject({ + optional: false, + type: { kind: "Scalar", name: "string" }, + }); + expect(Cat.properties.get("paws")).toMatchObject({ + optional: true, + type: { kind: "Scalar", name: "int8" }, + }); + }); + + it("allOf with multiple discriminators fallback to spread", async () => { + const serviceNamespace = await tspForOpenAPI3({ + schemas: { + Foo: { + type: "object", + required: ["kind"], + properties: { + kind: { type: "string" }, + }, + discriminator: { propertyName: "kind" }, + }, + Bar: { + type: "object", + required: ["type"], + properties: { + type: { type: "string" }, + }, + discriminator: { propertyName: "type" }, + }, + Thing: { + allOf: [{ $ref: "#/components/schemas/Foo" }, { $ref: "#/components/schemas/Bar" }], + }, + }, + }); + + /* @discriminator("kind") model Foo { kind: string, } */ + const Foo = serviceNamespace.models.get("Foo"); + assert(Foo, "Foo model not found"); + /* @discriminator("type") model Bar { type: string, } */ + const Bar = serviceNamespace.models.get("Bar"); + assert(Bar, "Bar model not found"); + + /* model Thing { ...Foo; ...Bar; } */ + const Thing = serviceNamespace.models.get("Thing"); + assert(Thing, "Thing model not found"); + expect(Thing.baseModel).toBeUndefined(); + expect(Thing.properties.size).toBe(2); + expect(Thing.properties.get("kind")).toMatchObject({ + optional: false, + type: { kind: "Scalar", name: "string" }, + }); + expect(Thing.properties.get("type")).toMatchObject({ + optional: false, + type: { kind: "Scalar", name: "string" }, + }); + }); }); }); diff --git a/packages/openapi3/test/tsp-openapi3/output/one-any-all/main.tsp b/packages/openapi3/test/tsp-openapi3/output/one-any-all/main.tsp index db4352b089..60aec09ddc 100644 --- a/packages/openapi3/test/tsp-openapi3/output/one-any-all/main.tsp +++ b/packages/openapi3/test/tsp-openapi3/output/one-any-all/main.tsp @@ -13,11 +13,13 @@ using OpenAPI; }) namespace OneAnyAllService; -model Cat extends Pet { +model Cat { + ...Pet; hunts: boolean; } -model Dog extends Pet { +model Dog { + ...Pet; bark: boolean; breed: "Husky" | "Corgi" | "Terrier"; }