Skip to content

[python-client] Add model default values - #1776

Merged
wing328 merged 11 commits into
OpenAPITools:masterfrom
spacether:issue_1748_add_model_default_values
Jan 9, 2019
Merged

[python-client] Add model default values#1776
wing328 merged 11 commits into
OpenAPITools:masterfrom
spacether:issue_1748_add_model_default_values

Conversation

@spacether

@spacetherspacether commented Dec 29, 2018

Copy link
Copy Markdown
Contributor

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/python-petstore.sh and ./bin/security/python-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\.
  • Filed the PR against the correct branch: master, 3.4.x, 4.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

This PR lets python client users set model defaults on integers, numbers, and arrays when they are not currently able to in a swagger2.0 spec. The new code loads model variable examples into the model variable default value if the default is not already set and an example exists for the variable.
Fixes this issue: #1748

Reasons to do this:
The current swagger-parser is not including default values that are float, integer, or array when processing a swagger2.0 spec. So in a swagger 2.0 spec, there is no way to set defaults on:

  • integers
  • numbers
  • arrays

This PR adds in fallback code which uses and assigns a spec example value as the default value if one exists in the spec.
So it uses examples to set defaults on the types

  • string
  • boolean
  • integers
  • numbers
  • arrays

Tests

  • test_type_holder_example.py added to verify that examples are loaded into defaults
  • test_type_holder_default.py added to verify what default-only definition does and does not work
  • samples updated to pass CircleCi tests (173 files)

@spacetherspacether changed the title Issue 1748 add model default values[python] Add model default valuesDec 30, 2018
@spacetherspacether changed the title [python] Add model default values[python-client] Add model default valuesDec 30, 2018
@spacether
spacetherforce-pushed the issue_1748_add_model_default_values branch from dcb9195 to 5b663a1CompareDecember 30, 2018 17:05
// include fallback to example, default defined as server only
// example is not defined as server only
if (p.getExample() != null) {
if (p.getExample().toString().equalsIgnoreCase("false"))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What about using Boolean.valueOf instead of using equalsIgnoreCase?

@spacetherspacetherJan 7, 2019

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Boolean.valueOf(string) will always return True or False,
so Boolean.valueOf("5") would return False.
When I pass in "5" as a boolean the code should return null. Code updated use equalsIgnoreCase to check for "true" and "false" otherwise return null.
I'd prefer matching on true false strings rather than truthy "0" "5" "1.234" "[1,2,3]" because truthiness differs from language.

equalsIgnoreCase lets us only look for a string match to true or false ignoring case.

@wing328wing328Jan 7, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

When I pass in "5" as a boolean the code should return null

If someone puts down 5 as the example value and assume it will be treated false in boolean, it's the user's issue (not something we need to handle) and I don't think it should return null (clearly there's a value defined)

We've seen spec using 1 or 0 to as example value for boolean type (not entirely correct as they should be using "true" or "false" instead) but still we want to able to handle these cases correctly as some languages (e.g. Perl) treat 1 as true.

@spacetherspacetherJan 7, 2019

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Coercing it to a boolean gives the user back some boolean which is usefull. I will change it to Boolean.valueOf. This behavior may surprise python users though because they will be using a python client with java casting of string truthiness.
"1" -> True in Python, false in Java
"0" -> True in Python, false in Java

@wing328wing328 added this to the 4.0.0 milestone Jan 7, 2019
@wing328
wing328 merged commit 539ec23 into OpenAPITools:masterJan 9, 2019
@wing328

Copy link
Copy Markdown
Member

@spacether the change looks good to me. Thanks for your contribution.

@spacether
spacether deleted the issue_1748_add_model_default_values branch January 9, 2019 05:19
jimschubert added a commit to jimschubert/openapi-generator that referenced this pull request Jan 9, 2019
* master: (52 commits)
Support for "x-enum-descriptions" (OpenAPITools#1752)
add new sample files
Add a checklist to issue report (OpenAPITools#1851)
Fix typo in template (OpenAPITools#1859)
update samples
add isModel to updatebooleanflagwithcodegenproperty (OpenAPITools#1844)
[python-client] Add model default values (OpenAPITools#1776)
fix: force to decode as utf-8 when header contains application/json to avoid text garbling. (OpenAPITools#1700)
Better support for composed schema (allOf) (OpenAPITools#1842)
Add additional properties to Java CodegenModel (OpenAPITools#1854)
Minor Angular type improvements (OpenAPITools#1843)
[DART] fix: set fields to null if json value is null. (OpenAPITools#1798)
add options to maven plugin (OpenAPITools#1845)
fix unqiue name in handling forward slash (OpenAPITools#1839)
better handle of oauth (OpenAPITools#1838)
[C] Support for authentication methods (OpenAPITools#1628)
better error message when parameter ref not defined (OpenAPITools#1837)
Add links to articles about openapi generator
Add a link to @watiko article
Delete langs command (OpenAPITools#1836)
...
@dojedadojeda mentioned this pull request Feb 21, 2019
6 tasks
@wing328

Copy link
Copy Markdown
Member

@spacether If the default value is not defined, it shouldn't fall back to the example value.

I'll file a PR to remove the fallback.

@spacether

Copy link
Copy Markdown
ContributorAuthor

Good to know. Thank you you @wing328 sorry for the breaking change.

@wing328

Copy link
Copy Markdown
Member

@spacether no worries. In the templates, you can do something similar to the following to have the default value fall back to example value (not something I recommend but ok if there's a valid use case)

{{{defaultValue}}}{{^defaultValue}}{{{example}}}{{/defaultValue}}

@wing328

Copy link
Copy Markdown
Member

FYI. Filed #2213

A-Joshi pushed a commit to ihsmarkitoss/openapi-generator that referenced this pull request Feb 27, 2019
* Adds two models to the v2.0 spec, uses examples as defaults in python client
* Adds array default and type_holder_default and type_holder_example tests
* Re-generated python security client with ./bin/security/python-petstore.sh
* Changes comment text, rebased master
* Updates client + server samples
* Adds missing samples updates
* Changes python client to look for true or false with booleans in toDefaultValue
* Changes boolean casting to use Boolean.valueOf
* Adds deserialization fix for python tests
* Changes Mock to namedtuple in python deserialization tests
* Actually remove unittest.mock
Sign up for freeto 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

@spacether@wing328