[kotlin-spring][server] Fix: treat "nullable: false, required: false" attributes with "default" fallback value as non-nullable in DTOs

[Picazsoo]

This PR fixes the issue where non-required attributes with defined default values are still generated as nullable in kotlin classes. These should be instead treated as non-nullable as the default value guarantees that upon deserialization there will always be at least the fallback default value.

basically e.g.

        nonRequiredWithDefaultString:
          type: string
          default: defaultValue

should generate:
override val nonRequiredWithDefaultString: kotlin.String = "defaultValue"
and not:
override val nonRequiredWithDefaultString: kotlin.String? = "defaultValue"
because the attribute has a known value to fall back to.

And

        nonRequiredNullableWithDefaultString:
          type: string
          nullable: true
          default: defaultValue

should still generate:
override val nonRequiredWithDefaultString: kotlin.String? = "defaultValue"
to explicitly allow the value to be set to null if one so wishes.

I implemented some basic tests (for numbers, strings, enums, lists, sets) and regenerated all the kotlin files. I also took the liberty to improve the DTO formatting (and hence readability) a bit, so that the generated code more closely resembles what would be written by a human (annotations above the attribute; not to the left of it)

I think that this is not a breaking change - but linters etc will probably warn about unnecessary safe call operators and/or elvis operators used to provide a fallback value before this fix is implemented.

Actually I can imagine this being a somewhat breaking change when used together with the x-kotlin-implements for implementation of arbitary developer-defined interface. There it might cause some issues and adjustment of the interfaces might be needed to remove the non-nullability. But the x-kotlin-implements has been released only in September, so I would expect the real world impact to be rather limited.

If you see some glaring gap in my logic, feel free to point it out. Maybe I am overlooking something here and having the attributes still nullable somehow makes sense.

I could not find an issue ticket for exactly this issue, so I went ahead and created the PR without linking anything. I hope it is ok.

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.
• Run the following to build the project and update samples:

  ./mvnw clean package || exit
  ./bin/generate-samples.sh ./bin/configs/*.yaml || exit
  ./bin/utils/export_docs_generators.sh || exit
  

  (For Windows users, please run the script in WSL)
  Commit all changed files.
  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*.
  IMPORTANT: Do NOT purge/delete any folders/files (e.g. tests) when regenerating the samples as manually written tests may be removed.
• File the PR against the correct branch: master (upcoming 7.x.0 minor release - breaking changes with fallbacks), 8.0.x (breaking changes without fallbacks)
• If your PR solves a reported issue, reference it using GitHub's linking syntax (e.g., having "fixes #123" present in the PR description)
• If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request. - tagging: @karismann, @Zomzog, @andrewemery, @4brunu, @yutaka0m, @stefankoppier, @e5l

[Picazsoo]

I am using these dummy multi-line comments as a way to make the long lines more readable.

[Picazsoo]

now the attribute is neatly lined below the annotations

[Picazsoo]

This shows that the default works correctly with most sensible defaults

[wing328]

Thanks for the PR

I'm no expert in Kotlin.

Regarding the following:

override val nonRequiredWithDefaultString: kotlin.String? = "defaultValue"

My understanding is that kotlin.String? indicates this variable can accept null.

Removing ? means it can no longer accept null, right?

[wing328]

FYI. Not directly related to this PR and I just filed #22176 to fix how optional parameter is handled in kotlin-spring generator.

[Picazsoo]

Thanks for the PR

I'm no expert in Kotlin.

Regarding the following:

override val nonRequiredWithDefaultString: kotlin.String? = "defaultValue"

My understanding is that kotlin.String? indicates this variable can accept null.

Removing ? means it can no longer accept null, right?

Yes, ? after attribute type signifies that the attribute is nullable. So it can be set to null and it might return null (if explicitly set to null)

Basically when calling a method or instantiating a class:
Non-nullable + default

• Caller can skip the argument → default is used.

• Caller cannot pass null → compile error.

Nullable + default

• Caller can skip the argument → default is used.

• Caller can explicitly pass null → value becomes null

This could definitely use eyes of someone kotlin-focused. I definitely would not like to introduce some major issue based on my potential confused idea.

I can imagine this might not make sense for kotlin clients. There by omitting the value and not sending it to the server you basically leave it to the server to fill in the default. If implemented for clients as well, it would mean that the client would always send the default value if the argument was skipped while creating a DTO - hence increasing the payload size. But for server it makes sense to me - you have a DTO deserialized with fields that are not present in the json payload -> substitute with the default value and hence guarantee to never return null.

I added more tests to also cover non required nullable with a null/non-null default value. For nullable with default, it should generate the value always as nullable to allow overriding the default value with null.

[stefankoppier]

Although it makes sense on the server side, and not on the client side, I'm not completely sure about this change myself, or at least the end-goal.

- Some users might want to implement some business logic if the value is not supplied, and later use the default value. But I'm unsure if this is possible currently, as I think the default value will be used and the constructor would not be supplied with a null argument.
- Some users might use the server side models for testing purposes, where they supply null values to optional parameters. Although you can argue that they should generate client code for that.

Edit: My bad, I missed that it only holds if nullable: false. Above arguments are not relevant ;)

[Picazsoo]

- Some users might use the server side models for testing purposes, where they supply null values to optional parameters. Although you can argue that they should generate client code for that.

I think this actually holds some water. Someone might be e.g. misusing the generated DTOs for testing purposes in such a way that they first instantiate them, then they serialize then into a body as json for integration tests. In this case this would be taking away the option to initialize with null and (with a specific jackson serialization config) the possibility to effectively skip these null attributes and thus not include them in the json at all.

This change would make this way of crafting json payloads with omitted attributes for testing impossible. But (in my subjective opinion) that is not a proper way to test anyway as the tests end up being dangerously coupled to the implementation and one can easily overlook e.g. accidentally renaming a request body parameter and the tests will not catch it.

[Picazsoo]

I am wondering if I should create a corresponding issue and link the PR from there? If we expect further discussion, maybe the issue would be a better place?

Don't get me wrong - I am in no hurry to get this merged. For my practical use, I can solve this via template overrides, but of course having it eventually merged into the project would make sense to me.

[Picazsoo]

Sorry for tagging you directly @wing328. I am just wondering if there is anything I can do to improve the chances of getting this merged. And @stefankoppier - I take it you don't object this change being merged?

I understand that this is a volunteer-based effort, so I don't want to come across as too pushy. I am just excited by the natural match between kotlin's language capabilities and open api specification and hence the possibility to make the generated code more complete.