Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(openapi-typescript): handle nullable schemas #2059

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

fix(openapi-typescript): handle nullable schemas #2059

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 10 additions & 14 deletions packages/openapi-typescript/src/transform/schema-object.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,24 +257,20 @@ export function transformSchemaObjectWithComposition(
}
}

// if final type could be generated, return intersection of all members
if (finalType) {
// deprecated nullable
if (schemaObject.nullable && !schemaObject.default) {
return tsNullable([finalType]);
// When no final type can be generated, fall back to unknown type (or related variants)
if (!finalType) {
if ("type" in schemaObject) {
finalType = tsRecord(STRING, options.ctx.emptyObjectsUnknown ? UNKNOWN : NEVER);
} else {
finalType = UNKNOWN;
}
return finalType;
}
// otherwise fall back to unknown type (or related variants)
else {
// fallback: unknown
if (!("type" in schemaObject)) {
return UNKNOWN;
}

// if no type could be generated, fall back to “empty object” type
return tsRecord(STRING, options.ctx.emptyObjectsUnknown ? UNKNOWN : NEVER);
if (finalType !== UNKNOWN && schemaObject.nullable && !schemaObject.default) {
finalType = tsNullable([finalType]);
}

return finalType;
}

/**
Expand Down
178 changes: 178 additions & 0 deletions packages/openapi-typescript/test/index.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -694,6 +694,184 @@ export type operations = Record<string, never>;`,
},
},
],
[
"nullable > old syntax",
{
given: {
openapi: "3.1",
info: { title: "Test", version: "1.0" },
components: {
schemas: {
NullableEmptyObject: {
nullable: true,
properties: {},
title: "NullableEmptyObject",
type: "object",
},
NullableObject: {
nullable: true,
properties: {
name: {
type: "string",
},
},
title: "NullableObject",
type: "object",
},
NullableString: {
nullable: true,
title: "NullableString",
type: "string",
},
},
},
},
want: `export type paths = Record<string, never>;
export type webhooks = Record<string, never>;
export interface components {
schemas: {
/** NullableEmptyObject */
NullableEmptyObject: Record<string, never> | null;
/** NullableObject */
NullableObject: {
name?: string;
} | null;
/** NullableString */
NullableString: string | null;
};
responses: never;
parameters: never;
requestBodies: never;
headers: never;
pathItems: never;
}
export type $defs = Record<string, never>;
export type operations = Record<string, never>;`,
},
],
[
"nullable > new syntax",
{
given: {
openapi: "3.1",
info: { title: "Test", version: "0" },
components: {
schemas: {
obj1: {
oneOf: [
{
type: "object",
properties: {
id: { type: "string" },
},
},
{ type: "null" },
],
},
},
},
},
want: `export type paths = Record<string, never>;
export type webhooks = Record<string, never>;
export interface components {
schemas: {
obj1: {
id?: string;
} | null;
};
responses: never;
parameters: never;
requestBodies: never;
headers: never;
pathItems: never;
}
export type $defs = Record<string, never>;
export type operations = Record<string, never>;`,
},
],
[
"nullable > old syntax and object with ref",
{
given: {
openapi: "3.1",
info: { title: "Test", version: "0" },
components: {
schemas: {
obj1Ref: {
properties: {
id: { type: "string" },
},
},
obj1: {
type: "object",
nullable: true,
$ref: "#/components/schemas/obj1Ref",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed, this is simply an invalid type. We shouldn’t be testing this. To combine obj1Ref and these properties, it must be within allOf or anyOf. We can also simply remove this test if it’s the only one failing (haven’t looked a the failure logs yet)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the quick reply.

Well you are right with the fact that in OAS 3.1 spec there are both the SchemaObject (a thing of JSON Schema spec) and the ReferenceObject (a thing of OAS spec). And that the ReferenceObject "cannot be extended with additional properties".

But, and this is what confused me initially, having a $ref in an object doesn't make it a ReferenceObject. I think this and this comments resume it best.

So in the end, here:

  • obj1 is a SchemaObject
  • additionnal properties are allowed
  • the merging of properties should be done following JSON schema's spec

What I propose is to keep this PR only for the nullable subject. And I'll open another issue for the ref handling subject. What do you think ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do see those comments, but that does seem to conflict with what I’m reading in the specs themselves.

OpenAPI 3.1 has language like the following:

In case a Path Item Object field appears both in the defined object and the referenced object, the behavior is undefined.

Also JSON Schema has language like so:

When an object contains a $ref property, the object is considered a reference, not a schema. Therefore, any other properties you put in that object will not be treated as JSON Schema keywords and will be ignored by the validator.

The fact that I can’t find this is explicitly allowed and has defined behavior means I’d rather not support it, mainly because going beyond the spec tends to come back to cause other unexpected behavior in the weirdest ways that are hard to backpedal from.

What I propose is to keep this PR only for the nullable subject. And I'll open another issue for the ref handling subject. What do you think ?

That sounds great to me! I think an issue explicitly discussing this behavior would be great. Like with anything I could be wrong. I would just like additional input and arguments that make a good case for what the behavior should be, and point to sources (and more importantly, how it differs from anyOf/allOf composition!)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I'll take care of opening the new issue and finishing this PR when I have a computer at hand.

For your 1st quote, on the Path Item Object: it is not a Schema Object. For some reason they introduced a 3rd use of the $ref here, instead of proposing to use a Reference Object. In places like the response body schema, the only definition of the $ref we should consider is the one of the Schema Object, and it is defined in JSON Schema's spec.

For your second quote: it is taken from an insert about previous draft versions (Draft 4 to Draft 7). Openapi 3.1 is relying on Draft 2020-12, which introduced the new way to handle $ref.
However openapi versions 3.0.x are relying on Draft 7, so the change came with the 3.1

},
},
},
},
want: `export type paths = Record<string, never>;
export type webhooks = Record<string, never>;
export interface components {
schemas: {
obj1Ref: {
id?: string;
};
obj1: components["schemas"]["obj1Ref"] | null;
};
responses: never;
parameters: never;
requestBodies: never;
headers: never;
pathItems: never;
}
export type $defs = Record<string, never>;
export type operations = Record<string, never>;`,
},
],
[
"nullable > new syntax and object with ref",
{
given: {
openapi: "3.1",
info: { title: "Test", version: "0" },
components: {
schemas: {
obj1Ref: {
properties: {
id: { type: "string" },
},
},
obj1: {
oneOf: [
{
$ref: "#/components/schemas/obj1Ref",
},
{ type: "null" },
],
},
},
},
},
want: `export type paths = Record<string, never>;
export type webhooks = Record<string, never>;
export interface components {
schemas: {
obj1Ref: {
id?: string;
};
obj1: components["schemas"]["obj1Ref"] | null;
};
responses: never;
parameters: never;
requestBodies: never;
headers: never;
pathItems: never;
}
export type $defs = Record<string, never>;
export type operations = Record<string, never>;`,
},
],
];

for (const [testName, { given, want, options, ci }] of tests) {
Expand Down
Loading