snoopdave commented on code in PR #236:
URL: https://github.com/apache/roller/pull/236#discussion_r4487173608
##########
app/src/main/java/org/apache/roller/weblogger/business/pings/WeblogUpdatePinger.java:
##########
@@ -114,42 +141,234 @@ public static PingResult sendPing(PingTarget pingTarget,
Weblog website) throws
LOGGER.debug("Executing ping to '" + pingTargetUrl + "' for
website '" + websiteUrl + "' (" + website.getName() + ")" +
(variantOptions.isEmpty() ? "" : " with variant options " + variantOptions));
}
- // Send the ping.
- XmlRpcClientConfigImpl config = new XmlRpcClientConfigImpl();
- config.setServerURL(new URL(pingTargetUrl));
- XmlRpcClient client = new XmlRpcClient();
- client.setConfig(config);
- PingResult pingResult =
parseResult(client.execute("weblogUpdates.ping", params.toArray()));
+ // Send the ping and read the response.
+ byte[] response = exchange(pingTargetUrl, buildRequest(params));
+ PingResult pingResult = parseResponse(response);
if (LOGGER.isDebugEnabled()) {
LOGGER.debug("Ping result is: " + pingResult);
}
return pingResult;
}
- private static PingResult parseResult(Object obj) {
- // Deal with the fact that some buggy ping targets may not respond
with the proper struct type.
- if (obj == null) {
- return new PingResult(null,null);
+ /**
+ * Build the XML-RPC <code>weblogUpdates.ping</code> request body for the
+ * given string parameters.
+ */
+ static String buildRequest(List<String> params) {
+ StringBuilder sb = new StringBuilder(256);
+ sb.append("<?xml version=\"1.0\" encoding=\"UTF-8\"?>");
+
sb.append("<methodCall><methodName>").append(PING_METHOD_NAME).append("</methodName>");
+ sb.append("<params>");
+ for (String param : params) {
+ sb.append("<param><value><string>");
+ xmlEscape(sb, param);
+ sb.append("</string></value></param>");
+ }
+ sb.append("</params></methodCall>");
+ return sb.toString();
+ }
+
+ /**
+ * POST the request body to the ping target and return the response bytes,
+ * capped at {@link #MAX_RESPONSE_BYTES}.
+ */
+ private static byte[] exchange(String pingTargetUrl, String requestBody)
throws IOException {
+ URL url = new URL(pingTargetUrl);
+ HttpURLConnection conn = (HttpURLConnection) url.openConnection();
+ try {
+ conn.setRequestMethod("POST");
+ conn.setConnectTimeout(CONNECT_TIMEOUT_MS);
+ conn.setReadTimeout(READ_TIMEOUT_MS);
+ conn.setDoOutput(true);
+ conn.setRequestProperty("Content-Type", "text/xml; charset=UTF-8");
+ conn.setRequestProperty("User-Agent", "Apache Roller");
+
+ byte[] body = requestBody.getBytes(StandardCharsets.UTF_8);
+ conn.setFixedLengthStreamingMode(body.length);
+ try (OutputStream out = conn.getOutputStream()) {
+ out.write(body);
+ }
+
+ int status = conn.getResponseCode();
+ InputStream in = (status >= 200 && status < 300) ?
conn.getInputStream() : conn.getErrorStream();
Review Comment:
🐞Claude Issue: **Important:** Non-2xx HTTP responses are treated as a
successful ping. On a non-2xx status this reads `getErrorStream()` and parses
it; a 5xx/4xx body is typically HTML or empty, so `parseResponse` throws
`SAXException` (or gets empty input) and returns `PingResult(null, null)` — no
exception, `isError()==false`. `PingQueueProcessor` then marks the ping
succeeded and removes it from the queue, and `pingNow` reports success. The
previous `XmlRpcClient` raised a transport error on non-2xx, so a down/erroring
target is now silently treated as delivered instead of failed/retried. Suggest
treating non-2xx as a failure, e.g. `throw new IOException("HTTP " + status)`
before reading the body, which also re-enters the existing retry logic, and add
a test for it.
##########
app/src/main/java/org/apache/roller/weblogger/business/pings/WeblogUpdatePinger.java:
##########
@@ -114,42 +141,234 @@ public static PingResult sendPing(PingTarget pingTarget,
Weblog website) throws
LOGGER.debug("Executing ping to '" + pingTargetUrl + "' for
website '" + websiteUrl + "' (" + website.getName() + ")" +
(variantOptions.isEmpty() ? "" : " with variant options " + variantOptions));
}
- // Send the ping.
- XmlRpcClientConfigImpl config = new XmlRpcClientConfigImpl();
- config.setServerURL(new URL(pingTargetUrl));
- XmlRpcClient client = new XmlRpcClient();
- client.setConfig(config);
- PingResult pingResult =
parseResult(client.execute("weblogUpdates.ping", params.toArray()));
+ // Send the ping and read the response.
+ byte[] response = exchange(pingTargetUrl, buildRequest(params));
+ PingResult pingResult = parseResponse(response);
if (LOGGER.isDebugEnabled()) {
LOGGER.debug("Ping result is: " + pingResult);
}
return pingResult;
}
- private static PingResult parseResult(Object obj) {
- // Deal with the fact that some buggy ping targets may not respond
with the proper struct type.
- if (obj == null) {
- return new PingResult(null,null);
+ /**
+ * Build the XML-RPC <code>weblogUpdates.ping</code> request body for the
+ * given string parameters.
+ */
+ static String buildRequest(List<String> params) {
+ StringBuilder sb = new StringBuilder(256);
+ sb.append("<?xml version=\"1.0\" encoding=\"UTF-8\"?>");
+
sb.append("<methodCall><methodName>").append(PING_METHOD_NAME).append("</methodName>");
+ sb.append("<params>");
+ for (String param : params) {
+ sb.append("<param><value><string>");
+ xmlEscape(sb, param);
+ sb.append("</string></value></param>");
+ }
+ sb.append("</params></methodCall>");
+ return sb.toString();
+ }
+
+ /**
+ * POST the request body to the ping target and return the response bytes,
+ * capped at {@link #MAX_RESPONSE_BYTES}.
+ */
+ private static byte[] exchange(String pingTargetUrl, String requestBody)
throws IOException {
+ URL url = new URL(pingTargetUrl);
+ HttpURLConnection conn = (HttpURLConnection) url.openConnection();
Review Comment:
🐞Claude Issue: **Important:** A non-HTTP ping target URL throws an uncaught
`ClassCastException` here. `(HttpURLConnection) url.openConnection()` fails
with `ClassCastException` for a `file:`/`ftp:`/other-scheme target, and
`Pings.pingNow` catches only `IOException` and `XmlRpcException`, so the
unchecked exception escapes to Struts. Low likelihood given admin-configured
targets, but cheap to guard: check the URL scheme (or that the connection is an
`HttpURLConnection`) and throw `IOException` otherwise, so it flows through the
normal ping-error handling.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
