Skip to content

[python] Add option to skip client validations #4137

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

Merged
merged 7 commits into from Oct 16, 2019
Merged

[python] Add option to skip client validations #4137

merged 7 commits into from Oct 16, 2019

Conversation

ghost
Copy link

@ghost ghost commented Oct 13, 2019

PR checklist

  • Read the contribution guidelines.
  • Ran the shell script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh, ./bin/openapi3/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in .\bin\windows\. If contributing template-only or documentation-only changes which will change sample output, be sure to build the project first.
  • Filed the PR against the correct branch: master, 4.1.x, 5.0.x. Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

Adds option to skip client side validations

@auto-labeler
Copy link

auto-labeler bot commented Oct 13, 2019

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

@ghost
Copy link
Author

ghost commented Oct 13, 2019

@taxpon (2017/07) @frol (2017/07) @mbohlool (2017/07) @cbornet (2017/09) @kenjones-cisco (2017/11) @tomplus (2018/10) @Jyhess (2019/01)

cc: @wing328

@@ -215,10 +229,13 @@ class {{classname}}(object):
if not isinstance(other, {{classname}}):
return False

return self.__dict__ == other.__dict__
return self.to_dict() == other.to_dict()
Copy link
Member

Choose a reason for hiding this comment

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

What's the reason to use to_dict() instead of __dict__?

Copy link
Author

Choose a reason for hiding this comment

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

So far configuration was not part of the model, now configuration is. So if the user does not provide configuration. The configuration is initialized since two objects have different configuration self.dict == other.dict will return false even though its the same object.

Test("abc") will not be equal to Test("abc") since configuration objects inside both are different. This change is required to prevent this behaviour

Copy link
Member

Choose a reason for hiding this comment

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

@slash-arun thanks for the explanation. I wonder if you can add test cases to https://github.com/OpenAPITools/openapi-generator/tree/master/samples/client/petstore/python/tests to cover the issue moving forward.

Copy link
Author

Choose a reason for hiding this comment

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

Sure, will work on the test cases


def __ne__(self, other):
"""Returns true if both objects are not equal"""
return not self == other
Copy link
Member

Choose a reason for hiding this comment

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

Performance would not self == other be better?

Copy link
Author

Choose a reason for hiding this comment

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

This again relates to the configuration object. Since Test("abc") will not be equal to Test("abc") because config is initialized separately for both the objects.

Copy link
Member

Choose a reason for hiding this comment

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

@slash-arun thanks for the explanation. I wonder if you can add test cases to https://github.com/OpenAPITools/openapi-generator/tree/master/samples/client/petstore/python/tests to cover the issue moving forward.

Copy link
Author

Choose a reason for hiding this comment

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

Sure, will work on the test cases

@spacether
Copy link
Contributor

@slash-arun can you describe the use case here more in depth?
Why would one want to turn off client side validations?
Is this when receiving data from the server, instantiating an instance of the model on the client?
Why not change the spec to remove the validations?
Will the server accept these values that don't pass validation checks?

@ghost
Copy link
Author

ghost commented Oct 16, 2019

Hi @spacether, The reason is we would want to run integration tests, similar to swagger-api/swagger-codegen#5530 but for python client

@wing328
Copy link
Member

wing328 commented Oct 16, 2019

Yup, that's a use case we've encounterd previously. Ruby client generator (and another one I forgot) supports similar option to disable client side validation so that one can use it to test the API backend

@wing328
Copy link
Member

wing328 commented Oct 16, 2019

I've created #4168 for tracking instead.

@wing328 wing328 merged commit d71b1cf into OpenAPITools:master Oct 16, 2019
@ghost ghost deleted the py_skip_validations branch October 16, 2019 07:50
@wing328 wing328 added this to the 4.2.0 milestone Oct 30, 2019
@@ -149,6 +149,8 @@ class Configuration(six.with_metaclass(TypeWithDefault, object)):
self.retries = None
"""Adding retries to override urllib3 default value 3
"""
# Disable client side validation
Copy link
Contributor

Choose a reason for hiding this comment

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

@wing328 typo? Shouldn't this be "Enable"

@grking8
Copy link
Contributor

grking8 commented Oct 5, 2020

Can this option only be set in the code, or via the CLI also?

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.

4 participants