Add a better message error on RoutingContextImpl #2677
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tsegismont
reviewed
Nov 12, 2024
There was a problem hiding this comment.
Thank you @mcruzdev
This is a good improvement. Just one concern about the exception type.
| String message = HttpVersion.HTTP_1_1 == request.version() ? | ||
| "For HTTP/1.x requests, the 'Host' header is required" : | ||
| "For HTTP/2 requests, the ':authority' pseudo-header is required"; | ||
| fail(400, new IllegalStateException(message)); |
There was a problem hiding this comment.
I don't think capturing a stacktrace is necessary in this case. In other places, for similar purpose, we use VertxException:
|
The build failure looks unrelated (same thing happened on the main branch). It should be gone after the PR has been updated and rebased 🤞 |
|
Thank you for the help! |
mcruzdev
force-pushed
the
no-authority-msg
branch
2 times, most recently
from
e32cd4b
to
0a91b7f
Compare
tsegismont
reviewed
Nov 15, 2024
There was a problem hiding this comment.
Thank you for the updates @mcruzdev
A couple of suggestions, to avoid creating a redundant stack trace.
vertx-web/src/main/java/io/vertx/ext/web/impl/RoutingContextImpl.java
Outdated
Show resolved
Hide resolved
vertx-web/src/main/java/io/vertx/ext/web/impl/RoutingContextImpl.java
Outdated
Show resolved
Hide resolved
mcruzdev
force-pushed
the
no-authority-msg
branch
from
3409a3e
to
aa58a59
Compare
tsegismont
reviewed
Nov 18, 2024
vertx-web/src/main/java/io/vertx/ext/web/impl/RoutingContextImpl.java
Outdated
Show resolved
Hide resolved
tsegismont
reviewed
Nov 18, 2024
vertx-web/src/main/java/io/vertx/ext/web/impl/RoutingContextImpl.java
Outdated
Show resolved
Hide resolved
tsegismont
reviewed
Nov 18, 2024
vertx-web/src/main/java/io/vertx/ext/web/impl/RoutingContextImpl.java
Outdated
Show resolved
Hide resolved
|
Can you please rebase the PR? The fixes for the build failures have been pushed to the main branch. Thanks |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation:
On Quarkus side we have the following issue quarkusio/quarkus#43936, the idea here is to improve the message error.