Wrong classifications for response.body.scope.add and response.body.scope.remove result-type-finder

Issue #20 new
Former user created an issue

Wrong classifications in lib/openapi-diff/spec-differ/result-type-finder.ts

Current values 'response.body.scope.add': 'breaking', 'response.body.scope.remove': 'non-breaking',

Expected values 'response.body.scope.add': 'non-breaking', 'response.body.scope.remove': 'breaking',

Comments (6)

  1. Ailton Pinto

    When you add a new field it is not a breaking change, because your consumer could use the new property or not.

    When you remove, if any consumer is using the property it will break the functionality.

  2. Ben Sayers

    @Ailton Pinto I don’t believe this is a bug. Let’s discuss this in terms of an example. Let’s say a response body changes as follows:

    old

    {
      "type": "string"
    }
    

    new

    {
      "type": ["string", "number"]
    }
    

    This change represents an expansion of the schema, with the new schema allowing numbers. Think about this from the perspective of a client receiving this response. It would have code that is expecting a string, and would likely break if a number value was returned.

    Now let’s say we flip the change around the other way:

    old

    {
      "type": ["string", "number"]
    }
    

    new

    {
      "type": "string"
    }
    

    This change represents a contraction of the schema, with the new schema no longer allowing numbers. If a client supported the old schema that would mean it knows how to parse both numbers and strings, so changing the schema to only return strings would not break the client.

    Let me know if you have any further questions. If you do, discussing this in terms of concrete examples often helps.

  3. Ailton Pinto

    @Ben Sayers The scenario you described seems for me to be a property change, let’s analyze another scenario:

    Old

    {
      type: string
    }
    

    New

    {
      type: string
      age: number
    }
    

    It’s not a breaking change because the client could use the age property or not, but currently, this scenario is classified wrongly as a breaking change

  4. Ben Sayers

    The example schema you pasted is not a valid json schema, but I’ll assume you mean this:

    old

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

    new

    {
      "properties": {
        "type": {
          "type": "string"
        },
        "age": {
          "type": "number"
        }
      },
      "type": "object"
    }
    

    In the old schema the following value was considered valid:

    {
      "type": "some-type",
      "age": "thirty"
    }
    

    This is because by default JSON Schema allows all values in all properties, and by adding keywords you are placing constraints on the allowed values. So since the original schema did not place any constraints on the property age, it was allowed to be any value.

    However, the new schema does not accept this value because it applies a constraint to this property, only allowing it to be of type number. This means that this property can no longer be of type string. It is also not allowed to be arraybooleannull or object.

    Because this change represents a contraction of the schema it will be classified as response.body.scope.remove and will be considered a non breaking change. Have you found the tool behaving in a different way than I just described?

  5. Christophe Bornet

    I confirm the issue described by @Ailton Pinto . The change is described as

    {
        "breakingDifferences": [
            {
                "type": "breaking",
                "action": "add",
                "code": "response.body.scope.add",
    

    but it should be described as non-breaking

  6. Christophe Bornet

    My bad it’s working as expected.

    What’s not working is that it’s not possible to add an optional field to a request.

  7. Log in to comment