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

Don't print non-utf8 bodies #125

Merged

Conversation

xd009642
Copy link
Contributor

@xd009642 xd009642 commented Aug 4, 2023

A lot of our services take in multipart data or binary data in the HTTP body and when a wiremock predicate fails megabytes of binary data get printed out to the console. Here I address this by not printing out a string if it isn't valid utf8 and instead printing out the body length.

Another alternative may be to put in an upper limit to how large a body will be printed and if it exceeds that size maybe saving the request to a file for later analysis. But this change was so simple I figured I'd open the PR first to start the discussion.

Issue: #124

A lot of our services take in multipart data or binary data in the HTTP
body and when a wiremock predicate fails megabytes of binary data get
printed out to the console. Here I address this by not printing out a
string if it isn't valid utf8 and instead printing out the body length.

Another alternative may be to put in an upper limit to how large a body
will be printed and if it exceeds that size maybe saving the request to
a file for later analysis. But this change was so simple I figured I'd
open the PR first to start the discussion.
@xd009642 xd009642 force-pushed the better-ux-for-binary-bodies branch from e75b13a to 1d4d604 Compare August 4, 2023 11:35
@xd009642
Copy link
Contributor Author

xd009642 commented Aug 4, 2023

I am slightly erring towards just putting a if self.body.len() > THRESHOLD and doing something else instead and keeping old from_utf8_lossy behaviour for body <= THRESHOLD.

Edit: Remove mention of cloning forgot about std::str::from_utf8

@LukeMathWalker
Copy link
Owner

I think it's a good idea to tweak the default behaviour.
I'd suggest:

  • Having a default size threshold
  • Only printing the body if it is UTF8

We can then provide a knob in the server config to bypass these checks and print the body anyway. It should also be possible to do it via an env variable, which should be mentioned in the printed message when the body is not outputted in full.

Wdyt?

@xd009642
Copy link
Contributor Author

xd009642 commented Aug 4, 2023

Sure sounds good to me, I'll add the env var, default threshold and method to configure it 👍

1. Setting limit via env var or builder
2. Having a default length limit
3. Printed suggestion on upping the limit
@xd009642
Copy link
Contributor Author

xd009642 commented Aug 4, 2023

Okay the way I've implemented this is by passing it from the builder, adding as a field in the request store and request and setting it accordingly. I didn't see any other env var configs in the code via a quick grep so just did something that felt right (though it could log invalid env var values).

Didn't add in examples to docs or flesh them out too much as the code may change if you don't like the approach, just enough docs to illustrate my intent and not take up too much thinking.

src/request.rs Outdated Show resolved Hide resolved
src/request.rs Outdated Show resolved Hide resolved
@xd009642
Copy link
Contributor Author

xd009642 commented Sep 6, 2023

Applied all the feedback, ready for another round of review 😄

src/request.rs Outdated Show resolved Hide resolved
src/request.rs Outdated Show resolved Hide resolved
Co-authored-by: Luca Palmieri <[email protected]>
@xd009642
Copy link
Contributor Author

xd009642 commented Oct 5, 2023

One note I see the coverage job failed and you're using actions-rs/tarpaulin, I generally wouldn't use it because it's unmaintained abandonware and since tarpaulin has windows and mac releases the release tarball name has changed. cargo-binstall is generally my recommended way of using it in CI 👀

@LukeMathWalker
Copy link
Owner

Yeah, I need to update the CI pipeline 😅

@LukeMathWalker LukeMathWalker merged commit 698cd76 into LukeMathWalker:main Nov 3, 2023
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.

2 participants