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

csharp Warnings with x-www-form-urlencoded and array parameters #3569

Closed
johnwc opened this issue Oct 24, 2023 · 12 comments · Fixed by #3593
Closed

csharp Warnings with x-www-form-urlencoded and array parameters #3569

johnwc opened this issue Oct 24, 2023 · 12 comments · Fixed by #3593
Assignees
Labels
generator Issues or improvements relater to generation capabilities. type:bug A broken experience WIP
Milestone

Comments

@johnwc
Copy link

johnwc commented Oct 24, 2023

Based on the closed issue #2291 and #219 is the c# serializer working for form data arrays?

When I try to generate a client using a schema that has application/x-www-form-urlencoded and parameters with type set as array, I get the following warning.

warn: Kiota.Builder.KiotaBuilder[0]
      OpenAPI warning: #/paths/~1reports~1deal_time_to_close/post - The operation postReportsDealTimeToClose has a request body which is not an object type. This is not supported by Kiota and serialization will fail.
warn: Kiota.Builder.KiotaBuilder[0]
      OpenAPI warning: #/paths/~1reports~1deal_time_to_close/post - The operation postReportsDealTimeToClose has a request body with a complex properties and the url form encoded content type. This is not supported by Kiota and serialization of complex properties will fail.
        "produces": [
          "application/json"
        ],
        "consumes": [
          "application/x-www-form-urlencoded"
        ],
        "parameters": [
          {
            "in": "formData",
            "name": "filters[created_at][]",
            "description": "Filter deals by created_at (accepts min/max unix timestamps i.e [1698084670, 1698105600]",
            "type": "array",
            "items": {
              "type": "integer",
              "format": "int32"
            },
            "required": false
          }
        ],
@baywet
Copy link
Member

baywet commented Oct 25, 2023

Thanks for trying kiota and for reaching out.
Can you confirm which version of Kiota you are using? I've just tried on the latest and it seems to be working fine.
Also I can't find a reference to this error message in the code.

@baywet baywet added this to the Backlog milestone Oct 25, 2023
@johnwc
Copy link
Author

johnwc commented Oct 25, 2023

Using Kiota tool 1.7.0, it seems to be here https://github.com/microsoft/kiota/blob/main/src/Kiota.Builder/Validation/UrlFormEncodedComplex.cs

> kiota --version
1.7.0+1d6d0257e38694f73e64f0412739250c6d3722f1

@baywet
Copy link
Member

baywet commented Oct 25, 2023

Thanks for the additional information. I missed the fact that your schema is using an array. The reason why we don't support arrays is because there's no standard way of serializing them:

  • one could explode the parameter i.e. property=value1&property=value2
  • one could use CSV representation i.e property=value1,value2 in which case, what should the separate be? (, ; ...)
  • one could use a JSON representation i.e property=["value1","value2"] in which case the value probably needs to be uri encoded as well
  • ...

For reference, we added this validation rule with this issue #2019

@johnwc
Copy link
Author

johnwc commented Oct 26, 2023

@baywet but it does do arrays, it does them as duplicate variables like your first example. See links below. And, the OpenAPI has the option (collectionFormat) to tell the client how it expects the array to be passed, with csv being the default.

https://github.com/microsoft/kiota-serialization-form-dotnet/blob/main/src/FormSerializationWriter.cs#L81
https://github.com/microsoft/kiota-serialization-form-dotnet/blob/main/src/FormSerializationWriter.cs#L110

@baywet
Copy link
Member

baywet commented Oct 26, 2023

Thanks for pointing this out, I think I got confused. @andrueastman I know we talked about this at length in the past. Between this in dotnet, and that typescript PR I believe my last statement was wrong.
Do you see any reason why the validation rule should keep warning for collections?

@johnwc
Copy link
Author

johnwc commented Oct 26, 2023

Shouldn't the serializer be updated to read the collectionFormat value to know how to write the arrays?

@baywet
Copy link
Member

baywet commented Oct 26, 2023

The equivalent of which is form with explode in OAS v3. The challenge being we're not carrying the explode information and we actually have a divergence of implementation:

this is related to #3238

Let's focus on the generation for a bit, in your scenario, while trying to reproduce it, I was getting the warning, but at least I was getting the generated code out, is that your experience as well?

@johnwc
Copy link
Author

johnwc commented Oct 26, 2023

Yes, that is my experience as well. Luckily the default behavior to write multiple var=value to the form data is how the API is expecting the data. But if it was expecting it to be csv, we would have issues having Kiota as an option for generating API clients.

@baywet
Copy link
Member

baywet commented Oct 26, 2023

thanks for confirming, so I'm guessing the first step is to remove the validation for arrays. And then once this is done, later in #3238 we'll normalize the explode behaviour based on the information that needs to be passed (which we can't pass today without resulting in a breaking change)

@baywet baywet added type:bug A broken experience generator Issues or improvements relater to generation capabilities. and removed question Needs: Attention 👋 labels Oct 26, 2023
@baywet baywet modified the milestones: Backlog, Kiota v1.8 Oct 26, 2023
@baywet
Copy link
Member

baywet commented Oct 27, 2023

Update: for the validation rule fix I put together #3593.
I also found a bug in the parsing library we use and put together microsoft/OpenAPI.NET#1444, until that bug is merged, released, and kiota grabs the latest release, you'll still get one of the warnings.
For the explode information, and for the normalization, we'll need to wait until v2 since it'd require a breaking change.

@johnwc
Copy link
Author

johnwc commented Oct 30, 2023

Can you explain the breaking change, and why defaults that are already being assumed can't be put in place?

@baywet
Copy link
Member

baywet commented Oct 30, 2023

This method needs to be passed the explode information. This would result in adding an additional parameter to the method to drive the different in behavior.
Because this method is defined in an interface, the change would mean a binary breaking change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
generator Issues or improvements relater to generation capabilities. type:bug A broken experience WIP
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants