[ruby] Fix init/serialization/deserialization of map/array model aliases

[zippolyte]

Follow up to #7419
Currently, trying to instantiate such an object fails as the generic initialize method doesn't work at all when the parent is an Array or Hash.
In the same way, serializing and deserializing doesn't work the generic way, so this is adding specific templates for the map/array aliases

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.
• If contributing template-only or documentation-only changes which will change sample output, build the project beforehand.
• Run the shell script ./bin/generate-samples.shto update all Petstore samples related to your fix. 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*. For Windows users, please run the script in Git BASH.
• File the PR against the correct branch: master
• Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

cc @cliffano (2017/07) @zlx (2017/09) @autopp (2019/02)

[auto-labeler]

👍 Thanks for opening this issue!
🏷 I have applied any labels matching special text in your issue.

The team will review the labels and make any necessary changes.

[jfeltesse-mdsol]

Not sure I understand the concept, what problem is the whole "alias" feature solving?

[zippolyte]

Well first of all, it's a feature that's supported in a bunch of different generators. Not sure exactly what was the origin of this feature, but more recently, it had to be used in the oneOf implementation for go, in case oneOf members are slices or maps.
Given that it's now used in our API spec, we have to implement it properly in the new languages we want to generate.

[jfeltesse-mdsol]

But what does the feature provide? Do you have an example where it's useful?

All languages are different and will have different ways to deal with things, I don't think "language X and Y do this" should be a reason.

[zippolyte]

Honestly I don't. We never used it until we needed it for oneOf in go.

[zippolyte]

@autopp, some additional feedback on this from the technical committee would be awesome, thanks 🙇‍♂️

[wing328]

Honestly I don't. We never used it until we needed it for oneOf in go.

I think the Go client generator should be updated to support array/map as a primitive type instead of being a model. (same for the Ruby client generators)

[jfeltesse-mdsol]

I don't think this is needed anymore since I implemented support for primitives, arrays and hashes for oneOf in #5706

Or maybe your use case here is yet different?

[zippolyte]

Well IMO it's either we fix it completely, or we completely remove the feature for this generator. Right now it's kinda half supported, meaning the ruby generator has a different behavior based on the generateAliasAsModel feature flag being true or false, but when true it generates code that just doesn't work.

If you don't feel like supporting this feature at all, I could try to see if I can work on making this generator completely ignore the generateAliasAsModel feature flag. WDYT ?

[jfeltesse-mdsol]

The problem is I just don't understand what the feature does. Examples of how the output would change with and without that flag set would help.

[zippolyte]

To generate alias (array, list, map) as model. When false, top-level objects defined as array, list, or map will result in those definitions generated as top-level Array-of-items, List-of-items, Map-of-items definitions. When true, A model representation either containing or extending the array,list,map (depending on specific generator implementation) will be generated.

from https://github.com/OpenAPITools/openapi-generator/blob/master/modules/openapi-generator-gradle-plugin/README.adoc

or

Defines whether primitive types defined at the model/schema level will be wrapped in a model

from https://openapi-generator.tech/docs/globals/

Basically, when true, the generator would generate a model for the primitive types, and use that model in the code instead of directly using the type it aliases. For instance, let's say you have a schema defined like the following in your spec:

ArrayAlias:
  type: array
  items:
    type: string

If you set the flag to false, nothing will get generated by the generator, and wherever this type is referenced in your spec, you'll just use a regular array of strings in the code.
If you set the flag to true, a class named ArrayAlias will get generated for the model, and the code expects this type instead of just an array of strings wherever the type is used.

In Ruby currently, when true, it creates a class in array_alias.rb that inherits Array (after the fix in #7419):

class ArrayAlias < Array
...
end

This class would be used as the type wherever it is ref'd in the spec. For instance if ref'd in another schema, the self.open_api_types would look like so:

def self.openapi_types
      {
        :'refd_array_alias' => :'ArrayAlias'
      }
    end

When false, it doesn't generate a new class, and the self.open_api_types method would just reference a Array<String>

def self.openapi_types
      {
        :'refd_array_alias' => :'Array<String>'
      }
    end

Does that make things clearer regarding what this feature does ?

[wing328]

Well IMO it's either we fix it completely, or we completely remove the feature for this generator.

The feature (generateAliasAsModel) was introduced to make it backward compatible when we wanted to remove such behaviour. It was primarily used by the Java generators based on request from users to keep such behaviour. There are discussions suggesting "extending a model with array or map type" is an anti-pattern in Java but regardless we introduced the option to keep some users happy.

Basically, when true, the generator would generate a model for the primitive types

Only array, map will be generated as models when generateAliasAsModel is set to true. String, integer and other primitive types won't be generated as models.

class ArrayAlias < Array

I'm not a Ruby expert but this looks like an anti-pattern in Ruby to me.

Is it correct to say the goal of this PR is to make primitive types work in oneOf in the Ruby client?

[zippolyte]

The main goal of this PR was to make the feature work, because we already use in our spec for another language (go). Our problem is that since we use it in go (on a per-model basis thanks to #6937), it ends up being used in ruby too, which was the reason for me fixing it.
It was used in go originally because it seemed very hard to make oneOf generation work for arrays and maps without it in go.

Now if primitives work for oneOf in ruby, I'm happy to close this, and make the ruby client generator ignore this feature flag.
Given how broken it is currently, generally unwanted it seems to be, and I couldn't find issues on it, I'm thinking it's probably not used, so we don't need to keep it.

[wing328]

First of all, thanks again for the PR. We truly appreciate your contributions. As @jfeltesse-mdsol has pointed out, #5706 will support primitive type as well so I suggest we close this one for the time being.

[zippolyte]

Done !
What about making the ruby client generator ignore this feature flag ?

[wing328]

What about making the ruby client generator ignore this feature flag ?

Good idea. I'll try to come up with a PR this weekend.

[jfeltesse-mdsol]

@zippolyte Thanks for the detailed explanation, it's much clearer now. As @wing328 pointed out, if you need a class to have those behaviors you wouldn't inherit but rather you'd include the Enumerable module, just like the Array and Hash classes do. Since overall the feature looks like it was a stopgap one I think we can move on now.