Skip to content

Conversation

@bmordue
Copy link
Contributor

@bmordue bmordue commented Oct 9, 2017

PR checklist

  • Read the contribution guidelines.
  • Ran the shell script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh and ./bin/security/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in .\bin\windows\.
  • Filed the PR against the correct branch: 3.0.0 branch for changes related to OpenAPI spec 3.0. Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming langauge.

Description of the PR

The intent is to avoid adding a Content-Type header on requests made by a generated Java-feign client if the request body is empty. This is to fix #6647

  • Do not set contentType for requests that have no body params specified.
  • In the Java-Feign template, avoid adding an empty @Headers annotation for "Content-Type" if contentType is not set.

}
//only add content-Type if its no a GET-Method
if(path.getGet() != null || ! operation.equals(path.getGet())){
if (hasBodyParameters){
Copy link
Contributor

Choose a reason for hiding this comment

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

@bmordue shall we keep the logic path.getGet() != null || ! operation.equals(path.getGet()) ?

For POST using HTTP form (not JSON/XML string), we also need the content-type right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I've made sure the default content-type for form data gets applied too.

I don't think the original logic was correct: any path that includes a GET operation would have a default content-type applied, which doesn't seem right. I've added some tests that demonstrate this (ac79987).

@wing328
Copy link
Contributor

wing328 commented Oct 10, 2017

Please run ./bin/java-petstore-all.sh to update Java Petstore client so that CIs can verify the result.

@wing328 wing328 added this to the v2.3.0 milestone Oct 10, 2017
@bmordue
Copy link
Contributor Author

bmordue commented Oct 16, 2017

Apologies for my slow reply, I've been travelling. I ran the update script and committed the changes.

@wing328
Copy link
Contributor

wing328 commented Oct 16, 2017

@JFCote
Copy link
Contributor

JFCote commented Oct 23, 2017

@wing328 Is it normal that this generator use "gson" ? AFAIK, most if not all generator here use jackson?
If that is ok, it looks good to me but I don't know Feign at all :)

@wing328
Copy link
Contributor

wing328 commented Oct 23, 2017

cc @davidgri for review as well since the previous if-condition has been revised.

@wing328
Copy link
Contributor

wing328 commented Oct 23, 2017

@JFCote Java Feign client uses jackson:

    <!-- JSON processing: jackson -->
    <dependency>
      <groupId>com.fasterxml.jackson.core</groupId>
      <artifactId>jackson-core</artifactId>
      <version>${jackson-version}</version>
    </dependency>
    <dependency>
      <groupId>com.fasterxml.jackson.core</groupId>
      <artifactId>jackson-annotations</artifactId>
      <version>${jackson-version}</version>
    </dependency>
    <dependency>
      <groupId>com.fasterxml.jackson.core</groupId>
      <artifactId>jackson-databind</artifactId>
      <version>${jackson-version}</version>
    </dependency>
    <dependency>
      <groupId>com.github.joschi.jackson</groupId>
      <artifactId>jackson-datatype-threetenbp</artifactId>
      <version>${jackson-threetenbp-version}</version>
    </dependency>
    <dependency>
      <groupId>org.apache.oltu.oauth2</groupId>
      <artifactId>org.apache.oltu.oauth2.client</artifactId>
      <version>${oltu-version}</version>
    </dependency>

private OffsetDateTimeTypeAdapter offsetDateTimeTypeAdapter = new OffsetDateTimeTypeAdapter();
private LocalDateTypeAdapter localDateTypeAdapter = new LocalDateTypeAdapter();

public static GsonBuilder createGson() {
Copy link
Contributor

Choose a reason for hiding this comment

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

@wing328 I'm talking about this. You can see use of Gson in other modifier files too.

Copy link
Contributor

Choose a reason for hiding this comment

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

The retrofit generator does use gson. I guess this change is due to regenerating all the Java samples.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok no problem then. :)

@wing328
Copy link
Contributor

wing328 commented Nov 5, 2017

If no further question or feedback on this, I'll merge it on coming Monday/Tuesday.

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.

[JAVA] Bug: Feign client applies Content-Type header when no request body is present

4 participants