OpenAPITools / openapi-generator

OpenAPI Generator allows generation of API client libraries (SDK generation), server stubs, documentation and configuration automatically given an OpenAPI Spec (v2, v3)
https://openapi-generator.tech
Apache License 2.0
21.63k stars 6.53k forks source link

[REQ] Log a warning for use of allOf without discriminator #4189

Open jimschubert opened 5 years ago

jimschubert commented 5 years ago

Is your feature request related to a problem? Please describe.

No, common usage errors.

Describe the solution you'd like

Evaluate models after parsing, and if any are allOf, but the target schema doesn't have a discriminator field, trigger this as a warning.

We currently only display warnings if there are already errors in the spec (see https://github.com/OpenAPITools/openapi-generator/blob/f43c720b08496e1144ce64317bc060277e4acec2/modules/openapi-generator/src/main/java/org/openapitools/codegen/config/CodegenConfigurator.java#L458).

I think warnings should be moved out of the error condition and display those plus the one suggestion in this PR regardless of errors.

We'd need to define whether or not setting the strict spec flag to false would hide this warning.

Describe alternatives you've considered

n/a

Additional context

I was emailed directly by a user whose team upgraded from OpenAPI 2.0 to OpenAPI 3.0 documentation. His team didn't understand why base classes would generate in the 3.x branch but not in the 4.x branch. I explained to him that it was common for people to miss that the spec requires a discriminator property, because it's not a very intuitive requirement. Ideally, our tooling should provide this information to all users.

wing328 commented 5 years ago

What about documenting this in a migration guide from OAS 2.0 to 3.0?

The user may be using allOf (OAS 2.0) for model composition, which behaves the same after migration to OAS 3.0

jimschubert commented 5 years ago

Good point about composition. I think we might want to evaluate allOf in it's entirety, because we appear to have previously done inheritance where it should've been composition where each ref is validated independently.

I think documentation would be fine, although I doubt that someone will go to our docs to understand the behavior... they're more likely to see it as generator output.

philippendrulat commented 4 years ago

I am currently confused about the warning that using allOf without discriminator.propertyName is deprecated. The documentation itself gives an example without discriminator when using allOf, which we did. Do we really need to introduce another property that confuses the client of the API to get rid of the warning?

Our API uses allOf like the petstore example and we get the deprecation warning.

We are using gradle with the parameters:

openApiGenerate {
    generatorName = 'jaxrs-cxf'
    inputSpec = "$rootDir/src/main/resources/openapi/openapi-spec.yaml"
    generateModelTests = false
    generateApiTests = false
    configOptions = [
        dateLibrary: "java8",
        java8: "true"
    ]
}

Is this a new bug? Is this what you initially described? Like I said, I am confused.

jwalton commented 4 years ago

Discriminators are definitely not required in OpenAPI 3.0: https://github.com/OAI/OpenAPI-Specification/blob/master/versions/3.0.1.md#discriminatorObject

There's even discussion about removing the discriminator entirely, so openapi-generator probably shouldn't rely on it: https://github.com/OAI/OpenAPI-Specification/issues/2143