[ruby] fix oneOf handling

[jfeltesse-mdsol]

Fixes#3630.

I have yet to update the documentation related templates and update the samples but opening now to gather initial feedback on the direction. 👌

Basically, this builds up on the work from @bkabrda on the java and go experimental clients.
I didn't go "ruby-experiemental" because the support of oneOf in the current ruby client is broken anyway.not doing this in the end.

How it works is, in the spirit of the "oneof as interfaces" this creates a module, not a class, for oneOf models and provides a set of class methods related to the functionality.
The core method is build_from_hash whose job in this case is to find the relevant oneOf item and transparently building and returning the correct model.
It does so by using the discriminator if present, mapping if present and "falls back" to the existing validation-based look up.

I changed some test yaml files to include more oneOf examples.no need in the end.

If I try to do a before/after using the modified composed-one.yaml file, it goes like (some parts trimmed for brevity):

openapi-generator generate -i composed-oneof.yaml -g ruby --library faraday -o ./out/ruby_oneof

model files

# current master
custom_one_of_schema.rb
obj_a.rb
obj_b.rb
obj_c.rb
obj_d.rb
# this branch
custom_one_of_array_schema_one_of.rb
custom_one_of_schema.rb
obj_a.rb
obj_b.rb
obj_c.rb
obj_c_data_one_of.rb
obj_d.rb

custom_one_of_schema.rb

# current mastermoduleOpenapiClientclassCustomOneOfSchemaattr_accessor:realtypeattr_accessor:messageattr_accessor:descriptionattr_accessor:code# Attribute mapping from ruby-style variable name to JSON key.defself.attribute_map{:'realtype'=>:'realtype',:'message'=>:'message',:'description'=>:'description',:'code'=>:'code'}end# Attribute type mapping.defself.openapi_types{:'realtype'=>:'String',:'message'=>:'String',:'description'=>:'String',:'code'=>:'Integer'}end# List of attributes with nullable: truedefself.openapi_nullableSet.new([])end# List of class defined in oneOf (OpenAPI v3)defself.openapi_one_of[:'ObjA',:'ObjB']end# discriminator's property name in OpenAPI v3defself.openapi_discriminator_name:'realtype'end# Initializes the object# @param [Hash] attributes Model attributes in the form of hashdefinitialize(attributes={})# another 200 lines of model code...

# this branchmoduleOpenapiClientmoduleCustomOneOfSchema# List of class defined in oneOf (OpenAPI v3)defself.openapi_one_of[:'ObjA',:'ObjB']end# discriminator's property name in OpenAPI v3defself.openapi_discriminator_name:'realtype'end# discriminator's mapping in OpenAPI v3defself.openapi_discriminator_mapping{:'a-type'=>:'ObjA',:'b-type'=>:'ObjB'}end# Builds the object from hash# @param [Hash] attributes Model attributes in the form of hash# @return [Object] Returns the model itselfdefself.build_from_hash(attributes)discriminator_value=attributes[openapi_discriminator_name]returnnilunlessdiscriminator_value_class=openapi_discriminator_mapping[discriminator_value.to_sym]returnnilunless_classOpenapiClient.const_get(_class).build_from_hash(attributes)endendend

Noteworthy is in the obj_c.rb the nested oneOf property has its model generated, albeit with a sligthly different name:

# current masterdefself.openapi_types{:'realtype'=>:'String',:'data'=>:'OneOfObjAObjB'# <==== model not generated!}end# this branchdefself.openapi_types{:'realtype'=>:'String',:'data'=>:'ObjCDataOneOf'# <==== "interface" module generated}end

Limitations / bugs / food for thought

• Interestingly, a custom_one_of_array_schema_one_of.rb "interface" module is now generated but the oneOf is taking place inside the items. I'm not sure this solution is going to work here. In this case maybe a regular model should be generated alongside some custom_one_of_array_schema_one_of_items.rb "interface" module?
  @bkabrda how do the java and experimental go clients fare in this case?
• In java the interface pattern used in this PR makes the models referenced in a oneOf carry the "dynamic" props illustrated in ObjD. [core] Provide User feedback on confusing use of oneOf #4695 sort of discourages that pattern but I guess if the tool can do it, why not. What could be improved in the case of ruby though is to make some "dynamicProps" module in the interface module and dynamically include the props at runtime upon deserializing. Performance may not be stellar so it's trade off between "pureness" of the models and speed I guess.

