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.

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 schema mappings #12600

Merged
merged 4 commits into from
Jul 3, 2022
Merged

Add schema mappings #12600

merged 4 commits into from
Jul 3, 2022

Conversation

wing328
Copy link
Member

@wing328 wing328 commented Jun 15, 2022

Add schema mappings

To fix #11506

FYI @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.0.1) (patch release), 6.1.x (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.

@wing328 wing328 marked this pull request as ready for review June 18, 2022 10:22
@wing328 wing328 added this to the 6.0.1 milestone Jun 18, 2022
@wing328 wing328 changed the title Add schema mapping Add schema mappings Jun 18, 2022
@pschichtel
Copy link

pschichtel commented Jun 21, 2022

I just tried this and managed to compile it. It took a while since 6.x introduced a bunch of other changes as well.

At times I wasn't entirely sure when to use importMappings and when to use schemaMappings. I think I still have a bunch of importMappings left over, that are not needed.

@wing328
Copy link
Member Author

wing328 commented Jun 22, 2022

@pschichtel thanks for testing the change.

Usually importMappings is used with typeMappings to import the new type specified in the typeMappings, e.g.

--type-mappings URI=URL --import-mappings URL=java.net.URL

(for java client)

Without the importMappings, the auto-generated code above won't compile.

@pschichtel
Copy link

pschichtel commented Jun 22, 2022

Ok, so basically we can map types, which are not generated, to other types and introduce import mappings for those.

we basically do 3 things:

  1. we map a few standard types (e.g. URI)
  2. we introduce a bunch of new formats with mappings such es integer+duration -> Duration, integer+timestamp, and a bunch more
  3. we generate clients, servers and shared models for a bunch of OAI specs. we use import mappings to remap model mappings to the correct packages in the various generated servers and clients.

what I noticed, and I'm not sure if that is related to this feature or something else included in version 6:

    Duration:
      description: An amount of time, measured in milliseconds.
      type: integer
      format: duration
      example: 60000

This type is referenced by a bunch of spec files. We don't generate models for "primitives" (anything but type:object), so normally properties referencing this would end up being a Integer in our generated clients/servers. However we introduce a typeMapping integer+duration -> Duration and an importMapping Duration -> java.time.Duration. That was previously enough to have the clients/servers have Duration for these properties. On this branch with version 6+, it instead generated a model Duration, which I corrected to java.time.Duration by adding a schemaMapping for it.

Is this intended behavior? It feels odd to me, but maybe I'm missing something.

@wing328
Copy link
Member Author

wing328 commented Jun 23, 2022

There was a bug (edge case?) when using importMappings to skip model from being generated and fixed via #11217 so we need to introduce another option schemaMappings instead.

@pschichtel
Copy link

But in these cases there shouldn't even be a model being generated, right? Since it's only a primitive without the type mapping.

@wing328
Copy link
Member Author

wing328 commented Jul 3, 2022

I couldn't repeat the issue with

    Duration:
      description: An amount of time, measured in milliseconds.
      type: integer
      format: duration
      example: 60000

and reference it in a property of another model as follows:

--- a/modules/openapi-generator/src/test/resources/3_0/petstore.yaml
+++ b/modules/openapi-generator/src/test/resources/3_0/petstore.yaml
@@ -736,3 +736,10 @@ components:
           type: string
         message:
           type: string
+        duration:
+          $ref: '#/components/schemas/Duration'
+    Duration:
+      description: An amount of time, measured in milliseconds.
+      type: integer
+      format: duration
+      example: 60000

command:

java -jar modules/openapi-generator-cli/target/openapi-generator-cli.jar generate -g java -i modules/openapi-generator/src/test/resources/3_0/petstore.yaml -o /tmp/java-okhttp/ --type-mappings integer+duration=Duration --import-mappings Duration=java.time.Duration

I don't see any "Duration" model file generated.

@wing328
Copy link
Member Author

wing328 commented Jul 3, 2022

Let's merge this and we can always submit another PR to further improve it.

@wing328 wing328 merged commit 2d3bfaf into master Jul 3, 2022
@wing328 wing328 deleted the schema-mapping branch July 3, 2022 09:46
@pschichtel
Copy link

Since the 6.0.1 milestone is already overdue, is there already a plan for when to release this?

@wing328
Copy link
Member Author

wing328 commented Jul 4, 2022

@mickroll
Copy link
Contributor

mickroll commented Jul 5, 2022

@pschichtel @wing328
At least some documentation about schemaMappings would have been nice.

@Gama11
Copy link
Contributor

Gama11 commented Jul 15, 2022

@wing328
Copy link
Member Author

wing328 commented Jul 17, 2022

@mickroll added via #12892

@mickroll
Copy link
Contributor

Great, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG][openapi-generator-maven-plugin] importmapping not respected since 5.4.0
4 participants