microsoft / kiota

OpenAPI based HTTP Client code generator
https://aka.ms/kiota/docs
MIT License
3.02k stars 210 forks source link

composed types serialization support for TypeScript #1812

Closed baywet closed 3 months ago

baywet commented 2 years ago

follow up of #1411 implemented via #1603 in dotnet/go/java. Effectively TypeScript is the only language that properly support union and intersection types. And we might want to implement the feature a different way that with wrapper classes. Opening this issue so we can discuss implementation details of the factory, serialization and deserialization methods for those types. (wrapper classes vs static functions).

sebastienlevert commented 1 year ago

Bringing this back onto our stack. When using the following command, we get that there is no UnionType support in TypeScript.

kiota generate -l typescript -d https://api.apis.guru/v2/specs/github.com/api.github.com/1.1.4/openapi.json -c GitHubClient -o github -i "/orgs/{org}/repos"

@koros this seems like a good next step for us to implement.

baywet commented 1 year ago

2462 should also be considered

baywet commented 1 year ago

meeting notes:

The move from classes to interfaces makes using native constructs instead of wrapper classes much easier since the serialization methods are now functions. e.g.

What should be decided is whether we want to project a type alias. The name wouldn't depend on composed type members to avoid breaking changes whenever a new member is introduced. But it'd make usage easier. In fact wrapper classes today hold two responsibilities: type alias and serialization methods.

jabrks commented 8 months ago

I was directed to this issue from https://github.com/microsoft/kiota/issues/4326 and asked to share my expectations around how Kiota would handle this scenario. I've reused some of the details from that issue below.

We have the following OpenAPI definition

openapi.yml ```yaml openapi: 3.0.1 info: title: Survey API version: 1.0.0 paths: /api/v1/answer: post: operationId: submitAnswer requestBody: content: application/json: schema: $ref: '#/components/schemas/SubmitAnswerRequest' required: true responses: '201': content: '*/*': {} description: Answer a question components: schemas: SubmitAnswerRequest: type: object discriminator: mapping: TEXT: '#/components/schemas/SubmitTextAnswerRequest' BOOLEAN: '#/components/schemas/SubmitBooleanAnswerRequest' propertyName: answerType oneOf: - $ref: '#/components/schemas/SubmitTextAnswerRequest' - $ref: '#/components/schemas/SubmitBooleanAnswerRequest' properties: answerType: $ref: '#/components/schemas/AnswerType' required: - answerType SubmitTextAnswerRequest: type: object allOf: - $ref: '#/components/schemas/SubmitAnswerRequest' - type: object properties: answer: type: string required: - answerType SubmitBooleanAnswerRequest: type: object allOf: - $ref: '#/components/schemas/SubmitAnswerRequest' - type: object properties: answer: type: boolean required: - answerType AnswerType: type: string enum: - TEXT - BOOLEAN ```

which currently outputs the following code

index.ts ```typescript // ... export type AnswerType = (typeof AnswerTypeObject)[keyof typeof AnswerTypeObject]; export interface SubmitAnswerRequest extends AdditionalDataHolder, Parsable { additionalData?: Record; answerType?: AnswerType; } export interface SubmitBooleanAnswerRequest extends Parsable, SubmitAnswerRequest { answer?: boolean; } export interface SubmitTextAnswerRequest extends Parsable, SubmitAnswerRequest { answer?: string; } export const AnswerTypeObject = { TEXT: "TEXT", BOOLEAN: "BOOLEAN", } as const; // ... interface AnswerRequestBuilder extends BaseRequestBuilder { post(body: SubmitBooleanAnswerRequest | SubmitTextAnswerRequest, requestConfiguration?: RequestConfiguration | undefined) : Promise; } ```

This allows the answer type to be a string or a boolean, regardless of what the value of answerType is. Ideally, it would take the answerType value into account and output something like this instead

