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

Ignore unresolved property placeholders in url #1022

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

elkkhan
Copy link

@elkkhan elkkhan commented Apr 17, 2024

fixes #1023

FeignClientRegistar seems to purposefully ignore unresolved URLs that are SpEL but doesn't do so for property placeholders like ${placeholder}.
For ignored SpEL strings, the input string is returned, but for placeholders, it tries to parse it into a URL object.
Up until JDK 20, this would be OK and would not fail - and unresolved placeholder URL would probably fail somewhere later down the flow. This is because the underlying implementation of the URL class doesn't actually try to parse the host until unless the URL.openConnection() is called, which is not called in this method.

Since JDK 20, this behaviour has changed and the underlying implementation of the URL class now parses the host eagerly without waiting for a URL.openConnection() call.

This was discovered when our tests that use @JsonTest started failing once we upgraded to JDK 20+

@spencergibb
Copy link
Member

What exception is thrown if the placeholder is not ignored?

@spencergibb
Copy link
Member

It doesn't ignore them, it just doesn't prepend http://

@elkkhan
Copy link
Author

elkkhan commented Apr 17, 2024

hi @spencergibb,

It doesn't ignore them, it just doesn't prepend http://

Yes, what I meant by ignoring is just that it early returns the input value instead of letting it go through below. so as a consequence it throws an IllegalArgumentException upon catching a MalformedURLException

if (!url.contains("://")) {
    url = "http://" + url;
}
try {
    new URL(url);
}
catch (MalformedURLException e) {
    throw new IllegalArgumentException(url + " is malformed", e);
}

@spencergibb
Copy link
Member

Why isn't that preferred, to error early rather than later?

@elkkhan
Copy link
Author

elkkhan commented Apr 17, 2024

Why isn't that preferred, to error early rather than later?

I actually would prefer that too but I would expect the behaviour to be consistent between SpEL and placeholders.
I chose to let placeholders through to avoid breaking existing code which would be a greater risk if we also let SpEL fail early. I'm not too sure about the volumes here though, as in how realistic/often is it for unresolved URLs to get to that method in the first place

@elkkhan
Copy link
Author

elkkhan commented May 22, 2024

hey @spencergibb @OlgaMaciaszek,
just checking to see if you have any further thoughts on this

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.

FeignClientsRegistar fails when trying to parse unresolved property placeholders in JDK 20+
3 participants