Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[TS] Regression in handling enums #4199

Closed
andreaTP opened this issue Feb 19, 2024 · 11 comments · Fixed by #4360
Closed

[TS] Regression in handling enums #4199

andreaTP opened this issue Feb 19, 2024 · 11 comments · Fixed by #4360
Assignees
Labels
type:bug A broken experience TypeScript Pull requests that update Javascript code WIP
Milestone

Comments

@andreaTP
Copy link
Contributor

The following OpenAPI description:

openapi: 3.0.3
info:
  title: Apicurio Registry API [v3]
  version: 3.0.x
  description: Apicurio Registry.
  license:
    name: Apache 2.0
    url: https://www.apache.org/licenses/LICENSE-2.0
paths:
  "/admin/rules":
    get:
      responses:
        '200':
          content:
            application/json:
              schema:
                "$ref": "#/components/schemas/RuleType"
          description: The list of names of the globally configured rules.
      description: 'List global rules

        '
components:
  schemas:
    RuleType:
      description: ''
      enum:
      - VALIDITY
      - COMPATIBILITY
      - INTEGRITY
      type: string
      example: VALIDITY

Generates invalid code:

export const RulesRequestBuilderRequestsMetadata: RequestsMetadata = {
    get: {
        uriTemplate: RulesRequestBuilderUriTemplate,
        responseBodyContentType: "application/json",
        adapterMethodName: "sendAsync",
        responseBodyFactory:  createRuleTypeFromDiscriminatorValue,
    },
};

where createRuleTypeFromDiscriminatorValue is not defined in any of the generated files.
Found here.

@andreaTP
Copy link
Contributor Author

I'm trying to understand "what to fix" here and I'm a bit confused.
Enum values are handled pretty much as primitive types, but this assumption cannot be used in the deserialization factory.

  • is not a PrimitiveTypesForDeserialization
  • ParseableFactory needs to return a Record

Do we want to encode Enums closer to classes or should we enable them to be used more like primitives?

@andrueastman
Copy link
Member

Thanks for raising this @andreaTP

I believe due to the IParsable constraint on classes sent by the various SendXXX in the request adapter, the handling of enums should be handled by the sendPrimitiveAsync and the relevant methods updated to check for the enum type. In a future version of Kiota where we make breaking changes to the IRequestAdapter we could consider probably move this out to a sendEnum method and have the enum level handling on its own.
I suspect this scenario affects other languages as well as I'm not aware of a scenario that we've encountered an Enum was returned as the result object.

@andrueastman andrueastman added the type:bug A broken experience label Feb 20, 2024
@andreaTP
Copy link
Contributor Author

Thanks for the feedback @andrueastman !
Unfortunately, it's not yet clear to me what should be done to fix this issue.

Either:

  • the type signature of responseBodyFactory should change to include Enums (and I'm not sure how)
  • Enums should be treated like objects and we should generate the create{XYZ}FromDiscriminatorValue

@andrueastman
Copy link
Member

Taking a deeper look at TS abstractions, it looks like like Parsable is an empty interface. Which means the Parsable constraint assumed doesn't hold here. So, we may use the second option here, but this may be different for other languages.

@andreaTP Any chance you can share the link to the original openApi description for the client in the PR? I'd like to compare behaviors in different languages first. Unfortunately, when I take a look at the GO client generated, it seems to suggest the path returns a collection and not a single value.
https://github.com/Apicurio/apicurio-registry/blob/33f348c600dbef7b2679a8e553079604eb1b3b18/go-sdk/pkg/registryclient-v3/admin/rules_request_builder.go#L90 (This would be resolvable using a sendCollectionOfEnumsValue method missing in TS abstractions that Go uses)

@andreaTP
Copy link
Contributor Author

You are correct, the original OpenAPI definition uses a collection, I removed it as the problem can be reproduced.

This is the original description for both Go and Typescript

@andrueastman
Copy link
Member

Taking a look across languages, looks like enums should ideally be handled separetely from either objects or primitives.

The TS abstractions is missing the sendEnum and sendEnumCollection methods that are present in other languages such as GO and java.
https://github.com/microsoft/kiota-abstractions-go/blob/a9e5a330669021f32fe021ff0e0671446e30c8d4/request_adapter.go#L26
https://github.com/microsoft/kiota-java/blob/082b0697e806823711f23abe6faa956ac10d91f4/components/abstractions/src/main/java/com/microsoft/kiota/RequestAdapter.java#L102C67-L102C85

Dotnet seems to have mapped enums to primitives(I suspect adding the methods later on would have been a breaking change. Created issue for it at microsoft/kiota-dotnet#199)

As this change is not breaking in TS, we should probably update the requestAdapter interface so that it includes these methods, and update the generator for enum handling, then have the relevant method call the getEnumValue/getCollectionOfEnumValues methods on the received response.

@andrueastman andrueastman added the TypeScript Pull requests that update Javascript code label Feb 21, 2024
@andreaTP
Copy link
Contributor Author

Is this work already scheduled on your side?

@baywet
Copy link
Member

baywet commented Feb 21, 2024

Hey everyone! 👋
Thanks for the discussion here, for information this is also something I had noticed while working on the proxy generation.
And I had created an issue that outlines at a high level what's missing microsoft/kiota-typescript#1042
As you know we're in the middle of reorganizing the team to get better productivity, I'm not aware that anyone is working on TypeScript at this point besides Andrew and I (on top of all the other things), and Seb, the PM for TypeScript as a language is on vacations for another week. So things are a bit floating at this point. Happy for you to grab it if you have the bandwidth and provide some help.

@petrhollayms petrhollayms added this to the Kiota v1.12 milestone Feb 21, 2024
@andreaTP
Copy link
Contributor Author

Thanks for the wrap-up @baywet , appreciated, I'm going to have a pretty busy schedule in the next weeks, so let's see who comes first to this one.

Copy link
Contributor

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.

@andreaTP
Copy link
Contributor Author

Can we set this to "never-stale"? We can close when completed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:bug A broken experience TypeScript Pull requests that update Javascript code WIP
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

5 participants