Request body is dropped if verb is REPORT

[eli-darkly]

The current implementation of OwinRequestMapper only calls BodyParser.Parse if BodyParser.ShouldParseBody(method) returns true. ShouldParseBody only returns true if the method is PUT, POST, OPTIONS, or PATCH.

This makes it impossible to fully test code that makes a REPORT request with a body: on receiving the request, WireMock.Net will simply discard the body and you can't verify its contents. While REPORT is somewhat obscure, it is in the HTTP Method Registry and it has well-defined semantics that specify it can have a body— otherwise it would be useless (it's meant as a cacheable alternative to POST).

More generally, I'm not sure why WireMock.Net has this verb-specific logic at all. It may be invalid to make a request with a body if the verb is one that shouldn't have a body, but when the server receives an HTTP request I would expect it to accurately pass on what the request properties really were.

This seems especially bad to me because the purpose of WireMock.Net is testing, and dropping possibly-invalid properties in this way limits what you can test. For instance, say that I have some code that makes a GET request, and I want to verify that it is not for some reason sending a body. My test might do something like Given(Request.Create().WithBody(...something...)).RespondWith(...some kind of error response...) - or check LogEntries afterwards and verify that the request body was null. But the test will always pass even if it should fail, because WireMock.Net will always say that the request body was null. It is telling me what it thinks the client code should have done, rather than what it did do.

[StefH]

Currently I follow this:

/*
            HEAD - No defined body semantics.
            GET - No defined body semantics.
            PUT - Body supported.
            POST - Body supported.
            DELETE - No defined body semantics.
            TRACE - Body not supported.
            OPTIONS - Body supported but no semantics on usage (maybe in the future).
            CONNECT - No defined body semantics
            PATCH - Body supported.
        */

I can add more support, or just allow all, but this break current functionality. So the best I can do is add a configuration setting which extends this list.

[eli-darkly]

I guess it depends on what you mean by breaking current functionality.

To me, the current behavior is not functionality, it's a bug: WireMock is not matching or recording the actual content of the request, but a transformation of the request. I can't think of any reason why someone would've written a test that depends on this behavior; as I described in the last paragraph of my last comment, I believe the current logic can cause false positives in tests. For instance, if someone is testing some code that sends a GET request, and the request is not supposed to have a body, and they set up WireMock specifically to reject any GET request that has a body... it won't; if their code incorrectly sent a body, the test would still pass. It'd be as if you were trying to test some code that is supposed to reject any invalid characters that the user types, but the test framework is filtering out all invalid characters before they reach your code; it makes no sense to depend on that behavior.

That's a practical issue, but there's also a conceptual issue: the behavior is contrary to the protocol spec. RFC2616 says that the message body should be ignored "if the request method does not include defined semantics for an entity-body". There's nothing there that says it has to be in that specific little list of methods that were typed into the BodyParser code in October 2018; if a method like REPORT exists in the method registry and its spec says it can have a body, then it's incorrect for a server to strip the body. That's part of the definition of HTTP, and WireMock is a testing tool for HTTP. Now, since the method registry can be updated at any time, it's not practical to keep updating the WireMock code every time something is added. So if you really want to do this kind of validation, then it would be more correct to have three states: "I know a body is supported for this method", "I know a body is not supported for this method", and "I don't know, so I won't mess with it." The list in BodyParser is just the ones you know about so far.

Anyway— if you feel strongly that I'm wrong, then yes, a configuration setting to optionally extend the list of supported methods— or, to just make it stop modifying the requests altogether— would be very helpful.

I also think it would be a good idea to document this behavior somewhere in the XML documentation comments (like on the IBodyRequestBuilder methods), rather than just in a code comment like the one you mentioned; I don't think that comment really counts as documentation, since someone would have to already know to look for it in BodyParser (I spent quite a bit of time reading through the code before I found it).

[StefH]

Thanks for explanation. I did a quick test and only 3 unit tests fail when for all http methods the body is retrieved.
For a get request, the body is an empty string, I'm not sure if thats correct.

I need to do some more tests / thinking on this.

Or you can alsp think about a PR?

[eli-darkly]

I'd be glad to put together a PR - just wanted to open this issue first to see if there was any agreement on the general idea.