search

pact-foundation / pact-specification

Describes the pact format and verification specifications
MIT License
295 stars 28 forks source link

Feature request - `array_contains` or `each_like` with multiple elements #38

Closed ivawzh closed 3 years ago

ivawzh commented 7 years ago

Say I want to talk to a service to create a pony. We know the response will be: 

  {
     id: 1,
     history: [
       { event: "create", date: "01-01-2000", id: 123 },  
       { event: "health_check", date: "01-01-2000", id: 124 },  
       { event: "verify", date: "01-01-2000", id: 125 },  
       { event: "approve", date: "01-01-2000", id: 126 }
     ]
  }

However, I only care IDs of two particular events - create and approve.

So my Pact is:

expected_response = {
  id: Pact.like(123),
  history: [
    {
      event: 'approve',
      id: Pact.like(2)
    },
    {
      event: 'create',
      id: Pact.like(1)
    }
  ]
}

Note I don't even care the order within the history array.

This Pact setup won't work because Pact will try to exact match on history array. And then it will complain about array order and length not match.

The current each_like method only accepts one element.

I think the ideal scenario will be having a new matcher called array_contains which allow us to match and generate multiple elements in an array; and ignore array length and order.

mefellows commented 7 years ago

You can still wrap the entire array in a like and I believe you can additionally restrict the value of event in history to approve and create using a regex. What you can't do at the moment is ask pact to validate that it contains exactly two elements within the array of n elements.

ivawzh commented 7 years ago

@mefellows If I use regex with a union value like:

expected_response = {
  history: Pact.like([
    {
      event: Pact.term(
        generate: 'approve', 
        matcher: /(approve|create)/)
      }),
      id: Pact.like(2)
    },
    {
      event: Pact.term(
        generate: 'create', 
        matcher: /(approve|create)/)
      }),
      id: Pact.like(1)
    }
  ]
})

It will fail for:

For now, a dirty hack around would be:

expected_response = {
  history: Pact.like([
   {
      event: Pact.term(
        generate: 'create', 
        matcher: /(approve|create|health_check|verify)/)
      }),
      id: Pact.like(1)
    },
    {
      event: Pact.term(
        generate: 'Dont mind me. I am a placeholder for making length check pass.', 
        matcher: /(approve|create|health_check|verify)/)
      })
    },
    {
      event: Pact.term(
        generate: 'Dont mind me. I am a placeholder for making length check pass.', 
        matcher: /(approve|create|health_check|verify)/)
      })
    },
    {
      event: Pact.term(
        generate: 'approve', 
        matcher: /(approve|create|health_check|verify)/)
      }),
      id: Pact.like(1)
    }
  ]
})

Ugly because consumer has to

  1. know all the possible event names, though I only care two of them,
  2. fill with placeholders because of array length check.

Furthermore, it will give me false positive if the actual response contains 4 verify events. Also if provider changes to provide 7 events, test will be broken though consumer is not actually affected.

bethesque commented 7 years ago

If you were to design the interface for this, what would you want it to look like, and what should it's pass/fail scenarios be?

ivawzh commented 7 years ago

For my current case, we only need something like 

{
  history: Pact.array_contains(
    { event: 'create', id: Pact.like(1) }, 
    { event: 'create', id: Pact.like(1) }
  )
}

If I am greedy, I imagine it'd be good to also have

bethesque commented 7 years ago

I've been giving this some thought.

What should we do in the scenario where an element exists in an array that doesn't have an example specified for it?

expected = Pact.array_contains "red", "green"
actual = ["blue", "red", "green"]

Do we match blue based on type? Do we ignore it completely? It makes me a little nervous to be just ignoring elements, but I guess that's what the consumer is doing anyway.

With the contains_any, this asserting doesn't assert much. If all the elements come back as Australia, then we may still think that China and India are options when they may not be. I don't like the idea of making an assertion that is un-assertable. I guess it's the same as doing a regex Australia|China|India though, and that's already allowed. It just makes me uncomfortable. I wonder if we could have a warning for options that are never found.

One thing is that pact doesn't provide a "does not contain" assertion. This is for a few reasons. One, as each interaction contract only applies to one interaction, you can never be assured that a different request/response pair might not have the thing you are asserting doesn't exist. Secondly, just because you don't use a thing, doesn't mean another consumer doesn't want a thing. Being allowed to say "x has a thing" and "x does not have a thing" would lead to potential conflicts. Following Postel's law means a consumer should be relaxed about what it accepts, and ignore things that don't apply to it.

