[aspnetcore]: Preliminary support for ASP.NET 3.0 Core preview 5 #2824

A-Joshi · 2019-05-05T13:12:55Z

PR checklist

Read the contribution guidelines.
Ran the shell script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh, ./bin/openapi3/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in .\bin\windows\. If contributing template-only or documentation-only changes which will change sample output, be sure to build the project first.
Filed the PR against the correct branch: master~~, 3.4.x, 4.0.x~~. Default: master.
Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

Preliminary support for ASP.NET Core 3.0 (preview 4). See issue #2350

No support for the new style routing or new Json library (still uses Newtonsoft).

docs/generators/aspnetcore.md

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

etherealjoy · 2019-05-05T13:29:57Z

I thought the signature got changed here

public void Configure(IApplicationBuilder app,  IWebHostEnvironment env)

modules/openapi-generator/src/main/resources/aspnetcore/2.1/Startup.mustache

etherealjoy · 2019-05-05T13:39:48Z

Does it makes sense to use static files when swashbuckle is not used? In pure WebApi scenarios?

A-Joshi · 2019-05-05T14:28:50Z

I thought the signature got changed here
public void Configure(IApplicationBuilder app,  IWebHostEnvironment env)

Yes - but the old style is still available with a deprecation warning:
"Startup.cs(67,56): warning CS0618: 'IHostingEnvironment' is obsolete: 'This type is obsolete and will be removed in a future version. The recommended alternative is Microsoft.AspNetCore.Hosting.IWebHostEnvironment.' [/Src/contract-first/contracts/aspnetcore-server-task-async/target/contracts/src/Contracts/Contracts.csproj]"

There are a couple more such warnings that will have to be cleaned up (aside from support for new routing and json libraries) before it is fully ready.

etherealjoy · 2019-05-05T15:29:08Z

@ALL
If it breaks current 2.2 version does not it make sense then to use a different version instead for the 3.0?
This would make it easier to maintain and also use the new features that come with the 3.0 version

A-Joshi · 2019-05-05T15:33:27Z

@ALL
If it breaks current 2.2 version does not it make sense then to use a different version instead for the 3.0?
This would make it easier to maintain and also use the new features that come with the 3.0 version

Does not break 2.2 - did not want to send the PR till the break was fixed.

jimschubert · 2019-05-05T16:06:13Z

edit: n/m. I read the issue before looking at the PR.

jimschubert

Thanks for this PR. It's looking pretty good. I think the biggest issue is that newtonsoft is still referenced, although it's possible to exclude the library. I had a handful of other logic or user (of the generator) clarifications.

docs/generators/aspnetcore.md

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

jimschubert · 2019-05-05T16:12:42Z

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

@@ -60,18 +64,22 @@
    private boolean useSwashbuckle = true;
    protected int serverPort = 8080;
    protected String serverHost = "0.0.0.0";
-    protected CliOption aspnetCoreVersion = new CliOption(ASPNET_CORE_VERSION, "ASP.NET Core version: 2.2 (default), 2.1, 2.0 (deprecated)");
+    protected CliOption swashbuckleVersion = new CliOption(SWASHBUCKLE_VERSION, "Swashbucke version: 3.0.0 (default), 4.0.0");


We should remove default version definition from being embedded in the help text. This will already be displayed via config-help:

swashbuckleVersion Swashbucke version: 3.0.0 (default), 4.0.0 (Default: 3.0.0)

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

jimschubert · 2019-05-05T16:17:14Z

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

+
+    private void setIsFramework() {
+        if (aspnetCoreVersion.getOptValue().startsWith("3.")) {
+            // default, do nothing


this block is setting a field and has a side effect of assigning a key/value in additionalProperties. Saying "do nothing" could be confusing. Similarly, "3.x" isn't the default defined in this type and could be confusing. Should probably remove comment since the log message below provides runtime and in-code explanation.

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

jimschubert · 2019-05-05T16:26:45Z

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

+    private void setUseNewtonsoft() {
+        if (aspnetCoreVersion.getOptValue().startsWith("2.")) {
+            LOGGER.warn("ASP.NET core version is " + aspnetCoreVersion.getOptValue() + " so staying on default json library.");
+            useNewtonsoft = false;


I feel like there's either a typo here or on the conditional a couple lines above. Is it intentional to disallow JSON.net in aspnetcore 2.x? If so, this may be considered a breaking change and should probably be done separately from this PR to generate discussion.

The change is not a breaking change:

This one is a little tricky - the MVC configuration in 2.x and 3.x use AddJsonOptions but mean different things: the default JSON in 2.x is implicitly Newtonsoft, while in 3.x and later it will be the Microsoft one.

I can change it use something like useDefaultJson or jsonLibrary. I don't like the former since the default in different cases means different things. Maybe explicitly defining the jsonLibrary as Newtonsoft or Microsoft but that makes the mustache file complex - since mustache does not have conditionals based on values.

What are your thoughts there?

My main concern is that for 2.x, when useNewtonSoft is false, this results in the NewtonSoft library not being referenced, but is still used on controllers and models. From a maintainability perspective (both built-in templates and custom user templates), I find that very confusing.

For 2.x NewtonSoft is used by the framework by default itself because it is part of the fw, but MS removed it from the framework starting from 3.x and manual addition of package is needed.
Which in this case the user really has no choice to not use NewtonSoft in 2.x
Since we would need to use NewtonSoft for model compatibility reasons it does not makes sense to allow the user to set this flag because if the fw is > 3.x we will need to add it

jimschubert · 2019-05-05T16:27:12Z

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

+    }
+
+    private void setSwashbuckleVersion() {
+        setCliOption(swashbuckleVersion);


Long comment, but minor (can be addressed later).

Only the CLI should be setting CLI options. We should read options once (in processOpts), then have nothing to do with CLIOption types. That is, we should load into fields and work from those fields directly. I'm concerned that making this method side-effecting as it is and assuming options parsing within the generator, things can become out of sync.

For example, a user can pass --additional-properties=swashbuckleVersion=3.0.5". Although this isn't defined in the enum of available versions, this is a valid use case that we allow for properties defined in additionalProperties; this is why you often see the guard that we only set the value if it doesn't already exist. CodegenConfiguratoralso allows for JSON deserialization into theCodegenConfigurator` type (to load these values from a config), which sorta bypasses both the cli options and additional properties (see CodegenConfigurator).

In all of these cases, this method will reset the user-provided value to whatever is originally hardcoded as the swashbuckleVersion default within the generator.

The other choice here is to check the version of Swashbuckle is valid for the ASP.NET Core chosen and throw an error if the version is not valid.

In this particular case I think you can use v 4.0.0 with version 2.x and 3.x but you cannot use v3.0.0 with 3.x and later.

What are you thoughts here - error or warn and correct - not sure what the general policy for this is.

I think my preference would be to set a default if one doesn't exist, and warn if it's something we don't expect will work. We'll probably want to be clear that anything below 4.0.0 may not work, but trust that the user understands the versioning. For example, someone might have a fork of Swashbuckle which does work, and they could reference and add the file with the reference to .openapi-generator-ignore. In that case, a warn+correct would be unexpected if the value was explicitly provided.

jimschubert · 2019-05-05T16:46:38Z

modules/openapi-generator/src/main/resources/aspnetcore/2.1/Startup.mustache

                {{#compatibilityVersion}}
                .SetCompatibilityVersion(CompatibilityVersion.{{compatibilityVersion}})
                {{/compatibilityVersion}}
-                .AddJsonOptions(opts =>
+                .{{#useNewtonsoft}}AddNewtonsoftJson{{/useNewtonsoft}}{{^useNewtonsoft}}AddJsonOptions{{/useNewtonsoft}}(opts =>


Variable useNewtonsoft will also need to be evaluated in controller.mustache and model.mustache. Currently, the artifact won't be loaded, but controllers and models are still tightly coupled to json serialize/deserialize using the library.

see the earlier comment about setting the value of useNewtonsoft.

I'm not sure I follow.

To put the concern a different way, as a user if I set useNewtonSoft=false, and the controller/model generation still includes NewtonSoft imports, I'd consider that a bug. Maybe the variable name is what's misleading here.

Indeed, since the models are generated with Netwonsoft in mind it is required that in 3.x it is included via the package dependency and for 2.x is there as part of core fw.

…licitly clear. Also fix small messages for the review comments

A-Joshi · 2019-05-05T18:08:23Z

I am beginning to think that instead of automatically correcting the value if invalid, then just throw an error. It will simplify some of the logic.

The PR does not address supporting the new features of 3.0 - it simply ensure that you can use 3.0 in a backward compatible way.

Thoughts?

etherealjoy · 2019-05-06T08:13:56Z

The PR does not address supporting the new features of 3.0 - it simply ensure that you can use 3.0 in a backward compatible way.

Here is what I also would like to hear the opinion. Because if we use the new features of 3.0 we will have to use a new template otherwise maintaining backwards compatibility will be too much.
Besides it also make contribution from more people easier to manage due to lesser chance of breaking 2.x

wing328 · 2019-05-15T13:22:24Z

What about merging this PR into master and then ask the community for feedback?

etherealjoy · 2019-05-15T13:28:16Z

This is not going to work yet. Routing changes are missing.
I think we can stick with JSON.NET instead of Utf8JsonReader/Writer. Utf8JsonReader is still lacking many features.
Additionally I believe ASP.NET Core 3.0 brings so many new things that it makes more sense to have it as a separate generator. e.g use ControllerWithView and Razor at the same time instead of Swashbucke with Mvc in the past.

etherealjoy · 2019-06-03T20:37:05Z

@wing328 @A-Joshi
I would like to give to give feedback after 3 weeks of using ASP.NET Core 3.0 Preview 5.

Utf8JsonReader is not feature complete so we are better off for now using JSON.NET
API Controllers and Controller with views use the Utf8JsonReader so incompatibility with net core client models. So we should continue to use ASP.NetCore MVC package
We can add Razor to to MVC so not an issue there.
We can continue to use the same generator because the diff from here mainly is the Configure.

New changes needed

In Configure, endpoint routing has to be activated
JSON.NET has to be added a an external dependency in .csproj
JSON camel case NamingStartegy has to use instead of deprecated option.
docker alpine runtime is available
rest is same :)

A-Joshi · 2019-06-04T13:28:21Z

Thanks - will take a look this week. Was out of action for some personal reasons.

…

On 6/3/19 4:37 PM, sunn wrote: @wing328 <https://github.com/wing328> @A-Joshi <https://github.com/A-Joshi> I would like to give to give feedback after 3 weeks of using ASP.NET Core 3.0 Preview 5. * |Utf8JsonReader| is not feature complete so we are better off for now using |JSON.NET| * API Controllers and Controller with views use the Utf8JsonReader so incompatibility with net core client models. So we should continue to use ASP.NetCore MVC package * We can add Razor to to MVC so not an issue there. * We can continue to use the same generator because the diff from here mainly is the Configure. New changes needed * In |Configure|, endpoint routing has to be activated * |JSON.NET| has to be added a an external dependency in |.csproj| * JSON camel case NamingStartegy has to use instead of deprecated option. * docker alpine runtime is available * rest is same :) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2824?email_source=notifications&email_token=ADKAIINE3UG6LNGQSIXXINLPYV6HNA5CNFSM4HK3HBRKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODW2T2TY#issuecomment-498416975>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADKAIIONWOZI6Y3RXOMUMZ3PYV6HNANCNFSM4HK3HBRA>.

A-Joshi · 2019-06-05T01:39:38Z

Could you give sample of what the changes you needed to do were?

@wing328 @A-Joshi
New changes needed
* In `Configure`, endpoint routing has to be activated

What did you have to change? parameter value to be set?

* `JSON.NET` has to be added a an external dependency in `.csproj`

What was the dependency you needed to add? I think its already there.

* JSON camel case NamingStartegy has to use instead of deprecated option.

Not sure what the change needed was?

* docker alpine runtime is available

Can look at this for the next round.

* rest is same :)

Can we get the PR added in ? It does not break 2.x and it will be easier to work against the master once this is merged in.

etherealjoy · 2019-06-05T08:38:37Z

I did these

In Configure, endpoint routing has to be activated

In Configure, shown here without swashbuckle

public void Configure(IApplicationBuilder app,  IWebHostEnvironment env)
{
	app.UseHttpsRedirection();
	app.UseRouting();
	app.UseEndpoints(endpoints =>
	{
		endpoints.MapControllers();
	});	
	if (env.IsDevelopment())
	{
		app.UseDeveloperExceptionPage();
	}
	else
	{
		app.UseHsts();
	}
}

JSON.NET has to be added a an external dependency in .csproj

In .csproj file

<PackageReference Include="Microsoft.AspNetCore.Mvc.NewtonsoftJson" 
          Version="3.0.0-preview5-19227-01" />

JSON camel case NamingStrategy has to use instead of deprecated option.

using Newtonsoft.Json.Converters;
using Newtonsoft.Json.Serialization;


public void ConfigureServices(IServiceCollection services)
{
	// Add framework services.
	services
		.AddMvc()
		.SetCompatibilityVersion(CompatibilityVersion.Version_3_0)
		.AddNewtonsoftJson(opts =>
			{
				opts.SerializerSettings.ContractResolver = 
						new CamelCasePropertyNamesContractResolver();
				opts.SerializerSettings.Converters.Add(new StringEnumConverter
				{
					NamingStrategy = new CamelCaseNamingStrategy()
				});
			}
		);

}

docker alpine runtime is available

In Dockerfile
FROM mcr.microsoft.com/dotnet/core/aspnet:3.0-alpine AS runtime

A-Joshi · 2019-06-06T01:54:42Z

@etherealjoy:
Added the parts you found above except the docker image - have to give it a little thought whether we should make the whole base docker image name configurable or pass in the parameter like alpine.

Can you test the new push please?

@wing328, @jimschubert - Assuming @etherealjoy is good then I suggest we merge it in - there are no changes for the version 2.1/2.2 while this will add preliminary support for 3.0 preview 5 in compatibility mode.

modules/openapi-generator/src/main/resources/aspnetcore/2.1/Startup.mustache

etherealjoy · 2019-06-06T07:15:13Z

I will try this out and give feedback.
And thank you for the updates :)

jimschubert · 2019-06-07T16:54:30Z

Regarding the recommendation of having 3.0 be a separate generator…

Rather than conditionally assigning locations for files between versions (e.g. 2.2/filename.mustache and 3.0/filename.mustache), why not just use the core generation's library feature? These are different versions of the same library, but there's no reason we can't use that option to reference library versions. This allows us to put common code under resources/aspnetcore and any version-specific overrides under their respective library folders. This would be a breaking change for anyone who has extended templates, so such a change would have to be done in release 4.1.0. This is just a thought about managing the concern of maintainability as Microsoft makes breaking changes between versions. I think it would be much easier to manage these as libraries rather than completely separate generators.

A-Joshi · 2019-06-07T17:11:41Z

Sounds like a good idea - for now though I would like to keep the current PR focused on the current changes and then add new changes/ideas later via different PR.

…

On 6/7/19 12:54 PM, Jim Schubert wrote: Regarding the recommendation of having 3.0 be a separate generator… Rather than conditionally assigning locations for files between versions (e.g. |2.2/filename.mustache| and |3.0/filename.mustache|), why not just use the core generation's |library| feature? These are different versions of the same library, but there's no reason we can't use that option to reference library versions. This allows us to put common code under |resources/aspnetcore| and any version-specific overrides under their respective library folders. This would be a breaking change for anyone who has extended templates, so such a change would have to be done in release 4.1.0. This is just a thought about managing the concern of maintainability as Microsoft makes breaking changes between versions. I think it would be much easier to manage these as libraries rather than completely separate generators. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2824?email_source=notifications&email_token=ADKAIIIYD4XE4IQA4ZF5UK3PZKHEZA5CNFSM4HK3HBRKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXGMPQI#issuecomment-499959745>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADKAIIPD72AHSDPOYWBCQ7TPZKHEZANCNFSM4HK3HBRA>.

modules/openapi-generator/src/main/resources/aspnetcore/2.1/Startup.mustache

etherealjoy · 2019-06-08T15:06:35Z

I tried this and it works using these options.

java -jar modules/openapi-generator-cli/target/openapi-generator-cli.jar generate -i \ 
/home/dev/api/swagger.yaml -o /home/dev/gen --additional-properties  \
aspnetCoreVersion=3.0,useNewtonsoft=true,useDefaultRoutng=false,useSwashbuckle=false \ 
-g aspnetcore

However SwashBuckle on .Net Core 3.0 needs version 5.0.
domaindrivendev/Swashbuckle.AspNetCore#1088
So Swashbuckle needs an update for .Net Core 3.0
#2646

docs/generators/aspnetcore.md

A-Joshi · 2019-06-09T02:21:12Z

You can change the swashbuckle version using the parameter "swashbuckleVersion=4.0.0" I have added 5.0.0 as a possible value but have not tested with that.

A-Joshi · 2019-06-18T01:52:30Z

@etherealjoy are you OK with it now?
@jimschubert @wing328 can we merge this one?

etherealjoy · 2019-06-18T13:01:29Z

Ok from my side.

etherealjoy · 2019-06-18T13:02:22Z

Update to swashbuckle 5 is needed to use it with 3.0 but we can do this later.

wing328 · 2019-06-25T06:39:33Z

Let's merge this and gather feedback from the community.

A-Joshi added 4 commits May 4, 2019 08:21

First set of fixes to support ver 3.0, useswashbuckle does not work yet

e79107f

Fix swashbuckle issues iwth aspnetcore 3.0

07a9a15

Use default routing for 2.2 and turn off default for 3.0

a99b3c7

fix up documentation

e165cfe

A-Joshi mentioned this pull request May 5, 2019

[REQ] New ASP.NET Core 3.0 server generator #2350

Closed

A-Joshi changed the title ~~Feature/version3~~ [aspnetcore]: Preliminary support for ASP.NET 3.0 preview 4 May 5, 2019

etherealjoy reviewed May 5, 2019

View reviewed changes

docs/generators/aspnetcore.md Outdated Show resolved Hide resolved

etherealjoy reviewed May 5, 2019

View reviewed changes

...napi-generator/src/main/java/org/openapitools/codegen/languages/AspNetCoreServerCodegen.java Outdated Show resolved Hide resolved

etherealjoy reviewed May 5, 2019

View reviewed changes

modules/openapi-generator/src/main/resources/aspnetcore/2.1/Startup.mustache Outdated Show resolved Hide resolved

etherealjoy changed the title ~~[aspnetcore]: Preliminary support for ASP.NET 3.0 preview 4~~ [aspnetcore]: Preliminary support for ASP.NET 3.0 Core preview 4 May 5, 2019

A-Joshi added 2 commits May 5, 2019 10:35

PR Feedback and wrong name in mustache file

1ebc2fb

Fix for 2.1 usage too

e8a1597

jimschubert reviewed May 5, 2019

View reviewed changes

Change isFramework to useFrameworkReference as name to make thngs exp…

2864707

…licitly clear. Also fix small messages for the review comments

wing328 added Enhancement: Compatibility Server: C-Sharp labels May 14, 2019

A-Joshi added 3 commits June 5, 2019 20:45

Make JSON.NET version configurable

6e92c84

Activate endpoint routing and use camel case NamingStrategy

2d5229e

Make Newtonsoft version configurable to match ASP.NET Core preview 5

8e0aa95

etherealjoy reviewed Jun 6, 2019

View reviewed changes

modules/openapi-generator/src/main/resources/aspnetcore/2.1/Startup.mustache Show resolved Hide resolved

A-Joshi changed the title ~~[aspnetcore]: Preliminary support for ASP.NET 3.0 Core preview 4~~ [aspnetcore]: Preliminary support for ASP.NET 3.0 Core preview 5 Jun 6, 2019

etherealjoy reviewed Jun 8, 2019

View reviewed changes

modules/openapi-generator/src/main/resources/aspnetcore/2.1/Startup.mustache Outdated Show resolved Hide resolved

etherealjoy reviewed Jun 8, 2019

View reviewed changes

docs/generators/aspnetcore.md Outdated Show resolved Hide resolved

Fix spelling of an option, remove a duplicate call and update docs

1c543c7

wing328 merged commit 94c583b into OpenAPITools:master Jun 25, 2019

wing328 added this to the 4.0.3 milestone Jun 25, 2019

[aspnetcore]: Preliminary support for ASP.NET 3.0 Core preview 5 #2824

[aspnetcore]: Preliminary support for ASP.NET 3.0 Core preview 5 #2824

Conversation

A-Joshi commented May 5, 2019

PR checklist

Description of the PR

etherealjoy commented May 5, 2019

etherealjoy commented May 5, 2019

A-Joshi commented May 5, 2019

etherealjoy commented May 5, 2019 • edited Loading

A-Joshi commented May 5, 2019

jimschubert commented May 5, 2019 • edited Loading

jimschubert left a 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

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

A-Joshi commented May 5, 2019 • edited Loading

etherealjoy commented May 6, 2019

wing328 commented May 15, 2019

etherealjoy commented May 15, 2019

etherealjoy commented Jun 3, 2019

A-Joshi commented Jun 4, 2019 via email

A-Joshi commented Jun 5, 2019

etherealjoy commented Jun 5, 2019

A-Joshi commented Jun 6, 2019

etherealjoy commented Jun 6, 2019 • edited Loading

jimschubert commented Jun 7, 2019

A-Joshi commented Jun 7, 2019 via email

etherealjoy commented Jun 8, 2019 • edited Loading

A-Joshi commented Jun 9, 2019

A-Joshi commented Jun 18, 2019

etherealjoy commented Jun 18, 2019

etherealjoy commented Jun 18, 2019

wing328 commented Jun 25, 2019

etherealjoy commented May 5, 2019 •

edited

Loading

jimschubert commented May 5, 2019 •

edited

Loading

A-Joshi commented May 5, 2019 •

edited

Loading

etherealjoy commented Jun 6, 2019 •

edited

Loading

etherealjoy commented Jun 8, 2019 •

edited

Loading