OpenAPI3: Fix more issues with inline properties (microsoft#2884)

fix microsoft#2882 Fix more issues when properties that should be inline are wrapped in `allOf` --------- Co-authored-by: Mark Cowlishaw <[email protected]>
weshaggard · Feb 6, 2024 · 9726b3d · 9726b3d
1 parent 8ed1d82
commit 9726b3d
Show file tree

Hide file tree

Showing 8 changed files with 175 additions and 62 deletions.
diff --git a/.changeset/fix-openapi3-more-circular-inline-issues-2024-1-5-16-53-4.md b/.changeset/fix-openapi3-more-circular-inline-issues-2024-1-5-16-53-4.md
@@ -0,0 +1,5 @@
+---
+"@typespec/compiler": minor
+---
+
+Emitter framework: `ObjectBuilder` will keep track when built using a `Placeholder` allowing data to be carried over when chaining `ObjectBuilder`
diff --git a/.changeset/fix-openapi3-more-circular-inline-issues-2024-1-5-16-55-16.md b/.changeset/fix-openapi3-more-circular-inline-issues-2024-1-5-16-55-16.md
@@ -0,0 +1,5 @@
+---
+"@typespec/openapi3": patch
+---
+
+Fix issues with `nullable` properties used in a cycle being wrapped in `allOf` when not needed
diff --git a/packages/compiler/package.json b/packages/compiler/package.json
@@ -67,6 +67,7 @@
     "dogfood": "node scripts/dogfood.js",
     "test": "vitest run",
     "test:ui": "vitest --ui",
+    "test:watch": "vitest -w",
     "test-official": "vitest run --coverage --reporter=junit --reporter=default --no-file-parallelism",
     "e2e:disabled": "vitest run --config ./vitest.config.e2e.ts",
     "gen-manifest": "node scripts/generate-manifest.js",

diff --git a/packages/compiler/src/emitter-framework/builders/object-builder.ts b/packages/compiler/src/emitter-framework/builders/object-builder.ts
@@ -2,22 +2,41 @@ import { compilerAssert } from "../../core/index.js";
 import { Placeholder } from "../placeholder.js";
 import { EmitEntity, EmitterResult } from "../types.js";
 
+const placeholderSym = Symbol("placeholder");
 // eslint is confused by merging generic interface and classes
 // eslint-disable-next-line @typescript-eslint/no-unused-vars
 export interface ObjectBuilder<T> extends Record<string, any> {}
 // eslint-disable-next-line @typescript-eslint/no-unsafe-declaration-merging
 export class ObjectBuilder<T> {
-  constructor(initializer: Record<string, unknown> | Placeholder<Record<string, unknown>> = {}) {
-    if (initializer instanceof Placeholder) {
-      initializer.onValue((v) => {
-        for (const [key, value] of Object.entries(v)) {
-          this.set(key, value as any);
-        }
-      });
-    } else {
-      for (const [key, value] of Object.entries(initializer)) {
+  private readonly [placeholderSym]: Placeholder<any> | undefined;
+
+  constructor(
+    initializer:
+      | Record<string, unknown>
+      | Placeholder<Record<string, unknown>>
+      | ObjectBuilder<T> = {}
+  ) {
+    const copyProperties = (source: Record<string, unknown>) => {
+      for (const [key, value] of Object.entries(source)) {
         this.set(key, value as any);
       }
+    };
+    const registerPlaceholder = (placeholder: Placeholder<Record<string, unknown>>) => {
+      placeholder.onValue(copyProperties);
+    };
+
+    if (initializer instanceof ObjectBuilder) {
+      if (initializer[placeholderSym]) {
+        this[placeholderSym] = initializer[placeholderSym];
+        registerPlaceholder(initializer[placeholderSym]);
+      }
+      copyProperties(initializer);
+    } else if (initializer instanceof Placeholder) {
+      this[placeholderSym] = initializer;
+
+      registerPlaceholder(initializer);
+    } else {
+      copyProperties(initializer);
     }
   }
 

diff --git a/packages/compiler/test/emitter-framework/object-builder.test.ts b/packages/compiler/test/emitter-framework/object-builder.test.ts
@@ -0,0 +1,48 @@
+import { beforeEach, describe, expect, it } from "vitest";
+import { ObjectBuilder } from "../../src/emitter-framework/builders/object-builder.js";
+import { Placeholder } from "../../src/emitter-framework/placeholder.js";
+
+describe("ObjectBuilder", () => {
+  it("create simple builder", () => {
+    const builder = new ObjectBuilder({
+      foo: "bar",
+    });
+    expect(builder.foo).toBe("bar");
+  });
+
+  it("create adds elements to builder", () => {
+    const builder = new ObjectBuilder({
+      foo: "bar",
+    });
+    builder.set("bar", "baz");
+    expect(builder.foo).toBe("bar");
+    expect(builder.bar).toBe("baz");
+  });
+
+  describe("when working with placeholders", () => {
+    let placeholder: Placeholder<any>;
+
+    beforeEach(() => {
+      placeholder = new Placeholder();
+    });
+
+    it("has no values to start with", () => {
+      const builder = new ObjectBuilder(placeholder);
+      expect(builder.foo).toBeUndefined();
+    });
+
+    it("resolve values from placeholder", () => {
+      const builder = new ObjectBuilder(placeholder);
+      expect(builder.foo).toBeUndefined();
+      placeholder.setValue({ foo: "bar" });
+      expect(builder.foo).toBe("bar");
+    });
+
+    it("create object builder from another object builder with a placeholder", () => {
+      const builder1 = new ObjectBuilder(placeholder);
+      const builder2 = new ObjectBuilder(builder1);
+      placeholder.setValue({ foo: "bar" });
+      expect(builder2.foo).toBe("bar");
+    });
+  });
+});
diff --git a/packages/openapi3/src/schema-emitter.ts b/packages/openapi3/src/schema-emitter.ts
@@ -335,13 +335,15 @@ export class OpenAPI3SchemaEmitter extends TypeEmitter<
     const refSchema = this.emitter.emitTypeReference(prop.type, {
       referenceContext: isMultipart ? { contentType: "application/json" } : {},
     });
+
     if (refSchema.kind !== "code") {
       throw new Error("Unexpected non-code result from emit reference");
     }
 
     const isRef = refSchema.value instanceof Placeholder || "$ref" in refSchema.value;
 
     const schema = this.#applyEncoding(prop, refSchema.value as any);
+
     // Apply decorators on the property to the type's schema
     const additionalProps: Partial<OpenAPI3Schema> = this.#applyConstraints(prop, {});
     if (prop.default) {
@@ -370,10 +372,11 @@ export class OpenAPI3SchemaEmitter extends TypeEmitter<
         delete schema.anyOf;
       }
 
-      const merged = ensureObjectBuilder(schema);
+      const merged = new ObjectBuilder(schema);
       for (const [key, value] of Object.entries(additionalProps)) {
         merged.set(key, value);
       }
+
       return merged;
     }
   }
@@ -510,7 +513,13 @@ export class OpenAPI3SchemaEmitter extends TypeEmitter<
           // but we can't make a ref "nullable", so wrap in an allOf (for models)
           // or oneOf (for all other types)
           if (type && type.kind === "Model") {
-            return B.object({ type: "object", allOf: B.array([schema]), nullable: true });
+            if (shouldInline(program, type)) {
+              const merged = new ObjectBuilder(schema);
+              merged.set("nullable", true);
+              return merged;
+            } else {
+              return B.object({ type: "object", allOf: B.array([schema]), nullable: true });
+            }
           } else {
             return B.object({ oneOf: B.array([schema]), nullable: true });
           }
@@ -820,7 +829,7 @@ export class OpenAPI3SchemaEmitter extends TypeEmitter<
       });
     }
 
