Adding gzip support for Spring Decoder #230

jskim1991 · 2019-10-16T14:12:32Z

Based on my usage of FeignClient, I was only able to get gzip content. So I only added test for gzip decompression. If there is a need to add support for other content-encoding with different types of http clients, please provide me some information and we can work on it together.

Thanks :)

pivotal-issuemaster · 2019-10-16T14:12:35Z

@jskim1991 Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

holy12345 · 2019-10-16T14:41:54Z

Hi @jskim1991 thanks for you PR. Test failed please fixed it.

On the other thing just exchange ideas, I see the judgment logic in SpringDecoder. Do you think create new class and put logic in this class. because SpringDecoder is default.(Loaded into the container by Spring by default)

The source code

	@Bean
	@ConditionalOnMissingBean
	public Decoder feignDecoder() {
		return new OptionalDecoder(
				new ResponseEntityDecoder(new SpringDecoder(this.messageConverters)));
	}

So I think If the property feign.compression.response.enabled is true, spring does not load the default class but loads the class that supports gzip.
So the order looks like this：

If the client customizes Decode then Spring loads the custom Decode.
If the property feign.compression.response.enabled is true then Spring loads the gizp decode.
if no things customizes then spring loads the SpringDecoder.

Any Thoughts?

OlgaMaciaszek · 2019-10-16T15:13:22Z

...-openfeign-core/src/main/java/org/springframework/cloud/openfeign/support/SpringDecoder.java

