Skip to content
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

[Python] each Configuration instance now has its own dicts #4485

Merged

Conversation

spacether
Copy link
Contributor

@spacether spacether commented Nov 14, 2019

Per this issue: #4389
The dictionaries used in Configuration instances are shared across instances.
This is not the intended function.
Each instance should use its own unique dict to store api_key and api_key_prefix
This PR makes that update and adds a test at samples/client/petstore/python/tests/test_configuration.py for verification

Related issue that may be closed by this PR:

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.

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

@wing328 wing328 merged commit a1a9e70 into OpenAPITools:master Nov 17, 2019
jimschubert added a commit to jimschubert/openapi-generator that referenced this pull request Nov 17, 2019
…ulti-level-model-hierarchy

* origin/master:
  minor fix to CI failure
  feat(dart-dio): correctly handle Map<String, Object>, List<Object> using JsonObject (OpenAPITools#4401)
  [OCAML] Fixes cloud.drone.io ocaml-test (OpenAPITools#4501)
  [elm] Add support for oneOf (OpenAPITools#4434)
  [BUG] [Java] Client resttemplate and webclient. Form Params are badly added when they are lists  (OpenAPITools#4461)
  fix: prevent ClassCastException when handling options of (issue OpenAPITools#4468) (OpenAPITools#4495)
  Fixes Python client Configuration class so each instance uses its own dicts (OpenAPITools#4485)
Copy link

@arbelpeled arbelpeled left a comment

Choose a reason for hiding this comment

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

Looks good!
Thanks for the quick fix (:

import petstore_api


class TestConfiguration(unittest.TestCase):

Choose a reason for hiding this comment

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

Nice

@TimoMorris
Copy link

Thanks for fixing this! 😃🚀

@wing328
Copy link
Member

wing328 commented Dec 2, 2019

@spacether thanks for the PR, which has been included in the v4.2.2 release: https://twitter.com/oas_generator/status/1201432648544972800

@spacether spacether deleted the issue_4389_fixes_configuration branch December 3, 2019 17:40
@tomplus
Copy link
Member

tomplus commented Feb 11, 2020

This is a breaking change. Some libraries use the feature of creating default configuration and this PR removed it. Method Configuration.set_default(cls, default) disappeared. I'll try to restore it in a different way.

@spacether
Copy link
Contributor Author

Sorry that we broke this functionality. We look forward to your PR.
Can you share more about your use case?
have you considered these solutions:

  • storing your configuration inputs in environment variables
  • making one configuration instance in a module, then importing it into all the modules which need it

@TimoMorris
Copy link

TimoMorris commented Feb 11, 2020

@tomplus Sorry to hear we've caused you difficulties 😞

FWIW I did quite a detailed write-up of the issues I found with the way the behavior around default configurations when I raised issue #3577 (essentially the same as this issue as this one), which you might find helpful to refer to.

Essentially, it wasn't only caching a copy of the first configuration instantiated to use as a default, but it would then always return this default, even if Configuration was called with other parameters explicitly passed. So it was over-strongly enforcing the default.

It seemed to me the expected behavior would be to (at most) cache the first configuration as a default and return this if you called Configuration() with no parameters. Though even that might be unexpected for some users. But to have the class silently ignore different parameters explicitly passed on subsequent calls led to some very confusing and hard-to-trace behavior.

I imagine it would be relatively straightforward to re-implement a set_default() method, or perhaps have a keyword-only argument e.g. set_as_default for the __init__() method. But however it's implemented, it seems to me that in the Pythonic spirit of "explicit is better than implicit" the user should have to opt-in to it, rather than the default-setting/default-getting behavior being 'on' by default.

And I'd agree with @spacether's suggestions that the user implementing the defaulting behaviour in their own code seems better.

@tomplus
Copy link
Member

tomplus commented Feb 11, 2020

Thanks for the discussion.

I know that it can be implemented in many ways but for me previous implementation was very handy (apart from bugs which were found). I have one server with many API exposed. I was able to create a well-prepared default configuration on startup and then used it without worrying about it.

I think the static method set_default() doesn’t break the principle “explicit is better than implicit” because I have to explicitly set configuration as default configuration.

I'm sending a PR soon.

@tomplus
Copy link
Member

tomplus commented Feb 14, 2020

Here is my PR #5315, Thanks for reviewing.

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.

6 participants