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.09k stars 480 forks source link

Filtering by Consumers when loading from PactBroker is broken #1193

Closed marcos-scholtz closed 4 years ago

marcos-scholtz commented 4 years ago

When loading pacts from the PactBroker, one can filter them by consumer(s) using the system-property:

-Dpactbroker.consumers=my-consumer-name

When doing this however, it always results in a "NoPactsFoundException". Without this parameter, it does find the pact correctly for this consumer.

The reason seems to be in the PactBrokerLoader class, in line 195:

      if (pactBrokerConsumers.isNotEmpty()) {
        val consumerInclusions = pactBrokerConsumers.flatMap { parseListExpression(it, resolver) }
        consumers = consumers.filter { consumerInclusions.isEmpty() || consumerInclusions.contains(it.name) }
      }

According to my tests, "consumers" is a List of "PactBrokerResult" instances. The PactBrokerResult has a "name" field, but it contains the following text:

Pact between my-consumer-name (d49036c9b747af48c33fc66de0ddf7af57756c49) and my-provider-name

This text does NOT match the "consumerInclusions", they merely contain "my-consumer-name".

One solution could be to simply check that "my-consumer-name" is contained in the result's name field in any position, but this sounds hacky, it would probably be better to load the specific pact from the broker and check the real consumer name, but that's one call more for each consumer.

artemptushkin commented 4 years ago

Same for me

au.com.dius.pact.provider:junit:4.1.7

It will work even if change to it.name.contains(consumerInclusions) but it looks like work around, I wonder why do you return as name texts like this Pact between my-consumer-name (d49036c9b747af48c33fc66de0ddf7af57756c49) and my-provider-name

artemptushkin commented 4 years ago

@uglyog please take a look at the created PR for this

anto-ac commented 4 years ago

I think I've hit exactly the same problem when I upgraded the pact broker earlier today, and it's down to which api is used to retrieve the pacts, with the old api removed when pact for verifications came out of beta.

@artemptushkin and @marcos-scholtz - just to confirm, with which version of the broker you run into this?

artemptushkin commented 4 years ago

@anto-ac I think the latest docker pull dius/pact-broker:2.63.0.0

uglyog commented 4 years ago

I've updated the pact broker loader to send the consumer names in the selectors if using the new endpoint. The pact broker should do the filtering. When using the old end-point, it works as before.

anto-ac commented 4 years ago

Has anyone had a chance to verify the changes? @artemptushkin maybe? If they work as expected, I'd be happy to release.

artemptushkin commented 4 years ago

@anto-ac I could, but whats the pact-broker version to test @uglyog ? It doesn't depend on any pact-jvm update, right?

anto-ac commented 4 years ago

@artemptushkin the latest version of the broker has the new for-verification endpoint enabled by default. It also has a feature flag to use the old endpoint. I guess we'd have to make sure the changes work with both.

artemptushkin commented 4 years ago

@anto-ac I tested from the latest master commit c16b4876 - it is okay now with the new endpoint [pact-broker 2.65.0.0], do you know the flag to test the old one?

and when should we expect the next release?

anto-ac commented 4 years ago

PACT_BROKER_FEATURES is env var and the flag value is disable_pacts_for_verification

I can release any time - I would just feel safer if we were certain this change works with both endpoints.

artemptushkin commented 4 years ago

@anto-ac I tested it with 2.66.0 broker and this env variable, there wasn't for-verification in the API response and pacts has been fetched successfully

anto-ac commented 4 years ago

Amazing @artemptushkin ! Thank you so much for testing. I'll try and release a new version when I get a chance, unless @rholshausen gets there before me.

anto-ac commented 4 years ago

@artemptushkin 4.1.8 is out. I am going to close this based on your testing.