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

Jump to bottom

[core] [regression] set parentName when a single possible parent exists #3771

Merged
merged 1 commit into from
Aug 28, 2019
Merged

[core] [regression] set parentName when a single possible parent exists #3771

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
10 changes: 10 additions & 0 deletions modules/openapi-generator/src/main/java/org/openapitools/codegen/utils/ModelUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -896,6 +896,8 @@ public static List<Schema> getInterfaces(ComposedSchema composed) {
public static String getParentName(ComposedSchema composedSchema, Map<String, Schema> allSchemas) {
List<Schema> interfaces = getInterfaces(composedSchema);

List<String> refedParentNames = new ArrayList<>();

if (interfaces != null && !interfaces.isEmpty()) {
for (Schema schema : interfaces) {
// get the actual schema
Expand All @@ -911,13 +913,21 @@ public static String getParentName(ComposedSchema composedSchema, Map<String, Sc
} else {
LOGGER.debug("Not a parent since discriminator.propertyName is not set {}", s.get$ref());
// not a parent since discriminator.propertyName is not set
mnahkies marked this conversation as resolved.
Show resolved Hide resolved
refedParentNames.add(parentName);
}
} else {
// not a ref, doing nothing
}
}
}

// parent name only makes sense when there is a single obvious parent
if (refedParentNames.size() == 1) {
LOGGER.warn("[deprecated] inheritance without use of 'discriminator.propertyName' is deprecated " +
"and will be removed in a future release. Generating model for {}", composedSchema.getName());
return refedParentNames.get(0);
Copy link
Member

Choose a reason for hiding this comment

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

Can we throw a warning about this behavior and advise the user to use "discriminator.propertyName" in OAS v3 instead to set a proper parent model as such behavior will be removed in the future?

Our goal is to move users to adopt OAS v3 instead of staying with OAS v2 (which is not the best specification to work with inheritance/composition)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I'd still be interested in how you would recommend applying the discriminator approach to a collection of schema's like I describe in the MR description - the parent in this case should not have knowledge of its children, and also I don't particularly want to add an arbitrary field like type: string solely for code generation purposes.

I'd be more than happy to refactor our definitions to do things the OAS v3 way over time - in general I've found v3 much better to work with than v2 ever was.

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 raise an issue at the openapi 3.0 specification to allow this type of inheritance without discriminator

Copy link
Member

Choose a reason for hiding this comment

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

Another way is to use vendor extension to describe this if the OAS 3.0 spec doesn't officially support it at the moment.

(nullable was not supported in OAS v2 and we'd to use x-nullable: true before OAS v3 officially supports it)

Copy link
Member

Choose a reason for hiding this comment

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

this is a great idea, and probably the easiest way to support this behavior in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That sounds like a good plan. @macjohnny would you be comfortable raising such an issue? I don't really have the bandwidth to write a well thought proposal at this time.

Also I've added a warning, and fixed the commit author (thanks for pointing that out). Let me know if you're happy with the wording

Copy link
Member

Choose a reason for hiding this comment

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

Btw, if this fallback is breaking some existing use cases (allOf with just one schema), we will need to add an option later to cater to both cases.

Copy link
Contributor

Choose a reason for hiding this comment

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

@wing328 in regards to your #3771 (comment) and mine #3771 (comment) I assume that's what is happening in my case.

}

return null;
}

Expand Down
9 changes: 5 additions & 4 deletions ...es/openapi-generator/src/test/java/org/openapitools/codegen/java/JavaInheritanceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,17 +32,18 @@

public class JavaInheritanceTest {

@Test(description = "convert a composed model without discriminator")

@Test(description = "convert a composed model with parent")
public void javaInheritanceTest() {
final Schema allOfModel = new Schema().name("Base");
final Schema parentModel = new Schema().name("Base");

final Schema schema = new ComposedSchema()
.addAllOfItem(new Schema().$ref("Base"))
.name("composed");

OpenAPI openAPI = TestUtils.createOpenAPI();
openAPI.setComponents(new Components()
.addSchemas(allOfModel.getName(), allOfModel)
.addSchemas(parentModel.getName(),parentModel)
.addSchemas(schema.getName(), schema)
);

Expand All @@ -52,7 +53,7 @@ public void javaInheritanceTest() {

Assert.assertEquals(cm.name, "sample");
Assert.assertEquals(cm.classname, "Sample");
Assert.assertEquals(cm.parent, null);
Assert.assertEquals(cm.parent, "Base");
Assert.assertEquals(cm.imports, Sets.newHashSet("Base"));
}

Expand Down