PR checklist

• Read the contribution guidelines.
• If contributing template-only or documentation-only changes which will change sample output, build the project before.
• Run the shell script(s) under ./bin/ (or Windows batch scripts under.\bin\windows) to update Petstore samples related to your fix. This is important, as CI jobs will verify all generator outputs of your HEAD commit, and these must match the expectations made by your contribution. You only need to run ./bin/{LANG}-petstore.sh, ./bin/openapi3/{LANG}-petstore.sh if updating the code or mustache templates for a language ({LANG}) (e.g. php, ruby, python, etc).
• File the PR against the correct branch: master, 4.3.x, 5.0.x. Default: master.
• Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

@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.

[gommo]

Does this PR address the below part in the original bug?

Another issue related to oneOf for the Ruby client is the generation of objects within a response schema. If you have the following OpenAPI spec, the class type gets generated as ExampleClient::OneOfsimpleObjectcomplexObject, and that class does not exist.

[jfeltesse-mdsol]

@gommo ah right, that's not addressed. Even the java generator with improved oneOf handling fails in this case. In both languages it refers to some "EndpointGET200" model that doesn't exist.

@bkabrda is this a known issue of the current implementation or should I open a new issue specifically for this?

[wing328]

is this a known issue of the current implementation or should I open a new issue specifically for this?

Better open an issue with details (e.g. mininal spec) to track the issue.

[jfeltesse-mdsol]

@wing328 I just opened #5903

[wing328]

@jfeltesse-mdsol thanks for the PR. When you've time, can you please PM me via Slack to discuss this PR?

https://join.slack.com/t/openapi-generator/shared_invite/enQtNzAyNDMyOTU0OTE1LTY5ZDBiNDI5NzI5ZjQ1Y2E5OWVjMjZkYzY1ZGM2MWQ4YWFjMzcyNDY5MGI4NjQxNDBiMTlmZTc5NjY2ZTQ5MGM

[jfeltesse-mdsol]

any chance to get this reviewed/merged?

[wing328]

Tested with a spec with an oneOf schema containing null:

 def self.openapi_one_of
[
:'Cat',
:'DogDot',
:'Null'
]
end

Something that we need to address later (not this PR) to properly handle nullable oneOf schemas.

[wing328]

I did a test with primary types in oneOf schemas and looks like your current approach assumes the schemas defined in oneOf are models (with properties). Something we need to handle/address in future PRs.

[wing328]

If discriminator.propertyName is set to petType, I got the following code in the oneOf model:

 # discriminator's property name in OpenAPI v3
def self.openapi_discriminator_name
:'pet_type'
end

I don't think this will work as the pet_type won't match the attribute/field petType in the JSON payload. Can you please take a look when you've time?

Example definition:

 One.Of.Response:
discriminator:
propertyName: petType
oneOf:
- $ref: '#/components/schemas/dog.dot'
- $ref: '#/components/schemas/Cat'

[jfeltesse-mdsol]

I did a test with primary types in oneOf schemas and looks like your current approach assumes the schemas defined in oneOf are models (with properties). Something we need to handle/address in future PRs.

Are you referring to the case where the oneOf members are other types like array or scalars? How are the other languages handling this?

[wing328]

Example:

 One.Of.Primitive.Types:
oneOf:
- type: string
- type: integer

Java (jersey2), Go, PowerShell generator should be able to handle this without issues.

[wing328]

If I understand correctly, your current approach relies on discriminator (which is optional) so we will need to come up with the implementation to support oneOf without the discriminator.

[jfeltesse-mdsol]

If I understand correctly, your current approach relies on discriminator (which is optional) so we will need to come up with the implementation to support oneOf without the discriminator.

Actually it does work without a discriminator, the build_from_hash logic goes in this order:

• discriminator + mapped model
• discriminator alone (e.g. discriminator value is a model name)
• if no discriminator, for each oneOf item try to validate the model and return the first one that validates.

[wing328]

That's good to know.

if no discriminator, for each oneOf item try to validate the model and return the first one that validates.

Should it go through the rest to ensure there's only one match (to fulfill the definitions of oneOf)?

[wing328]

For the spec used by the Ruby petstore samples listed below, can you please switch it to modules/openapi-generator/src/test/resources/3_0/java/petstore-with-fake-endpoints-models-for-testing-with-http-signature.yaml instead which covers many more test cases for oneOf, anyOf models?

• https://github.com/OpenAPITools/openapi-generator/blob/master/bin/configs/ruby.yaml#L4
• https://github.com/OpenAPITools/openapi-generator/blob/master/bin/configs/ruby-faraday.yaml#L4

[jfeltesse-mdsol]

Should it go through the rest to ensure there's only one match (to fulfill the definitions of oneOf)?

I don't think so, the logic I described is used when deserizalizing the response and doing validation at that stage brings no benefit. On the request side you wouldn't use the module directly but rather pass the relevant data directly. There, alongside the existing client side validations, checking the oneOf promise may make sense but it should be done in a separate PR to keep things bite-sized.

[wing328]

I don't think so, the logic I described is used when deserizalizing the response

So anyOf and oneOf are essentially the same with respect to the logic you described?

[jfeltesse-mdsol]

I don't think so, the logic I described is used when deserizalizing the response

So anyOf and oneOf are essentially the same with respect to the logic you described?

Correct.

As a user though, I have a hard time picturing how validation when receiving can be useful. Imagine I'm using an autogenerated client to access an external API I have no control over. What good does it do to me that even though I received some data, the client throws an error (and prevents me from accessing the data?) because the API's openapi spec file incorrectly advertises oneOf when it really should be anyOf?
As long as the data I receive can be deserialized properly, I don't really care whether it happens to also match another schema. It's a bit arbitrary because you might expect the 2nd item in the oneOf list and get the 1st one (because it checks in order) but then the clients are not AI-powered, there's a minimum expectation that the openapi spec matches the server side implementation, isn't there?

Having said that though, it's not a huge challenge to validate that part. How the client should behave when encountering such cases though? Throw an error? Return null? Return the first valid item and log a warning? some other way??

[wing328]

What good does it do to me that even though I received some data, the client throws an error (and prevents me from accessing the data?)

The user can then report the issue to the API owner to look into the error as the response (payload) doesn't conform to the contract/spec (oneOf in this case). If multiple matches are expected, then anyOf should be used instead.

there's a minimum expectation that the openapi spec matches the server side implementation

Sorry that it's not something I can agree with. Both client and server should conform to the contract (openapi spec).

How the client should behave when encountering such cases though?

In Java, Go, PowerShell implementation, the client throws an exception or returns an error.

[zippolyte]

This should be wrapped in a try/rescue. build_from_hash for enum values can raise. And when that's the case, the loop should continue to try the next model.

[jfeltesse-mdsol]

build_from_hash for enum values can raise

can you provide an example?

[zippolyte]

It's in here https://github.com/OpenAPITools/openapi-generator/blob/a0a7464786eb727d1ca4b483ad711bc79bc2bdf6/modules/openapi-generator/src/main/resources/ruby-client/partial_model_enum_class.mustache#L17

[zippolyte]

Oh one more thing I noticed, as I'm trying out this PR on our spec: the oneOf types can be primitive types like Float, String, ... so we can't call build_from_hash on those. Maybe you could add some special cases, comparing the openapi_one_of value to primitive types, and try to directly parse the value in that case ?

[jfeltesse-mdsol]

Yes, this has been pointed out and it is something I plan to address #5706 (comment)

[zippolyte]

Ah right, sorry for the noise 🙈

[jfeltesse-mdsol]

heads up here, I've not abandoned this PR, it's just that I got super busy at work. I might be able to go back to this in a couple of weeks.

[wing328]

@jfeltesse-mdsol please take your time. We appreciate your contributions to this.

[jfeltesse-mdsol]

@wing328 About #5706 (comment), I rebased my branch with latest master, re-built the generator and updated the inputSpec with

