OpenAPITools / OpenAPITools/openapi-generator
[BUG] [Ruby] Model property validation can be improved
Nobody has claimed this yet.
- Dominant language
- Java
- Stars
- 26.8k
- Forks
- 7.7k
- PR merge metrics
- PR metrics pending
Description
Bug Report Checklist
- Have you provided a full/minimal spec to reproduce the issue?
- Have you validated the input using an OpenAPI validator (example)?
- Have you tested with the latest master to confirm the issue still exists?
- Have you searched for related issues/PRs?
- What's the actual output vs expected output?
- [Optional] Sponsorship to speed up the bug fix or feature request (example)
Description
In the ruby-client generator, schema properties that are required or have other constraints are generated with custom attribute assignment functions that validate said properties. The way this is currently implemented can lead to NoMethodErrors inside the list_invalid_properties method. If a property is required and has another validation (e.g. maximum), but that attribute is not passed in to the initialize function, the if attributes.key?(:'{{{name}}}') block does not assign the attribute, which skips the validation for that attribute. If the user then calls the list_invalid_properties method, 'invalid value for "{{{name}}}", {{{name}}} cannot be nil.') is pushed to the invalid_properties Array, but the function does not return. A property that also has a maximum will then attempt to run @{{{name}}} >{{#exclusiveMaximum}}={{/exclusiveMaximum}} {{{maximum}}} due to the fact that all of the {{required}} checks in this file (e.g line 168) are prefixed with ^. This leads to a NoMethodError as the NilClass does not support this method.
Having the function return early or always prefixing the checks with the !nil? block would fix this issue, but I don't see a reason for the list_invalid_properties or valid? functions to exist for these models at all. It seems the design was supposed to validate all attributes upon initialization, via the build_from_hash method, and via direct assignment (=). If this is that case, is an invalid model ever supposed to exist? Currently the only way I can get a model to have an invalid attribute is by leaving an attribute nil by never declaring that attribute. Only in that case will the attribute be invalid, but this can be solved by adding an else to assign the attribute to nil if it does not have a default value in the if statement surrounding the attribute assignment in the initialize method. This should cause any validation errors to be caught in the assignment validation.
I may be misunderstanding the function of the list_invalid_properties and valid? methods, so if I am, could I get some clarity here?
openapi-generator version
6.6.0
OpenAPI declaration file content or url
Generation Details
our openapi-config.yml
gemAuthor: Bandwidth
gemAuthorEmail: email@email.com
gemDescription: The official client SDK for Bandwidth's Voice, Messaging, MFA, and WebRTC APIs
gemHomepage: https://github.com/Bandwidth/ruby-sdk
gemLicense: MIT
gemName: bandwidth-sdk
gemRequiredRubyVersion: '>=2.7'
gemSummary: Bandwidth Ruby SDK
gemVersion: 11.0.0
library: faraday
moduleName: Bandwidth
Steps to reproduce
Generate client using openapi-generator-cli generate -g ruby -i bandwidth.yml -c openapi-config.yml -o ./
Then create a new Bandwidth::VerifyCodeRequest with no arguments and call its list_invalid_properties method.
verify_code_request_invalid = Bandwidth::VerifyCodeRequest.new
verify_code_request_invalid.list_invalid_properties
This causes the NoMethodError
Failure/Error: if @expiration_time_in_minutes > 15
NoMethodError:
undefined method `>' for nil:NilClass
if @expiration_time_in_minutes > 15
Related issues/PRs
N/A
Suggest a fix
The if statements in the list_invalid_properties method should be always prefixed with the !nil? check to fix the NoMethodError. If the attribute validation is not working as intended as I've described above, then the else self.name = nil should be added to the if statements around the attribute assignments in the initialize method. A final else can be added to the build_from_hash method to mimic the new behavior of the initialize method, and the list_invalid_properties and valid? methods can likely be removed. This would be a breaking change to the SDK and may be unnecessary, so please let me know if this work is worth doing and I'll gladly open a PR to do some or all of the work mentioned.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start with modules/openapi-generator/src/main/resources/ruby-client/partial_model_generic.mustache, especially the list_invalid_properties, initialize, and build_from_hash sections. Generate the Ruby client using the documented openapi-generator-cli command and reproduce the VerifyCodeRequest example. Done means the generated model no longer raises the reported NoMethodError and its validation behavior is covered or clearly resolved.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- openapi, ruby
- Domain
- api, tooling
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100