Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] [Ruby] Initalize validation should apply to 'build_from_hash' too #5199

Open
NeilW opened this issue Feb 3, 2020 · 5 comments
Open

[BUG] [Ruby] Initalize validation should apply to 'build_from_hash' too #5199

NeilW opened this issue Feb 3, 2020 · 5 comments

Comments

@NeilW
Copy link

@NeilW NeilW commented Feb 3, 2020

Description

The changes to introduce better validation to the attributes hash in the initialize method, should also apply in the build_from_hash method.

openapi-generator version

$ docker run --rm openapitools/openapi-generator-cli version
4.2.3-SNAPSHOT

Steps to reproduce
  1. Generate any ruby API
  2. Build a hash with an incorrectly spelled attribute and create a model object with it.

E.g.

irb(main):012:0> Kubernetes::IoK8sApiBatchV1Job.new(api_version: 'batch/v1', kid: 'Job')
...
ArgumentError (`kid` is not a valid attribute in `Kubernetes::IoK8sApiBatchV1Job`. Please check the name to make sure it's valid. List of attributes: [:api_version, :kind, :metadata, :spec, :status])

vs

irb(main):013:0> Kubernetes::IoK8sApiBatchV1Job.build_from_hash(api_version: 'batch/v1', kid: 'Job')
=> #<Kubernetes::IoK8sApiBatchV1Job:0x000055ae59e2d238>
Related issues/PRs

#2226

Suggest a fix

Factor out the validation code from initialize and either call it directly from the 'build_from_hash' method, and/or add a 'strictValidation' parameter to allow backward compatibility.

@NeilW NeilW added the Issue: Bug label Feb 3, 2020
@NeilW

This comment has been minimized.

Copy link
Author

@NeilW NeilW commented Feb 3, 2020

Additionally the attribute names requirements are different between the methods

irb(main):022:0> Kubernetes::IoK8sApiBatchV1Job.build_from_hash(api_version: 'batch/v1').to_hash
=> {}
irb(main):023:0> Kubernetes::IoK8sApiBatchV1Job.new(api_version: 'batch/v1').to_hash
=> {:apiVersion=>"batch/v1"}
irb(main):024:0> Kubernetes::IoK8sApiBatchV1Job.build_from_hash(apiVersion: 'batch/v1').to_hash
=> {:apiVersion=>"batch/v1"}
irb(main):025:0> Kubernetes::IoK8sApiBatchV1Job.new(apiVersion: 'batch/v1').to_hash
...
ArgumentError (`apiVersion` is not a valid attribute in `Kubernetes::IoK8sApiBatchV1Job`. Please check the name to make sure it's valid. List of attributes: [:api_version, :kind, :metadata, :spec, :status])
@ackintosh

This comment has been minimized.

Copy link
Member

@ackintosh ackintosh commented Feb 10, 2020

cc: Technical Committee (Ruby)
@cliffano (2017/07) @zlx (2017/09) @autopp (2019/02)
(Please tell me if I'm saying something wrong... 🙏 )

I think it is intentional that build_from_hash doesn't throw errors.

  • build_from_hash is used when deserializing JSON payload into Ruby object
  • Let's say adding a property to the JSON payload
    • If build_from_hash throws errors, we have to upgrade clients before api-servers. It's fine if the api is ours, but it will be hard if the api is an external API
@NeilW

This comment has been minimized.

Copy link
Author

@NeilW NeilW commented Feb 10, 2020

That may be the case - also the round trip of XYZ::build_from_hash(my_object.to_hash) should be an identity.

But there is also a use case where you want to validate the hash - say from user input. Which is the same process but with errors thrown.

Which perhaps suggests another method build_strictly_from_hash, with both that method and build_from_hash curried onto a common refactor that can handle both cases?

@autopp

This comment has been minimized.

Copy link
Contributor

@autopp autopp commented Feb 11, 2020

I agree with @ackintosh's opinion. In particular, the following can be problematic.

  • If build_from_hash throws errors, we have to upgrade clients before api-servers. It's fine if the api is ours, but it will be hard if the api is an external API

But there is also a use case where you want to validate the hash - say from user input. Which is the same process but with errors thrown.

I understand this use case. By the way, new can receive Hash as well as build_from_hash. Is this insufficient? (Maybe the point indicated by #5199 (comment) is a problem?)

@NeilW

This comment has been minimized.

Copy link
Author

@NeilW NeilW commented Feb 11, 2020

New doesn't appear to recurse like build_from_hash does. So when you have heavily nested Typed APIs (hi Kubernetes), you don't get the result you want.

And as you say new takes Ruby style attributes rather than camelCase attributes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.