+							: null;
+
+			if (encoding != null) {
+				if (encoding.contains(HttpEncoding.GZIP_ENCODING)) {


Please make this behaviour optional and not turned on by default in order to maintain backwards compatibility. You can, for example make it dependant on a new property.

OlgaMaciaszek · 2019-10-16T15:14:17Z

...-openfeign-core/src/main/java/org/springframework/cloud/openfeign/support/SpringDecoder.java

+			if (encoding != null) {
+				if (encoding.contains(HttpEncoding.GZIP_ENCODING)) {
+					Response build = response.toBuilder()
+							.body(this.decompress(response).getBytes()).build();


Remove this.

decompress(response) can return null - add nullcheck.

OlgaMaciaszek · 2019-10-16T15:40:23Z

...gn-core/src/test/java/org/springframework/cloud/openfeign/SpringDecoderCompressionTests.java

+				.isEqualTo(new Hello("hello world via response"));
+	}
+
+	public static class Hello {


does this have to be public?

pivotal-issuemaster · 2019-10-16T15:43:22Z

@jskim1991 Thank you for signing the Contributor License Agreement!

OlgaMaciaszek · 2019-10-16T15:52:30Z

@holy12345 - I agree with you, with the distinction that I still would like to see maybe an additional property used to opt in apart from feign.compression.response.enabled, cause we would be changing behaviour in a minor version and could cause issues for users that handled it on their own. We cannot add breaking changes in a minor.

OlgaMaciaszek · 2019-10-16T16:23:59Z

Build seems to be failing due to a circleci-related issue. Contacting them to find out what the problem is.

OlgaMaciaszek · 2019-10-16T16:40:37Z

@jskim1991 I have merged this fix for the circleci issue: https://github.com/spring-cloud/spring-cloud-openfeign/pull/231/files Plese merge master to your branch to get this commit and rebuild the PR.

jskim1991 · 2019-10-16T23:04:26Z

@OlgaMaciaszek, thank you so much for your feedback. I will work on it to meet your directions and let you know.

jskim1991 · 2019-10-17T03:29:07Z

@OlgaMaciaszek, i thought about your comment on backward compatibility. If users customized feign.codec.Decoder, it doesn't use SpringDecoder and also, the logic checks for Http Header to see what content type is returned. If you really want me to add a new property, I will do so but I am not sure why that would fail in normal cases as well as when customized decoder is implemented. Please let me know. Thanks :)

Edit: i guess one possible way is they do decompression part and use SpringDecoder on the decompressed response. I think your request for an additional property makes sense.

…m1991/spring-cloud-openfeign into spring-decoder-gzip-support

codecov · 2019-10-19T14:46:38Z

Codecov Report

Merging #230 into master will increase coverage by 0.53%.
The diff coverage is 83.33%.

@@             Coverage Diff              @@
##             master     #230      +/-   ##
============================================
+ Coverage     76.44%   76.98%   +0.53%     
- Complexity      336      345       +9     
============================================
  Files            41       43       +2     
  Lines          1422     1451      +29     
  Branches        207      213       +6     
============================================
+ Hits           1087     1117      +30     
+ Misses          246      241       -5     
- Partials         89       93       +4

Impacted Files	Coverage Δ	Complexity Δ
...mework/cloud/openfeign/FeignAutoConfiguration.java	`88.88% <ø> (ø)`	`3 <0> (ø)`	⬇️
...amework/cloud/openfeign/support/SpringEncoder.java	`75.86% <100%> (+0.42%)`	`11 <0> (ø)`	⬇️
...feign/support/DefaultGzipDecoderConfiguration.java	`100% <100%> (ø)`	`2 <2> (?)`
...rk/cloud/openfeign/support/DefaultGzipDecoder.java	`79.16% <79.16%> (ø)`	`6 <6> (?)`
...bbon/HttpClientFeignLoadBalancedConfiguration.java	`91.17% <0%> (-5.89%)`	`2% <0%> (ø)`
...mework/cloud/openfeign/FeignClientFactoryBean.java	`72.35% <0%> (+4.7%)`	`47% <0%> (+1%)`	⬆️

jskim1991 · 2019-10-19T14:49:25Z

@OlgaMaciaszek I committed some new changes to support backward compatibility as you requested. Please let me know if there is anything I need to change to make it better. Thanks :)

Edit: I was thinking about which would be a better way to inject gzipdecoder. Let me know what you think of it.

new OptionalDecoder(new ResponseEntityDecoder(new DefaultGzipDecoder(new SpringDecoder)))
new DefaultGzipDecoder(new OptionalDecoder(new ResponseEntityDecoder(new SpringDecoder)))

I tried both and don't see much difference. Going with option 1 made sense to me initially since OptionalDecoder and ResponseEntityDecoder have some logic and flow to get to a specific point in SpringDecoder. So I think it is aligned with the previous design where it starts from OptionalDecoder and goes in order.

jskim1991 · 2019-10-24T16:37:17Z

@OlgaMaciaszek I am still waiting for your feedback

ryanjbaxter · 2019-10-28T14:20:16Z

...a/org/springframework/cloud/openfeign/encoding/FeignAcceptGzipEncodingAutoConfiguration.java

@@ -39,7 +39,7 @@
 @EnableConfigurationProperties(FeignClientEncodingProperties.class)
 @ConditionalOnClass(Feign.class)
 @ConditionalOnBean(Client.class)
-@ConditionalOnProperty(value = "feign.compression.response.enabled",


the havingValue property is not necessary

ryanjbaxter · 2019-10-28T14:20:39Z

.../org/springframework/cloud/openfeign/encoding/FeignContentGzipEncodingAutoConfiguration.java

@@ -42,7 +42,7 @@
 // The OK HTTP client uses "transparent" compression.
 // If the content-encoding header is present it disable transparent compression
 @ConditionalOnMissingBean(type = "okhttp3.OkHttpClient")
-@ConditionalOnProperty(value = "feign.compression.request.enabled",


the havingValue property is not necessary

ryanjbaxter · 2019-10-28T14:22:43Z

...feign-core/src/main/java/org/springframework/cloud/openfeign/support/DefaultGzipDecoder.java

+		}
+		GZIPInputStream gzipInputStream = new GZIPInputStream(
+				response.body().asInputStream());
+		BufferedReader reader = new BufferedReader(


I believe you need to close these readers

ryanjbaxter · 2019-10-28T14:24:21Z

...c/main/java/org/springframework/cloud/openfeign/support/DefaultGzipDecoderConfiguration.java

+	}
+
+	@Bean
+	@ConditionalOnProperty(value = "feign.compression.response.useGzipDecoder",


We should add some documentation about the new property

jskim1991 · 2019-10-29T15:52:03Z

@ryanjbaxter I have made some changes that you requested. Please let me know if there is anything else I need to change. Thanks :)

OlgaMaciaszek

Looks good, but there are still some things that should be fixed.

OlgaMaciaszek · 2019-10-29T16:59:19Z

...c/main/java/org/springframework/cloud/openfeign/support/DefaultGzipDecoderConfiguration.java

+ */
+@Configuration
+@ConditionalOnProperty(value = "feign.compression.response.enabled",
+		matchIfMissing = false)


matchIfMissing = false is not needed. That's the default. value = is not needed.

OlgaMaciaszek · 2019-10-29T17:20:29Z

...feign-core/src/main/java/org/springframework/cloud/openfeign/support/DefaultGzipDecoder.java

+import org.springframework.cloud.openfeign.encoding.HttpEncoding;
+
+/**
+ * When response is compressed as gzip, this decompresses and uses SpringDecoder to


change to {@link SpringDecoder}

OlgaMaciaszek · 2019-10-29T17:24:15Z

...c/main/java/org/springframework/cloud/openfeign/support/DefaultGzipDecoderConfiguration.java

+@ConditionalOnProperty(value = "feign.compression.response.enabled",
+		matchIfMissing = false)
+// The OK HTTP client uses "transparent" compression.
+// If the accept-encoding header is present it disable transparent compression


Fix the grammar and punctuation:
If the accept-encoding header is present, it disables transparent compression.

OlgaMaciaszek · 2019-10-29T17:25:46Z

...c/main/java/org/springframework/cloud/openfeign/support/DefaultGzipDecoderConfiguration.java

+	}
+
+	@Bean
+	@ConditionalOnProperty(value = "feign.compression.response.useGzipDecoder",


Remove value = and matchIfMissing = false. They are unnecessary.

For this, i see most configs use dashes between words in the documentation. Should I change this property name from camel case to dashes?

Spring will automatically do that

OlgaMaciaszek · 2019-10-29T17:37:05Z

...c/main/java/org/springframework/cloud/openfeign/support/DefaultGzipDecoderConfiguration.java

+		this.messageConverters = messageConverters;
+	}
+
+	@Bean


We should probably have a @ConditionalOnMissingBean here, i.e., if the user provides their own Decoder implementation, that should have priority and, at the same time, ensure that this configuration is processed before the FeignClientsConfiguration that provides its own @Bean @ConditionalOnMissingBean Decoder.

jskim1991 · 2019-10-30T13:30:10Z

@OlgaMaciaszek Thank you for feedback. Please let me know what else I need to change.

OlgaMaciaszek

Looks good now.

Adding gzip support for Spring Decoder

19530d3

spring-projects-issues added the waiting-for-triage label Oct 16, 2019

Restoring test config

f8e29e3

OlgaMaciaszek requested changes Oct 16, 2019

View reviewed changes

OlgaMaciaszek added enhancement New feature or request and removed waiting-for-triage labels Oct 16, 2019

OlgaMaciaszek added this to In progress in Hoxton.RC1 via automation Oct 16, 2019

OlgaMaciaszek added this to the 2.1.4.RELEASE milestone Oct 16, 2019

Switch to medium resource class in circleci. (spring-cloud#231)

63a9245

Jaesik Kim and others added 4 commits October 17, 2019 19:18

Adding gzip support for Spring Decoder

2d14e74

Restoring test config

9783219

Merge branch 'spring-decoder-gzip-support' of https://github.com/jski…

d38f04a

…m1991/spring-cloud-openfeign into spring-decoder-gzip-support

Creating extra property for Gzip Decoder

ef2b478

ryanjbaxter requested changes Oct 28, 2019

View reviewed changes

ryanjbaxter modified the milestones: 2.1.4.RELEASE, 2.2.0.RC1 Oct 28, 2019

ryanjbaxter added this to In progress in Hoxton.RC2 via automation Oct 28, 2019

ryanjbaxter removed this from In progress in Hoxton.RC1 Oct 28, 2019

Fixing requested changes and updating doc

bd16746

Missed one change request

a97e099

OlgaMaciaszek requested changes Oct 29, 2019

View reviewed changes

Removing some properties

c94dd6d

ryanjbaxter approved these changes Nov 4, 2019

View reviewed changes

OlgaMaciaszek approved these changes Nov 4, 2019

View reviewed changes

OlgaMaciaszek merged commit bc10583 into spring-cloud:master Nov 4, 2019

Hoxton.RC2 automation moved this from In progress to Done Nov 4, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding gzip support for Spring Decoder #230

Adding gzip support for Spring Decoder #230

jskim1991 commented Oct 16, 2019

pivotal-issuemaster commented Oct 16, 2019

holy12345 commented Oct 16, 2019

OlgaMaciaszek Oct 16, 2019

OlgaMaciaszek Oct 16, 2019

OlgaMaciaszek Oct 16, 2019

OlgaMaciaszek Oct 16, 2019

pivotal-issuemaster commented Oct 16, 2019

OlgaMaciaszek commented Oct 16, 2019

OlgaMaciaszek commented Oct 16, 2019

OlgaMaciaszek commented Oct 16, 2019

jskim1991 commented Oct 16, 2019

jskim1991 commented Oct 17, 2019 •

edited

codecov bot commented Oct 19, 2019 •

edited

jskim1991 commented Oct 19, 2019 •

edited

jskim1991 commented Oct 24, 2019

ryanjbaxter Oct 28, 2019

ryanjbaxter Oct 28, 2019

ryanjbaxter Oct 28, 2019

ryanjbaxter Oct 28, 2019

jskim1991 commented Oct 29, 2019

OlgaMaciaszek left a comment

OlgaMaciaszek Oct 29, 2019

OlgaMaciaszek Oct 29, 2019

OlgaMaciaszek Oct 29, 2019

OlgaMaciaszek Oct 29, 2019

jskim1991 Oct 29, 2019

ryanjbaxter Nov 4, 2019

OlgaMaciaszek Oct 29, 2019

jskim1991 commented Oct 30, 2019

OlgaMaciaszek left a comment

Adding gzip support for Spring Decoder #230

Adding gzip support for Spring Decoder #230

Conversation

jskim1991 commented Oct 16, 2019

pivotal-issuemaster commented Oct 16, 2019

holy12345 commented Oct 16, 2019

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

pivotal-issuemaster commented Oct 16, 2019

OlgaMaciaszek commented Oct 16, 2019

OlgaMaciaszek commented Oct 16, 2019

OlgaMaciaszek commented Oct 16, 2019

jskim1991 commented Oct 16, 2019

jskim1991 commented Oct 17, 2019 • edited

codecov bot commented Oct 19, 2019 • edited

Codecov Report

jskim1991 commented Oct 19, 2019 • edited

jskim1991 commented Oct 24, 2019

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

jskim1991 commented Oct 29, 2019

OlgaMaciaszek 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

Choose a reason for hiding this comment

jskim1991 commented Oct 30, 2019

OlgaMaciaszek left a comment

Choose a reason for hiding this comment

jskim1991 commented Oct 17, 2019 •

edited

codecov bot commented Oct 19, 2019 •

edited

jskim1991 commented Oct 19, 2019 •

edited