search

pact-foundation / pact-go

Golang version of Pact. Pact is a contract testing framework for HTTP APIs and non-HTTP asynchronous messaging systems.
http://pact.io
MIT License
857 stars 108 forks source link

Pact::Term fails in WithRequest clause #73

Closed gallamine closed 6 years ago

gallamine commented 6 years ago

Software versions

$ go env
GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/william.cox/go"
GORACE=""
GOROOT="/usr/local/go"
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/js/8ycxvlln2ljgdjyy834g14rr0000gq/T/go-build397368137=/tmp/go-build -gno-record-gcc-switches -fno-common"
CXX="clang++"
CGO_ENABLED="1"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"

I'm trying to set up a test with a Pact::Term as part of the WithRequest.

It looks like this: 

Given("The score_server has data to send us with model IDs").
UponReceiving("a GET request to v2 endpoint").
WithRequest(dsl.Request{
 Method: "GET",
 Path: "/scores/v2",
 Query: "threshold=0.9",
 Headers: map[string]string{
   "If-Modified-Since": dsl.Term(imsString,
                         `[a-zA-Z]{3}, [0-9]{2} [a-zA-Z]{3} [0-9]{4} [0-9]{2}:[0-9]{2}:[0-9]{2} [A-Z]{3}`),
    "x-api-key": apiKey}, }).
...

Essentially I'm trying to allow for variable If-Modified-Since datestrings in my Request headers but when I run the tests I get errors saying,

* Expected "\n\t\t{\n\t\t\t\"json_class\": \"Pact::Term\",\n\t\t\t\"data\": {\n\t\t\t \"generate\": \"Tue, 20 Mar 2018 11:38:04 EDT\",\n\t\t\t \"matcher\": {\n\t\t\t \"json_class\": \"Regexp\",\n\t\t\t \"o\": 0,\n\t\t\t \"s\": \"[a-zA-Z]{3}, [0-9]{2} [a-zA-Z]{3} [0-9]{4} [0-9]{2}:[0-9]{2}:[0-9]{2} [A-Z]{3}\"\n\t\t\t }\n\t\t\t}\n\t\t}" but got "Tue, 20 Mar 2018 11:38:04 EDT" at $.headers.If-Modified-Since

It appears that the dsl.Term isn't being properly validated/parsed when in the WithRequest.

mefellows commented 6 years ago

Thanks @gallamine, I've found the source of the issue but will take some refactoring to get it to work. It should work, however, and is a confirmed issue.

mefellows commented 6 years ago

Hi @gallamine, when you get a moment, I've made some serious changes to the Pact Go interface to accommodate what appears to be a long standing bug in the implementation. It's currently released as a beta v0.0.12-matching-rules.

Some background

The new changes mean you can't just insert strings into places where complex/nested matching logic could go. Previously it was supported, albeit there were a bunch of corner cases that meant it wasn't working as it should have. In my attempt to make the interface simple to use, it made the implementation messy. I've now rectified that, using strongly typed interfaces.

The new interface looks familiar, but takes stronger types.

from examples/consumer/goconsumer/user_service_test.go:

    body :=
        like(dsl.Matcher{
            "user": dsl.Matcher{
                "name": name,
                "type": term("admin", "admin|user|guest"),
            },
        })

    pact.
        AddInteraction().
        Given("User billy exists").
        UponReceiving("A request to login with user 'billy'").
        WithRequest(request{
            Method: "POST",
            Path:   term("/users/login/1", "/users/login/[0-9]+"),
            Query:  dsl.MapMatcher{
                "foo": term("bar", "[a-zA-Z]+"),
            },
            Body: loginRequest,
        }).
        WillRespondWith(dsl.Response{
            Status: 200,
            Body:   body,
        })

So this is great. We can match on "all-the-things". In some cases, however, we don't want complex matching - we just want to assert the value. This is where plain strings were used previously, but now need a type instead. See Path below, and the value for foo in the query string:

    pact.
        AddInteraction().
        Given("User billy does not exist").
        UponReceiving("A request to login with user 'billy'").
        WithRequest(request{
            Method:  "POST",
            Path:    s("/users/login/10"),
            Body:    loginRequest,
            Headers: commonHeaders,
            Query: dsl.MapMatcher{
                "foo": s("anything"),
            },
        }).
        WillRespondWith(dsl.Response{
            Status:  404,
            Headers: commonHeaders,
        })

I still think it reads well enough, and is achieved using a union/sum type approach.

For posterity, I also considered:

  1. Modifying the builder interface, so that instead of passing in a Request (or Response) struct, it was completely function driven. So instead of a Path field, it could have been a Path or MatchPath to allow for string vs matching. I didn't like this for a number of reasons, but mostly the API started to look pretty confusing and the interface increased dramatically.
  2. Made all matchable elements of the API simply interface{} and use type-assertions to coerce things into the right shape down the track. I didn't like this, as it makes the API ambiguous and also means we need much more documentation to describe how to use it.
  3. Using union types and strongly typing the API. This made the interface less "flexible", but didn't suffer from either of the first two problems. I favour strongly typed where possible, and want the API to be self documenting.

Next Steps

  1. If you don't mind giving this a go, I'd really appreciate the feedback on the API
  2. Given the size of this change and the fact that it's backwards compatible, I'll look to merge it with the work i'm doing on #31 to remove the need for a daemon. This will increase the usability of the library also.
  3. Whilst implementing this change, I've also added a bunch of new matchers which I'll document and publish with it.

NOTE: I aliased some functions above for readability, which I think worked:

var like = dsl.Like
var eachLike = dsl.EachLike
var term = dsl.Term
type s = dsl.String
type request = dsl.Request
mefellows commented 6 years ago

@gallamine capturing our Gitter chat for posterity:

Ok, i'm testing your feat/matching-rules-daemonless branch now. It appears to be working, aside from the previous issue with dsl.Like. This test works: 

pact. AddInteraction(). Given("There exists some data"). UponReceiving("A HEAD request"). WithRequest(dsl.Request{ Method: "HEAD", Path: dsl.String("/scores"), Query: dsl.MapMatcher{"threshold": dsl.String("0.9")}, Headers: dsl.MapMatcher{"If-Modified-Since": dsl.String(imsString), "x-api-key": dsl.String(apiKey)}, }). WillRespondWith(dsl.Response{ Status: 200, Headers: dsl.MapMatcher{"Content-Type": dsl.String("text/plain"), "X-Api-Content-Length": dsl.String("100"), "last-modified": dsl.Term(time.Now().Format(time.RFC1123), `[a-zA-Z]{3}, [0-9]{2} [a-zA-Z]{3} [0-9]{4} [0-9]{2}:[0-9]{2}:[0-9]{2} [A-Z]{3}`)}, })

changing dsl.String("100") to dsl.Like("100") results in the following error:

