Skip to content

GH-3154: Support UriBuilderFactory.EncodingMode #3162

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 5 commits into from
Jan 30, 2020
Merged

GH-3154: Support UriBuilderFactory.EncodingMode #3162

merged 5 commits into from
Jan 30, 2020

Conversation

artembilan
Copy link
Member

Fixes #3154

Spring Framework now provides a DefaultUriBuilderFactory.EncodingMode
for encoding URIs in the RestTemplate before and after uri template
enrichment with uri variables.
Therefore encodeUri and manual uri variables substitution is not necessary
in Spring Integration HTTP components

  • Deprecate AbstractHttpRequestExecutingMessageHandler.encodeUri in favor of
    DefaultUriBuilderFactory.EncodingMode and respective configuration
    on the RestTemplate in HTTP module and WebClient in WebFlux module

@artembilan
spring-projectsGH-3154: Support UriBuilderFactory.EncodingMode
473bbb4 
Fixes spring-projects#3154

Spring Framework now provides a `DefaultUriBuilderFactory.EncodingMode`
for encoding URIs in the `RestTemplate` before and after uri template
enrichment with uri variables.
Therefore `encodeUri` and manual uri variables substitution is not necessary
in Spring Integration HTTP components

* Deprecate `AbstractHttpRequestExecutingMessageHandler.encodeUri` in favor of
`DefaultUriBuilderFactory.EncodingMode` and respective configuration
on the `RestTemplate` in HTTP module and `WebClient` in WebFlux module
@artembilan artembilan added this to the 5.3 M2 milestone Jan 30, 2020
@artembilan artembilan changed the title GH-3154: Support UriBuilderFactory.EncodingMode [DO NOT MERGE YET] GH-3154: Support UriBuilderFactory.EncodingMode Jan 30, 2020
@artembilan
Copy link
Member Author

TODO:

  • The same support for WebFlux module
  • Docs
  • Migration Guide
  • Clean up affected test cases for code style and JUnit 5

Will address them lately, but you can review now.

Thanks

garyrussell
garyrussell suggested changes Jan 30, 2020
View reviewed changes
Copy link
Contributor

@garyrussell garyrussell left a comment

Choose a reason for hiding this comment

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

A few issues/questions.

