-
-
Notifications
You must be signed in to change notification settings - Fork 6.4k
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
Add OpenAPI Normalizer #14172
Add OpenAPI Normalizer #14172
Conversation
@@ -494,6 +497,18 @@ private void generatePlainTextHelp(StringBuilder sb, CodegenConfig config) { | |||
sb.append(newline); | |||
} | |||
|
|||
if (Boolean.TRUE.equals(openapiNormalizer)) { | |||
sb.append(newline).append("OPENAIP NORMALIZER RULEA").append(newline).append(newline); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧐 This looks like a typo OPENAIP
Also, is RULEA
an example, or also a typo?
Map<String, String> map = config.openapiNormalizer() | ||
.entrySet() | ||
.stream() | ||
.collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue, (a, b) -> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❔ Is this just to check for duplicates? I'm failing to see how we could get any when the source collection is already a map.
@@ -455,6 +466,7 @@ public GeneratorSettings() { | |||
schemaMappings = Collections.unmodifiableMap(new HashMap<>(0)); | |||
inlineSchemaNameMappings = Collections.unmodifiableMap(new HashMap<>(0)); | |||
inlineSchemaNameDefaults = Collections.unmodifiableMap(new HashMap<>(0)); | |||
openapiNormalizer = Collections.unmodifiableMap(new HashMap<>(0)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓ Why not use Collections.emptyMap()
if it should be unmodifable in any case?
* @return a reference to this Builder | ||
*/ | ||
public Builder withOpenAPINormalizer(Map<String, String> openapiNormalizer) { | ||
this.openapiNormalizer = openapiNormalizer; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❔ Is it allowed to put null here, or should this use requireNonNull
.
Alternatively, I you could write:
this.openapiNormalizer.clear();
if(openapiNormalizer != null) {
this.openapiNormalizer.putAll(openapiNormalizer);
}
which would achieve two things: First, a defensive copy. Second, it prevents the field from ever being null.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like @leonard84 's approach since it's bad to set maps and collections to null in general
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can apply such improvement not only for this new option (or property) but all other options/properties. We will do it in a separate PR in the future instead.
.../openapi-generator-core/src/main/java/org/openapitools/codegen/config/GeneratorSettings.java
Show resolved
Hide resolved
} | ||
|
||
if (schema instanceof ArraySchema) { | ||
if (schema.getItems() != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 I would extract every block of the enclosing if-else
into its own method to reduce the complexity/length of this method.
} | ||
|
||
private void normalizeNonComposedSchema(Schema schema, Set<Schema> visitedSchemas) { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❔ is this intentionally empty, maybe add a comment?
refSchema.getExtensions().put("x-parent", true); | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you remove the else and make it a normal block, you can omit this line.
Assert.assertEquals(files.size(), 24); | ||
validateJavaSourceFiles(files); | ||
|
||
TestUtils.assertFileContains(Paths.get(output + "/src/main/java/xyz/abcdef/model/Child.java"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❔ isn't this missing a test for Person
to be an abstract
class.
TestUtils.assertFileContains(Paths.get(output + "/src/main/java/xyz/abcdef/model/Adult.java"), | ||
"public class Adult extends Person {"); | ||
TestUtils.assertFileContains(Paths.get(output + "/src/main/java/xyz/abcdef/model/AnotherChild.java"), | ||
"public class AnotherChild {"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm missing a test for the error case, i.e., an allOf
with multiple refs. Also, one with multiple layers of inheritance.
@@ -0,0 +1,80 @@ | |||
openapi: 3.0.1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This time no sample code? Is it because it's an unofficial extension?
@leonard84 thanks for the feedback as always. I'll go through them tomorrow. Did you have time to give it a try with the spec owned by your team or your company (I don't have access to that) to see if the output (especially the model inheritance) is what you're looking for without the use of discriminator? |
I was wondering, how hard would it be to support |
It seems to work fine |
Although, the |
@leonard84 very good question. Are you free for a quick chat (IM) via Slack when you've time? https://join.slack.com/t/openapi-generator/shared_invite/zt-12jxxd7p2-XUeQM~4pzsU9x~eGLQqX2g |
* @return a reference to this Builder | ||
*/ | ||
public Builder withOpenAPINormalizer(Map<String, String> openapiNormalizer) { | ||
this.openapiNormalizer = openapiNormalizer; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like @leonard84 's approach since it's bad to set maps and collections to null in general
import java.util.*; | ||
import java.util.stream.Collectors; | ||
|
||
public class OpenAPINormalizer { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how will each generator extend and modify this class ? since each generator extends DefaultCodgen
and can only assign true or false to Map entries
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how will each generator extend and modify this class ?
Normalizer is similar to the inline model resolver (not generator specified). Again these operations (massaging the spec) are done before any generators further process the spec.
If later there's such use case in which the rules are added for just one particular generator, I still do not foresee issue as the rules should be switched off by default.
Again good question and we'll see how to best handle rules that are tailor-made for just one particular generator as these use cases surface.
@@ -661,6 +677,7 @@ public ClientOptInput toClientOptInput() { | |||
config.schemaMapping().putAll(generatorSettings.getSchemaMappings()); | |||
config.inlineSchemaNameMapping().putAll(generatorSettings.getInlineSchemaNameMappings()); | |||
config.inlineSchemaNameDefault().putAll(generatorSettings.getInlineSchemaNameDefaults()); | |||
config.openapiNormalizer().putAll(generatorSettings.getOpenAPINormalizer()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this might lead to null reference exception since there are no guards on setOpenAPINormalizer
modules/openapi-generator/src/test/resources/3_0/allOf_extension_parent.yaml
Outdated
Show resolved
Hide resolved
Person: | ||
description: person using x-parent (abstrct) to indicate it's a parent class | ||
type: object | ||
x-parent: "abstract" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest a better name for this than abstract, maybe interface
, since abstract
implies a child object can have only 1 parent
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
e.g. this schema https://github.com/OpenAPITools/openapi-generator/blob/master/modules/openapi-generator/src/test/resources/3_0/allOfMultiParent.yaml#L64-L83
the object has multiple $ref
parents, what happens if one of them is x-parent: true
and the other is x-parent: false
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
another case is dart which supports mixins, so you can have
class A extends B with C,D implements F,G
will we have an x-parent=abstract
and x-parent=mixin
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest a better name for this than abstract, maybe interface, since abstract implies a child object can have only 1 parent
interface
is supported via x-implements
(e.g. Java client generators such as jersey2
library support this extension)
abstract
is just an example here for testing purpose (to ensure its value is not overwritten by the normalizer) but coincidentally there's another PR to make parent abstract
by default in the model.
The goal is eventually allow users to specify the parent type (e.g. abstract
, partial public
, etc)
The logic/implementation in this PR supports multiple parents (using $ref). This is same as before without this PR, which means we already support multiple parents if the languages/implementations supports it or default to just a single parent (first) if the language/implementation does not support multiple inheritance.
Add OpenAPI Normalizer to normalize the spec before further processed by OpenAPI Generator.
Only 1 rule is supported to start with:
REF_AS_PARENT_IN_ALLOF
. When it's set totrue
, the child schema of aallOf
schema will be considered a parent of the object if it's a $ref.This is done by adding
x-parent: true
to the parent schema to indicate it's a parent without the use ofdiscriminator
. (x-parent can also be used to set the type of the parent schema, e.g.x-parent: abstract
to indicate a abstract base class)cc @OpenAPITools/generator-core-team
PR checklist
This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
These must match the expectations made by your contribution.
You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example
./bin/generate-samples.sh bin/configs/java*
.For Windows users, please run the script in Git BASH.
master
(6.3.0) (minor release - breaking changes with fallbacks),7.0.x
(breaking changes without fallbacks)