mattpolzin / OpenAPIKit

Codable Swift OpenAPI implementation.
MIT License
280 stars 35 forks source link

An error when parsing `array` with `maxLength` #239

Closed kean closed 2 years ago

kean commented 2 years ago

I've encountered multiple specs with incorrect combinations of properties that define a schema. The most recent example is Jira's spec that has maxLength duplicated in the incorrect place:

schema:
maxLength: 128
type: array
items:
    maxLength: 128
    type: string

I think they were trying to specify the length of the string, not the maximum number of items in the array, which maxItems is for. Yet, this minor mistake prevents the entire spec from being decoded.

I'm again, unsure how this should be handled. But from my perspective, I think it should be a warning at best, not a critical error that terminates parsing. I think it would be ideal in a team setting as well. The people writing specs and the consumers are typically different teams. I think a minor issue in a spec shouldn't block the consumer.

P.S. OpenAPI specs are a lot like HTML. I've yet to find a perfect spec with no issues.

kean commented 2 years ago

Also, the error message is a bit confusing:

.dataCorrupted(Swift.DecodingError.Context(codingPath: [], debugDescription: "The given data was not valid YAML.", underlyingError: Optional(A schema contains properties for multiple types of schemas, namely: ["array", "object"].)))"

I understand this is a limitation of Codable, but it's a bit confusing to see "The given data was not valid YAML.". The YAML if fine, but there is inconsistency with an OpenAPI spec.

mattpolzin commented 2 years ago

OpenAPI specs are a lot like HTML. I've yet to find a perfect spec with no issues.

My company maintains one; we run it through OpenAPIKit with validation as part of our CI flow 😛

the error message is a bit confusing

Try running it through OpenAPI.Error if you have not already. I don't know off the top of my head if this will produce better results in this specific case, but it's part of the motivation for that type.

do {
  try decoder.decode(OpenAPI.Document.self, from: ...)
} catch let error {
  print(OpenAPI.Error(from: error).localizedDescription)  
}
mattpolzin commented 2 years ago

I think I've got a rough idea forming in my head for how to handle more of these errors as warnings when they are not critical. I agree that there's a general theme of strict-without-benefit in the current parsing OpenAPIKit does.