Skip to content

Conversation

galamome
Copy link

fix bug 21680: REMOVE_PROPERTIES_FROM_TYPE_OTHER_THAN_OBJECT normalize option

fixes #21680

Created a new normalize option called REMOVE_PROPERTIES_FROM_TYPE_OTHER_THAN_OBJECT to remove "properties" attribute from type other than "object".
If my PR is accepted, the new option shall be documented in normalizer documentation

I created a new unit test, and tested the whole process with the complete specification (faulty specification) with the following command:

java -jar modules/openapi-generator-cli/target/openapi-generator-cli.jar generate \
-g typescript-fetch \
--additional-properties=supportES6=true,npmVersion=6.9.0,typescriptThreePlus=true \
-i ~/buildDev/bug_openapi/interface/openapi.json \
-o ~/buildDev/bug_openapi/typescript-client-test \
--openapi-normalizer REMOVE_PROPERTIES_FROM_TYPE_OTHER_THAN_OBJECT=true

I also added an advice in modules/openapi-generator/src/main/java/org/openapitools/codegen/InlineModelResolver.java to use --openapi-normalizer REMOVE_PROPERTIES_FROM_TYPE_OTHER_THAN_OBJECT=true when the oddity is encountered.

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 || exit
    ./bin/generate-samples.sh ./bin/configs/*.yaml || exit
    ./bin/utils/export_docs_generators.sh || exit
    
    (For Windows users, please run the script in WSL)
    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*.
    IMPORTANT: Do NOT purge/delete any folders/files (e.g. tests) when regenerating the samples as manually written tests may be removed.
  • File the PR against the correct branch: master (upcoming 7.x.0 minor release - breaking changes with fallbacks), 8.0.x (breaking changes without fallbacks)
  • If your PR solves a reported issue, reference it using GitHub's linking syntax (e.g., having "fixes #123" present in the PR description)
  • If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

@@ -356,7 +356,9 @@ private void gatherInlineModels(Schema schema, String modelPrefix) {
} else if (schema.getProperties() != null) {
// If non-object type is specified but also properties
LOGGER.error("Illegal schema found with non-object type combined with properties," +
" no properties should be defined:\n " + schema.toString());
" no properties should be defined:" +
" consider using --openapi-normalizer REMOVE_PROPERTIES_FROM_TYPE_OTHER_THAN_OBJECT=true\n " +
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the PR

what about automatically fixing it in inline model resolver instead?

[main] ERROR o.o.codegen.InlineModelResolver - Illegal schema found with non-object type combined with properties, no properties should be defined:
 class ArraySchema {
    class Schema {
        type: array
        format: null
        $ref: null
        description: null
        title: null
        multipleOf: null
        maximum: null
        exclusiveMaximum: null
        minimum: null
        exclusiveMinimum: null
        maxLength: null
        minLength: null
        pattern: null
        maxItems: null
        minItems: null
        uniqueItems: null
        maxProperties: null
        minProperties: null
        required: null
        not: null
        properties: {empty=class BooleanSchema {
            class Schema {
                type: boolean
                format: null
                $ref: null
                description: null
                title: null
                multipleOf: null
                maximum: null
                exclusiveMaximum: null
                minimum: null
                exclusiveMinimum: null
                maxLength: null
                minLength: null
                pattern: null
                maxItems: null
                minItems: null
                uniqueItems: null
                maxProperties: null
                minProperties: null

so that we can avoid another normalizer rule.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello,

I don't have an opinion about where the normalisation should be done. Mattias-Sehlstedt commented in the bug report, and advised to put it in the normalizer since it solves an oddity.
My thought was to leave the possibility to opt in to apply the normalizer rule and remove this oddity.

Maybe in order to avoid to create another normalizer rule, I could modify my PR in order to leave it in the normalizer but make it default. The drawback is that we would loose the possibility to opt out (and keep the "properties" attribute for a schema other than object, which is the oddity).
Do you think that would fit ?

By the way, I think "additionalProperties" attribute also should be removed for a schema other than object. At least it is what modules/openapi-generator/src/main/java/org/openapitools/codegen/InlineModelResolver.java says.

Looking forward to solve this bug, thanks to your advice,

Guillaume

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

Successfully merging this pull request may close these issues.

2 participants