fix(normalizer): support OAS 3.1 annotated enums (oneOf + const) for consistent enum generation (C#)#23869
fix(normalizer): support OAS 3.1 annotated enums (oneOf + const) for consistent enum generation (C#)#23869nagabalaji-b wants to merge 23 commits into
Conversation
…oding on .NET 9+ On .NET 9+, HttpUtility.ParseQueryString() already URL-encodes values internally. The previous fix only checked for '.NET 9' literally, missing .NET 10+ (PowerShell 7.6). Replace the string prefix check with a numeric major version comparison (>= 9) and move the RuntimeInformation check outside the foreach loop to avoid redundant parsing. Fixes: CSCwu08056
fix(csharp): numeric .NET version check to prevent double URL-encoding on .NET 9+ (CSCwu08056)
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 8 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| if (subSchema.getEnum() == null || subSchema.getEnum().isEmpty()) { | ||
| // Check if this sub-schema has an enum or const value (OpenAPI 3.1 uses const for single-value enums) | ||
| List<Object> subSchemaEnumValues = subSchema.getEnum(); | ||
| if ((subSchemaEnumValues == null || subSchemaEnumValues.isEmpty()) && subSchema.getConst() == null) { |
There was a problem hiding this comment.
Rather than doing (subSchemaEnumValues == null || subSchemaEnumValues.isEmpty()) twice, could we extract this to a variable boolean definesEnum = ModelUtils.hasEnum(subSchema) and then have
if (!definesEnum && subSchema.getConst() == null) {This could allow use to skip the comments almost entirely and instead allow the variable/method names be the explanation themselves.
| |sortModelPropertiesByRequiredFlag|Sort model properties to place required parameters before optional parameters.| |true| | ||
| |sortParamsByRequiredFlag|Sort method arguments to place required parameters before optional parameters.| |true| | ||
| |sourceFolder|source folder for generated code| |OpenAPI/src| | ||
| |sourceFolder|source folder for generated code| |OpenAPI\src| |
There was a problem hiding this comment.
Are these path changes intentional?
|
Thanks for adjusting to the suggestions. 👍 I am not a maintainer of the project, so I cannot make a formal review with an approve. |
| // After normalization, oneOf with const values should be simplified to enum | ||
| assertEquals(schema16.getOneOf(), null); | ||
| assertEquals(schema16.getEnum().size(), 3); | ||
| assertEquals(schema16.getEnum().get(0), 1); |
There was a problem hiding this comment.
(I'm not repo maintainer but the PR is something I was thinking about myself: so thank you)
The test against deprecated (https://github.com/nagabalaji-b/openapi-generator/blob/3496c3b7cffbc0cdc728cc77bd64708b8ba6ae9a/modules/openapi-generator/src/test/resources/3_1/simplifyOneOfAnyOf_test.yaml#L133) is lost here (and the information from schema too)
Here, seems it is not possible with current enum support implementation ("simple" list of values and optional list of descriptions with x-enum-descriptions) to support enum as "schema" enabling to keep all "schemas" related information with no loss.
There was a problem hiding this comment.
@nagabalaji-b can you please take a look and add back the deprecated test?
There was a problem hiding this comment.
@wing328 Done. I added back the deprecated test coverage:
Code change: Modified simplifyComposedSchemaWithEnums() to collect and preserve per-value deprecated flags from OAS 3.1 oneOf/anyOf + const sub-schemas into x-enum-deprecated extension.
Test assertion: Added explicit checks in testOpenAPINormalizerSimplifyOneOfAnyOf31Spec() for TypeIntegerWithOneOf schema:
Verifies x-enum-deprecated list size = 3
Verifies flags match expected values: [true, false, false] (first enum value was deprecated in the original YAML)
Validation: Ran the test locally and confirmed BUILD SUCCESS.
The deprecated metadata from the annotated enum pattern (oneOf + const) is now preserved through normalization, so generators can access it via the x-enum-deprecated extension.
|
please update the samples to fix https://github.com/OpenAPITools/openapi-generator/actions/runs/26996697206/job/81834665420?pr=23869 |
There was a problem hiding this comment.
1 issue found across 74 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
nagabalaji-b
force-pushed
the
openapitools-master
branch
from
ddbb944 to
b45f152
Compare
There was a problem hiding this comment.
1 issue found across 74 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
@wing328 , i ran bin\generate-samples.sh and commited the generated samples. |
This reverts commit 6ef74e7.
There was a problem hiding this comment.
7 issues found across 746 files
Note: This PR contains a large number of files. cubic only reviews up to 40 files per PR, so some files may not have been reviewed. cubic prioritizes the most important files to review.
On a pro plan you can use ultrareview for larger PRs.
Re-trigger cubic
This reverts commit 5e4b895.
There was a problem hiding this comment.
10 issues found across 84 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="samples/client/petstore/csharp/generichost/latest/AnnotatedEnum/src/Org.OpenAPITools.Test/Api/ApiTestsBase.cs">
<violation number="1" location="samples/client/petstore/csharp/generichost/latest/AnnotatedEnum/src/Org.OpenAPITools.Test/Api/ApiTestsBase.cs:22">
P2: Embedded manual instructions in generated test base file reference non-existent file `ApiTests.Base.cs` (actual file is `ApiTestsBase.cs`) and method `ApiTestsBase#AddApiHttpClients` which is not defined on this class; it exists on `HostConfiguration` instead. These stale template comments mislead developers following setup steps.</violation>
</file>
<file name="samples/client/petstore/csharp/generichost/latest/AnnotatedEnum/src/Org.OpenAPITools/Model/OneOfNullAndRef.cs">
<violation number="1" location="samples/client/petstore/csharp/generichost/latest/AnnotatedEnum/src/Org.OpenAPITools/Model/OneOfNullAndRef.cs:100">
P2: Deserializer accepts top-level JSON arrays for an object model, silently producing a default instance instead of rejecting malformed input.</violation>
<violation number="2" location="samples/client/petstore/csharp/generichost/latest/AnnotatedEnum/src/Org.OpenAPITools/Model/OneOfNullAndRef.cs:165">
P1: Serializer can throw when `Number` is explicitly set to null because nullable `.Value` is dereferenced under `IsSet`.</violation>
</file>
<file name="samples/client/petstore/csharp/generichost/latest/AnnotatedEnum/src/Org.OpenAPITools/Model/AnyOfStringArrayOfString.cs">
<violation number="1" location="samples/client/petstore/csharp/generichost/latest/AnnotatedEnum/src/Org.OpenAPITools/Model/AnyOfStringArrayOfString.cs:112">
P1: Deserializer incorrectly rejects raw string payloads and silently drops array payloads for `anyOf(string, array<string>)`</violation>
</file>
<file name="samples/client/petstore/csharp/generichost/latest/AnnotatedEnum/src/Org.OpenAPITools/Model/ParentWithPluralOneOfPropertyNumber.cs">
<violation number="1" location="samples/client/petstore/csharp/generichost/latest/AnnotatedEnum/src/Org.OpenAPITools/Model/ParentWithPluralOneOfPropertyNumber.cs:37">
P2: Public oneOf wrapper type has only internal constructors, making it non-instantiable for external SDK consumers</violation>
</file>
<file name="samples/client/petstore/csharp/generichost/latest/AnnotatedEnum/src/Org.OpenAPITools/Client/DateTimeNullableJsonConverter.cs">
<violation number="1" location="samples/client/petstore/csharp/generichost/latest/AnnotatedEnum/src/Org.OpenAPITools/Client/DateTimeNullableJsonConverter.cs:55">
P1: Deserializer silently converts invalid date-time strings to null instead of throwing an exception, making invalid data indistinguishable from JSON null and inconsistent with the non-nullable DateTimeJsonConverter.</violation>
</file>
<file name="samples/client/petstore/csharp/generichost/latest/AnnotatedEnum/src/Org.OpenAPITools/Model/TypeIntegerWithOneOf.cs">
<violation number="1" location="samples/client/petstore/csharp/generichost/latest/AnnotatedEnum/src/Org.OpenAPITools/Model/TypeIntegerWithOneOf.cs:119">
P1: Integer enum JSON converter uses string-based read/write instead of numeric tokens, breaking interoperability for integer-typed OpenAPI enums.</violation>
</file>
<file name="samples/client/petstore/csharp/generichost/latest/AnnotatedEnum/src/Org.OpenAPITools/Model/OneOfNullableTest.cs">
<violation number="1" location="samples/client/petstore/csharp/generichost/latest/AnnotatedEnum/src/Org.OpenAPITools/Model/OneOfNullableTest.cs:125">
P0: OneOfNullableTestJsonConverter.Read uses copies of the un-advanced utf8JsonReader instead of utf8JsonReaderOneOf, causing both TryDeserialize calls to start from the root of the JSON document rather than from the current property value. This prevents correct oneOf variant detection and will throw JsonException for valid payloads.</violation>
</file>
<file name="samples/client/petstore/csharp/generichost/latest/AnnotatedEnum/src/Org.OpenAPITools/Model/OneOfNullAndRef2.cs">
<violation number="1" location="samples/client/petstore/csharp/generichost/latest/AnnotatedEnum/src/Org.OpenAPITools/Model/OneOfNullAndRef2.cs:100">
P2: Converter accepts `StartArray` for an object model, silently deserializing invalid JSON arrays instead of rejecting them. This weakens schema enforcement and creates an asymmetric contract since `Write()` only produces objects.</violation>
<violation number="2" location="samples/client/petstore/csharp/generichost/latest/AnnotatedEnum/src/Org.OpenAPITools/Model/OneOfNullAndRef2.cs:165">
P1: Serializer can throw when `NumberOption` is set to null due to forced nullable dereference (`Number!.Value`). The `Number` setter allows `null`, and `Option<T>.IsSet` is `true` even when the wrapped value is `null`. `WriteProperties` only checks `IsSet` before dereferencing `Number!.Value`, which will crash at runtime if `Number` is null. The same file's `Read` method explicitly rejects this state with `ArgumentNullException`, and `ParentWithPluralOneOfProperty.cs` in the same sample has a defensive pre-write null check, demonstrating the generator should emit one here as well.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
|
|
||
| if (utf8JsonReaderOneOf.TokenType == JsonTokenType.PropertyName && currentDepth == utf8JsonReaderOneOf.CurrentDepth - 1) | ||
| { | ||
| Utf8JsonReader utf8JsonReaderInt = utf8JsonReader; |
There was a problem hiding this comment.
P0: OneOfNullableTestJsonConverter.Read uses copies of the un-advanced utf8JsonReader instead of utf8JsonReaderOneOf, causing both TryDeserialize calls to start from the root of the JSON document rather than from the current property value. This prevents correct oneOf variant detection and will throw JsonException for valid payloads.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At samples/client/petstore/csharp/generichost/latest/AnnotatedEnum/src/Org.OpenAPITools/Model/OneOfNullableTest.cs, line 125:
<comment>OneOfNullableTestJsonConverter.Read uses copies of the un-advanced utf8JsonReader instead of utf8JsonReaderOneOf, causing both TryDeserialize calls to start from the root of the JSON document rather than from the current property value. This prevents correct oneOf variant detection and will throw JsonException for valid payloads.</comment>
<file context>
@@ -0,0 +1,190 @@
+
+ if (utf8JsonReaderOneOf.TokenType == JsonTokenType.PropertyName && currentDepth == utf8JsonReaderOneOf.CurrentDepth - 1)
+ {
+ Utf8JsonReader utf8JsonReaderInt = utf8JsonReader;
+ ClientUtils.TryDeserialize<int?>(ref utf8JsonReaderInt, jsonSerializerOptions, out varInt);
+
</file context>
| { | ||
| if (oneOfNullAndRef.NumberOption.IsSet) | ||
| { | ||
| var numberRawValue = NumberValueConverter.ToJsonValue(oneOfNullAndRef.Number!.Value); |
There was a problem hiding this comment.
P1: Serializer can throw when Number is explicitly set to null because nullable .Value is dereferenced under IsSet.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At samples/client/petstore/csharp/generichost/latest/AnnotatedEnum/src/Org.OpenAPITools/Model/OneOfNullAndRef.cs, line 165:
<comment>Serializer can throw when `Number` is explicitly set to null because nullable `.Value` is dereferenced under `IsSet`.</comment>
<file context>
@@ -0,0 +1,170 @@
+ {
+ if (oneOfNullAndRef.NumberOption.IsSet)
+ {
+ var numberRawValue = NumberValueConverter.ToJsonValue(oneOfNullAndRef.Number!.Value);
+ writer.WriteString("number", numberRawValue);
+ }
</file context>
| { | ||
| int currentDepth = utf8JsonReader.CurrentDepth; | ||
|
|
||
| if (utf8JsonReader.TokenType != JsonTokenType.StartObject && utf8JsonReader.TokenType != JsonTokenType.StartArray) |
There was a problem hiding this comment.
P1: Deserializer incorrectly rejects raw string payloads and silently drops array payloads for anyOf(string, array<string>)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At samples/client/petstore/csharp/generichost/latest/AnnotatedEnum/src/Org.OpenAPITools/Model/AnyOfStringArrayOfString.cs, line 112:
<comment>Deserializer incorrectly rejects raw string payloads and silently drops array payloads for `anyOf(string, array<string>)`</comment>
<file context>
@@ -0,0 +1,202 @@
+ {
+ int currentDepth = utf8JsonReader.CurrentDepth;
+
+ if (utf8JsonReader.TokenType != JsonTokenType.StartObject && utf8JsonReader.TokenType != JsonTokenType.StartArray)
+ throw new JsonException();
+
</file context>
| /// <returns></returns> | ||
| public override DateTime? Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options) { | ||
| if (reader.TokenType == JsonTokenType.Null) | ||
| return null; |
There was a problem hiding this comment.
P1: Deserializer silently converts invalid date-time strings to null instead of throwing an exception, making invalid data indistinguishable from JSON null and inconsistent with the non-nullable DateTimeJsonConverter.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At samples/client/petstore/csharp/generichost/latest/AnnotatedEnum/src/Org.OpenAPITools/Client/DateTimeNullableJsonConverter.cs, line 55:
<comment>Deserializer silently converts invalid date-time strings to null instead of throwing an exception, making invalid data indistinguishable from JSON null and inconsistent with the non-nullable DateTimeJsonConverter.</comment>
<file context>
@@ -0,0 +1,80 @@
+ /// <returns></returns>
+ public override DateTime? Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options) {
+ if (reader.TokenType == JsonTokenType.Null)
+ return null;
+
+ string value = reader.GetString()!;
</file context>
| /// <returns></returns> | ||
| public override TypeIntegerWithOneOf Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options) | ||
| { | ||
| string? rawValue = reader.GetString(); |
There was a problem hiding this comment.
P1: Integer enum JSON converter uses string-based read/write instead of numeric tokens, breaking interoperability for integer-typed OpenAPI enums.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At samples/client/petstore/csharp/generichost/latest/AnnotatedEnum/src/Org.OpenAPITools/Model/TypeIntegerWithOneOf.cs, line 119:
<comment>Integer enum JSON converter uses string-based read/write instead of numeric tokens, breaking interoperability for integer-typed OpenAPI enums.</comment>
<file context>
@@ -0,0 +1,180 @@
+ /// <returns></returns>
+ public override TypeIntegerWithOneOf Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
+ {
+ string? rawValue = reader.GetString();
+
+ TypeIntegerWithOneOf? result = rawValue == null
</file context>
| { | ||
| if (oneOfNullAndRef2.NumberOption.IsSet) | ||
| { | ||
| var numberRawValue = NumberValueConverter.ToJsonValue(oneOfNullAndRef2.Number!.Value); |
There was a problem hiding this comment.
P1: Serializer can throw when NumberOption is set to null due to forced nullable dereference (Number!.Value). The Number setter allows null, and Option<T>.IsSet is true even when the wrapped value is null. WriteProperties only checks IsSet before dereferencing Number!.Value, which will crash at runtime if Number is null. The same file's Read method explicitly rejects this state with ArgumentNullException, and ParentWithPluralOneOfProperty.cs in the same sample has a defensive pre-write null check, demonstrating the generator should emit one here as well.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At samples/client/petstore/csharp/generichost/latest/AnnotatedEnum/src/Org.OpenAPITools/Model/OneOfNullAndRef2.cs, line 165:
<comment>Serializer can throw when `NumberOption` is set to null due to forced nullable dereference (`Number!.Value`). The `Number` setter allows `null`, and `Option<T>.IsSet` is `true` even when the wrapped value is `null`. `WriteProperties` only checks `IsSet` before dereferencing `Number!.Value`, which will crash at runtime if `Number` is null. The same file's `Read` method explicitly rejects this state with `ArgumentNullException`, and `ParentWithPluralOneOfProperty.cs` in the same sample has a defensive pre-write null check, demonstrating the generator should emit one here as well.</comment>
<file context>
@@ -0,0 +1,170 @@
+ {
+ if (oneOfNullAndRef2.NumberOption.IsSet)
+ {
+ var numberRawValue = NumberValueConverter.ToJsonValue(oneOfNullAndRef2.Number!.Value);
+ writer.WriteString("number", numberRawValue);
+ }
</file context>
| * Follow these manual steps to construct tests. | ||
| * This file will not be overwritten. | ||
| * ********************************************************************************* | ||
| * 1. Navigate to ApiTests.Base.cs and ensure any tokens are being created correctly. |
There was a problem hiding this comment.
P2: Embedded manual instructions in generated test base file reference non-existent file ApiTests.Base.cs (actual file is ApiTestsBase.cs) and method ApiTestsBase#AddApiHttpClients which is not defined on this class; it exists on HostConfiguration instead. These stale template comments mislead developers following setup steps.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At samples/client/petstore/csharp/generichost/latest/AnnotatedEnum/src/Org.OpenAPITools.Test/Api/ApiTestsBase.cs, line 22:
<comment>Embedded manual instructions in generated test base file reference non-existent file `ApiTests.Base.cs` (actual file is `ApiTestsBase.cs`) and method `ApiTestsBase#AddApiHttpClients` which is not defined on this class; it exists on `HostConfiguration` instead. These stale template comments mislead developers following setup steps.</comment>
<file context>
@@ -0,0 +1,58 @@
+* Follow these manual steps to construct tests.
+* This file will not be overwritten.
+* *********************************************************************************
+* 1. Navigate to ApiTests.Base.cs and ensure any tokens are being created correctly.
+* Take care not to commit credentials to any repository.
+*
</file context>
| { | ||
| int currentDepth = utf8JsonReader.CurrentDepth; | ||
|
|
||
| if (utf8JsonReader.TokenType != JsonTokenType.StartObject && utf8JsonReader.TokenType != JsonTokenType.StartArray) |
There was a problem hiding this comment.
P2: Deserializer accepts top-level JSON arrays for an object model, silently producing a default instance instead of rejecting malformed input.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At samples/client/petstore/csharp/generichost/latest/AnnotatedEnum/src/Org.OpenAPITools/Model/OneOfNullAndRef.cs, line 100:
<comment>Deserializer accepts top-level JSON arrays for an object model, silently producing a default instance instead of rejecting malformed input.</comment>
<file context>
@@ -0,0 +1,170 @@
+ {
+ int currentDepth = utf8JsonReader.CurrentDepth;
+
+ if (utf8JsonReader.TokenType != JsonTokenType.StartObject && utf8JsonReader.TokenType != JsonTokenType.StartArray)
+ throw new JsonException();
+
</file context>
| /// Initializes a new instance of the <see cref="ParentWithPluralOneOfPropertyNumber" /> class. | ||
| /// </summary> | ||
| /// <param name="number"></param> | ||
| internal ParentWithPluralOneOfPropertyNumber(Number number) |
There was a problem hiding this comment.
P2: Public oneOf wrapper type has only internal constructors, making it non-instantiable for external SDK consumers
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At samples/client/petstore/csharp/generichost/latest/AnnotatedEnum/src/Org.OpenAPITools/Model/ParentWithPluralOneOfPropertyNumber.cs, line 37:
<comment>Public oneOf wrapper type has only internal constructors, making it non-instantiable for external SDK consumers</comment>
<file context>
@@ -0,0 +1,189 @@
+ /// Initializes a new instance of the <see cref="ParentWithPluralOneOfPropertyNumber" /> class.
+ /// </summary>
+ /// <param name="number"></param>
+ internal ParentWithPluralOneOfPropertyNumber(Number number)
+ {
+ Number = number;
</file context>
| { | ||
| int currentDepth = utf8JsonReader.CurrentDepth; | ||
|
|
||
| if (utf8JsonReader.TokenType != JsonTokenType.StartObject && utf8JsonReader.TokenType != JsonTokenType.StartArray) |
There was a problem hiding this comment.
P2: Converter accepts StartArray for an object model, silently deserializing invalid JSON arrays instead of rejecting them. This weakens schema enforcement and creates an asymmetric contract since Write() only produces objects.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At samples/client/petstore/csharp/generichost/latest/AnnotatedEnum/src/Org.OpenAPITools/Model/OneOfNullAndRef2.cs, line 100:
<comment>Converter accepts `StartArray` for an object model, silently deserializing invalid JSON arrays instead of rejecting them. This weakens schema enforcement and creates an asymmetric contract since `Write()` only produces objects.</comment>
<file context>
@@ -0,0 +1,170 @@
+ {
+ int currentDepth = utf8JsonReader.CurrentDepth;
+
+ if (utf8JsonReader.TokenType != JsonTokenType.StartObject && utf8JsonReader.TokenType != JsonTokenType.StartArray)
+ throw new JsonException();
+
</file context>
| @@ -0,0 +1,10 @@ | |||
| # for csharp generichost - OAS 3.1 annotated enum with const + deprecated | |||
| generatorName: csharp | |||
| outputDir: samples/client/petstore/csharp/generichost/latest/AnnotatedEnum | |||
There was a problem hiding this comment.
please add the new output folder to the c# github workflow so that it will be tested moving forward.
|
Is a new sample necessary? Can it be added to the petstore sample? |
|
@devhl-labs thanks for reviewing this change. I'll take a look later to see if these samples are needed and whether these can be added to existing specs/samples instead. |
There was a problem hiding this comment.
4 issues found across 88 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="samples/client/petstore/csharp/generichost/latest/AnnotatedEnum/src/Org.OpenAPITools/Model/OneOfNullableTest.cs">
<violation number="1" location="samples/client/petstore/csharp/generichost/latest/AnnotatedEnum/src/Org.OpenAPITools/Model/OneOfNullableTest.cs:195">
P1: WriteProperties is empty, causing serialization to produce an empty JSON object and lose all property data</violation>
</file>
<file name="samples/client/petstore/csharp/generichost/latest/AnnotatedEnum/src/Org.OpenAPITools/Model/PropertiesWithAnyOf.cs">
<violation number="1" location="samples/client/petstore/csharp/generichost/latest/AnnotatedEnum/src/Org.OpenAPITools/Model/PropertiesWithAnyOf.cs:134">
P2: Object converter `PropertiesWithAnyOfJsonConverter` accepts `StartArray` for a non-collection model and silently returns an empty object instead of throwing `JsonException`.</violation>
</file>
<file name="modules/openapi-generator/src/main/java/org/openapitools/codegen/OpenAPINormalizer.java">
<violation number="1" location="modules/openapi-generator/src/main/java/org/openapitools/codegen/OpenAPINormalizer.java:1652">
P2: `const: null` is conflated with an absent const because `getConst() == null` cannot distinguish the two. In OAS 3.1 / JSON Schema, `null` is a valid const value, so a oneOf/anyOf branch with `const: null` (or a mixed schema containing it) causes the normalizer to bail out and skip enum simplification instead of treating the null as a valid enum member. In practice this means nullable annotated enums expressed with `const: null` will not be normalized.</violation>
</file>
<file name="samples/client/petstore/csharp/generichost/latest/AnnotatedEnum/src/Org.OpenAPITools/Model/ParentWithOneOfProperty.cs">
<violation number="1" location="samples/client/petstore/csharp/generichost/latest/AnnotatedEnum/src/Org.OpenAPITools/Model/ParentWithOneOfProperty.cs:134">
P1: `number` null semantics are inconsistent between read and write paths. On deserialization, explicit JSON `null` causes `GetString()` to return null, bypassing the Option assignment so `IsSet` stays false and the explicit null is silently lost. On serialization, if `NumberOption.IsSet` is true but the value is null (possible via the public setter), `Number!.Value` dereferences null and throws at runtime.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| /// <param name="oneOfNullableTest"></param> | ||
| /// <param name="jsonSerializerOptions"></param> | ||
| /// <exception cref="NotImplementedException"></exception> | ||
| public void WriteProperties(Utf8JsonWriter writer, OneOfNullableTest oneOfNullableTest, JsonSerializerOptions jsonSerializerOptions) |
There was a problem hiding this comment.
P1: WriteProperties is empty, causing serialization to produce an empty JSON object and lose all property data
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At samples/client/petstore/csharp/generichost/latest/AnnotatedEnum/src/Org.OpenAPITools/Model/OneOfNullableTest.cs, line 195:
<comment>WriteProperties is empty, causing serialization to produce an empty JSON object and lose all property data</comment>
<file context>
@@ -0,0 +1,200 @@
+ /// <param name="oneOfNullableTest"></param>
+ /// <param name="jsonSerializerOptions"></param>
+ /// <exception cref="NotImplementedException"></exception>
+ public void WriteProperties(Utf8JsonWriter writer, OneOfNullableTest oneOfNullableTest, JsonSerializerOptions jsonSerializerOptions)
+ {
+
</file context>
| { | ||
| case "number": | ||
| string? numberRawValue = utf8JsonReader.GetString(); | ||
| if (numberRawValue != null) |
There was a problem hiding this comment.
P1: number null semantics are inconsistent between read and write paths. On deserialization, explicit JSON null causes GetString() to return null, bypassing the Option assignment so IsSet stays false and the explicit null is silently lost. On serialization, if NumberOption.IsSet is true but the value is null (possible via the public setter), Number!.Value dereferences null and throws at runtime.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At samples/client/petstore/csharp/generichost/latest/AnnotatedEnum/src/Org.OpenAPITools/Model/ParentWithOneOfProperty.cs, line 134:
<comment>`number` null semantics are inconsistent between read and write paths. On deserialization, explicit JSON `null` causes `GetString()` to return null, bypassing the Option assignment so `IsSet` stays false and the explicit null is silently lost. On serialization, if `NumberOption.IsSet` is true but the value is null (possible via the public setter), `Number!.Value` dereferences null and throws at runtime.</comment>
<file context>
@@ -0,0 +1,180 @@
+ {
+ case "number":
+ string? numberRawValue = utf8JsonReader.GetString();
+ if (numberRawValue != null)
+ number = new Option<Number?>(NumberValueConverter.FromStringOrDefault(numberRawValue));
+ break;
</file context>
| { | ||
| int currentDepth = utf8JsonReader.CurrentDepth; | ||
|
|
||
| if (utf8JsonReader.TokenType != JsonTokenType.StartObject && utf8JsonReader.TokenType != JsonTokenType.StartArray) |
There was a problem hiding this comment.
P2: Object converter PropertiesWithAnyOfJsonConverter accepts StartArray for a non-collection model and silently returns an empty object instead of throwing JsonException.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At samples/client/petstore/csharp/generichost/latest/AnnotatedEnum/src/Org.OpenAPITools/Model/PropertiesWithAnyOf.cs, line 134:
<comment>Object converter `PropertiesWithAnyOfJsonConverter` accepts `StartArray` for a non-collection model and silently returns an empty object instead of throwing `JsonException`.</comment>
<file context>
@@ -0,0 +1,209 @@
+ {
+ int currentDepth = utf8JsonReader.CurrentDepth;
+
+ if (utf8JsonReader.TokenType != JsonTokenType.StartObject && utf8JsonReader.TokenType != JsonTokenType.StartArray)
+ throw new JsonException();
+
</file context>
| if (utf8JsonReader.TokenType != JsonTokenType.StartObject && utf8JsonReader.TokenType != JsonTokenType.StartArray) | |
| if (utf8JsonReader.TokenType != JsonTokenType.StartObject) | |
| throw new JsonException(); |
| if (subSchema.getEnum() == null || subSchema.getEnum().isEmpty()) { | ||
| // Check if this sub-schema has an enum or const value (OAS 3.1 uses const for single-value enums) | ||
| boolean definesEnum = ModelUtils.hasEnum(subSchema); | ||
| if (!definesEnum && subSchema.getConst() == null) { |
There was a problem hiding this comment.
P2: const: null is conflated with an absent const because getConst() == null cannot distinguish the two. In OAS 3.1 / JSON Schema, null is a valid const value, so a oneOf/anyOf branch with const: null (or a mixed schema containing it) causes the normalizer to bail out and skip enum simplification instead of treating the null as a valid enum member. In practice this means nullable annotated enums expressed with const: null will not be normalized.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At modules/openapi-generator/src/main/java/org/openapitools/codegen/OpenAPINormalizer.java, line 1652:
<comment>`const: null` is conflated with an absent const because `getConst() == null` cannot distinguish the two. In OAS 3.1 / JSON Schema, `null` is a valid const value, so a oneOf/anyOf branch with `const: null` (or a mixed schema containing it) causes the normalizer to bail out and skip enum simplification instead of treating the null as a valid enum member. In practice this means nullable annotated enums expressed with `const: null` will not be normalized.</comment>
<file context>
@@ -1646,10 +1647,15 @@ protected Schema simplifyComposedSchemaWithEnums(Schema schema, List<Object> sub
- if (subSchema.getEnum() == null || subSchema.getEnum().isEmpty()) {
+ // Check if this sub-schema has an enum or const value (OAS 3.1 uses const for single-value enums)
+ boolean definesEnum = ModelUtils.hasEnum(subSchema);
+ if (!definesEnum && subSchema.getConst() == null) {
return schema;
}
</file context>
…convention
CI's samples job runs bin/generate-samples.sh twice. On the second pass the generator skips idempotent files (.openapi-generator-ignore and Test/{Api,Model}/*Tests.cs already on disk), so they are omitted from the FILES manifest. Other generichost samples (e.g. ComposedEnum) follow this convention; AnnotatedEnum's FILES was previously committed after a single regen and included 17 stale entries. Regenerated to match CI output.
|
Hi not really reviewing full list of of updates because of out if time and may be not the best one. Even if I'm myself interested by the feature at short term because of it is a quick-win, I'm questioning about long term: we are adding a new vendor extension to support the deprecation for enums so another dedicated one. Was there some reflexion to have a new "CodegenEnum" usage based onto same pattern in place for schema, variables, ... @wing328 what is you feedback ? (not to change this PR but regarding long term solution outside of vendor extension usage) |
7 participants
Summary
This PR addresses normalization for OpenAPI 3.1 annotated enums, where enum values are expressed using
oneOf+const. The main focus is on ensuring C# behavior aligns with traditional enum handling.Problem
OpenAPI 3.0 typically utilizes enum arrays. In contrast, OpenAPI 3.1 often adopts an annotated enum style with
oneOf+constand per-value descriptions. Prior to this fix, const-based composed schemas were not consistently simplified into enum-like schemas, potentially impacting strong enum generation in C#.Example (OAS 3.1)
Equivalent OAS 3.0 Shape
C# Before vs After
Before Fix (String Property)
The property is a plain string because
oneOf+constwas not normalized to enum semantics.After Fix (Strong Enum)
After normalization, the schema is recognized as an enum and generates a strongly-typed enum with the appropriate JSON converter and member attributes.
Actual Behavior Before
OAS 3.1
oneOf+constwas not consistently normalized to enum semantics. C# output could revert to non-enum-style modeling for this pattern.Expected Behavior
OAS 3.1
oneOf+constshould normalize in the same manner as OAS 3.0 enums. C# should consistently produce strongly typed enum output.Changes in This PR
constas a single enum value when an enum is absent in composed sub-schemas.oneOfandanyOfenum-like paths.C# Result After Fix (Illustrative)
Validation
The targeted normalizer test for OAS 3.1 simplifying
oneOf/anyOfpaths passed. Samples have been regenerated for Java configurations. Documentation for the generator has been exported.Compatibility
This is a bug fix only. No intended breaking behavior changes. The normalization improvement is generator-agnostic; C# is the primary verified use case.
PR Checklist
@muttleyxd @devhl-labs @lucasheim @shibayan (C# generators)
Summary by cubic
Normalize OpenAPI 3.1 annotated enums (
oneOf+const) as true enums. This ensures that C# code generation produces strong enums consistently, matching OAS 3.0 behavior.constas a single enum value when an enum is missing in sub-schemas.oneOf/anyOfenum-like schemas into a single enum in the parent, preserving types and per-value descriptions.Written for commit 51e0b0f. Summary will update with new commits. Review in cubic