redskap / swagger-brake

Swagger contract checker for breaking API changes
Apache License 2.0
58 stars 16 forks source link

Stack overflow on recursive schemas #8

Closed dalewking closed 5 years ago

dalewking commented 5 years ago

We had our own library that tried to do what swagger-brake is doing and ran into the same problem.

The solution is that you have to remember where you have been when recursing and don't recurse into a schema you have already seen on the way down.

Here is a simplified swagger spec that causes the stack overflow. Note that PaymentItemDTO contains an array of children PaymentItemDTO objects.

{
  "swagger":"2.0",
  "host":"localhost",
  "basePath":"/",
  "tags":[
    {
      "name":"audit-controller",
      "description":"Audit Controller"
    },
    {
      "name":"audit-fee-controller",
      "description":"Audit Fee Controller"
    },
    {
      "name":"operation-handler",
      "description":"Operation Handler"
    }
  ],
  "paths":{
    "/api/v1/audits/summary/{businessId}":{
      "get":{
        "tags":[
          "audit-controller"
        ],
        "summary":"fetchAuditSummaryByBusinessId",
        "operationId":"fetchAuditSummaryByBusinessIdUsingGET",
        "produces":[
          "*/*"
        ],
        "parameters":[
          {
            "name":"businessId",
            "in":"path",
            "description":"businessId",
            "required":true,
            "type":"string",
            "format":"uuid"
          }
        ],
        "responses":{
          "200":{
            "description":"OK",
            "schema":{
              "$ref":"#/definitions/AuditSummaryDTO"
            }
          },
          "401":{
            "description":"Unauthorized"
          },
          "403":{
            "description":"Forbidden"
          },
          "404":{
            "description":"Not Found"
          }
        },
        "security":[
          {
            "jwt":[
              "global"
            ]
          }
        ],
        "deprecated":false
      }
    }
  },
  "securityDefinitions":{
    "jwt":{
      "type":"apiKey",
      "name":"Authorization",
      "in":"header"
    }
  },
  "definitions":{
    "AuditSummaryDTO":{
      "type":"object",
      "properties":{
        "payoffSavings":{
          "type":"number"
        },
        "unverifiedPayoff":{
          "type":"number"
        },
        "unverifiedPayoffBreakdown":{
          "type":"array",
          "items":{
            "$ref":"#/definitions/PaymentItemDTO"
          }
        },
        "unverifiedVehicles":{
          "type":"integer",
          "format":"int64"
        }
      },
      "title":"AuditSummaryDTO"
    },
    "PaymentItemDTO":{
      "type":"object",
      "properties":{
        "amountApplied":{
          "type":"number"
        },
        "children":{
          "type":"array",
          "items":{
            "$ref":"#/definitions/PaymentItemDTO"
          }
        },
        "description":{
          "type":"string"
        },
        "recordType":{
          "type":"string",
          "enum":[
            "PRINCIPAL",
            "INTEREST",
            "CPP",
            "TRANSPORTATION_FEE",
            "AUDIT_FEE",
            "OTHER_FEE"
          ]
        }
      },
      "title":"PaymentItemDTO"
    }
  }
}
galovics commented 5 years ago

Totally valid point. I knew this will happen, I just didn't consider the recursive structure as a common use-case. I'll take a look. Thanks!

dalewking commented 5 years ago

Here is an outline of how I fixed it in our library (which was written by an intern so we are looking to replace with swagger-brake):

I created a set to remember the path:

private Set<Object> seen = new HashSet<>();

The method to determine if 2 types are the same looks like this:

    boolean hasChangedElement = false;

    // Evaluate the map the first time we see it, otherwise ignore since it is a recursive instance
    if(seen.add(old)) {
        hasChangedElement = // determine if it is changed which includes recursion

        seen.remove(old);
    }

    return hasChangedElement;
galovics commented 5 years ago

Fixed.