You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@guacamole.apache.org by necouchman <gi...@git.apache.org> on 2018/02/08 03:58:31 UTC
[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 <vn...@...>
Date: 2018-02-08T03:57:21Z
GUACAMOLE-504: Add getHttpStatusCode() method to GuacamoleException class.
----
---
[GitHub] guacamole-client pull request #252: GUACAMOLE-504: Add getHttpStatusCode() m...
Posted by necouchman <gi...@git.apache.org>.
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...
Posted by mike-jumper <gi...@git.apache.org>.
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...
Posted by necouchman <gi...@git.apache.org>.
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...
Posted by mike-jumper <gi...@git.apache.org>.
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...
Posted by necouchman <gi...@git.apache.org>.
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...
Posted by mike-jumper <gi...@git.apache.org>.
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...
Posted by necouchman <gi...@git.apache.org>.
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...
Posted by necouchman <gi...@git.apache.org>.
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...
Posted by necouchman <gi...@git.apache.org>.
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...
Posted by necouchman <gi...@git.apache.org>.
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...
Posted by mike-jumper <gi...@git.apache.org>.
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...
Posted by asfgit <gi...@git.apache.org>.
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...
Posted by mike-jumper <gi...@git.apache.org>.
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...
Posted by mike-jumper <gi...@git.apache.org>.
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...
Posted by necouchman <gi...@git.apache.org>.
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...
Posted by necouchman <gi...@git.apache.org>.
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...
Posted by mike-jumper <gi...@git.apache.org>.
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...
Posted by necouchman <gi...@git.apache.org>.
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...
Posted by necouchman <gi...@git.apache.org>.
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.
---