Feature ETP-2946: Tomcat ui detection#846
Feature ETP-2946: Tomcat ui detection#846sebastianbarrozo wants to merge 1 commit intofeature/ETP-2938-Y26from
Conversation
|
| serverAvailable = (responseCode >= 200 && responseCode < 500); | ||
| connection.disconnect(); | ||
| } catch (Exception e) { | ||
| serverAvailable = false; |
There was a problem hiding this comment.
Suggestion
While it's acceptable to catch exceptions and set the server as unavailable, it may be beneficial for troubleshooting to log the exception details at least at a debug level. This is non-blocking as empty catch blocks are only blocking if errors are swallowed in a way that prevents error detection. In this context, the fallback path is clear, but some minimal logging aids support.
catch (Exception e) {
serverAvailable = false;
// Consider logging exception details for diagnostics
}Rationale: Etendo exception handling best practices encourage at least some logging, even for non-critical paths, to aid debugging. The current code swallows exceptions silently.
| connection.setConnectTimeout(3000); // 3 seconds timeout | ||
| connection.setReadTimeout(3000); | ||
| int responseCode = connection.getResponseCode(); | ||
| serverAvailable = (responseCode >= 200 && responseCode < 500); |
There was a problem hiding this comment.
Suggestion
Consider catching IOException explicitly or distinguishing between connection errors and HTTP non-2xx codes. This would allow for more granular error messaging or fallback if desired in future improvements, though it is not critical in this workflow.
int responseCode = connection.getResponseCode();
serverAvailable = (responseCode >= 200 && responseCode < 500);
// Consider catching IOException separately to distinguish connection failures vs. HTTP errors
connection.disconnect();Rationale: Etendo error handling patterns encourage distinguishing between common connection errors and protocol errors for robust diagnostics and future extensibility.
| // Check if new UI server is available | ||
| boolean serverAvailable = false; | ||
| try { | ||
| URL url = new URL(nextgenUrl); |
There was a problem hiding this comment.
Warning
The catch (Exception e) {} block is empty, which is a critical issue. All exceptions must be handled, at least by logging them, to prevent silently masking potential problems. Additionally, the HttpURLConnection should be disconnected in a finally block to ensure resources are released even if an exception occurs.
HttpURLConnection connection = null;
try {
URL url = new URL(nextgenUrl);
connection = (HttpURLConnection) url.openConnection();
connection.setRequestMethod("GET");
connection.setConnectTimeout(3000); // 3 seconds timeout
connection.setReadTimeout(3000);
int responseCode = connection.getResponseCode();
serverAvailable = (responseCode >= 200 && responseCode < 500);
} catch (Exception e) {
// Log the exception to understand why the connection failed
System.err.println("Error checking NextGen UI server availability: " + e.getMessage());
serverAvailable = false;
} finally {
if (connection != null) {
connection.disconnect();
}
}Rationale: Empty catch blocks are a common source of hidden bugs and make debugging extremely difficult. Proper logging is essential for error detection and system monitoring. Furthermore, not explicitly disconnecting HttpURLConnection can lead to resource leaks and connection exhaustion over time. The Etendo best practices for exception handling often involve logging and rethrowing OBException or similar framework-specific exceptions.
etendobot
left a comment
There was a problem hiding this comment.
Changes requested by agent. Please resolve blocking issues.

0 New Issues
0 Fixed Issues
0 Accepted Issues
No data about coverage (0.00% Estimated after merge)
No description provided.