pact-foundation / pact-specification

Describes the pact format and verification specifications
MIT License
297 stars 28 forks source link

Rolling out breaking changes where consumer goes first #29

Open BenSayers opened 8 years ago

BenSayers commented 8 years ago

I'm thinking about how to keep the Pact tests green when you want to roll out a breaking change to an API. Here is a concrete example to help make things clear.:

Imagine I have a get customer api that returns this json response body:

{
    "name": "Bob Smith"
}

I want to change the response body to this:

{
    "firstName": "Bob",
    "lastName": "Smith"
}

In this example the consumer of this API has a pact test for this integration. The consumer also will not be able to deploy if the Pact file does not go green when verified against the production version of the provider.

The easiest way to make this change is to update the provider to include both versions of the responses at the same time.

{
    "firstName": "Bob",
    "lastName": "Smith",
    "name": "Bob Smith"
}

Once the provider has rolled this change out to production the consumer can consume the new API and the pact file containing updated examples will go green when verified against the production version of the provider. Everything went smoothly in this scenario.

Things get more tricky if the consumer wants to go first and support receiving both versions of the responses:

Is there a good way for the consumer to go first? I was thinking that the Pact specification should allow optional values in the response but this seem to have been discussed before and decided to be a bad idea. Any guidance would be appreciated.

I raised the issue here as I'm wondering if the Pact specification needs to be changed to support this use case.

uglyog commented 8 years ago

You've described the best way to achieve what you need. It was first talked about with doing zero downtime database migrations, but the concepts are the same. In essence, if you only ever add to your API and don't rename or remove things, it is easy to implement. With deletes or renames, if you do it over a series of releases, you can rollout safe changes.

As you pointed out (via the link to the FAQ), if we make attributes optional there is a class of errors you can't detect (for instance an misnamed attribute). But that is the general case.

For your requirement we could extend the matching to add an ignore matcher. That way it is explicit, driven from the consumer and easy to remove once your refactor or redesign is complete. It is also very easy to implement.

BenSayers commented 8 years ago

I could have given the example in terms of an additive change and the issue of the consumer going first still arises. Explicitly having the consumer specify that a property can be missing sounds like a nice way to achieve this. I'm thinking that the consumer would use the multiple matcher support in V3 to achieve this use case? For example the consumer could say the property either equals a particular value or is ignored if missing? In order for that to work properly the ignored matcher would have to fail if the property was defined. Is that what you were imagining?

uglyog commented 8 years ago

I was thinking that the ignore matcher would just ignore the attribute, but now I see how having something fail if it was present would be useful, when combined with other matchers. Maybe an absent matcher would be a better name. Or we could have both.

bethesque commented 8 years ago

I've always been uncomfortable with the idea of being able to assert that an element does not exist. It seems to be none of a consumer's business if a provider gives fields that another consumer might be interested in, but it isn't. I can't think of a concrete example of why this is a bad idea yet, but I'll let it percolate and see what my subconscious throws out.

On Tue, Jun 21, 2016 at 10:18 AM, Ronald Holshausen < notifications@github.com> wrote:

I was thinking that the ignore matcher would just ignore the attribute, but now I see how having something fail if it was present would be useful, when combined with other matchers. Maybe an absent matcher would be a better name. Or we could have both.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/pact-foundation/pact-specification/issues/29#issuecomment-227307497, or mute the thread https://github.com/notifications/unsubscribe/AAbPFBKYzjw6TQx6G3sj9MaRZarE4TYVks5qNy3bgaJpZM4I5dOw .

mboudreau commented 8 years ago

I'm in the same boat as Beth. Consumer's shouldn't care about the data that's not there, they should only care about their own data. If you want to do change management for an API breaking change, that's what versioning is for, or add a new/different route.

On Tue, Jun 21, 2016 at 2:20 PM Beth Skurrie notifications@github.com wrote:

I've always been uncomfortable with the idea of being able to assert that an element does not exist. It seems to be none of a consumer's business if a provider gives fields that another consumer might be interested in, but it isn't. I can't think of a concrete example of why this is a bad idea yet, but I'll let it percolate and see what my subconscious throws out.

On Tue, Jun 21, 2016 at 10:18 AM, Ronald Holshausen < notifications@github.com> wrote:

I was thinking that the ignore matcher would just ignore the attribute, but now I see how having something fail if it was present would be useful, when combined with other matchers. Maybe an absent matcher would be a better name. Or we could have both.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub < https://github.com/pact-foundation/pact-specification/issues/29#issuecomment-227307497 , or mute the thread < https://github.com/notifications/unsubscribe/AAbPFBKYzjw6TQx6G3sj9MaRZarE4TYVks5qNy3bgaJpZM4I5dOw

.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/pact-foundation/pact-specification/issues/29#issuecomment-227336961, or mute the thread https://github.com/notifications/unsubscribe/AAjA5Cd_ab7WyO-w_T0bRqUmdJwBwVfFks5qN2apgaJpZM4I5dOw .

uglyog commented 8 years ago

I disagree on the basis that I believe framework and library authors should not be prescriptive. If someone wants to create a test that fails until a provider removes a deprecated field, that is their prerogative.

mboudreau commented 8 years ago

Yeah, I get what you're saying. I don't have a particular problem with it as long as it's its own type and not the default. There's some interesting things you can do if the feature is there, but yeah, we might want to downplay the issue on the provider side, but show it only on the consumer side?

On Tue, Jun 21, 2016 at 2:35 PM Ronald Holshausen notifications@github.com wrote:

I disagree on the basis that I believe framework and library authors should not be prescriptive. If someone wants to create a test that fails until a provider removes a deprecated field, that is their prerogative.

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/pact-foundation/pact-specification/issues/29#issuecomment-227338365, or mute the thread https://github.com/notifications/unsubscribe/AAjA5OH1o6SaO92Vw_z2fdysv0vC1j70ks5qN2oTgaJpZM4I5dOw .

BenSayers commented 8 years ago

I'm not attached to the idea of the consumer being able to assert that an element does not exist, it was floated as a possible way the consumer could write a pact file that represents the fact the consumer is forwards and backwards compatible. I'm open to any alternatives that solve the same problem, though I cannot think of any concrete alternative suggestions myself.

I do think its important that Pact not be prescriptive about how API changes are implemented. I've encountered a number of situations where the provider going first was not practical. For example sometimes due to scheduling reasons the consumer team is ready to work on the consumer side today but the provider team will not be free for several months. Pact's usefulness is greatly increased if it is flexible enough to work smoothly in these situations.

hermanma commented 7 years ago

OPs idea of having both fields would still be a breaking change if the consumer (which I am assuming the producer has no control over) has a deserializer that fails on unknown fields right?

Is there a way for the producer to tag the field such that consumers won't break for sure?

BenSayers commented 7 years ago

I would strongly discourage consumers from being so strict about the response. It creates a very rigid system that is very difficult to change. It means all consumers need to be updated to be aware of any API changes even if they don't need the new data being returned.

bethesque commented 7 years ago

@hermanma do everything you can to change your consumer to ignore extra fields. Even java libraries usually have an option to allow you to do this, even if it's not turned on by default. Otherwise, you're back in the bad old days of WSDL where all the consumers have to be updated whether or not they care about the changes or not.