Add initialization of hints to request/response modification. #299

re6exp · 2018-04-24T18:26:58Z

No description provided.

pivotal-issuemaster · 2018-04-24T18:26:59Z

@re6exp Please sign the Contributor License Agreement!

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

See the FAQ for frequently asked questions.

spencergibb · 2018-04-24T18:30:29Z

...ay-core/src/main/java/org/springframework/cloud/gateway/route/builder/GatewayFilterSpec.java

@@ -123,14 +124,14 @@ public GatewayFilterSpec hystrix(Consumer<HystrixGatewayFilterFactory.Config> co
 		return filter(factory.apply(this.routeBuilder.getId(), configConsumer));
 	}

-	public <T, R> GatewayFilterSpec modifyRequestBody(Class<T> inClass, Class<R> outClass, RewriteFunction<T, R> rewriteFunction) {
+	public <T, R> GatewayFilterSpec modifyRequestBody(Class<T> inClass, Class<R> outClass, RewriteFunction<T, R> rewriteFunction, Map<String, Object> inHints, Map<String, Object> outHints) {


Since RC1 is already public, let's leave the original methods and either overload, or create a spec.

Don't worry about RC1, just keep the originals and either overload or create a spec similar to what is in your branch.

So, I'll overload methods with specs. Is it OK? If yes, tomorrow I'll send you PR to master branch, keeping existed signatures as they were before.

OK, tomorrow I am going to review what you write here... It's late (Moscow time 10 p.m.), I'm pretty tired and I'm going to go to bed.

spencergibb · 2018-04-24T19:13:21Z

Tomorrow is fine

…xisting signature.

codecov-io · 2018-04-25T08:57:50Z

Codecov Report

Merging #299 into master will decrease coverage by 9.29%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##           master     #299     +/-   ##
=========================================
- Coverage   71.73%   62.43%   -9.3%     
=========================================
  Files         119        8    -111     
  Lines        3216      410   -2806     
  Branches      231       34    -197     
=========================================
- Hits         2307      256   -2051     
+ Misses        783      130    -653     
+ Partials      126       24    -102

Impacted Files	Coverage Δ
...framework/cloud/gateway/route/builder/UriSpec.java
...ud/gateway/filter/AdaptCachedBodyGlobalFilter.java
.../gateway/config/GatewayRedisAutoConfiguration.java
...ter/factory/rewrite/HttpMessageWriterResponse.java
...cloud/gateway/filter/NettyWriteResponseFilter.java
...filter/factory/RedirectToGatewayFilterFactory.java
.../gateway/handler/RoutePredicateHandlerMapping.java
...way/filter/ratelimit/PrincipalNameKeyResolver.java
...CloudFoundryRouteServiceRoutePredicateFactory.java
...y/filter/factory/AbstractGatewayFilterFactory.java
... and 97 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7a1bbf0...98394ab. Read the comment docs.

pivotal-issuemaster · 2018-04-25T12:50:30Z

@re6exp Thank you for signing the Contributor License Agreement!

re6exp · 2018-04-27T09:36:15Z

@spencergibb
Hm... do not merge, I've found some errors while I've been adding tests. Now I'm fixing them.
They are:

no MediaType to set to out results both in request/response, they are only being got from headers;
request tests do not work completely because of throw new IllegalStateException( "SyncInvocableHandlerMethod should have completed synchronously."); in RewriteUtils#process();
see also issue ModifyResponse doesn't modify Content-Length Header #304 ;

re6exp · 2018-04-28T15:01:27Z

@spencergibb

I've fixed some issues with response body modification. Tests covering response body modification were copied from my old branch with some modification. They work. With Spring Web 5.0.6 BUILD-SNAPSHOT all of them run successfully including SSE.

BUT tests for request body modification can't even run because of IllegalStateException in RewriteUtils. I have no idea how to fix it.

…galStateException in RewriteUtils.

spencergibb · 2018-05-17T14:35:37Z

rebuilding

spencergibb

I few comments

spencergibb · 2018-05-17T14:37:11Z

spring-cloud-gateway-core/pom.xml

+		<dependency>
+			<groupId>org.springframework</groupId>
+			<artifactId>spring-web</artifactId>
+			<version>5.0.6.BUILD-SNAPSHOT</version>


This is bad. This should already be a dependency.

Prior version doesn't work. I wrote comments here and in old issues.

rebase, and you won't need this anymore.

Glad to hear that!

Removed (by my side - not committed yet).

spencergibb · 2018-05-17T14:37:37Z

...c/main/java/org/springframework/cloud/gateway/config/WebFluxGatewayConfigurationSupport.java

+ *
+ */
+@Configuration
+@EnableWebFlux


This is autoconfigured by boot.

in other words, it shouldn't be needed here, just WebFluxConfigurer

spencergibb · 2018-05-17T14:38:40Z

...ingframework/cloud/gateway/filter/factory/rewrite/ModifyRequestBodyGatewayFilterFactory.java

@@ -37,7 +40,7 @@

 import static org.springframework.cloud.gateway.filter.factory.rewrite.RewriteUtils.process;

-public class ModifyRequestBodyGatewayFilterFactory
+public class ModifyRequestBodyGatewayFilterFactory<T, R>


I don't think these generics make sense here.

When user writes code it helps him.

As I understand, old code will work too.

spencergibb · 2018-05-17T15:14:59Z

Build is timing out

Running org.springframework.cloud.gateway.filter.body.RewriteRequestBodyFilterTest

re6exp · 2018-05-17T15:17:45Z

Hm, it worked... 22 days ago.

UPD: code worked - I add local snapshot to my app, it works.

re6exp · 2018-05-17T15:23:02Z

The main problem for me was request modification tests. They were being terminated abnormally.
Hints are working in real code of my app.

So, Spencer, should I change smth you wrote about, and commit one more time, or you plan to do by yourself?

re6exp · 2018-05-17T15:30:09Z

I can do it tomorrow.

re6exp · 2018-05-18T07:29:44Z

Tests for response body modification run perfectly - thx to Rossen, he fixed problem with sse serialization in v. 5.0.6 which has been released some time ago.

But ModifyRequestBodyTest doesn't work - neither before nor now, as I wrote in comments before. Because I don't use request body modification in my app I can't confirm this code does work really. Tests do not. These tests worked on my original branch, with old API - this API was bad I know but it worked, tests were passed. After I change API to the new, I always get an exception throwed in RewriteUtils while testing request body modification. I have no idea how to fix it.

Time out while running ModifyRequestBodyTest, the cause is:

java.lang.IllegalStateException: SyncInvocableHandlerMethod should have completed synchronously.
	at org.springframework.cloud.gateway.filter.factory.rewrite.RewriteUtils.process(RewriteUtils.java:49) ~[classes/:na]
	at org.springframework.cloud.gateway.filter.factory.rewrite.ModifyRequestBodyGatewayFilterFactory.lambda$0(ModifyRequestBodyGatewayFilterFactory.java:66) ~[classes/:na]
	at org.springframework.cloud.gateway.filter.OrderedGatewayFilter.filter(OrderedGatewayFilter.java:44) ~[classes/:na]
	at org.springframework.cloud.gateway.handler.FilteringWebHandler$DefaultGatewayFilterChain.lambda$0(FilteringWebHandler.java:115) ~[classes/:na]
	at reactor.core.publisher.MonoDefer.subscribe(MonoDefer.java:44) ~[reactor-core-3.1.7.RELEASE.jar:3.1.7.RELEASE]
	at reactor.core.publisher.MonoDefer.subscribe(MonoDefer.java:52) ~[reactor-core-3.1.7.RELEASE.jar:3.1.7.RELEASE]
	at reactor.core.publisher.Mono.subscribe(Mono.java:3080) ~[reactor-core-3.1.7.RELEASE.jar:3.1.7.RELEASE]
	at reactor.core.publisher.MonoIgnoreThen$ThenIgnoreMain.drain(MonoIgnoreThen.java:167) ~[reactor-core-3.1.7.RELEASE.jar:3.1.7.RELEASE]

This blocks any changes I made in this PR.

rstoyanchev · 2018-05-18T14:26:21Z

I've only had a very brief look, but RewriteUtils#process seems to be written in a way that assumes no blocking can occur. When I look at places where it's used, that's not the case, like here reading from the body.

The error message is referring to a Spring Framework class, the SyncInvocableHandlerMethod, which is not relevant or used here AFAICS, but that class has a similar check and it's guaranteed to not block because it can only be configured with synchronous argument resolvers.

…ious commit

re6exp · 2018-05-19T07:51:42Z

So, this leads to when I run tests, they can't be successfully finished, do I understand right?

Reading the body cause to blocking operation, which is disallowed (so isTerminated() check returns false) and leads junit tests to crash... Do I understand your comment correct?

There is an example of a gateway application, which somehow can be executed and uses httpbin.org-like backend. I did not run it. May be it works, OK, but the tests do not. :(

I can assume it my fault, but this code works with another API and should, in principle, working here too.

rstoyanchev · 2018-05-21T01:29:40Z

Reading the body cause to blocking operation, which is disallowed (so isTerminated() check returns false) and leads junit tests to crash... Do I understand your comment correct?

The body may not be read immediately, so the isTerminated check makes no sense. It cannot be guaranteed to return true.

re6exp · 2018-05-21T07:49:47Z

And in case of using junits it crashs. As I guessed...
Thank you, Rossen!

Add initialization of hints to request/response modification.

b7b82ec

spencergibb requested changes Apr 24, 2018

View reviewed changes

Add spec hints support to request/response modification, keeping an e…

d376074

…xisting signature.

re6exp added 2 commits April 28, 2018 18:29

Add out MediaType set method. Some minor fixes.

aefb299

Add request body modification tests. They do not work because of Ille…

1073a4a

…galStateException in RewriteUtils.

dsyer force-pushed the master branch from 0583a38 to 1656bbc Compare May 2, 2018 13:16

re6exp mentioned this pull request May 10, 2018

Reading request body in filter produces an IllegalStateException #135

Closed

re6exp mentioned this pull request May 17, 2018

Hints in body modifying #325

Closed

spencergibb requested changes May 17, 2018

View reviewed changes

re6exp added 3 commits May 18, 2018 07:22

Renamed tests units - body modification

353fcf6

Merge remote-tracking branch 'upstream/master'

58f0249

Remove annotations in WebFluxGatewayConfigurationSupport

3c2b1ff

Remove cache() in ModifyRequestBodyGatewayFilterFactory added in prev…

98394ab

…ious commit

spencergibb mentioned this pull request May 21, 2018

WIP: Updates rewrite filters to use framework web function classes. #329

Merged

re6exp mentioned this pull request Jun 9, 2018

Add more filters #10

Closed

22 tasks

re6exp closed this Jun 21, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add initialization of hints to request/response modification. #299

Add initialization of hints to request/response modification. #299

re6exp commented Apr 24, 2018

pivotal-issuemaster commented Apr 24, 2018

spencergibb Apr 24, 2018

spencergibb Apr 24, 2018

re6exp Apr 24, 2018

spencergibb commented Apr 24, 2018

codecov-io commented Apr 25, 2018 •

edited

pivotal-issuemaster commented Apr 25, 2018

re6exp commented Apr 27, 2018 •

edited

re6exp commented Apr 28, 2018 •

edited

spencergibb commented May 17, 2018

spencergibb left a comment

spencergibb May 17, 2018

re6exp May 17, 2018

spencergibb May 17, 2018

re6exp May 17, 2018

re6exp May 18, 2018

spencergibb May 17, 2018

spencergibb May 17, 2018

re6exp May 17, 2018

re6exp May 18, 2018

spencergibb May 17, 2018

re6exp May 17, 2018

re6exp May 17, 2018

spencergibb commented May 17, 2018

re6exp commented May 17, 2018 •

edited

re6exp commented May 17, 2018

re6exp commented May 17, 2018

re6exp commented May 18, 2018 •

edited

rstoyanchev commented May 18, 2018

re6exp commented May 19, 2018

rstoyanchev commented May 21, 2018

re6exp commented May 21, 2018

Add initialization of hints to request/response modification. #299

Add initialization of hints to request/response modification. #299

Conversation

re6exp commented Apr 24, 2018

pivotal-issuemaster commented Apr 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spencergibb commented Apr 24, 2018

codecov-io commented Apr 25, 2018 • edited

Codecov Report

pivotal-issuemaster commented Apr 25, 2018

re6exp commented Apr 27, 2018 • edited

re6exp commented Apr 28, 2018 • edited

spencergibb commented May 17, 2018

spencergibb 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

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

spencergibb commented May 17, 2018

re6exp commented May 17, 2018 • edited

re6exp commented May 17, 2018

re6exp commented May 17, 2018

re6exp commented May 18, 2018 • edited

rstoyanchev commented May 18, 2018

re6exp commented May 19, 2018

rstoyanchev commented May 21, 2018

re6exp commented May 21, 2018

codecov-io commented Apr 25, 2018 •

edited

re6exp commented Apr 27, 2018 •

edited

re6exp commented Apr 28, 2018 •

edited

re6exp commented May 17, 2018 •

edited

re6exp commented May 18, 2018 •

edited