pact-foundation / pact-net

.NET version of Pact. Enables consumer driven contract testing, providing a mock service and DSL for the consumer project, and interaction playback and verification for the service provider project.
https://pact.io
MIT License
833 stars 231 forks source link

Interaction matching - body is ignored in certain cases #47

Closed xelibrion closed 8 years ago

xelibrion commented 9 years ago

Hey @neilcampbell, question - would it make sense to run a match on request body even if request body for registered interaction is not specified?

Currently library will consider two requests matching even when body of the actual request is completely different (i.e. not null) from request body for registered interaction (that is null).

neilcampbell commented 9 years ago

@xelibrion Yeah it's a good question and I remember having a conversation about this at some point with someone else, but cant remember the exact outcome. Based on the pact specification for v2 (which we are currently not checking on just yet) the current behaviour is correct. https://github.com/bethesque/pact-specification/blob/version-2/testcases/request/body/no%20body.json. @bethesque Can you confirm that this is correct? Postel's law is making me question this one.

bethesque commented 9 years ago

I think you're right @neilcampbell, @uglyog, this doesn't seem right https://github.com/bethesque/pact-specification/blob/version-2/testcases/request/body/no%20body.json

uglyog commented 9 years ago

When there is no expectation on the body (as opposed to expecting an empty body), then surely it must match regardless if you receive a body or not?

On 3 September 2015 at 07:35, Beth Skurrie notifications@github.com wrote:

I think you're right @neilcampbell https://github.com/neilcampbell, @uglyog https://github.com/uglyog, this doesn't seem right https://github.com/bethesque/pact-specification/blob/version-2/testcases/request/body/no%20body.json

— Reply to this email directly or view it on GitHub https://github.com/SEEK-Jobs/pact-net/issues/47#issuecomment-137251507.

xelibrion commented 9 years ago

@neilcampbell @bethesque @uglyog I think there actually might be two parts to this question. First one is matching registered interaction with actual request on consumer side, and the second one is matching consumer expectation and provider response.

As for the latter one, it is perfectly fine to have extra fields or entire body in the response in my opinion, but I don't think that exactly the same rules should apply to the former, although I can see that it might be questionable and we potentially might need two modes of interaction matching on consumer side (strict/non-strict).

uglyog commented 9 years ago

For reference to this https://github.com/DiUS/pact-jvm/issues/85 Google groups thread: https://groups.google.com/forum/#!topic/pact-dev/Dop34iwT3BE

neilcampbell commented 9 years ago

@xelibrion I had a bit of a think through what we discussed on the weekend in relation to matching the correct request with response for the mock. We could definitely do what we talked about, however it would deviate from the what the specification outlines (which I don't want to do). See item 2 in Handling Requests https://github.com/bethesque/pact-specification/tree/master/implementation-guidelines#handling-requests

bethesque commented 9 years ago

There's a difference between an expectation of a null body, and no expectation for the body. I think (correct me if I'm wrong Ron), if you expect a null request body, then you should get a null request body. If you make no expectation on the request body, then I think we're not checking it at all. When the request is replayed though, it's going to have a null body, rather than whatever the actual request was, which makes it potentially buggy.

uglyog commented 9 years ago

@bethesque That's correct. If the body field is missing from the pact file, it can be anything because we are not checking it. It does not have to be null.

xelibrion commented 9 years ago

@neilcampbell Fair enough. To solve the problem that I had (spending two hours debugging PactNet because I've forgotten to specify body for a POST request in expectation) is not necessary to change expectation matching rules - would be enough to display a warning/suggestion in test output that body was empty in expectation, but it was present in actual request. Would make dev onboarding slightly easier.

bethesque commented 9 years ago

Sounds like a good idea.

neilcampbell commented 8 years ago

@bethesque @uglyog Was just looking at closing this issue and implementing a fix for some of the stuff discussed and wanted to confirm a couple of behaviours.

I can see that when no body has been specified we don't go any further with regards to checking the body (https://github.com/bethesque/pact-specification/blob/version-2/testcases/request/body/no%20body.json), which is the current behaviour for PactNet.

Now if I explicitly set the expected body to null, should I ensure that the actual body is null as well and serialize that in the pact file ("body": null)? Is there a specification test for both request and response to show this scenario?

bethesque commented 8 years ago

I'm not sure if there's a specification, but the behaviour you described in the last paragraph is the behaviour of the Ruby impl.

neilcampbell commented 8 years ago

@bethesque Ok cool. I will create a specification test and PR for it, that way it is explicit.