Openapi-generator: [BUG] [Python] Default configuration gives unexpected behaviour

Created on 7 Aug 2019 · 6Comments · Source: OpenAPITools/openapi-generator

Description

After it has been called once, the Configuration class constructor effectively ignores the parameters passed to it on subsequent calls, and instead returns a copy of the configuration created the first time it was called.

openapi-generator version

v4.0.1 (this is the version I was using, and also where the issue first appears)

OpenAPI declaration file content or url

https://github.com/OpenAPITools/openapi-generator/blob/master/modules/openapi-generator/src/main/resources/python/configuration.mustache

Command line used for generation

java -jar openapi-generator-cli-4.0.1.jar generate -i my-spec.json -g python -o .\my_package -DprojectName=my-project -DpackageName=my_package

Steps to reproduce

Once you've generated the api client package as above, run:

import my_package

config1 = my_package.Configuration(host="https://my-first-site.com")
config2 = my_package.Configuration(host="https://a-different-site.com")

print(config1.host)  # https://my-first-site.com
print(config2.host)  # https://my-first-site.com

Or alternatively:

import my_package

config1 = my_package.Configuration()  # this call uses the default host from the spec
config2 = my_package.Configuration(host="https://a-different-site.com")

print(config1.host)  # https://default-host-from-spec.com
print(config2.host)  # https://default-host-from-spec.com

The two hosts for the two different configurations should be different.

[Note: it may seem strange to want to set different hosts for one API, but the software I'm working with doesn't have a single central API, but rather allows users to set up their own instance(s) of the API. Each user's API has the same spec, except for being hosted wherever the user has set it up. I'm working to develop tools that any user could use, but obviously they have to provide the host to begin with.]

Related issues/PRs

this commit - this commit introduced the TypeWithDefault class which Configuration inherits from.
#3002 (for issue #3003) - this added some parameters to the Configuration constructor, which is a great enhancement, but the way it interacts with the TypeWithDefault parent class leads to the above unexpected and undesired behavior.

Suggest a fix/enhancement

It depends what the purpose of TypeWithDefault is supposed to be. I can see two potential purposes:

a) Enforce the configuration to always be the 'default', i.e. with the host URL from the API spec (except when manually changed by setting attributes directly):
❌ This has already been undermined by the changes in #3002, as it's now possible to set a different configuration on the first call. In any case, I would also argue this enforcement is an anti-pattern and not in the normal spirit of Python (being an "adult consenting" language).

b) To set a ._default class attribute, so that the user has this available should they ever need to retrieve the default configuration:
❌ It goes further than this, as it returns this default on every call.

Either way, TypeWithDefault is currently not correctly doing what it was supposed to. It's also worth noting that I don't think the TypeWithDefault.set_method() could ever work correctly - how would you create a different configuration to pass in as the new default, when the class will always return the 'old' default for any configuration you create?!

[There's a small chance I might have missed something here, as I don't have much experience with six and haven't done much metaprogramming, but that seems to be how it behaves.]

The solutions I see are:
1) The configuration should be fixed to the default given by the API spec -> revert #3002
2) There should be a default configuration available to retrieve as a class attribute -> change the return value of the TypeWithDefault.__call__() method from return copy.copy(cls._default) to return type.__call__(cls, **kwargs)
3) Have no default configuration -> remove TypeWithDefault entirely.

My choice:

As previously mentioned, I think the "enforcement" behavior is an anti-pattern, and unhelpful. Furthermore, the changes made in #3002 are also really useful, so I am against (1).
I think (2) is a bad compromise, as it would mean all TypeWithDefault does is create the ._default attribute. If users really need a copy of the default configuration, they can just call Configuration() with no arguments, i.e. the default parameters.
As mentioned in the previous point, getting a 'default' configuration is still possible, and this would mean that users can call Configuration() with their desired parameters and get back what they expect, no surprises.

I've made a gist with the change suggested in (3): https://gist.github.com/TimoMorris/b3b0b64f966408a5475dce05bc4243dc
I haven't done a PR, partly because I wanted to get some feedback on what is partly a design decision, and partly because I'm not sure I know enough to make the change myself.
(I also changed the constructor slightly to avoid api_key and api_key_prefix having dictionaries as their default argument, as this can cause subtle bugs - this would be a good change to make in any case.)

Python Bug

Source