avoid setting debug property if not needed #18872

ZeeD · 2024-06-06T08:38:25Z

this pr add an optional boolean flag to enable / disable / keep as it is the debug property
fixes #18871

@cbornet @tomplus @krjakbrjak @fa0311 @multani

PR checklist

Read the contribution guidelines.
Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
Run the following to build the project and update samples:
```
./mvnw clean package 
./bin/generate-samples.sh ./bin/configs/*.yaml
./bin/utils/export_docs_generators.sh
```
(For Windows users, please run the script in Git BASH)
Commit all changed files.
This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
These must match the expectations made by your contribution.
You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/configs/java*.
IMPORTANT: Do NOT purge/delete any folders/files (e.g. tests) when regenerating the samples as manually written tests may be removed.
File the PR against the correct branch: master (upcoming 7.6.0 minor release - breaking changes with fallbacks), 8.0.x (breaking changes without fallbacks)
If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

wing328 · 2024-06-06T08:41:23Z

thanks for the PR. please follow step 3 to update the samples so that the CI can verify the change.

ZeeD · 2024-06-06T09:17:46Z

updated the samples

ZeeD · 2024-06-12T12:43:42Z

Hi.
may someone have a look at the changes?

wing328 · 2024-06-12T14:32:58Z

can you please review the test failures when you've time?

ZeeD · 2024-06-12T15:31:25Z

Oh, it was the fact that bool | None was not supported in python 3.7. switched to Optional[bool]

wing328 · 2024-06-13T04:49:45Z

that's ok.

all tests passed. thanks again for the PR.

if no one has further feedback on this PR, i'll merge it later this week.

multani

Not setting self.debug to a specific value will not set self.__debug, which may create AttributeError because the Configuration object is not correctly initialized.

I would also suggest to add a few tests to actually show this behavior:

Configuration created without passing debug
Configuration created with debug=False
Configuration created with debug=True

In all three cases, configuration.debug should be called to verify the expected behavior. With this change, the first test would fail.

multani · 2024-06-15T09:02:15Z

modules/openapi-generator/src/main/resources/python/configuration.mustache

+        if debug is not None:
+            self.debug = debug


If this self.debug property is not always set, then the underlying __debug property will also not always be set, which may produce AttributeError.

I would suggest to at least set self.__debug to False by default so that the debug setter/getter don't have a way to fail, and all the code that are also using this attribute will also not fail.

I can easily add a default value for self.__debug, but I'm not sure how to write the tests.
Can you point me to a sample I can adapt?

You can check here for instance: https://github.com/OpenAPITools/openapi-generator/blob/master/samples/openapi3/client/petstore/python/tests/test_configuration.py

There are several tests around this class, you should be able to create a test that fails with your current change and make it pass by setting an initial value for __debug.

There may be other places with this kind of tests (they are written manually), but I didn't check.

Uhm... I was starting to write some tests, but it seems there are some unrelated failures.

$ cd samples/openapi3/client/petstore/python $ poetry install [...] $ poetry run pytest [...] ================================================= short test summary info ================================================== FAILED tests/test_model.py::ModelTests::test_valdiator - AttributeError: 'ModelTests' object has no attribute 'assertEquals'. Did you mean: 'assertEqual'? FAILED tests/test_pet_api.py::PetApiTests::test_add_pet_and_get_pet_by_id - urllib3.exceptions.MaxRetryError: HTTPConnectionPool(host='localhost', port=80): Max retries exceeded with url: /v2/pet... FAILED tests/test_pet_api.py::PetApiTests::test_add_pet_and_get_pet_by_id_with_http_info - urllib3.exceptions.MaxRetryError: HTTPConnectionPool(host='localhost', port=80): Max retries exceeded with url: /v2/pet... FAILED tests/test_pet_api.py::PetApiTests::test_add_pet_and_get_pet_by_id_without_preload_content_flag - urllib3.exceptions.MaxRetryError: HTTPConnectionPool(host='localhost', port=80): Max retries exceeded with url: /v2/pet... FAILED tests/test_pet_api.py::PetApiTests::test_delete_pet - urllib3.exceptions.MaxRetryError: HTTPConnectionPool(host='localhost', port=80): Max retries exceeded with url: /v2/pet... FAILED tests/test_pet_api.py::PetApiTests::test_find_pets_by_status - urllib3.exceptions.MaxRetryError: HTTPConnectionPool(host='localhost', port=80): Max retries exceeded with url: /v2/pet... FAILED tests/test_pet_api.py::PetApiTests::test_find_pets_by_tags - urllib3.exceptions.MaxRetryError: HTTPConnectionPool(host='localhost', port=80): Max retries exceeded with url: /v2/pet... FAILED tests/test_pet_api.py::PetApiTests::test_get_pet_by_id_serialize - urllib3.exceptions.MaxRetryError: HTTPConnectionPool(host='localhost', port=80): Max retries exceeded with url: /v2/pet... FAILED tests/test_pet_api.py::PetApiTests::test_update_pet - urllib3.exceptions.MaxRetryError: HTTPConnectionPool(host='localhost', port=80): Max retries exceeded with url: /v2/pet... FAILED tests/test_pet_api.py::PetApiTests::test_update_pet_with_form - urllib3.exceptions.MaxRetryError: HTTPConnectionPool(host='localhost', port=80): Max retries exceeded with url: /v2/pet... FAILED tests/test_pet_api.py::PetApiTests::test_upload_file - urllib3.exceptions.MaxRetryError: HTTPConnectionPool(host='localhost', port=80): Max retries exceeded with url: /v2/pet... ================================== 11 failed, 264 passed, 3 skipped, 9 warnings in 14.25s ==================================

I'm normally using python 3.12, where it was removed
Should I also update the unrelated tests?

For assertEquals, this should be migrated in another PR I think. Can you test with 3.11 instead?

For the URL failures, there's an explanation how to start a test container to support the tests at the top of tests/test_pet_api.py

(oh, and for the various PetApiTests failed tests, while it's easy to follow the instructions in the tests

openapi-generator/samples/openapi3/client/petstore/python/tests/test_pet_api.py

Lines 6 to 11 in 793aba7

Run the tests.

$ docker pull swaggerapi/petstore

$ docker run -d -e SWAGGER_HOST=http://petstore.swagger.io -e SWAGGER_BASE_PATH=/v2 -p 80:8080 swaggerapi/petstore

$ pip install -U pytest

$ cd petstore_api-python

$ pytest

I would suggest to enhance them with testcontainers so you can avoid launching the docker image by hand

ZeeD · 2024-06-17T08:22:36Z

Updated the pr to add explicit check the behavior on the various values you can pass to the constructor

multani · 2024-06-17T08:27:27Z

modules/openapi-generator/src/main/resources/python/configuration.mustache

@@ -137,6 +137,7 @@ conf = {{{packageName}}}.Configuration(
    """

    _default = None
+    __debug = False


There's no reason to make it a class attribute, can you move this as an instance attribute line 217?

multani · 2024-06-17T08:31:02Z

samples/openapi3/client/petstore/python/tests/test_configuration.py

@@ -76,5 +76,14 @@ def test_get_host_from_settings(self):
        self.assertEqual("http://petstore.swagger.io:8080/v2", self.config.get_host_from_settings(0, {'port': '8080'}))
        self.assertEqual("http://dev-petstore.swagger.io:8080/v2", self.config.get_host_from_settings(0, {'server': 'dev-petstore', 'port': '8080'}))

+    def testConfigurationDebug(self):
+        for debug, expected in [(True, True), (False, False), (None, False)]:
+            with self.subTest(debug=debug, expected=expected):


TIL about unittest's subTest 🙇

multani · 2024-06-17T08:31:09Z

samples/openapi3/client/petstore/python/tests/test_configuration.py

+        with self.subTest(expected=False):
+            c = petstore_api.Configuration()
+            self.assertEqual(expected, c.debug)


I think this works a bit by chance, since expected in the assert is the one taken from the for-loop line 80.

Can you make the test a bit more explicit?

Suggested change

with self.subTest(expected=False):

c = petstore_api.Configuration()

self.assertEqual(expected, c.debug)

expected = False

with self.subTest(debug="unset", expected=expected):

c = petstore_api.Configuration()

self.assertEqual(expected, c.debug)

d'oh, you're right!
rewrote them and added explicit message

multani

Thanks for the tests and fixes!

ZeeD · 2024-06-20T08:18:21Z

I was just wondering if there is anything else to do on my side.

multani · 2024-06-20T08:20:25Z

@wing328 can you check this pull request?
It's good to merge for me 👍

wing328 · 2024-06-20T08:23:23Z

@multani thanks for reviewing the change

@ZeeD thanks for the PR

will merge after all tests pass

ZeeD · 2024-06-20T10:35:12Z

Thanks!

avoid setting debug property if not needed

b34c7e0

update samples

ad984d2

fix compatibility with python 3.7

c12633f

wing328 added Client: Python Issue: Bug labels Jun 13, 2024

wing328 added this to the 7.7.0 milestone Jun 13, 2024

multani suggested changes Jun 15, 2024

View reviewed changes

ZeeD added 3 commits June 17, 2024 09:28

always set Configuration.__debug

da70b93

update samples

2f719a3

check Configuration behavior when debug parameter is / is not set

b269b63

multani suggested changes Jun 17, 2024

View reviewed changes

address PR requests

197a492

ZeeD force-pushed the patch-1 branch from 29634d0 to 197a492 Compare June 17, 2024 09:30

multani approved these changes Jun 17, 2024

View reviewed changes

Merge remote-tracking branch 'origin/master' into patch-1

1687849

wing328 merged commit e5ae07c into OpenAPITools:master Jun 20, 2024
35 checks passed

ZeeD deleted the patch-1 branch June 20, 2024 10:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

avoid setting debug property if not needed #18872

avoid setting debug property if not needed #18872

ZeeD commented Jun 6, 2024 •

edited

Loading

wing328 commented Jun 6, 2024

ZeeD commented Jun 6, 2024

ZeeD commented Jun 12, 2024

wing328 commented Jun 12, 2024

ZeeD commented Jun 12, 2024

wing328 commented Jun 13, 2024

multani left a comment

multani Jun 15, 2024

ZeeD Jun 16, 2024

multani Jun 16, 2024

ZeeD Jun 17, 2024

multani Jun 17, 2024

ZeeD Jun 17, 2024

ZeeD commented Jun 17, 2024

multani Jun 17, 2024

ZeeD Jun 17, 2024

multani Jun 17, 2024

multani Jun 17, 2024

ZeeD Jun 17, 2024

multani left a comment

ZeeD commented Jun 20, 2024

multani commented Jun 20, 2024

wing328 commented Jun 20, 2024

ZeeD commented Jun 20, 2024

	Run the tests.
	$ docker pull swaggerapi/petstore
	$ docker run -d -e SWAGGER_HOST=http://petstore.swagger.io -e SWAGGER_BASE_PATH=/v2 -p 80:8080 swaggerapi/petstore
	$ pip install -U pytest
	$ cd petstore_api-python
	$ pytest

avoid setting debug property if not needed #18872

avoid setting debug property if not needed #18872

Conversation

ZeeD commented Jun 6, 2024 • edited Loading

PR checklist

wing328 commented Jun 6, 2024

ZeeD commented Jun 6, 2024

ZeeD commented Jun 12, 2024

wing328 commented Jun 12, 2024

ZeeD commented Jun 12, 2024

wing328 commented Jun 13, 2024

multani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZeeD commented Jun 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

multani left a comment

Choose a reason for hiding this comment

ZeeD commented Jun 20, 2024

multani commented Jun 20, 2024

wing328 commented Jun 20, 2024

ZeeD commented Jun 20, 2024

ZeeD commented Jun 6, 2024 •

edited

Loading