[Python] Add option to select content-type using body or force it for… #10686

tomplus · 2021-10-25T06:19:44Z

In this PR I introduce a new parameter _content_type to api calls and change select_header_content_type to get access to the body of the request. It helps to set a valid content-type for sending requests in some cases. Current logic is the same, but the new method can be easily overridden/patched to perform better detection. What's more, users can force _content_type if necessary.

Context - some APIs (eg. kubernetes, kubernetes-client/python#959, tomplus/kubernetes_asyncio#68) define their own type of content-type (application/json-patch+json, application/strategic-merge-patch+json) where current logic doesn’t work and it’s very difficult to deal with it.

PR python-legacy, I’ll prepare similar for python later.

#8116

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/utils/export_docs_generators.sh
```
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*.
For Windows users, please run the script in Git BASH.
File the PR against the correct branch: master (5.3.0), 6.0.x
If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

@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), please take a look.

Thank you.

… API call

tomplus · 2021-11-03T16:07:38Z

Could you review this PR? Thanks in advance!

spacether · 2021-11-03T16:16:43Z

modules/openapi-generator/src/main/resources/python-legacy/api_client.mustache

@@ -545,10 +545,12 @@ class ApiClient(object):
        else:
            return ', '.join(accepts)

-    def select_header_content_type(self, content_types):
+    def select_header_content_type(self, content_types, method=None, body=None):


Why have you added the parameters method and body here when they re unused in the method?

Thanks for this question. To be backward compatible I don't provide an extra code here but these parameters help me to do it later (patch or subclass the ApiClient).

But maybe we can "break" it a little and add support for JSON Patch:

if method == 'PATCH' and isinstance(body, list) and 'application/json-patch+json' in content_types: return 'application/json-patch+json'

What do you think?

It doesn't make sense to add parameters that are not used by our code because other users want to override them.
Why not pass in _content_type = 'application/json-patch+json' for your mentioned case?

To be more precisely - I use openapi-generator to build my client library. I'd like to prepare my own select_header_content_type which has a custom detection and uses the body and method (for instance I can check if it's json or not, it contains list or not etc.). Even if this original method doesn't use them, my method can.

My end-user, who uses my library, can pass _content_type to manually force the content-type, as you said and it's also important.

In this PR I'd like to provide these two options - forcing content-type and possibility to provide custom detection/selection.

I've decided to add support for 'application/json-patch+json' where these parameters are used.

So I don't like adding parameters that our code base that we don't use but I understand why you need them so adding them is okay.
Can you add a test that patches select_header_content_type and verifies that it is called with all of your needed inputs?

Sure, I've just added tests.

spacether · 2021-11-03T16:22:36Z

This looks like a great feature addition, thanks!
Can you add a test maybe in test_fake_api.py to make sure that the new _content_type parameter is being used in deeper calls when it is passed in?
If you omit that test someone could change or break this feature in the future and we would have no test coverage to let us know that there is a problem.

spacether · 2021-11-22T16:38:52Z

The circleCI error is unrelated:

Could not GET 'https://repo.jfrog.org/artifactory/gradle-plugins/org/jfrog/buildinfo/build-info-extractor-gradle/2.0.16/build-info-extractor-gradle-2.0.16.pom'. Received status code 403 from server: Forbidden

spacether

This looks great; thank you for writing it and adding those tests!

tomplus · 2021-11-22T22:08:15Z

Thanks!

wing328 · 2021-11-23T03:30:00Z

Looks like this breaks the build due to import error

==================================== ERRORS ====================================
____________________ ERROR collecting tests/test_pet_api.py ____________________
ImportError while importing test module '/home/travis/build/OpenAPITools/openapi-generator/samples/client/petstore/python-legacy/tests/test_pet_api.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
tests/test_pet_api.py:16: in <module>
    from unittest.mock import patch
E   ImportError: No module named mock

---------- coverage: platform linux2, python 2.7.12-final-0 ----------
Name                                                                      Stmts   Miss  Cover
---------------------------------------------------------------------------------------------
petstore_api/__init__.py                                                     65      0   100%
petstore_api/api/__init__.py                                                  7      0   100%
petstore_api/api/another_fake_api.py                                         39     29    26%
petstore_api/api/fake_api.py                                                496    460     7%
petstore_api/api/fake_classname_tags_123_api.py                              39     29    26%
petstore_api/api/pet_api.py                                                 273    247    10%
petstore_api/api/store_api.py                                               118    102    14%
petstore_api/api/user_api.py                                                225    201    11%
petstore_api/api_client.py                                                  290    243    16%
petstore_api/configuration.py                                               156    124    21%
petstore_api/exceptions.py                                                   70     47    33%
petstore_api/models/__init__.py                                              50      0   100%
petstore_api/models/additional_properties_any_type.py                        54     34    37%
petstore_api/models/additional_properties_array.py                           54     34    37%

Ref: https://app.travis-ci.com/github/OpenAPITools/openapi-generator/builds/242237307

@tomplus can you please take a look when you've time?

(travis ci is NOT enabled for PR due to credit limits)

tomplus · 2021-11-23T08:06:45Z

Ups! Sorry about that. I'll fix it ASAP.

tomplus · 2021-11-23T13:14:03Z

@wing328 PR to fix the build #10939

[Python] Add option to select content-type using body or force it for…

472e882

… API call

spacether added the Client: Python label Nov 3, 2021

spacether reviewed Nov 3, 2021

View reviewed changes

Add support for application/json-patch+json

c9566d9

spacether mentioned this pull request Nov 19, 2021

[Python] remove default content-type in the request #10782

Merged

5 tasks

Add unittests.

6f795c8

spacether added this to the 5.3.1 milestone Nov 22, 2021

spacether approved these changes Nov 22, 2021

View reviewed changes

spacether merged commit e9f2ccd into OpenAPITools:master Nov 22, 2021

tomplus mentioned this pull request Nov 23, 2021

[Python] Add missing Mock library for Python 2.7. #10939

Merged

5 tasks

tomplus mentioned this pull request Nov 28, 2021

[Python] Add option to select/detect content-type. #10978

Merged

5 tasks

tomplus mentioned this pull request Dec 9, 2021

content-type for patch commands kubernetes-client/python#959

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Python] Add option to select content-type using body or force it for… #10686

[Python] Add option to select content-type using body or force it for… #10686

tomplus commented Oct 25, 2021 •

edited by spacether

Loading

tomplus commented Nov 3, 2021

spacether Nov 3, 2021

tomplus Nov 3, 2021

spacether Nov 4, 2021 •

edited

Loading

tomplus Nov 10, 2021

tomplus Nov 19, 2021

spacether Nov 19, 2021 •

edited

Loading

tomplus Nov 20, 2021

spacether commented Nov 3, 2021

spacether commented Nov 22, 2021

spacether left a comment

tomplus commented Nov 22, 2021

wing328 commented Nov 23, 2021

tomplus commented Nov 23, 2021

tomplus commented Nov 23, 2021

[Python] Add option to select content-type using body or force it for… #10686

[Python] Add option to select content-type using body or force it for… #10686

Conversation

tomplus commented Oct 25, 2021 • edited by spacether Loading

PR checklist

tomplus commented Nov 3, 2021

spacether Nov 3, 2021

Choose a reason for hiding this comment

tomplus Nov 3, 2021

Choose a reason for hiding this comment

spacether Nov 4, 2021 • edited Loading

Choose a reason for hiding this comment

tomplus Nov 10, 2021

Choose a reason for hiding this comment

tomplus Nov 19, 2021

Choose a reason for hiding this comment

spacether Nov 19, 2021 • edited Loading

Choose a reason for hiding this comment

tomplus Nov 20, 2021

Choose a reason for hiding this comment

spacether commented Nov 3, 2021

spacether commented Nov 22, 2021

spacether left a comment

Choose a reason for hiding this comment

tomplus commented Nov 22, 2021

wing328 commented Nov 23, 2021

tomplus commented Nov 23, 2021

tomplus commented Nov 23, 2021

tomplus commented Oct 25, 2021 •

edited by spacether

Loading

spacether Nov 4, 2021 •

edited

Loading

spacether Nov 19, 2021 •

edited

Loading