[Python] Support for per-operation servers

[jirikuncar]

Support servers: definition.

See:

• go-experimental[Go] Add multiple servers support to Go(-experimental) client #4635
• java[java] Support templated servers #4998

PR checklist

• Read the contribution guidelines.
• If contributing template-only or documentation-only changes which will change sample output, build the project before.
• Run the shell script(s) under ./bin/ (or Windows batch scripts under.\bin\windows) to update Petstore samples related to your fix. This is important, as CI jobs will verify all generator outputs of your HEAD commit, and these must match the expectations made by your contribution. You only need to run ./bin/{LANG}-petstore.sh, ./bin/openapi3/{LANG}-petstore.sh if updating the code or mustache templates for a language ({LANG}) (e.g. php, ruby, python, etc).
• File the PR against the correct branch: master, 4.3.x, 5.0.x. Default: master.
• Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

[therve]

Some minor nits, looks good otherwise!

[jirikuncar]

Can anyone please review this PR? Thank you!

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

[jirikuncar]

@jimschubert@spacether how can I get more info on failed Travis job?

Loading composer repositories with package information
Updating dependencies (including require-dev)
[Composer\Downloader\TransportException] Content-Length mismatch, received 151799 bytes out of the expected 1479855 install [--prefer-source] [--prefer-dist] [--dry-run] [--dev] [--no-dev] [--no-custom-installers] [--no-autoloader] [--no-scripts] [--no-progress] [--no-suggest] [-v|vv|vvv|--verbose] [-o|--optimize-autoloader] [-a|--classmap-authoritative] [--apcu-autoloader] [--ignore-platform-reqs] [--] [<packages>]...
[ERROR] Failed to execute goal org.codehaus.mojo:exec-maven-plugin:1.2.1:exec (bundle-install) on project Slim4PetstoreServerTests: Command execution failed. Process exited with an error: 1 (Exit value: 1) -> [Help 1]
[ERROR] [ERROR] To see the full stack trace of the errors, re-run Maven with the -e switch.
[ERROR] Re-run Maven using the -X switch to enable full debug logging.
[ERROR] [ERROR] For more information about the errors and possible solutions, please read the following articles:
[ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoExecutionException
[ERROR] [ERROR] After correcting the problems, you can resume the build with the command
[ERROR] mvn <args> -rf :Slim4PetstoreServerTests

[spacether]

@jimschubert@spacether how can I get more info on failed Travis job?

Loading composer repositories with package information
Updating dependencies (including require-dev)
[Composer\Downloader\TransportException] Content-Length mismatch, received 151799 bytes out of the expected 1479855 

It looks like this this error is unrelated to your update. I am closing and reopening this PR to kick of the CI tests again. Sometimes we have transient failures in CI and kicking off the jobs again can sometime solve them.

[spacether]

Possible improvement:
This works and a more standard python way to do it is to use
assert_called_with

[jirikuncar]

If we remove support for Python 2, then yes.

[jirikuncar]

It's still declared in

openapi-generator/samples/openapi3/client/petstore/python-experimental/setup.py

Line 33 in 294d164

EXTRAS= {':python_version <= "2.7"': ['future']}

. I would be happy to change it in the future for Python 3.6+ only.

[wing328]

I think we can drop Python 2.x support in the upcoming 5.x release. (not saying you need to do it as part of this PR though)

[spacether]

Thank you for this PR. Over all it looks good. I have one tweak that I need and an optional request to add a test:

• Our configuration class init method is growing to accept too many arguments when fewer arguments would provide an easier to use and understand interface for our users. Your PR proposal increases the number of non-self arguments to 12. There are actually only 8 types of information being loaded in. If we keep storing every parameter at the init level, this function will end up with 20+ inputs. You mentioned that you are trying to keep the usage similar to other languages. Looking at the Go PR, a single servers property is stored in the server configuration struct rather than storing 4 or more new parameters in the struct. That is in the direction of what I am asking here which is to minimize the number of init parameters. Python style tools like pylint try to prevent too many inputs from being used in functions/methods with the error too-many-arguments (R0913). If you look at how we built signing_info we pass in one class instance rather than adding 6 parameters to the init method. Please structure this input so it adds one or two init parameters for this information. That lets us keep this method easy to understand, use, and maintain. How about something like:

server_info=None
# which is a dict like: {index:1, variables={}}
operation_server_info=None:
# which is a dict like {index_by_operation_id: {'get_pet': 0}, variables_by_operation_id: {'get_pet': blah}}

• Nice to have: Please consider adding a sample that exercises the server_operation_index and server_operation_variables inputs. These features are added but have not yet been tested.

[jirikuncar]

@jimschubert I have tried to create a minimal specification on which we can showcase the dynamic server configuration to avoid modifications of default "Petstore" example. This should follow the work for extensions from #6469.

[jirikuncar]

All tests are passing. cc @jimschubert@spacether

[spacether]

The changes requested point that I brought up has not been addressed. Can you please address it?

[jirikuncar]

@spacether which one?

[jirikuncar]

The configuration object could be a mapping with defined schema. I am trying to keep the configuration similar between different clients Go/Java/Python/... I don't see why passing server_info={"index": 1} is better than server_index=1 which is the common use-case or server_variables={"region": "eu"} for customers from EU.

[spacether]

The unaddressed point is Our configuration class init method is growing to accept too many arguments, please reduce the number of inputs that you use here to 2 or 1. It is better because the init signature is growing to include too many parameters.
I am fine with many configuration_instance.property_name access to data inside the configuration, but our init method should not contain every parameter that every config feature needs as variable input.
If we do that, the method signature gets too large and much harder to understand.
Instead, related variables that implement one config feature should be grouped together as a single dict input or class instance input.

• This is consistent with what the python style tool pylint suggests that we do through its too many arguments warning
• This is what we have done in the past with the signing_info which added one input rather than 6
• This is consistent with what the Go generator does where only one servers input is added in the configuration Struct
• https://www.matheus.ro/2018/01/29/clean-code-avoid-many-arguments-functions/

Will it help if we discuss this? I can be available over Slack if you want to chat.

[jirikuncar]

The problem with Python configuration object is that it acts both as Go configuration and context at the same time. We will have the same problem in the future.

[spacether]

The problem with Python configuration object is that it acts both as Go configuration and context at the same time. We will have the same problem in the future.

So in my mind that's not the problem. Here's my thought process on what is the problem. New users want to use the configuration instance. They go to read its init method, if they only need to use one feature, they should only have to pass in one argument. If I am a new user and I want to use one feature, and I have to read through 20+ variables to try to understand which specific input I should be passing in when those 20+ variables are inputs for 10 features, then we have made using this class unnecessarily complex and hard to understand. Many users will not be using the configurable servers feature. It is easier for them if they can skip over the 1 or 2 inputs because they are not relevant vs reading these 4 variables and thinking hey do I need this input for feature X. What do you think?

[jirikuncar]

@spacether it's more complex than you describe. It's not really a single feature. One could break it into:

1. Support for servers
2. Support for servers per endpoint
3. Support for templated servers (x2)

Based on usage in other libraries:

• Most users might change only a single variable (e.g. region);
• Some users might change the server index;
• In rare cases users want to change a configuration of a specific endpoint;

Nesting is not more user friendly! Moreover we have the same pattern in other languages and I want to make sure that it's consistent. All parameters have reasonable defaults so users do not have to specify them if they are not planning to use these features.

[spacether]

Group consensus is that keeping these as non nested inputs is preferred.

[spacether]

Can you resolve the conflicts then I can merge it in?