C# CodeGen: Add support for non-exploded array-types with explicit style in query parameters

[redoz]

Implemented according to the query parameter section here: https://swagger.io/docs/specification/serialization/

I also added some tests to verify that the templates generate the expected output, and I did some manual testing against a throw-away web-service to verify that the output is also valid, so I'm somewhat confident I didn't break things.

Template is more or less the same thing copy-pasted 3 times which isn't ideal, but I'm no expert in liquid and I figured at least this way it will be obvious to whoever is looking at it what is happening.

I ended up injecting an additional scope in the template to ensure we wouldn't end up defining the same variable (first_) more than once per scope.

Hopefully I've managed to adhere to the existing style but if you need me to change something, let me know!

Edit: And I spoke too soon...

[jeremyVignelles]

Wouldn't it be easier to have a delimiter string parameter and use that delimiter, instead of duplicating the code based on two booleans?

Also, there is a risk of conflict with #2959

[redoz]

Wouldn't it be easier to have a delimiter string parameter and use that delimiter, instead of duplicating the code based on two booleans?

Probably. I attempted to follow the existing patterns and I figured this is at least obvious even if you have little to no liquid knowledge (like me).

What's worse is that I didn't realize default values aren't set up in the parameter or model so some existing tests broke so this can't be merged as is.

So that's an actual question, how should I patch up default values? The spec says that for query parameter the default style is form and explode defaults to true when style is form.

We can handle that in the template (seems like it would devolve into a bit of a mess though), we can do it in the model, or at some earlier stage of processing?

Also, there is a risk of conflict with #2959

Regarding conflicts, we'll burn that bridge if and when we get there.

[redoz]

I did some checking and found this

        /// <summary>Gets or sets the explode setting for the parameter (OpenAPI only).</summary>
        [JsonProperty(PropertyName = "explode", DefaultValueHandling = DefaultValueHandling.Ignore)]
        public bool Explode
        {
            get => _explode;
            set
            {
                _explode = value;
                ParentOperation?.UpdateRequestBody(this);
            }
        }

I guess it wont be possible to patch this behavior up in the templating model as we have no idea if explode was actually set to anything or just got defaulted to false.

It almost feels like this should be defined as a nullable bool, with default value handling set to Ignore (so we don't emit null), and at least then we'd know whether we'd have to infer a default value or not. Unless Newtonsoft provides some other mechanic for resolving default values for properties with dependencies on other properties?

This was supposed to be a quick fix, and now it's nothing but :)

[jeremyVignelles]

You could know whether the property has been set (because you've invoked the setter), but that feels really hackish.
If I remember correctly, there should be a way in Newtonsoft.Json to have a boolean property that says whether the property was set in the JSON, but it doesn't feel right either.

[redoz]

Best idea I have right now (after poking around Newtonsoft a bit) is maybe a custom JsonConverter? I'll think about it some more, but I'll happily accept input from @RicoSuter who I'm hoping would have a better solution.

Edit: After a bit more trial an error I think I managed to patch up the default behavior for style and explode, it's not the nicest thing I've seen but I think it's correct. At least all the tests pass now.

[RicoSuter]

In other places the library uses Actual* properties for this... so eg it would be an ActualStyle and ActualExplode property - question is a bit whether these two belong to the OpenAPI/JSON Schema model classes or into the code generator models (there we could use Style and Explode)?

[redoz]

In other places the library uses Actual* properties for this... so eg it would be an ActualStyle and ActualExplode property - question is a bit whether these two belong to the OpenAPI/JSON Schema model classes or into the code generator models (there we could use Style and Explode)?

I didn't notice the Actual* pattern so it wasn't really clear how these default values/rules should be handled, but I'll admit the current solution feels like a hack.

In my opinion this should be part of the OpenApi object model, as it pertains to the standard itself, not just the code-generator, but if you have a solution in mind let me know and I'll see if I can find the time to update the PR.

[redoz]

@RicoSuter I've introduced ActualStyle and ActualExplode properties on the OpenApiParameter and changed the model to use those instead.

Also added a couple of tests to validate that explode is now serialized correctly as mentioned in #2987.

[redoz]

@RicoSuter Is there anything I can do to help move this along?

[redoz]

@RicoSuter Should I keep this PR alive? I'd be willing to tweak it some more if it has to, but if there's no interest in merging this I'll just close it.

[RicoSuter]

Please keep it, there are a lot PRs and just limited time... I hope to get time to look into this at some point.

[redoz]

@RicoSuter 100% understandable, just let me know if there's anything I can do to help move things along.

[redoz]

Rebased and resolved conflict.

[WizX20]

Are there any issues with the PR? It's been a while and the fix looks ok to me.

[redoz]

FYI I won't be maintaining this PR anymore, if someone else want's to pick it up, be my guest.