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

Cannot mix message and rest pacts #610

Closed
artamonovkirill opened this issue Jan 2, 2018 · 20 comments
Closed

Cannot mix message and rest pacts #610

artamonovkirill opened this issue Jan 2, 2018 · 20 comments

Comments

@artamonovkirill
Copy link
Contributor

  • au.com.dius.pact.model.PactReader loads only message pacts if any message pact present in pact json.
  • au.com.dius.pact.model.PactMerge only allows merging RequestResponsePact to another RequestResponsePact or MessagePact to another MessagePact, but not MessagePact to RequestResponsePact or vice versa.
@uglyog
Copy link
Member

uglyog commented Jan 15, 2018

This is by design, as we feel that synchronous and asynchronous contract tests should be separate.

Could you expand on your requirement to mix them?

@artamonovkirill
Copy link
Contributor Author

@uglyog,

  • We have two services that communicate both via a queue and via REST. At the moment we had to create '%consumer%' and '%consumer% (queue)' participants in the broker. That looks a bit ugly.
  • We already run provider side tests separately (with RestPactRunner and MessagePactRunner), as for one provider there are sometimes message and REST consumers.
  • Also, pact JSON format fully supports having both message and REST pacts.

@uglyog
Copy link
Member

uglyog commented Jan 29, 2018

I have done this with having two different consumer names (like consumer and consumer-amqp). The reason we keep them separate is the different types of pacts have to be verified in a different manner.

@artamonovkirill
Copy link
Contributor Author

@uglyog,

the different types of pacts have to be verified in a different manner.
It's also the case when a provider has a consumer A for REST API and consumer B for messaging API.
If I would use just @RunWith(PactRunner), some pact will fail as either AmqpTarget or HttpTarget won't be present. But that separation already tackled by splitting pacts by MessagePactRunner and RestPactRunner.
So, I think provider side is fully covered for tackling mixed pact types for a single consumer (except for reading pacts of different types for a single json).

Using two different consumer names for the same service also breaks a bit the network graph on pact broker side.

@huwtl
Copy link

huwtl commented Oct 22, 2018

@uglyog Just wondering if there's no other way to distinguish between messaging and HTTP style pacts? From the perspective of a user of this library, the consumer name should ideally be the name of the application. Requiring two different names for a consumer application such as "some-app" and "some-app-messaging" feels a bit unnatural. For example, attempting to follow the advice from https://docs.pact.io/best_practices/pact_nirvana#6-use-tags-to-ensure-your-provider-is-compatible-with-your-production-consumer appears to make the process more complicated when an application can potentially have two associated consumer names.

@uglyog
Copy link
Member

uglyog commented Nov 3, 2018

The problem is the verification tooling expects one or the other. It either interacts with a running HTTP server or it invokes methods to get message payloads. It is all setup in the beginning of the verification process before any pact file is loaded, and will require a big change to be able to do both from the contents of one pact file.

@peternelissen
Copy link

Two big downsides:

  • the network graph gets too many different elements. The two "different" consumers are placed somewhere else on the graph. But they are one and the same application.
  • a consumer only has one real name. It's easy to forget about the one that has "-amqp" behind it.

@peternelissen
Copy link

A third downside:

  • using can-i-deploy becomes more difficult. You want to integrate the can-i-deploy in your pipelines, but some projects will have a second name next to the real name.

@uglyog I understand your reasoning of splitting sync and async tests. But this should not stand in the way of naming your consumer with 1 unique name. Maybe its possible to add the type of contract as an extra parameter? (to the name of the json file and in the content itself)

@uglyog
Copy link
Member

uglyog commented Apr 14, 2020

@peternelissen I have no issue with merging the different types of interactions into one file, but the problem is that will require a change to the file format and need to be implemented across about nine language implementations. It's something that needs be to tracked from the specification project https://github.com/pact-foundation/pact-specification.

@broeser
Copy link

broeser commented May 5, 2020

@uglyog I understand that implementing this is very complicated.
Would it be possible to create a configuration page for the Network graph, where one can setup aliases? So that at least the "graph looks wrong"-issue can be solved?

@uglyog
Copy link
Member

uglyog commented May 10, 2020

@bethesque thoughts?

@bethesque
Copy link
Member

@broeser we plan to add support to the Pact Broker for multiple types of pacts between the same consumers, but I cannot give you any estimate of when it's going to be picked up I'm afraid.

Here is the feature request: https://pact.canny.io/feature-requests/p/support-multiple-protocols-of-pact-for-the-same-application

@solarmosaic-kflorence
Copy link

Honestly not a big deal to have to use different consumer/provider names, even though not ideal, but having to modify our CI/CD setup to account for this is quite painful.

@uglyog
Copy link
Member

uglyog commented Jun 2, 2021

Linking #1376

@artemptushkin
Copy link

@uglyog I see this one https://pact.canny.io/feature-requests/p/support-multiple-protocols-of-pact-for-the-same-application is ready in Pact, when can we receive support for this feature in pact-jvm? Or it should work automatically?

@rholshausen
Copy link
Contributor

@artamonovkirill You will need to use V4 Pacts, it should work but this has not been tested fully, so there may still be some areas that don't support it.

@rholshausen
Copy link
Contributor

See https://github.com/pact-foundation/pact-jvm/blob/master/consumer/junit5/src/test/java/au/com/dius/pact/consumer/junit5/V4PactBuilderTest.java for an example

@misko321
Copy link

misko321 commented Jul 19, 2023

Hey @rholshausen

It seems that generating the Pact file containing both HTTP and message interactions on the consumer side works fine in V4, good job with that 👍

But how can both of these types of interaction be tested on the provider side? I cannot have it in one test, cause I need to set one, specific target on the context. And I cannot split it into two tests, because each test executes all contracts, so unavoidably, in each test class contracts of the other target will fail 🤔

@rholshausen
Copy link
Contributor

@misko321 I have created #1708 to track that

@monochromata
Copy link
Contributor

@misko321 as a workaround before #1708 is implemented you might use two different test classes with distinct @PactFilter annotations.

We use

  • @PactFilter(value="^\\/.*", filter = InteractionFilter.ByRequestPath.classs) for the test class that verifies the HTTP interactions (works because message interactions don't have a request path)
  • For the test class that verifies the message interactions: @PactFilter(value={} /* required but unused in our custom filter */, filter = MessageInteractionFilter.class) where MessageInteractionFilter is a custom implementation of the InteractionFilter that returns true for all interactions that are instance of AsynchronousMessage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests