[BUG][typescript-fetch]: Do not remove primitive values when using oneOf #21882
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -42,12 +42,7 @@ export function {{classname}}FromJSONTyped(json: any, ignoreDiscriminator: boole | |
| {{/discriminator}} | ||
| {{^discriminator}} | ||
| {{#oneOfModels}} | ||
| if (typeof json === 'object' && instanceOf{{{.}}}(json)) { | ||
| return {{{.}}}FromJSONTyped(json, true); | ||
| } | ||
| {{/oneOfModels}} | ||
|
|
@@ -61,7 +56,6 @@ export function {{classname}}FromJSONTyped(json: any, ignoreDiscriminator: boole | |
| } | ||
| {{#-last}} | ||
| } | ||
| } | ||
| {{/-last}} | ||
| {{/oneOfArrays}} | ||
|
|
@@ -70,60 +64,36 @@ export function {{classname}}FromJSONTyped(json: any, ignoreDiscriminator: boole | |
| {{#items}} | ||
| {{#isDateType}} | ||
| if (Array.isArray(json)) { | ||
|
There was a problem hiding this comment. This looks like it fixes a bug in how arrays of dates or date-times |
||
| if (json.every(item => !(isNaN(new Date(item).getTime())))) { | ||
| return json.map(item => new Date(item)); | ||
| } | ||
| } | ||
| {{/isDateType}} | ||
| {{#isDateTimeType}} | ||
| if (Array.isArray(json)) { | ||
| if (json.every(item => !(isNaN(new Date(item).getTime())))) { | ||
| return json.map(item => new Date(item)); | ||
| } | ||
| } | ||
| {{/isDateTimeType}} | ||
|
There was a problem hiding this comment. These conditions didn't transform the There was a problem hiding this comment. I'm not sure if I follow why we wouldn't want to verify if the schema has a consistent array of primitive values (either numbers or enums represented as numbers). This code is doing validation in the case where the The Swagger OpenAPI documentation provides the following guidance:
It also provides an example where the JSON is invalid for the I think there is some ambiguity in these specs about what should be done if a user defines a But I don't think it's a good idea to get rid of the validation logic here. I think its arguable about what should be done if the JSON fails validation though. Throwing an Error is consistent with the client generators for other languages. There was a problem hiding this comment. Validation is nice to have, but the old implementation returns I also believe that this "return value as soon as one condition is met" approach is not suitable for TestPayload:
oneOf:
- type: object
properties:
foo:
type: string
required:
- foo
- type: object
properties:
bar:
type: number
required:
- barexport function TestPayloadFromJSONTyped(json: any, ignoreDiscriminator: boolean): TestPayload {
if (json == null) {
return json;
}
if (typeof json !== 'object') {
return json;
}
if (instanceOfTestPayloadOneOf(json)) {
return TestPayloadOneOfFromJSONTyped(json, true);
}
if (instanceOfTestPayloadOneOf1(json)) {
return TestPayloadOneOf1FromJSONTyped(json, true);
}
return {} as any;
}
export function instanceOfTestPayloadOneOf(value: object): value is TestPayloadOneOf {
if (!('foo' in value) || value['foo'] === undefined) return false;
return true;
}
export function TestPayloadOneOfFromJSONTyped(json: any, ignoreDiscriminator: boolean): TestPayloadOneOf {
if (json == null) {
return json;
}
return {
'foo': json['foo'],
};
}
export function instanceOfTestPayloadOneOf1(value: object): value is TestPayloadOneOf1 {
if (!('bar' in value) || value['bar'] === undefined) return false;
return true;
}
export function TestPayloadOneOf1FromJSONTyped(json: any, ignoreDiscriminator: boolean): TestPayloadOneOf1 {
if (json == null) {
return json;
}
return {
'bar': json['bar'],
};
}According to the openapi spec for The validation is also incomplete, I get the impression that the original intent of the My main concern with this PR is to keep valid values unchanged. I can work with a client that does not validate request/response payloads, but I cannot work with a client that changes valid request/response payloads. To me the current validation implementation seems flawed, it is both rejecting valid values and accepting invalid ones. A working validation implementation requires a lot more work. I took a brief look at the other typescript client generators and could not find a validation implementation there either. Until we got a validation implementation that covers all possible cases we should not change values that could be valid. There was a problem hiding this comment. I can defiantly see that the generated code is not correctly implementing It also makes sense to me that the generator shouldn't attempt to support the generic What I am concerned about is that the existing code has been added to address issues which prior authors encountered that may (or may not) be regressed by removing the code. Sadly unit test coverage for many changes is not the best. I think it is worth while to ensure that this change will not regress any scenarios that are unrelated to The original PR which added a The PRs which added validation code to use the FWIF there was a change in PR #20983 to make the The bulk of the non-model validation code for the There was a problem hiding this comment.
Imagine you use an OpenAPI generated client; the API changes (eg a new oneOf discriminator is added) and suddenly all existing clients throw exceptions because they can’t handle the new addition. It essentially means you can never update the API (it‘s not even breaking just adding a new type) without first forcing everyone to update to the new client. Always side with gracefulness There was a problem hiding this comment. @aeneasr I believe that adding a new type to a There was a problem hiding this comment.
One thing that surprised me while looking into this issue is that apparently every typescript client generator has its own logic for model generation. I was already using the oneOf:
- type: object
- type: array
items:
nullable: truefor a long time without issues, but wanted to switch to But I just noticed that there is a There was a problem hiding this comment. Setting
To me, this is another sign that value mapping and value validation should be separated. |
||
| {{/items}} | ||
| {{/isArray}} | ||
| {{^isArray}} | ||
| {{#isDateType}} | ||
| if (!(isNaN(new Date(json).getTime()))) { | ||
|
There was a problem hiding this comment. These null-checks are unnecessary because the function already returns in the beginning when |
||
| return new Date(json); | ||
| } | ||
| {{/isDateType}} | ||
| {{^isDateType}} | ||
| {{#isDateTimeType}} | ||
| if (!(isNaN(new Date(json).getTime()))) { | ||
| return new Date(json); | ||
| } | ||
| {{/isDateTimeType}} | ||
| {{/isDateType}} | ||
| {{/isArray}} | ||
| {{/oneOfPrimitives}} | ||
| return json; | ||
| {{/discriminator}} | ||
| } | ||
|
|
||
|
|
@@ -147,12 +117,7 @@ export function {{classname}}ToJSONTyped(value?: {{classname}} | null, ignoreDis | |
| {{/discriminator}} | ||
| {{^discriminator}} | ||
| {{#oneOfModels}} | ||
| if (typeof value === 'object' && instanceOf{{{.}}}(value)) { | ||
| return {{{.}}}ToJSON(value as {{{.}}}); | ||
| } | ||
| {{/oneOfModels}} | ||
|
|
@@ -166,7 +131,6 @@ export function {{classname}}ToJSONTyped(value?: {{classname}} | null, ignoreDis | |
| } | ||
| {{#-last}} | ||
| } | ||
| } | ||
| {{/-last}} | ||
| {{/oneOfArrays}} | ||
|
|
@@ -175,57 +139,33 @@ export function {{classname}}ToJSONTyped(value?: {{classname}} | null, ignoreDis | |
| {{#items}} | ||
| {{#isDateType}} | ||
| if (Array.isArray(value)) { | ||
| if (value.every(item => item instanceof Date)) { | ||
| return value.map(value => value.toISOString().substring(0,10)); | ||
| } | ||
| } | ||
| {{/isDateType}} | ||
| {{#isDateTimeType}} | ||
| if (Array.isArray(value)) { | ||
| if (value.every(item => item instanceof Date)) { | ||
| return value.map(value => value.toISOString()); | ||
| } | ||
| } | ||
| {{/isDateTimeType}} | ||
| {{/items}} | ||
| {{/isArray}} | ||
| {{^isArray}} | ||
| {{#isDateType}} | ||
| if (value instanceof Date) { | ||
| return value.toISOString().substring(0,10); | ||
| } | ||
| {{/isDateType}} | ||
| {{#isDateTimeType}} | ||
| if (value instanceof Date) { | ||
| return value.toISOString(); | ||
| } | ||
| {{/isDateTimeType}} | ||
| {{/isArray}} | ||
| {{/oneOfPrimitives}} | ||
| return value; | ||
| {{/discriminator}} | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do not return early if
jsonis not anobjectbecause there might be more conditions down below that operate on values that are not anobjectThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like a good change to make sure that the verification doesn't exit prematurely if the schema consists of a model and a primitive.