[Python] remove default content-type in the request by kevchentw · Pull Request #10782 · OpenAPITools/openapi-generator

kevchentw · 2021-11-04T20:56:54Z

Before this fix, the content-type of the request defaults to "application/json".

With this fix, we no longer set such default.

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.

cc @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)

spacether · 2021-11-04T22:57:38Z

Can you add a test that checks that 1 or two content types are set when they are included in the spec?
Should we allow users to select a specific content type that is present in the given content_types?

kevchentw · 2021-11-05T04:18:32Z

Can you add a test that checks that 1 or two content types are set when they are included in the spec? Should we allow users to select a specific content type that is present in the given content_types?

Sure. Let me add some tests.

I think we cannot let the user to specific content type right now. It defaults to use the first one. Maybe we can create another issue for this?

kevchentw · 2021-11-06T02:24:17Z

@spacether
I think those tests are enough?

openapi-generator/samples/client/petstore/python/tests/test_api_client.py

Lines 104 to 123 in 3f3f837

    
           def test_select_header_content_type(self): 
        
               content_types = ['APPLICATION/JSON', 'APPLICATION/XML'] 
        
               content_type = self.api_client.select_header_content_type(content_types) 
        
               self.assertEqual(content_type, 'application/json') 
        
               content_types = ['application/json', 'application/xml'] 
        
               content_type = self.api_client.select_header_content_type(content_types) 
        
               self.assertEqual(content_type, 'application/json') 
        
               content_types = ['application/xml', 'application/json'] 
        
               content_type = self.api_client.select_header_content_type(content_types) 
        
               self.assertEqual(content_type, 'application/json') 
        
               content_types = ['text/plain', 'application/xml'] 
        
               content_type = self.api_client.select_header_content_type(content_types) 
        
               self.assertEqual(content_type, 'text/plain') 
        
               content_types = [] 
        
               content_type = self.api_client.select_header_content_type(content_types) 
        
               self.assertEqual(content_type, None)

spacether · 2021-11-19T17:50:58Z

This PR and #10686 (review) are both seeking to solve the same problem

spacether · 2021-11-19T17:51:39Z

        :return: Content-Type (e.g. application/json).
        """
        if not content_types:
-            return 'application/json'


I am concerned that this could be a breaking change for servers/client that depended upon this functionality.
How about adding a generator argument useJsonAsDefaultContentType and default it to True.
With that we could put your PR in a patch release.
If you want to default it to False, then this will need to be released as a minor release because it would be a breaking change with a fallback.

I am concerned that this could be a breaking change for servers/client that depended upon this functionality.
How about adding a generator argument useJsonAsDefaultContentType and default it to True.

We've done something similar in other generators (e.g. Ruby) to remove such default behaviour (which was added before I worked on this project). No complains so far.

spacether · 2021-11-19T18:02:07Z

@spacether I think those tests are enough?

openapi-generator/samples/client/petstore/python/tests/test_api_client.py

Lines 104 to 123 in 3f3f837

def test_select_header_content_type(self):

content_types = ['APPLICATION/JSON', 'APPLICATION/XML']

content_type = self.api_client.select_header_content_type(content_types)

self.assertEqual(content_type, 'application/json')

content_types = ['application/json', 'application/xml']

content_type = self.api_client.select_header_content_type(content_types)

self.assertEqual(content_type, 'application/json')

content_types = ['application/xml', 'application/json']

content_type = self.api_client.select_header_content_type(content_types)

self.assertEqual(content_type, 'application/json')

content_types = ['text/plain', 'application/xml']

content_type = self.api_client.select_header_content_type(content_types)

self.assertEqual(content_type, 'text/plain')

content_types = []

content_type = self.api_client.select_header_content_type(content_types)

self.assertEqual(content_type, None)

Thank you for adding those tests

wing328 · 2022-02-14T09:56:24Z

@kevchentw can you please resolve the merge conflicts when you've time and then PM me via Slack?

https://join.slack.com/t/openapi-generator/shared_invite/zt-12jxxd7p2-XUeQM~4pzsU9x~eGLQqX2g

kevchentw changed the title ~~[Pytonn] remove default content-type in the request~~ [Python] remove default content-type in the request Nov 4, 2021

kevchentw mentioned this pull request Nov 4, 2021

[Java][jersey2] set content-type as "application/octet-stream" in the request if not set #10783

Closed

5 tasks

spacether added the Client: Python label Nov 4, 2021

spacether reviewed Nov 4, 2021

View reviewed changes

Comment thread modules/openapi-generator/src/main/resources/python-legacy/api.mustache Outdated

spacether added this to the 5.3.1 milestone Nov 5, 2021

spacether reviewed Nov 19, 2021

View reviewed changes

wing328 modified the milestones: 5.3.1, 5.4.0 Dec 29, 2021

wing328 added the Issue: Bug label Jan 3, 2022

wing328 modified the milestones: 5.4.0, 6.0.0 Jan 31, 2022

kevchentw added 2 commits February 22, 2022 03:57

remove default content-type in the request

cfe9482

add remove default content-type test

90927af

kevchentw force-pushed the fix-8116-2 branch from 013f0c5 to 90927af Compare February 22, 2022 04:07

kevchentw added 9 commits February 22, 2022 04:13

fix duplicate content type

ff6aac1

update sample

65c8aae

add missing params

5d440ed

update sample

c4d707c

only assign content type if exist

537687a

update sample

d94a83f

update sample

35fe9a3

format code

cef7579

update sample

5d9970a

wing328 merged commit ab63786 into OpenAPITools:master Feb 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

[Python] remove default content-type in the request#10782
wing328 merged 11 commits intoOpenAPITools:masterfrom
kevchentw:fix-8116-2

kevchentw commented Nov 4, 2021 •

edited

Loading

Uh oh!

Uh oh!

spacether commented Nov 4, 2021

Uh oh!

kevchentw commented Nov 5, 2021

Uh oh!

kevchentw commented Nov 6, 2021

Uh oh!

spacether commented Nov 19, 2021

Uh oh!

spacether Nov 19, 2021 •

edited

Loading

Uh oh!

wing328 Jan 3, 2022

Uh oh!

spacether commented Nov 19, 2021

Uh oh!

wing328 commented Feb 14, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

kevchentw commented Nov 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR checklist

Uh oh!

Uh oh!

spacether commented Nov 4, 2021

Uh oh!

kevchentw commented Nov 5, 2021

Uh oh!

kevchentw commented Nov 6, 2021

Uh oh!

spacether commented Nov 19, 2021

Uh oh!

spacether Nov 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wing328 Jan 3, 2022

Choose a reason for hiding this comment

Uh oh!

spacether commented Nov 19, 2021

Uh oh!

wing328 commented Feb 14, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kevchentw commented Nov 4, 2021 •

edited

Loading

spacether Nov 19, 2021 •

edited

Loading