-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Fix Quarkus REST issue where HEAD returned 405 #49221
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This comment has been minimized.
This comment has been minimized.
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 is probably correct, but I find the repeated calls to findRequestMatch
hard to read, in two cases it looks like we may be invoking it several times in a row with the same parameters. Perhaps there's a way to rewrite it more clearly?
boolean hadNullMethodMapper = false; | ||
if (mapper == null) { | ||
mapper = target.get(null); //another layer of resource locators maybe | ||
// we set this without checking if we matched, but we only use it after | ||
// we check for a null mapper, so by the time we use it, it must have meant that | ||
// we had a matcher for a null method | ||
hadNullMethodMapper = true; | ||
res = findRequestMatch(mapper, requestContext); |
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.
Here it seems we're invoking findRequestMatch()
again discarding the first value of res
, but probably this only happens when mapper
was null
so res
was also null
. It makes it a bit hard to read, though.
RequestMapper.RequestMatch<RuntimeResource> res = mapper | ||
.map(requestContext.getRemaining().isEmpty() ? "/" : requestContext.getRemaining()); | ||
if (res == null) { | ||
res = findRequestMatch(mapper, requestContext); |
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 again looks like we could invoke findRequestMatch()
twice with the same parameters, if it returned null
the first time.
2ceaf35
to
874aaae
Compare
@FroMage Thanks for the review! I've tried to make it that I've rewrote this block a bit. It is more changes in the PR, but hopefully now it is more clear that |
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.
Thanks, that's easier to understand :)
This comment has been minimized.
This comment has been minimized.
The CI failures are likely unrelated to the PR as similar failures can be seen in other PRs as well |
That's a lot of failures, though 😲 |
closes: quarkusio#49172 Signed-off-by: mposolda <[email protected]>
874aaae
to
0702c4a
Compare
Status for workflow
|
Uh oh!
There was an error while loading. Please reload this page.