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

Matcher of a type or null #51

Closed
lippea opened this issue May 19, 2017 · 37 comments
Closed

Matcher of a type or null #51

lippea opened this issue May 19, 2017 · 37 comments

Comments

@lippea
Copy link

lippea commented May 19, 2017

Is there a way to match the value to a type or null (not empty, not a string)?

@lippea lippea changed the title Mather of a type or null Matcher of a type or null May 19, 2017
@mefellows
Copy link
Member

Are you referring to optional types? If so, we don't support this. The article explains why.

@lippea
Copy link
Author

lippea commented May 21, 2017

@mefellows not optional attributes, but the value could be null, like {"item": null}
It mean the "item" must exist, but the value is null, which is not part of the contract

@mefellows
Copy link
Member

Hmm I don't believe so. From my perspective, a null value is equivalent to not having it there in the first place.

Out of interest, have you tried?

@lippea
Copy link
Author

lippea commented May 22, 2017

I tried and it failed on the null values and pass on the other values :)
Is this a similar issue? pact-foundation/pact-jvm#261

@mefellows
Copy link
Member

Sorry to say, but it does look like the same one. Leaving open until v3 implementation is completed.

@lippea
Copy link
Author

lippea commented May 22, 2017

thanks @mefellows , that would be great.

@mefellows
Copy link
Member

@bethesque just checking, this is not an available feature correct?

@bethesque
Copy link
Member

No. It's because this test doesn't tell you anything. If you always get nulls in test environment, then you think you've passed, but what then the production environment returns a different type to what you expected, and your code blows up. It will not be supported.

@atikenny
Copy link

I know this ticket is closed but I ran into this just now and wanted to share a different opinion maybe people find it useful.

While developing frontend applications I ran into a lot of cases where some kind of empty(ish) value was present in the response from the backend that created issues on the consumer side. Usually these were empty arrays, empty objects or a string that says "null". This lead to all sorts of long investigation and some defensive programming as a remedy (which I personally don't really like).

So usually after a couple of meetings with service owners we managed to push for providing nothing instead of an empty value in case some data was actually not available. With a very exaggerated example you can end up with the following.

Response:

{
  "usefulData": { "a": 1, "b": 2, "c": 3 },
  "emptyThing": [],
  "almostUsefulData": {
      "butStillEmpty": [],
      "thisDoesntHelpEither": {
          "exxageratedButHappens": []
      }
  },
  "andTheListGoesOn": [],
  "nowThisIsJustRude": "null",
  "butThisIsNotMuchBetterEitherBecauseIStillDontCare": null
}

Which in the consumer will cause code like:

const almostUsefulData = response && response.almostUsefulData;
const butStillEmpty =
  almostUsefulData &&
  almostUsefulData.butStillEmpty &&
  almostUsefulData.butStillEmpty.length;
const thisDoesntHelpEither =
  almostUsefulData && almostUsefulData.thisDoesntHelpEither;
const exxageratedButHappens =
  thisDoesntHelpEither &&
  thisDoesntHelpEither.exxageratedButHappens &&
  thisDoesntHelpEither.exxageratedButHappens;
const hasData = almostUsefulData && (butStillEmpty || exxageratedButHappens);

if (hasData) {
  // show it
}

I left a bug in the above code just to show how easy it is to make one and hard to figure out where it is :).

Instead of this much simpler one:

const hasData = response && response.almostUsefulData;

if (hasData) {
  // show it
}

BUT:
I understand the argument that leaving things out and not testing them can end up causing issues. So what I would do is force a state on the provider when things are available and force another one where they are not and test if things look how I would like them. That way I know that I am ready for the data when I get it and I don't break when I don't have it.

Opinions?

@mefellows
Copy link
Member

I like your slightly contrived, a little bit long yet also illuminating example :)

In any case, yes, our standard advice is to make sure you explicitly test the case where values exist, don't exist or are returned as empty values (e.g. null or []). These may or may not be done as part of Pact tests, but we would suggest they are captured in pacts and made explicit back to any provider.

@atikenny
Copy link

Thanks for the quick response, sorry for the example but it really is not that far from reality.

So if I understand it correctly the only thing missing from Pact to validate these kind of responses from the provider is something saying:

// GIVEN
state: "you don't have data"

// WHEN
calling the provider

// THEN
"missingData" is not present

So for now all we can do is write pact for when things DO exist and use other forms of testing when things are missing.

@mefellows
Copy link
Member

There's nothing stopping you from doing exactly that now. That's the whole point of provider states.

Or am I missing something?

@atikenny
Copy link

Probably I am missing something because I couldn't find a way to check with pact for things that should NOT be present. Time to google...

@mefellows
Copy link
Member

You would write a test as you stated above with a state indicating so (e.g. given almostUsefulData is empty) and in the willRespondWith you would simply omit the fields you were previously expecting. Now you have a contract with two interactions - one with and one without data. You then must ensure your provider will honour those two different states.

@kxramas
Copy link

kxramas commented Nov 15, 2019

It is not clear to me if there are 10 fields in json which can either have a value or not have value or takes only defined format then should we write additional 30 pact tests each with a state of a field testing all combinations of possible values for all fields ?

Thanks

@mefellows
Copy link
Member

@kxramas It's hard to say without more context - it's unusual that every field is optional however, and usually there are different sub-shapes in the payload you might be separately interested in which could form the basis of different test expectations. For example, a User may have a "profile" sub-object which is optional (perhaps that information is collected later on). Often times, this can be improved by having links to the resource if available (e.g. using HAL links), but sometimes you can't change the implementation.

If they are all indeed optional, I'd first look at how you might be able to improve the API. The key thing is how your client behaves in the different circumstances - will it not work if none of the fields came back, or just a few? That might help you in your endeavour.

@bethesque
Copy link
Member

If it was me, I'd do one test which had every field populated, and one that had the minimum that could be expected. Really, what you're testing in this case is that code that deserialises the response into your local domain objects can handle both scenarios.

@maximvl
Copy link

maximvl commented Jan 15, 2021

Hello, I would like to bring this up again because this is a serious limitation of the tool. In our company we have api responses with hundreds of fields of which dozens are optional and can be null. Unless I'm missing something about your position on optional types you suggest to write hundreds of cases for each combination of null/type fields which is terribly unpractical. Also you must understand that after a certain size projects will not adapt their code and style just for one tool. I understand the desire for ideal world where everything is perfectly typed but in fast growing environment this simply does not happen. As you see the amount of requests for this feature I suggest you to reconsider it. Thank you.

@mefellows
Copy link
Member

Hi Max, thanks for the feedback. I can certainly empathise.

At this stage, we're exploring the idea of "provider driven" contracts, where more of a schema comparison from the provider side against consumer test examples. You could think of this as a contained experiment, and learnings may be applied into the Matcher library. At this stage, as you can see from the above discussion (and Pact's official position on it, changing the core Matchers to support this is unlikely to change quickly.

See pactflow/roadmap#4 (Pactflow specific) and pact-foundation/pact-specification#76 for future thinking that encompasses your suggestion.

@maximvl
Copy link

maximvl commented Jan 15, 2021

@mefellows thank you for fast response :)

I found in specification 3 there is a combine field which can have or value with a list of matchers, this seems like a way to implement optional matching, is that correct?

@mefellows
Copy link
Member

You're welcome.

With the combine matcher, I believe what you're suggesting is in theory possible, but I'm not entirely sure I'm afraid.

@uglyog @bethesque care to weigh in on this one?

@maximvl
Copy link

maximvl commented Jan 15, 2021

I checked into the v3 features, I was able to generate specs but it seems the verifier does not support and style of matchers yet. Also I think contract generator does not have an api for combined fields.

@bethesque
Copy link
Member

In our company we have api responses with hundreds of fields of which dozens are optional and can be null

Do you need to test all the combinations? How few examples could you use to cover all the fields?

but it seems the verifier does not support and style of matchers yet.

No, it's not supported in v2. Which language are you verifying in?

It sounds to me like Pact is not a good fit for this particular use case. I would look at using the "Atlassian flow" which allows you to verify a pact against an OAS. You can continue to use Pact on the consumer side, but verify using https://www.npmjs.com/package/swagger-mock-validator on the provider side.

@bethesque
Copy link
Member

Or wait until Pactflow's "provider driven" flow is out, as Matt suggested.

@mefellows
Copy link
Member

mefellows commented Jan 17, 2021

You can continue to use Pact on the consumer side, but verify using https://www.npmjs.com/package/swagger-mock-validator on the provider side.

It's also worth expanding on that briefly, because it might sound like a limitation (still having to use Pact on the consumer side). But actually, it allows you to vastly expand your suite of consumer tests without burdening the provider side, because all it does is compare schemas. It also allows this specific use case.

@maximvl
Copy link

maximvl commented Jan 19, 2021

@bethesque thanks for response,

How few examples could you use to cover all the fields?

So this is exactly my point, since different fields are optional Pact contracts will not cover the responses our backend can produce. Unless I'm missing something.

I understand this is not supported in v2 so I'm looking forward v3, actually I'm preparing a pr because it seems that it can be done.

@mefellows
About the swagger mock validator - actually I'm probably not fully understanding things, currently our setup is like this that we are not using broker and just running pact-verifier in our backend CI pipeline, but the idea is to expand this. You are saying "all it does is compare schemas" - is that similar to Pact verifier? Or there are more things happening in the verifier?

@mefellows
Copy link
Member

@maximvl apologies for the delay.

The basic flow:

  1. Provider validates their API matches a given OAS (in the future, we'll support other modes here) using whatever tool they choose (e.g. Dredd)
  2. Provider publishes OAS to Pactflow, along with the evidence in one
  3. Provider runs can-i-deploy to ensure the contract is compatible with its consumers

Separate to the above (before/after) the consumer runs its own pipeline:

  1. Consumer writes and publishes contracts (via Pact) as they do today
  2. Consumer runs can-i-deploy, which will check if the given provider's OAS is compatible with the pact

The pact contract is now statically compared to the OAS, using a type of schema verification. If a field is null or not in the pact, so long as the OAS schema allows it it is ✅ .

See also pactflow/roadmap#4. Please note that this is a feature that will be unique to Pactflow. It may be made open via the OSS pact broker eventually, but it's not currently planned.

@pavelkomarov
Copy link

What if instead of "oops everything could fall through because it's all null", you implement syntax like

[
  1639587840000,
  77.8544510992,
  0.3886455188,
  "upright",
  "walking" & null
]

where the test case then has to hit both things on either side of the & condition?

@mefellows
Copy link
Member

I think we've talked about something like that. You would still need a way of communicating those two states to the provider, so presumably it would need to map to a provider state (e.g. by convention). How do you envisage the consumer DSL and provider side working here?

@akanksha-centric
Copy link

Hello, I am also struggling with similar kind of issues, I am writing test case for an api having 100 of filed and there are multiple optional/null value fields that some time return value or some time not. I have also read pact optional attribute but it is not helpful I don't want to create multiple scenarios, What are the other why to validate both attribute having null or some value.

and is there are any way to compare only schema of expected response not value?

@mefellows
Copy link
Member

Hi @akanksha-centric.

No, Pact doesn't have a separate way of doing this other than what is discussed here.

If you want a purely schema based test, you might be interested in a feature releasing next week at Pactflow which does exactly this: https://pactflow.io/bi-directional-contract-testing/#register

@pavelkomarov
Copy link

pavelkomarov commented Mar 16, 2022

Pact should allow nullable fields. I don't care what the orthodoxy says. The answers given here are objectively unhelpful. Nullable fields are such a basic thing apps do. At my company we literally cloned and modified a pact verifier, because we really needed this functionality.

@mefellows
Copy link
Member

mefellows commented Mar 17, 2022

Pact should allow nullable fields. I don't care what the orthodoxy says. The answers given here are objectively unhelpful. Nullable fields are such a basic thing apps do. At my company we literally cloned and modified a pact verifier, because we really needed this functionality.

That is the beauty of open source! You can disagree and fork.

I know it's hard, but as soon as you start to follow the line of thinking it quickly moves the framework away from what is uniquely powerful with Pact and its core design - and at that point you may as well do schema validation through other means and tools. We've taken the choice that it's best to be strict, even if that means it doesn't work for everyone.

@dantswain
Copy link

Hi I just wanted to add a couple data points here. I don't think my use case completely makes the argument one way or another but I haven't seen this perspective represented in the discussion:

  • I have an API that returns a list of objects, and each of those objects has one or more optional fields. It is a perfectly valid state to have a mix of them in the response. On the other hand, having a list where every item either has all of the optional fields set or doesn't have any of them set is extremely unlikely. I think I can figure out how to test this without it getting combinatorically complex, but it's a bit of a mind warp trying to rationalize it and then I'm going to have to explain it to everyone on my team who is even less familiar with PACT.

  • I'm in an organization that is pushing PACT adoption (which I generally think is a good thing), so "use something else" isn't that helpful of a suggestion.

@mefellows
Copy link
Member

Thanks @dantswain. I don't think this changes the philosophical argument here but another data point is helpful.

The reality is, there are other solutions to test this kind of contract (the most common and readily available being a schema check). After many debates, we always land on "why would Pact support this when there are other tools that can do it" with the additional "now it's a different mode we need to support / document / maintain" and finally, we have to explain the loss in fidelity that that kind of testing enables. That is, a schema check enables a wide range of false positives (see https://pactflow.io/blog/schemas-are-not-contracts/ for a wider post on the matter).

@daniel-hyun-chae
Copy link

daniel-hyun-chae commented Sep 19, 2023

For me, it makes sense to allow optional field definition in contract because that's the truth about the contract.
It should be up to the provider test to verify all different cases against the contract.
Maybe some matrix such as contract coverage will be awesome to see how much of the contract is covered by the provider tests. The capability to write negative tests on provider side would help this even further.

I don't understand why Pact is trying to prevent optional field on principle level.
On consumer side, sometimes, it doesn't really matter whether something is null or string.
If being scenario specific is important, more targeted contract can be written only for those special cases.
In general, being able to use type definition or schema to create contract is a great help for the maintenance. So, it would be nice to have the freedom to choose it.

@mefellows
Copy link
Member

It should be up to the provider test to verify all different cases against the contract.

No, that is not what contract testing is about. The whole point of capturing the consumer contract is to avoid the ambiguity in such cases.

On consumer side, sometimes, it doesn't really matter whether something is null or string.

It's quite simple, if it doesn't matter, exclude it from the contract altogether.

If being scenario specific is important, more targeted contract can be written only for those special cases.

Exactly.

In general, being able to use type definition or schema to create contract is a great help for the maintenance. So, it would be nice to have the freedom to choose it.

Pact is simply not a schema checking tool - there are plenty of schema checking tools out there, Pact is not one of them.

For a long form on why schemas are not contracts, see this post: https://pactflow.io/blog/schemas-are-not-contracts/.

I'm going to lock this conversation, the position from Pact is clear on this topic.

@pact-foundation pact-foundation locked as too heated and limited conversation to collaborators Sep 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

10 participants