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

wip: Attempt to get FullHttpRequest support in Servlet requests #471

Merged
merged 5 commits into from
Jun 7, 2023

Conversation

timyates
Copy link
Contributor

@timyates timyates commented Jun 5, 2023

No description provided.

@Override
public ByteBuffer<?> contents() {
if (bodyIsReadAsync) {
throw new IllegalStateException("Cannot fetch body content of an asynchronous request");
Copy link
Member

Choose a reason for hiding this comment

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

should return null on failure (bufferContents should error though)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Like this? 0bc7ca8

@timyates timyates marked this pull request as ready for review June 6, 2023 12:35
public ExecutionFlow<ByteBuffer<?>> bufferContents() {
ByteBuffer<?> contents = contents();
if (contents == null) {
throw new IllegalStateException("Cannot buffer contents asynchronously");
Copy link
Member

Choose a reason for hiding this comment

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

i changed my mind on this, please return null in this case (null, not ExecutionFlow.just(null)). I will adjust the supermethod in core accordingly.

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
throw new IllegalStateException("Cannot buffer contents asynchronously");
return null;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

return ExecutionFlow.just(contents);
}

private final class ServletByteBuffer<T> implements ByteBuffer<T> {
Copy link
Contributor

Choose a reason for hiding this comment

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

should not we move this class to servlet-core so that we are able to use it in the serverless implementations?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe extract it into its own file and annotate it with @Experimental

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 1bb046f

Which isn't showing up in the PR for unknown reasons... github status is lying I suspect

@timyates timyates requested a review from sdelamo June 7, 2023 16:28
@sonarqubecloud
Copy link

sonarqubecloud bot commented Jun 7, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

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

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@sdelamo sdelamo merged commit 8dc90d0 into master Jun 7, 2023
@sdelamo sdelamo deleted the full-body-support branch June 7, 2023 17:19
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
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants