microsoft / kiota-typescript

TypeScript libraries for Kiota-generated API clients.
https://aka.ms/kiota/docs
MIT License
32 stars 25 forks source link

Enums containing spaces do not deserialize properly. #1219

Closed NavidMitchell closed 5 days ago

NavidMitchell commented 4 weeks ago

I am working with a spec and they have enums in their OpenAPI spec like follows. Arguably this spec is not the best. However, I cannot change it since it is not developed by my company.

    "status": {
          "type": "string",
          "enum": [
            "IN_PROGRESS",
            "IN PROGRESS",
            "In Progress",
            "In Progress "
          ]
        }

This creates the corresponding typescript code.

export type Test_status = (typeof Test_statusObject)[keyof typeof Test_statusObject];
export const Test_statusObject = {
    IN_PROGRESS: "IN_PROGRESS",
    INPROGRESS: "IN PROGRESS"
} as const;

The code that deserializes the json to an Enum is here packages/serialization/json/src/jsonParseNode.ts

    public getEnumValue = <T>(type: any): T | undefined => {
        const rawValue = this.getStringValue();
        if (!rawValue) {
            return undefined;
        }
        return type[toFirstCharacterUpper(rawValue)];
    };

As you can see this code does not handle the cases where spaces are used.

In addition the following values will be lost since there are not in the Test_statusObject.

        "In Progress",
        "In Progress "
andrueastman commented 3 weeks ago

@koros Should the in getEnumValue be changed to lookup the raw value from the values in the const? And drop the need of toFirstCharacterUpper as well?

koros commented 2 weeks ago

Sample Open API file:

openapi: 3.0.1
info:
  title: Example of Enum Values
  version: 1.0.0
paths:
  /job-status:
    get:
      summary: Get Job Status
      description: Retrieve a list of job status enum values.
      responses:
        '200':
          description: OK
          content:
            application/json:
              schema:
                $ref: '#/components/schemas/JobStatus'
components:
  schemas:
    JobStatus:
      type: string
      enum: 
        - IN_PROGRESS
        - IN PROGRESS
        - In Progress
        - In Progress 
        - Waiting For Approval
        - WAITING FOR APPROVAL

Currently the generated code is as follows:

export type JobStatus = (typeof JobStatusObject)[keyof typeof JobStatusObject];
export const JobStatusObject = {
    IN_PROGRESS: "IN_PROGRESS",
    INPROGRESS: "IN PROGRESS",
    WaitingForApproval: "Waiting For Approval",
} as const;

Proposed Changes:

Generation:

const jobStatusValues = [
  "IN_PROGRESS",
  "IN PROGRESS",
  "In Progress",
  "In Progress ",
  "Waiting For Approval",
  "WAITING FOR APPROVAL",
] as const;

type JobStatus = (typeof jobStatusValues)[number];

Abstraction:

public getEnumValue = <T>(type: any): T | undefined => {
    const rawValue = this.getStringValue();
    return rawValue && type.includes(rawValue as T) ? (rawValue as T) : undefined;
};
koros commented 2 weeks ago

@baywet let me know if I should go ahead with this

baywet commented 2 weeks ago

@koros what would be the impact of that change on an enum using numbers for serialization? i.e. what would prevously have been

export const JobStatusObject = {
    IN_PROGRESS: 1,
    INPROGRESS: 2",
    WaitingForApproval: 4,
} as const;

?

Also what would be the impact on the consuming developer for that change?

koros commented 2 weeks ago

@baywet It will probably be a breaking change, what other option can we use? the existing code strips white spaces and does case insensitive comparison here: https://github.com/microsoft/kiota/blob/f07446c03b9e44116f6cd3ecab208177e56e0b44/src/Kiota.Builder/KiotaBuilder.cs#L1893

baywet commented 2 weeks ago

@koros besides sorting options very precisely and appending _1... for the colliding ones, I don't see a solution that wouldn't introduce a regression here. @NavidMitchell I think we didn't explore a couple of options here. This seems to be an edge case caused by an poorly designed API (even if the data isn't clean, the API should clean things up on the fly to avoid putting the burden on clients) Have you considered:

NavidMitchell commented 2 weeks ago

@baywet I'm not sure I agree that the tool should change the data defined in the API. However, since spaces obviously do not map well to any programming language enum type. I personally would never define a spec the way my client has.

I think your suggestion of a using a string may be the best approach for my situation, at the moment.

As for kiota, since a space is valid for an Enum definition in OpenAPI it may be best to just map them as a String Literal Union in typescript. Since this would always map to the OpenAPI spec correctly. This is what some of the other generators do. (i.e. type Easing = "ease-in" | "ease-out" | "ease in out" )

baywet commented 2 weeks ago

Thanks for the additional context here.

We went through enums design for kiota typescript clients multiple times https://github.com/microsoft/kiota/issues/2105

At the end we landed on the current solution as the other options didn't seem to work or provide a great developer experience.

Consider the following TypeScript

// 1. generates a bunch of extra code impacting size
enum BodyTypeEnum {
    Text = "text",
    HTML = "HTML"
}
console.log(BodyTypeEnum.Text)

// 2. does not work as the values behind the symbols don't get transipled in a way they can be iterated through, so if they don't match, deserialization is going to fail
// plus repeats the value when it's used in the transpile result, increasing size
const enum BodyTypeConstEnum {
    Text = "text",
    HTML = "HTML"
}
console.log(BodyTypeConstEnum.Text)

// 3. works for our purpose, but is more challenging whith potentially colliding values
const BodyTypeConst = {
   Text: "text",
   Html: "html",
} as const;

type BodyTypeCT = keyof typeof BodyTypeConst;
console.log(BodyTypeConst.Text)

// 4. does not work as the values behind the symbols don't get transipled in a way they can be iterated through, so if they don't match, deserialization is going to fail
// plus repeats the value when it's used in the transpile result, increasing size
// and only gives you safety from typos if the variable/parameter is explicitely typed, not a great developer experience
type BodyTypeUnion = "text" | "HTML";
const value: BodyTypeUnion = "text";
console.log(value)

Which results in the following JS

"use strict";
// 1
var BodyTypeEnum;
(function (BodyTypeEnum) {
    BodyTypeEnum["Text"] = "text";
    BodyTypeEnum["HTML"] = "HTML";
})(BodyTypeEnum || (BodyTypeEnum = {}));
console.log(BodyTypeEnum.Text);
// 2
console.log("text" /* BodyTypeConstEnum.Text */);
// 3. 
const BodyTypeConst = {
    Text: "text",
    Html: "html",
};
console.log(BodyTypeConst.Text);
// 4
const value = "text";
console.log(value);
microsoft-github-policy-service[bot] commented 1 week ago

This issue has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 4 days. It will be closed if no further activity occurs within 3 days of this comment.