--- a/bin/configs/ruby-faraday.yaml+++ b/bin/configs/ruby-faraday.yaml@@ -1,7 +1,7 @@
generatorName: ruby
outputDir: samples/client/petstore/ruby-faraday
library: faraday
-inputSpec: modules/openapi-generator/src/test/resources/3_0/petstore-with-fake-endpoints-models-for-testing.yaml+inputSpec: modules/openapi-generator/src/test/resources/3_0/java/petstore-with-fake-endpoints-models-for-testing-with-http-signature.yaml
templateDir: modules/openapi-generator/src/main/resources/ruby-client
additionalProperties:
gemVersion: 1.0.0

But then running ./bin/generate-samples.sh bin/configs/ruby-faraday.yaml fails with:

[main] WARN o.o.c.languages.RubyClientCodegen - 200_response (model name starts with number) cannot be used as model name. Renamed to Model200Response
[main] WARN o.o.c.languages.RubyClientCodegen - Return (reserved word) cannot be used as model name. Renamed to ModelReturn
[main] WARN o.o.c.languages.RubyClientCodegen - 200_response (model name starts with number) cannot be used as model name. Renamed to Model200Response
[main] WARN o.o.c.languages.RubyClientCodegen - Return (reserved word) cannot be used as model name. Renamed to ModelReturn
[main] WARN o.o.c.languages.RubyClientCodegen - 200_response (model name starts with number) cannot be used as model name. Renamed to Model200Response
[main] INFO o.o.codegen.DefaultGenerator - Model inline_object_3 (marked as unused due to form parameters) is generated due to the system property skipFormModel=false (default)
Exception in thread "main" java.lang.RuntimeException: Could not process model 'inline_object_3'.Please make sure that your schema is correct!
at org.openapitools.codegen.DefaultGenerator.generateModels(DefaultGenerator.java:499)
at org.openapitools.codegen.DefaultGenerator.generate(DefaultGenerator.java:879)
at org.openapitools.codegen.cmd.Generate.execute(Generate.java:432)
at org.openapitools.codegen.cmd.OpenApiGeneratorCommand.run(OpenApiGeneratorCommand.java:32)
at org.openapitools.codegen.OpenAPIGenerator.main(OpenAPIGenerator.java:66)
Caused by: java.lang.ClassCastException: class java.time.OffsetDateTime cannot be cast to class java.lang.String (java.time.OffsetDateTime and java.lang.String are in module java.base of loader 'bootstrap')
at org.openapitools.codegen.languages.AbstractRubyCodegen.toDefaultValue(AbstractRubyCodegen.java:150)
at org.openapitools.codegen.DefaultCodegen.fromProperty(DefaultCodegen.java:3083)
at org.openapitools.codegen.DefaultCodegen.addVars(DefaultCodegen.java:4853)
at org.openapitools.codegen.DefaultCodegen.addVars(DefaultCodegen.java:4802)
at org.openapitools.codegen.DefaultCodegen.fromModel(DefaultCodegen.java:2550)
at org.openapitools.codegen.DefaultGenerator.processModels(DefaultGenerator.java:1259)
at org.openapitools.codegen.DefaultGenerator.generateModels(DefaultGenerator.java:494)

Any clue? The input file doesn't have any inline_object_3.

[jfeltesse-mdsol]

@wing328 About the logic and ensuring the oneOf promise is fullfilled, how about verifying it only when a discriminator is not set? This is to conform to the openapi spec's explanation about discriminator:

a discriminator MAY act as a "hint" to shortcut validation and selection of the matching schema which may be a costly operation, depending on the complexity of the schema. We can then describe exactly which field tells us which schema to use:

https://github.com/OAI/OpenAPI-Specification/blob/master/versions/3.0.3.md#discriminator-object

IMHO, if we always validate all the schemas then it makes the discriminator property pointless and undermines the work of the API spec author.

[jfeltesse-mdsol]

@wing328 So I had a look at validating the models that may be defined in the oneOf array so that when looping through those we can double check the oneOf promise. The take away is this is not as simple as it sounds and it should be done in a different PR.

Basically, a bare bones implementation may look like

{{^discriminator}}valid_models=[]openapi_one_of.eachdo |_class|
model={{moduleName}}.const_get(_class).build_from_hash(attributes)valid_models << modelifmodel.valid?endreturnvalid_models[0]ifvalid_models.size == 1raise"{{classname}} is using oneOf but more than 1 schema validated #{attributes} => #{valid_models.inspect}"{{/discriminator}}

Looks good right? Except the valid? method does nothing most of the time...
The current code's validation only looks at required fields, min/max lengths, nullable, enums, patterns... but that's about it and if none of those are declared in the schema, it only ever returns true. Just with that we can't identify the models that may be breaking the oneOf promise, for that we'd need to actually do a proper JSON schema validation. A concrete example, with the code above, in the following spec all 3 schemas are considered valid if I pass say { "realtype" => "a", "code" => "b" } to build_from_hash:

ObjA:type: objectproperties:realtype:type: stringmessage:type: stringObjB:type: objectproperties:realtype:type: stringcode:type: integerObjC:type: objectproperties:realtype:type: stringcode:type: stringObjAOrObjB:oneOf:- $ref: '#/components/schemas/ObjA'- $ref: '#/components/schemas/ObjB'- $ref: '#/components/schemas/ObjC'

In this case, that's because the valid? method doesn't check the types or the property names.

So yeah, not opening that pandora's box just now.

I'll try to complete the PR with support for scalar values though, stay tuned!

[jfeltesse-mdsol]

Unless the CI reveals bugs, I think this is feature complete. The most interesting bits are in d1c599d (#5706).

Now oneOf supports scalars, arrays, hashes and the combinations you may think of. Maybe not perfect but should support most of the cases. And yeah, it doesn't try to check the oneOf promise. I got it to actually check whether the properties passed existed (see the changes in base_object, before it'd build_from_hash without passing the data to the constructor, bypassing the property name checks...) but the values in base_object are cast without checking the type. If someone's pumped about fixing this, it should be the topic of a separate PR. I can't fix all the issues in one PR 😅

[jfeltesse-mdsol]

@wing328 I tried to update the default ruby version that's incorrect here d1c599d (#5706) (by default the generator would use >= 2.4) but the ensure-up-to-date script reverts that change. How to update those docs?

[wing328]

Travis CI reported the following errors:

An error occurred while loading spec_helper.
Failure/Error:
class ChildCat < ParentPet
attr_accessor :name
attr_accessor :pet_type
class EnumAttributeValidator
attr_reader :datatype
attr_reader :allowable_values
def initialize(datatype, allowable_values)
NameError:
uninitialized constant Petstore::ParentPet
# ./lib/petstore/models/child_cat.rb:17:in `<module:Petstore>'
# ./lib/petstore/models/child_cat.rb:16:in `<top (required)>'
# ./lib/petstore.rb:35:in `require'
# ./lib/petstore.rb:35:in `<top (required)>'
# ./spec/spec_helper.rb:14:in `require'
# ./spec/spec_helper.rb:14:in `<top (required)>'
No examples found.
Finished in 0.00003 seconds (files took 0.1754 seconds to load)
0 examples, 0 failures, 1 error occurred outside of examples

Ref: https://travis-ci.org/github/OpenAPITools/openapi-generator/builds/739218479

Please take a look when you've time.

[wing328]

but the ensure-up-to-date script reverts that change. How to update those docs?

CircleCI, which runs the ensure-up-to-date script, didn't report any failure so I assume it's no longer an issue, right?

[jfeltesse-mdsol]

@wing328 I'll have a look at the Travis failure tomorrow.

As for the Circle CI one, it's still a problem, I reverted the change so it passes, that's it. The doc here is still going to advertise the default ruby version for generated clients as ">= 1.9" when in fact it's ">= 2.4". How to update the doc and make the CI not change it back?

[wing328]

How to update the doc and make the CI not change it back?

One way is to run ./bin/utils/ensure-up-to-date locally and commit the changes (in the doc).

[jfeltesse-mdsol]

One way is to run ./bin/utils/ensure-up-to-date locally and commit the changes (in the doc).

Not sure if I explain this correctly but locally the script will always revert to the previous value.

I commit this:

