Add support for branches in consumer version selectors

[bethesque]

The Pact Broker now supports branches as first class entities. You can read more about this here: https://docs.pact.io/pact_broker/branches

Description

Please add support for specifying branch related properties in the consumer version selectors that are sent to the Pact Broker when requesting pacts to verify. Please do not do any verification of the consumer version selectors on the client side - the validation rules are subject to change. Just ensure that any error response is displayed to the user.

• For implementations that wrap the pact-ruby-standalone, update to the latest standalone version
• For implementations that use the rust implementation, update to the latest rust version.
• Add a String branch property to the consumer version selector (if there is a domain model for this).
• Add a Boolean mainBranch (or main_branch for snake case languages) property consumer version selector (if there is a domain model for this).
• Add a Boolean matchingBranch (or matching_branch for snake case languages) property consumer version selector (if there is a domain model for this).
• Expose and document the branch, mainBranch (or main_branch for snake case languages) and matchingBranch (or matching_branch) properties in the user facing API.
• Pass the branch, mainBranch and matchingBranch (must be camelcase) through to the relevant implementation (ruby and/or rust)

Verifying the changes

• Publish test pacts to the test broker

export PACT_BROKER_BASE_URL="https://test.pact.dius.com.au"
export PACT_BROKER_USERNAME="dXfltyFMgNOFZAxr8io9wJ37iUpY42M"
export PACT_BROKER_PASSWORD="O5AIZWxelWbLvqMd8PkAVycBJh2Psyg1"

docker run --rm  \
  -e PACT_BROKER_BASE_URL  \
  -e PACT_BROKER_USERNAME  \
  -e PACT_BROKER_PASSWORD  \
  pactfoundation/pact-cli:0.50.0.14 \
  publish \
  /pact/example/pacts \
  --consumer-app-version fake-git-sha-for-demo-$(date +%s) \
  --branch main

docker run --rm  \
  -e PACT_BROKER_BASE_URL  \
  -e PACT_BROKER_USERNAME  \
  -e PACT_BROKER_PASSWORD  \
  pactfoundation/pact-cli:0.50.0.14 \
  publish \
  /pact/example/pacts \
  --consumer-app-version fake-git-sha-for-demo-$(date +%s) \
  --branch feat/x

• Using your pact client library, verify { "mainBranch": true }
  • You should receive the main pact
• Using your pact client library, verify { "branch": "feat/x" }
  • You should receive the feat/x pact
• Using your pact client library, verify { "matchingBranch": true } with the provider branch set to feat/x
  • You should receive the feat/x pact

[RadekKoubsky]

Hi, I read through the code and if I understand it correctly, it should work like this:

Current code sends the following selectors body with tags to pacts/provider/{provider}/for-verification endpoint:

{
  "consumerVersionSelectors": [
    {
      "latest": true,
      "tag": "master"
    },
    {
      "latest": true,
      "tag": "stage"
    }
  ]
}

With the branch etc. feature, it will also send branch, mainBranch and matchingBranch properties

{
  "consumerVersionSelectors": [
    {
      "latest": true,
      "branch": "master",
      "mainBranch": "true",
      "matchingBranch": "true",
    }
  ]
}

The values of tags for the old code are parsed from maven/sys props: -Dpactbroker.consumerversionselectors.tags=master,stage

The branch, mainBranch and matchingBranch selector properties will be parsed in the same way in the new code.

I have couple of questions:

1. How the new properties are evaluated and sent to broker, are they mutually exclusive or something else? Does the text Please do not do any verification of the consumer version selectors on the client side - the validation rules are subject to change. applies to this?

2. Expose and document the branch, mainBranch (or main_branch for snake case languages) and matchingBranch (or matching_branch) properties in the user facing API. Can you please elaborate on this, does it mean "props should be configurable from annotation and cmd"?

3. Pass the branch, mainBranch and matchingBranch (must be camelcase) through to the relevant implementation (ruby and/or rust) When I look at the provider-jvm code sending the selectors json to its endpoint, I see only kotlin code sending request via http client in the PactBrokerClient#fetchPactsUsingNewEndpoint method.

Can you please confirm my assumption is correct. I will try to extend the selector model with parsing the props and sending them to broker on Monday and see how it works.

Thanks

[bethesque]

The properties in the selector are separate:

{
    "consumerVersionSelectors":
    [
        {
            "branch": "master"
        },
        {
            "mainBranch": "true"
        },
        {
            "matchingBranch": "true"
        }
    ]
}

How the new properties are evaluated and sent to broker, are they mutually exclusive or something else?

Some can be set together, some can't.

Does the text Please do not do any verification of the consumer version selectors on the client side - the validation rules are subject to change. applies to this?

Yes

Can you please elaborate on this, does it mean "props should be configurable from annotation and cmd"?

I would think so. This is a generic issue that has been raised in each of the pact client languages.

When I look at the provider-jvm code sending the selectors json to its endpoint, I see only kotlin code sending request via http client in the PactBrokerClient#fetchPactsUsingNewEndpoint method.

That's the place. As I said before, this is a generic issue that has been raised in all the client libraries. JVM has its own implementation - it does not wrap anything, so you can ignore that bit.

The selectors are meant to be modelled as objects with multiple optional properties. Looking at the example you have of -Dpactbroker.consumerversionselectors.tags=master,stage I don't know how it's been modelled in that interface. You may have difficulties. I'm afraid I don't know anything about the JVM interface, so I can't help you there. You'll need to get @uglyog to give you some pointers.

[tinexw]

Hi @RadekKoubsky , I started working on this issue but got stuck at some point and did not have time go back to it. If you want you can pick up where I left off. My current version can be found here.

Some of the open tasks:

• [ ] Finish the logic to build the selectors based on the annotation values in PactBrokerLoader#buildConsumerVersionSelectors and add tests.
• [ ] Add system poperties for new attributes
• [ ] Decide how to handle the open questions regarding backwards compatibility and the latest tag (s. my comment below) and implement it.
• [ ] Rebase with master and fix the build afterwards.
• [ ] Add providerVersionBranch property.
• [ ] Implement new properties for maven and gradle.
• [ ] (Optional) Improve the way errors from the broker are shown to the user. Right now they are only logged on DEBUG level.

[tinexw]

Hey @bethesque @uglyog ,

I have a question regarding the backwards compatibility and the latest tag.

Currently, the latest tag can only be true or false. It is not possible to omit it. The default is true. I initially thought I can leave it like this for the branch attributes. However, this is not possible because this way it will always be send to the broker and when used in combination with the mainBranch attribute the broker returns an error:

{"errors":{"consumerVersionSelectors":["cannot specify mainBranch=true with any other criteria apart from consumer (at index 0)"]}}

So what I would do instead is to make the latest attribute optional. This would be a breaking change though. As an alternative I could introduce a completely new annotation that replaces au.com.dius.pact.provider.junitsupport.loader.VersionSelector. What do you think?

[bethesque]

So what I would do instead is to make the latest attribute optional. This would be a breaking change though.

Would this be a theoretical breaking change (as in, it's conceptually a change to the user facing API) or a concrete breaking change (it would mean existing code would stop working)?

[tinexw]

From what I understand I think it would be a real breaking change.

From the Consumer Version Selectors docs

If a tag is specified, and latest is true, then the latest pact for each of the consumers with that tag will be returned. If a tag is specified and the latest flag is not set to true, all the pacts with the specified tag will be returned.

With this change, the latest flag would not longer be set to true if it is omitted. Thus, all pacts would be returned instead of only the latest one.

[RadekKoubsky]

@tinexw hi, thanks for starting to implement this issue

regarding the backward compatibility with tags) What about ignoring the latest field when serializing ConsumerVersionSelector.latest to json in case of the mainBranch property is set.

Something like:

if (mainBranch == null){
   obj.add("latest", Json.toJson(latest))
} else {
   log.debug("The 'mainBranch' property is set, excluding 'latest' field from consumer version selector json")
}

I will try to implement it with the new system properties and use it in my scenario.

[tinexw]

Yeah, in theory yes but that it's even more "magic". The current implementation already contains some "magic" e.g. there is the option to either give a list of latest values or a single one or to omit and this is not really documented (or at least I could not find it). I would really opt for simplifying the current implementation instead of adding more "magic". The optimal solution in my opinion would be to just pass through all the attributes as is and have the broker do the validation.

Just in the case above you would need to add the same logic for the matchingBranch for example and there are more properties coming e.g. deployed and released.

[bethesque]

The latest flag is only relevant when you are using a tag, or when you have no other properties specified (mainBranch, deployedOrReleased etc). This is not a breaking change from the API's perspective. Setting "latest=false" has never been a requirement of the API, only that latest=true when it is required.

The API should be called with latest=true if latest is set to true, otherwise, omit the latest property completely.

[bethesque]

FYI, the medium term goal is for "latest" to not even be used any more. The recommended selectors will be { mainBranch: true }, { deployedOrReleased: true } (and { matchingBranch: true } if people like to use matching feature branches). Tags are on the way out: https://docs.pact.io/blog/2021/10/08/why-we-are-getting-rid-of-tags

[bethesque]

I've added a list of examples here

But as I said, please do not validate these locally, as they are likely to change over time. I would have preferred the selectors themselves were not even typed, because we have exactly this problem every time we add new properties to the selectors - rollout takes time and people can't use new Pact Broker features until the client gets an update. Perhaps a better option would be to replace the current selector implementation with a raw hash or json object that gets passed straight into the API.

[RadekKoubsky]

Hi, after messing with the typed implementation, I have decided to pass the raw json from cli which is much simpler and flexible solution as you mentioned.

I have created PR with the untyped configuration passed directly from cli, can you please review, thanks.

EDIT: Please look at it as a not final solution, this is just to get the branches working. It would be great to have the decision "typed/untyped" at the entry point of the provider code and just switch between those two implementations. Unfortunately, I do not have resources to rewrite the provider code within that scope.

[tinexw]

I'll try to answer/elaborate on all the open questions/points/comments ;-)

1. Breaking Change

The latest flag is only relevant when you are using a tag, or when you have no other properties specified (mainBranch, deployedOrReleased etc). This is not a breaking change from the API's perspective. Setting "latest=false" has never been a requirement of the API, only that latest=true when it is required.

What I meant with breaking change is that it's a breaking change for the user of the pact-jvm library. Let's say they are using @PactBroker(consumerVersionSelectors = @VersionSelector(tag = "main")) until now. The selector that currently gets send to the broker is {"consumerVersionSelectors":[{"latest":true,"tag":"main"}]} because latest is automatically set to trueif omitted. Thus, only the latest pact will be returned for the maintag. My phrasing from before might have been misleading when I said "So what I would do instead is to make the latest attribute optional." It currently already is optional. What I actually meant is that I would allow for it to be completely omitted and would change the default from true to not set. This would then change the behavior which I would consider a breaking change. Because @PactBroker(consumerVersionSelectors = @VersionSelector(tag = "main")) would then result in {"consumerVersionSelectors":[{"tag":"main"}]} and all the pacts for the main tag would be returned instead of only the latest one.

2. The existing "magic"

Just wanted to elaborate on this a little what I meant by it. If you define

@PactBroker(consumerVersionSelectors = @VersionSelector(consumer="foo,bar", tag = "main,develop", latest="false,true"))

then four selectors are actually send to the broker:

