[typescript-fetch] oneOf models now consider primitives when converting

[DavidGrath]

PR checklist

• Read the contribution guidelines.
• Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
• Run the following to build the project and update samples:

  ./mvnw clean package || exit
  ./bin/generate-samples.sh ./bin/configs/*.yaml || exit
  ./bin/utils/export_docs_generators.sh || exit
  

  (For Windows users, please run the script in WSL)
  Commit all changed files.
  This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
  These must match the expectations made by your contribution.
  You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/configs/java*.
  IMPORTANT: Do NOT purge/delete any folders/files (e.g. tests) when regenerating the samples as manually written tests may be removed.
• File the PR against the correct branch: master (upcoming 7.x.0 minor release - breaking changes with fallbacks), 8.0.x (breaking changes without fallbacks)
• If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

The output looks like this:

import type { MyNumericValue } from './MyNumericValue';
import {
    instanceOfMyNumericValue,
    MyNumericValueFromJSON,
    MyNumericValueFromJSONTyped,
    MyNumericValueToJSON,
} from './MyNumericValue';

/**
 * @type MyCustomSpeed
 * A value that can be a number or a specific string.
 * @export
 */
export type MyCustomSpeed = Array<number> | Array<string> | Date | MyNumericValue | number | string;

export function MyCustomSpeedFromJSON(json: any): MyCustomSpeed {
    return MyCustomSpeedFromJSONTyped(json, false);
}

export function MyCustomSpeedFromJSONTyped(json: any, ignoreDiscriminator: boolean): MyCustomSpeed {
    if (json == null) {
        return json;
    }
    if (typeof json !== 'object') {
        return json;
    }
    if (instanceOfMyNumericValue(json)) {
        return MyNumericValueFromJSONTyped(json, true);
    }
    if(!(isNaN(new Date(json).getTime()))) {
        return json == null ? undefined : (new Date(json));
    }
    if(!(isNaN(new Date(json).getTime()))) {
        return json == null ? undefined : (new Date(json));
    }
    if(typeof json === 'string' && (json === 'fixed-value-a' || json === 'fixed-value-b' || json === 'fixed-value-c')) {
        return json;
    }
    if(typeof json === 'number' && (json === 10 || json === 20 || json === 30)) {
        return json;
    }
    if (Array.isArray(json)) {
        if (json.every(item => typeof item === 'number')) {
            return json;
        }
     }
    if (Array.isArray(json)) {
        if (json.every(item => typeof item === 'string')) {
            return json;
        }
     }
    return {} as any;
}

export function MyCustomSpeedToJSON(json: any): any {
    return MyCustomSpeedToJSONTyped(json, false);
}

