Skip to content

Conversation

@ckoegel
Copy link
Contributor

@ckoegel ckoegel commented Jul 17, 2023

Addresses #15895 This PR aims to improve the model initialization and attribute validation functionalities in the Ruby generator. This PR modifies the initialize and build_from_hash methods, which currently have slightly different behaviors, to behave the same when creating model instances. It also fixes a bug in the attribute validation process, no longer allowing for invalid models to be created. This removes the need for the list_invalid_properties and valid? methods, which have been marked as deprecated.

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.
  • Run the following to build the project and update samples:
    ./mvnw clean package 
    ./bin/generate-samples.sh
    ./bin/utils/export_docs_generators.sh
    
    Commit all changed files.
    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.
  • In case you are adding a new generator, run the following additional script :
    ./bin/utils/ensure-up-to-date
    
    Commit all changed files.
  • File the PR against the correct branch: master (6.3.0) (minor release - breaking changes with fallbacks), 7.0.x (breaking changes without fallbacks)
  • If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

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

# Check to see if the all the properties in the model are valid
# @return true if the model is valid
def valid?
warn '[DEPRECATED] the `valid?` method is obsolete'
Copy link
Member

@wing328 wing328 Jul 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the oneOf, anyOf implementation is still using this method so we need to update the implementation accordingly to replace valid? with something else.

ref: https://github.com/OpenAPITools/openapi-generator/blob/master/modules/openapi-generator/src/main/resources/ruby-client/partial_oneof_module.mustache#L126

Copy link
Contributor Author

@ckoegel ckoegel Jul 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the reference to valid? on line 126 can be removed. Since the model is created from the build_from_hash method, which now fully supports attribute validation, any model created from that method should be guaranteed to be valid. Let me know if this line can be updated to return model if model.

Edit: The valid? method contains enum attribute validation, which the custom attribute writers currently do not. Adding enum attribute validation to these writers will make the the new, build_from_hash, and assignment operators fully support attribute validation, making the valid? method obsolete in this case. Let me know if this is the solution we'd like to move forward with, in which case, I'll add enum validation to the attribute writers.
If this is not the solution we'd like to move forward with, it may make sense to leave the valid? method as is (un-deprecated) and continue to use it in the oneOf/anyOf models

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in which case, I'll add enum validation to the attribute writers.

Yes, I think we need enum validation as well in attribute writers.

I've filed #16137 to add some tests for oneOf. Will merge your change into my PR to see if there's any change in oneOf model behaviour.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried removing the valid? check in oneOf template and the tests also passed so I think we are in good shape to mark valid? as deprecated

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI. #16137 has been merged.

We can remove valid? later in oneOf template

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think we need enum validation as well in attribute writers.

Please submit another PR for this as I'll merge this for the time being since I've some other enhancements for Ruby models as well.

Thanks again for the PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UPDATE: filed #16147 to remove invalid? in oneOf template and add anyOf support.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants