1. Atlassian Avatar Atlassian
  2. Tooling and Automation
  3. swagger-mock-validator
  4. Issues

Test incorrectly passes when mock expects a field that is not in the spec

Issue #84 new
Tim Jones created an issue

Given the following pact file, which defines a json response with a string property named “someField“, and the example swagger document from https://petstore.swagger.io/v2/swagger.json (which does not define that field), validation passes.

‌

Expected behaviour: When the pact file expects a field in the response that isn’t defined by the spec, validation fails

Actual behaviour: When the pact file expects a field in the response that isn’t defined by the spec, validation passes

{
  "consumer": {
    "name": "MyConsumer"
  },
  "provider": {
    "name": "pactWith"
  },
  "interactions": [
    {
      "description": "A get request to get a pet 1845563262948980200",
      "providerState": "A pet 1845563262948980200 exists",
      "request": {
        "method": "GET",
        "path": "/v2/pet/1845563262948980200",
        "headers": {
          "api_key": "[]"
        }
      },
      "response": {
        "status": 200,
        "headers": {
        },
        "body": {
          "someField": "some string"
        },
        "matchingRules": {
          "$.body.someField": {
            "match": "type"
          }
        }
      }
    }
  ],
  "metadata": {
    "pactSpecification": {
      "version": "2.0.0"
    }
  }
}

‌

After a bit of digging, I see that [matching rules are not supported](https://bitbucket.org/atlassian/swagger-mock-validator/src/6e1175a9e3e83f569d7790b41ba62e1fa72a14a5/docs/PACT.md). However, I don’t think this is the reason, as the following pact without the matching rules also (I think incorrectly) passes the test: 

{
  "consumer": {
    "name": "MyConsumer"
  },
  "provider": {
    "name": "pactWith"
  },
  "interactions": [
    {
      "description": "A get request to get a pet 1845563262948980200",
      "providerState": "A pet 1845563262948980200 exists",
      "request": {
        "method": "GET",
        "path": "/v2/pet/1845563262948980200",
        "headers": {
          "api_key": "[]"
        }
      },
      "response": {
        "status": 200,
        "headers": {
        },
        "body": {
          "someField": "some string"
        }
      }
    }
  ],
  "metadata": {
    "pactSpecification": {
      "version": "2.0.0"
    }
  }

‌

It is possible this is the same issue as https://bitbucket.org/atlassian/swagger-mock-validator/issues/83/tool-is-not-catching-valid-errors

Comments (15)

  1. Tim Jones reporter

    I think this issue is also related https://bitbucket.org/atlassian/swagger-mock-validator/issues/78/response-parser-removes-required , although this case is slightly different.

    I suppose it is technically correct to say:

    “Hey, I require a field named ‘someField', does your API spec say you return it?“
    ”Well, I’ve never heard of that field, but it is valid to have any additional properties you like, so I’m going to say yes”

    however, I can’t think of a time when that behaviour would be expected.

    Pact says “I require at least these fields“. Swagger says “Here are some fields I can produce, some are required, some not“.

    There are two directions here:

    1. Response fields required by the swagger but not present in the pact (I believe this is what is discussed on issue #78). I think passing validation if the pact response doesn’t name all spec-required fields is correct, since pact describes the subset your application requires, not the entire expected response.
    2. Response fields required by the pact but not present in the swagger (this bug report). I think validation should fail if the swagger spec does not explicitly name the fields listed in the pact.

    ‌

  2. Tim Jones reporter

    If the resolution ends up being “by design because of the way additionalProperties works“, would you consider adding an option to override this behaviour?

    My primary use case for this validator is to do partial verification when it’s not possible to add pact verification to the provider. When this is the case, we also don’t control the swagger.

    ‌

  4. Tim Jones reporter

    I agree. However, the tool currently exits with a success (0) when the pact file expects fields that are not present in the provider’s spec.

  6. Ben Sayers

    The reason that swagger mock validator accepts this Pact file is because the response body schema allows additional properties. We get a lot of questions about this topic internally, I’ll share our FAQ on the subject below, have a read of it if you want more detail behind this design decision.

    I have a misspelled property in my pact response body but swagger mock validator says that this is valid, why?

    Answer

    The key to understanding what happens here is to understand how additionalProperties works in JSON Schema (which is what Swagger uses to define the shape of request and response bodies).

    additionalProperties can have one of three values:

    • true which means additional properties are allowed on the object
    • false which means additional properties are not allowed on the object
    • object where the object is a json schema object, which means additional properties are allowed on the object but the values of each property must match the json schema provided

    Critically, when additionalProperties is not defined it defaults to true.

    My advice for Swagger authors would be to follow Postel's law and be conservative in what you send and liberal in what you accept. So request bodies should allow additional properties in objects (liberal) and response bodies should not allow additional properties in objects (conservative).

    Why didn't we just make swagger mock validator fail when additional properties were detected in response bodies by default? Well we actually started this way but ran into the following issue:

    Imagine you have this schema as a response body:

    {
    "properties": {
        "firstName": {
            "type": "string"
        },
        "lastName": {
            "type": "string"
        }
    },
    "type": "object"
}

    If swagger-mock-validator did default to disallowing additional properties in response bodies then this body would be valid:

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

    And this body would be invalid:

    {
    "first": "Bob",
    "last": "Smith"
}

    So far everything is ok. But now imagine we changed the response body schema to this:

    {
    "allOf": [
        {
            "properties": {
                "firstName": {
                    "type": "string"
                }
            },
            "type": "object"
        },
        {
            "properties": {
                "lastName": {
                    "type": "string"
                }
            },
            "type": "object"
        }
    ]
}

    The allOf operator is a way of passing multiple schemas and applying them to the same object. In order to be considered valid the object must pass validation of both schemas. So now lets take the example that worked for the previous schema:

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

    This example will fail to validate against both of the nested schemas! In the schema containing the firstName property it will fail because lastName is an additional property, and in the schema containing lastName it will fail because firstName is an additional property. The only value that would be considered valid is an empty object!

    So we had to change the behaviour of swagger-mock-validator to follow the defaults of JSON Schema so that we did not break keywords such as allOf, anyOf and oneOf.

  7. Tim Jones reporter

    ^ This is a great write up. I hadn’t considered the impact of allOf, and I see the technical challenge.

    I think the point remains that this behaviour isn’t what a lot of users expect (especially given the number of questions it seems to generate). I think the current behaviour could only produce low confidence contract validation, unless the authors of the swagger were very strict (in my experience, this usually isn’t the case). However, I see why you can’t change the behaviour directly.

    Have you considered emitting a warning when properties not described in the unified spec are required in the Pact?

    If warnings were emitted, and it were possible to make warnings fail the validation, then I think confidence that a spec-implementing provider would satisfy the Pact contract could be considerably higher.

    What do you think?

  8. Ben Sayers

    We have considered emitting this as as warning, the two major concerns are:

    • There is no clean way to determine if a property is truely undefined

      • In cases not involving logical keywords there’s no clean way to determine if a property truely is undefined or if the swagger is intentionally allowing any property. Examples:

        • An API that accepts a generic map of key value pairs

          • Schema: {type: "object"}
          • Pact value: {foo: "bar"}

        • An API that accepts a mix of predefined properties and a generic map of key value pairs

          • Schema: {type: "object", properties: {id: {type: "string"}}, additionalProperties: {type: "string"}}
          • Pact value: {identifier: "some-id"}
          • Pact value: {metadata: "some-metadata"}

      • In cases involving logical keywords things are even more complex. The will be multiple locations where the constraints on the property are defined, which makes determining what the intention of the swagger author is even more difficult.

    • Assuming we could come up with a rule set that didn’t cause lots of false positives or false negatives: Right now we use a json schema validator (ajv) to determine if a pact request/response body matches a swagger request/response body schema. I’m not aware of any json schema validator that either has the ability to detect “suspected mismatches” or has a feature we could build on top of to easily detect the mismatches. So we would have to implement a json schema validator from the ground up in order to detect these mismatches. This is a significant undertaking.

    In Json Schema everything is allowed by default and each keyword places constraints on values, and I think a warning feature like this would be an attempt to invert some of that behaviour and make undefined properties disallowed by default. I can’t shake we’d be fighting a losing battle by trying to change such a fundamental behaviour of Json Schema.

  9. Tim Jones reporter

    There is no clean way to determine if a property is truely undefined

    I think this is a good argument for it being a warning and not an error - in the case of the API with a generic map, you’d read the warning and say “yes, we know“.

    no clean way to determine if a property truely is undefined or if the swagger is intentionally allowing any property.

    I think the rule I would expect is something like “warn if a property is not explicitly described in the swagger“ - in this case both situations you describe would be warnings, and I think the rule is clear enough that false warnings would be avoided (although, if you mean a false positive as in “warning about something that isn’t a problem“, then yes, that might happen).

    However, the implementation considerations seem like a clear practical reason not to do this (when reporting, I had assumed that there was a custom validator inside this repo).

    Pragmatically, I’m concerned that not having this feature means that the validator is probably not suitable as a substitute for provider verification (at least, not without direction on how to write strict swagger specs). Since it looks like that is something people want to do (see eg #76), I think it would be good to add a warning and some suggestions for how to create strict specs to the documentation. If you agree, I would be happy to contribute a PR adding this.

    ‌

  11. raghanag

    Hi @Ben Sayers some formats in the openapi3 schema are not getting recognized by the tool and getting the below error. Do I have to create a separate issue for this? 

    Error: unknown format "int64" is used in schema at path "#/allOf/1/allOf/0/properties/PrimaryBillingAddressId"
    at Object.generate_format [as code] (/Users/naragha/ocode/fod/node_modules/ajv/lib/dotjs/format.js:69:15)
    at Object.generate_validate [as validate] (/Users/naragha/ocode/fod/node_modules/ajv/lib/dotjs/validate.js:382:35)
    at Object.generate_properties [as code] (/Users/naragha/ocode/fod/node_modules/ajv/lib/dotjs/properties.js:201:26)
    at Object.generate_validate [as validate] (/Users/naragha/ocode/fod/node_modules/ajv/lib/dotjs/validate.js:382:35)
    at Object.generate_allOf [as code] (/Users/naragha/ocode/fod/node_modules/ajv/lib/dotjs/allOf.js:25:27)
    at Object.generate_validate [as validate] (/Users/naragha/ocode/fod/node_modules/ajv/lib/dotjs/validate.js:382:35)
    at Object.generate_allOf [as code] (/Users/naragha/ocode/fod/node_modules/ajv/lib/dotjs/allOf.js:25:27)
    at generate_validate (/Users/naragha/ocode/fod/node_modules/ajv/lib/dotjs/validate.js:382:35)
    at localCompile (/Users/naragha/ocode/fod/node_modules/ajv/lib/compile/index.js:88:22)
    at Ajv.compile (/Users/naragha/ocode/fod/node_modules/ajv/lib/compile/index.js:55:13)

    Below are the properties of the PrimaryBillingAddressId in spec

    "PrimaryBillingAddressId": {
                        "type": "integer",
                        "format": "int64",
                        "readOnly": true,
                        "nullable": true,
                        "x-hints": {
                            "precision": 18
                        }
                    }

    ‌

  12. Craig Schwarzwald

    We have also been looking into this, and while I can see the challenge for things like allOf, anyOf and oneOf, it seems like the tool is currently lacking in a desired way for the likely majority case where none those things are used in a spec due to challenges in the likely more minority case where they are.

    First, @Tim Jones and any others facing this issue, we’ve had some success at creating strict swagger specs by specifying "additionalProperties”: false in our swagger specs within the object definitions. With that property set, this tool will work as you mentioned it’s expected behavior should be. ie: The below swagger and pact will result in a failure:

    {Swagger snippet}
  "definitions": {
    "someObject": {
      "additionalProperties": false,
      "properties": {
        "myOnlyPropertyName": {
          "type": "string"
        }
      }
    }
  }

    {PACT snippet}
  "response": {
    "status": 200,
    "body": {
      "myOnlyPropertyName": "someVal",
      "someOtherNonExistantName": "someOtherVal"
    }
  }

    However, I don’t believe this “solution” is the best ideal way of handling this. As it is fairly easy for provider teams to forget to (or not even know to) put that "additionalProperties”: false flag on every single object in every single swagger spec of every provider throughout every service our company creates.

    I’d love to see if we could agree on an enhancement to the tool that would allow us to run a command line of something like: swagger-mock-validator --additionalPropertiesDefault=false swagger.yaml pact.json which would have the tool automatically insert this flag into the swagger spec of every object (where an additionalProperties value isn’t already specified within the spec for that specific object) before doing the compare, similar to how the tool already strips out the required flag from the specs.

    I believe a solution like that would give us the best of all worlds and still allow for all cases.

    ‌

  13. Henrik Rudstrøm Account Deactivated

    We are currently working on a POC for contract testing using open api schemas for our organization, with a lot of people to convince this issue has been giving us some some headaches.

    Our latest solution to this in this ongoing process is like others here have suggested: To use strict schemas with additionalProperties set to false. To solve the issue of with spec authors not being consistent with this, we are using an openapi linter (https://github.com/stoplightio/spectral, but there are several others). We added a custom linting rule that requires the spec author to set a value for additionalProperties, it can be any value, but it at least would force the spec author to make a choice about it (and preferably set it to false).

    Im still worried that this wont fly in our organisation as this dictates how we have to write our specs. Setting additionalProperties: false makes all the apis closed for extension, which might hamper the provider teams as we are running tests generated and validated against these specs. And also the zalando guidelines (which we want to partially adopt) are very verbose about not using additionalProperties: false https://opensource.zalando.com/restful-api-guidelines/#111

    @Ben Sayers : Im curious, what is your opinion on the zalando guidelines here, and what is atlassians stance on this is, are you consistently using additionalProperties: false when writing specs? Or do you get enough value from these tests even with the false negatives that occur when additionalProperties are not set?

    One other option that comes to mind is to use OpenAPI vendor extensions (https://swagger.io/docs/specification/openapi-extensions/) for example replace all objects without additionalProperties set except those that have a property x-pact-open set. (or the inverted way around x-pact-closed == additonalProperties: false when validating a pacti). At least then there is a way to communicate the intent and resolve any issues caused by altering the schema, but it has the drawback that the spec authors will need to understand how that works, and chances are it is the authors of the consumer that will face the issues of it being used wrong. I dont know, just throwing it out there.

  14. Craig Schwarzwald

    For what it’s worth, my opinion on the zalando guidelines Henrik posted above is they make perfect sense from the standpoint of the provider teams. The Open/Closed Principle is one of the most important concepts in good software design, so allowing provider teams to keep their objects open to extension fits perfectly with that.

    However, this tool, in my opinion, needs to think about things in the context of comparing a contract, and not from the standpoint of what makes sense for a provider spec in isolation. Just like the tool opted to remove the required flag from properties in the api-spec because some consumers may not want to utilize that property, the tool should also (at least allow via some option to) make the objects strict for compares since that’s what you’d want the vast majority of the time when performing pact contract validation.

    I don’t think either viewpoint, defaulting additionalProperties to true or false, is wrong. I think each has it’s purpose based on the context of the situation. OpenAPI should default to have objects extensible by default. But I also think this tool should (at least have the option to) have the objects be strict by default to make for more appropriate contract compares.

  15. Henrik Rudstrøm Account Deactivated

    I think you sum it up nicely @Craig Schwarzwald : The context the schema is used in requires different interpretations of the schema.

    Since writing the last post we have looked more into how we can pre-process our schemas to set additionalProperties to false for mock validation. The major technical challenge mentioned earlier here: Schemas that use allOf can be solved by merging the the sub-definition of the allOf expression into a single definition before setting additonalProperties to false. It seems the heavy lifting for that has already been done: https://www.npmjs.com/package/json-schema-merge-allof

    We are currently writing a pre-processor for our own toolset, but would be happy to contribute that code here instead.

  16. Log in to comment
Assignee
–
Type
bug
Priority
major
Status
new
Votes
0
Watchers
4