I understand this is a little different when we're talking about array elements rather than json nodes, however, I still feel it would be a bad feature choice.

Sorry, this is a lot of nit picking, but thinking through all the positive and negative implications of a feature is part and parcel with the specification design! It's gotta be done.

uglyog commented 7 years ago

The issue is when people have lists with key value pairs, there can be any number but they are only interested in some of them. See https://github.com/DiUS/pact-jvm/issues/378

ivawzh commented 7 years ago

I actually did not give too much thought before I mention array_contains_either, array_contains_any, array_contains_none. I agree with @bethesque, maybe in most of the common cases we should use context (given and upon_receiving) to help setup response instead of having array_contains_either, array_contains_any, array_contains_none to declare general response types. Some examples:

# example for array_contains
# the provider will stub some things to make sure at least one girl will be created. 
stubbed_service
  .given('I hope there will be at least one girl')
  .upon_receiving('make me 12 ponies')
  .will_respond_with(
    Pact.array_contains({ name: Pact.like('Bell'), gender: 'girl' })
   )

# example for array_contains_none
# the provider will stub some things to make sure all girls. 
stubbed_service
  .given('I hope there will be no boys')
  .upon_receiving('make me 12 ponies')
  .will_respond_with(
    Pact.each_like({ name: Pact.like('Bell'), gender: 'girl' })
   )

# example for array_contains_any
# the provider does not need to be stubbed. 
stubbed_service
  .given('boy or girl is up in the air')
  .upon_receiving('make me 12 ponies')
  .will_respond_with(
    Pact.each_like({ 
      name: Pact.like('Bell'), 
      gender: Pact.term(
        generate: 'girl', 
        matcher: /(boy|girl)/)
      })
    })
   )
)

I think in the array_contains_any example the consumer test will be a bit hacky because the generated array will only contain girl in this setup. Thus it cannot test if downstream logics can properly handle boy and girl at the same time. But it's a minor issue.

Contains_either is implying when one type occurs then the other type won't occur. That's effectively a 'does not contain' assertion. Hence we don't need to consider.

TimothyJones commented 6 years ago

array_contains would have been useful on our last project - for modelling a set of returned elements, we don't necessarily know the order that they'll come in.

I agree with the arguments against array_contains_any and array_contains_none

uglyog commented 6 years ago

I think the solution to this problem is to be able to apply matchers based on a predicate (See https://github.com/DiUS/pact-jvm/issues/727)

For example:

validatedAnswers minLike(1) {
  when(type == ''typeA") {
    favourite_colour regex('blue|red')
    ...
  }
}

This should add a type matcher something like (see what I did there? ;-) )

    "matchingRules": {
          "body": {
            "$.validatedAnswers": {
              "matchers": [
                {
                  "match": "type",
                  "min": 1
                }
              ],
              "combine": "AND"
            },
            "$.validatedAnswers[*].favourite_colour": {
              "matchers": [
                {
                  "match": "regex",
                  "regex": "red|blue"
                }
              ],
              "combine": "AND",
              "predicate": [
                "$.validatedAnswers[@].type == 'typeA'" // this need to be related to the same array item somehow
              ]
            }
          }
        }
mefellows commented 6 years ago

This should add a type matcher something like (see what I did there? ;-) )

🙄

I like the concept of a predicate, it would simplify many of the polymorphic payloads I see around the place.

I'll add a note to review the latest OAS specification that deals with these types of variants, as it could be useful in being fully across for v4 of the spec.

On Sun, Aug 12, 2018 at 1:33 PM, Ronald Holshausen <notifications@github.com

wrote:

