[kotlin] Fix optional header params in openhttp client

[isycat]

This change adjusts the kotlin openhttp api template to ignore null headers when building the request rather than convert them to "null" string

PR checklist

• Read the contribution guidelines.
• Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
• If contributing template-only or documentation-only changes which will change sample output, build the project beforehand.
• Run the shell script ./bin/generate-samples.shto update all Petstore samples related to your fix. This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master. These must match the expectations made by your contribution. You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/configs/java*. For Windows users, please run the script in Git BASH.
• File the PR against the correct branch: master
• Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

[jimschubert]

note that this will be a breaking change if sortParamsByRequiredFlag is false. If you specify a default value before other parameters with no default value, you can only invoke the method with named parameters.

This will also potentially break Java interop as it would require @JvmOverloads on all methods, and this could have a non-trivial overhead on larger generated clients.

The only good way I see to address those concerns would be to put this functionality behind a new user option (something like defaultOptionalArgsToNull) and default it to true, which I think would work if we get this into the 5.0 release.

Then this change would become:

{{^required}}?{{#defaultOptionalArgsToNull}} = null{{/defaultOptionalArgsToNull}}{{/required}}

I don't know if defaultOptionalArgsToNull is the best name, though.

[4brunu]

I use the sortParamsByRequiredFlag to false, because of some bugs caused by the reorder of the parameters in the past, so I agree with the new flag, to ensure the correct behaviour of sortParamsByRequiredFlag.

[isycat]

Unless I have misunderstood something, there is no breakage within kotlin from adding = null. You can pass positioned params for a method that has defaulted params before others:

private fun catOld(optionalParam: String?, requiredParam: Int, optionalParam2: String?): String {
    return "$optionalParam$requiredParam$optionalParam2"
}

private fun cat(optionalParam: String? = null, requiredParam: Int, optionalParam2: String? = null): String {
    return "$optionalParam$requiredParam$optionalParam2"
}

@Test
fun testOptionalFirst() {
    // Without defaults, all 3 values must be specified
    assertEquals("123", catOld("1", 2, "3"))
    assertEquals("123", catOld("1", 2, optionalParam2 = "3"))
    assertEquals("123", catOld("1", requiredParam = 2, optionalParam2 = "3"))
    assertEquals("123", catOld(optionalParam = "1", requiredParam = 2, optionalParam2 = "3"))

    // With defaults, the same possible combinations are still valid
    assertEquals("123", cat("1", 2, "3"))
    assertEquals("123", cat("1", 2, optionalParam2 = "3"))
    assertEquals("123", cat("1", requiredParam = 2, optionalParam2 = "3"))
    assertEquals("123", cat(optionalParam = "1", requiredParam = 2, optionalParam2 = "3"))

    // plus:
    assertEquals("12null", cat("1", 2))
    assertEquals("12null", cat("1", requiredParam = 2))
    assertEquals("12null", cat(optionalParam = "1", requiredParam = 2))
    assertEquals("null23", cat(requiredParam = 2, optionalParam2 = "3"))
}

(Was this a recent kotlin change?)

The Java overloads concern sounds legitimate (and therefore also the ability to turn the behavior off)

I am not sure when I will be able to squeeze in the new config param change, so I am going to separate the null defaulting from this PR, so as to not block the null string fix.

[isycat]

(PR updated with null defaulting removed and generated petstore changes)

[jimschubert]

(Was this a recent kotlin change?)

It's been around a while, but I recall being surprised by it when I started using Kotlin because it wasn't documented. The documentation was later updated August 2017 to clarify the corner case. I think most people who started learning Kotlin before this doc update probably don't know it's even a thing, most likely because we're all using named args for clarity :)

[jimschubert]

cc kotlin technical committee @dr4ke616 @karismann @Zomzog @andrewemery @4brunu @yutaka0m

[jimschubert]

Looks good. Thanks for offering to address the default argument issue separately… it definitely makes the smaller reviews easier.