...rg/springframework/integration/http/outbound/AbstractHttpRequestExecutingMessageHandler.java Outdated
@@ -118,6 +115,7 @@
public AbstractHttpRequestExecutingMessageHandler(Expression uriExpression) {
Assert.notNull(uriExpression, "URI Expression is required");
this.uriExpression = uriExpression;
this.uriFactory.setEncodingMode(DefaultUriBuilderFactory.EncodingMode.TEMPLATE_AND_VALUES);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this needed? This is the default.

...rg/springframework/integration/http/outbound/AbstractHttpRequestExecutingMessageHandler.java Outdated
@@ -85,6 +82,8 @@
private static final List<HttpMethod> NO_BODY_HTTP_METHODS =
Arrays.asList(HttpMethod.GET, HttpMethod.HEAD, HttpMethod.TRACE);

DefaultUriBuilderFactory uriFactory = new DefaultUriBuilderFactory();
Copy link
Contributor

Choose a reason for hiding this comment

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

Why package visibility?

Why not UriBuilderFactory?

Copy link
Member Author

Choose a reason for hiding this comment

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

The UriBuilderFactory doesn't have a setEncodingMode()

Copy link
Contributor

Choose a reason for hiding this comment

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

and package?

...rg/springframework/integration/http/outbound/AbstractHttpRequestExecutingMessageHandler.java
* @param encodingMode the mode to use for uri encoding
* @since 5.3
*/
public void setEncodingMode(DefaultUriBuilderFactory.EncodingMode encodingMode) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not setUriBuilderFactory ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Right. I'm going to document that for more complex scenarios it would be better to configure an external RestTemplate and already use its setUriTemplateHandler().

I would even go with all the RestTempalte options as an external reference. I'm not a fun of mutating internal objects.
But that would be too drastic even in the point release...

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, makes sense; proceed with the remaining work 😄

@artembilan
* Really populate uriFactory into an internal RestTemplate
4002501 
* Ensure in tests that `encoding-mode` is populated properly into an internal `RestTemplate`
* Clean up affected HTTP tests for AssertJ and JUnit 5
garyrussell
garyrussell suggested changes Jan 30, 2020
View reviewed changes
.../java/org/springframework/integration/http/config/HttpOutboundChannelAdapterParserTests.java Outdated
DirectFieldAccessor handlerAccessor = new DirectFieldAccessor(handler);
assertThat(handlerAccessor.getPropertyValue("expectReply")).isEqualTo(false);
assertThat(endpointAccessor.getPropertyValue("inputChannel"))
.isEqualTo(this.applicationContext.getBean("requests"));
assertThat(handlerAccessor.getPropertyValue("outputChannel")).isNull();
DirectFieldAccessor templateAccessor = new DirectFieldAccessor(handlerAccessor.getPropertyValue("restTemplate"));
DirectFieldAccessor templateAccessor = new DirectFieldAccessor(handlerAccessor.getPropertyValue("restTemplate"
));
Copy link
Contributor

Choose a reason for hiding this comment

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

Unusual formatting.

.../java/org/springframework/integration/http/config/HttpOutboundChannelAdapterParserTests.java Outdated
DirectFieldAccessor handlerAccessor = new DirectFieldAccessor(handler);
assertThat(handlerAccessor.getPropertyValue("expectReply")).isEqualTo(false);
assertThat(endpointAccessor.getPropertyValue("inputChannel"))
.isEqualTo(this.applicationContext.getBean("requests"));
assertThat(handlerAccessor.getPropertyValue("outputChannel")).isNull();
DirectFieldAccessor templateAccessor = new DirectFieldAccessor(handlerAccessor.getPropertyValue("restTemplate"));
DirectFieldAccessor templateAccessor = new DirectFieldAccessor(handlerAccessor.getPropertyValue("restTemplate"
));
Copy link
Contributor

Choose a reason for hiding this comment

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

Here too.

.../java/org/springframework/integration/http/config/HttpOutboundChannelAdapterParserTests.java Outdated
DirectFieldAccessor handlerAccessor = new DirectFieldAccessor(handler);
assertThat(handlerAccessor.getPropertyValue("expectReply")).isEqualTo(false);
assertThat(endpointAccessor.getPropertyValue("inputChannel"))
.isEqualTo(this.applicationContext.getBean("requests"));
assertThat(handlerAccessor.getPropertyValue("outputChannel")).isNull();
DirectFieldAccessor templateAccessor = new DirectFieldAccessor(handlerAccessor.getPropertyValue("restTemplate"));
DirectFieldAccessor templateAccessor = new DirectFieldAccessor(handlerAccessor.getPropertyValue("restTemplate"
));
Copy link
Contributor

Choose a reason for hiding this comment

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

And here

@artembilan
* Clean up formatting
615c36d
@artembilan
* Apply fix for WebFlux module
6a30f1c 
* Add docs for new `encoding-mode` option
@artembilan artembilan changed the title [DO NOT MERGE YET] GH-3154: Support UriBuilderFactory.EncodingMode GH-3154: Support UriBuilderFactory.EncodingMode Jan 30, 2020
@garyrussell
Copy link
Contributor

Task :spring-integration-http:checkstyleTest FAILED
[ant:checkstyle] [ERROR] /home/travis/build/spring-projects/spring-integration/spring-integration-http/src/test/java/org/springframework/integration/http/config/HttpOutboundChannelAdapterParserTests.java:22:8: Unused import - java.nio.charset.Charset. [UnusedImports]

@garyrussell garyrussell merged commit 89d86e1 into spring-projects:master Jan 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@garyrussell garyrussell garyrussell approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
5.3 M2
Development

Successfully merging this pull request may close these issues.

URI Is Being Generated With URI Variables Before RestTemplate Does It
2 participants
@artembilan @garyrussell