-    return new ObjectBuilder(schema);
+    return new ObjectBuilder<OpenAPI3Schema>(schema);
   }
 
   #inlineType(type: Type, schema: ObjectBuilder<any>) {
@@ -886,7 +895,8 @@ export class OpenAPI3SchemaEmitter extends TypeEmitter<
       );
       return newTarget;
     }
-    return new ObjectBuilder(target);
+    const result = new ObjectBuilder(target);
+    return result;
   }
   #mergeFormatAndEncoding(
     format: string | undefined,
@@ -947,16 +957,6 @@ function includeDerivedModel(model: Model): boolean {
   );
 }
 
-function ensureObjectBuilder<
-  T extends Record<string, unknown> | Placeholder<Record<string, unknown>> | undefined,
->(type: T | ObjectBuilder<T>): ObjectBuilder<T> {
-  if (type instanceof ObjectBuilder) {
-    return type;
-  } else {
-    return new ObjectBuilder(type);
-  }
-}
-
 const B = {
   array: <T>(items: T[]): ArrayBuilder<T> => {
     const builder = new ArrayBuilder<T>();

diff --git a/packages/openapi3/test/nullable-properties.test.ts b/packages/openapi3/test/nullable-properties.test.ts
@@ -0,0 +1,74 @@
+import { deepStrictEqual, ok } from "assert";
+import { describe, expect, it } from "vitest";
+import { OpenAPI3SchemaProperty } from "../src/types.js";
+import { oapiForModel, openApiFor } from "./test-host.js";
+
+describe("openapi3: nullable properties", () => {
+  it("makes nullable schema when union with null", async () => {
+    const res = await openApiFor(
+      `
+        model Thing {
+          id: string;
+          properties: Thing | null;
+        }
+        op doStuff(): Thing;
+        `
+    );
+    deepStrictEqual(res.components.schemas.Thing.properties.properties, {
+      type: "object",
+      allOf: [{ $ref: "#/components/schemas/Thing" }],
+      nullable: true,
+    });
+  });
+
+  it("handles a nullable enum", async () => {
+    const res = await oapiForModel(
+      "X",
+      `
+        enum A {
+          a: 1
+        }
+        
+        model X {
+          prop: A | null
+        }
+        `
+    );
+    deepStrictEqual(res.schemas.X.properties.prop.oneOf, [
+      {
+        $ref: "#/components/schemas/A",
+      },
+    ]);
+    ok(res.schemas.X.properties.prop.nullable);
+  });
+
+  describe("when used in circular references", () => {
+    async function expectInCircularReference(ref: string, value: OpenAPI3SchemaProperty) {
+      const res = await openApiFor(
+        `
+        model Test {
+          children: ${ref} | null;
+        }
+        
+        op test(filters: ${ref}): {}[];
+        `
+      );
+      expect(res.components.schemas.Test.properties.children).toEqual(value);
+    }
+    it("keep nullable array inline", async () => {
+      await expectInCircularReference("Test[]", {
+        type: "array",
+        items: { $ref: "#/components/schemas/Test" },
+        nullable: true,
+      });
+    });
+
+    it("keep nullable Record<T> inline", async () => {
+      await expectInCircularReference("Record<Test>", {
+        type: "object",
+        additionalProperties: { $ref: "#/components/schemas/Test" },
+        nullable: true,
+      });
+    });
+  });
+});
diff --git a/packages/openapi3/test/union-schema.test.ts b/packages/openapi3/test/union-schema.test.ts
@@ -4,23 +4,6 @@ import { describe, it } from "vitest";
 import { diagnoseOpenApiFor, oapiForModel, openApiFor } from "./test-host.js";
 
 describe("openapi3: union type", () => {
-  it("makes nullable schema when union with null", async () => {
-    const res = await openApiFor(
-      `
-      model Thing {
-        id: string;
-        properties: Thing | null;
-      }
-      op doStuff(): Thing;
-      `
-    );
-    deepStrictEqual(res.components.schemas.Thing.properties.properties, {
-      type: "object",
-      allOf: [{ $ref: "#/components/schemas/Thing" }],
-      nullable: true,
-    });
-  });
-
   it("handles discriminated unions", async () => {
     const res = await openApiFor(
       `
@@ -277,28 +260,6 @@ describe("openapi3: union type", () => {
     ]);
   });
 
-  it("handles a nullable enum", async () => {
-    const res = await oapiForModel(
-      "X",
-      `
-      enum A {
-        a: 1
-      }
-      
-      model X {
-        prop: A | null
-      }
-      `
-    );
-    ok(res.isRef);
-    deepStrictEqual(res.schemas.X.properties.prop.oneOf, [
-      {
-        $ref: "#/components/schemas/A",
-      },
-    ]);
-    ok(res.schemas.X.properties.prop.nullable);
-  });
-
   it("handles discriminated union mapping with multiple visibilities", async () => {
     const res = await openApiFor(`
       model A {