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

[codegen] change x-oneOf-name to x-one-of-name. Consistency with naming conventions and x-all-of-name #5820

Merged
merged 8 commits into from
Apr 24, 2020
Merged

[codegen] change x-oneOf-name to x-one-of-name. Consistency with naming conventions and x-all-of-name #5820

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
39da276
change x-oneOf-name to x-one-of-name.
sebastien-rosset Apr 3, 2020
b70a4ea
Add code comments
sebastien-rosset Apr 4, 2020
18b5298
Add code comments
sebastien-rosset Apr 4, 2020
7ab1139
sync from master
sebastien-rosset Apr 7, 2020
4f550a9
Merge branch 'master' of github.com:CiscoM31/openapi-generator into x…
sebastien-rosset Apr 8, 2020
32d2a09
Merge branch 'master' of github.com:CiscoM31/openapi-generator into x…
sebastien-rosset Apr 10, 2020
f8cd333
Merge branch 'master' of github.com:CiscoM31/openapi-generator into x…
sebastien-rosset Apr 10, 2020
c9893e5
Merge branch 'master' of github.com:CiscoM31/openapi-generator into x…
sebastien-rosset Apr 23, 2020
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
16 changes: 11 additions & 5 deletions modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultCodegen.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1837,7 +1837,13 @@ public String toAnyOfName(List<String> names, ComposedSchema composedSchema) {
}

/**
* Return the name of the oneOf schema
* Return the name of the oneOf schema.
*
* This name is used to set the value of CodegenProperty.openApiType.
Copy link
Contributor Author

@sebastien-rosset sebastien-rosset Apr 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@wing328 , it would be good to explain how this field is used. In other words, the 'x-one-of-name' extension exists for a reason. Why should an OAS author set this extension? What is the value of setting that extension?

Without x-one-of-name, the generated oneOf name for the schema below would be OneOf<Animal,Plant,Fungus,Bacteria>. It would be helpful to document how this influences the generated output.

    LivingOrganism: 
  	x-one-of-name: LivingOrganism
      oneOf: 
  	- $ref: '#/components/schemas/Animal' 
  	- $ref: '#/components/schemas/Plant' 
  	- $ref: '#/components/schemas/Fungus' 
  	- $ref: '#/components/schemas/Bacteria'

Is it ok to set the value of x-one-of-name to the same value as the container? That's typically a good name.

Copy link
Member

Choose a reason for hiding this comment

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

Is it ok to set the value of x-one-of-name to the same value as the container?

Let's step back for a second. Why not update LivingOrganism if the users want to have complete control of the naming?

In other words, x-one-of-name seems to be redundant if we can simply use the container (schema) name.

Copy link
Contributor Author

@sebastien-rosset sebastien-rosset Apr 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Is it ok to set the value of x-one-of-name to the same value as the container?

Let's step back for a second. Why not update LivingOrganism if the users want to have complete control of the naming?

In other words, x-one-of-name seems to be redundant if we can simply use the container (schema) name.

Yes, I totally agree the the container name would be the best default name. I was wondering why that was not the case. I suspect this is because in ModelUtils, the functions typically don't have access to anything other than the local schema passed as an argument, which makes it harder to access elements like the container schema (which would be needed to get the name of the container schema)
Here the use of x-one-of-name is just a workaround.

Copy link
Member

Choose a reason for hiding this comment

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

Without x-one-of-name, the generated oneOf name for the schema below would be OneOf<Animal,Plant,Fungus,Bacteria>

I think it's because the default toOneOfName is used: https://github.com/OpenAPITools/openapi-generator/blob/master/modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultCodegen.java

I'll do some tests to try to figure out the "correct" way to override this method in the language generator so that we don't need to use x-one-of-name if possible.

*
* If the 'x-one-of-name' extension is specified in the OAS document, return that value.
* Otherwise, a name is constructed by creating a comma-separated list of all the names
* of the oneOf schemas.
*
* @param names List of names
* @param composedSchema composed schema
Expand All @@ -1846,8 +1852,8 @@ public String toAnyOfName(List<String> names, ComposedSchema composedSchema) {
@SuppressWarnings("static-method")
public String toOneOfName(List<String> names, ComposedSchema composedSchema) {
Map<String, Object> exts = composedSchema.getExtensions();
if (exts != null && exts.containsKey("x-oneOf-name")) {
return (String) exts.get("x-oneOf-name");
if (exts != null && exts.containsKey("x-one-of-name")) {
return (String) exts.get("x-one-of-name");
}
return "oneOf<" + String.join(",", names) + ">";
}
Expand Down Expand Up @@ -5788,14 +5794,14 @@ public void setRemoveEnumValuePrefix(final boolean removeEnumValuePrefix) {
//// Following methods are related to the "useOneOfInterfaces" feature

/**
* Add "x-oneOf-name" extension to a given oneOf schema (assuming it has at least 1 oneOf elements)
* Add "x-one-of-name" extension to a given oneOf schema (assuming it has at least 1 oneOf elements)
*
* @param s schema to add the extension to
* @param name name of the parent oneOf schema
*/
public void addOneOfNameExtension(ComposedSchema s, String name) {
if (s.getOneOf() != null && s.getOneOf().size() > 0) {
s.addExtension("x-oneOf-name", name);
s.addExtension("x-one-of-name", name);
}
}

Expand Down
6 changes: 3 additions & 3 deletions modules/openapi-generator/src/test/java/org/openapitools/codegen/DefaultCodegenTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1401,7 +1401,7 @@ public void testUseOneOfInterfaces() {
.get("application/json")
.getSchema()
.getExtensions()
.get("x-oneOf-name"),
.get("x-one-of-name"),
"CreateState"
);
Assert.assertEquals(
Expand All @@ -1414,11 +1414,11 @@ public void testUseOneOfInterfaces() {
.get("application/json")
.getSchema()
.getExtensions()
.get("x-oneOf-name"),
.get("x-one-of-name"),
"GetState200"
);
// for the array schema, assert that a oneOf interface was added to schema map
Schema items = ((ArraySchema) openAPI.getComponents().getSchemas().get("CustomOneOfArraySchema")).getItems();
Assert.assertEquals(items.getExtensions().get("x-oneOf-name"), "CustomOneOfArraySchemaOneOf");
Assert.assertEquals(items.getExtensions().get("x-one-of-name"), "CustomOneOfArraySchemaOneOf");
}
}
6 changes: 3 additions & 3 deletions samples/openapi3/client/petstore/go-experimental/go-petstore/api/openapi.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1919,7 +1919,7 @@ components:
properties:
color:
type: string
x-oneOf-name: Fruit
x-one-of-name: Fruit
apple:
properties:
cultivar:
Expand All @@ -1939,7 +1939,7 @@ components:
oneOf:
- $ref: '#/components/schemas/whale'
- $ref: '#/components/schemas/zebra'
x-oneOf-name: Mammal
x-one-of-name: Mammal
whale:
properties:
hasBaleen:
Expand Down Expand Up @@ -1975,7 +1975,7 @@ components:
oneOf:
- $ref: '#/components/schemas/appleReq'
- $ref: '#/components/schemas/bananaReq'
x-oneOf-name: FruitReq
x-one-of-name: FruitReq
appleReq:
properties:
cultivar:
Expand Down