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

Jump to bottom

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

wants to merge 1 commit into from
+82 −68

Conversation

EmadAlblueshi
Copy link
Contributor

@EmadAlblueshi EmadAlblueshi commented Jan 17, 2025 •
edited
Loading

Motivation:

The message structure should be consistent to all error messages in EventBusBridge. The following is a list of a consistent error types with descriptive messages:

Type : INVALID_JSON
Message : Malformed JSON

Type : MISSING_TYPE
Message : Message type is missing

Type : INVALID_TYPE 
Message : Invalid message type

Type : MISSING_ADDRESS
Message : Message address is missing

Type : REJECTED
Message : Message is rejected

Type : HANDLERS_MAX_LIMIT
Message : Registration handlers exceed maximum limit

Type : ADDRESS_MAX_LENGTH
Message : Address exceeds maximum length

Type ADDRESS_ALREADY_REGISTERED
Message : Address is already registered

Type ADDRESS_REGISTRATION
Message : Address registration is failed

Type : ACCESS_DENIED
Message : Address access is denied

Type : INVALID_REPLY_ADDRESS
Message : Reply address is invalid

Type : AUTHZ
Message : Authorization failed

Type : AUTHN
Message Authentication is required

Type SOCKET_EXCEPTION
Message : A socket exception occurred while attempting to establish or maintain a network connection

The failureCode for the above is -1 and all types with messages are tested except for SOCKET_EXCEPTION which I didn't find any unit test for such a use case so I added generic descriptive message in case the Throwable#getMessage() is null.

Finally, this PR addresses inconsistency of all error messages in EventBusBridge #2693

Thanks!

@EmadAlblueshi
Copy link
Contributor Author

Ping @tsegismont :)

@EmadAlblueshi
Copy link
Contributor Author

EmadAlblueshi commented Jan 17, 2025 •
edited
Loading

Regarding the behavior of the EventBusBridge, error message types should be categorized after reporting to determine whether to maintain the connection or close it immediately.

The primary objective is to minimize any potential attempts to exploit connection resources or expose security vulnerabilities.

For example, if the message is invalid or the authentication is required why we should keep the connection maintained?

At present, I believe it is necessary to improve the behavior of the EventBusBridge to address those edge cases more effectively.

I’m happy to know and learn more from you! @tsegismont @vietj

@EmadAlblueshi EmadAlblueshi marked this pull request as draft January 18, 2025 23:19
@EmadAlblueshi EmadAlblueshi marked this pull request as ready for review January 18, 2025 23:44
vietj
vietj reviewed Jan 20, 2025
View reviewed changes
vertx-web/src/main/java/io/vertx/ext/web/handler/sockjs/impl/EventBusBridgeImpl.java
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

vietj
vietj requested changes Jan 20, 2025
View reviewed changes
Copy link
Contributor

@vietj vietj left a comment

Choose a reason for hiding this comment

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

This looks like a good change to me, however it brings breaking changes in the formatting of the json error.

We should keep the old behavior and test it to ensure people migrating from Vert.x 4 will not have unexpected issues.

@vietj vietj added this to the 5.0.0 milestone Jan 20, 2025
vietj
vietj reviewed Jan 20, 2025
View reviewed changes
vertx-web/src/main/java/io/vertx/ext/web/handler/sockjs/impl/EventBusBridgeImpl.java
@@ -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

@EmadAlblueshi
Copy link
Contributor Author

This looks like a good change to me, however it brings breaking changes in the formatting of the json error.

We should keep the old behavior and test it to ensure people migrating from Vert.x 4 will not have unexpected issues.

Honestly, If we keep the formatting of the json error as is this won't reflect the purpose of the PR and will be only for error types revamping from String to Enum. is this what you recommend?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vietj vietj vietj requested changes

Assignees
No one assigned
Labels
None yet
Milestone
5.0.0
Development

Successfully merging this pull request may close these issues.
2 participants
@EmadAlblueshi @vietj