Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Filtering by Consumers when loading from PactBroker is broken #1193

Closed
marcos-scholtz opened this issue Aug 24, 2020 · 13 comments
Closed

Filtering by Consumers when loading from PactBroker is broken #1193

marcos-scholtz opened this issue Aug 24, 2020 · 13 comments
Labels
bug Indicates an unexpected problem or unintended behavior

Comments

@marcos-scholtz
Copy link

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
Copy link

artemptushkin commented Sep 4, 2020

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

@uglyog uglyog added the bug Indicates an unexpected problem or unintended behavior label Sep 5, 2020
@artemptushkin
Copy link

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

@anto-ac
Copy link
Collaborator

anto-ac commented Sep 25, 2020

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
Copy link

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

@uglyog
Copy link
Member

uglyog commented Oct 1, 2020

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
Copy link
Collaborator

anto-ac commented Oct 5, 2020

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

@artemptushkin
Copy link

@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
Copy link
Collaborator

anto-ac commented Oct 6, 2020

@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
Copy link

@anto-ac I tested from the latest master commit c16b487 - 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
Copy link
Collaborator

anto-ac commented Oct 7, 2020

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
Copy link

@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
Copy link
Collaborator

anto-ac commented Oct 13, 2020

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
Copy link
Collaborator

anto-ac commented Oct 14, 2020

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Indicates an unexpected problem or unintended behavior
Projects
None yet
Development

No branches or pull requests

4 participants