microsoft / kiota-typescript

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

Wrong serializer used if array of string uses enum #1276

Open fredmaggiowski opened 2 months ago

fredmaggiowski commented 2 months ago

Hello, we are facing a problem with an API that holds an array of strings which has specific allowed values defined by enum.

Such array is being transformed to a comma separated list rather than having a proper JSON serialization.

Portion of the JSON Schema we are using

  "requestBody": {
        "content": {
            "application/json": {
                "schema": {
                    "additionalProperties": false,
                    "properties": {
                        "contexts": {
                            "items": {
                                "enum": [
                                    "company",
                                    "project"
                                ],
                                "type": "string"
                            },
                            "type": "array"
                        },

This JSON schema is correctly interpreted by Swagger UI

immagine

However when serializing the payload we get this result:

{
    contexts: 'company, project',
    name: 'extension name'
}

I've reproduced this issue in a test I have in my project where I'm intercepting the request produced by the generated client:

Screenshot 2024-07-12 alle 17 08 03

After doing a bit of reverse engineering I've found this line that may be the culprint

immagine

As far as I understood, here the contexts field is being registered as an enum rather than an CollectionOfObjects and I think may be the cause of the serialization issue.

By updating the OAS definition not to use the enum there is a visible change to the above row: https://github.com/mia-platform/console-sdk/pull/186/files#diff-edaa8455836441c91748a0db0ddb0ae4d819148908dd0da2de5b92c60de36d5aR378

see https://github.com/mia-platform/console-sdk/pull/186

- if(extensionsPutRequestBody.contexts)
-    writer.writeEnumValue<ExtensionsPutRequestBody_contexts>("contexts", ...extensionsPutRequestBody.contexts);
+writer.writeCollectionOfPrimitiveValues<string>("contexts", extensionsPutRequestBody.contexts);

Does this make sense, do you have any insights of something I may overlook?

Thanks

baywet commented 2 months ago

Hi @fredmaggiowski, Thanks for using kiota and for reaching out. I believe your analysis is correct, and the code generation is not doing what it's supposed to be doing here. I think the idea of csv is for when an enum is flaggable instead of just being a collection. This is the section of code that'd need to be updated in the generator Is this something you'd like to look into and submit a pull request for?

fredmaggiowski commented 1 month ago

Hi, thanks for your reply!

I've tried looking into it yesterday but didn't manage to find enough time to properly study the repo and create a valid test case to replicate the issue in a test 😞

baywet commented 1 month ago

Thanks for looking into it. Do you have any specific question that could help bootstrap your work?

fredmaggiowski commented 1 month ago

Hi, I haven't got much time to work this last week; I've briefly read the tests of the class you linked me but couldn't figure out the proper way to fit my use case in them.

I've planned to work on this by the end of the week.

rners01 commented 3 weeks ago

@fredmaggiowski @baywet Hey, I've the same issue, was it fixed?

fredmaggiowski commented 3 weeks ago

Hi @rners01, unfortunately I couldn't find some time to put on the issue yet :(

baywet commented 3 weeks ago

@rners01 do you want to try submitting a pull request for this?

rners01 commented 3 weeks ago

Hey @baywet, okay, can you give some tips?

baywet commented 3 weeks ago

@rners01

This is the section of code that'd need to be updated in the generator

You can add unit tests here (duplicate the existing, tweak the setup, tweak the assertions)

Let us know if you have any additional comments or questions.

rners01 commented 1 week ago

Hey @baywet , I'm trying to add some test but the C# enum is not treated as one then in WritePropertySerializer

debug example with existing WritesConstructorWithEnumValue test

image
baywet commented 1 week ago

Hi @rners01 Thank you for the additional information. I'm having difficulties following the issue here. Can you open a draft PR so we can see the changes and help you better please?

Kindest13 commented 1 week ago

@baywet created https://github.com/microsoft/kiota/pull/5309