Support more metadata in SwaggerConf

wanno-drijfhout commented 4 years ago

For our project, we need the swagger-maven-plugin to emit securityScheme (under components) and we want to use server URL variables. The OpenAPI spec supports those, but the swagger-maven-plugin doesn't implement those. This PR adds support for those data fields and objects. Additionally, it adds comments where there weren't any.

Note: this is still incomplete, because components has many more (useful) things to specify, like examples. However, I think this PR is satisfactory for now and in line with the existing codebase.

Alternative idea

I do think an alternative architecture could be considered: I think the <swaggerConfig> field could just be raw OpenAPI 3.0 YAML/JSON (or a syntactic XML-equivalent). Instead of re-modelling every OpenAPI construct in Java, the plugin could just parse the raw OpenAPI spec "template" and use it as basis for further modification. I think CDATA or simply referring to an actual YAML/JSON "template" file should work. I do hope this approach would still allow Maven variable substitution (e.g., to inject version numbers compile-time). Opinion?

wanno-drijfhout commented 4 years ago

Seems that there's yet another non-determinism issue:

[INFO] -------------------------------------------------------
[INFO]  T E S T S
[INFO] -------------------------------------------------------
[INFO] Running io.openapitools.swagger.GenerateMojoIT
[main] INFO org.reflections.Reflections - Reflections took 38 ms to scan 1 urls, producing 16 keys and 21 values 
[main] INFO org.reflections.Reflections - Reflections took 10 ms to scan 1 urls, producing 16 keys and 21 values 
[main] INFO org.reflections.Reflections - Reflections took 8 ms to scan 1 urls, producing 16 keys and 21 values 
[main] INFO org.reflections.Reflections - Reflections took 19 ms to scan 1 urls, producing 16 keys and 21 values 
[main] INFO org.reflections.Reflections - Reflections took 18 ms to scan 1 urls, producing 16 keys and 21 values 
[main] INFO org.reflections.Reflections - Reflections took 10 ms to scan 1 urls, producing 16 keys and 21 values 
[main] INFO org.reflections.Reflections - Reflections took 8 ms to scan 1 urls, producing 16 keys and 21 values 
[main] INFO org.reflections.Reflections - Reflections took 9 ms to scan 1 urls, producing 16 keys and 21 values 
[main] INFO org.reflections.Reflections - Reflections took 8 ms to scan 1 urls, producing 16 keys and 21 values 
[main] INFO org.reflections.Reflections - Reflections took 7 ms to scan 1 urls, producing 16 keys and 21 values 
[main] INFO org.reflections.Reflections - Reflections took 8 ms to scan 1 urls, producing 16 keys and 21 values 
[main] INFO org.reflections.Reflections - Reflections took 8 ms to scan 1 urls, producing 16 keys and 21 values 
[main] INFO org.reflections.Reflections - Reflections took 8 ms to scan 1 urls, producing 16 keys and 21 values 
[main] INFO org.reflections.Reflections - Reflections took 14 ms to scan 1 urls, producing 16 keys and 21 values 
[main] INFO org.reflections.Reflections - Reflections took 10 ms to scan 1 urls, producing 16 keys and 21 values 
[INFO] Tests run: 8, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 2.138 s - in io.openapitools.swagger.GenerateMojoIT
[INFO] 
[INFO] Results:
[INFO] 
[INFO] Tests run: 8, Failures: 0, Errors: 0, Skipped: 0

or sometimes:

