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.

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

11 -> 12 Migration guide wrongly suggests Request.getHttpURI as replacement for HttpServletRequest.getRequestURL #11911

Closed
mperktold opened this issue Jun 12, 2024 · 0 comments · Fixed by #11916
Labels
Bug For general bugs on Jetty side

Comments

@mperktold
Copy link

Jetty version(s)
Jetty 12.0.10

Jetty Environment
core, docs

Description
Both the migration guides in the docs as well as this code sample mention that Request.getHttpURI replaces HttpServletRequest.getRequestURL. However, they are not identical: Request.getHttpURI also includes the query part, while HttpServletRequest.getRequestURL doesn't. This becomes obvious when looking at Jetty's servlet API implementation, which strips the query part before returning the URL:

public StringBuffer getRequestURL()
{
// Use the ServletContextRequest here as even if changed in the Request, it must match the servletPath and pathInfo
return new StringBuffer(HttpURI.build(getRequest().getHttpURI()).query(null).asString());
}

This might not make a difference in many cases, but we did have a case where this incorrect migration introduced a bug, so it might be worth pointing out in the migration guides.

@mperktold mperktold added the Bug For general bugs on Jetty side label Jun 12, 2024
janbartel added a commit that referenced this issue Jun 14, 2024
* Issue #11911 Fix documentation example for Request.getHttpUri
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug For general bugs on Jetty side
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant