[GitHub] guacamole-client pull request #252: GUACAMOLE-504: Add getHttpStatusCode() m...
Github user asfgit closed the pull request at: https://github.com/apache/guacamole-client/pull/252 ---
[GitHub] guacamole-client pull request #252: GUACAMOLE-504: Add getHttpStatusCode() m...
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/252#discussion_r168154358
--- Diff:
guacamole-common/src/main/java/org/apache/guacamole/websocket/GuacamoleWebSocketTunnelEndpoint.java
---
@@ -85,6 +92,21 @@ private void closeConnection(Session session,
GuacamoleStatus guac_status) {
}
+/**
+ * Sends the given Guacaomle Status and closes the given
+ * connection.
+ *
+ * @param session
+ * The outbound WebSocket connection to close.
+ *
+ * @param guac_status
--- End diff --
Fixed.
---
[GitHub] guacamole-client pull request #252: GUACAMOLE-504: Add getHttpStatusCode() m...
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/252#discussion_r168080950
--- Diff:
guacamole-common/src/main/java/org/apache/guacamole/websocket/GuacamoleWebSocketTunnelEndpoint.java
---
@@ -85,6 +92,21 @@ private void closeConnection(Session session,
GuacamoleStatus guac_status) {
}
+/**
+ * Sends the given Guacaomle Status and closes the given
+ * connection.
+ *
+ * @param session
+ * The outbound WebSocket connection to close.
+ *
+ * @param guac_status
--- End diff --
Being new/touched code, this should be updated to match current code style
(`guacStatus` not `guac_status`). The old `guac_status` parameter is a holdover
from the days where I used C style for everything.
---
[GitHub] guacamole-client pull request #252: GUACAMOLE-504: Add getHttpStatusCode() m...
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/252#discussion_r167415300
--- Diff:
guacamole-common/src/main/java/org/apache/guacamole/websocket/GuacamoleWebSocketTunnelEndpoint.java
---
@@ -109,15 +113,15 @@ public void onOpen(final Session session,
EndpointConfig config) {
// Get tunnel
tunnel = createTunnel(session, config);
if (tunnel == null) {
-closeConnection(session,
GuacamoleStatus.RESOURCE_NOT_FOUND);
+closeConnection(session,
GuacamoleStatus.RESOURCE_NOT_FOUND, null);
--- End diff --
Okay, dense moment is passed - I know what to do, now...hang on, another
change coming.
---
[GitHub] guacamole-client pull request #252: GUACAMOLE-504: Add getHttpStatusCode() m...
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/252#discussion_r167409840
--- Diff:
guacamole-common/src/main/java/org/apache/guacamole/websocket/GuacamoleWebSocketTunnelEndpoint.java
---
@@ -109,15 +113,15 @@ public void onOpen(final Session session,
EndpointConfig config) {
// Get tunnel
tunnel = createTunnel(session, config);
if (tunnel == null) {
-closeConnection(session,
GuacamoleStatus.RESOURCE_NOT_FOUND);
+closeConnection(session,
GuacamoleStatus.RESOURCE_NOT_FOUND, null);
--- End diff --
I re-implemented this with just `int` and no overloading, since the only
reason to pass the `GuacamoleStatus` parameter is to access the
`getGuacamoleStatusCode()` method. Let me know how this looks - it doesn't
feel very elegant, but maybe better than `Integer` and `null` route.
---
[GitHub] guacamole-client pull request #252: GUACAMOLE-504: Add getHttpStatusCode() m...
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/252#discussion_r167409807
--- Diff:
guacamole-common/src/main/java/org/apache/guacamole/websocket/GuacamoleWebSocketTunnelEndpoint.java
---
@@ -71,11 +71,15 @@
*
* @param session The outbound WebSocket connection to close.
* @param guac_status The status to send.
+ * @param webSocketCode The numeric WebSocket status to send.
*/
-private void closeConnection(Session session, GuacamoleStatus
guac_status) {
+private void closeConnection(Session session, GuacamoleStatus
guac_status,
+Integer webSocketCode) {
try {
-CloseCode code =
CloseReason.CloseCodes.getCloseCode(guac_status.getWebSocketCode());
+if (webSocketCode == null)
--- End diff --
Switched to `int` and the same approach - passing both parameters as an
`int` and not passing `GuacamoleStatus` at all.
---
[GitHub] guacamole-client pull request #252: GUACAMOLE-504: Add getHttpStatusCode() m...
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/252#discussion_r167409116
--- Diff:
guacamole-common/src/main/java/org/apache/guacamole/servlet/GuacamoleHTTPTunnelServlet.java
---
@@ -256,14 +259,18 @@ else if(query.startsWith(WRITE_PREFIX))
// Catch any thrown guacamole exception and attempt to pass within
the
// HTTP response, logging each error appropriately.
-catch (GuacamoleClientException e) {
-logger.warn("HTTP tunnel request rejected: {}",
e.getMessage());
-sendError(response, e.getStatus(), e.getMessage());
-}
catch (GuacamoleException e) {
-logger.error("HTTP tunnel request failed: {}", e.getMessage());
-logger.debug("Internal error in HTTP tunnel.", e);
-sendError(response, e.getStatus(), "Internal server error.");
+if (e instanceof GuacamoleClientException) {
--- End diff --
Fixed.
---
[GitHub] guacamole-client pull request #252: GUACAMOLE-504: Add getHttpStatusCode() m...
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/252#discussion_r167409002
--- Diff:
guacamole-common/src/main/java/org/apache/guacamole/websocket/GuacamoleWebSocketTunnelEndpoint.java
---
@@ -109,15 +113,15 @@ public void onOpen(final Session session,
EndpointConfig config) {
// Get tunnel
tunnel = createTunnel(session, config);
if (tunnel == null) {
-closeConnection(session,
GuacamoleStatus.RESOURCE_NOT_FOUND);
+closeConnection(session,
GuacamoleStatus.RESOURCE_NOT_FOUND, null);
--- End diff --
Sounds good.
---
[GitHub] guacamole-client pull request #252: GUACAMOLE-504: Add getHttpStatusCode() m...
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/252#discussion_r167408999
--- Diff:
guacamole-common/src/main/java/org/apache/guacamole/websocket/GuacamoleWebSocketTunnelEndpoint.java
---
@@ -71,11 +71,15 @@
*
* @param session The outbound WebSocket connection to close.
* @param guac_status The status to send.
+ * @param webSocketCode The numeric WebSocket status to send.
*/
-private void closeConnection(Session session, GuacamoleStatus
guac_status) {
+private void closeConnection(Session session, GuacamoleStatus
guac_status,
+Integer webSocketCode) {
try {
-CloseCode code =
CloseReason.CloseCodes.getCloseCode(guac_status.getWebSocketCode());
+if (webSocketCode == null)
--- End diff --
Ok.
---
[GitHub] guacamole-client pull request #252: GUACAMOLE-504: Add getHttpStatusCode() m...
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/252#discussion_r167408983
--- Diff:
guacamole-common/src/main/java/org/apache/guacamole/servlet/GuacamoleHTTPTunnelServlet.java
---
@@ -256,14 +259,18 @@ else if(query.startsWith(WRITE_PREFIX))
// Catch any thrown guacamole exception and attempt to pass within
the
// HTTP response, logging each error appropriately.
-catch (GuacamoleClientException e) {
-logger.warn("HTTP tunnel request rejected: {}",
e.getMessage());
-sendError(response, e.getStatus(), e.getMessage());
-}
catch (GuacamoleException e) {
-logger.error("HTTP tunnel request failed: {}", e.getMessage());
-logger.debug("Internal error in HTTP tunnel.", e);
-sendError(response, e.getStatus(), "Internal server error.");
+if (e instanceof GuacamoleClientException) {
--- End diff --
Okay - I wasn't sure what the more accepted/elegant approach was. Will
rework it that way.
---
[GitHub] guacamole-client pull request #252: GUACAMOLE-504: Add getHttpStatusCode() m...
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/252#discussion_r167408776
--- Diff:
guacamole-common/src/main/java/org/apache/guacamole/servlet/GuacamoleHTTPTunnelServlet.java
---
@@ -256,14 +259,18 @@ else if(query.startsWith(WRITE_PREFIX))
// Catch any thrown guacamole exception and attempt to pass within
the
// HTTP response, logging each error appropriately.
-catch (GuacamoleClientException e) {
-logger.warn("HTTP tunnel request rejected: {}",
e.getMessage());
-sendError(response, e.getStatus(), e.getMessage());
-}
catch (GuacamoleException e) {
-logger.error("HTTP tunnel request failed: {}", e.getMessage());
-logger.debug("Internal error in HTTP tunnel.", e);
-sendError(response, e.getStatus(), "Internal server error.");
+if (e instanceof GuacamoleClientException) {
--- End diff --
There's no need to manually check `instanceof` - this is exactly what
`catch` is for.
---
[GitHub] guacamole-client pull request #252: GUACAMOLE-504: Add getHttpStatusCode() m...
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/252#discussion_r167408912
--- Diff:
guacamole-common/src/main/java/org/apache/guacamole/websocket/GuacamoleWebSocketTunnelEndpoint.java
---
@@ -109,15 +113,15 @@ public void onOpen(final Session session,
EndpointConfig config) {
// Get tunnel
tunnel = createTunnel(session, config);
if (tunnel == null) {
-closeConnection(session,
GuacamoleStatus.RESOURCE_NOT_FOUND);
+closeConnection(session,
GuacamoleStatus.RESOURCE_NOT_FOUND, null);
--- End diff --
Assuming that we do go the status code / WebSocket code route (ie: same as
the HTTP case), the DRY approach here would be to overload `closeConnection()`
with a variant which accepts only `GuacamoleStatus`, pulling the codes it needs
from there.
---
[GitHub] guacamole-client pull request #252: GUACAMOLE-504: Add getHttpStatusCode() m...
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/252#discussion_r167408870
--- Diff:
guacamole-common/src/main/java/org/apache/guacamole/websocket/GuacamoleWebSocketTunnelEndpoint.java
---
@@ -71,11 +71,15 @@
*
* @param session The outbound WebSocket connection to close.
* @param guac_status The status to send.
+ * @param webSocketCode The numeric WebSocket status to send.
*/
-private void closeConnection(Session session, GuacamoleStatus
guac_status) {
+private void closeConnection(Session session, GuacamoleStatus
guac_status,
+Integer webSocketCode) {
try {
-CloseCode code =
CloseReason.CloseCodes.getCloseCode(guac_status.getWebSocketCode());
+if (webSocketCode == null)
--- End diff --
Why not the same approach that you used with the HTTP status code handling?
(Separate parameters for each type of status code rather than passing
`GuacamoleStatus`)
It's generally bad practice to rely on wrapper classes like `Integer` when
not necessary, and I think this is one of those cases where we really should be
looking for a way to use a primitive `int`.
---
[GitHub] guacamole-client pull request #252: GUACAMOLE-504: Add getHttpStatusCode() m...
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/252#discussion_r167134989
--- Diff:
guacamole-common/src/main/java/org/apache/guacamole/servlet/GuacamoleHTTPTunnelServlet.java
---
@@ -149,26 +149,23 @@ protected void doPost(HttpServletRequest request,
HttpServletResponse response)
* @param response
* The HTTP response to use to send the error.
*
- * @param guacStatus
- * The status to send
- *
- * @param message
- * A human-readable message that can be presented to the user.
+ * @param guacamoleException
+ * The exception that caused this error.
*
* @throws ServletException
* If an error prevents sending of the error code.
*/
protected void sendError(HttpServletResponse response,
-GuacamoleStatus guacStatus, String message)
+GuacamoleException guacamoleException)
throws ServletException {
try {
// If response not committed, send error code and message
if (!response.isCommitted()) {
-response.addHeader("Guacamole-Status-Code",
Integer.toString(guacStatus.getGuacamoleStatusCode()));
-response.addHeader("Guacamole-Error-Message", message);
-response.sendError(guacStatus.getHttpStatusCode());
+response.addHeader("Guacamole-Status-Code",
Integer.toString(guacamoleException.getStatus().getGuacamoleStatusCode()));
+response.addHeader("Guacamole-Error-Message",
guacamoleException.getMessage());
--- End diff --
Okay, I think I've reworked this in a way that avoids duplicating too much
code but successfully hides internals we don't want to reveal.
---
[GitHub] guacamole-client pull request #252: GUACAMOLE-504: Add getHttpStatusCode() m...
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/252#discussion_r167104001
--- Diff:
guacamole-common/src/main/java/org/apache/guacamole/servlet/GuacamoleHTTPTunnelServlet.java
---
@@ -149,26 +149,23 @@ protected void doPost(HttpServletRequest request,
HttpServletResponse response)
* @param response
* The HTTP response to use to send the error.
*
- * @param guacStatus
- * The status to send
- *
- * @param message
- * A human-readable message that can be presented to the user.
+ * @param guacamoleException
+ * The exception that caused this error.
*
* @throws ServletException
* If an error prevents sending of the error code.
*/
protected void sendError(HttpServletResponse response,
-GuacamoleStatus guacStatus, String message)
+GuacamoleException guacamoleException)
throws ServletException {
try {
// If response not committed, send error code and message
if (!response.isCommitted()) {
-response.addHeader("Guacamole-Status-Code",
Integer.toString(guacStatus.getGuacamoleStatusCode()));
-response.addHeader("Guacamole-Error-Message", message);
-response.sendError(guacStatus.getHttpStatusCode());
+response.addHeader("Guacamole-Status-Code",
Integer.toString(guacamoleException.getStatus().getGuacamoleStatusCode()));
+response.addHeader("Guacamole-Error-Message",
guacamoleException.getMessage());
--- End diff --
The exception message should only be exposed via the
`Guacamole-Error-Message` header when the exception itself deals with a
client-side issue (any subclass of `GuacamoleClientException`). For other
exceptions, the message should be assumed to contain internal information, and
should not be forwarded along to the client.
This is the reason for the difference in handling below, where "Internal
server error" is explicitly substituted for the exception message if the
exception is not a `GuacamoleClientException`.
---
[GitHub] guacamole-client pull request #252: GUACAMOLE-504: Add getHttpStatusCode() m...
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/252#discussion_r166950252
--- Diff:
guacamole-common/src/main/java/org/apache/guacamole/GuacamoleException.java ---
@@ -68,5 +68,15 @@ public GuacamoleException(Throwable cause) {
public GuacamoleStatus getStatus() {
return GuacamoleStatus.SERVER_ERROR;
}
+
+/**
+ * Returns the numeric HTTP status code associated with this exception.
+ *
+ * @return
+ * The numeric HTTP status code associated with this exception.
+ */
+public Integer getHttpStatusCode() {
--- End diff --
Changed.
---
[GitHub] guacamole-client pull request #252: GUACAMOLE-504: Add getHttpStatusCode() m...
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/252#discussion_r166950217
--- Diff:
guacamole-common/src/main/java/org/apache/guacamole/GuacamoleException.java ---
@@ -68,5 +68,15 @@ public GuacamoleException(Throwable cause) {
public GuacamoleStatus getStatus() {
return GuacamoleStatus.SERVER_ERROR;
}
+
+/**
+ * Returns the numeric HTTP status code associated with this exception.
--- End diff --
Reworded it a bit.
---
[GitHub] guacamole-client pull request #252: GUACAMOLE-504: Add getHttpStatusCode() m...
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/252#discussion_r166861642
--- Diff:
guacamole-common/src/main/java/org/apache/guacamole/GuacamoleException.java ---
@@ -68,5 +68,15 @@ public GuacamoleException(Throwable cause) {
public GuacamoleStatus getStatus() {
return GuacamoleStatus.SERVER_ERROR;
}
+
+/**
+ * Returns the numeric HTTP status code associated with this exception.
--- End diff --
I think the semantics of the value returned here need to be more clearly
defined. With `GuacamoleStatus`, for example, `getHttpStatusCode()` is defined
as returning "the most applicable HTTP error code". In this case, we should be
clear that the HTTP status code should be the nearest equivalent to the status
represented by this exception.
---
[GitHub] guacamole-client pull request #252: GUACAMOLE-504: Add getHttpStatusCode() m...
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/252#discussion_r166861849
--- Diff:
guacamole-common/src/main/java/org/apache/guacamole/GuacamoleException.java ---
@@ -68,5 +68,15 @@ public GuacamoleException(Throwable cause) {
public GuacamoleStatus getStatus() {
return GuacamoleStatus.SERVER_ERROR;
}
+
+/**
+ * Returns the numeric HTTP status code associated with this exception.
+ *
+ * @return
+ * The numeric HTTP status code associated with this exception.
+ */
+public Integer getHttpStatusCode() {
--- End diff --
This should a primitive `int`, not `Integer`.
---
[GitHub] guacamole-client pull request #252: GUACAMOLE-504: Add getHttpStatusCode() m...
GitHub user necouchman opened a pull request: https://github.com/apache/guacamole-client/pull/252 GUACAMOLE-504: Add getHttpStatusCode() method to GuacamoleException class Adds the getHttpStatusCode() method to the GuacamoleException class, which will allow for exceptions to explicitly override the HTTP status code as required. You can merge this pull request into a Git repository by running: $ git pull https://github.com/necouchman/guacamole-client GUACAMOLE-504 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/guacamole-client/pull/252.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #252 commit 7ffa2a0cd1326693e37ea61f63640f7e3f385340 Author: Nick Couchman Date: 2018-02-08T03:57:21Z GUACAMOLE-504: Add getHttpStatusCode() method to GuacamoleException class. ---
