-
Notifications
You must be signed in to change notification settings - Fork 435
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
Consumer stubs co-dependency #224
Comments
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.
Yeah and here comes the tricky part that when the contracts lay in a single repo then indeed we have a problem.
No there isn't.
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 ? |
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! |
Yeah that's a problem indeed.
Yeah you can try to play around with classifiers maybe? You have different options.
Yeah please do! I'm really eager to hear what you've decided to do. Until then I'll close this issue. |
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 Then we could add a flag in the plugins to allow an additional generation of stub
|
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 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 |
The |
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:
|
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 I'm starting to lean towards not doing this feature ATM but I'm still eagerly listening to feedback :) |
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 |
Exactly... all the consumers are going to receive the same response... so the same contracts for all of them... |
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.
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.
@julio-martin wasn't your suggestion about NOT doing the same contract for all consumers but to have contracts per consumers? |
@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. |
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)
1 stub per consumer (separated contracts per consumers)Consumer A
Consumer B
CommentIn reality, you will be receiving both Pros of merging
Cons of merging
|
Then we are losing the concept of contract testing, isn't it? |
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. |
I've talked to Marcin and there are three separate threads:
EDIT: |
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. |
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:
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:
|
Do we know the |
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. |
@castorm and I completely disagree with this statement
You can use either producer or consumer contact approach. For example spring Initilizr uses the former. |
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. |
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? |
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 |
So do we agree we should provide an option to use
Then during consumer tests we could use |
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
will produce a PRODUCER SIDE (bar)The folder structure on the producer side would look like this
will produce a CONSUMER SIDE (baz-service)Assuming that we have a consumer with Here's a test @AutoConfigureStubRunner(ids = {
"com.example:foo:+:stubs:8095",
"com.example:bar:1.0.0:stubs:8096"
}, stubPerConsumer=true) then
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 Are we all on the same page with this? |
LGTM |
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) |
@julio-martin I was on holidays - sorry for the delay :P I took care of this issue in another issue #276 so from our perspective it's better to be consistent in the naming. |
Scenario
Example
contract1 (consumer1)
contract2 (consumer2)
** 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?
The text was updated successfully, but these errors were encountered: