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

Pass special content to interceptors #7296

Merged

Conversation

lorban
Copy link
Contributor

@lorban lorban commented Dec 16, 2021

Modify the AsyncContentProducer accordingly and update GzipHttpInputInterceptor to correctly handle the existing HttpInput.Content contract.

I have not modified the special contents (error and EOF) to make them return an empty buffer when getByteBuffer() is called as IMHO the Content contract already is clear on that: calling getByteBuffer() when isSpecial() returns true does throw ISE.

So IMHO if there is some code out there that calls Content.getByteBuffer() without first checking the special flag, it is in breach of the contract as it was documented since 10.0.0; the fact that we never passed special content to interceptors should be irrelevant.

@lorban lorban force-pushed the jetty-10.0.x-7281-interceptors-pass-special-content branch from c9e9cd7 to d4445b6 Compare December 16, 2021 13:38
@lorban lorban marked this pull request as ready for review December 16, 2021 15:48
@lorban lorban requested review from sbordet and gregw December 16, 2021 15:48
@lorban lorban self-assigned this Dec 16, 2021
@lorban lorban added the Bug For general bugs on Jetty side label Dec 16, 2021
@lorban lorban linked an issue Dec 16, 2021 that may be closed by this pull request
Copy link
Contributor

@gregw gregw left a comment

Choose a reason for hiding this comment

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

Can we add and example to the javadoc somewhere like:

@Override
public HttpInput.Content readFrom(HttpInput.Content content) {
    if (content.hasContent())
        processContent(content.getByteBuffer());
    if (content.isEof())
        endProcessing();
    return processedContent();
}

@lorban
Copy link
Contributor Author

lorban commented Dec 17, 2021

@gregw I've added some javadoc to the Interceptor interface, including some code examples and a reference to GzipHttpInputInterceptor.

@lorban lorban requested a review from gregw December 17, 2021 08:22
@lorban lorban force-pushed the jetty-10.0.x-7281-interceptors-pass-special-content branch from c7f16da to 1844b7d Compare December 17, 2021 08:27
Copy link
Contributor

@sbordet sbordet left a comment

Choose a reason for hiding this comment

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

Please fix redundant return.

@lorban lorban requested a review from sbordet December 21, 2021 11:27
Copy link
Contributor

@sbordet sbordet left a comment

Choose a reason for hiding this comment

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

I see 3 variables here:

  • what type of input content (some content, empty content, EOF content, error content)
  • what the interceptor does with the input content (full consume, partial consume, no consume)
  • what type of output content (null, empty, non-empty, EOF, error, throw)

I would like, if possible, to have tests that cover all combinations of the 3 variables above.
I'd be happy to see tests for the most common cases.
Otherwise I'm good.

@lorban lorban force-pushed the jetty-10.0.x-7281-interceptors-pass-special-content branch 2 times, most recently from da59afc to 18a1179 Compare January 10, 2022 09:02
@lorban lorban requested a review from sbordet January 10, 2022 09:03
sbordet
sbordet previously approved these changes Jan 11, 2022
@gregw
Copy link
Contributor

gregw commented Jan 11, 2022

Doh! forced push!!!!! I can't review just the changes since my last review!!!!

gregw
gregw previously approved these changes Jan 12, 2022
Signed-off-by: Ludovic Orban <lorban@bitronix.be>
Signed-off-by: Ludovic Orban <lorban@bitronix.be>
Signed-off-by: Ludovic Orban <lorban@bitronix.be>
Signed-off-by: Ludovic Orban <lorban@bitronix.be>
Signed-off-by: Ludovic Orban <lorban@bitronix.be>
… interceptor and clarify its javadoc

Signed-off-by: Ludovic Orban <lorban@bitronix.be>
Signed-off-by: Ludovic Orban <lorban@bitronix.be>
Signed-off-by: Ludovic Orban <lorban@bitronix.be>
@lorban lorban dismissed stale reviews from gregw and sbordet via df1f1b6 January 13, 2022 08:02
@lorban lorban force-pushed the jetty-10.0.x-7281-interceptors-pass-special-content branch from 18a1179 to df1f1b6 Compare January 13, 2022 08:02
@lorban lorban requested review from sbordet and gregw January 13, 2022 08:03
@lorban lorban merged commit f0810c0 into jetty-10.0.x Jan 13, 2022
@lorban lorban deleted the jetty-10.0.x-7281-interceptors-pass-special-content branch January 13, 2022 14:13
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

EOFs are not passed to interceptors any more - shouldn't they?
5 participants