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

Provide native Java 11 HTTP client versions of FormValidation#URLCheck methods #7508

Merged
merged 3 commits into from
Dec 12, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions core/src/main/java/hudson/ProxyConfiguration.java
Original file line number Diff line number Diff line change
Expand Up @@ -357,7 +357,7 @@ public static InputStream getInputStream(URL url) throws IOException {
/**
* Return a new {@link HttpClient} with Jenkins-specific default settings.
*
* <p>Equivalent to {@code newHttpClientBuilder().build()}.
* <p>Equivalent to {@code newHttpClientBuilder().followRedirects(HttpClient.Redirect.NORMAL).build()}.
*
* <p>The Jenkins-specific default settings include a proxy server and proxy authentication (as
* configured by {@link ProxyConfiguration}) and a connection timeout (as configured by {@link
Expand All @@ -367,7 +367,7 @@ public static InputStream getInputStream(URL url) throws IOException {
* @since TODO
*/
public static HttpClient newHttpClient() {
return newHttpClientBuilder().build();
return newHttpClientBuilder().followRedirects(HttpClient.Redirect.NORMAL).build();
Copy link
Member Author

Choose a reason for hiding this comment

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

Unrelated change while I was here: while testing with URLs like https://app.assembla.com/login that have a redirect, I noticed that the old HttpURLConnection API automatically followed redirects but the new Java 11 HTTP client API was not. To ease migration I figured it would be best to follow redirects by default. I could not think of a case where you would not want to follow them, and it matches the behavior of the old API. Of course anyone who wants to opt out can always do so by creating a custom builder without this option (something that was not possible with the old API).

}

/**
Expand Down
59 changes: 57 additions & 2 deletions core/src/main/java/hudson/util/FormValidation.java
Original file line number Diff line number Diff line change
Expand Up @@ -44,14 +44,19 @@
import java.io.IOException;
import java.io.InputStreamReader;
import java.lang.reflect.Method;
import java.net.HttpURLConnection;
import java.net.URI;
import java.net.URL;
import java.net.URLConnection;
import java.net.http.HttpClient;
import java.net.http.HttpRequest;
import java.nio.charset.StandardCharsets;
import java.util.ArrayList;
import java.util.Base64;
import java.util.Collection;
import java.util.List;
import java.util.Locale;
import java.util.stream.Stream;
import javax.servlet.ServletException;
import jenkins.model.Jenkins;
import jenkins.util.SystemProperties;
Expand Down Expand Up @@ -461,10 +466,46 @@ public static FormValidation validateBase64(String value, boolean allowWhitespac
* This allows the check method to call various utility methods in a concise syntax.
*/
public abstract static class URLCheck {
/**
* Open the given URI and read text content from it. This method honors the Content-type
* header.
*
* @throws IOException if the URI scheme is not supported, the connection was interrupted,
* or the response was an error
* @since TODO
*/
protected Stream<String> open(URI uri) throws IOException {
HttpClient httpClient = ProxyConfiguration.newHttpClient();
HttpRequest httpRequest;
try {
httpRequest = ProxyConfiguration.newHttpRequestBuilder(uri).GET().build();
} catch (IllegalArgumentException e) {
throw new IOException(e);
}
java.net.http.HttpResponse<Stream<String>> httpResponse;
try {
httpResponse =
httpClient.send(httpRequest, java.net.http.HttpResponse.BodyHandlers.ofLines());
} catch (InterruptedException e) {
throw new IOException(e);
}
if (httpResponse.statusCode() != HttpURLConnection.HTTP_OK) {
throw new IOException(
"Server returned HTTP response code "
+ httpResponse.statusCode()
+ " for URI "
+ uri);
}
return httpResponse.body();
}

/**
* Opens the given URL and reads text content from it.
* This method honors Content-type header.
*
* @deprecated use {@link #open(URI)}
*/
@Deprecated
protected BufferedReader open(URL url) throws IOException {
// use HTTP content type to find out the charset.
URLConnection con = ProxyConfiguration.open(url);
Expand All @@ -475,11 +516,25 @@ protected BufferedReader open(URL url) throws IOException {
new InputStreamReader(con.getInputStream(), getCharset(con)));
}

/**
* Find the string literal from the given stream of lines.
*
* @return true if found, false otherwise
* @since TODO
*/
protected boolean findText(Stream<String> in, String literal) {
try (in) {
return in.anyMatch(line -> line.contains(literal));
}
}

/**
* Finds the string literal from the given reader.
* @return
* true if found, false otherwise.
* @deprecated use {@link #findText(Stream, String)}
*/
@Deprecated
protected boolean findText(BufferedReader in, String literal) throws IOException {
String line;
while ((line = in.readLine()) != null)
Expand All @@ -499,9 +554,9 @@ protected FormValidation handleIOException(String url, IOException e) throws IOE
// any invalid URL comes here
if (e.getMessage().equals(url))
// Sun JRE (and probably others too) often return just the URL in the error.
return error("Unable to connect " + url);
return error("Unable to connect " + url, e);
Copy link
Member Author

Choose a reason for hiding this comment

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

Minor unrelated change while I was here: append the stack trace for debuggability. We love stack traces.

else
return error(e.getMessage());
return error(e.getMessage(), e);
}

/**
Expand Down
24 changes: 24 additions & 0 deletions core/src/test/java/hudson/util/FormValidationTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,15 @@
import static org.hamcrest.CoreMatchers.containsString;
import static org.hamcrest.CoreMatchers.not;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.is;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertTrue;

import java.io.IOException;
import java.net.URI;
import java.util.Arrays;
import javax.servlet.ServletException;
import org.junit.Test;

/**
Expand Down Expand Up @@ -116,4 +120,24 @@ public void formValidationException() {
FormValidation fv = FormValidation.error(new Exception("<html"), "Message<html");
assertThat(fv.renderHtml(), not(containsString("<html")));
}

@Test
public void testUrlCheck() throws IOException, ServletException {
FormValidation.URLCheck urlCheck = new FormValidation.URLCheck() {
@Override
protected FormValidation check() throws ServletException, IOException {
String uri = "https://www.jenkins.io/";
try {
if (findText(open(URI.create(uri)), "Jenkins")) {
return FormValidation.ok();
} else {
return FormValidation.error("This is a valid URI but it does not look like Jenkins");
}
} catch (IOException e) {
return handleIOException(uri, e);
}
}
};
assertThat(urlCheck.check(), is(FormValidation.ok()));
}
}