tsp-openapi3 - improve model generation of schemas using allOf (micro…

…soft#4232) Fixes microsoft#4152 Depends on microsoft#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 <Christopher.Radek@microsoft.com>
weidongxu-microsoft · Sep 3, 2024 · 862814e · 862814e
1 parent b0b6a0a
commit 862814e
Show file tree

Hide file tree

Showing 7 changed files with 260 additions and 23 deletions.
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
@@ -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.
diff --git 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
@@ -65,6 +65,8 @@ export interface TypeSpecModel extends TypeSpecDeclaration {
    * Defaults to 'object'
    */
   type?: OpenAPI3Schema["type"];
+
+  spread?: string[];
 }
 
 export interface TypeSpecAlias extends Pick<TypeSpecDeclaration, "name" | "doc" | "scope"> {

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
@@ -86,20 +86,21 @@ 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",
       name,
       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
@@ -8,6 +8,7 @@ export interface Context {
 
   generateTypeFromRefableSchema(schema: Refable<OpenAPI3Schema>, 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<OpenAPI3Schema>, 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
@@ -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
@@ -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";
 }