Skip to content

Encoding in AbstractHttpRequestExecutingMessageHandler.java is now done when flag is set#2929

Merged
artembilan merged 3 commits into
spring-projects:masterfrom
ezylot:patch-1
May 16, 2019
Merged

Encoding in AbstractHttpRequestExecutingMessageHandler.java is now done when flag is set#2929
artembilan merged 3 commits into
spring-projects:masterfrom
ezylot:patch-1

Conversation

@ezylot
Copy link
Copy Markdown
Contributor

@ezylot ezylot commented May 14, 2019

Before we did not actually encode the URI. You can see this in the screenshot below, where the String "häuser" is not actually encoded to the correct percentage encoding.

withoutEncode

When we first encode it and then call the .toUri() we can see that the "häuser" is now propoerly encoded to "h%C3%A4user"

withEncode

@pivotal-issuemaster
Copy link
Copy Markdown

@ezylot Please sign the Contributor License Agreement!

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

See the FAQ for frequently asked questions.

@pivotal-issuemaster
Copy link
Copy Markdown

@ezylot Thank you for signing the Contributor License Agreement!

@ezylot ezylot changed the title Encoding is now done when flag is set Encoding in AbstractHttpRequestExecutingMessageHandler.java is now done when flag is set May 14, 2019
Copy link
Copy Markdown
Member

@artembilan artembilan left a comment

Choose a reason for hiding this comment

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

Please, add your name to the @author list and modify, please, HttpRequestExecutingMessageHandlerTests.testUriExpression() to reflect your change.
This way we definitely going to have a test coverage.

Thank you!

@artembilan artembilan added this to the 5.1.6 milestone May 14, 2019
@ezylot
Copy link
Copy Markdown
Contributor Author

ezylot commented May 14, 2019

I added myself as an author and added 2 tests that check this specific problem

Copy link
Copy Markdown
Member

@artembilan artembilan left a comment

Choose a reason for hiding this comment

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

Please, fix Checkstyle violation

import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import java.util.*;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

No asterisk imports at all, please.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Oh boy, did not check that. Fixed.

Copy link
Copy Markdown
Member

@artembilan artembilan left a comment

Choose a reason for hiding this comment

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

LGTM

Will merge after green Travis report.

@artembilan
Copy link
Copy Markdown
Member

There are still a couple Checkstyle violations:

[ant:checkstyle] [ERROR] /home/travis/build/spring-projects/spring-integration/spring-integration-http/src/test/java/org/springframework/integration/http/outbound/HttpRequestExecutingMessageHandlerTests.java:770:45: '}' at column 31 should be alone on a line. [RightCurly]
[ant:checkstyle] [ERROR] /home/travis/build/spring-projects/spring-integration/spring-integration-http/src/test/java/org/springframework/integration/http/outbound/HttpRequestExecutingMessageHandlerTests.java:792:45: '}' at column 31 should be alone on a line. [RightCurly]

@ezylot
Copy link
Copy Markdown
Contributor Author

ezylot commented May 14, 2019

Damnit, give me a second ^^

@ezylot
Copy link
Copy Markdown
Contributor Author

ezylot commented May 16, 2019

should be good to go, i think ;)

@artembilan artembilan merged commit 87169bc into spring-projects:master May 16, 2019
@artembilan
Copy link
Copy Markdown
Member

Sorry, have missed your fresh commit somehow...

Now it is merged and I'm going to cherry-pick it to 5.1.x.

Thank you for contribution; looking forward for more!

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.

3 participants