- Notifications
You must be signed in to change notification settings - Fork556
Unify error message structure in EventBusBridge#2701
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.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:master
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
EmadAlblueshi commentedJan 17, 2025
Ping@tsegismont :) |
EmadAlblueshi commentedJan 17, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Regarding the behavior of the 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 I’m happy to know and learn more from you!@tsegismont@vietj |
Signed-off-by: Emad Alblueshi <emad.albloushi@gmail.com>
e714fec to7671044Compare| privatestaticvoidreplyError(SockJSSocketsock,Stringerr) { | ||
| JsonObjectenvelope =newJsonObject().put("type","err").put("body",err); | ||
| privatestaticvoidreplyError(SockJSSocketsock,Stringtype,Stringmessage) { | ||
| JsonObjectenvelope =newJsonObject() |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
yes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
vietj left a comment
There was a problem hiding this 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.
| Stringaddress =msg.getString("address"); | ||
| if (address ==null) { | ||
| replyError(sock,"missing_address"); | ||
| replyError(sock,"MISSING_ADDRESS","Message address is missing"); |
There was a problem hiding this comment.
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 commentedJan 21, 2025
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 |
Uh oh!
There was an error while loading.Please reload this page.
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:The
failureCodefor the above is-1and all types with messages are tested except forSOCKET_EXCEPTIONwhich I didn't find any unit test for such a use case so I added generic descriptive message in case theThrowable#getMessage()isnull.Finally, this PR addresses inconsistency of all error messages in
EventBusBridge#2693Thanks!