index.ts ```diff export type AnswerType = (typeof AnswerTypeObject)[keyof typeof AnswerTypeObject]; export interface SubmitAnswerRequest extends AdditionalDataHolder, Parsable { additionalData?: Record; answerType?: AnswerType; } export interface SubmitBooleanAnswerRequest extends Parsable, SubmitAnswerRequest { + answerType: "BOOLEAN"; answer?: boolean; } export interface SubmitTextAnswerRequest extends Parsable, SubmitAnswerRequest { + answerType: "TEXT"; answer?: string; } export const AnswerTypeObject = { TEXT: "TEXT", BOOLEAN: "BOOLEAN", } as const; interface AnswerRequestBuilder extends BaseRequestBuilder { post(body: SubmitBooleanAnswerRequest | SubmitTextAnswerRequest, requestConfiguration?: RequestConfiguration | undefined) : Promise; } ```

as this would narrow the types to ensure that only valid answers are allowed, and ensure any invalid calls to the client result in compiler errors as demonstrated here

// Argument of type '{ answerType: "BOOLEAN"; answer: string; }' is not assignable to parameter of type 'SubmitBooleanAnswerRequest | SubmitTextAnswerRequest'.
//   Types of property 'answer' are incompatible.
//    Type 'string' is not assignable to type 'boolean | undefined'.
client.api.v1.answer.post({ answerType: 'BOOLEAN', answer: 'true' });
                          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

// Argument of type '{ answerType: "TEXT"; answer: boolean; }' is not assignable to parameter of type 'SubmitBooleanAnswerRequest | SubmitTextAnswerRequest'.
//  Types of property 'answer' are incompatible.
//    Type 'boolean' is not assignable to type 'string'.
client.api.v1.answer.post({ answerType: 'TEXT', answer: true });
                          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
baywet commented 8 months ago

Thanks for the additional information @jabrks In a composed type scenario with a discriminator like you've shared, would you expect to have access to the discriminator value, or for the client to "guess it"?

What I'm trying to understand here is whether kiota clients would be able to leverage typescript union/intersection types or whether, because of the discriminator, we'll need wrapper types anyway.

// those examples are taking advantage of the typescript union types
// because a string is passed, this would fall on one of the member of the union types, and it'd be "obvious" the discriminator value needs to be `answerType: 'TEXT'`
client.api.v1.answer.post({ answer: 'true' });
// same but for boolean
client.api.v1.answer.post({ answer: true });
//those examples are taking advantage of a wrapper type, closer to what we do in other languages
client.api.v1.answer.post({answerType: 'TEXT', memberType1: { answer: 'true' } });
// same but for boolean
client.api.v1.answer.post({answerType: 'BOOLEAN', memberType2: { answer: true } });
// those examples are probably defined as `{ answerType: 'BOOLEAN' | 'TEXT'} & ({answer: boolean} | {answer: string})` which is equivalent to `{ answerType: 'BOOLEAN' | 'TEXT'} & {answer: boolean| string}` which would not be a great developer experience.
// we might be able to tweak them with something like `{ answerType: 'BOOLEAN' | 'TEXT'} & ({answer: boolean} answerType is 'BOOLEAN' | {answer: string} answerType is 'TEXT')` is that syntax is even supported
client.api.v1.answer.post({answerType: 'TEXT', answer: 'true' });
// same but for boolean
client.api.v1.answer.post({answerType: 'BOOLEAN', answer: true });
jabrks commented 8 months ago

I would expect to have to explicitly define it, if that's what you're asking @baywet?

jabrks commented 8 months ago

But to elaborate, if I wrote

client.api.v1.answer.post({ answer: true });

then the only acceptable value for answerType would be BOOLEAN, yes.

I'm not as keen on the memberType wrapper syntax if that is avoidable, it would be preferable to me to match the structure of the request payload exactly when calling the client

baywet commented 8 months ago

Thanks for the input. I'm not sure we'll be able to "guess" the discriminator value without having to generate very convoluted code because how how much type information is retained at runtime, but it's good to have that information in mind.

jabrks commented 8 months ago

No problem @baywet. That's fine, I would expect to have to explicitly state the discriminator as per my original example

client.api.v1.answer.post({ answerType: 'BOOLEAN', answer: true });

but ideally without the wrapper types as mentioned

pschaeflein commented 7 months ago

@baywet - My expectation is the same as jabrks:

  • I need to provide the discriminator
  • I don't want member/wrapper types.
jlarmstrongiv commented 7 months ago

I ran into this error by using TypeSpec to generate OpenAPI 3 definitions, which are then provided to Kiota.