Data object classes for templating context

[rienafairefr]

Sorry in advance for the massive PR, though I can't seem to see how it could be made with less

PR checklist

• Read the contribution guidelines.
• Ran the shell script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh and ./bin/security/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in .\bin\windows\.
• Filed the PR against the correct branch: master, 3.3.x, 4.0.x. Default: master.
• Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

@jimschubert @wing328

Description of the PR

in some discussion with @jimschubert here I've seen that the current plethora of Map<String, Object> pseudo-classes should be avoided. Indeed, it's making us do a lot of casting back and forth between Object and Map<String, Object>, a lot of unchecked casting warnings are suppressed, type safety is not used, overall it's something that could be changed for the better

I've implemented a bunch of data-only classes (suffixed Bundles for now but I could see using DTO or Bag if whatever terminology is preferred) for replacing all these Map<String, Object> that could be replaced. Of course we need to keep the vendorExtensions, additionalProperties maps, and probably others I'm forgetting of the top of my head, unchanged, because these really need to be truly dynamic (they can be changed by the spec or the user).

There is a bit of conundrum with the additionalProperties there because when executing a template, we want to pass it a context that can be a POJO, but we also want the values from additionalProperties to be available (e.g. {{licenseInfo}} or {{appDescription}}. I added a AdditionalPropertiesBundle base class to handle that, but it feels hacky to me: I convert the POJO to a map, and add the additionalproperties to it, just before it goes into the template.execute(context...), it needs a better solution I think.

[jimschubert]

I'll try to review this later this weekend. At first glance, I notice that you're passing "bundle" templating objects around the workflow. That might tie templating too tightly to the workflow (which I feel would be better as procedurally input > transform > templating). I like the idea of getting rid of Model<String, Object> usage everywhere, but I'll need to dig in more deeply to see if those models can just be lifted out of your new templating package.

[rienafairefr]

I agree, for now these are in org.openapitools.codegen.templating, a better home might be org.openapitools.codegen.models or something like that.

The jackson conversion for Bundle->Map is failing hard, because there is cycle in the model (CodegenModel has a children list and these contain a parent field), I need to do it differently :-)

[jimschubert]

I've created a Separation of Concerns project to track some of the larger refactoring efforts. I've added this PR and #690 to that project, because I think they're both directly related to the efforts I've documented in the project.

As I discussed in #690, Map<String, Object> is quite a pain to work with. I was originally only envisioning the model passed to template.execute(context...) as being a strong typed object. This is because I'd like to see the generation workflow redone as well. That is, rather than DefaultGenerator converting an OpenAPI spec into our Codegen* models and dispatching to the DefaultConfig or derived CodedgenConfig instances at each step, I think it would be more maintainable and intuitive to parse the spec, transform it to a "ready" API abstraction, then pass this abstraction into the workflow object, handing off to the framework generator only once for post-processing right before passing it to templating. This reduces the back-and-forth processing and repeated mutations along the way. See #841 and #843 for more in-depth details.

If we change all input parameters to these *Bundle types, this is a major breaking change and would need to retarget merging into the 4.0.0 branch.

Edit: It's considered a major breaking change because a use case for the generator is for consumers to create and artifact with opeanpi-generator as a dependency, and extend CodegenConfig to create new or modified generators. They can compile, add the jar to the class path, and pass the fully qualified type (package path + simple name) to the -g option to use the custom generator. See Customization.md for details.

[rienafairefr]

Thanks @jimschubert. I've gone back and put backward compatibility in place again, not modifying the public interfaces and methods. Methods using the Map<String, Object> are deprecated:
The *Bundle types are just HashMaps with strongly typed getter/setter, with casting, they will be easily moved to strongly typed fields in a next step, maybe for 4.0.x, when we remove the deprecated methods entirely (with all the *Codegen classes using the Bundle type).

The code needs a good squashing, sorry about that :-)

[jimschubert]

@rienafairefr sorry I've just seen your response. I've had a lot of stuff come up lately (I'm building a house and will be moving in about 6 weeks). I'll try to get this reviewed over the next week.

[rienafairefr]

@jimschubert I've rewritten and removed the changes in public interface altogether, the generators still operate on Map<String, Object> as was the case before, it is just the final step (file generation) that uses an almost-strongly-typed object. I say almost because the *Bundle classes are still operable as Map<String, Object> (because we need to add additionalProperties to it for example)

[jimschubert]

@rienafairefr awesome. I'll try to take a look this week. Is there anything you'd like me to try and tackle?