Fix the bug of nullable scalar generated as object #6240

wanlwanl · 2025-03-04T05:10:07Z

Fix #5156 by using anyOf instead of allOf for unions, ao we can set diffirent types in anyOf
When std types are already defined, use ref to the scalar only.

The root cause is type: object. Before this PR, we add type: object to scalars like Str | null, but it should be type: string. But models is already type: object, so it's not impacted.

playground

openapi: 3.0.0
info:
  title: Widget Service
  version: 0.0.0
tags: []
paths: {}
components:
  schemas:
    Prop:
      type: object # <------------- already an object
      required:
        - a
      properties:
        a:
          type: string
    Str:
      type: string
    Test:
      type: object
      required:
        - a
        - b
      properties:
        a:
          type: object # <-------------------- should be string
          allOf:
            - $ref: '#/components/schemas/Str'
          nullable: true
        b:
          type: object
          allOf:
            - $ref: '#/components/schemas/Prop'
          nullable: true

Copilot

PR Overview

This PR fixes the bug where a nullable scalar type is incorrectly generated as an object by replacing the use of allOf with anyOf for unions and refactoring the handling of scalar and model types.

Updated the test cases to expect a scalar (i.e. "string") for nullable scalars
Refactored the schema emitter to split the handling of model and scalar types and introduced a helper function to collect standard scalar names
Updated the changelog to document the fix

Reviewed Changes

File	Description
packages/openapi3/test/union-schema.test.ts	Updated expected schema for unions using nullable types
packages/openapi3/src/schema-emitter-3-0.ts	Refactored emitter logic for proper handling of scalar and model types and added a helper function for scalar info
.chronus/changes/wanl-pr-fix-allof-object-2025-2-4-12-52-38.md	Added changelog entry documenting the bug fix

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (2)

packages/openapi3/src/schema-emitter-3-0.ts:298

[nitpick] Consider moving the ScalarMember interface to a module-level definition instead of defining it within a function, which can improve readability and reusability.

interface ScalarMember {

packages/openapi3/src/schema-emitter-3-0.ts:303

[nitpick] The function name 'collectScalarInfo' could be more specific (e.g. 'collectStdScalarNames') to accurately convey that it returns a set of standard scalar names.

function collectScalarInfo(scalarMembers: ScalarMember[]): Set<string> {

azure-sdk · 2025-03-04T05:11:59Z

All changed packages have been documented.

✅ @typespec/openapi3

Show changes

`@typespec/openapi3` - fix ✏️

Fix union of custom scalar with null creating an object with allOf reference

azure-sdk · 2025-03-04T05:27:47Z

You can try these changes here

🛝 Playground	🌐 Website	🛝 VSCode Extension

packages/openapi3/src/schema-emitter-3-0.ts

.chronus/changes/wanl-pr-fix-allof-object-2025-2-4-12-52-38.md

packages/openapi3/src/schema-emitter-3-0.ts

…t' into wanl/pr/fix-allof-object

packages/openapi3/test/union-schema.test.ts

This reverts commit 40bde20.

This reverts commit 5beb68c.

This reverts commit 134041e.

packages/openapi3/test/union-schema.test.ts

packages/openapi3/src/schema-emitter-3-0.ts

albertxavier100 · 2025-03-07T12:06:41Z

packages/openapi3/src/schema-emitter-3-0.ts

+            const rootSchema = this.getSchemaForStdScalars(
+              currentRootScalar as unknown as Scalar & { name: IntrinsicScalarName },
+            );
+            return new ObjectBuilder({ ...rootSchema, ...additionalProps });


since we need to choose between

use {} as null value in order to add $ref for better reference

or add nullable and inline the std scalar schema
I implement the first option to safe time due to different time zone. Please feel free to leave comments if you want to change to other options.

i believe we agreed on 2. {} means anything. I think the best result for any consuming tool for scalar is to inline as they are most likely to do that anyway.

packages/openapi3/test/union-schema.test.ts

timotheeguerin · 2025-03-10T15:07:57Z

packages/openapi3/test/union-schema.test.ts

@@ -682,6 +681,82 @@ worksFor(["3.0.0"], ({ diagnoseOpenApiFor, oapiForModel, openApiFor }) => {
        message: "Cannot have a union containing only null types.",
      });
    });
+


so i am still confused by openapi3 nullable and allOf relation, does it also mean we technically have the same issue for models(are scalar not the only problematic ones)?

The root cause is type: object. Before this PR, we add type: object to scalars like Str | null, but it should be type: string. But models is already type: object, so it's not impacted.

playground

openapi: 3.0.0 info: title: Widget Service version: 0.0.0 tags: [] paths: {} components: schemas: Prop: type: object # <------------- already an object required: - a properties: a: type: string Str: type: string Test: type: object required: - a - b properties: a: type: object # <-------------------- should be string allOf: - $ref: '#/components/schemas/Str' nullable: true b: type: object allOf: - $ref: '#/components/schemas/Prop' nullable: true

also updated the description

yeah I get the original issue, but this has opened a can of worms.

How in openapi3 those 2 different?

schemas: str: type: string MyModel: type: object properties: {...} //--- why do we allow b but not a Other: a: type: string allOf: - $ref: '#/components/schemas/str' nullable: true b: type: object allOf: - $ref: '#/components/schemas/MyModel' nullable: true

Yeah, you are right, thanks for pointing out. I tested playground.

Looks like if we don't use {not: anyOf: [...allTypes]}, we have to duplicate model schema with nullable: true 😢

I feel like we can focus on scalar and create another pr to solve model issues

packages/openapi3/test/union-schema.test.ts

timotheeguerin · 2025-03-10T15:09:57Z

packages/openapi3/test/union-schema.test.ts

+          { type: "string", nullable: true, minimum: 1 },
+        ],
+      });
+    });


I think need @chrisradek also to do a final review on this as he's more knowledgable on the issue. He's OOF mon-wed.

Got it, thanks

…object

wanlwanl · 2025-03-11T14:59:36Z

packages/openapi3/src/schema-emitter-3-0.ts

+
+            const nullableProperty = nullable === true ? true : undefined;
+            const innerSchema = this.getSchemaForScalar(type);
+            // TODO: for the same constraints, use intersection of constraints from union wide and union member


if inline, we got the same issue: #6368

chrisradek · 2025-03-14T21:32:19Z

@timotheeguerin and I looked through some more tooling/stackoverflow posts and from what we can gather, it seems at least many tools support setting nullable: true alongside allOf.

So for this PR, we can simplify things a lot. We can keep the current allOf behavior and just update the type so that for scalar types, type matches the scalar instead of object.

So you could end up with:

scalar testString extends string;
model Test {
  field: testString | null;
}

generating:

Test:
  type: object
  required:
    - field
  properties:
    field:
      type: string
      allOf:
        - $ref: '#/components/schemas/testString'
      nullable: true

This won't work for all tooling, but matches the behavior we currently have for nullable objects. In the future if we're asked to support other outputs (e.g. inlined-schemas) we can support that via a flag.

wanlwanl and others added 2 commits March 4, 2025 12:51

Fix bug: scalar as object

b448c8e

Add changes

4baf602

wanlwanl requested review from bterlson, markcowl, allenjzhang, timotheeguerin, witemple-msft, chrisradek and AlitzelMendez as code owners March 4, 2025 05:10

wanlwanl requested a review from Copilot March 4, 2025 05:10

Copilot AI reviewed Mar 4, 2025

View reviewed changes

albertxavier100 and others added 2 commits March 4, 2025 15:24

remove unused

3756ad2

Merge branch 'main' into wanl/pr/fix-allof-object

fb1b447

timotheeguerin reviewed Mar 4, 2025

View reviewed changes

packages/openapi3/src/schema-emitter-3-0.ts Outdated Show resolved Hide resolved

timotheeguerin requested changes Mar 4, 2025

View reviewed changes

packages/openapi3/src/schema-emitter-3-0.ts Outdated Show resolved Hide resolved

timotheeguerin reviewed Mar 4, 2025

View reviewed changes

.chronus/changes/wanl-pr-fix-allof-object-2025-2-4-12-52-38.md Outdated Show resolved Hide resolved

timotheeguerin reviewed Mar 4, 2025

View reviewed changes

packages/openapi3/src/schema-emitter-3-0.ts Outdated Show resolved Hide resolved

chrisradek reviewed Mar 4, 2025

View reviewed changes

packages/openapi3/src/schema-emitter-3-0.ts Outdated Show resolved Hide resolved

albertxavier100 added 6 commits March 5, 2025 14:25

fix

1a8106e

Merge remote-tracking branch 'albertxavier100/wanl/pr/fix-allof-objec…

1b81754

…t' into wanl/pr/fix-allof-object

rename

5f05fa6

simplify

134041e

update tests

5beb68c

improve

40bde20

timotheeguerin reviewed Mar 5, 2025

View reviewed changes

packages/openapi3/test/union-schema.test.ts Outdated Show resolved Hide resolved

timotheeguerin reviewed Mar 5, 2025

View reviewed changes

packages/openapi3/test/union-schema.test.ts Outdated Show resolved Hide resolved

albertxavier100 added 2 commits March 6, 2025 09:59

Revert "improve"

683f76d

This reverts commit 40bde20.

Revert "update tests"

e6c6cc0

This reverts commit 5beb68c.

albertxavier100 and others added 3 commits March 6, 2025 10:00

Revert "simplify"

6dd0ca8

This reverts commit 134041e.

improve non extended scalar

fc30f1b

Merge branch 'main' into wanl/pr/fix-allof-object

c4077bd

markcowl assigned timotheeguerin and chrisradek Mar 6, 2025

timotheeguerin reviewed Mar 6, 2025

View reviewed changes

packages/openapi3/test/union-schema.test.ts Show resolved Hide resolved

timotheeguerin reviewed Mar 6, 2025

View reviewed changes

packages/openapi3/src/schema-emitter-3-0.ts Outdated Show resolved Hide resolved

albertxavier100 added 3 commits March 7, 2025 20:02

check by type

97e2023

WIP: when should I use $ref?

7e36725

option: inline the scalars

99dab14

albertxavier100 reviewed Mar 7, 2025

View reviewed changes

albertxavier100 and others added 2 commits March 8, 2025 05:29

add test: make sure keeping meta

bd859e0

Merge branch 'main' into wanl/pr/fix-allof-object

060bdb7

timotheeguerin reviewed Mar 10, 2025

View reviewed changes

packages/openapi3/test/union-schema.test.ts Outdated Show resolved Hide resolved

timotheeguerin reviewed Mar 10, 2025

View reviewed changes

packages/openapi3/test/union-schema.test.ts Outdated Show resolved Hide resolved

timotheeguerin reviewed Mar 10, 2025

View reviewed changes

albertxavier100 and others added 6 commits March 11, 2025 18:25

Merge remote-tracking branch 'microsoft/main' into wanl/pr/fix-allof-…

fb8eb5f

…object

remove

22289ff

fix metadata and constraints

a4c78f3

fix nullable prop

b04fb5b

cleanup

f5825cd

remove dup tests

3610c12

wanlwanl commented Mar 11, 2025

View reviewed changes

wanlwanl mentioned this pull request Mar 18, 2025

Fix union of custom scalar with null creating an object with allOf reference #6506

Merged

wanlwanl closed this Mar 19, 2025

Fix the bug of nullable scalar generated as object #6240

Fix the bug of nullable scalar generated as object #6240

Uh oh!

Conversation

wanlwanl commented Mar 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

PR Overview

Reviewed Changes

Uh oh!

azure-sdk commented Mar 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

@typespec/openapi3 - fix ✏️

Uh oh!

azure-sdk commented Mar 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisradek commented Mar 14, 2025

Uh oh!

Uh oh!

wanlwanl commented Mar 4, 2025 •

edited

Loading

azure-sdk commented Mar 4, 2025 •

edited

Loading

`@typespec/openapi3` - fix ✏️

azure-sdk commented Mar 4, 2025 •

edited

Loading