Spring-cloud-contract: Consumer stubs co-dependency

[castorm]

Scenario

Given a _contract1_ from _consumer1_ to _producer_ over an _endpoint_
When a _contract2_ from _consumer2_ to _producer_ over the same _endpoint_ is created with a more lenient response body
And _producer:v2:stubs_ is produced
Then _consumer1_ is potentially inadvertently broken by using _producer:latest:stubs_

Example

contract1 (consumer1)

Contract.make {
    request {
        method 'GET'
        url '/endpoint'
    }
    response {
        status 200
        body([
                name        : 'name'
                description : 'description'
        ])
        headers { contentType(applicationJson()) }
    }
}

contract2 (consumer2)

Contract.make {
    request {
        method 'GET'
        url '/endpoint'
    }
    response {
        status 200
        body([
                name        : 'name'
        ])
        headers { contentType(applicationJson()) }
    }
}

_** It's all the same but the missing description field._

Description

When running consumer1 tests with the stubs generated from previous contracts, it might be that the chosen stub is based on contract2, hence violating consumer1 expectations.

Ideally, consumer1 shouldn't even know consumer2 even existed, and it definitely shouldn't be affected by consumer2's contracts.

Reviewing the documentation, packaging all contracts by producer is the proposed approach, but it's flawed by the aforementioned lack of isolation between consumers.

_Am I missing something?_

Options

Priority
I understand there is the priority attribute to help, but I don't think is meant for this purpose, as it requires consumers to go messing with each other's contracts. Also it requires the consumer to be aware of the existence of potential clashes.

Consumer self-declared identification
If the consumer was able to identify itself when performing the request (i.e. in a header), it could use that in the contract. But it would still require for the consumer to be aware that clashes are possible.

Package by producer+consumer not just producer
It would be possible to isolate consumers by producing different artifacts by the pair (producer, consumer) instead of just producer, but it feels kind of overkill and not documented, so future users might find themselves in the same spot we are now.

_Is there any other option built-in SCC?_

_Should there be?_

[marcingrzejszczak]

Hi @castorm

thanks a lot for the issue. It's really interesting and perfectly filed! I gave it a thought and this is what I think.

I think priority is the way to go. Now, If the contracts lay with the producer then it's the producer's responsibility to ensure that the priority is properly set.

but I don't think is meant for this purpose, as it requires consumers to go messing with each other's contracts

Yeah and here comes the tricky part that when the contracts lay in a single repo then indeed we have a problem.

Is there any other option built-in SCC?

No there isn't.

Should there be?

I was thinking about this and it can be very, very difficult to achieve actually. We'd have to parse the bodies and do a diff between contracts. Believe me or not but JSON is a terrible structure to work with so it can be really problematic to achieve.

I think that if the consumers follow the convention that all consumer expectations lay in the same root folder for a given producer, then it's still the producer's responsibility to ensure that priorities are properly set.

Does it make any sense @castorm ?

[castorm]

Hi @marcingrzejszczak, thanks for your reply.

I understand there is no easy way around it, and I'm not sure it makes sense trying to be too clever about it in the tool, even if diffing contracts was easier. Actually, even if both contracts above returned the same properties, different values could also break consumers' tests, so it wouldn't solve it anyway.

For us the solution is not perfect, as it requires for the producer to visually assess whether a new contract could break other client's expectations. But there could be many contracts, and conflicts not always obvious.

On a personal note, I'd rather keeping producer's role as supplier/facilitator instead of an in-depth supervisor of contracts.

I'm more inclined to reorganize the repo so we have different contract projects per consumer. This way consumers could only break themselves, and breaking changes would be closer to the breakage, both in time and in code, hence easier to figure out. Consumers would be the ones deciding how to design their own contracts.

Sadly, this will mean fighting a bit against SCC as it's designed to refer to artifacts as if they were producer's, so result might not be obvious and introduce more complexity.

Anyway, we'll think about whether it's worth the try or not and let you know our results.

Thanks!

[marcingrzejszczak]

For us the solution is not perfect, as it requires for the producer to visually assess whether a new contract could break other client's expectations.

Yeah that's a problem indeed.

I'm personally more inclined to reorganize the repo so we have different contract projects per consumer. Although this will mean fighting a bit against SCC as it's designed to refer to artifacts as if they were producer's, so result might not be obvious and introduce more complexity.

Yeah you can try to play around with classifiers maybe? You have different options.

Anyway, we'll think about it and let you know our results.

Yeah please do! I'm really eager to hear what you've decided to do. Until then I'll close this issue.

[marcingrzejszczak]

After discussing this on Gitter with @julio-martin it turned out that he had a very similar issue

//CONSUMER A
    request {
        method 'GET'
        url '/api/v1/xxxx'
    }
    response {
        status 200
        body([[
                      code     : 'AAA',
                      languages: ['en', 'es', 'pt']
              ], [
                      code     : 'BBB',
                      languages: ['en', 'es', 'pt']
              ], [
                      code     : 'CCC',
                      languages: ['en', 'es', 'pt']
              ]])
        headers { contentType(applicationJson()) }
    }

//CONSUMER B
     request {
        method 'GET'
        url '/api/v1/xxxx'
    }
    response {
        status 200
        body([[
                      code     : 'AAA',
                      currencies: ['USD', 'EUR']
              ], [
                      code     : 'BBB',
                      currencies: ['USD', 'EUR']
              ], [
                      code     : 'CCC',
                    currencies: ['USD', 'EUR']
              ]])
        headers { contentType(applicationJson()) }
    }

Basically it's impossible to discern between the two. One option is to merge 2 contracts into one but actually that's more of a workaround to the problem.

So we could do the following

On the producer side / in the common repo you'd have to do sth like /src/test/resources/{consumer}/some/folder

Then we could add a flag in the plugins to allow an additional generation of stub per consumer generation. That way we could have 2 options

• a generic stub is created with all the stubs producer-stubs.jar
• a generic stub + a stubs per consumer are created producer-stubs.jar and producer-consumer1stubs.jar

WDYT @julio-martin @jkubrynski @castorm @Fitzoh

[jkubrynski]

I think it could bring more problems than potential profits. On production, the server is producing the same response to the both consumers - so it will always contain description field and for the second case fields currencies and languages will always exist in the same response.

What we should do is to verify if the contract between client and server is valid. If you confirm the customer can talk to the server returning currencies only it doesn't mean it could also handle request with languages field - because you've missed @IgnoreUnknownFields for json or you have incorrect schema for XML mapping.

[marcingrzejszczak]

The @IgnoreUnknownFields argument is sth that really makes sense for me. I've seen cases when users didn't have it turned on and there were stupid bugs out there.

[julio-martin]

I think, create a stub per consumer will help a lot, right now, a consumer can break other consumer, so from a producer perspective there is a problem of consistency and maintanability:

1. As a consumer, you don't have to know what are the contracts of others consumers.
2. Contracts as a documentation have the correct sense, so every consumer contract will have what they expect.
3. As we mentioned, you can have different response for the same request depending on the consumer, that's what you need in your tests as a consumer.
4. Producers are going to remain the same, they are going to check that returns what every consumer expects.

[marcingrzejszczak]

Yeah @julio-martin but if we go towards having stubs per consumer then the response in your contract might be ok for in your integration tests. When however you do some tests on deployed apps, suddenly the response you get will be different (cause it will contain other fields) and if you don't set the @IgnoreUnknownFields flag then your application will break.

I'm starting to lean towards not doing this feature ATM but I'm still eagerly listening to feedback :)

[castorm]

Maybe I'm missing something but... otherwise, how is this consumer driven contract? a single contract defined by the producer would be enough for all consumers, right? that'd be producer driven.

Regardless of @IgnoreUnknownFields if you as a consumer need to receive all of (and only) the fields you are expecting, you can still represent that in the contract, but as far as I know, there is no guarantee on producer's contract test that ensures you are receiving all of (and only) the fields you are expecting. So you never will be testing this. By definition @IgnoreUnknownFields is assumed (which IMO is also the way to go).

[julio-martin]

Exactly... all the consumers are going to receive the same response... so the same contracts for all of them...

[marcingrzejszczak]

Maybe I'm missing something but... otherwise, what's the point of having consumer driven contracts? a single contract defined by the producer would be enough for all consumers, right?

If the producer says "how it's going to be" then it's producer contract. If consumers drive the changes of contracts - it's consumer driven. So now we're talking only about whether some contracts should be merged into one if multiple teams want to drive the changes of the same endpoint or should they be completely separate.

Regardless of @IgnoreUnknownFields if you as a client need to receive all of (and only) the fields you are expecting, you can still represent that in the contract, but as far as I know, there no such guarantee when testing the producer, is there?

On the producer side tests will be generated and whatever you put in the request / response producer side will be sent to the producer. I don't understand this question.

Exactly... all the consumers are going to receive the same response... so the same contracts for all of them...

@julio-martin wasn't your suggestion about NOT doing the same contract for all consumers but to have contracts per consumers?

[julio-martin]

@marcingrzejszczak yep, I would like to have different contracts per consumer requirements, I don't want to do a workaround that implies that contracts are not the "real" ones.

That's why I think a stub per consumer can help a lot.

Maybe, I missunderstood you, but I think you were suggesting to add the real response of the producer instead of adding in the contract what the consumer expects.

[marcingrzejszczak]

But actually what @jkubrynski suggest is closer to the "real" one then what would happen with stubs per consumer.

1 stub per all consumers (merged contract)

request {
   url '/foo'
   method GET()
}
response {
    status 200
    body(
       foo: "foo",
       bar: "bar"
    }
}

1 stub per consumer (separated contracts per consumers)

Consumer A

request {
   url '/foo'
   method GET()
}
response {
    status 200
    body(
       foo: "foo"
    }
}

Consumer B

request {
   url '/foo'
   method GET()
}
response {
    status 200
    body(
       bar: "bar"
    }
}

Comment

In reality, you will be receiving both foo and bar fields cause that's how the producer works. It turns out that Consumer A doesn't care about bar and Consumer B doesn't care about foo. But those fields will be there. So actually the 1st scenario is the "real" one. Two others are situations where the consumer says what it cares about. Right?

Pros of merging

• If we merge this into a single contract then we have less work as producers to support the base class.

Cons of merging

• We no longer know which consumers uses us cause we've merged 2 contracts into one

[julio-martin]

Then we are losing the concept of contract testing, isn't it?

[fitzoh]

If you start using shared consumer contracts, does that open the door to someone modifying the shared contract and unintentionally breaking consumers that they weren't aware of?

Say there are 5 consumers which have that same interaction, and there's a large coordinated change to updated it between 4 of the consumers but the 5th somehow fell through the cracks.

I'm a little concerned that this kind of loses the point of consumer driven contracts and instead just becomes a producer oriented contract.

[jkubrynski]

I've talked to Marcin and there are three separate threads:

