GH 3936 : Request attribute for webclient in webflux integration has been added #3950

saxenaj · 2022-11-17T19:36:27Z

No description provided.

artembilan

This is good so far.
Not too much, but having a proper test would be great.

Also, please, consider to add [[x6.0-webflux]] entry into whats-new.adoc in the end.
And mention this new feature in the webflux.adoc.

Thank you!

artembilan · 2022-11-17T19:39:48Z

...java/org/springframework/integration/webflux/config/WebFluxOutboundChannelAdapterParser.java

+
+		if (hasAttibuteVariablesExpression) {
+			builder.addPropertyValue("attributeVariablesExpression",
+					BeanDefinitionBuilder.rootBeanDefinition(ExpressionFactoryBean.class)


See IntegrationNamespaceUtils.createExpressionDefIfAttributeDefined() instead for convenience.

Code has been updated to use IntegrationNamespaceUtils.createExpressionDefIfAttributeDefined()

@saxenaj ,

you know, there is no reason to add this kind of comments if you agree with us: we just simply will see that in the next commit you add to the PR.
Otherwise I got at a bout 10 emails from you instead of just one about a new commit. 😄

artembilan · 2022-11-17T19:50:54Z

.../org/springframework/integration/webflux/outbound/WebFluxRequestExecutingMessageHandler.java

+	@Override
+	protected final void doInit() {
+		BeanFactory beanFactory = getBeanFactory();
+		this.evaluationContext = ExpressionUtils.createStandardEvaluationContext(beanFactory);


You can just have a single line here: doesn't look like there is a reason in extracting beanFactory variable.

beanFactory variable has been removed and code has been updated to single line

artembilan · 2022-11-17T19:53:51Z

...springframework/integration/webflux/outbound/WebFluxRequestExecutingMessageHandlerTests.java

-import static org.assertj.core.api.Assertions.assertThat;
+import reactor.core.publisher.Flux;
+import reactor.core.publisher.Mono;
+import reactor.test.StepVerifier;


This is wrong imports re-order according our new code style.
Please, rebase to the latest main and see src/idea or src/eclipse for editor configs to import into your IDE.

Import sequence has been updated to follow code style

artembilan · 2022-11-17T19:54:12Z

...springframework/integration/webflux/outbound/WebFluxRequestExecutingMessageHandlerTests.java

@@ -214,6 +218,10 @@ void testFluxReply() {
 		reactiveHandler.setExpectedResponseType(String.class);
 		reactiveHandler.setReplyPayloadToFlux(true);

+		reactiveHandler.setAttributeVariablesExpression(new SpelExpressionParser().parseExpression("{name:{first:'Nikola',last:'Tesla'},dob:{day:10,month:'July',year:1856}}"));


We prefer no more than 120 symbols in one code line.

Also, doesn't look like you do some verification for those attributes to propagate.

New test case has been added to check the propagation of attributes. Also code has been changes to keep less than 120 char in one line.

artembilan · 2022-11-17T19:54:56Z

...springframework/integration/webflux/outbound/WebFluxRequestExecutingMessageHandlerTests.java

@@ -376,4 +384,8 @@ void testMaxInMemorySizeExceeded() {
 				.isEqualTo("Exceeded limit on max bytes to buffer : 1");
 	}

+	private void setBeanFactory(WebFluxRequestExecutingMessageHandler handler) {


Not sure why we need this method, if you simply can do that directly in a line of code.

Code has been updated accordingly.

saxenaj · 2022-11-17T20:02:14Z

Thanks for quick turnaround. I will take a look on review comments.

saxenaj · 2022-11-18T06:03:32Z

This is good so far. Not too much, but having a proper test would be great.

Also, please, consider to add [[x6.0-webflux]] entry into whats-new.adoc in the end. And mention this new feature in the webflux.adoc.

Thank you!

New test case has been added to check for request attribute. Also the whats-new.adoc and webflux.adoc has been updated.

artembilan

There is a Checkstyle error in the CI build for this PR.
I'm not sure why you didn't receive a respective email to fix already.
Consider to run ./gradlew clean :spring-integration-webflux:check locally to be sure that you don't have Checkstyle violations and don't introduce new compilation warnings or tests are passing.

.../org/springframework/integration/webflux/outbound/WebFluxRequestExecutingMessageHandler.java

artembilan · 2022-11-18T14:58:41Z

.../org/springframework/integration/webflux/outbound/WebFluxRequestExecutingMessageHandler.java

+
+		if(attributeVariablesExpression != null) {
+			Map<String, Object> attributeMap = evaluateAttributeVariables(requestMessage);
+			requestSpec = requestSpec.attributes(map -> map.putAll(attributeMap));


Let's do some optimization not calling this method if attributeMap.isEmpty()!

artembilan · 2022-11-18T15:02:01Z

...springframework/integration/webflux/outbound/WebFluxRequestExecutingMessageHandlerTests.java

+		reactiveHandler.setOutputChannel(replyChannel);
+		reactiveHandler.setExpectedResponseType(String.class);
+		reactiveHandler.setReplyPayloadToFlux(true);
+		Expression expr = new SpelExpressionParser().parseExpression("{name:{first:'Nikola'}}");


Just for your knowledge: there is a FunctionExpression impl to be able to call as a lamda.
So, this one would be as simple as: Expression expr = new FunctionExpression<Object>(o -> Map.of("first", "Nikola")).
and much faster than SpEL parsing and its evaluation via reflection 😄

artembilan · 2022-11-18T15:08:42Z

src/reference/asciidoc/webflux.adoc

+[[webflux-request-attribute]]
+=== WebFlux Request Attribute
+
+Starting with Version 6, Webflux outbound request can have request attribute which can be specified in `attribute-variables-expression` attribute. This attribute will take an `expression` that should be evaluated in `Map`.


"Version" word not capitalized.
Version number is 6.0.
One sentence per line rule, please: https://asciidoctor.org/docs/asciidoc-recommended-practices/#one-sentence-per-line.

I wouldn't imply an XML config. Better to say WebFluxRequestExecutingMessageHandler and setAttributeVariablesExpression().
The XML configuration is really left in a maintenance mode and better to move away from it, more over with modern trends about native images.

Would be great to have some sample configuration and the reasoning behind this feature.

Request attribute is standard webclient feature which can be used in multiple ways. Sample configuration (https://github.com/saxenaj/spring-integration/blob/GH-3936/spring-integration-webflux/src/test/java/org/springframework/integration/webflux/config/WebFluxOutboundGatewayParserTests-context.xml#L43) and implementation (https://github.com/saxenaj/spring-integration/blob/GH-3936/spring-integration-webflux/src/test/java/org/springframework/integration/webflux/outbound/WebFluxRequestExecutingMessageHandlerTests.java#L391) are there in test cases.

.../org/springframework/integration/webflux/outbound/WebFluxRequestExecutingMessageHandler.java

src/reference/asciidoc/whats-new.adoc

artembilan · 2022-11-18T15:14:30Z

src/reference/asciidoc/whats-new.adoc

+
+[[x6.0-webflux]]
+
+=== Webflux request attribute support


Blank line here between name and section body.

artembilan · 2022-11-18T15:29:35Z

src/reference/asciidoc/whats-new.adoc

+[[x6.0-webflux]]
+
+=== Webflux request attribute support
+Webclient Request attribute support has been added for outbound request.See <<./webflux.adoc#webflux-request-attribute,WebFlux Request Attribute>> for more information.


One sentence per line, please.

saxenaj · 2022-11-18T18:52:13Z

tests

artembilan · 2022-11-18T18:50:12Z

.../org/springframework/integration/webflux/outbound/WebFluxRequestExecutingMessageHandler.java

+	 * Configure expression to evaluate request attribute which will be added to webclient request attribute.
+	 * @param attributeVariablesExpression the expression to evaluate request attribute.
+	 * @since 6.0
+	 * @see WebFluxRequestExecutingMessageHandler#evaluateAttributeVariables(Message)


You are pointing here to private method which is out of end-user interest.
I'd say @see WebClient.RequestBodySpec#attributes

.../org/springframework/integration/webflux/outbound/WebFluxRequestExecutingMessageHandler.java

artembilan · 2022-11-18T18:53:25Z

src/reference/asciidoc/webflux.adoc

@@ -323,4 +323,5 @@ See <<./http.adoc#http-header-mapping,HTTP Header Mappings>> for more possible o
 [[webflux-request-attribute]]
 === WebFlux Request Attribute


Attributes

artembilan · 2022-11-18T18:53:30Z

src/reference/asciidoc/whats-new.adoc

 === Webflux request attribute support
-Webclient Request attribute support has been added for outbound request.See <<./webflux.adoc#webflux-request-attribute,WebFlux Request Attribute>> for more information.
+
+Webclient Request attribute support has been added for outbound request.


attributes and probably better to mention a WebFluxRequestExecutingMessageHandler.
That "outbound request" is too vague from Spring Integration context.

artembilan · 2022-11-18T18:53:38Z

src/reference/asciidoc/whats-new.adoc

-Webclient Request attribute support has been added for outbound request.See <<./webflux.adoc#webflux-request-attribute,WebFlux Request Attribute>> for more information.
+
+Webclient Request attribute support has been added for outbound request.
+See <<./webflux.adoc#webflux-request-attribute,WebFlux Request Attribute>> for more information.


Attributes

.../org/springframework/integration/webflux/outbound/WebFluxRequestExecutingMessageHandler.java

artembilan · 2022-11-18T19:13:44Z

.../org/springframework/integration/webflux/outbound/WebFluxRequestExecutingMessageHandler.java

+
+		if (this.attributeVariablesExpression != null) {
+			Map<String, Object> attributeMap = evaluateAttributeVariables(requestMessage);
+			if (attributeMap != null && !attributeMap.isEmpty()) {


!CollectionUtils.isEmpty(attributeMap)

artembilan · 2022-11-18T19:21:06Z

src/reference/asciidoc/webflux.adoc

+=== WebFlux Request Attributes
+
+Starting with version 6.0, Webflux outbound request `WebFluxRequestExecutingMessageHandler` can have request attributes which can be specified in `setAttributeVariablesExpression()`.
+This will take an `expression` that should be evaluated in `Map`.


I still would like to see a general sentence for reasoning behind these attributes.
Doesn't look like this is a common feature since we haven't bumped to its missing support for since version 5.0.
Probably the security would be a great sample to carry on some info from message to the request where downstream filter would expect an attribute to be able to authorize a principal or so.

My point is that you need to convince me somehow in using this new feature from the doc.
No one is going to look into those tests in the project.

how about this?

Starting with version 6.0, Webflux outbound request WebFluxRequestExecutingMessageHandler can have request attributes which can be specified in setAttributeVariablesExpression().
This will take an expression that should be evaluated in Map.
This will be helpful if an information in form of key-value object needs be passed from Message to request and downstream filter will get access to this attribute for further processing.

Sounds good!
👍

artembilan

Pulling locally for the final review, clean up and merge.

artembilan · 2022-11-18T21:34:56Z

Merged as ddddd1d after some refactoring.

@saxenaj ,

thank you very much for the contribution; looking forward for more!

artembilan · 2022-11-28T17:18:59Z

Was missed to be closed after merge into main.

saxenaj and others added 2 commits November 17, 2022 13:20

Add webclient request attribute

e42f236

Add author name

7483e14

saxenaj mentioned this pull request Nov 17, 2022

Consider adding request attribute support for webclient in webflux integration #3936

Closed

artembilan requested changes Nov 17, 2022

View reviewed changes

saxenaj marked this pull request as draft November 18, 2022 03:58

Changes for review comments

4209c2e

saxenaj marked this pull request as ready for review November 18, 2022 05:55

saxenaj requested a review from artembilan November 18, 2022 06:04

artembilan requested changes Nov 18, 2022

View reviewed changes

Changes for review comments

8237a99

saxenaj closed this Nov 18, 2022

saxenaj reopened this Nov 18, 2022

artembilan requested changes Nov 18, 2022

View reviewed changes

Changes for review comments

8d051e3

artembilan requested changes Nov 18, 2022

View reviewed changes

Changes for review comments

c60c3b8

saxenaj requested a review from artembilan November 18, 2022 20:15

artembilan approved these changes Nov 18, 2022

View reviewed changes

artembilan closed this Nov 28, 2022

		@@ -323,4 +323,5 @@ See <<./http.adoc#http-header-mapping,HTTP Header Mappings>> for more possible o
		[[webflux-request-attribute]]
		=== WebFlux Request Attribute

GH 3936 : Request attribute for webclient in webflux integration has been added #3950

GH 3936 : Request attribute for webclient in webflux integration has been added #3950

Uh oh!

Conversation

saxenaj commented Nov 17, 2022

Uh oh!

artembilan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saxenaj Nov 18, 2022 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saxenaj commented Nov 17, 2022

Uh oh!

saxenaj commented Nov 18, 2022

Uh oh!

artembilan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saxenaj commented Nov 18, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

artembilan left a comment

Choose a reason for hiding this comment

saxenaj Nov 18, 2022 •

edited

Loading