Add OpenAPI Normalizer by wing328 · Pull Request #14172 · OpenAPITools/openapi-generator

wing328 · 2022-12-05T03:15:32Z

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 to true, the child schema of a allOf 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 of discriminator. (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

Read the contribution guidelines.
Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
Run the following to build the project and update samples:
```
./mvnw clean package 
./bin/generate-samples.sh
./bin/utils/export_docs_generators.sh
```
Commit all changed files.
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.
File the PR against the correct branch: master (6.3.0) (minor release - breaking changes with fallbacks), 7.0.x (breaking changes without fallbacks)
If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

leonard84 · 2022-12-05T09:53:58Z

modules/openapi-generator-cli/src/main/java/org/openapitools/codegen/cmd/ConfigHelp.java

        }

+        if (Boolean.TRUE.equals(openapiNormalizer)) {
+            sb.append(newline).append("OPENAIP NORMALIZER RULEA").append(newline).append(newline);


🧐 This looks like a typo OPENAIP
Also, is RULEA an example, or also a typo?

leonard84 · 2022-12-05T09:57:13Z

modules/openapi-generator-cli/src/main/java/org/openapitools/codegen/cmd/ConfigHelp.java

+            Map<String, String> map = config.openapiNormalizer()
+                    .entrySet()
+                    .stream()
+                    .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue, (a, b) -> {


❔ 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.

leonard84 · 2022-12-05T09:58:29Z

.../openapi-generator-core/src/main/java/org/openapitools/codegen/config/GeneratorSettings.java

        schemaMappings = Collections.unmodifiableMap(new HashMap<>(0));
        inlineSchemaNameMappings = Collections.unmodifiableMap(new HashMap<>(0));
        inlineSchemaNameDefaults = Collections.unmodifiableMap(new HashMap<>(0));
+        openapiNormalizer = Collections.unmodifiableMap(new HashMap<>(0));


❓ Why not use Collections.emptyMap() if it should be unmodifable in any case?

leonard84 · 2022-12-05T09:59:27Z

.../openapi-generator-core/src/main/java/org/openapitools/codegen/config/GeneratorSettings.java

+         * @return a reference to this Builder
+         */
+        public Builder withOpenAPINormalizer(Map<String, String> openapiNormalizer) {
+            this.openapiNormalizer = openapiNormalizer;


❔ 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.

I like @leonard84 's approach since it's bad to set maps and collections to null in general

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

leonard84 · 2022-12-05T13:20:22Z

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

+        }
+
+        if (schema instanceof ArraySchema) {
+            if (schema.getItems() != null) {


🤔 I would extract every block of the enclosing if-else into its own method to reduce the complexity/length of this method.

leonard84 · 2022-12-05T13:20:57Z

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

+    }
+
+    private void normalizeNonComposedSchema(Schema schema, Set<Schema> visitedSchemas) {
+


❔ is this intentionally empty, maybe add a comment?

leonard84 · 2022-12-05T13:23:18Z

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

+                    refSchema.getExtensions().put("x-parent", true);
+                } else {


If you remove the else and make it a normal block, you can omit this line.

leonard84 · 2022-12-05T13:29:10Z

...les/openapi-generator/src/test/java/org/openapitools/codegen/java/JavaClientCodegenTest.java

+        Assert.assertEquals(files.size(), 24);
+        validateJavaSourceFiles(files);
+
+        TestUtils.assertFileContains(Paths.get(output + "/src/main/java/xyz/abcdef/model/Child.java"),


❔ isn't this missing a test for Person to be an abstract class.

leonard84 · 2022-12-05T13:35:06Z

...les/openapi-generator/src/test/java/org/openapitools/codegen/java/JavaClientCodegenTest.java

+        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 {");


I'm missing a test for the error case, i.e., an allOf with multiple refs. Also, one with multiple layers of inheritance.

leonard84 · 2022-12-06T08:58:01Z

modules/openapi-generator/src/test/resources/3_0/allOf_extension_parent.yaml

@@ -0,0 +1,80 @@
+openapi: 3.0.1


This time no sample code? Is it because it's an unofficial extension?

wing328 · 2022-12-06T14:26:41Z

@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?

leonard84 · 2022-12-06T17:43:26Z

I was wondering, how hard would it be to support x-parent: interface and enable it to have the parent properties defined via interface, allowing for one parent class and multiple interfaces?

leonard84 · 2022-12-07T08:36:59Z

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?

It seems to work fine

leonard84 · 2022-12-07T08:46:02Z

Although, the x-parent: abstract doesn't seem to add the modifier.

wing328 · 2022-12-07T15:31:31Z

@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

ahmednfwela · 2022-12-21T08:45:17Z

.../openapi-generator-core/src/main/java/org/openapitools/codegen/config/GeneratorSettings.java

+         * @return a reference to this Builder
+         */
+        public Builder withOpenAPINormalizer(Map<String, String> openapiNormalizer) {
+            this.openapiNormalizer = openapiNormalizer;


I like @leonard84 's approach since it's bad to set maps and collections to null in general

ahmednfwela · 2022-12-21T09:02:09Z

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

+import java.util.*;
+import java.util.stream.Collectors;
+
+public class OpenAPINormalizer {


how will each generator extend and modify this class ? since each generator extends DefaultCodgen and can only assign true or false to Map entries

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.

ahmednfwela · 2022-12-21T09:04:02Z

...les/openapi-generator/src/main/java/org/openapitools/codegen/config/CodegenConfigurator.java

        config.schemaMapping().putAll(generatorSettings.getSchemaMappings());
        config.inlineSchemaNameMapping().putAll(generatorSettings.getInlineSchemaNameMappings());
        config.inlineSchemaNameDefault().putAll(generatorSettings.getInlineSchemaNameDefaults());
+        config.openapiNormalizer().putAll(generatorSettings.getOpenAPINormalizer());


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

ahmednfwela · 2022-12-21T09:14:17Z

modules/openapi-generator/src/test/resources/3_0/allOf_extension_parent.yaml

+    Person:
+      description: person using x-parent (abstrct) to indicate it's a parent class
+      type: object
+      x-parent: "abstract"


I suggest a better name for this than abstract, maybe interface, since abstract implies a child object can have only 1 parent

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 ?

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 ?

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.

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

wing328 added 5 commits December 3, 2022 01:04

add x-parent support

784d0e0

add docstring

a886d89

add openapi normalizer rule to use ref as parent in allof

aac013e

add openapi normalizer with 1 rule

7c6dfe5

revise wordings

b5d14a1

wing328 requested a review from jimschubert as a code owner December 5, 2022 03:15

wing328 added the Enhancement: Feature label Dec 5, 2022

wing328 added this to the 6.3.0 milestone Dec 5, 2022

fix javadoc warnings

e0ec0bb

wing328 marked this pull request as draft December 5, 2022 03:45

better test

c5d0249

leonard84 reviewed Dec 5, 2022

View reviewed changes

fix docstring

4bdfb04

leonard84 reviewed Dec 6, 2022

View reviewed changes

wing328 mentioned this pull request Dec 15, 2022

Use Gradle 7.6 to build Gradle plugin #13860

Merged

5 tasks

wing328 added 2 commits December 16, 2022 14:58

Merge remote-tracking branch 'origin/master' into allof-parent-type

3ea7159

minor update

e70c01c

wing328 added the Feature: Generator label Dec 16, 2022

minor improvements

034a9ba

This was referenced Dec 20, 2022

[BUG] No more inheritance for components generated by version 5.0.0 #8495

Closed

[BUG] [Java] "allOf" does not support inheritance #9756

Closed

[BUG] Model inheritance in Java and Swift is broken in v6.0.0 #12650

Open

[BUG] Inheritance is broken #2845

Closed

wing328 marked this pull request as ready for review December 21, 2022 07:21

ahmednfwela reviewed Dec 21, 2022

View reviewed changes

fix typo

f58232b

wing328 mentioned this pull request Dec 22, 2022

Nullable subschemas #13850

Merged

ahmednfwela mentioned this pull request Dec 27, 2022

[REQ] Add parameter to standardize handling null types in all language #12257

Open

wing328 merged commit b71aecb into master Dec 30, 2022

wing328 deleted the allof-parent-type branch December 30, 2022 08:03

jpraet mentioned this pull request Jan 2, 2023

Evaluate OpenAPI tooling belgif/rest-guide#25

Closed

billybraga reviewed Jan 9, 2023

View reviewed changes

modules/openapi-generator/src/main/java/org/openapitools/codegen/OpenAPINormalizer.java Show resolved Hide resolved

alexhocks reviewed Jan 13, 2023

View reviewed changes

modules/openapi-generator/src/main/java/org/openapitools/codegen/OpenAPINormalizer.java Show resolved Hide resolved

kzander91 mentioned this pull request Apr 6, 2023

[BUG][JAVA][SPRING] Model Inheritance over multiple layers with REF_AS_PARENT_IN_ALLOF=true, allOf and required parameters produces compilation error #15148

Closed

5 tasks

		}

		private void normalizeNonComposedSchema(Schema schema, Set<Schema> visitedSchemas) {

Uh oh!

Conversation

wing328 commented Dec 5, 2022

PR checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wing328 commented Dec 6, 2022

Uh oh!

leonard84 commented Dec 6, 2022

Uh oh!

leonard84 commented Dec 7, 2022

Uh oh!

leonard84 commented Dec 7, 2022

Uh oh!

wing328 commented Dec 7, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wing328 Dec 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

wing328 Dec 21, 2022 •

edited

Loading