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

Implemented servlet 6.1 redirect with content #11743

Merged
merged 15 commits into from
May 20, 2024
Merged

Conversation

gregw
Copy link
Contributor

@gregw gregw commented May 3, 2024

Added option for server to generate a short html redirect body content, as per RFC9110 (default false) Allowed an aggregated servlet response content to be used if clear is false.

Added option for server to generate a short html redirect body content, as per RFC9110 (default false)
Allowed an aggregated servlet response content to be used if clear is false.
Copy link
Contributor

@janbartel janbartel left a comment

Choose a reason for hiding this comment

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

Where's the implementation of this line of the javadoc for the new sendRedirect method:

This method has no effect if called from an include.

@gregw gregw requested a review from janbartel May 3, 2024 13:02
This style of extensibility (calling virtuals from constructors) is very fragile.
@gregw gregw requested review from janbartel and joakime May 4, 2024 23:30
gregw added 3 commits May 5, 2024 09:45
Also update EE10 to also noop included response methods
Also update EE10 to also noop included response methods
@gregw
Copy link
Contributor Author

gregw commented May 6, 2024

@sbordet @janbartel @lachlan-roberts nudge!

Copy link
Contributor

@janbartel janbartel left a comment

Choose a reason for hiding this comment

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

Made a suggestion to clarify exactly how the CrossContextInclude wrapper is used. Other than that, looks fine, so +1

@gregw
Copy link
Contributor Author

gregw commented May 6, 2024

The CI failure is due to the EE1 JSP artifact not being released yet. Builds locally OK.

…ee11/servlet/ServletApiResponse.java

Co-authored-by: Jan Bartel <janb@webtide.com>
@joakime
Copy link
Contributor

joakime commented May 6, 2024

The CI failure is due to the EE1 JSP artifact not being released yet. Builds locally OK.

I went ahead and released 11.0.0-M19 of the mortbay jsp-impl jars.
And updated this branch to use them.

joakime
joakime previously requested changes May 6, 2024
Copy link
Contributor

@joakime joakime left a comment

Choose a reason for hiding this comment

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

The 304 status code special case seems to be in the wrong place.
Also the other parts of the spec related to 3xx and response bodies say that if there's no response body, there is also no trailers produced.

*/
@Deprecated
Copy link
Contributor

Choose a reason for hiding this comment

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

Orthogonal change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes sorry. We have not been reviewing 12.1 changes unless necessary. I thought the redirect changes needed review, but some other servlet 6.1 changes got swept up.

@gregw gregw requested a review from sbordet May 16, 2024 07:07
@gregw
Copy link
Contributor Author

gregw commented May 17, 2024

@sbordet nudge

…/Response.java

Co-authored-by: Simone Bordet <simone.bordet@gmail.com>
@gregw gregw requested a review from sbordet May 19, 2024 21:45
@gregw gregw merged commit 4c1c6a2 into jetty-12.1.x May 20, 2024
10 checks passed
@joakime joakime deleted the jetty-12.1.0/redirect branch June 4, 2024 18:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants