Feature Request: Support ignoring properties on a golang struct

[jfkriz]

Software versions

• OS: Mac OSX 10.12.6
• Consumer Pact library: pact-go v1.0.0-beta
• Provider Pact library: pact-go v1.0.0-beta
• Golang Version: go version go1.9.4 darwin/amd64
• Golang environment:

GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/kon5612/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/y1/xrh87b7d14l3q3kngz2lj6tsz_q0kx/T/go-build877678506=/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"

Expected behaviour

Summary
As a Consumer generating a pact from a golang struct
I want to be able to flag struct members as "ignored"
So that I can exclude them from the pact interaction and rules, and thus ignore them during Provider verification

Details
I would like the ability to tag struct members as ignored, so they do not get included in the interaction body or rules in the generated pact. I know this is similar to the discussion about Optional Attributes, so I want to call it an "ignored" attribute, rather than "optional", to make the distinction. The use case is that we are an aggregate service - we expose a public contract to our consumers, and when invoked, we will orchestrate calls to internal services, gather up those responses, and compose a response to our consumers. For the most part, we are a pass-through for the internal data, with some transformation and additional logic added where necessary. There are times when our internal service contracts will include optional data fields in their response, and in some cases, we will pass that data along to our consumers. Being a pass-through, we don't really care about what this data looks like, and we will only send it along if it is included in the response to us. We would like to be able to use pact to verify the interactions with our internal services, and there are definitely many fields that we do want to verify, but because these "optional" fields are in the struct, they fail to verify against the provider when they are missing.

One option we have considered is to find use cases in the internal service where these optional fields will always exist, and use those for Provider verification, then the pact can be verified properly - however we'd rather just have the ability to mark these fields as ignored, and assume the risks associated with this.

We have completed a proof-of-concept in a fork of the pact-go library, and have gotten the desired results. However, I don't want to submit a merge request for this without some discussion, since this is very similar to the "optional" feature that is not supported. We have not modified the provider verifier in any way, this is simply a way to exclude certain fields from the pact file when generating it in the Consumer tests.

Actual behaviour

When verifying a pact, some "optional" fields that I don't really care about are always being verified, and verification fails when those fields are not returned from the provider.

Steps to reproduce

N/A

Relevent log files

N/A

[mefellows]

I might need some support from @bethesque and @uglyog here, but my initial reaction is this:

1. At a minimum, the pact file should contain a precise definition the agreement - tick
2. As long as it is a conscious decision from the user of the library to ignore a field, and it promotes best practices by default I think I could see this working

Just so we're clear, are we talking about request fields also, or just (provider) response fields?

FWIW I have a similar use-case at the moment with a GraphQL endpoint that aggregates and re-shapes data from multiple data sources, some of which I can't control.

[bethesque]

I always feel that pass through services are a bad use case for contract testing, because the data that comes back depends more on downstream services than the one that's being tested. The difficulty we're having with this scenario reinforces this idea.

I would also like to know, are we talking about request fields or response fields?

Can you give a description of what your current work around is doing to the pact?

[mefellows]

I know, but if it's just a bit more than a pass-through (transforming, aggregating etc. - like a BFF) I feel contract-testing is still pretty key!

[jfkriz]

I'm specifically considering this for response fields, as that is the use case we're trying to satisfy, but I could see this on request fields as well.

…

On Mon, Jul 23, 2018, 7:59 PM Matt Fellows ***@***.***> wrote: I know, but if it's just a bit more than a pass-through (transforming, aggregating etc. - like a BFF) I feel contract-testing is still pretty key! — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#92 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACyhjIQqr2otEDq4xZfkQ2WvjCrPhqsFks5uJmNFgaJpZM4VbMqJ> .

[jfkriz]

@bethesque, in response to your question, the code we've updated in the pact-go repository is in a branch on a fork in dsl/matcher.go, and adds a boolean ignore tag to the other supported tags (min for a slice, and example & regex for string-like values). The purpose of this tag is to allow a developer to intentionally mark properties in a struct as ignored during pact generation. The change does NOT in any way impact pact verification, it just allows a dev to intentionally say "I know this piece of data might be in the contract (as defined by the struct), but I do not want to consider it as part of the pact to be used for provider verification". So when a property is marked with a pact:"ignore=true" tag, the pact generation will skip over the property, as if it were not included in the struct at all.

What @mefellows described above (especially the BFF reference) is a good description of what we're doing. As a "mostly pass-through service", there are some fields we get from our providers that we just want to pass along unchanged to our consumers, and those are the fields we'd like to exclude from pact verification (especially since a lot of them are only populated in certain cases). But there are also a lot of fields that we either use for business logic, or that we want to transform in some way to make more user-friendly to our consumers, where we definitely do want to do pact verification. Does that make sense?

We could certainly fork this and just use it internally in my organization, but I feel like this is something that the community could benefit from - as long as it doesn't stray too far from the goals of the larger Pact Foundation. I think this fits well, in that it does not alter the Pact verification at all, and that's where it is different from the "Optional attribute" discussion. This is just giving the developer an option to say that a certain field/collection/map should not be considered when generating a Pact. (This is my first time using Pact, and am still pretty new to it, but I can think of at least a couple similar use cases at past clients where I would have wanted this feature).

[bethesque]

I often end up using a mix of Pact tests and "normal" tests when I'm testing a provider client. Things that I want included in the contract, I test using Pact, and variations that I don't want included in the pact, I test using standard mocks. For example, I've just been writing tests for a CLI that uses HAL to navigate an API, and to make sure it behaves gracefully with older versions of the API, I have a "normal" test case to show what happens when a certain relation is not present. The absence of the relation is not something that I can or need to test in a contract.

I'm wondering if that is the cleaner solution to your problem @jfkriz?

I'm a big believer in using the right tool for the right job and not trying to make a tool to do many things, but I'm also a pragmatist. I can see why this would be useful, but at the same time, the idea of modifying a tool to try and make it do something that is contrary to the intent of the tool makes me wary. I'd want to be quite sure that it was the right decision before we introduced the change. I'm open to being convinced though - @jfkriz bring out your most persuasive arguments as to why this is something that belongs in pact, and why using a mix of pact and "normal" test cases isn't a better solution.

[jfkriz]

Thanks @bethesque - my most compelling case is actually that this feature exists in other Pact Consumer DSLs already, just in a slightly different form. Citing pact-jvm as one example, I can use the PactDslJsonBody to build an arbitrary response in my consumer test, and I can specify the exact structure I expect in the response, allowing me to include and exclude fields as I choose. There's an example of using the DSL to build a response in the README. Similarly, in pact-js, I can add interactions to the provider, and I can specify an arbitrary body object, with whatever properties and matchers I desire, as shown in this example in the README. The key takeaway here is that in those examples, I (as a Consumer) have the ability to say "For this particular interaction with this provider, here are the parts of the response I care about".

In the Go language, and in the pact-go library, I can do something similar, but there are other options that are more elegant, in my opinion. I can certainly use the various matchers to build an expected response body matcher, as shown in this example. However, because Go has the concept of tags, and the matcher.go has the extremely useful Match function that will introspect a struct, recurse on its properties, and build a pact interaction body and rules based on struct properties and tags, it is desirable to be able to take advantage of that for this scenario. It is really just another simpler, more concise option for building the same interaction. Is it required - absolutely not. But is it convenient for a developer - absolutely!

To address your other question (regarding using "normal" tests in combination with pact tests), we actually are doing this already to some extent. We already have many automated end-to-end tests that validate different use cases for our services. Now we're trying to implement contract tests for the things our services depend upon. The problem is that without this particular feature, if we want to use Pact at all, we are forced to write Pact consumer tests that are much more verbose than we'd prefer. We can possibly implement some mocking as a workaround for this (and eventually, we probably will), but since we're just beginning our dive into Pact, I'm trying to make things as painless as possible for our organization as a whole, so that other teams and groups can see the value Pact can bring, and we can get wider adoption. If there is a lot of friction (like needing mocked services) to just begin using this, I'm afraid I will not be able to get the buy-in needed from folks outside our team.

So in summary, I don't believe we are asking for fundamental changes to the Pact specification or to do anything that is contrary to the the goals of the Pact project as a whole. We are simply wanting to make things more convenient for a developer, allowing someone to do in a tagged struct and a single line of code what would take many, many lines otherwise.

[bethesque]

Ok, I'm very confused. The JS library and the Go library both use the same underlying Pact "engine". I can't see why there are things in one that you can't do in the other. I'll have a chat to @mefellows and see if he can explain what I'm missing.

[jfkriz]

@bethesque , I apologize, I am not doing a good job of describing the change. I updated my branch with a couple of example unit tests that generate a pact file, and also included the updated pact file so you can see that they are almost identical (response.body is different, but response.matchingRules are identical). It is a very simple example, where a User will always have a first and last name, but middle name is often missing, so we do not want it included in pact verification, and their mailing address will always have line1, city, state, and postal code, and sometimes also have line 2 and country, but we don't want to include those in pact verification. Let me know if this helps - and @mefellows , let me know your thoughts as well. Thanks!

• Current method to generate a Pact using DSL, ignoring the fields mentioned above that are not always populated
  • Generated matching rules
• New way to generate a Pact with the exact same matching rules
  • Generated matching rules

[bethesque]

One of the "you should not use pact if this is the case" points is, don't use it if you can't control the provider state data. Can you explain why you can't control the data?

[jfkriz]

It's a situation where even though the providers are within our organization, we are all on different release cadences and timelines. We will never be in sync, but as we adopt more and more contract best practices, it will be easier for new consumer tests to be added without running into the same issues we are currently seeing. So while we do have some control over the provider state data, because of the size of the organization and the number of teams involved, we cannot get everyone on board with this at once, especially without being able to show some value. In our particular case, we have some data that sometimes comes back from a provider, and sometimes does not. Ideally, we'd like to have some control over our provider's state, and if all goes as planned, eventually we will, assuming we can prove the value of making changes to allow this. But at the moment, we're just trying to get everyone to buy into all of these concepts, and it is hard to do so without having something concrete to show. Also, regardless of whether a consumer has control over provider state data or not, their contract test should only test the things they really care about. The goal of the proposed change is just to give consumers another option for specifying the things they care about.

So while trying to champion Pact within the organization, I am trying to make it easier for consumers in our organization (specifically those working in Golang) to write the tests to generate the Pact contracts. There will be less resistance to doing it if it is really easy to do. In my previous message, I gave examples of what my proposed change would do. Again, just another option for a consumer to specify what they care about validating against the provider contract.

This will allow us to generate useful contracts and have simple code examples for other teams to follow, which will (should...) lead to buy-in from the entire organization, making it a priority for providers to make the necessary changes to allow consumers to specify/control state during their contract tests, and then everyone is happy. At least that's my hope...

[AlexRamey]

@jfkriz I actually wrote the matcher function and am very excited to hear you find it useful!

At the organization where I work, I try to encourage the consumers to specify all fields, even the optional ones, and for the providers to supply all fields when doing contract testing. For the provider side, I've written a similar recursive traversal function called seed that will take in an empty struct and fill out all the fields with dummy values. So, on our provider side, we don't use real data. We mock our underlying services and let our "real code" run on top of them. We inject fully-hydrated mock responses (using seed to fully hydrate them) into our mock services so that our "real code" returns fully hydrated responses.

So, ideally, you would not have to omit optional fields from your contract. This makes sense b/c, in the wild, when those optional fields are provided, you want to have confidence that they are of the correct type, etc . . .

My concern over an ignore tag would be it encouraging others to omit optional fields, when ideally you would want to include them. With that being said, I understand practical issues that could warrant this (open-ended map[string]string in your DTO, which I think would break the auto matcher helper) and wouldn't be too opposed to an ignore tag as long as it is accompanied by a strongly worded comment about how it is not best practices to use it.

How are things going with Pact in your organization nowadays (I realize it's been a while since this discussion went cold)

[mefellows]

Closing as won't fix, but if a good PR/suggestion can be proposed we can re-open.