[BUG] Invalid warning about inline schema usage in allOf

[typhoon2k]

Bug Report Checklist

• [x] Have you provided a full/minimal spec to reproduce the issue?
• [x] Have you validated the input using an OpenAPI validator (example)?
• [x] What's the version of OpenAPI Generator used?
• [x] Have you search for related issues/PRs?
• [x] What's the actual output vs expected output?
• [ ] [Optional] Bounty to sponsor the fix (example)

Description

Generator is logging More than one inline schema specified in allOf:. Only the first one is recognized. All others are ignored. warning, when generating Java, C# or TypeScript code from schema that is using allOf without discriminator and with more than 2 referenced schemas.

openapi-generator version

4.3.x, 5.0.0

OpenAPI declaration file content or url

Example file allOf_composition.yaml can be used to repeat this problem (schema SuperMan). We have a use case that has 3 refs (and no inline schemas at all), which leads to the same warning.

Generation Details

    java \
        -jar openapi-generator-cli-5.0.0-beta.jar \
            generate \
            --input-spec $input_file \
            --generator-name typescript-axios \
            --output $output_folder

Steps to reproduce

1. run generator
2. check logs

Related issues/PRs

There are issues, which are discussing invalid inheritance/composition support and it seems that all of them are fixed now. In our case generated code is correct (Java/C#/TypeScript) and all fields are there; it's a warning that is wrong.

Suggest a fix

Remove obsolete warning?

[typhoon2k]

There are reports about this anomaly in already closed issue - https://github.com/OpenAPITools/openapi-generator/issues/1358#issuecomment-466228501.

[typhoon2k]

It seems that this behavior was introduced long time ago - https://github.com/swagger-api/swagger-codegen/pull/4251/files. According to comment the idea was that counter (which is evaluated for triggering warning message) would be increased only for models with discriminator:

                int modelImplCnt = 0; // only one inline object allowed in a ComposedModel
                for (Model innerModel: ((ComposedModel)model).getAllOf()) {
                    if (innerModel instanceof ModelImpl) {
                        ...
                        if (modelImplCnt++ > 1) {
                            LOGGER.warn("More than one inline schema specified in allOf:. Only the first one is recognized. All others are ignored.");
                            break; // only one ModelImpl with discriminator allowed in allOf
                        }
                    }
                }

I.e. to notify about cases with allOf having multiple schemas with discriminator. Now we have one another if that is checking this.

[typhoon2k]

@wing328 , ping.

[iafilius]

Hi, i see this message (openapi 5.2.0 /master) just 8 times when generating a go client library of a vendor provided swagger spec file of 1.2MB. allOf is used 218 times, but scanning though it manual i did not see a reason for the 8 warnings, but strange, just 8 time of 218 allOf usage. I'm not so sure if that message should be removed, i like to check where it comes from in my case. Adding java option: -Dlog.level=debug does not show it's context, so it's difficult to find. any idea how to see which context of code it is triggered by?

For the record an overview with tested versions and particular logline count.

#version : #count off loglines with vendor provided spec file
4.2.2 : 0
4.2.3 : 0
4.3.1 : <crashed>
5.1.1 : 8
5.2.0 : 8
5.2.1-20210716.042228-7 : 8

So somewhere between 4.2.3 and 5.1.1 it started to show up:

Regards,

[languitar]

Why is this closed? Still seems to happen in the latest release.

[Morriz]

bump

[consultantleon]

I'm seeing this invalid warning logged in 5.4.0 .

Actually all composed models in the allOf are correctly expanded to the (java) model.

Why is PR https://github.com/OpenAPITools/openapi-generator/pull/7446 not updated and merged yet? Multiple allOf models do seem to be composed correctly.

[nbrugger-tgm]

@typhoon2k why was this closed? It seems that this is very much still happening.

[nbrugger-tgm]

Call me crazy but imo this code throws this erro/log for every allOf with more than 2 entries of any type

for (Object innerSchema : composed.getAllOf()) { // TODO need to work with anyOf, oneOf as well
                    if (m.discriminator == null && ((Schema) innerSchema).getDiscriminator() != null) {
                        LOGGER.debug("discriminator is set to null (not correctly set earlier): {}", m.name);
                        m.setDiscriminator(createDiscriminator(m.name, (Schema) innerSchema));
                        modelDiscriminators++;
                    }

                    if (((Schema) innerSchema).getXml() != null) {
                        m.xmlPrefix = ((Schema) innerSchema).getXml().getPrefix();
                        m.xmlNamespace = ((Schema) innerSchema).getXml().getNamespace();
                        m.xmlName = ((Schema) innerSchema).getXml().getName();
                    }
                    if (modelDiscriminators > 1) {
                        LOGGER.error("Allof composed schema is inheriting >1 discriminator. Only use one discriminator: {}", composed);
                    }

                    if (modelImplCnt++ > 1) {
                        LOGGER.warn("More than one inline schema specified in allOf:. Only the first one is recognized. All others are ignored.");
                        break; // only one schema with discriminator allowed in allOf
                    }
                }

copied from DefaultCodegen.java at master

the increment is done every single item WHAT?

[draialexis]

I'm seeing this invalid warning in 7.8.0, in a Java Spring Boot project

[wing328]

UPDATE: merged https://github.com/OpenAPITools/openapi-generator/pull/19646 to reduce logging for these 2 entries to debug

[nbrugger-tgm]

Hi i could be wrong but imo the code I shared also has a logic error.

The loop can never ever run more than 2 times(assuming modImplCnt starts at 0). I am not sure if this is intended. I don't exactly know what this is looping over but imo before the modelImplCnt++ there needs to be an isInlineSchems(inmerSchema) because the current code does not indicate that every element is a inline schema (and even if there are mutltiple inline schemas why would this be a problem warranting to completely break the loop?)

So as it's standing this loop looks very fishy to me.

[nbrugger-tgm]

Also next to the break there is a comment // only one schema with discriminator allowed in allOf

That doesn't match up with the code from my limited knowledge. Since the if checks for every element not only ones with , discriminators.

Maybe the break belongs to the previous if that actually does check for discriminators?

@wing328 how good do you know and understand this codebase? Because to me it looks like there are multiple questionable "things". Also tbh I don't think the logging statement should have been removed/set to debug. It causes errors in codegen and now people won't know why their codegen fails. Previously they at least could find this thread/issue.

[draialexis]

I would encourage each of us to keep a cool head, but also agree that changing the log level probably does not solve this ticket.

I think it could be a good idea to indeed lower the log level temporarily while investigating the bug, depending on circumstances. Solving the bug would close the ticket. The logs could then maybe get reinstated.

If bug there be.

In the absence of a bug, I think the logs should be outright deleted, because they lie.

In my java project using 7.8.0, the generated DTO does seem to be composed of allOf the 4 things I declared in the yaml.

[wing328]

thanks for reviewing these piece of code (which isn't written by me if i remember correctly)

agreed that we need to clean up or review these but since it's not causing issues at the moment, it's not something with the highest priority.

[draialexis]

I understand it may not be a priority, of course.

But closing the issue like this doesn’t say “later”, it says “never”.

Does that make sense?

[wing328]

reopened for tracking