Skip to content
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.

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

HeartBeat request is not properly url encoded #4121

Open
NewAgeCZ opened this issue Sep 29, 2022 · 3 comments
Open

HeartBeat request is not properly url encoded #4121

NewAgeCZ opened this issue Sep 29, 2022 · 3 comments

Comments

@NewAgeCZ
Copy link
Contributor

Hi all,

Describe the bug
I am migrating an application that uses Jersey 1 to RestTemplate.
During migration I have encountered heart beat requests returning 400 Bad request status code and upon further debugging found out, that the sent request is not complete.
The application uses instanceId in format of service#version and that is the problem.

Using spring-cloud-netflix-eureka-client 3.1.2 I tested this scenario with both Jersey 1 and 2 passing.

Sample
I forked the repo and modified testcase AbstractEurekaHttpClientTest adding some characters that have to be escaped into instanceId.

testSendHeartBeat now failed for RestTemplate with 400 status code (as expected), however WebClient test still passed with 200 although logs showed 400 returned as well (no idea why, I did not proceed down the rabbit hole).

I replaced String concatenation in both WebClient and RestTemplate with UriBuilder and UriComponentsBuilder where deemed appropriate.

Feel free to check it out: https://github.com/NewAgeCZ/spring-cloud-netflix/commits/fix/urlencoding

Best regards!

@spencergibb
Copy link
Member

PRs welcome

NewAgeCZ added a commit to NewAgeCZ/spring-cloud-netflix that referenced this issue Mar 5, 2023
NewAgeCZ added a commit to NewAgeCZ/spring-cloud-netflix that referenced this issue Mar 5, 2023
NewAgeCZ added a commit to NewAgeCZ/spring-cloud-netflix that referenced this issue Mar 5, 2023
NewAgeCZ added a commit to NewAgeCZ/spring-cloud-netflix that referenced this issue Mar 5, 2023
NewAgeCZ added a commit to NewAgeCZ/spring-cloud-netflix that referenced this issue Mar 5, 2023
NewAgeCZ added a commit to NewAgeCZ/spring-cloud-netflix that referenced this issue Mar 5, 2023
NewAgeCZ added a commit to NewAgeCZ/spring-cloud-netflix that referenced this issue Mar 5, 2023
NewAgeCZ added a commit to NewAgeCZ/spring-cloud-netflix that referenced this issue Mar 5, 2023
@ZIRAKrezovic
Copy link
Contributor

Hi @spencergibb. PR has been available for some time now #4163

I am happy to jump in and assist if @NewAgeCZ does not have time at present.

@NewAgeCZ
Copy link
Contributor Author

NewAgeCZ commented Apr 3, 2024

Hi @ZIRAKrezovic,
I have been waiting for the PR review to come... almost forgot about this fix. If the code changes are approved, I can handle rebasing onto lastest upstream.

Lets wait for the review and I will notice you afterwards if my time schedule would not allow me to react in reasonable time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants