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.

Sign up for GitHub

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

Jump to bottom

Implemented servlet 6.1 redirect with content #11743

Merged
merged 15 commits into from
May 20, 2024
Merged

Implemented servlet 6.1 redirect with content #11743

merged 15 commits into from
May 20, 2024

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.

@gregw
Implemented servlet 6.1 redirect with content
cc29db0 
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.
janbartel
janbartel requested changes May 3, 2024
View reviewed changes
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
@gregw
Fixed init order
71f378b 
This style of extensibility (calling virtuals from constructors) is very fragile.
janbartel
janbartel requested changes May 4, 2024
View reviewed changes
...ee11/jetty-ee11-servlet/src/main/java/org/eclipse/jetty/ee11/servlet/ServletApiResponse.java Show resolved Hide resolved
...ee11/jetty-ee11-servlet/src/main/java/org/eclipse/jetty/ee11/servlet/ServletApiResponse.java Show resolved Hide resolved
...ee11/jetty-ee11-servlet/src/main/java/org/eclipse/jetty/ee11/servlet/ServletApiResponse.java Show resolved Hide resolved
...ee11/jetty-ee11-servlet/src/main/java/org/eclipse/jetty/ee11/servlet/ServletApiResponse.java Show resolved Hide resolved
...ee11/jetty-ee11-servlet/src/main/java/org/eclipse/jetty/ee11/servlet/ServletApiResponse.java Show resolved Hide resolved
...ee11/jetty-ee11-servlet/src/main/java/org/eclipse/jetty/ee11/servlet/ServletApiResponse.java Show resolved Hide resolved
...ee11/jetty-ee11-servlet/src/main/java/org/eclipse/jetty/ee11/servlet/ServletApiResponse.java Show resolved Hide resolved
...ee11/jetty-ee11-servlet/src/main/java/org/eclipse/jetty/ee11/servlet/ServletApiResponse.java Show resolved Hide resolved
gregw added 2 commits May 5, 2024 08:54
@gregw gregw requested review from janbartel and joakime May 4, 2024 23:30
gregw added 3 commits May 5, 2024 09:45
@gregw
Update javadoc from review
b5ffbfa 
Also update EE10 to also noop included response methods
@gregw
Update javadoc from review
44b0a14 
Also update EE10 to also noop included response methods
@gregw
Merge branch 'jetty-12.1.x' into jetty-12.1.0/redirect
4c1e036
@gregw
Copy link
Contributor Author

gregw commented May 6, 2024

@sbordet @janbartel @lachlan-roberts nudge!

janbartel
janbartel approved these changes May 6, 2024
View reviewed changes
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

...ee11/jetty-ee11-servlet/src/main/java/org/eclipse/jetty/ee11/servlet/ServletApiResponse.java Outdated Show resolved Hide resolved
@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.

@gregw @janbartel
Update jetty-ee11/jetty-ee11-servlet/src/main/java/org/eclipse/jetty/…
2d534f6 
…ee11/servlet/ServletApiResponse.java

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

joakime commented May 6, 2024 •
edited
Loading

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
View reviewed changes
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.

jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Response.java Show resolved Hide resolved
jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpStatus.java Outdated Show resolved Hide resolved
jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Response.java Show resolved Hide resolved
sbordet
sbordet requested changes May 8, 2024
View reviewed changes
jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpStatus.java Outdated Show resolved Hide resolved
jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/ConnectionMetaData.java Outdated
*/
@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.

jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Response.java Show resolved Hide resolved
jetty-ee11/jetty-ee11-servlet/src/main/java/org/eclipse/jetty/ee11/servlet/PushBuilderImpl.java Outdated Show resolved Hide resolved
...-ee11/jetty-ee11-servlet/src/main/java/org/eclipse/jetty/ee11/servlet/ServletApiRequest.java Outdated Show resolved Hide resolved
@gregw gregw requested review from joakime, sbordet and janbartel May 8, 2024 22:14
@gregw gregw requested a review from sbordet May 16, 2024 07:07
@gregw
Copy link
Contributor Author

gregw commented May 17, 2024

@sbordet nudge

@gregw gregw dismissed joakime’s stale review May 17, 2024 00:39

on vacation

@gregw @sbordet
Update jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server…
ac86884 
…/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
@olamy olamy mentioned this pull request Sep 29, 2024
Merged
@olamy olamy mentioned this pull request Oct 17, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sbordet sbordet sbordet approved these changes

@lachlan-roberts lachlan-roberts Awaiting requested review from lachlan-roberts

@joakime joakime Awaiting requested review from joakime

@janbartel janbartel Awaiting requested review from janbartel

Assignees
No one assigned
Labels
None yet
Projects
Jetty 12.1.0
Status: ✅ Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@gregw @joakime @janbartel @sbordet