commit ab351a70fb (HEAD -> ruby/fix_oneof)
Author: Julien Feltesse <jfeltesse@mdsol.com>
Date: Wed Oct 28 15:34:55 2020 +0900
use ruby 2.4 by default
diff --git a/docs/generators/ruby.md b/docs/generators/ruby.md
index 4c1cbca4e2..24b340eeec 100644
--- a/docs/generators/ruby.md+++ b/docs/generators/ruby.md@@ -16,7 +16,7 @@
|gemHomepage|gem homepage. | |http://org.openapitools|
|gemLicense|gem license. | |unlicense|
|gemName|gem name (convention: underscore_case).| |openapi_client|
-|gemRequiredRubyVersion|gem required Ruby version. | |&gt;= 1.9|+|gemRequiredRubyVersion|gem required Ruby version. | |&gt;= 2.4|
|gemSummary|gem summary. | |A ruby wrapper for the REST APIs|
|gemVersion|gem version.| |1.0.0|
|hideGenerationTimestamp|Hides the generation timestamp when files are generated.| |true|

Run the script:

./bin/utils/ensure-up-to-date

And I get that diff:

diff --git a/docs/generators/ruby.md b/docs/generators/ruby.md
index 24b340eeec..4c1cbca4e2 100644
--- a/docs/generators/ruby.md+++ b/docs/generators/ruby.md@@ -16,7 +16,7 @@
|gemHomepage|gem homepage. | |http://org.openapitools|
|gemLicense|gem license. | |unlicense|
|gemName|gem name (convention: underscore_case).| |openapi_client|
-|gemRequiredRubyVersion|gem required Ruby version. | |>= 2.4|+|gemRequiredRubyVersion|gem required Ruby version. | |>= 1.9|
|gemSummary|gem summary. | |A ruby wrapper for the REST APIs|
|gemVersion|gem version.| |1.0.0|
|hideGenerationTimestamp|Hides the generation timestamp when files are generated.| |true|

Where the heck the 1.9 value comes from I do not know. I even tried to modify the value in modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/RubyClientCodegen.java, rebuild and re-run: same result. Giving up because I can't spend hours fighting that script.

[wing328]

Where the heck the 1.9 value comes from I do not know. I even tried to modify the value in modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/RubyClientCodegen.java, rebuild and re-run: same result. Giving up because I can't spend hours fighting that script.

The doc is auto-generated. We'll take care of it in a separate PR instead.

[jfeltesse-mdsol]

As mentioned in the last commit, I reverted to the old configs because the new ones would run into the issue described at #4690
We actually have a fix for this one and we'll submit a PR soon.

[jfeltesse-mdsol]

CI passes but please do not merge yet, thought more about my last fix and I think it's not going to work in all cases. Will post an update, probably tomorrow. 🙆‍♂️

[jfeltesse-mdsol]

fails on the .net things which I'm pretty sure aren't related to the PR here... 😑

[jfeltesse-mdsol]

@wing328 This PR is ready to be merged but some CIs are failing randomly. I tried to force-push a couple of times but it's one CI failing once, another CI failing afterwards, and so on. On unrelated stuff.

[zippolyte]

hey @wing328, any chance this can get merged in soon ? We're excited to start using this feature for generating our ruby client.

[wing328]

We were busy with the v5.0.0-beta3 release: https://twitter.com/oas_generator/status/1329828788264321025. Let me try to review today or tomorrow.

Is it correct to say that you've tested the change locally and it works in your use cases?

[zippolyte]

Yeah it works for the few use cases I tried

[wing328]

@zippolyte thanks for testing it. I did a quick look and the PR looks good overall. I will merge this instead of being the bottleneck to get this PR merged. Will open an issue or file a PR if I find anything later.

[zippolyte]

Just found a minor bug in the generated tests. They fail for oneOf containing primitive types because of the generated following code

it'lists the models referenced in the oneOf array'doexpect(described_class.openapi_one_of).to_notbe_emptydescribed_class.openapi_one_of.each{ |klass| expect{DatadogAPIClient::V2.const_get(klass)}.to_notraise_error}end

Primitive types are not constants of the API client module.
I propose to just remove this test, WDYT ?
Or maybe not change anything, since test are meant to be manually edited anyway.

[jfeltesse-mdsol]

ah, probably an oversight of mine when I refactored the code to accept scalar types and not just schemas. I think removing the 2nd line is ok since we wouldn't be able to make advanced tests in the generated code (lack of context). "Real" oneOf specs should be added in the samples but I didn't do that since the config need to be updated first.