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

TCK: Dispatch forward and includes attributes do not meet the spec #9074

Merged

Conversation

janbartel
Copy link
Contributor

@janbartel janbartel commented Dec 19, 2022

Fix several spec non-compliances with forward and include attribute handling:

  • Section 9.3.1 of the spec says the target of a named dispatcher that is an include must not have the jakarta.servlet.include.* attributes set.
  • Section 9.4.2 of the spec says that the target of a named dispatcher forward must not have the jakarta.servlet.forward.* attributes set.
  • Section 9.4.2 of the spec says that a forward must report the values for the jakarta.servlet.forward.* attributes as the equivalent values from the original request (ie getRequestURI getPathInfo getQueryString etc etc) that went into the servlet call chain.

Added tests for the above.

@janbartel janbartel added Bug For general bugs on Jetty side TCK For various Specification Test Compatibility Kits (eg: Servlet, WebSocket, HTTP/2, etc) Jetty 12 labels Dec 19, 2022
@janbartel janbartel changed the title TCK: Named dispatch includes must not have include attributes set. TCK: Dispatch forward and includes attributes do not meet the spec Dec 20, 2022
@janbartel
Copy link
Contributor Author

@lachlan-roberts need your review on this. Pay particular attention to the changes to solve point 3 (ie reporting the values from the original request) - is there a better solution here?

/**
* Name of original request attribute
*/
public static final String __ORIGINAL_REQUEST = "org.eclipse.jetty.originalRequest";
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe put ee10 in the name to protect against future cross context dispatches between ee10, ee11 and then back to ee10.

Copy link
Contributor

@lachlan-roberts lachlan-roberts left a comment

Choose a reason for hiding this comment

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

Where do you set the __ORIGINAL_REQUEST attribute?

I would have thought it would be made available by ServletApiRequest#getAttribute or something similar.

@gregw
Copy link
Contributor

gregw commented Dec 24, 2022

Where do you set the __ORIGINAL_REQUEST attribute?

I would have thought it would be made available by ServletApiRequest#getAttribute or something similar.

@lachlan-roberts
It is in the code:

                case __ORIGINAL_REQUEST:
                    HttpServletRequest originalRequest = (HttpServletRequest)super.getAttribute(name);
                    return originalRequest == null ? _httpServletRequest : originalRequest;

So the inner most forwarded wrapper works out that it is the innermost by getting a null value for the super.getAttribute, thus it returns it's wrapped request as the original.
All outer forward wrappers will see this non null value and pass it up.

@lachlan-roberts
Copy link
Contributor

@gregw ok, so its not really the original request but the first forwarded request.

@gregw
Copy link
Contributor

gregw commented Dec 24, 2022

@gregw ok, so its not really the original request but the first forwarded request.

It's the request wrapped by the first forwarded request, so that is either the original request or something that wraps the original request in a non consequential way.

@joakime joakime added this to the 12.0.x milestone Jan 2, 2023
@janbartel janbartel merged commit 012e7c7 into jetty-12.0.x Jan 12, 2023
@janbartel janbartel deleted the jetty-12.0.x-named-dispatch-include-attributes branch January 12, 2023 01:37
gregpoulos pushed a commit to gregpoulos/jetty.project that referenced this pull request Jan 16, 2023
…x-document-modules

* upstream/jetty-12.0.x:
  Issue jetty#9167 - making assumption in flaky test
  jetty 12.0.x cleanup duplicate osgi pom metadata (jetty#9093)
  Jetty 12 - Add tests in util/resource for alternate FileSystem implementations (jetty#9149)
  Cleanup non-retainable `Retainable`s (jetty#9159)
  Fixes retainability of special Chunks (jetty#9073)
  TCK: Dispatch forward and includes attributes do not meet the spec (jetty#9074)
  re-enable h3 tests (jetty#8773)
  More fundamental test case
  Reorganization of jetty-client classes. (jetty#9127)
  Removing @disabled from SslUploadTest
  Removing @disabled from jetty-start
  jetty#9134 added test
  ee10: DefaultServlet: Replace checks for isStreaming() by !isWriting()
  jetty#9078 make HttpContent.getByteBuffer() implementations return new ByteBuffer instances and document that contract
  Fixes jetty#9141 - Thread-safe Content.Chunk#slice (jetty#9142)
  Remove `@Disabled` from `jetty-jmx` (jetty#9143)
  Bump maven.version from 3.8.6 to 3.8.7
  Bump maven.version from 3.8.6 to 3.8.7
gregpoulos pushed a commit to gregpoulos/jetty.project that referenced this pull request Jan 16, 2023
… into jetty-12.0.x-documentation-lowercase-jetty_home-attribute

* 'jetty-12.0.x' of https://github.com/eclipse/jetty.project:
  Issue jetty#9167 - making assumption in flaky test
  jetty 12.0.x cleanup duplicate osgi pom metadata (jetty#9093)
  Jetty 12 - Add tests in util/resource for alternate FileSystem implementations (jetty#9149)
  Cleanup non-retainable `Retainable`s (jetty#9159)
  Fixes retainability of special Chunks (jetty#9073)
  TCK: Dispatch forward and includes attributes do not meet the spec (jetty#9074)
  re-enable h3 tests (jetty#8773)
  More fundamental test case
  Reorganization of jetty-client classes. (jetty#9127)
  Removing @disabled from SslUploadTest
  Removing @disabled from jetty-start
  Bump maven.version from 3.8.6 to 3.8.7
  Bump maven.version from 3.8.6 to 3.8.7
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 TCK For various Specification Test Compatibility Kits (eg: Servlet, WebSocket, HTTP/2, etc)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants