Ignore unresolved property placeholders in `url`

[elkkhan]

trafficstars

fixes https://github.com/spring-cloud/spring-cloud-openfeign/issues/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]

What exception is thrown if the placeholder is not ignored?

[spencergibb]

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

[elkkhan]

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]

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

[elkkhan]

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]

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

[OlgaMaciaszek]

@elkkhan Thanks for creating this PR. I agree with @spencergibb that it would be preferable to get an early exception. At the same time, I agree with you that consistency is important. However, we should unify it towards the more desirable outcome. Given the historical handling of SpEl, we cannot introduce this change now (in a non-major release), but you could create a PR and we'd mark it as one for a major release and merge it whenever we start working on one. Feel free to create it. I'm going to close this one.