1. When you use contract-per-consumer, in fact, you're testing the stub you expect to receive, not the one you will receive on production, which sometimes can cause serious issues (for example when you validate schema or don't want to ignore unknown fields).
2. What we could do is to provide an option in the stub runner, to specify a pattern for mappings that should be used to feed the Wiremock. For example you keep you contracts in contracts/consumer1/*.groovy and in @AutoConfigureStubRunner you could specify pattern="**/consumer1/**
3. Still, we're looking for the best way, to figure out, we're breaking the contract with the consumer 'X' during server compilation. It would be great to be able to check what was the contract version the consumer verified in the version which is currently running on production.

EDIT:
the name of the flag can be stubsPerConsumer

[marcingrzejszczak]

I'm thinking also about the case where the plugins could see that there are 2 contracts with the same request side but different responses. In that case, it could produce a warning message in the logs or sth.

[castorm]

Although I think the pattern proposal would help with the problem at hand, I can't help but think it's a workaround instead of a built-in solution.

I think the convention for stub resolution is where we should be focusing on:

stubrunner:
  ids:
    - producer-groupId:producer-artifactId:+:stubs

The fact the consumer is not considered at all in the expression for importing the stubs settles this assumption about the stubs being common for all consumers, which IMO is wrong.

I don't think you can satisfy all consumers with the same stub and at the same time let the consumers drive the contract (which is used to generate the stub).

So to me solutions are:

• Not to drive contracts by consumer (which would defeat the purpose of the library).
• Not to generate stubs from contracts (contracts are consumer-driven, but stubs are not, causing the issue at hand).
• Accept that stubs are effectively driven by consumers and reflect that in the way they are generated and used.

[jkubrynski]

Do we know the spring.application.name or artifactId of the application we're testing during stubs download?

[marcingrzejszczak]

I don't fully agree but I understand your concept.

The Best built in solution will be to follow a convention. Inside your producer or the common repo it will be enough just to keep the contracts of a given consumer under the folder that represents its (groupid)/artifactid names. Then if you set a flag called useStubsPerConsumer then the proper ones will be picked. I think that's the cleanest solution.

[marcingrzejszczak]

@castorm and I completely disagree with this statement

Not to drive contracts by consumer (which would defeat the purpose of the library).

You can use either producer or consumer contact approach. For example spring Initilizr uses the former.

[castorm]

Why don't you agree?

I'd try to settle the discussion on what's (opinionatedly) right and wrong first before jumping to solutions. Otherwise, you could be establishing a wrong default.

[castorm]

You can use either producer or consumer contact approach. For example spring Initilizr uses the former.

Ok, I see your point. I guess it's up to you guys to consider how do you expect your tool is going to b e/should be mostly used.

Although, I don't fully understand the need for producer-driven. Is it just to leverage the stub generation? you could be using directly wiremock for that, couldn't you?

[marcingrzejszczak]

You can read the whole discussion about this in this topic or on Gitter. Like I mentioned previously both approaches presented in this thread have their good and bad sides. That's why I say that I don't fully agree. But I understand the reasoning behind both approaches

[jkubrynski]

So do we agree we should provide an option to use stubPerConsumer? Now we need to find out the easiest way to define which contract should be applied for particular client. IMHO we could use sub-roots in contracts directory for each consumer:

• contracts/consumer-a
• contracts/consumer-b, etc

Then during consumer tests we could use spring.application.name as a default to find stubs, when stubPerConsumer is set to true. Otherwise mapping can be defined the same way we currently use for converting artifact-id to stub-id

[marcingrzejszczak]

Yeah I'll rewrite this so it's clear what we want to achieve.

PRODUCER SIDE (foo)

The folder structure on the producer side would look like this

src/test/resources/contracts/baz-service/some/contracts/...

will produce a foo-stubs.jar

PRODUCER SIDE (bar)

The folder structure on the producer side would look like this

src/test/resources/contracts/baz-service/some/contracts/...

will produce a bar-stubs.jar

CONSUMER SIDE (baz-service)

Assuming that we have a consumer with spring.application.name=baz-service

Here's a test

@AutoConfigureStubRunner(ids = {
    "com.example:foo:+:stubs:8095",
    "com.example:bar:1.0.0:stubs:8096"
}, stubPerConsumer=true)

then

• the foo-stubs and the bar-stubs are downloaded and unpacked
• two http stub servers are started on ports 8095 and 8096
• before feeding the servers with stubs we analyze the path (the simplest way will be to just check whether the path contains some value instead of checking if that value is exactly in some place in the path. Also we want to accept stubs that are created via assembly plugins and they have a different structure)
  
  
  
  • if the path contains the baz-service then the mapping will be applied to the http server stub
  
  
  • if the path contains the baz-service then when message will be triggered we will allow message triggering
  
  

I think that if someone wants to tweak the name one can create a profile, or in that particular test explicitly define some other value of spring.application.name. I don't think there's a need for a new prop.

Are we all on the same page with this?

[jkubrynski]

LGTM

[julio-martin]

It looks good to me, the only thing to take into account, I think you can´t put '-' in the folder of consumers if I don´t remember wrong because the Autogenerated tests can´t have that character, so the spring.application.name could have that character, is it like that? (sorry for the late response, I was so busy yesterday)

[marcingrzejszczak]

@julio-martin I was on holidays - sorry for the delay :P I took care of this issue in another issue https://github.com/spring-cloud/spring-cloud-contract/issues/276 so from our perspective it's better to be consistent in the naming.