2018/03/27 20:55:25 [INFO] checking pact-mock-service within range >= 2.6.4, < 3.0.0 2018/03/27 20:55:26 [INFO] checking pact-provider-verifier within range >= 1.12.0, < 3.0.0 2018/03/27 20:55:27 [INFO] checking pact-broker within range >= 1.14.0, < 2.0.0 2018/03/27 20:55:28 [ERROR] service: [2018-03-27 20:55:28] INFO WEBrick 1.3.1 2018/03/27 20:55:28 [ERROR] service: [2018-03-27 20:55:28] INFO ruby 2.2.2 (2015-04-13) [x86_64-linux] 2018/03/27 20:55:28 [ERROR] service: [2018-03-27 20:55:28] WARN TCPServer Error: Cannot assign requested address - bind(2) for "::1" port 33365 2018/03/27 20:55:28 [ERROR] service: [2018-03-27 20:55:28] INFO WEBrick::HTTPServer#start: pid=175 port=33365 2018/03/27 20:55:28 [ERROR] service: [2018-03-27 20:55:28] ERROR NoMethodError: undefined method `split' for 100:Fixnum 2018/03/27 20:55:28 [ERROR] service: /home/go/bin/pact/lib/vendor/ruby/2.2.0/gems/rack-2.0.4/lib/rack/handler/webrick.rb:98:in `block in service' 2018/03/27 20:55:28 [ERROR] service: /home/go/bin/pact/lib/vendor/ruby/2.2.0/gems/rack-2.0.4/lib/rack/handler/webrick.rb:90:in `each' 2018/03/27 20:55:28 [ERROR] service: /home/go/bin/pact/lib/vendor/ruby/2.2.0/gems/rack-2.0.4/lib/rack/handler/webrick.rb:90:in `service' 2018/03/27 20:55:28 [ERROR] service: /home/go/bin/pact/lib/vendor/ruby/2.2.0/gems/webrick-1.3.1/lib/webrick/httpserver.rb:138:in `service' 2018/03/27 20:55:28 [ERROR] service: /home/go/bin/pact/lib/vendor/ruby/2.2.0/gems/webrick-1.3.1/lib/webrick/httpserver.rb:94:in `run' 2018/03/27 20:55:28 [ERROR] service: /home/go/bin/pact/lib/vendor/ruby/2.2.0/gems/webrick-1.3.1/lib/webrick/server.rb:191:in `block in start_thread' INFO: 2018/03/27 20:55:28 Received Status: 500 Internal Server Error 2018/03/27 20:55:28 Error on Verify: Service returned unexpected response: 500 FAIL github.com/distil/plt_transfer/loader 2.916s

Matt Fellows @mefellows 08:09

Great thx, that should be enough for me to repro

Matt Fellows @mefellows 08:29

@gallamine OK I think it's a bug in the underlying Pact CLI tools - are you a Tester by any chance For some reason, numbers - even if presented as strings - seem to be coerced into integers and then stored in the pact e.g.

{
"status": 200,
"headers": {
"X-Api-Content-Length": {
"json_class": "Pact::SomethingLike",
"contents": 100
}
},
"body": {
"json_class": "Pact::SomethingLike",
"contents": {
"user": {
"name": "Jean-Marie de La Beaujardière😀😍",
"type": {
"json_class": "Pact::Term",
"data": {
"generate": "admin",
"matcher": {
"json_class": "Regexp",
"o": 0,
"s": "admin|user|guest"
}
}
}
}
}
}
}

William @gallamine 08:34

@mefellows interesting. Not sure i exactly follow, but is there a workaround that you can think of? bbl. dinner calls.

Matt Fellows @mefellows 08:35

You probably don't need to follow, but looking into it now np - enjoy Hopefully your dessert will be a passing test ok so the good news is that it's somewhere in pact-go that's marshalling a string to a number when it goes over the wire. Hopefully it's something we can control!

Matt Fellows @mefellows 08:46

Ok I think I know what it is. We've always supported string-like objects in the DSL e.g. {"Foo":"bar"} But marshalling this in JSON is problematic, especially if combining with matchers and other string-like objects.

This was essentially the cause of the issues you experienced, and I thought I'd found a solution. The problem is type information is lost along the way. I could probably deal with this specific situation, but the warning signs are there that this is not a good idea. So, I'm going to deprecate that support and add a warning if I see someone attempting it. I'll push out an update for it some time this morning and update the examples

https://github.com/pact-foundation/pact-go/commit/6216f7df99eaaf9f74efcdfb4997f4b7f389cae1 should be good for you to go with. Build is green, and contains an integration test (as an example) with the problematic matcher you had. Thanks for helping, no doubt we'll chat again your tonight/my tomorrow :)

mefellows commented 6 years ago

This is now in the release/1.x.x line. Closing as it will be released soon.