export function MyCustomSpeedToJSONTyped(value?: MyCustomSpeed | null, ignoreDiscriminator: boolean = false): any {
    if (value == null) {
        return value;
    }
    if (typeof value !== 'object') {
        return value;
    }
    if (instanceOfMyNumericValue(value)) {
        return MyNumericValueToJSON(value as MyNumericValue);
    }
    if(value instanceof Date) {
        return ((value).toISOString().substring(0,10));
     }
    if(value instanceof Date) {
        return value == null ? undefined : ((value).toISOString());
    }
    if(typeof value === 'string' && (value === 'fixed-value-a' || value === 'fixed-value-b' || value === 'fixed-value-c')) {
        return value;
    }
    if(typeof value === 'number' && (value === 10 || value === 20 || value === 30)) {
        return value;
    }
    if (Array.isArray(value)) {
        if (value.every(item => typeof item === 'number') {
            return value;
        }
    }
    if (Array.isArray(value)) {
        if (value.every(item => typeof item === 'string') {
            return value;
        }
    }
    return {};
}

@joscha @macjohnny , please help review for TypeScript

[macjohnny]

@DavidGrath can you resolve the merge conflict?

[DavidGrath]

@macjohnny, yes, please give me till early Saturday or sooner

[DavidGrath]

@macjohnny, resolved

[DavidGrath]

@macjohnny thank you

[macjohnny]

@DavidGrath thank you for the fix!

[btpnlsl]

@DavidGrath there was one other thing I noticed about the serialization / deserialization code which I thought I would check with you about. Does it make sense to distinguish between isDateType & isDateTimeType in a oneOf template when both data types are being coerced into Date?

In the sample OAS you provided for the bug two of the oneOf schemas are date/date-time related:

        - type: string
          format: date
        - type: string
          format: date-time

Which results in this code in the deserialization method (which is fine but the second if will never be true)

    // is getTime supposed to distinguish between `isDateType` & `isDateTimeType`?
    if (!(isNaN(new Date(json).getTime()))) {
        return json == null ? undefined : (new Date(json));
    }
    if (!(isNaN(new Date(json).getTime()))) {
        return json == null ? undefined : (new Date(json));
    }

But the serialization method might cause an issue:

    if (value instanceof Date) {
        return ((value).toISOString().substring(0,10));
    }
    if (value instanceof Date) {
        return value == null ? undefined : ((value).toISOString());
    }

Since the isDateType check will always come first, the serialization code will always truncate values to remove the time portion, even if initial representation came from a DateTime.

If we reverse the checks for isDateType & isDateTimeType in the template I think the method will default to serialize with the more precise format if both isDateType & isDateTimeType are present, but use the truncated format if only isDateTime is present.

Does that seem reasonable to you?

[DavidGrath]

@btpnlsl , sorry for the delayed response. You're right. I mainly copied and modified the date and time logic from somewhere else and so I didn't think too deeply about the need to distinguish between the two or the need for extra precision. I think if I make another PR for a separate issue, I can include this improvement as well

[WIStudent]

I am currently looking into the modelOneOf.mustache because of #21878 and I am having trouble undestanding the purpose of this part:

    if(typeof json === 'string' && (json === 'fixed-value-a' || json === 'fixed-value-b' || json === 'fixed-value-c')) {
        return json;
    }
    if(typeof json === 'number' && (json === 10 || json === 20 || json === 30)) {
        return json;
    }
    if (Array.isArray(json)) {
        if (json.every(item => typeof item === 'number')) {
            return json;
        }
     }
    if (Array.isArray(json)) {
        if (json.every(item => typeof item === 'string')) {
            return json;
        }
     }
    return {} as any;

It isn't transforming the json, but if the conditions aren't met, it is returning {} which is also wrong if I am expecting Array<number> | Array<string> | number | string. If the purpose is to do some validation, shouldn't it throw an Error instead if no condition is met? If not, can't we just return json?

[btpnlsl]

@WIStudent It's validation code that is attempting to ensure that the json matches one of the possible types that compose the oneOf schema.

Looking at the code snippet I would assume that the schema for that type looks something like

oneOf:
    - type: string
      enum:
        - "fixed-value-a"
        - "fixed-value-b"
        - "fixed-value-c"
    - type: integer
      format: int64
      enum: [10, 20, 30]
   - type: array
      items:
        type: number
   - type: array
      items:
        type: string

And the code that you referenced is attempting to validate the json.

One thing I have noticed is that the Open API parser / generators may not be doing a great job at either failing to parse a spec with an ambiguous oneOf schema. For instance:

oneOf:
    - type: number
    - type: integer
      enum: [10, 20, 30]

How should this be evaluated? Is it just that any number is valid? Should this just evaluate to the most primitive possible representation for the language that is being generated? Is it possible by doing so that there may be some edge cases where the contractual representation of the same oneOf schema is different for different languages?

The OAS spec has very little to say about how oneOf works outside of the use with a discriminator. The JSON schema validation IETF spec is a bit more verbose saying:

An instance validates successfully against this keyword if it
validates successfully against exactly one schema defined by this
keyword's value.

Which implies that having a oneOf schema that consists of a number and a enum represented as a number is invalid because it's impossible to validate against exactly one schema. However I don't believe that the OAS parser is failing this validation.

Then there is the question about what the generator should do if the incoming Json does not match against any of the possible oneOf schemas. The current typescript-fetch generator his it returning an empty object. I think that there is a convincing argument for throwing an Error, especially since other clients like the C-Sharp and Go clients are throwing if they fail to validate the Json against a possible Schema.

[WIStudent]

@btpnlsl I took a different approach and opened a pull request #21882 that only keeps the conditions where the value gets transformed, and otherwise returns the original value.