[csharp] clean boolean additional properties #6899

jimschubert · 2017-11-07T05:19:22Z

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 and ./bin/security/{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\.
Filed the PR against the correct branch: 3.0.0 branch for changes related to OpenAPI spec 3.0. Default: master.
Copied the technical committee to review the pull request if your PR is targeting a particular programming langauge.

Description of the PR

This is a general cleanup of how we store booleans on additionalProperties. It seems as though either "false" used to be treated in template as false or we were more consistent in replacing false strings with false booleans on the additionalProperties object in the C# generators. I personally implemented and tested the functionality in the linked issue, so I'm not sure why something like {{^property}} is generated regardless of the boolean value now (unless I was crazy when I implemented it 💃 ). I tried to determine why some functionality that has worked before (e.g. nonPublicApi) doesn't work in 2.2.3 but I was unable to track down any issue. I assume a git bisect would take quite a long time, and handling booleans explicitly seemed both easier and a best practice.

I've also taken this opportunity to expose both supportsAsync and validatable as options on the C# generator, since these are valid user-defined settings.

I've also added some warnings to clarify when the generator removes an explicit setting due to lack of support in the target framework or template. This removal of settings may not be ideal, but my changes in this PR were done to remain consistent with other settings which remove values.

see #6784

It appears as though "false" strings in additionalProperties are no longer treated as false booleans. This may be an issue elsewhere, but a simple fix is to always explicitly set the boolean value in a generator class back to the additionalProperties map to convert boolean Strings to boolean Objects.

Some functionality is missing from .NET 4.0, such as IReadonlyDictionary and Type.GetTypeInfo(). This commit resolves compilation of generated .NET 4.0 code, requiring no conditional versioning of Newtonsoft.Json.

jimschubert · 2017-11-08T03:38:54Z

I've resolved compilation of .NET 4.0 sample via Jetbrains Rider. The build.sh script complains about ambiguity:

error CS0104: `ReadOnlyDictionary' is an ambiguous reference between `System.Collections.ObjectModel.ReadOnlyDictionary<TKey,TValue>' and `IO.Swagger.Client.ReadOnlyDictionary<T,K>'

I'll need to look more closely and see why this error occurs, considering ReadOnlyDictionary wasn't introduced until .net 4.5.

Sample build.sh wasn't accounting for targeting different FCL correctly. That is, when passing "net40" to the -sdk option, it would use the default -sdk:4 and -langversion:6. These don't necessarily match with what is installed on a machine with only .NET 4.0 (which is our targeted use case here). To resolve, we need to define another version-specific value for passing to the mcs -sdk option (see man mcs for details). This option currently isn't overridable in the client codegen class. Also, langversion is set specifically to the version of C# available to the targeted SDK version. If there is need, we may extend this to something like: langversion=${MCS_LANG_VERSION:-6} To allow users to run as: env MCS_LANG_VERSION=5 sh build.sh I haven't done this because I doubt there's much of a use case via this script. I'm assuming most consumers will build via IDE or MSBuild.

jimschubert · 2017-11-09T05:09:06Z

So the issue was that the targetFramework (used in project files) was being used for the mcs -sdk option's value. These aren't the same values, or the same format. For example, net40 -> 4.

To resolve this issue, I've explicitly added a translation value to additionalProperties so we can use the correct mono SDK for the selected target framework. The issue from my previous comment about ReadOnlyDictionary ambiguities between the implementation I added and that of System.Collections.ObjectModel.ReadOnlyDictionary had to do with mono falling back to the default SDK.

Once I resolved this issue, I found that specifying -sdk:4 correctly also caused mono to specify the C# language version correctly and broke compilation because Rider auto-created delegates for the ReadOnlyDictionary using nameof. Oddly, I had different behaviors with mono 4.8 and mono 5.4.1.6. Rather than fix this one-off error, I've explicitly mapped langversion based on the sdk value passed.

After making these changes and cleaning up some of the bash scripts under bin, each sample that can be built with a build.sh script now build successfully using latest mono.

I know a lot of the commits in the PR are unrelated to the sanitization of booleans, but I figured the build cleanup was directly related.

jimschubert · 2017-11-09T05:12:45Z

bin/csharp-petstore.sh

@@ -26,6 +26,6 @@ fi

 # if you've executed sbt assembly previously it will use that instead.
 export JAVA_OPTS="${JAVA_OPTS} -XX:MaxPermSize=256M -Xmx1024M -DloggerPath=conf/log4j.properties"
-ags="generate $@ -i modules/swagger-codegen/src/test/resources/2_0/petstore-with-fake-endpoints-models-for-testing.yaml -l csharp -o samples/client/petstore/csharp/SwaggerClientNet40 --additional-properties packageGuid={321C8C3F-0156-40C1-AE42-D59761FB9B6C} -c ./bin/csharp-petstore-net-40.json"
+ags="generate $@ -i modules/swagger-codegen/src/test/resources/2_0/petstore-with-fake-endpoints-models-for-testing.yaml -l csharp -o samples/client/petstore/csharp/SwaggerClient --additional-properties packageGuid={321C8C3F-0156-40C1-AE42-D59761FB9B6C}"


@wing328 Your commit when you implemented .NET 4.0 generator added csharp-petstore-net-40.sh as well as modifying this file. It looked to me like you may not have meant to make csharp-petstore.sh compile .NET 4.0?

Feel free to revert this change and the changes in csharp-petstore-all.sh.

I would actually be cool with making this file compile .NET 4.5 as the minimal example (since 4.5 is an in-place update to 4.0 anyway), and create a separate script for .NET 3.5.

I was my mistake. Thanks for fixing it.

jimschubert · 2017-11-09T05:16:03Z

modules/swagger-codegen/src/main/java/io/swagger/codegen/languages/CSharpClientCodegen.java

@@ -17,21 +18,33 @@
    private static final String NET45 = "v4.5";
    private static final String NET40 = "v4.0";
    private static final String NET35 = "v3.5";
+    // TODO: v5.0 is PCL, not netstandard version 1.3, and not a specific .NET Framework. This needs to be updated,
+    // especially because it will conflict with .NET Framework 5.0 when released, and PCL 5 refers to Framework 4.0.


PCL 5.0 actually refers to .NET Framework 4.6 and higher, .NET Core 1.0 and Windows Universal 10.0. I commented this correctly later in the file. As a TODO, I don't think it's a huge issue.

I'd like to straighten this out before 2.3.0 release.

Yup, 2.3.0 is a release with breaking changes so please feel free to fix these wordings before the release.

jimschubert · 2017-11-09T05:20:44Z

modules/swagger-codegen/src/main/java/io/swagger/codegen/languages/CSharpClientCodegen.java

        } else if (NETSTANDARD.equals(this.targetFramework)) {
+            // TODO: NETSTANDARD here is misrepresenting a PCL v5.0 which supports .NET Framework 4.6+, .NET Core 1.0, and Windows Universal 10.0
+            additionalProperties.put(MCS_NET_VERSION_KEY, "4.6-api");


NOTE: I added this, although this NETSTANDARD isn't writing build.sh at the moment.

This reverts commit 2c9f98c.

* Revert "[csharp] clean boolean additional properties 6784 (#6899)" This reverts commit 2c9f98c. * Revert "[Swift4] Add throw to reserved words (#6952)" This reverts commit 970de01. * Revert "add a docker build tag for pushing docker image instead of just latest (#6837)" This reverts commit 4e482ee.

jimschubert added 4 commits November 6, 2017 23:43

[nancyfx] Clean up async default option handling

6c4a32c

[nancyfx] Include asyncServer=false in sample script

a8192e6

[csharp] Regenerate samples

59cde60

jimschubert mentioned this pull request Nov 7, 2017

[CSharp] Issue Generating client when nonPublicApi = false #6784

Closed

jimschubert added 2 commits November 7, 2017 22:31

[csharp] Resolve .net 4 generation issues

9a07807

Some functionality is missing from .NET 4.0, such as IReadonlyDictionary and Type.GetTypeInfo(). This commit resolves compilation of generated .NET 4.0 code, requiring no conditional versioning of Newtonsoft.Json.

[csharp] Regenerate .net 4.0 sample

2b50b24

jimschubert added 5 commits November 8, 2017 23:30

[csharp] Revert bin/csharp-petstore.sh to 3.5

681b5f8

[csharp] Regenerate .NET 3.5 sample

b8effd4

[csharp] Resolve nuget issue with existing files

918b623

[csharp] Update -all.sh, regenerate samples

ea4623e

jimschubert commented Nov 9, 2017

View reviewed changes

jimschubert mentioned this pull request Nov 14, 2017

[csharp] Clean up ref/inner enum structure #6887

Merged

4 tasks

wing328 merged commit 2c9f98c into swagger-api:master Nov 15, 2017

wing328 added Client: C-Sharp Enhancement: General labels Nov 15, 2017

wing328 added this to the v2.3.0 milestone Nov 15, 2017

jimschubert deleted the csharp-clean-boolean-additionalProperties-6784 branch November 15, 2017 14:39

wing328 added a commit that referenced this pull request Nov 16, 2017

Revert "[csharp] clean boolean additional properties 6784 (#6899)"

aaa44f7

This reverts commit 2c9f98c.

wing328 changed the title ~~[csharp] clean boolean additional properties 6784~~ [csharp] clean boolean additional properties Dec 10, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[csharp] clean boolean additional properties #6899

[csharp] clean boolean additional properties #6899

Uh oh!

jimschubert commented Nov 7, 2017

Uh oh!

jimschubert commented Nov 8, 2017

Uh oh!

jimschubert commented Nov 9, 2017

Uh oh!

jimschubert Nov 9, 2017

Uh oh!

wing328 Nov 15, 2017

Uh oh!

jimschubert Nov 9, 2017

Uh oh!

wing328 Nov 15, 2017

Uh oh!

jimschubert Nov 9, 2017

Uh oh!

Uh oh!

[csharp] clean boolean additional properties #6899

[csharp] clean boolean additional properties #6899

Uh oh!

Conversation

jimschubert commented Nov 7, 2017

PR checklist

Description of the PR

Uh oh!

jimschubert commented Nov 8, 2017

Uh oh!

jimschubert commented Nov 9, 2017

Uh oh!

jimschubert Nov 9, 2017

Choose a reason for hiding this comment

Uh oh!

wing328 Nov 15, 2017

Choose a reason for hiding this comment

Uh oh!

jimschubert Nov 9, 2017

Choose a reason for hiding this comment

Uh oh!

wing328 Nov 15, 2017

Choose a reason for hiding this comment

Uh oh!

jimschubert Nov 9, 2017

Choose a reason for hiding this comment

Uh oh!

Uh oh!