[java] Referenced schema is not generated properly

[padamstx]

Description

After applying the fix for issue #1624 (PR #1625), the attached schema results in incorrect code generation for the "java" generator, although I suspect other generators would suffer from the same fate.
Prior to applying the fix for #1624, the generator throws an NPE :)

The crux of the problem is that the java generator will generate the "Message" class as expected, with the "context" field being declared as a "Context" object. Unfortunately, the Context model is not generated because it is apparently considered to be a top-level map.

One might think that a possible solution would be to generate the Message.context field so that it is of type Map<String, Object> since the Context model is optimized away after being interpreted as a top-level map. But in my opinion, since Context is a referenced schema, we should not be optimizing that away. It is a distinct model defined in the apispec as an object that can have arbitrary properties set on it and is referenced from a schema property belonging to the Message schema. Therefore, it should be rendered as such... i.e. we should generate a class called Context which is a subclass of Map<String, Object>. At the very least, there should probably be a configuration property that can enable/disable the feature introduced by issue #1296.

I could see this schema flattening being done if the Message.context schema property's schema was an inline schema, but not in the case where Message.context's schema is a reference to another named schema.

openapi-generator version

master (latest)

OpenAPI declaration file content or url

Here's the apispec which exhibits the problem:
testcase.zip

Command line used for generation

Using latest master code + fix for issue #1624:

java -jar <location-of-jar>/openapi-generator-cli.jar generate -i testcase.json -g java -o ./java/testcase

Steps to reproduce

1. Process the apispec using the command above
2. Observe the emitted Message.java class; the "context" field is of type Context, but the Context model did not get generated.
   Note: if the fix for NPE in ModelUtils.isFreeFormObject #1624 is not applied, then the result will be an NPE

Related issues/PRs

#1296

Suggest a fix/enhancement

Either revert the changes in #1296 or support a configuration property that can disable that feature.

[jmini]

Related issues/PRs:

We have discussed aliasing java Map<?,?> and List<?> in #472 (comment)

I agree that if we do not generate the Context class (because it extends a Map), then Message can not reference it.

[wing328]

@padamstx thanks for reporting the issue. I could repeat it with the latest master. I'll look into it over the weekend to see if I can come up with a fix to better handle the alias to map.

[padamstx]

@wing328 I can understand the reasoning behind the changes to implement the "alias to map" feature, but in our (IBM Cloud developer experience) code generators, we would not want that feature triggered. In the context of my testcase above, we'd want Context to be generated even if that means that it is simply a class with no fields of its own that subclasses Map<String, Object>.
So, perhaps the solution to this issue would be:

1. fix the inconsistency that I identified above
2. a broader change that allows the "alias to map" feature to be enabled/disabled via a config property.

While we're discussing this, I'll also mention that over the last few weeks while I've been working to port our generators from swagger-codegen to openapi-generator to pick up openapi 3 support, I've been tripped up here and there by the order in which the "isMapSchema" and "isObjectSchema" checks are made in a few places.
The logic in "isMapSchema" overlaps slightly with the logic in "isObjectSchema" so the order of the checks matters. IIRC, an empty (no properties) object schema with additionalProperties defined will result in "true" being returned by both methods of ModelUtils.
We would always want to prefer the "object schema" path rather than the "map schema" path (tbh, I'm not sure that we really ever have a need or use for "map schema"). For example, in my local sandbox, within ModelUtils.unaliasSchema(), I needed to re-order the checks so that the "isObjectSchema" check is done before the "isMapSchema" check... and I'm not yet sure how to reconcile this with the current code in the master branch (my work is currently based on the 3.3.3 release, with plans to move up to the most recent stable release (3.3.4?) soon).

[wing328]

we'd want Context to be generated even if that means that it is simply a class with no fields of its own that subclasses Map<String, Object>.

@padamstx understood. Here are some follow-up questions:

1. Is it correct to say that alias to map (in other words, not generating the map as a model/class) is also acceptable in your use case but you prefer subclassing Map instead?

2. According to https://softwareengineering.stackexchange.com/a/246318 (there's a link to an IBM article), it's an anti-pattern. From what I know it's not a common practice to subclass map so please share with me the use case of subclassing map if you don't mind.

3. I think another valid reason is that the previous version outputs the map as a standalone class and people want to simply keep it that way to avoid breaking changes to the auto-generated SDKs (in other words, users consuming the SDK needs to update their code accordingly as the map models no longer exist)

We definitely want to take this opportunity to understand more about the preference for subclassing map instead of treating it as an alias to map.

[padamstx]

Is it correct to say that alias to map (in other words, not generating the map as a model/class) is also acceptable in your use case but you prefer subclassing Map instead?

@wing328 Actually, I don't think the alias to map approach will work in our situation. In our use case, we have a handful of custom generators (java, go, node.js, swift, python, ruby, apex - salesforce java-like language - plus a few others) which attempt to generate code using patterns that are consistent across those languages... or at least as consistent as is reasonable, accounting for language differences. Our goal is to provide a set of SDKs for each IBM Watson service on the IBM Cloud such that a client programmer (user of one or more services) could easily switch between languages when using a particular service without needing to learn a whole new SDK. Thinking about the other dimension, we also want to deliver SDKs that are consistent within a language across the various services as well.
One overriding issue we have is that we do, in fact, try to maintain compatibility as we deliver each new verson of a particular generated SDK, and in this case we will be developing our custom generators on top of openapi-generator such that they generate code that is the same (or as similar as possible) as our swagger 2.0 generators that run on top of swagger-codegen.

From what I know it's not a common practice to subclass map so please share with me the use case of subclassing map if you don't mind.

Haha, foiled by my own colleagues :) I skimmed through that article and it certainly makes sense for the typical large/complex application situation. But, in our case because the "Context" type (in my example above) is a named/referenced type, we'd want that to remain visible in the generated code, especially if we wanted to extend it later by adding properties to it. By adding a single property to the Context schema, we'd of course end up with a Context class with that new property and it would still subclass HashMap in that case. And this gets into your question 3 above about compatibility. We need to maintain compatibility (as much as we can at least) with the prior code that we've generated, but also try to lay the groundwork to generate code in the future that is compatible with what we might generate currently with our new generators.

This might just be a simple matter of competing requirements... perhaps it makes sense for the generators that come with openapi-generator to implement the "alias to map" feature as you have recently done, while our custom generators would need to use our current "subclass map" approach. In order for that to succeed, I think we'd need some sort of option (perhaps in DefaultCodegen) that we can use to enable/disable this feature. Perhaps this option could be implemented as a simple function (e.g. "DefaultCodegen.shouldAliasMap()" that we override and always return false, or something along those lines), or I guess a config property would work also as we could simply programmatically set it to our desired value at the beginning of our code generators.

[jmini]

As discussed in #472 if a flag to create alias for List/Map is introduced, the default value should be not to create the aliases (and of course not to use them in the other models).

Flag can be activated in special cases like this one or also if users wants to get back the behavior they had with Swagger-Codegen v2.x

[wing328]

Our goal is to provide a set of SDKs for each IBM Watson service on the IBM Cloud such that a client programmer (user of one or more services) could easily switch between languages when using a particular service without needing to learn a whole new SDK

Understood. I had the same goal when started contributing to the code base a few years back but later I got feedback from developers with different preferences when using a SDK of a particular programming language (e.g. returning an ApiResponse object vs returning a list containing response payload, headers and status code). We still want to achieve that as we've been providing many different options to meet different developer's preferences and we hope your team will be able to deliver a consistent developer experience using OpenAPI Generator.

As @jmini has pointed out, we'll probably add a switch/flag to allow users to control the behaviour (subclassing map vs alias to map). I've filed #1638 to fix the alias to map and will submit another PR to add the switch later before the 4.0.0 release.

[padamstx]

Thanks @wing328... sounds good.

[wing328]

Just merged #1638. Will leave this issue opened for tracking until we introduce the flag/switch.