Feature/9621 aspnetcore parameter default values #15753

ntwilkie-amici · 2023-06-05T11:47:13Z

Adds a new option to the aspnetcore generator "useParameterDefaultValues", defaults to false to maintain backwards compatability
Updates AspNetCoreServerCodegen to support default values by; adding an override for fromOperation() to sort parameters, ensuring that parameters with defaults are at the end, adds an override for postProcessParameter() with additional logic to make parameters with defaults non-nullable, updates getNullableType() logic to make parameters with defaults non-nullable
Updates AbstractCSharpCodegen to remove logic in updateCodegenParametersEnum() which makes optional parameters nullable
Updates queryParam template to include default values

When a default value is specified for a parameter, then running the generator with useParameterDefaultValues=true will add the default values to controller interface methods. Note that any parameters with a default value will be moved to the end of the parameter list.

Example:

in: query
name: foo
required: false
schema:
  type: integer
  default: 1

Generates:

public abstract IActionResult MyMethod([FromQuery (Name = "foo")]int foo = 1)

Generating without the flag should result in no changes, without defaults and nullable types created as per other configuration options.

The change to AbstractCSharpCodegen looks like it's removing some redundant code, but I would welcome some feedback as I wasn't sure if it would have an impact in other places?

Thank you.

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 
./bin/generate-samples.sh
./bin/utils/export_docs_generators.sh
```
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*.
For Windows users, please run the script in Git BASH.
In case you are adding a new generator, run the following additional script :
```
./bin/utils/ensure-up-to-date
```
Commit all changed files.
File the PR against the correct branch: master (6.3.0) (minor release - breaking changes with fallbacks), 7.0.x (breaking changes without fallbacks)
If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

@mandrean @frankyjuang @shibayan @Blackclaws @lucamazzanti

…ues in controller methods - Adds a new option to the aspnetcore generator "useParameterDefaultValues", defaults to false to maintain backwards compatability - Updates `AspNetCoreServerCodegen` to support default values by; adding an override for `fromOperation()` to sort parameters, ensuring that parameters with defaults are at the end, adds an override for `postProcessParameter()` with additional logic to make parameters with defaults non-nullable, updates `getNullableType()` logic to make parameters with defaults non-nullable - Updates `AbstractCSharpCodegen` to remove logic in `updateCodegenParametersEnum()` which makes optional parameters nullable - Updates `queryParam` template to include default values

wing328 · 2023-06-05T11:59:43Z

docs/generators/aspnetcore.md

@@ -51,6 +51,7 @@ These options may be applied as additional-properties (cli) or configOptions (pl
 |useDefaultRouting|Use default routing for the ASP.NET Core version.| |true|
 |useFrameworkReference|Use frameworkReference for ASP.NET Core 3.0+ and PackageReference ASP.NET Core 2.2 or earlier.| |false|
 |useNewtonsoft|Uses the Newtonsoft JSON library.| |true|
+|useParameterDefaultValues|Use parameter default values in controller methods.| |false|


thanks for the PR. I don't think we need another option to support default values in the parameters as it should be support by default so I would suggest we remove this option.

Thanks for the review, I've removed the option and made support for default values the standard behaviour.

wing328 · 2023-06-05T11:59:59Z

...penapi-generator/src/main/java/org/openapitools/codegen/languages/AbstractCSharpCodegen.java

-
-            if (!parameter.required && parameter.vendorExtensions.get("x-csharp-value-type") != null) { //optional
-                parameter.dataType = parameter.dataType + "?";
-            }


why remove this code block?

The code means that parameters with defaults are coming through as nullable, it seemed redundant as the update to the types is also being done elsewhere, but I wasn't sure if there is a wider impact on other C# generators? Rather than removing this, an additional check could be added for default values (now that they are standard behaviour) - I don't think anyone would expect to get a nullable type for something which has a default.

cc @mandrean (2017/08) @frankyjuang (2019/09) @shibayan (2020/02) @Blackclaws (2021/03) @lucamazzanti (2021/05)

cc @JFCote @devhl-labs

A parameter with a default can be nullable. This block is something that should be refactored out in favor of using the isNullable property, but until that happens you should not remove it.

wing328 · 2023-06-05T12:09:20Z

...napi-generator/src/main/java/org/openapitools/codegen/languages/AspNetCoreServerCodegen.java

+
+        return op;
+    }
+


Why do we need to sort the parameters based on default values? is that a must?

Adding a line or 2 of comments explaining what this code block does

Yes, it's required that parameters with defaults are listed after ones without. I've added some documentation.

I am also sorting model and operation arguments in the csharp generator. I believe sorting model and operation arguments should be done in the abstract. That is probably worthy of a pr of it's own. Though personally, I don't have much desire to change templates for the other libraries. Perhaps we should sort in the abstract, but the in the csharp generator, revert restsharp and httpclient libraries to their current sorting so no template changes are needed.

wing328 · 2023-06-05T12:10:14Z

samples/openapi3/client/petstore/dart-dio/petstore_client_lib_fake/lib/src/model/cat.dart

@@ -42,11 +42,6 @@ class _$CatSerializer implements PrimitiveSerializer<Cat> {
    Cat object, {
    FullType specifiedType = FullType.unspecified,
  }) sync* {
-    yield r'className';


can you please merge the latest master into this branch? there shoudn't be any update to the dart samples as part of this change.

…ake using default values standard behaviour - Adds additional documentation to explain why parameters are sorted with defaults at the end

…values

devhl-labs · 2023-06-06T03:21:02Z

I think you should default this useParameterDefaultValues option to true. Not using defaults previously was a bug and we are moving to a major version, so the breaking change should be fine imo.

wing328 · 2023-07-02T15:20:18Z

can you please resolve the merge conflicts when you've time?

ntwilkie-amici · 2023-08-21T07:57:22Z

Hello - sorry, I've dropped the ball on this while I've been side lined with other things. Looks like there's been an update to how options are handled and some refactoring around the the netcore naming.

I'll close this PR and will see if I can set aside some time for a new one against the latest version.

ntwilkie-amici added 2 commits June 5, 2023 11:57

[aspnetcore] OpenAPITools#9621 Updates samples and documentation

7e8fb00

wing328 reviewed Jun 5, 2023

View reviewed changes

ntwilkie-amici added 3 commits June 5, 2023 13:51

[aspnetcore] OpenAPITools#9621 Removes useParameterDefaultValues to m…

ce1249e

…ake using default values standard behaviour - Adds additional documentation to explain why parameters are sorted with defaults at the end

Merge branch 'master' into feature/9621-aspnetcore-parameter-default-…

cba96ff

…values

[aspnetcore] OpenAPITools#9621 Updates samples and documentation

0cff07f

wing328 added Enhancement: General Server: C-Sharp labels Jun 6, 2023

wing328 added this to the 7.0.0 milestone Jun 6, 2023

ntwilkie-amici closed this Aug 21, 2023

ntwilkie-amici deleted the feature/9621-aspnetcore-parameter-default-values branch August 21, 2023 07:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/9621 aspnetcore parameter default values #15753

Feature/9621 aspnetcore parameter default values #15753

ntwilkie-amici commented Jun 5, 2023

wing328 Jun 5, 2023

ntwilkie-amici Jun 5, 2023

wing328 Jun 5, 2023

ntwilkie-amici Jun 5, 2023

wing328 Jun 6, 2023

devhl-labs Jun 6, 2023

wing328 Jun 5, 2023

ntwilkie-amici Jun 5, 2023

devhl-labs Jun 6, 2023

wing328 Jun 5, 2023

devhl-labs commented Jun 6, 2023

wing328 commented Jul 2, 2023

ntwilkie-amici commented Aug 21, 2023

Feature/9621 aspnetcore parameter default values #15753

Feature/9621 aspnetcore parameter default values #15753

Conversation

ntwilkie-amici commented Jun 5, 2023

PR checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devhl-labs commented Jun 6, 2023

wing328 commented Jul 2, 2023

ntwilkie-amici commented Aug 21, 2023