Ruby client nullable

[slemrmartin]

This PR fixes Ruby clients on columns with "nullable": true. Attribute's key with nil value isn't skipped, but sent to server with nil value.

Issue: #4363 (comment)

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.

[slemrmartin]

@cliffano @zlx @autopp please review

[slemrmartin]

cc @wing328

[autopp]

@slemrmartin Thanks for PR, I'll review it.

[autopp]

@slemrmartin This PR seems to solve original issue. However, the reverse problem appears to occur.
Even if nullable attribute is omitted, null is always sent to server.

I think we can solve both problems by using instance_variable_defined? instead of openapi_nullable.
How about that?

[slemrmartin]

@autopp If nullable attribute is omitted, then !self.class.openapi_nullable.include?(attr) condition is true, isn't it? Which means that null isn't sent to server. Or which case I'm missing?

[autopp]

@slemrmartin Sorry for the incomprehension.
Let me illustrate my concern with the following model example.

Foo:
  type: object
  properties:
    x:
      type: integer
      nullable: true

On current master:

p OpenapiClient::Foo.new({ x: 1 }).to_hash   #=> {:x=>1}
p OpenapiClient::Foo.new({ x: nil }).to_hash #=> {} <- always empty
p OpenapiClient::Foo.new({}).to_hash         #=> {}

And on this branch:

p OpenapiClient::Foo.new({ x: 1 }).to_hash   #=> {:x=>1}
p OpenapiClient::Foo.new({ x: nil }).to_hash #=> {:x=>nil}
p OpenapiClient::Foo.new({}).to_hash         #=> {:x=>nil} <- always nil

For these, I think the following are expected:

p OpenapiClient::Foo.new({ x: 1 }).to_hash   #=> {:x=>1}
p OpenapiClient::Foo.new({ x: nil }).to_hash #=> {:x=>nil}
p OpenapiClient::Foo.new({}).to_hash         #=> {}

Please let me know if I misunderstand.

[slemrmartin]

@autopp now I understand, thanks for clarification.
Then it should be combination of openapi_nullable and instance_variable_defined? I think, because if nullable is not defined or false in definition, then variable x set to nil shouldn't be allowed

    x:
      type: integer
      nullable: true

p OpenapiClient::Foo.new({ x: nil }) #=> {:x=>nil}
p OpenapiClient::Foo.new({ }) #=> {}

, but

    x:
      type: integer
      nullable: false

p OpenapiClient::Foo.new({ x: nil }) #=> {}
p OpenapiClient::Foo.new({ }) #=> {}

I don't know openapi specification to the depth so correct me if I'm wrong

[autopp]

indent seems to be too depth.

[wing328]

And what about defining it as a hash instead similar to self.openapi_types?

If an object contains a lot of nullable properties, using include? may result in performance issues.

[slemrmartin]

I don't see usage for key-value, but what about using Set instead of Array, it should have better performance for include?

[slemrmartin]

I don't like the format

def self.openapi_nullable
      Set.new([
      ])
end

is it possible to define it in template in a way that empty set won't generate new line?

[autopp]

Hmm... it seems to be difficult because there is no way to directly enumerate nullable attributes in template.
@wing328 Do you have any ideas?

By the way, I don't like this format, but it's acceptable (Rubocop also allows this), so I think this PR can be merged.
Isn't it okay to make another PR for reformatting?

[wing328]

Set is definitely better than array in this case.

About empty set, I think it's acceptable for day 1 requirement. We can always improve it later with another PR.

[autopp]

@slemrmartin your suggestion sounds good 👍
Can you fix it with a comment about the indentation?

[wing328]

@slemrmartin thanks for the PR, which has been included in the v4.2.1 release: https://twitter.com/oas_generator/status/1195339336922759168

[slemrmartin]

@wing328 thanks for your help