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

Extract Netty specific RequestFilter TCK test #9387

Closed
wants to merge 1 commit into from

Conversation

timyates
Copy link
Contributor

@timyates timyates commented Jun 5, 2023

This test requires FullBodyRequest support which doesn't exist in Servlet.

Introduced here #9353

To avoid having to exclude all of the Request Filter tests from the Servlet TCK, we have extracted this single test into it's own specification.

See micronaut-projects/micronaut-servlet#470 for discussion.

This test requires FullBodyRequest support which doesn't exist in Servlet.

Introduced here #9353

To avoid having to exclude all of the Request Filter tests from the Servlet TCK,
we have extracted this single test into it's own specification.

See micronaut-projects/micronaut-servlet#470 for discussion.
@timyates timyates requested review from sdelamo and yawkat June 5, 2023 12:04
@timyates timyates self-assigned this Jun 5, 2023
@sonarqubecloud
Copy link

sonarqubecloud bot commented Jun 5, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@yawkat yawkat requested a review from graemerocher June 5, 2023 12:13
@sdelamo
Copy link
Contributor

sdelamo commented Jun 5, 2023

what does it mean to be a Full HTTP request?

@yawkat
Copy link
Member

yawkat commented Jun 5, 2023

that the full body is immediately available for reading

@timyates
Copy link
Contributor Author

timyates commented Jun 5, 2023

I guess the more correct fix is to implement the required parts of #9353 in Servlet, so the full TCK passes there too...

@yawkat
Copy link
Member

yawkat commented Jun 5, 2023

the test doesn't even work all the time in netty, it's best effort. if you used a larger body it would fail.

(unless you enable FULL_CONTENT mode, which the test doesn't)

@graemerocher
Copy link
Contributor

don't think we should do this, this interface should be simply to implement in servlet

@graemerocher
Copy link
Contributor

extracting the test is probably fine tho

@yawkat
Copy link
Member

yawkat commented Jun 5, 2023

@graemerocher i dont see a sensible way to implement it though

@graemerocher
Copy link
Contributor

servlet exposes getInputStream() on the request. So you would have a ByteBuffer implementation that backs onto the this InputStream implementation and resets the input stream to the start position after the bytes are read (which is how any servlet filter that reads the body would have to be implemented)

@graemerocher
Copy link
Contributor

actually I think resetting the input stream will probably not work, it would need to be implemented as a wrapper, and cache the bytes a bit like https://github.com/spring-projects/spring-framework/blob/main/spring-web/src/main/java/org/springframework/web/util/ContentCachingRequestWrapper.java

@yawkat
Copy link
Member

yawkat commented Jun 5, 2023

it would essentially require something like full_content though. we can't risk blocking in the body annotation binder, so we can't block in FullHttpRequest.contents

@graemerocher
Copy link
Contributor

a servlet container is not like Netty and always has the full content available from the request

@yawkat
Copy link
Member

yawkat commented Jun 5, 2023

not always, the stream is still lazy

@graemerocher
Copy link
Contributor

the way you toggle that off and on is by enabling/disabling async request processing. This would be the equivalent of full_content

@yawkat
Copy link
Member

yawkat commented Jun 5, 2023

the body is not necessarily fully present no matter whether youre using async mode or not in the servlet.

@graemerocher
Copy link
Contributor

how so? It is available via getInputStream() is it not? Reading the body is a servlet filter is technically possible as demonstrated by the range of other implementations that exist out there including the one referenced above.

@yawkat
Copy link
Member

yawkat commented Jun 5, 2023

but reading the stream may block while the netty implementation never will. theres no api to know whether it will block or not

@graemerocher
Copy link
Contributor

if you disable asynchronous request processing you already assume the code that is going to be executed is blocking

@timyates
Copy link
Contributor Author

timyates commented Jun 5, 2023

I got this together as a POC

Not sure it's right... PTAL: micronaut-projects/micronaut-servlet#471

@timyates
Copy link
Contributor Author

timyates commented Jun 6, 2023

Closing this now, as it's superseded by micronaut-projects/micronaut-servlet#471

@timyates timyates closed this Jun 6, 2023
@timyates timyates deleted the full-body-tck-test-extraction branch June 6, 2023 14:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants