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

Ensure reading request body does not drain the input stream if stream is resettable #1537

Merged
merged 5 commits into from
Aug 22, 2024

Conversation

Ndiritu
Copy link
Contributor

@Ndiritu Ndiritu commented Aug 19, 2024

We currently override the Request body's writeTo method which was not resetting the input stream after the body is written e.g. by a logging interceptor. The network call would find an empty request body.

This PR determines if the request body is resettable & configures isOneShot() to prevent non-resettable streams from being drained by logging middleware.

closes microsoftgraph/msgraph-sdk-java#2037

@Ndiritu Ndiritu force-pushed the fix/request-body-write branch from 418176a to cabb0fd Compare August 19, 2024 15:33
@Ndiritu Ndiritu force-pushed the fix/request-body-write branch 2 times, most recently from a777477 to 67b084f Compare August 21, 2024 18:06
@baywet
Copy link
Member

baywet commented Aug 21, 2024

for others who might be following along:

  • the logging interceptor is draining the stream
  • when the transport picks it up, it's already drained
  • the main reason it's most likely not doing a buffered copy is because they instead leverage the "isOneShot" method on the body to avoid draining the request body if it shouldn't.
  • Our implementation needs to implement that method in the request adapter, based on whether the underlying stream is markable or not
  • additionally, when the underlying stream is markable, in the writeTo method, we need to reset/mark it to zero
  • This option is much better in terms of performance than buffering everything for this edge case.
  • It's also much better in terms of UX than giving a setting on the request adapter to buffer ONLY when requested.

@Ndiritu Ndiritu force-pushed the fix/request-body-write branch from fd88bfa to 6c2d170 Compare August 22, 2024 12:02
@Ndiritu Ndiritu changed the title Update request body write to retain inputstream buffer Ensure reading request body does not drain the input stream if stream is not resettable Aug 22, 2024
@Ndiritu Ndiritu marked this pull request as ready for review August 22, 2024 12:06
@Ndiritu Ndiritu requested a review from a team as a code owner August 22, 2024 12:06
@Ndiritu Ndiritu requested a review from baywet August 22, 2024 12:06
baywet
baywet previously approved these changes Aug 22, 2024
Copy link
Member

@baywet baywet left a comment

Choose a reason for hiding this comment

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

Thank you for making the changes!
A version bump, a changelog entry and we should be good to go!

baywet
baywet previously approved these changes Aug 22, 2024
@Ndiritu
Copy link
Contributor Author

Ndiritu commented Aug 22, 2024

Thank you for making the changes! A version bump, a changelog entry and we should be good to go!

Ah no release please yet :)
On it.

@Ndiritu Ndiritu force-pushed the fix/request-body-write branch from fb23874 to 2c3191f Compare August 22, 2024 12:29
@Ndiritu Ndiritu requested a review from baywet August 22, 2024 12:29
@Ndiritu Ndiritu enabled auto-merge August 22, 2024 12:29
Copy link

@Ndiritu Ndiritu merged commit 6b5fde9 into main Aug 22, 2024
9 checks passed
@Ndiritu Ndiritu deleted the fix/request-body-write branch August 22, 2024 12:40
@baywet
Copy link
Member

baywet commented Aug 22, 2024

@Ndiritu the tag also needs to be manually created on this repo for the time being. I'll let you take care of that :)

@mkomko
Copy link

mkomko commented Aug 22, 2024

@Ndiritu @baywet Thanks a lot guys! ❤️

@Ndiritu Ndiritu changed the title Ensure reading request body does not drain the input stream if stream is not resettable Ensure reading request body does not drain the input stream if stream is resettable Dec 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

0-byte body on sdk 6.12 with android when using HttpLoggingInterceptor
3 participants