I think the solution to this problem is to be able to apply matchers based on a predicate (See DiUS/pact-jvm#727 https://github.com/DiUS/pact-jvm/issues/727)

For example:

validatedAnswers minLike(1) { when(type == ''typeA") { favourite_colour regex('blue|red') ... }}

This should add a type matcher something like (see what I did there? ;-) )

"matchingRules": {
      "body": {
        "$.validatedAnswers": {
          "matchers": [
            {
              "match": "type",
              "min": 1
            }
          ],
          "combine": "AND"
        },
        "$.validatedAnswers[*].favourite_colour": {
          "matchers": [
            {
              "match": "regex",
              "regex": "red|blue"
            }
          ],
          "combine": "AND",
          "predicate": [
            "$.validatedAnswers[@].type == 'typeA'" // this need to be related to the same array item somehow
          ]
        }
      }
    }

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/pact-foundation/pact-specification/issues/38#issuecomment-412316207, or mute the thread https://github.com/notifications/unsubscribe-auth/AADSjAJnDlliS_4oB6HzfxLg9W1Aokr7ks5uP6ILgaJpZM4OGJxv .

-- Matt Fellows

bethesque commented 6 years ago

I can see how this would be useful, but it also seems to me to be getting too much into the business logic of the response. Instead of worrying about the keys and types in the responses, we're starting to say, "when this key/value is like this, then the other key/value should be like this" and then we start putting business rules into the contracts that then make them quite brittle. I'm not against it, but I'm mulling through the possible consequences.

kiranpatel11 commented 5 years ago

@bethesque not sure having predicates is against philosophy of contract testing.

I am working with below usecase :

` GET customer/12345/orders

Response
{
    Orders : [ 
    {
        orderId : "xyz",
        "status"   : "INPROGRESS",
        "orderCreatedOn" : "20-July-2019",
        …..

    },
    {
        orderId : "abc",
        "status"   : "COMPLETED",
        "orderCreatedOn" : "12-June-2019",
        "orderCompletedOn" : "20-June-2019",
        …..
    }]
}     `

The consumer expects the "orderCompletedOn" to be there when an order is in COMPLETED status. I think this is pretty much contractual in nature. In relatively complex business context, these kind of apis are norms.

TimothyJones commented 5 years ago

The consumer expects the "orderCompletedOn" to be there when an order is in COMPLETED status. I think this is pretty much contractual in nature. In relatively complex business context, these kind of apis are norms.

I agree with this.

it also seems to me to be getting too much into the business logic of the response.

I also agree with this - I don't think it's the right approach to put logic into your pact test.

It sounds like you're looking for "the response to X is Y(a) if the provider state is S(a); but Y(b) if the provider state is S(b)".

Your contract will be more straightforward if you're able to say "when the provider state is S(a), and the request is X, the response is Y(a)" and "when the provider state is S(b), and the request is X, the response is Y(b)"

To do this I would use two tests, resulting in two separate interactions in the pact:

1) With the provider state "an order is in progress" 2) With the provider state "an order is completed"

kiranpatel11 commented 5 years ago

With the provider state "an order is in progress" With the provider state "an order is completed"

This is exactly what we have done, but was wondering if we have to write a testcase to simulate more realworld case of retrieving all customer orders. Semantically, I think it is still same thing even if we write tests with 2 different states, we still need to know and test same business logic.

TimothyJones commented 5 years ago

we still need to know and test same business logic.

I don't think the contract test should be testing business logic, though - it should be testing "can the client understand all the responses it might get".

I see the point about being able to write a test with multiple types in the array, though. However, we don't want to write tests that are like: "this array contains either A or B", which passes when the provider only ever returns A (this is the reason that eachLike has a minimum of 1 - so an empty array won't pass it during provider verification).

As above, I'd be in favour of a matcher that can express "this array contains an element like X anywhere in it".

mefellows commented 5 years ago

The consumer expects the "orderCompletedOn" to be there when an order is in COMPLETED status. I think this is pretty much contractual in nature. In relatively complex business context, these kind of apis are norms.

I have this exact situation on a client now, where data coming in from a BMS system comes in as a list of records, with dynamic fields basic on the specific value of a key.

zeldigas commented 4 years ago

Any progress or ETA on that?

Looks like this feature is essential to create contracts on formats like Siren - it is list based, so to create a contract that entity has a link with specific rel and not put restriction on index in array, this feature described here is essential.

To be more specific, let's look at an example:

{
  "class": [ "order" ],
  "properties": { 
      "orderNumber": 42, 
      "itemCount": 3,
      "status": "pending"
  },
  "entities": [
    { 
      "class": [ "items", "collection" ], 
      "rel": [ "http://x.io/rels/order-items" ], 
      "href": "http://api.x.io/orders/42/items"
    },
    {
      "class": [ "info", "customer" ],
      "rel": [ "http://x.io/rels/customer" ], 
      "properties": { 
        "customerId": "pj123",
        "name": "Peter Joseph"
      },
      "links": [
        { "rel": [ "self" ], "href": "http://api.x.io/customers/pj123" }
      ]
    }
  ],
  "actions": [
    {
      "name": "add-item",
      "title": "Add Item",
      "method": "POST",
      "href": "http://api.x.io/orders/42/items",
      "type": "application/x-www-form-urlencoded",
      "fields": [
        { "name": "orderNumber", "type": "hidden", "value": "42" },
        { "name": "productCode", "type": "text" },
        { "name": "quantity", "type": "number" }
      ]
    }
  ],
  "links": [
    { "rel": [ "self" ], "href": "http://api.x.io/orders/42" },
    { "rel": [ "previous" ], "href": "http://api.x.io/orders/41" },
    { "rel": [ "next" ], "href": "http://api.x.io/orders/43" }
  ]
}

Right now Pact can only help with good verification of properties section. I have no idea how to verify presence of next rel and add-item action not relying on their positions in lists

TimothyJones commented 4 years ago

I like this feature request, and a matcher for this example would be useful.

However, I think the example that @zeldigas has can be covered with provider states to dial down the state of the resource enough to be matched with the current matchers.

zeldigas commented 4 years ago

@TimothyJones could you advise how to do it with current set of matchers, taking into account that relying on position of item in list is not acceptable? I'd be happy if you could educate me how to assert presence of previous link "item" in links, without pinning your assertion to current structure (i.e. that it's second item in a list). Client should not care about that, so the provider can put it in any position as well as supply 10+ links that client does not care about.

uglyog commented 3 years ago

I have an initial prototype of the "Array Contains" matcher working in Rust.

Simple example: https://github.com/pact-foundation/pact-reference/blob/feat/v4-spec/rust/pact_matching/src/json.rs#L828 Siren example: https://github.com/pact-foundation/pact-reference/blob/feat/v4-spec/rust/pact_matching/src/json.rs#L882

uglyog commented 3 years ago

Closing this issue as the matcher has been released for Pact-Rust and Pact-JS V3.

zeldigas commented 3 years ago

@uglyog I can't find description for syntax of this matcher in docs. Is it only in rust source code so far?

uglyog commented 3 years ago

@zeldigas it is a work in progress, so no docs have been written to date. It is implemented in Rust and Pact-JS V3 and Pact C++. I'm currently implementing it for Pact-JVM. You can find an example in JS here: https://github.com/pactflow/example-siren/blob/master/consumer/src/__tests__/delete-order.spec.js#L73

uglyog commented 3 years ago

In that project, if you comment out the delete endpoint in the provider, and then run the Rust CLI verifier, you get this error:

$ ./target/debug/pact_verifier_cli -f '/home/ronald/Development/Projects/Pact/example-siren/consumer/pacts/Siren Order Provider-Siren Order Service.json' -p 8080

Verifying a pact between Siren Order Provider and Siren Order Service
  get root
    returns a response which
      has status code 200 (OK)
      includes headers
        "Content-Type" with value "application/vnd.siren+json" (OK)
      has a matching body (OK)
  get all orders
    returns a response which
      has status code 200 (OK)
      includes headers
        "Content-Type" with value "application/vnd.siren+json" (OK)
      has a matching body (FAILED)
  delete order
    returns a response which
      has status code 200 (FAILED)
      includes headers
      has a matching body (OK)

Failures:

1) Verifying a pact between Siren Order Provider and Siren Order Service - get all orders returns a response which 
    1.1) has a matching body
           $.entities.0.actions -> Variant at index 1 ({"href":"http://localhost:9000/orders/1234","method":"DELETE","name":"delete"}) was not found in the actual list

2) Verifying a pact between Siren Order Provider and Siren Order Service - delete order returns a response which 
    2.1) has status code 200
           expected 200 but was 405

There were 2 pact failures
zeldigas commented 3 years ago

Great, thank you so much! I'll give it a try

anurag-evive commented 3 years ago

@uglyog Hey, was array_contains released for pact-jvm?

uglyog commented 3 years ago

Yes, with version 4.2.0-beta

mefellows commented 3 years ago

I couldn't find the updated matchers in the docs Ron, are they available in one of these pages: https://docs.pact.io/implementation_guides/jvm/consumer#building-json-bodies-with-pactdsljsonbody-dsl?

uglyog commented 3 years ago

There are no secondary docs written yet. Primary docs are the source code :-D

docs.pact.io is updated from the master branch. The 4.2.x branch has not been merged to master yet (I'll probably do it this week)

Docs: https://github.com/pact-foundation/pact-jvm/tree/v4.2.x/consumer#array-contains-matcher-v4-specification

mefellows commented 3 years ago

Ah! Of course, that makes sense.