{"consumerVersionSelectors":[
  {"consumer":"foo","latest":false,"tag":"main"},
  {"consumer":"bar","latest":false,"tag":"main"},
  {"consumer":"foo","latest":true,"tag":"develop"},
  {"consumer":"bar","latest":true,"tag":"develop"}
]"

You can also leave out latest, then it is set to true in all four selectors or you can only define one value e.g. latest="false" in which case it is set to this value in all four selectors. So there are already some ways that go beyond "just passing values through" and thus my feeling is a little bit that there should be no additional logic that modifies the given values because it will become quite confusing.

3. Regarding the raw hash / json object

I understand the issue with

I would have preferred the selectors themselves were not even typed, because we have exactly this problem every time we add new properties to the selectors - rollout takes time and people can't use new Pact Broker features until the client gets an update.

However, I think there are a couple of reasons why it might not make sense in the Java implementation:

• Not really Java-esque. This is maybe more a theoretical argument, but I think in the Java world you are used to using types ;-)
• Poorer readability: In annotations you are quite limited in the types you can use. So you would need to use a string or maybe a list of strings and parse it somehow. Probably the best option would be to parse the JSON as string. Starting with Java 15 and the option to define Text Blocks I can imagine this to be fine since the JSON will be readable. With previous versions, you would need to escape the JSON string which would not be super great for readability.
• Deprecation warnings: I do think it's actually nice that you can get deprecation warnings about deprecated attributes by the compiler ;-) With the raw hash you would lose this.

Do you know how many users actually use the annotation though? Or if values are mostly passed through via system properties anyway?

[bethesque]

Thanks Kristine. That was very helpful.

Do you know how many users actually use the annotation though?

I have no idea!

@PactBroker(consumerVersionSelectors = @VersionSelector(tag = "main")) ... The selector that currently gets send to the broker is {"consumerVersionSelectors":[{"latest":true,"tag":"main"}]} because latest is automatically set to true if omitted.

That is unfortunate.

@PactBroker(consumerVersionSelectors = @VersionSelector(consumer="foo,bar", tag = "main,develop", latest="false,true"))

then four selectors are actually send to the broker:

{"consumerVersionSelectors":[ {"consumer":"foo","latest":false,"tag":"main"}, {"consumer":"bar","latest":false,"tag":"main"}, {"consumer":"foo","latest":true,"tag":"develop"}, {"consumer":"bar","latest":true,"tag":"develop"} ]"

This is confusing to me, and not at all how I intended the selectors to be used. I'm guessing it wasn't easy to map the actual API model to the annotations and compromises had to be made.

What could do is: default "latest" to null (if it is not already). If there is a tag defined in the selector and latest is null, set the latest to true in the JSON selector; if it is true, send it; if it is false, omit it. Otherwise, just pass everything else through, omitting any properties with null values. That would maintain the existing functionality, but make everything else as close to the real API (and hence, the shared user documentation) as possible.

That doesn't really help us with the problem that the JVM version selector doesn't really map well to the underlying selector though. I'm not sure how to implement all the new types of selectors within the existing class.

[RadekKoubsky]

Hi, thanks for the analysis, I am definitely for the easy-to-use provider api implemented in Java. However, I would like to have the option to pass configuration to the broker directly when the provider api does not implement the latest broker api and I don't want to wait until it's implemented.

As I said in #1471 , the main use case of the raw json is: If provider code has not yet implemented the latest broker api, pass raw selectors json directly to the broker api.

Another use case might be "use this as dev feature for testing the new broker api in jvm applications".

[galvo]

Is there any update on this enhancement?

[tinexw]

Hey @galvo , sorry, unfortunately not. There were too many open questions and then I lost track of it unfortunately.

[uglyog]

Just a note that the selectors can be overridden by supplying the raw JSON: See https://github.com/pact-foundation/pact-jvm/pull/1471

[rholshausen]

I have released 4.3.11 with a DSL for selectors for both Gradle and JUnit 5. It may work with JUnit 4, but I have not tested that.