TypeScript - anyOf/oneOf arrays do not emit the correct serialization and deserialization information

[baywet]

Follow up of #5348 which revealed multiple bugs in TypeScript composed types generation

Repro

openapi: 3.0.3
info:
  title: Example
  description: Example
  version: 1.0.0
servers:
  - url: https://example.com/api
paths:
  '/example1':
    post:
      summary: Test error generation.
      description: "Test generating error with message property."
      operationId: postErrorGeneration
      responses:
        '200':
          description: Success
          content:
            application/json:
              schema:
                $ref: '#/components/schemas/Success'
components:
  schemas:
    Success:
      type: object
      oneOf:
        - type: string
        - type: array
          items:
            type: string

(or change the oneOf by anyOf)

Will emit the following

For the serialization code

export function serializeItemPostRequestBody_tax_rates(writer: SerializationWriter, itemPostRequestBody_tax_rates: Partial<string> | undefined | null = {}) : void {
    if (itemPostRequestBody_tax_rates === undefined || itemPostRequestBody_tax_rates === null) return;
    switch (typeof itemPostRequestBody_tax_rates) {
        case "string":
            writer.writeCollectionOfPrimitiveValues<string>(key, itemPostRequestBody_tax_rates);
            break;
    }
}

Multiple things are wrong here:

1. the parameter declaration should contain string[] in addition to what it contains
2. the switch should have TWO entries
3. the first entry should call writeStringValue
4. what is this key symbol that does not exist?
5. a second entry should check for object + Array.isArray (and call collectionOfPrimitiveValues)

for the deserialization code

export function deserializeIntoItemPostRequestBody_tax_rates(itemPostRequestBody_tax_rates: Partial<string | string> | undefined = {}) : Record<string, (node: ParseNode) => void> {
    return {
    }
}

Multiple things are wrong here:

1. one parameter union type value is missing the array symbol
2. the returned body is empty, is that expected?
3. the parameter type does not match for the factory (see below0

for the factory

/**
 * Creates a new instance of the appropriate class based on discriminator value
 * @param parseNode The parse node to use to read the discriminator value and create the object
 * @returns {string[] | string}
 */
// @ts-ignore
export function createItemPostRequestBody_tax_ratesFromDiscriminatorValue(parseNode: ParseNode | undefined) : ((instance?: Parsable) => Record<string, (node: ParseNode) => void>) {
    return deserializeIntoItemPostRequestBody_tax_rates;
}

Multiple things are wrong again here:

1. no triage for deserialization (should happen at some point)
2. returned value doesn't match parameter type (see above)
3. the documented return type does not match the actual function return type.

[rkodev]

@baywet my thoughts in this issue is to reintroduce the wrapper class for the composed types. I don't think there is any other way to deserialize the types without atleast some changes in the kiota-typescript in order to support deserialization of composed types

[rkodev]

Here is an example for the changes

/* tslint:disable */
/* eslint-disable */
// Generated by Microsoft Kiota
// @ts-ignore
import { type BaseRequestBuilder, type Parsable, type ParsableFactory, type ParseNode, type RequestConfiguration, type RequestInformation, type RequestsMetadata, type SerializationWriter } from '@microsoft/kiota-abstractions';

/**
 * Creates a new instance of the appropriate class based on discriminator value
 * @param parseNode The parse node to use to read the discriminator value and create the object
 * @returns {Success}
 */
// @ts-ignore
export function createSuccessFromDiscriminatorValue(parseNode: ParseNode | null | undefined) : ((instance?: Parsable) => Record<string, (node: ParseNode) => void>) {
    return deserializeIntoSuccess;
}
/**
 * The deserialization information for the current model
 * @returns {Record<string, (node: ParseNode) => void>}
 */
// @ts-ignore
export function deserializeIntoSuccess(success: Success | null | undefined = {} as Success) : Record<string, (node: ParseNode) => void> {
    if (!success) return {};
    return {
        "string": n => { success.string = n.getCollectionOfPrimitiveValues<string>(); },
        "successString": n => { success.successString = n.getStringValue(); },
    }
}
/**
 * Builds and executes requests for operations under /example1
 */
export interface Example1RequestBuilder extends BaseRequestBuilder<Example1RequestBuilder> {
    /**
     * Test generating error with message property.
     * @param requestConfiguration Configuration for the request such as headers, query parameters, and middleware options.
     * @returns {Promise<Success>}
     */
     post(requestConfiguration?: RequestConfiguration<object> | null | undefined) : Promise<Success | undefined>;
    /**
     * Test generating error with message property.
     * @param requestConfiguration Configuration for the request such as headers, query parameters, and middleware options.
     * @returns {RequestInformation}
     */
     toPostRequestInformation(requestConfiguration?: RequestConfiguration<object> | null | undefined) : RequestInformation;
}
/**
 * Serializes information the current object
 * @param writer Serialization writer to use to serialize this model
 */
// @ts-ignore
export function serializeSuccess(writer: SerializationWriter, success: Success | null | undefined = {} as Success) : void {
    if (success === undefined || success === null) return;
    if(success.string){
      writer.writeCollectionOfPrimitiveValues<string>(undefined, success.string);
    } else if(success.successString){
      writer.writeStringValue(undefined, success.successString);
    }
}
/**
 * Composed type wrapper for classes {@link string[]}, {@link string}
 */
export interface Success extends Parsable {
    /**
     * Composed type representation for type {@link string[]}
     */
    string?: string[] | null;
    /**
     * Composed type representation for type {@link string}
     */
    successString?: string | null;
}
/**
 * Uri template for the request builder.
 */
export const Example1RequestBuilderUriTemplate = "{+baseurl}/example1";
/**
 * Metadata for all the requests in the request builder.
 */
export const Example1RequestBuilderRequestsMetadata: RequestsMetadata = {
    post: {
        uriTemplate: Example1RequestBuilderUriTemplate,
        responseBodyContentType: "application/json",
        adapterMethodName: "send",
        responseBodyFactory:  createSuccessFromDiscriminatorValue,
    },
};
/* tslint:enable */
/* eslint-enable */

[rkodev]

@baywet @andrueastman @koros

The approach to re-introduce the wrapper class is because I cant think of any other way to deserialize the classes that would not result in changing the ParseNode factory. As it is, deserialization of the composed types can be any union of primitives, collections or objects. It would not be possible to evaluate the correct send method since the response is unknown.

Happy to hear your thoughts on the same

[andrueastman]

Just to confirm here @rkodev,

Is the evaluation of the correct send method the only challenge you see?

Assuming the deserializer and serializer functions are correctly generated with the type information from the description, could we possibly

• have the generator detect the scenario where the composed type is a combination of different send methods at generation time (e.g. collection and non collection or primitive and collection)
• generate/use a sendmethod that will return an intermediate type that can be eventually used by the relevant parsenodes (e.g. a stream/Arraybuffer).
• once the stream/Arraybuffer(or intermediate type) is returned, generate code to convert to the relevant parseNode and let the generated functions do the job as they should.(this could also be a functionality added to the libs)

Thoughts?

The approach to re-introduce the wrapper class is because I cant think of any other way to deserialize the classes that would not result in changing the ParseNode factory.

I believe using the first class features of the language would be the best outcome here if we can get there.

[rkodev]

have the generator detect the scenario where the composed type is a combination of different send methods at generation time (e.g. collection and non collection or primitive and collection)

Should be possible

generate/use a sendmethod that will return an intermediate type that can be eventually used by the relevant parsenodes (e.g. a stream/Arraybuffer).

Should be possible with the sendPrimitive with type ArrayBuffer

once the stream/Arraybuffer(or intermediate type) is returned, generate code to convert to the relevant parseNode and let the generated functions do the job as they should.(this could also be a functionality added to the libs)

This will need to be handle at library level since we do risk generation of a larger amount of code.

The only challenge on this might be how to detect if the ArrayBuffer is a Parsable type X or Parsable Type Y, or a collection of type X. While it might be easier to evaluate if the content of the arraybuffer is a primitive/collection of primitives it might mean to brute force the responses for interfaces?

Another issue I found with brute force approach, you might find type X and type Y have an intersecting property i.e x.name and y.name, in this case while the response might be of type Y, if the order of brute force is type X before Y it might erroneously be converted to type X

[andrueastman]

The only challenge on this might be how to detect if the ArrayBuffer is a Parsable type X or Parsable Type Y, or a collection of type X. While it might be easier to evaluate if the content of the arraybuffer is a primitive/collection of primitives it might mean to brute force the responses for interfaces?

Won't the generated deserailizer function do all this? All you would need to do is initialize a ParseNode instance with the stream and pass it to the function. Yes?

[baywet]

Thanks for adding much needed details on this topic. In the case of deserialization whenever primitive types are present (or collections of Parsable) as member of the union types, I believe the main limitation is the fact that factory methods:

1. always declare returning a parsable.
2. do not contain "triage" code to return the collections of parsable or the scalar values.

If we could start unblocking this, we'd probably get to the next logical blocker (which might be the parsenode interface itself?)

For the serialization case, I think having the serializeMethod being a triage method should suffice as you've illustrated, and shouldn't require wrapper classes, anything I'm missing here?

[rkodev]

@andrueastman @baywet I have a possible solution for this

1. always declare returning a parsable.

We can add an alias for Parsable to the exported type e.g export type Success = string[] | string | Parsable; and return the exported type which will technically be a parsable since its a union type.

- export type Success = string[] | string;
+ export type Success = string[] | string | Parsable;

/**
 * Creates a new instance of the appropriate class based on discriminator value
 * @param parseNode The parse node to use to read the discriminator value and create the object
 * @returns {string[] | string}
 */
// @ts-ignore
export function createSuccessFromDiscriminatorValue(parseNode: ParseNode | undefined) : ((instance?: Parsable) => Record<string, (node: ParseNode) => void>) {
    return deserializeIntoSuccess;
}
/**
 * The deserialization information for the current model
 * @returns {Record<string, (node: ParseNode) => void>}
 */
// @ts-ignore
-export function deserializeIntoSuccess(success: Partial<string[] | string> | undefined = {}) : Record<string, (node: ParseNode) => void> {
+export function deserializeIntoSuccess(success: Partial<Success> | undefined = {}) : Record<string, (node: ParseNode) => void> {
+ // add a 'default' call if the key is an empty string?
    return {
+      "": n => { success = n.getStringValue() ?? n.getCollectionOfPrimitiveValues<string>(); },
    }
}

[rkodev]

2. do not contain "triage" code to return the collections of parsable or the scalar values.

This only affects the objects / collection of objects when a type discriminator is not present, in such cases I'm not sure if we can conclusively determine the result unless we generate a wrapper class exclusively for this scenarios? that is IFF we do not have a discriminator on the return type and it involves objects then we can return the wrapper class?

Another option is using intersection types when intersecting only objects / only primitives / only collections of objects e.t.c . However this is will not work with an intersection involving types that 'collide' like an array e.t.c

My thoughts are we should use the wrapper as a safe fallback when types are not 'compatible' and a discriminator is not provided.

[baywet]

switching from typescript union types to wrapper types is going to cause a lot of grief to people evolving their OAD:

1. first version of OAD contains an anyOf only object types.
2. client generation with a union type, write some code, take a dependency on that.
3. new version of OAD adds a primitive type to the anyOf.
4. client update, now switches to a wrapper, everything is broken.

We should steer clear of any solution that's not consistent through time.

I like your Partial/add IParsable by default suggestion here, might be worth exploring. Let us know what your results are.

[rkodev]

@baywet @andrueastman Could you look at this solution . The only remaing bit would be to add a default call to serialize composed type as in this example

export function deserializeIntoSuccess(success: Partial<Parsable | string[] | string> | undefined = {}) : Record<string, (node: ParseNode) => void> {
    return {
        "" : n => { success = n.getStringValue() ?? n.getCollectionOfPrimitiveValues<string>(); },
    }
}

[baywet]

As additional information, GitHub's api (api.github.com) for path repos/{owner}/{repo}/pulls is also impacted by this issue.