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.

Sign up for GitHub

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

Unify error message structure in EventBusBridge #2701

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Unify error message structure in EventBusBridge #2701

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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 vertx-web/src/main/asciidoc/index.adoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2549,8 +2549,8 @@ follow the bridge wire protocol and look like this:
----
{
"type": "err",
"failureType": "socketException",
"message": "optionally a message from the exception being raised"
"failureType": "SOCKET_EXCEPTION",
"message": "A message from the exception being raised"
}
----

Expand Down
56 changes: 31 additions & 25 deletions vertx-web/src/main/java/io/vertx/ext/web/handler/sockjs/impl/EventBusBridgeImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,13 +80,13 @@ private void handleSocketData(SockJSSocket sock, Buffer data, Map<String, Messag
try {
msg = new JsonObject(data.toString());
} catch (DecodeException e) {
replyError(sock, "invalid_json");
replyError(sock, "INVALID_JSON", "Malformed JSON");
return;
}

String type = msg.getString("type");
if (type == null) {
replyError(sock, "missing_type");
replyError(sock, "MISSING_TYPE", "Message type is missing");
return;
}

Expand All @@ -95,7 +95,7 @@ private void handleSocketData(SockJSSocket sock, Buffer data, Map<String, Messag
} else {
String address = msg.getString("address");
if (address == null) {
replyError(sock, "missing_address");
replyError(sock, "MISSING_ADDRESS", "Message address is missing");
return;
}
switch (type) {
Expand All @@ -113,7 +113,7 @@ private void handleSocketData(SockJSSocket sock, Buffer data, Map<String, Messag
break;
default:
LOG.error("Invalid type in incoming message: " + type);
replyError(sock, "invalid_type");
replyError(sock, "INVALID_TYPE", "Invalid message type");
}
}

Expand Down Expand Up @@ -165,19 +165,19 @@ private void internalHandleSendOrPub(SockJSSocket sock, boolean send, JsonObject
() -> {
String address = msg.getString("address");
if (address == null) {
replyError(sock, "missing_address");
replyError(sock, "MISSING_ADDRESS", "Message address is missing");
Copy link
Contributor

Choose a reason for hiding this comment

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

we should define those in an package protected enum instead of havingthem scattered in the code base

return;
}
doSendOrPub(send, sock, address, msg);
}, () -> replyError(sock, "rejected"));
}, () -> replyError(sock, "REJECTED", "Message is rejected"));
}

private boolean checkMaxHandlers(SockJSSocket sock, SockInfo info) {
if (info.handlerCount < maxHandlersPerSocket) {
return true;
} else {
LOG.warn("Refusing to register as max_handlers_per_socket reached already");
replyError(sock, "max_handlers_reached");
replyError(sock, "HANDLERS_MAX_LIMIT", "Registration handlers exceed the maximum limit");
return false;
}
}
Expand All @@ -192,11 +192,11 @@ private void internalHandleRegister(SockJSSocket sock, JsonObject rawMsg, Map<St
final boolean debug = LOG.isDebugEnabled();
final String address = rawMsg.getString("address");
if (address == null) {
replyError(sock, "missing_address");
replyError(sock, "MISSING_ADDRESS", "Message address is missing");
return;
} else if (address.length() > maxAddressLength) {
LOG.warn("Refusing to register as address length > max_address_length");
replyError(sock, "max_address_length_reached");
replyError(sock, "ADDRESS_MAX_LENGTH", "Address exceeds maximum length");
return;
}
Match match = checkMatches(false, address, null);
Expand All @@ -206,7 +206,7 @@ private void internalHandleRegister(SockJSSocket sock, JsonObject rawMsg, Map<St
// loop could DDoS the bridge.
if (registrations.containsKey(address)) {
LOG.warn("Refusing to register as address is already registered");
replyError(sock, "address_already_registered");
replyError(sock, "ADDRESS_ALREADY_REGISTERED", "Address is already registered");
return;
}

Expand Down Expand Up @@ -247,25 +247,25 @@ private void internalHandleRegister(SockJSSocket sock, JsonObject rawMsg, Map<St
checkCallHook(() -> new BridgeEventImpl(BridgeEventType.REGISTERED, rawMsg, sock));
} else {
LOG.warn("Cannot register handler for address " + address, ar.cause());
replyError(sock, "registration_failure");
replyError(sock, "ADDRESS_REGISTRATION", "Address registration is failed");
}
});
} else {
// inbound match failed
if (debug) {
LOG.debug("Cannot register handler for address " + address + " because there is no inbound match");
}
replyError(sock, "access_denied");
replyError(sock, "ACCESS_DENIED", "Address access is denied");
}
}, () -> replyError(sock, "rejected"));
}, () -> replyError(sock, "REJECTED", "Message is rejected"));
}

private void internalHandleUnregister(SockJSSocket sock, JsonObject rawMsg, Map<String, MessageConsumer<?>> registrations) {
checkCallHook(() -> new BridgeEventImpl(BridgeEventType.UNREGISTER, rawMsg, sock),
() -> {
String address = rawMsg.getString("address");
if (address == null) {
replyError(sock, "missing_address");
replyError(sock, "MISSING_ADDRESS","Message address is missing");
return;
}
Match match = checkMatches(false, address, null);
Expand All @@ -280,9 +280,9 @@ private void internalHandleUnregister(SockJSSocket sock, JsonObject rawMsg, Map<
if (LOG.isDebugEnabled()) {
LOG.debug("Cannot unregister handler for address " + address + " because there is no inbound match");
}
replyError(sock, "access_denied");
replyError(sock, "ACCESS_DENIED", "Address access is denied");
}
}, () -> replyError(sock, "rejected"));
}, () -> replyError(sock, "REJECTED", "Message is rejected"));
}

private void internalHandlePing(final SockJSSocket sock) {
Expand Down Expand Up @@ -317,7 +317,7 @@ public void handle(final SockJSSocket sock) {
checkCallHook(() -> new BridgeEventImpl(BridgeEventType.SOCKET_IDLE, null, sock),
// We didn't receive a ping in time so close the socket
((SockJSSocketBase) sock)::closeAfterSessionExpired,
() -> replyError(sock, "rejected"));
() -> replyError(sock, "REJECTED", "Message is rejected"));
}
});
SockInfo sockInfo = new SockInfo();
Expand All @@ -334,9 +334,11 @@ private void handleSocketClosed(SockJSSocket sock, Map<String, MessageConsumer<?
private void handleSocketException(SockJSSocket sock, Throwable err, Map<String, MessageConsumer<?>> registrations) {
LOG.error("SockJSSocket exception", err);
clearSocketState(sock, registrations);
final JsonObject msg = new JsonObject().put("type", "err").put("failureType", "socketException");
final JsonObject msg = new JsonObject().put("type", "err").put("failureCode", -1).put("failureType", "SOCKET_EXCEPTION");
if (err != null) {
msg.put("message", err.getMessage());
} else {
msg.put("message", "A socket exception occurred while attempting to establish or maintain a network connection");
}
checkCallHook(() -> new BridgeEventImpl(BridgeEventType.SOCKET_ERROR, msg, sock));
}
Expand Down Expand Up @@ -408,7 +410,7 @@ private void doSendOrPub(boolean send, SockJSSocket sock, String address,
if (replyAddress != null && replyAddress.length() > 36) {
// vertx-eventbus.js ids are always 36 chars
LOG.error("Will not send message, reply address is > 36 chars");
replyError(sock, "invalid_reply_address");
replyError(sock, "INVALID_REPLY_ADDRESS", "Reply address is invalid");
return;
}
final boolean debug = LOG.isDebugEnabled();
Expand All @@ -431,19 +433,19 @@ private void doSendOrPub(boolean send, SockJSSocket sock, String address,
if (ok) {
checkAndSend(send, address, body, headers, sock, replyAddress, awaitingReply);
} else {
replyError(sock, "access_denied");
replyError(sock, "ACCESS_DENIED", "Address access is denied");
if (debug) {
LOG.debug("Inbound message for address " + address + " rejected because is not authorised");
}
}
})
.onFailure(err -> {
replyError(sock, "auth_error");
replyError(sock, "AUTHZ", "Authorization failed");
LOG.error("Error in performing authorization", err);
});
} else {
// no web session
replyError(sock, "not_logged_in");
replyError(sock, "AUTHN", "Authentication is required");
if (debug) {
LOG.debug("Inbound message for address " + address +
" rejected because it requires auth and user is not authenticated");
Expand All @@ -454,7 +456,7 @@ private void doSendOrPub(boolean send, SockJSSocket sock, String address,
}
} else {
// inbound match failed
replyError(sock, "access_denied");
replyError(sock, "ACCESS_DENIED", "Address access is denied");
if (debug) {
LOG.debug("Inbound message for address " + address + " rejected because there is no match");
}
Expand Down Expand Up @@ -590,8 +592,12 @@ private boolean regexMatches(String matchRegex, String address) {
return m.matches();
}

private static void replyError(SockJSSocket sock, String err) {
JsonObject envelope = new JsonObject().put("type", "err").put("body", err);
private static void replyError(SockJSSocket sock, String type, String message) {
JsonObject envelope = new JsonObject()
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need to keep the old behavior as well to avoid breaking change and test it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vietj Do you mean using 'body' key for error messages?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vietj

IMHO the existing behavior should be changed you can check the Vert.x TCP EventBus bridge uses “message” key instead of “body” to report descriptive message for error type messages.

https://github.com/vert-x3/vertx-tcp-eventbus-bridge/blob/master/src/main/java/io/vertx/ext/eventbus/bridge/tcp/impl/protocol/FrameHelper.java#L86

.put("type", "err")
.put("failureCode", -1)
.put("failureType", type)
.put("message", message);
sock.write(buffer(envelope.encode()));
}

Expand Down
Loading
Loading