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

Proper handling of chunked input streams in LoggingInterceptor #4753

Merged
merged 2 commits into from
Mar 31, 2021

Conversation

dkBrazz
Copy link

@dkBrazz dkBrazz commented Mar 22, 2021

Fixes unexpected entity cutting in logs
#4034, #3306 and #3281

@dkBrazz dkBrazz force-pushed the fix-chunked-logging-truncation branch from 761d330 to 3d74396 Compare March 23, 2021 10:29
Signed-off-by: Denis Kurochkin <[email protected]>
@jansupol
Copy link
Contributor

The PR looks good. What it does, though, enables to read all the chunks from the chunked stream. That is assumingly the preferred behaviour. I can imagine a case where people do not want to log all the chunks and want to log just the first one, the way it worked now. Perhaps for performance reasons, or to keep the log small.
@senivam wdyt? Should we add a possibility to turn off logging all the chunks by the new LoggingFeature builder you created?

@dkBrazz
Copy link
Author

dkBrazz commented Mar 31, 2021

a case where people do not want to log all the chunks and want to log just the first one, the way it worked now. Perhaps for performance reasons, or to keep the log small.

I think existing maxEntitySize should be enough to cover it

Copy link
Contributor

@senivam senivam left a comment

Choose a reason for hiding this comment

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

tests, provided in the PR prove that maxEntitySize works as designed, so, functionality with restricted entity size is not affected. For me it looks good.

@senivam senivam merged commit 6b87643 into eclipse-ee4j:master Mar 31, 2021
@senivam senivam added this to the 2.34 milestone Mar 31, 2021
senivam pushed a commit to senivam/jersey that referenced this pull request Apr 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants