Fix the bug of nullable scalar generated as object #6240
Conversation
wanlwanl
requested review from
bterlson,
markcowl,
allenjzhang,
timotheeguerin,
witemple-msft,
chrisradek and
AlitzelMendez
as code owners
There was a problem hiding this comment.
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> {
|
All changed packages have been documented.
Show changes
|
|
You can try these changes here
|
| @@ -619,8 +619,7 @@ worksFor(["3.0.0"], ({ diagnoseOpenApiFor, oapiForModel, openApiFor }) => { | |||
| x: { | |||
| anyOf: [ | |||
| { | |||
| type: "object", | |||
| allOf: [{ $ref: "#/components/schemas/MyStr" }], | |||
| type: "string", | |||
There was a problem hiding this comment.
@chrisradek what do you think here? I am not sure we want to loose the ref, I think this should produce
type: string
nullable: true
allOf: [{ $ref: "#/components/schemas/MyStr" }]
There was a problem hiding this comment.
I think technically, unless MyStr also has nullable: true on it, this wouldn't work according to the spec. nullable only applies to its own schema, not subschemas.
This is one of those things that wasn't stated in the original 3.0.0 spec version, but was clarified later (at least in 3.0.4). I think prior to the clarification some tooling did actually support what you're proposing too.
Could try some tools/validators to see how they actually treat this, otherwise might need to copy the ref and add nullable to it :-/
| const stdScalar = $.scalar.getStdBase(type); | ||
| if (stdScalar?.name) { | ||
| // already has same std scalar member, use $ref | ||
| if (stdScalarNames.has(stdScalar.name) && "$ref" in schema) { |
There was a problem hiding this comment.
can you not use identity instead of name matching, what if someone make a new scalar with the same name
There was a problem hiding this comment.
stdScalarNames and stdScalar are string, int32, etc. -- the typespec builtin types. it can't be in made with the same name?
There was a problem hiding this comment.
Yes it can https://typespec.io/playground/?c=c2NhbGFyIHN0cmluZzs%3D&e=%40typespec%2Fopenapi3&options=%7B%7D
You have to get the scalar reference in the compiler and check they match or at least if you use string check the parent namespace is TypeSpec
| const rootSchema = this.getSchemaForStdScalars( | ||
| currentRootScalar as unknown as Scalar & { name: IntrinsicScalarName }, | ||
| ); | ||
| return new ObjectBuilder({ ...rootSchema, ...additionalProps }); |
There was a problem hiding this comment.
since we need to choose between
- use
{}as null value in order to add$reffor better reference - or add
nullableand 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.
Fix #5156 by using
anyOfinstead ofallOffor unions, ao we can set diffirent types inanyOfWhen std types are already defined, use ref to the scalar only.