org.junit.ComparisonFailure: expected:<...n":"The Subdomain","[default":"services","enum":["services","web","main"]],"x-custom-property"...> but was:<...n":"The Subdomain","[enum":["services","web","main"],"default":"services"],"x-custom-property"...>
    at org.junit.Assert.assertEquals(Assert.java:115)
    at org.junit.Assert.assertEquals(Assert.java:144)
    at io.openapitools.swagger.GenerateMojoIT.assertFileContentEquals(GenerateMojoIT.java:42)
    at io.openapitools.swagger.GenerateMojoIT.assertJsonEquals(GenerateMojoIT.java:32)
    at io.openapitools.swagger.GenerateMojoIT.testGenerate(GenerateMojoIT.java:82)
    at io.openapitools.swagger.GenerateMojoIT.testGenerateFull(GenerateMojoIT.java:118)
    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.lang.reflect.Method.invoke(Method.java:498)
    at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50)
    at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
    at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47)
    at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
    at org.apache.maven.plugin.testing.MojoRule$2.evaluate(MojoRule.java:308)
    at org.junit.rules.RunRules.evaluate(RunRules.java:20)
    at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:325)
    at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:78)
    at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:57)
    at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290)
    at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71)
    at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288)
    at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58)
    at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268)
    at org.junit.runners.ParentRunner.run(ParentRunner.java:363)
    at org.eclipse.jdt.internal.junit4.runner.JUnit4TestReference.run(JUnit4TestReference.java:89)
    at org.eclipse.jdt.internal.junit.runner.TestExecution.run(TestExecution.java:41)
    at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:542)
    at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:770)
    at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.run(RemoteTestRunner.java:464)
    at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.main(RemoteTestRunner.java:210)

Don't know what could be the cause, though...

langecode commented 4 years ago

Thanks for the contribution. I’ll take a look as soon as possible.

wanno-drijfhout commented 4 years ago

Thanks for the contribution. I’ll take a look as soon as possible.

Thanks! So, I've done some investigation and I have an hypothesis: the ObjectMapper serializes the ServerVariable object non-deterministically because there's no annotations on the proper serialization order. However, if that hypothesis is correct, I'd expect we'd have had this particular problem a lot earlier for other classes, because ServerVariable is just a POJO.

Because I cannot modify ServerVariable (as it's part of the swagger core library), we'd have to work around it. I see the following options:

either by coercing the ObjectMapper in the OutputFormat class to serialize ServerVariable instances differently
or by setting mapper.enable(MapperFeature.SORT_PROPERTIES_ALPHABETICALLY) in the OutputFormat class (which would upset the ordering of the entire OpenAPI specification file, however).

Both options seem unappealing to me, so I hope I'm wrong (or that there is yet another alternative). I will await your guidance on this matter.

wanno-drijfhout commented 4 years ago

@langecode Would you have the time to review/collaborate on this PR soon :-) ?

langecode commented 4 years ago

Yeah. Just have been staring up on a new job. It seems it is only non-deterministic on Java 8? Or at least I cannot provoke the error running the test on higher VM versions?

But I have checked out the PR and will go through it.

langecode commented 4 years ago

So finally I had a bit of time looking through this - sorry for the wait.

I see your point with the need to re-model the OpenAPI domain model. I think originally we did this to ease the migration from the kongchen plugin - as we were using that but running into some problems. For now I like your approach - may yield a bit more information in the Maven output in case of failure. But it could be worth considering another approach in a new major version.

For the concrete problem, I have a theory however I am not sure. If I remember correctly Jackson uses the Java Bean API to discover properties of the POJOs. Looking at the error from the test and the other serializations that do not fail I have a hypothesis. The only thing standing out with the ServerVariable class is the choice of modelling some properties with _. My thought is that for Java SE >8 the order of the beans is base on the order of the getters (as looking for getters is one of the discovery methods) however for Java SE 8 the properties defined with _ are put after other properties. This is only a theory but the _ fields seems to be the only thing distinguishing the ServerVariable.

I started out doing a more semantic validation of the output - however it seems many commit the output into Git and want a deterministic output. But as you, I haven't any really good solution to this. I would lean towards a fix for just ServerVariable - I have tried for that along with setting up a bit of default values for empty sets and maps (https://github.com/openapi-tools/swagger-maven-plugin/tree/wanno-drijfhout-more-metadata).

langecode commented 4 years ago

Closed by #58

openapi-tools / swagger-maven-plugin

Support more metadata in SwaggerConf #53

Alternative idea