pact-foundation / pact-jvm

JVM 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://docs.pact.io
Apache License 2.0
1.08k stars 479 forks source link

Matching against empty body #85

Closed algra closed 9 years ago

algra commented 9 years ago

I have Play application exposing an API, my controller looks like that

class ApiController @Inject()(someClass: SomeClass) extends Controller {
  def addStuff = CorsAction.async(CustomBodyParsers.somePayloadFromJson) { request ⇒
    async {
      await(someClass.someMethod(someParams))
      Ok
    }
  }
}

CorsAction is doing quite nothing except adding headers to allow cross-domain calls, removing it does not make any difference:

object CorsAction extends ActionBuilder[Request] {
  val corsHeaders = Seq(
    "Access-Control-Allow-Origin" -> "*",
    "Access-Control-Allow-Methods" -> "POST, GET, PUT, DELETE, OPTIONS",
    "Access-Control-Allow-Headers" -> "Origin, X-Requested-With, Content-Type, Accept, Referer, User-Agent",
    "Access-Control-Max-Age" -> "1728000"
  )
  def invokeBlock[A](request: Request[A], block: (Request[A]) => Future[Result]) =  Async.async {
    await(block(request)).withHeaders(corsHeaders: _*)
  }
}

This is how I'm matching response in pact file:

{
  "description": "A POST request to add stuff",
  "request": {
    "method": "post",
    "path": "/add",
    "headers": {
      "Content-Type": "application/json"
    },
    "body": {
      "someName": "someValue"
    }
  },
  "response": {
    "status": 200
  }
}

And pact fails with the following output:

BodyMismatch(None,Some(),None,/) (Transformer.scala:20)

So, I guess None and empty body are not 100% correctly matched here: https://github.com/DiUS/pact-jvm/blob/master/pact-jvm-matchers/src/main/scala/au/com/dius/pact/model/Matching.scala#L101-L106

Can you, please, check it?

uglyog commented 9 years ago

It is treating a missing body (i.e. no body attribute in the pact file) as different as an empty body. This should not be the case.

Changing the pact file to have an empty body may match. But I'll update the de-serialisation code to return a None for an empty body as well.

uglyog commented 9 years ago

You should not get a failure now, as the empty response body should be mapped to None.

algra commented 9 years ago

Yeah, I tried to put "body": "" in pact file, but it did not helped, mismatch was still there, don't have log output, but it was something like

Some("") vs Some()

Anyway, should be fixed now with your fix, thanks a lot for a quick one :) Do you have plans to release a new version to maven repo?

algra commented 9 years ago

BTW, after checking the code, I did not really understood how to create a pact for checking only status code without checking body. Like I don't care what's in the body as long as http 200 is coming back after sending post request, guys on provider could put "ok" or "done" or "status": "ok" or whatever else.

The only way I found is leaving pact without body, like in my initial post and checking pacts by creating my own copy of ResponseMatching with DiffConfig(allowUnexpectedKeys = true, structural = true) or am I missing something?

uglyog commented 9 years ago

Ok, I now understand what you require. You are correct, leaving the body out means you don't care about it. I'm going to revert the previous commits, because an empty body is different than not validating the body.

I'll review the matching code you hi-lighted and double check the behaviour with the Ruby and .Net versions, but my thoughts are to remove the if statement from line 103. This should fix your problem.

algra commented 9 years ago

Yes, right, sorry for explaining it wrong way at the beginning.

I'm quite new to Scala, but to me it looks like it would even be possible to replaces lines 102-103 with the following:

case (None, _) => List()
uglyog commented 9 years ago

That is probably a better version, but the commit I made should resolve your issue.

algra commented 9 years ago

Yep, this will also fix the issue. Would it be possible to publish 2.1.10 with last change to maven repo?

uglyog commented 9 years ago

Yes, I'm doing the release now. It will take a few hours for the artifacts to be available on maven central.

algra commented 9 years ago

Cool, thanks! Closing issue now.

algra commented 9 years ago

Unfortunately I have to annoy you once more and reopen issue, because I found that this issue is not completely fixed during writing more pacts. Situation:

I've added additional check for that and text files to verify this situation, as well as fixed failing unit test. Pull request is on the way, but I have to say, I did not checked .NET or ruby implementations on how they behaving in situations like that.

uglyog commented 9 years ago

.NET and ruby has the correct behaviour (I asked on the pact-dev group). I'm going to add the specification tests to the specification project.

uglyog commented 9 years ago

Copied the empty body test cases over to the pact spec project