Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 11 commits into from
Jun 25, 2019
Merged

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
e79107f
First set of fixes to support ver 3.0, useswashbuckle does not work yet
A-Joshi May 4, 2019
07a9a15
Fix swashbuckle issues iwth aspnetcore 3.0
A-Joshi May 5, 2019
a99b3c7
Use default routing for 2.2 and turn off default for 3.0
A-Joshi May 5, 2019
e165cfe
fix up documentation
A-Joshi May 5, 2019
1ebc2fb
PR Feedback and wrong name in mustache file
A-Joshi May 5, 2019
e8a1597
Fix for 2.1 usage too
A-Joshi May 5, 2019
2864707
Change isFramework to useFrameworkReference as name to make thngs exp…
A-Joshi May 5, 2019
6e92c84
Make JSON.NET version configurable
A-Joshi Jun 6, 2019
2d5229e
Activate endpoint routing and use camel case NamingStrategy
A-Joshi Jun 6, 2019
8e0aa95
Make Newtonsoft version configurable to match ASP.NET Core preview 5
A-Joshi Jun 6, 2019
1c543c7
Fix spelling of an option, remove a duplicate call and update docs
A-Joshi Jun 9, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions docs/generators/aspnetcore.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,14 +16,18 @@ sidebar_label: aspnetcore
|packageVersion|C# package version.| |1.0.0|
|packageGuid|The GUID that will be associated with the C# project| |null|
|sourceFolder|source folder for generated code| |src|
|compatibilityVersion|ASP.Net Core CompatibilityVersion| |Version_2_1|
|aspnetCoreVersion|ASP.NET Core version: 2.2 (default), 2.1, 2.0 (deprecated)| |2.2|
|compatibilityVersion|ASP.Net Core CompatibilityVersion| |Version_2_2|
|aspnetCoreVersion|ASP.NET Core version: 3.0 (preview4 only), 2.2 (default), 2.1, 2.0 (deprecated)| |2.2|
|swashbuckleVersion|Swashbucke version: 3.0.0 (default), 4.0.0| |3.0.0|
|sortParamsByRequiredFlag|Sort method arguments to place required parameters before optional parameters.| |true|
|useDateTimeOffset|Use DateTimeOffset to model date-time properties| |false|
|useCollection|Deserialize array types to Collection<T> instead of List<T>.| |false|
|returnICollection|Return ICollection<T> instead of the concrete type.| |false|
|useSwashbuckle|Uses the Swashbuckle.AspNetCore NuGet package for documentation.| |true|
|isLibrary|Is the build a library| |false|
|isFramework|Use frameworkReference for ASP.NET Core 3.0+ and PackageReference ASP.NET Core 2.2 or earlier.| |false|
|useNewtonsoft|Uses the Newtonsoft JSN library.| |true|
A-Joshi marked this conversation as resolved.
Show resolved Hide resolved
|useDefaultRoutng|Use default routing for the ASP.NET Core version. For 3.0 turn off default because it is not yet supported.| |true|
A-Joshi marked this conversation as resolved.
Show resolved Hide resolved
A-Joshi marked this conversation as resolved.
Show resolved Hide resolved
|classModifier|Class Modifier can be empty, abstract| ||
|operationModifier|Operation Modifier can be virtual, abstract or partial| |virtual|
|buildTarget|Target to build an application or library| |program|
Expand Down
112 changes: 100 additions & 12 deletions ...i-generator/src/main/java/org/openapitools/codegen/languages/AspNetCoreServerCodegen.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ public class AspNetCoreServerCodegen extends AbstractCSharpCodegen {

public static final String USE_SWASHBUCKLE = "useSwashbuckle";
public static final String ASPNET_CORE_VERSION = "aspnetCoreVersion";
public static final String SWASHBUCKLE_VERSION = "swashbuckleVersion";
public static final String CLASS_MODIFIER = "classModifier";
public static final String OPERATION_MODIFIER = "operationModifier";
public static final String OPERATION_IS_ASYNC = "operationIsAsync";
Expand All @@ -51,6 +52,9 @@ public class AspNetCoreServerCodegen extends AbstractCSharpCodegen {
public static final String SDK_LIB = "Microsoft.NET.Sdk";
public static final String COMPATIBILITY_VERSION = "compatibilityVersion";
public static final String IS_LIBRARY = "isLibrary";
public static final String IS_FRAMEWORK = "isFramework";
public static final String USE_NEWtONSOFT = "useNewtonsoft";
A-Joshi marked this conversation as resolved.
Show resolved Hide resolved
public static final String USE_DEFAULT_ROUTING = "useDefaultRoutng";

private String packageGuid = "{" + randomUUID().toString().toUpperCase(Locale.ROOT) + "}";

Expand All @@ -60,18 +64,22 @@ public class AspNetCoreServerCodegen extends AbstractCSharpCodegen {
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");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

; // default to 2.1
protected CliOption aspnetCoreVersion = new CliOption(ASPNET_CORE_VERSION, "ASP.NET Core version: 3.0 (preview4 only), 2.2 (default), 2.1, 2.0 (deprecated)");
A-Joshi marked this conversation as resolved.
Show resolved Hide resolved
private CliOption classModifier = new CliOption(CLASS_MODIFIER, "Class Modifier can be empty, abstract");
private CliOption operationModifier = new CliOption(OPERATION_MODIFIER, "Operation Modifier can be virtual, abstract or partial");
private CliOption modelClassModifier = new CliOption(MODEL_CLASS_MODIFIER, "Model Class Modifier can be nothing or partial");
private boolean generateBody = true;
private CliOption buildTarget = new CliOption("buildTarget", "Target to build an application or library");
private String projectSdk = SDK_WEB;
private String compatibilityVersion = "Version_2_1";
private String compatibilityVersion = "Version_2_2";
private boolean operationIsAsync = false;
private boolean operationResultTask = false;
private boolean isLibrary = false;
private boolean isFramework = false;
private boolean useNewtonsoft = true;
private boolean useDefaultRoutng = true;

public AspNetCoreServerCodegen() {
super();
Expand Down Expand Up @@ -145,10 +153,17 @@ public AspNetCoreServerCodegen() {
aspnetCoreVersion.addEnum("2.0", "ASP.NET COre 2.0");
aspnetCoreVersion.addEnum("2.1", "ASP.NET Core 2.1");
aspnetCoreVersion.addEnum("2.2", "ASP.NET Core 2.2");
aspnetCoreVersion.addEnum("3.0", "ASP.NET Core 3.0");
aspnetCoreVersion.setDefault("2.2");
aspnetCoreVersion.setOptValue(aspnetCoreVersion.getDefault());
addOption(aspnetCoreVersion.getOpt(), aspnetCoreVersion.getDescription(), aspnetCoreVersion.getOptValue());

swashbuckleVersion.addEnum("3.0.0", "Swashbuckle 3.0.0");
swashbuckleVersion.addEnum("4.0.0", "Swashbuckle 4.0.0");
swashbuckleVersion.setDefault("3.0.0");
swashbuckleVersion.setOptValue(swashbuckleVersion.getDefault());
addOption(swashbuckleVersion.getOpt(), swashbuckleVersion.getDescription(), swashbuckleVersion.getOptValue());

// CLI Switches
addSwitch(CodegenConstants.SORT_PARAMS_BY_REQUIRED_FLAG,
CodegenConstants.SORT_PARAMS_BY_REQUIRED_FLAG_DESC,
Expand All @@ -174,6 +189,18 @@ public AspNetCoreServerCodegen() {
"Is the build a library",
isLibrary);

addSwitch(IS_FRAMEWORK,
"Use frameworkReference for ASP.NET Core 3.0+ and PackageReference ASP.NET Core 2.2 or earlier.",
isFramework);

addSwitch(USE_NEWtONSOFT,
"Uses the Newtonsoft JSN library.",
useNewtonsoft);

addSwitch(USE_DEFAULT_ROUTING,
"Use default routing for the ASP.NET Core version. For 3.0 turn off default because it is not yet supported.",
useDefaultRoutng);

classModifier.addEnum("", "Keep class default with no modifier");
classModifier.addEnum("abstract", "Make class abstract");
classModifier.setDefault("");
Expand Down Expand Up @@ -274,6 +301,10 @@ public void processOpts() {

// determine the ASP.NET core version setting
setAspnetCoreVersion(packageFolder);
setSwashbuckleVersion();
setIsFramework();
setUseNewtonsoft();
setUseEndpointRouting();

supportingFiles.add(new SupportingFile("build.sh.mustache", "", "build.sh"));
supportingFiles.add(new SupportingFile("build.bat.mustache", "", "build.bat"));
Expand Down Expand Up @@ -445,16 +476,16 @@ private void setBuildTarget() {

private void setAspnetCoreVersion(String packageFolder) {
setCliOption(aspnetCoreVersion);
if ("2.0".equals(aspnetCoreVersion.getOptValue())) {
embeddedTemplateDir = templateDir = "aspnetcore/2.0";
supportingFiles.add(new SupportingFile("web.config", packageFolder, "web.config"));
LOGGER.info("ASP.NET core version: 2.0");
compatibilityVersion = null;
} else {
// default, do nothing
LOGGER.info("ASP.NET core version: " + aspnetCoreVersion.getOptValue());
compatibilityVersion = "Version_" + aspnetCoreVersion.getOptValue().replace(".", "_");
}
if ("2.0".equals(aspnetCoreVersion.getOptValue())) {
A-Joshi marked this conversation as resolved.
Show resolved Hide resolved
embeddedTemplateDir = templateDir = "aspnetcore/2.0";
supportingFiles.add(new SupportingFile("web.config", packageFolder, "web.config"));
LOGGER.info("ASP.NET core version: 2.0");
compatibilityVersion = null;
} else {
// default, do nothing
LOGGER.info("ASP.NET core version: " + aspnetCoreVersion.getOptValue());
compatibilityVersion = "Version_" + aspnetCoreVersion.getOptValue().replace(".", "_");
}
additionalProperties.put(COMPATIBILITY_VERSION, compatibilityVersion);
}

Expand Down Expand Up @@ -482,4 +513,61 @@ private void setOperationIsAsync() {
additionalProperties.put(OPERATION_IS_ASYNC, operationIsAsync);
}
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

LOGGER.warn("ASP.NET core version is " + aspnetCoreVersion.getOptValue() + " so changing to use frameworkReference instead of packageReference ");
A-Joshi marked this conversation as resolved.
Show resolved Hide resolved
isFramework = true;
additionalProperties.put(IS_FRAMEWORK, isFramework);
} else {
if (additionalProperties.containsKey(IS_FRAMEWORK)) {
isFramework = convertPropertyToBooleanAndWriteBack(IS_FRAMEWORK);
} else {
additionalProperties.put(IS_FRAMEWORK, isFramework);
}
}

}

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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

additionalProperties.put(USE_NEWtONSOFT, useNewtonsoft);
} else {
if (additionalProperties.containsKey(USE_NEWtONSOFT)) {
useNewtonsoft = convertPropertyToBooleanAndWriteBack(USE_NEWtONSOFT);
} else {
additionalProperties.put(USE_NEWtONSOFT, useNewtonsoft);
}
}
}

private void setUseEndpointRouting() {
if (aspnetCoreVersion.getOptValue().startsWith("2.")) {
LOGGER.warn("ASP.NET core version is " + aspnetCoreVersion.getOptValue() + " so staying on default endpoint routing.");
useDefaultRoutng = true;
additionalProperties.put(USE_DEFAULT_ROUTING, useDefaultRoutng);
} else {
if (additionalProperties.containsKey(USE_NEWtONSOFT)) {
useDefaultRoutng = convertPropertyToBooleanAndWriteBack(USE_DEFAULT_ROUTING);
} else {
additionalProperties.put(USE_DEFAULT_ROUTING, useDefaultRoutng);
}
}
}

private void setSwashbuckleVersion() {
setCliOption(swashbuckleVersion);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


if (aspnetCoreVersion.getOptValue().startsWith("3.")) {
LOGGER.warn("ASP.NET core version is " + aspnetCoreVersion.getOptValue() + " so changing default Swashbuckle version to 4.0.0.");
swashbuckleVersion.setOptValue("4.0.0");
additionalProperties.put(SWASHBUCKLE_VERSION, swashbuckleVersion.getOptValue());
} else {
// default, do nothing
LOGGER.info("Swashbuckle version: " + swashbuckleVersion.getOptValue());
}
}
}
14 changes: 12 additions & 2 deletions modules/openapi-generator/src/main/resources/aspnetcore/2.1/Project.csproj.mustache
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,20 @@
<PackageId>{{packageName}}</PackageId>
</PropertyGroup>
<ItemGroup>
{{#isFramework}}
{{#isLibrary}}
<FrameworkReference Include="Microsoft.AspNetCore.App" />
{{/isLibrary}}
{{/isFramework}}
{{^isFramework}}
<PackageReference Include="Microsoft.AspNetCore.App" />
{{/isFramework}}
{{#useNewtonsoft}}
<PackageReference Include="Microsoft.AspNetCore.Mvc.NewtonsoftJson" Version="3.0.0-preview4-19216-03" />
{{/useNewtonsoft}}
{{#useSwashbuckle}}
<PackageReference Include="Swashbuckle.AspNetCore" Version="3.0.0"/>
<PackageReference Include="Swashbuckle.AspNetCore.Annotations" Version="3.0.0" />
<PackageReference Include="Swashbuckle.AspNetCore" Version="{{swashbuckleVersion}}"/>
<PackageReference Include="Swashbuckle.AspNetCore.Annotations" Version="{{swashbuckleVersion}}" />
{{/useSwashbuckle}}
</ItemGroup>
<ItemGroup>
Expand Down
4 changes: 2 additions & 2 deletions modules/openapi-generator/src/main/resources/aspnetcore/2.1/Startup.mustache
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,11 +42,11 @@ namespace {{packageName}}
{
// Add framework services.
services
.AddMvc()
.AddMvc({{^useEndpointRouting}}opts => opts.EnableEndpointRouting = false{{/useEndpointRouting}})
A-Joshi marked this conversation as resolved.
Show resolved Hide resolved
{{#compatibilityVersion}}
.SetCompatibilityVersion(CompatibilityVersion.{{compatibilityVersion}})
{{/compatibilityVersion}}
.AddJsonOptions(opts =>
.{{#useNewtonsoft}}AddNewtonsoftJson{{/useNewtonsoft}}{{^useNewtonsoft}}AddJsonOptions{{/useNewtonsoft}}(opts =>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see the earlier comment about setting the value of useNewtonsoft.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
opts.SerializerSettings.ContractResolver = new CamelCasePropertyNamesContractResolver();
opts.SerializerSettings.Converters.Add(new StringEnumConverter
Expand Down