microsoft / kiota

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

[TS] Regression in handling enums #4199

Closed andreaTP closed 4 months ago

andreaTP commented 5 months ago

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 commented 5 months ago

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.

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

andrueastman commented 5 months ago

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.

andreaTP commented 5 months ago

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

Either:

andrueastman commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 https://github.com/microsoft/kiota-abstractions-dotnet/issues/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.

andreaTP commented 5 months ago

Is this work already scheduled on your side?

baywet commented 5 months ago

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 https://github.com/microsoft/kiota-typescript/issues/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.

andreaTP commented 5 months ago

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.

microsoft-github-policy-service[bot] commented 4 months 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.

andreaTP commented 4 months ago

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