Skip to content

Issue #10593 Fix nullable property implemented with oneOf construct #10602

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.

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

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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: 8 additions & 0 deletions bin/configs/java-nullable-object-property.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
generatorName: java
outputDir: samples/client/petstore/java/nullable-object-property
library: native
inputSpec: modules/openapi-generator/src/test/resources/3_0/issue_10593.yaml
templateDir: modules/openapi-generator/src/main/resources/Java
additionalProperties:
artifactId: java-nullable-property
hideGenerationTimestamp: "true"
8 changes: 8 additions & 0 deletions bin/configs/kotlin-nullable-object-property.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
generatorName: kotlin
outputDir: samples/client/petstore/kotlin-nullable-object-propery
inputSpec: modules/openapi-generator/src/test/resources/3_0/issue_10593.yaml
templateDir: modules/openapi-generator/src/main/resources/kotlin-client
additionalProperties:
artifactId: nullable-object-property
serializableModel: "true"
nullableReturnType: "true"
4 changes: 4 additions & 0 deletions bin/configs/php-nullable-object-property.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
generatorName: php
outputDir: samples/client/petstore/php/nullable-object-property-php
inputSpec: modules/openapi-generator/src/test/resources/3_0/issue_10593.yaml
templateDir: modules/openapi-generator/src/main/resources/php
4 changes: 4 additions & 0 deletions bin/configs/typescript-fetch-nullable-object-property.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
generatorName: typescript-fetch
outputDir: samples/client/petstore/typescript-fetch/builds/nullable-object-property
inputSpec: modules/openapi-generator/src/test/resources/3_0/issue_10593.yaml
templateDir: modules/openapi-generator/src/main/resources/typescript-fetch
1 change: 1 addition & 0 deletions docs/generators/typescript-angular.md
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@ These options may be applied as additional-properties (cli) or configOptions (pl
<li>String</li>
<li>any</li>
<li>boolean</li>
<li>null</li>
<li>number</li>
<li>object</li>
<li>string</li>
Expand Down
1 change: 1 addition & 0 deletions docs/generators/typescript-aurelia.md
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ These options may be applied as additional-properties (cli) or configOptions (pl
<li>String</li>
<li>any</li>
<li>boolean</li>
<li>null</li>
<li>number</li>
<li>object</li>
<li>string</li>
Expand Down
1 change: 1 addition & 0 deletions docs/generators/typescript-axios.md
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ These options may be applied as additional-properties (cli) or configOptions (pl
<li>String</li>
<li>any</li>
<li>boolean</li>
<li>null</li>
<li>number</li>
<li>object</li>
<li>string</li>
Expand Down
1 change: 1 addition & 0 deletions docs/generators/typescript-fetch.md
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ These options may be applied as additional-properties (cli) or configOptions (pl
<li>String</li>
<li>any</li>
<li>boolean</li>
<li>null</li>
<li>number</li>
<li>object</li>
<li>string</li>
Expand Down
1 change: 1 addition & 0 deletions docs/generators/typescript-inversify.md
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ These options may be applied as additional-properties (cli) or configOptions (pl
<li>String</li>
<li>any</li>
<li>boolean</li>
<li>null</li>
<li>number</li>
<li>object</li>
<li>string</li>
Expand Down
1 change: 1 addition & 0 deletions docs/generators/typescript-jquery.md
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ These options may be applied as additional-properties (cli) or configOptions (pl
<li>String</li>
<li>any</li>
<li>boolean</li>
<li>null</li>
<li>number</li>
<li>object</li>
<li>string</li>
Expand Down
1 change: 1 addition & 0 deletions docs/generators/typescript-nestjs.md
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ These options may be applied as additional-properties (cli) or configOptions (pl
<li>String</li>
<li>any</li>
<li>boolean</li>
<li>null</li>
<li>number</li>
<li>object</li>
<li>string</li>
Expand Down
1 change: 1 addition & 0 deletions docs/generators/typescript-node.md
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ These options may be applied as additional-properties (cli) or configOptions (pl
<li>String</li>
<li>any</li>
<li>boolean</li>
<li>null</li>
<li>number</li>
<li>object</li>
<li>string</li>
Expand Down
1 change: 1 addition & 0 deletions docs/generators/typescript-redux-query.md
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ These options may be applied as additional-properties (cli) or configOptions (pl
<li>String</li>
<li>any</li>
<li>boolean</li>
<li>null</li>
<li>number</li>
<li>object</li>
<li>string</li>
Expand Down
1 change: 1 addition & 0 deletions docs/generators/typescript-rxjs.md
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ These options may be applied as additional-properties (cli) or configOptions (pl
<li>String</li>
<li>any</li>
<li>boolean</li>
<li>null</li>
<li>number</li>
<li>object</li>
<li>string</li>
Expand Down
1 change: 1 addition & 0 deletions docs/generators/typescript.md
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ These options may be applied as additional-properties (cli) or configOptions (pl
<li>String</li>
<li>any</li>
<li>boolean</li>
<li>null</li>
<li>number</li>
<li>string</li>
</ul>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2203,6 +2203,10 @@ public String toAnyOfName(List<String> names, ComposedSchema composedSchema) {
* Otherwise, a name is constructed by creating a comma-separated list of all the names
* of the oneOf schemas.
*
* If the oneOf schema contains only one element and have the openapi 3.0 nullable flag,
* or one element and the openapi 3.1 'null' type, only this element is returned.
Copy link
Contributor

Choose a reason for hiding this comment

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

how about the case with more than one type in oneOf?

oneOf:
  - 'null'
  - ref: ModelA
  - ref: ModelB

How do we handle this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't change that one, it will behave like before.

Since many language plugin does not seen to really support oneOf, it will still be broken.

I think typescript will be ok and produce ModelA | Model B | null

Copy link
Contributor

@amakhrov amakhrov Oct 14, 2021

Choose a reason for hiding this comment

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

Do you think it's worth adding to the test spec and see what the generated typescript code looks like?
The reason I'm specifically curious about it is that you seem to filter out the "null" part of the union. And I'm not sure whether it still preserves in some flags and resurfaces later

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a case in the yaml. We can see the result in this commit :

9b9f58659cefc284e9a4c0ce3c361c8c404d2292

*
*
* @param names List of names
* @param composedSchema composed schema
* @return name of the oneOf schema
Expand All @@ -2213,6 +2217,12 @@ public String toOneOfName(List<String> names, ComposedSchema composedSchema) {
if (exts != null && exts.containsKey("x-one-of-name")) {
return (String) exts.get("x-one-of-name");
}
if (Boolean.TRUE.equals(composedSchema.getNullable()) || ModelUtils.isNullableComposedSchema(composedSchema)) {
String singleType = names.stream().filter(p -> !p.equals("null")).findFirst().orElse(null);
if(singleType != null) {
return singleType;
}
}
return "oneOf<" + String.join(",", names) + ">";
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,7 @@ private boolean isModelNeeded(Schema schema, Set<Schema> visitedSchemas) {
if (m.getAnyOf() != null && !m.getAnyOf().isEmpty()) {
return true;
}
if (m.getOneOf() != null && !m.getOneOf().isEmpty()) {
if (m.getOneOf() != null && !m.getOneOf().isEmpty() && !isNullableOneOfComposedSchema(m)) {
return true;
}
}
Expand Down Expand Up @@ -352,7 +352,7 @@ private void gatherInlineModels(Schema schema, String modelPrefix) {
}
m.setAnyOf(newAnyOf);
}
if (m.getOneOf() != null) {
if (m.getOneOf() != null && !isNullableOneOfComposedSchema(m)) {
List<Schema> newOneOf = new ArrayList<Schema>();
for (Schema inner : m.getOneOf()) {
String schemaName = resolveModelName(inner.getTitle(), modelPrefix + "_oneOf");
Expand Down Expand Up @@ -900,5 +900,17 @@ private String addSchemas(String name, Schema schema) {
return name;
}

private boolean isNullableOneOfComposedSchema(ComposedSchema schema) {
// OAS 3.0 oneOf with nullable attribute and single oneOf element
if (Boolean.TRUE.equals(schema.getNullable()) && schema.getOneOf() != null && schema.getOneOf().size() == 1) {
return true;
}

// OAS 3.1 nullable schema
if (ModelUtils.isNullableComposedSchema(schema)) {
return true;
}

return false;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
package org.openapitools.codegen.languages;

import io.swagger.v3.oas.models.media.ArraySchema;
import io.swagger.v3.oas.models.media.ComposedSchema;
import io.swagger.v3.oas.models.media.Schema;
import io.swagger.v3.oas.models.media.StringSchema;
import org.apache.commons.io.FilenameUtils;
Expand Down Expand Up @@ -333,7 +334,24 @@ public String getTypeDeclaration(Schema p) {
String type = super.getTypeDeclaration(p);
return (!languageSpecificPrimitives.contains(type))
? "\\" + modelPackage + "\\" + type : type;
} else if (p instanceof ComposedSchema) {
// Support nullable defined using oneOf construct
ComposedSchema composedSchema = (ComposedSchema)p;
Boolean isNullable = Boolean.TRUE.equals(p.getNullable())
|| ModelUtils.isNullableComposedSchema(composedSchema);
if (composedSchema.getOneOf() != null && isNullable) {
Schema inner = composedSchema
.getOneOf()
.stream()
.filter(
subSchema -> !ModelUtils.isNullType(subSchema)
).findFirst().orElse(null);
if (inner != null) {
return this.getTypeDeclaration(inner);
}
}
}

return super.getTypeDeclaration(p);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -307,7 +307,8 @@ public AbstractTypeScriptClientCodegen() {
"Error",
"Map",
"object",
"Set"
"Set",
"null"
));

languageGenericTypes = new HashSet<>(Collections.singletonList(
Expand Down Expand Up @@ -344,6 +345,7 @@ public AbstractTypeScriptClientCodegen() {
typeMapping.put("URI", "string");
typeMapping.put("Error", "Error");
typeMapping.put("AnyType", "any");
typeMapping.put("null", "null");

cliOptions.add(new CliOption(CodegenConstants.ENUM_NAME_SUFFIX, CodegenConstants.ENUM_NAME_SUFFIX_DESC).defaultValue(this.enumSuffix));
cliOptions.add(new CliOption(CodegenConstants.ENUM_PROPERTY_NAMING, CodegenConstants.ENUM_PROPERTY_NAMING_DESC).defaultValue(this.enumPropertyNaming.name()));
Expand Down Expand Up @@ -1093,7 +1095,8 @@ public String toAnyOfName(List<String> names, ComposedSchema composedSchema) {
@Override
public String toOneOfName(List<String> names, ComposedSchema composedSchema) {
List<String> types = getTypesFromSchemas(composedSchema.getOneOf());

// Null is already handled by the isNullable flag
types.removeIf(p -> p.equals("null"));
return String.join(" | ", types);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,8 @@ public TypeScriptClientCodegen() {
"File",
"Error",
"Map",
"Set"
"Set",
"null"
));

languageGenericTypes = new HashSet<>(Arrays.asList(
Expand Down Expand Up @@ -181,6 +182,7 @@ public TypeScriptClientCodegen() {
typeMapping.put("UUID", "string");
typeMapping.put("Error", "Error");
typeMapping.put("AnyType", "any");
typeMapping.put("null", "null");


cliOptions.add(new CliOption(NPM_NAME, "The name under which you want to publish generated npm package." +
Expand Down Expand Up @@ -1521,7 +1523,8 @@ public String toAnyOfName(List<String> names, ComposedSchema composedSchema) {
@Override
public String toOneOfName(List<String> names, ComposedSchema composedSchema) {
List<String> types = getTypesFromSchemas(composedSchema.getOneOf());

// Null is already handled by the isNullable flag
types.removeIf(p -> p.equals("null"));
return String.join(" | ", types);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@
import io.swagger.v3.oas.models.responses.ApiResponses;
import io.swagger.v3.oas.models.security.SecurityScheme;
import io.swagger.v3.parser.core.models.ParseOptions;

import org.openapitools.codegen.config.CodegenConfigurator;
import org.openapitools.codegen.config.GlobalSettings;
import org.openapitools.codegen.model.ModelMap;
Expand Down Expand Up @@ -3732,7 +3731,7 @@ public void testComposedPropertyTypes() {
modelName = "ObjectWithComposedProperties";
CodegenModel m = codegen.fromModel(modelName, openAPI.getComponents().getSchemas().get(modelName));
/* TODO inline allOf schema are created as separate models and the following assumptions that
the properties are non-model are no longer valid and need to be revised
the properties are non-model are no longer valid and need to be revised
assertTrue(m.vars.get(0).getIsMap());
assertTrue(m.vars.get(1).getIsNumber());
assertTrue(m.vars.get(2).getIsUnboundedInteger());
Expand Down Expand Up @@ -4245,4 +4244,34 @@ public void testFromPropertyRequiredAndOptional() {
Assert.assertEquals(fooOptional.vars.get(0).name, "foo");
Assert.assertEquals(fooOptional.requiredVars.size(), 0);
}

@Test
public void testNullabelObjectProperties() {
DefaultCodegen codegen = new DefaultCodegen();
final OpenAPI openAPI = TestUtils.parseFlattenSpec("src/test/resources/3_0/issue_10593.yaml");
codegen.setOpenAPI(openAPI);
Schema schema = openAPI.getComponents().getSchemas().get("ModelWithNullableObjectProperty");

String propertyName;
CodegenProperty property;

// openapi 3.0 nullable
propertyName = "propertyName30";
property = codegen.fromProperty(propertyName, (Schema) schema.getProperties().get(propertyName));
assertEquals(property.openApiType, "PropertyType");
assertTrue(property.isNullable);

// openapi 3.1 'null'
propertyName = "propertyName31";
property = codegen.fromProperty(propertyName, (Schema) schema.getProperties().get(propertyName));
assertEquals(property.openApiType, "PropertyType");
assertTrue(property.isNullable);

// Non regression on regular oneOf construct
propertyName = "nonNullableProperty";
property = codegen.fromProperty(propertyName, (Schema) schema.getProperties().get(propertyName));
assertEquals(property.openApiType, "ModelWithNullableObjectProperty_nonNullableProperty");
assertFalse(property.isNullable);
// oneOf property resolve to any type and is set to nullable
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,10 @@
import java.util.Arrays;
import java.util.HashMap;

import static org.testng.Assert.assertEquals;
import static org.testng.Assert.assertTrue;
import static org.testng.AssertJUnit.assertFalse;

public class AbstractPhpCodegenTest {

@Test
Expand Down Expand Up @@ -166,6 +170,39 @@ public void testEnumPropertyWithDefaultValue() {
Assert.assertEquals(cp1.getDefaultValue(), "'VALUE'");
}

@Test(description = "Issue #10593")
public void testModelWithNullableObjectProperty() {
final AbstractPhpCodegen codegen = new P_AbstractPhpCodegen();

final OpenAPI openAPI = TestUtils.parseFlattenSpec("src/test/resources/3_0/issue_10593.yaml");
codegen.setOpenAPI(openAPI);
Schema schema = openAPI.getComponents().getSchemas().get("ModelWithNullableObjectProperty");

String propertyName;
CodegenProperty property;

// regular property
propertyName = "propertyName";
property = codegen.fromProperty(propertyName, (Schema) schema.getProperties().get(propertyName));
assertEquals(property.openApiType, "PropertyType");
assertFalse(property.isNullable);
assertEquals(property.dataType, "\\php\\Model\\PropertyType");

// openapi 3.0 nullable
propertyName = "propertyName30";
property = codegen.fromProperty(propertyName, (Schema) schema.getProperties().get(propertyName));
assertEquals(property.openApiType, "PropertyType");
assertTrue(property.isNullable);
assertEquals(property.dataType, "\\php\\Model\\PropertyType");

// openapi 3.1 'null'
propertyName = "propertyName31";
property = codegen.fromProperty(propertyName, (Schema) schema.getProperties().get(propertyName));
assertEquals(property.openApiType, "PropertyType");
assertTrue(property.isNullable);
assertEquals(property.dataType, "\\php\\Model\\PropertyType");
}

private static class P_AbstractPhpCodegen extends AbstractPhpCodegen {
@Override
public CodegenType getTag() {
Expand Down
Loading