Skip to content

Commit 71a84ff

Browse files
wing328KannaKim
wing328
and
KannaKim
authored
fix(csharp): limit public oneOf constructors to operation inputs (#24700)
Co-authored-by: Kanna Kim <kimkanna18@gmail.com>
1 parent f15049b commit 71a84ff

191 files changed

Lines changed: 687 additions & 468 deletions

File tree

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

‎modules/openapi-generator/src/main/java/org/openapitools/codegen/CodegenConstants.java‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -524,6 +524,7 @@ public static enum ENUM_PROPERTY_NAMING_TYPE {camelCase, PascalCase, snake_case,
524524
public static final String X_MODIFIERS = "x-modifiers";
525525
public static final String X_MODIFIER_PREFIX = "x-modifier-";
526526
public static final String X_MODEL_IS_MUTABLE = "x-model-is-mutable";
527+
public static final String X_MODEL_IS_OPERATION_INPUT = "x-model-is-operation-input";
527528
public static final String X_IMPLEMENTS = "x-implements";
528529
public static final String X_IS_ONE_OF_INTERFACE = "x-is-one-of-interface";
529530
public static final String USE_ENUM_VALUE_INTERFACE = "useEnumValueInterface";

‎modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/AbstractCSharpCodegen.java‎

Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,12 @@
2121
import com.samskivert.mustache.Mustache;
2222
import com.samskivert.mustache.Mustache.Lambda;
2323
import com.samskivert.mustache.Template;
24+
import io.swagger.v3.oas.models.Operation;
25+
import io.swagger.v3.oas.models.PathItem;
26+
import io.swagger.v3.oas.models.media.Content;
2427
import io.swagger.v3.oas.models.media.Schema;
28+
import io.swagger.v3.oas.models.parameters.Parameter;
29+
import io.swagger.v3.oas.models.parameters.RequestBody;
2530
import lombok.Getter;
2631
import lombok.Setter;
2732
import org.apache.commons.io.FilenameUtils;
@@ -616,6 +621,7 @@ public ModelsMap postProcessModels(ModelsMap objs) {
616621
@Override
617622
public Map<String, ModelsMap> postProcessAllModels(Map<String, ModelsMap> objs) {
618623
final Map<String, ModelsMap> processed = super.postProcessAllModels(objs);
624+
final Set<String> operationInputModels = getOperationInputModels();
619625

620626
Map<String, CodegenModel> enumRefs = new HashMap<>();
621627
for (Map.Entry<String, ModelsMap> entry : processed.entrySet()) {
@@ -645,6 +651,9 @@ public Map<String, ModelsMap> postProcessAllModels(Map<String, ModelsMap> objs)
645651
continue;
646652
}
647653

654+
if (operationInputModels.contains(entry.getKey()) || operationInputModels.contains(model.schemaName)) {
655+
model.vendorExtensions.put(X_MODEL_IS_OPERATION_INPUT, true);
656+
}
648657
model.vendorExtensions.put(X_MODEL_IS_MUTABLE, modelIsMutable(model, null));
649658

650659
CodegenComposedSchemas composedSchemas = model.getComposedSchemas();
@@ -718,6 +727,77 @@ public Map<String, ModelsMap> postProcessAllModels(Map<String, ModelsMap> objs)
718727
return processed;
719728
}
720729

730+
private Set<String> getOperationInputModels() {
731+
Set<String> operationInputModels = new HashSet<>();
732+
Set<String> visitedModels = new HashSet<>();
733+
if (openAPI == null || openAPI.getPaths() == null) {
734+
return operationInputModels;
735+
}
736+
737+
for (PathItem pathItem : openAPI.getPaths().values()) {
738+
collectOperationInputModels(pathItem.getParameters(), operationInputModels, visitedModels);
739+
for (Operation operation : pathItem.readOperations()) {
740+
collectOperationInputModels(operation.getParameters(), operationInputModels, visitedModels);
741+
RequestBody requestBody = ModelUtils.getReferencedRequestBody(openAPI, operation.getRequestBody());
742+
if (requestBody != null) {
743+
collectOperationInputModels(requestBody.getContent(), operationInputModels, visitedModels);
744+
}
745+
}
746+
}
747+
return operationInputModels;
748+
}
749+
750+
private void collectOperationInputModels(List<Parameter> parameters, Set<String> operationInputModels, Set<String> visitedModels) {
751+
if (parameters == null) {
752+
return;
753+
}
754+
for (Parameter unresolvedParameter : parameters) {
755+
Parameter parameter = ModelUtils.getReferencedParameter(openAPI, unresolvedParameter);
756+
if (parameter != null) {
757+
collectOperationInputModels(parameter.getSchema(), operationInputModels, visitedModels);
758+
collectOperationInputModels(parameter.getContent(), operationInputModels, visitedModels);
759+
}
760+
}
761+
}
762+
763+
private void collectOperationInputModels(Content content, Set<String> operationInputModels, Set<String> visitedModels) {
764+
if (content == null) {
765+
return;
766+
}
767+
content.values().forEach(mediaType -> collectOperationInputModels(mediaType.getSchema(), operationInputModels, visitedModels));
768+
}
769+
770+
private void collectOperationInputModels(Schema schema, Set<String> operationInputModels, Set<String> visitedModels) {
771+
if (schema == null || Boolean.TRUE.equals(schema.getReadOnly())) {
772+
return;
773+
}
774+
if (schema.get$ref() != null) {
775+
String modelName = ModelUtils.getSimpleRef(schema.get$ref());
776+
operationInputModels.add(modelName);
777+
if (visitedModels.add(modelName)) {
778+
collectOperationInputModels(ModelUtils.getSchema(openAPI, modelName), operationInputModels, visitedModels);
779+
}
780+
}
781+
if (schema.getOneOf() != null) {
782+
schema.getOneOf().forEach(child -> collectOperationInputModels((Schema) child, operationInputModels, visitedModels));
783+
}
784+
if (schema.getAllOf() != null) {
785+
schema.getAllOf().forEach(child -> collectOperationInputModels((Schema) child, operationInputModels, visitedModels));
786+
}
787+
if (schema.getAnyOf() != null) {
788+
schema.getAnyOf().forEach(child -> collectOperationInputModels((Schema) child, operationInputModels, visitedModels));
789+
}
790+
// TODO: Traverse OAS 3.1 schema keywords that can reference operation-input models,
791+
// such as prefixItems, contains, if/then/else, dependentSchemas, and unevaluatedProperties.
792+
collectOperationInputModels(ModelUtils.getSchemaItems(schema), operationInputModels, visitedModels); // in case schema has array type
793+
if (schema.getAdditionalProperties() instanceof Schema) {
794+
collectOperationInputModels((Schema) schema.getAdditionalProperties(), operationInputModels, visitedModels);
795+
}
796+
if (schema.getProperties() != null) {
797+
schema.getProperties().values().forEach(property -> collectOperationInputModels((Schema) property, operationInputModels, visitedModels));
798+
}
799+
}
800+
721801
/**
722802
* Returns true if the model contains any properties with a public setter
723803
* If true, the model's constructor accessor should be made public to ensure end users

‎modules/openapi-generator/src/main/resources/csharp/libraries/generichost/modelGeneric.mustache‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@
1717
/// <param name="{{#lambda.camel_case}}{{name}}{{/lambda.camel_case}}">{{description}}{{^description}}{{#lambda.camel_case}}{{name}}{{/lambda.camel_case}}{{/description}}{{#defaultValue}} (default to {{.}}){{/defaultValue}}</param>
1818
{{/isDiscriminator}}
1919
{{/allVars}}
20-
{{#model.vendorExtensions.x-model-is-mutable}}{{>visibility}}{{/model.vendorExtensions.x-model-is-mutable}}{{^model.vendorExtensions.x-model-is-mutable}}internal{{/model.vendorExtensions.x-model-is-mutable}} {{classname}}({{#lambda.joinWithComma}}{{{dataType}}} {{#lambda.escape_reserved_word}}{{#lambda.camel_case}}{{name}}{{/lambda.camel_case}}{{/lambda.escape_reserved_word}} {{#model.composedSchemas.anyOf}}{{^required}}Option<{{/required}}{{{dataType}}}{{>NullConditionalProperty}}{{^required}}>{{/required}} {{#lambda.escape_reserved_word}}{{#lambda.camel_case}}{{baseType}}{{/lambda.camel_case}}{{/lambda.escape_reserved_word}} {{/model.composedSchemas.anyOf}}{{>ModelSignature}}{{/lambda.joinWithComma}}){{#parent}} : base({{#lambda.joinWithComma}}{{#parentModel.composedSchemas.oneOf}}{{#lambda.escape_reserved_word}}{{#lambda.camel_case}}{{parent}}{{/lambda.camel_case}}{{/lambda.escape_reserved_word}}.{{#lambda.titlecase}}{{baseType}}{{/lambda.titlecase}} {{/parentModel.composedSchemas.oneOf}}{{>ModelBaseSignature}}{{/lambda.joinWithComma}}){{/parent}}
20+
{{#model.vendorExtensions.x-model-is-operation-input}}{{>visibility}}{{/model.vendorExtensions.x-model-is-operation-input}}{{^model.vendorExtensions.x-model-is-operation-input}}internal{{/model.vendorExtensions.x-model-is-operation-input}} {{classname}}({{#lambda.joinWithComma}}{{{dataType}}} {{#lambda.escape_reserved_word}}{{#lambda.camel_case}}{{name}}{{/lambda.camel_case}}{{/lambda.escape_reserved_word}} {{#model.composedSchemas.anyOf}}{{^required}}Option<{{/required}}{{{dataType}}}{{>NullConditionalProperty}}{{^required}}>{{/required}} {{#lambda.escape_reserved_word}}{{#lambda.camel_case}}{{baseType}}{{/lambda.camel_case}}{{/lambda.escape_reserved_word}} {{/model.composedSchemas.anyOf}}{{>ModelSignature}}{{/lambda.joinWithComma}}){{#parent}} : base({{#lambda.joinWithComma}}{{#parentModel.composedSchemas.oneOf}}{{#lambda.escape_reserved_word}}{{#lambda.camel_case}}{{parent}}{{/lambda.camel_case}}{{/lambda.escape_reserved_word}}.{{#lambda.titlecase}}{{baseType}}{{/lambda.titlecase}} {{/parentModel.composedSchemas.oneOf}}{{>ModelBaseSignature}}{{/lambda.joinWithComma}}){{/parent}}
2121
{
2222
{{#composedSchemas.anyOf}}
2323
{{#lambda.titlecase}}{{name}}{{/lambda.titlecase}}{{^required}}Option{{/required}} = {{#lambda.escape_reserved_word}}{{#lambda.camel_case}}{{name}}{{/lambda.camel_case}}{{/lambda.escape_reserved_word}};

‎modules/openapi-generator/src/test/java/org/openapitools/codegen/csharpnetcore/CSharpClientCodegenTest.java‎

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,42 @@
4747

4848
public class CSharpClientCodegenTest {
4949

50+
@Test
51+
public void testGenericHostOneOfConstructorsRespectApiVisibility() throws IOException {
52+
Map<String, File> publicModels = generateIssue23046Models(false);
53+
File publicModel = publicModels.get("DynamicMetadataValue");
54+
assertFileContains(publicModel.toPath(),
55+
"public DynamicMetadataValue(string @string)",
56+
"public DynamicMetadataValue(decimal @decimal)",
57+
"public DynamicMetadataValue(bool @bool)",
58+
"public DynamicMetadataValue(DateTime dateTime)");
59+
assertFileNotContains(publicModel.toPath(), "internal DynamicMetadataValue(");
60+
assertFileContains(publicModels.get("ResponseOnlyValue").toPath(),
61+
"internal ResponseOnlyValue(string @string)",
62+
"internal ResponseOnlyValue(int @int)");
63+
assertFileNotContains(publicModels.get("ResponseOnlyValue").toPath(), "public ResponseOnlyValue(");
64+
assertFileContains(publicModels.get("QueryParameterValue").toPath(),
65+
"public QueryParameterValue(string @string)",
66+
"public QueryParameterValue(int @int)");
67+
assertFileNotContains(publicModels.get("QueryParameterValue").toPath(), "internal QueryParameterValue(");
68+
assertFileContains(publicModels.get("ReadOnlyValue").toPath(),
69+
"internal ReadOnlyValue(string @string)",
70+
"internal ReadOnlyValue(int @int)");
71+
assertFileNotContains(publicModels.get("ReadOnlyValue").toPath(), "public ReadOnlyValue(");
72+
assertFileContains(publicModels.get("ForbiddenValue").toPath(),
73+
"internal ForbiddenValue(string @string)",
74+
"internal ForbiddenValue(int @int)");
75+
assertFileNotContains(publicModels.get("ForbiddenValue").toPath(), "public ForbiddenValue(");
76+
77+
File internalModel = generateIssue23046Models(true).get("DynamicMetadataValue");
78+
assertFileContains(internalModel.toPath(),
79+
"internal DynamicMetadataValue(string @string)",
80+
"internal DynamicMetadataValue(decimal @decimal)",
81+
"internal DynamicMetadataValue(bool @bool)",
82+
"internal DynamicMetadataValue(DateTime dateTime)");
83+
assertFileNotContains(internalModel.toPath(), "public DynamicMetadataValue(");
84+
}
85+
5086
@Test
5187
public void testGenericHostInnerStringEnumUnknownHandlingPreservesNullBehavior() throws IOException {
5288
// Unknown enum values must throw for both nullable and non-nullable properties.
@@ -539,4 +575,37 @@ public void testMapResponse() throws Exception {
539575
Assert.assertFalse(cr1.isModel);
540576
Assert.assertTrue(cr1.isMap);
541577
}
578+
579+
private Map<String, File> generateIssue23046Models(boolean nonPublicApi) throws IOException {
580+
File output = Files.createTempDirectory("test").toFile().getCanonicalFile();
581+
output.deleteOnExit();
582+
final OpenAPI openAPI = TestUtils.parseFlattenSpec("src/test/resources/3_0/csharp/issue_23046.yaml");
583+
final DefaultGenerator defaultGenerator = new DefaultGenerator();
584+
final ClientOptInput clientOptInput = new ClientOptInput();
585+
clientOptInput.openAPI(openAPI);
586+
CSharpClientCodegen cSharpClientCodegen = new CSharpClientCodegen();
587+
cSharpClientCodegen.setLibrary("generichost");
588+
cSharpClientCodegen.setOutputDir(output.getAbsolutePath());
589+
cSharpClientCodegen.setNonPublicApi(nonPublicApi);
590+
clientOptInput.config(cSharpClientCodegen);
591+
defaultGenerator.opts(clientOptInput);
592+
593+
Map<String, File> files = defaultGenerator.generate().stream()
594+
.collect(Collectors.toMap(File::getPath, Function.identity()));
595+
Map<String, File> models = Map.of(
596+
"DynamicMetadataValue", getGeneratedModel(files, output, "DynamicMetadataValue"),
597+
"ResponseOnlyValue", getGeneratedModel(files, output, "ResponseOnlyValue"),
598+
"QueryParameterValue", getGeneratedModel(files, output, "QueryParameterValue"),
599+
"ReadOnlyValue", getGeneratedModel(files, output, "ReadOnlyValue"),
600+
"ForbiddenValue", getGeneratedModel(files, output, "ForbiddenValue"));
601+
return models;
602+
}
603+
604+
private File getGeneratedModel(Map<String, File> files, File output, String modelName) {
605+
String path = Paths.get(output.getAbsolutePath(),
606+
"src", "Org.OpenAPITools", "Model", modelName + ".cs").toString();
607+
File model = files.get(path);
608+
assertNotNull(model, "Could not find generated model: " + path);
609+
return model;
610+
}
542611
}

‎modules/openapi-generator/src/test/resources/3_0/csharp/issue_23046.yaml‎

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,69 @@
1+
openapi: 3.0.3
2+
info:
3+
title: Minimal repro
4+
version: 1.0.0
5+
paths:
6+
/upload:
7+
post:
8+
requestBody:
9+
content:
10+
application/json:
11+
schema:
12+
$ref: '#/components/schemas/InitiateUploadRequest'
13+
responses:
14+
'200':
15+
description: OK
16+
content:
17+
application/json:
18+
schema:
19+
$ref: '#/components/schemas/ResponseOnlyValue'
20+
/search:
21+
get:
22+
parameters:
23+
- name: filter
24+
in: query
25+
schema:
26+
$ref: '#/components/schemas/QueryParameterValue'
27+
responses:
28+
'204':
29+
description: No content
30+
components:
31+
schemas:
32+
InitiateUploadRequest:
33+
type: object
34+
properties:
35+
dynamicMetadata:
36+
type: object
37+
additionalProperties:
38+
$ref: '#/components/schemas/DynamicMetadataValue'
39+
readOnlyMetadata:
40+
type: object
41+
readOnly: true
42+
additionalProperties:
43+
$ref: '#/components/schemas/ReadOnlyValue'
44+
allowedValue:
45+
not:
46+
$ref: '#/components/schemas/ForbiddenValue'
47+
DynamicMetadataValue:
48+
oneOf:
49+
- type: string
50+
- type: number
51+
- type: boolean
52+
- type: string
53+
format: date-time
54+
ResponseOnlyValue:
55+
oneOf:
56+
- type: string
57+
- type: integer
58+
QueryParameterValue:
59+
oneOf:
60+
- type: string
61+
- type: integer
62+
ReadOnlyValue:
63+
oneOf:
64+
- type: string
65+
- type: integer
66+
ForbiddenValue:
67+
oneOf:
68+
- type: string
69+
- type: integer

‎samples/client/petstore/csharp/generichost/latest/OneOfList/src/Org.OpenAPITools/Model/OneOfArrayRequest.cs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@ public partial class OneOfArrayRequest : IValidatableObject
3434
/// Initializes a new instance of the <see cref="OneOfArrayRequest" /> class.
3535
/// </summary>
3636
/// <param name="list"></param>
37-
internal OneOfArrayRequest(List<string> list)
37+
public OneOfArrayRequest(List<string> list)
3838
{
3939
List = list;
4040
OnCreated();
@@ -44,7 +44,7 @@ internal OneOfArrayRequest(List<string> list)
4444
/// Initializes a new instance of the <see cref="OneOfArrayRequest" /> class.
4545
/// </summary>
4646
/// <param name="list1"></param>
47-
internal OneOfArrayRequest(List<TestObject> list1)
47+
public OneOfArrayRequest(List<TestObject> list1)
4848
{
4949
List1 = list1;
5050
OnCreated();

‎samples/client/petstore/csharp/generichost/latest/UseDateTimeOffset/src/Org.OpenAPITools/Model/Fruit.cs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ public partial class Fruit : IValidatableObject
3535
/// </summary>
3636
/// <param name="apple"></param>
3737
/// <param name="color">color</param>
38-
public Fruit(Apple apple, Option<string?> color = default)
38+
internal Fruit(Apple apple, Option<string?> color = default)
3939
{
4040
Apple = apple;
4141
ColorOption = color;
@@ -47,7 +47,7 @@ public Fruit(Apple apple, Option<string?> color = default)
4747
/// </summary>
4848
/// <param name="banana"></param>
4949
/// <param name="color">color</param>
50-
public Fruit(Banana banana, Option<string?> color = default)
50+
internal Fruit(Banana banana, Option<string?> color = default)
5151
{
5252
Banana = banana;
5353
ColorOption = color;

‎samples/client/petstore/csharp/generichost/latest/UseDateTimeOffset/src/Org.OpenAPITools/Model/FruitReq.cs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@ public partial class FruitReq : IValidatableObject
3434
/// Initializes a new instance of the <see cref="FruitReq" /> class.
3535
/// </summary>
3636
/// <param name="appleReq"></param>
37-
public FruitReq(AppleReq appleReq)
37+
internal FruitReq(AppleReq appleReq)
3838
{
3939
AppleReq = appleReq;
4040
OnCreated();
@@ -44,7 +44,7 @@ public FruitReq(AppleReq appleReq)
4444
/// Initializes a new instance of the <see cref="FruitReq" /> class.
4545
/// </summary>
4646
/// <param name="bananaReq"></param>
47-
public FruitReq(BananaReq bananaReq)
47+
internal FruitReq(BananaReq bananaReq)
4848
{
4949
BananaReq = bananaReq;
5050
OnCreated();

‎samples/client/petstore/csharp/generichost/latest/UseDateTimeOffset/src/Org.OpenAPITools/Model/Mammal.cs‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@ public partial class Mammal : IValidatableObject
3434
/// Initializes a new instance of the <see cref="Mammal" /> class.
3535
/// </summary>
3636
/// <param name="whale"></param>
37-
public Mammal(Whale whale)
37+
internal Mammal(Whale whale)
3838
{
3939
Whale = whale;
4040
OnCreated();
@@ -44,7 +44,7 @@ public Mammal(Whale whale)
4444
/// Initializes a new instance of the <see cref="Mammal" /> class.
4545
/// </summary>
4646
/// <param name="zebra"></param>
47-
public Mammal(Zebra zebra)
47+
internal Mammal(Zebra zebra)
4848
{
4949
Zebra = zebra;
5050
OnCreated();
@@ -54,7 +54,7 @@ public Mammal(Zebra zebra)
5454
/// Initializes a new instance of the <see cref="Mammal" /> class.
5555
/// </summary>
5656
/// <param name="pig"></param>
57-
public Mammal(Pig pig)
57+
internal Mammal(Pig pig)
5858
{
5959
Pig = pig;
6060
OnCreated();

0 commit comments

Comments
 (0)