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.

Sign up for GitHub

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

Jump to bottom

Wrong serializer used if array of string uses enum #1276

Open
fredmaggiowski opened this issue Jul 12, 2024 · 13 comments · May be fixed by microsoft/kiota#5309
Open

Wrong serializer used if array of string uses enum #1276

fredmaggiowski opened this issue Jul 12, 2024 · 13 comments · May be fixed by microsoft/kiota#5309
Labels
bug Something isn't working Needs: Attention 👋 type:bug A broken experience

Comments

@fredmaggiowski
Copy link

fredmaggiowski commented Jul 12, 2024 •
edited
Loading

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 mia-platform/console-sdk#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
@github-project-automation github-project-automation bot moved this to Needs Triage 🔍 in Kiota Jul 12, 2024
@baywet
Copy link
Member

baywet commented Jul 12, 2024

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?

@baywet baywet added bug Something isn't working type:bug A broken experience labels Jul 12, 2024
@baywet baywet moved this from Needs Triage 🔍 to Waits for author 🔁 in Kiota Jul 12, 2024
@baywet baywet added the status:waiting-for-author-feedback Issue that we've responded but needs author feedback to close label Jul 12, 2024
@fredmaggiowski
Copy link
Author

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 😞

@microsoft-github-policy-service microsoft-github-policy-service bot added Needs: Attention 👋 and removed status:waiting-for-author-feedback Issue that we've responded but needs author feedback to close labels Jul 16, 2024
@baywet
Copy link
Member

baywet commented Jul 16, 2024

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

@fredmaggiowski
Copy link
Author

fredmaggiowski commented Jul 22, 2024 •
edited
Loading

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.

@fredmaggiowski fredmaggiowski mentioned this issue Jul 23, 2024
Merged
@andrueastman andrueastman added status:waiting-for-author-feedback Issue that we've responded but needs author feedback to close and removed Needs: Attention 👋 labels Aug 5, 2024
@github-project-automation github-project-automation bot moved this from Waits for author 🔁 to Done ✔️ in Kiota Aug 16, 2024
@baywet baywet reopened this Aug 16, 2024
@github-project-automation github-project-automation bot moved this from Done ✔️ to In Progress 🚧 in Kiota Aug 16, 2024
@rners01
Copy link

rners01 commented Aug 21, 2024

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

@fredmaggiowski
Copy link
Author

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

@microsoft-github-policy-service microsoft-github-policy-service bot added Needs: Attention 👋 and removed status:waiting-for-author-feedback Issue that we've responded but needs author feedback to close labels Aug 21, 2024
@baywet
Copy link
Member

baywet commented Aug 21, 2024

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

@rners01
Copy link

rners01 commented Aug 22, 2024

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

@baywet
Copy link
Member

baywet commented Aug 22, 2024

@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
Copy link

rners01 commented Sep 2, 2024 •
edited
Loading

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
Copy link
Member

baywet commented Sep 3, 2024

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 Kindest13 linked a pull request Sep 3, 2024 that will close this issue
Draft
@Kindest13
Copy link
Contributor

@baywet created
microsoft/kiota#5309

@brad-technologik
Copy link

Is there a workaround or update on the PR for this? Being able to serialize an array of enums seems like a pretty crucial feature for this type of library..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working Needs: Attention 👋 type:bug A broken experience
Projects
Kiota
Status: In Progress 🚧
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix: enum serialization Kindest13/kiota
6 participants
@andrueastman @fredmaggiowski @baywet @Kindest13 @rners01 @brad-technologik