civisanalytics / swagger-diff

Utility for comparing two Swagger specifications.
BSD 3-Clause "New" or "Revised" License
264 stars 32 forks source link

key = ref[idx + 1..-1], will raise exception if idx is nil #60

Closed lizlex closed 6 years ago

lizlex commented 6 years ago

https://github.com/civisanalytics/swagger-diff/blob/b38541d7f38479bc60e169e1d6e05c2c97862a1c/lib/swagger/diff/specification.rb#L107

Test Data: $ref is written error

parameters:
- name: ids
    in: body
    description: id list 
    required: true
    schema:
     type: array
     items:
     $ref: string
lizlex commented 6 years ago

add if ... else can fix this

if nil==idx
  key = ""
else
  key = ref[idx + 1..-1]
end
jeffreyc commented 6 years ago

Thank you for your bug report! Are you able to provide a more specific example? While Swagger does support file references, Swagger-Diff does not. If that's what you're encountering, Swagger-Diff would need to be updated to parse relative schema files (vs. simply handling embedded schemas). Unfortunately, simply handling a nil value when generating the key would cause Swagger-Diff to miss any differences in the referenced schema.

If you're not using a relative schema file, then you may have an invalid specification, as a "$ref" should contain a URI, and most valid URI patterns include a "/". https://swagger.io/docs/specification/using-ref/ is for OpenAPI 3, but "$ref Syntax" on that page does a nice job of summarizing valid "$ref" values, and appears to be consistent with OpenAPI 2.

lizlex commented 6 years ago

Hi jeffreyc, thanks for you quickly response! I know the swagger file is invalid in syntax, but I use notepad to edit the file, and can't find this error intuitively.

When using swagger-diff to compare the 2 files, I noticed that, the invalid file can be correctly parsed(parse_swagger), and validated(validate_swagger). For program robustness,I suggest swagger-diff add some validation statement before using the nil object or in validate_swagger() raise an exception to point out the error

old_pet.txt new_pet.txt

jeffreyc commented 6 years ago

Unfortunately, OAI validation is imperfect. Just because Swagger-Diff can parse and validate a specification doesn't mean it's valid. In this case, $ref: string (L49) is valid OAI, but only if there's a file named "string" in the same directory as the specification that contains the referenced schema (though if there were, Swagger-Diff doesn't currently support parsing specifications split across multiple files). But the error when that happens could be friendlier. I'm going to close this issue, but I'll think on how to better handle invalid specifications.