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

Fix issues with Proxy mode and Binary Request Bodies #334

Merged
merged 9 commits into from
Sep 1, 2019
Merged

Fix issues with Proxy mode and Binary Request Bodies #334

merged 9 commits into from
Sep 1, 2019

Conversation

andi0b
Copy link
Contributor

@andi0b andi0b commented Aug 26, 2019

We used Wiremocks.NET in Proxy mode to sometimes test against the real API and sometimes just mock it. This stopped working when we started POSTing JPEGs. The body was altered because WireMocks.NET started to parse the binary content as UTF8 and converted it back. This causes issues with binary contents, you can see it because the length changes.

This pull request contains:

  • Test to show this issue
  • Fix for this issue (by replacing Encoding.UTF8 with another UTF8 decoder which throws Exceptions on unmappable characters, so the code can fallback to binary parsing)
  • Another fix in RequestMessageBodyMatcher I discovered because my test didn't work correctly. (Enable binary matching also if the content is detected as string or JSON).

@codecov
Copy link

codecov bot commented Aug 26, 2019

Codecov Report

Merging #334 into master will increase coverage by 0.09%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #334      +/-   ##
==========================================
+ Coverage   80.16%   80.25%   +0.09%     
==========================================
  Files         111      111              
  Lines        4442     4442              
==========================================
+ Hits         3561     3565       +4     
+ Misses        881      877       -4
Impacted Files Coverage Δ
....Net/Matchers/Request/RequestMessageBodyMatcher.cs 100% <100%> (+1.42%) ⬆️
src/WireMock.Net/Util/BodyParser.cs 96.87% <100%> (+3.12%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8c9a51c...3ff44ad. Read the comment docs.

Copy link
Collaborator

@StefH StefH left a comment

Choose a reason for hiding this comment

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

Thanks for the PR.

Can you please take a look at my comments?

@andi0b
Copy link
Contributor Author

andi0b commented Aug 26, 2019

@StefH

Thanks for the PR.

Can you please take a look at my comments?

I agree with your comments, I will do it as suggested.

@StefH StefH added the bug label Aug 27, 2019
@andi0b
Copy link
Contributor Author

andi0b commented Sep 1, 2019

@StefH Please review my changes. I added some test coverage, but I have to admit I don't fully understand the multipart thing in BodyParser.

I'm not used to a pull request workflow, is there anything else I should do? Merge sth?

@StefH
Copy link
Collaborator

StefH commented Sep 1, 2019

Hello @andi0b,

  1. It's better to make changes in a separate branch, and then make a PR to merge to my master. In this way, I can also get your branch and make adjustments if needed.

  2. There is a merge conflict in BodyParser.cs
    To solve this, you must get latest master from my project en solve merge conflicts.

  3. You cannot merge. Once the code is ok, and all my merge comments are resolved, then I can merge this to master.

@StefH
Copy link
Collaborator

StefH commented Sep 1, 2019

  1. Please check you whitespace setting in Visual Studio.
    I use 4 spaces, and never tabs.

If you use different, a lot of code changes will be displayed.

@andi0b
Copy link
Contributor Author

andi0b commented Sep 1, 2019

1. Please check you whitespace setting in Visual Studio.
   I use 4 spaces, and never tabs.

Yeah, actually I use Rider with auto detect of code style (as there is no .editorconfig), and it's using 4 spaces, no idea what happened there... maybe it tried to outsmart all of us.

But there is also something weird with this change: c0a43ed#diff-78e158879161b0265ca8030bed233919

# Conflicts:
#	src/WireMock.Net/Util/BodyParser.cs
@andi0b
Copy link
Contributor Author

andi0b commented Sep 1, 2019

already solved it, see next commit

I don't really know what to do about the failing test FluentMockServer_Should_exclude_body_for_methods_where_body_is_definitely_disallowed. It used to pass because of the bug I fixed with the matching.

So the first part never matched because the content isn't json:

            // Assign
            string content = "hello";
            var server = FluentMockServer.Start();

            server
                .Given(Request.Create().WithBody(b => true))
                .AtPriority(0)
                .RespondWith(Response.Create().WithStatusCode(400));

Copy link
Contributor Author

@andi0b andi0b left a comment

Choose a reason for hiding this comment

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

done

@StefH
Copy link
Collaborator

StefH commented Sep 1, 2019

But there is also something weird with this change: c0a43ed#diff-78e158879161b0265ca8030bed233919

--> Yes, I think that the previous fix in that file (done by someone else) also used a different space/tabs setting.

@StefH StefH merged commit 2f40602 into WireMock-Net:master Sep 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants