mattpolzin / OpenAPIKit

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

JSONSchema warnings #249

Closed mattpolzin closed 2 years ago

mattpolzin commented 2 years ago

Fixes https://github.com/mattpolzin/OpenAPIKit/issues/239

⚠️ Breaking Changes ⚠️ JSONSchema changes from an enum to a struct (as usual, to facilitate storage of warnings). For most code, use will not change, but if you switch over any JSONSchemas, you should change to switching over the newly exposed value property of JSONSchema.

kean commented 2 years ago

Hey, awesome work. I migrated to the latest version and I'm now using OpenAPIKit directly rather than a fork.

I found one potential caveat during migration. This code still compiles, but it's in correct because you should be using schema.value.

private func getPathParameterType(for parameter: OpenAPI.Parameter) throws -> TypeName {
    let schema: JSONSchema = <get schema from parameter>
    switch schema {
    case .integer: return TypeName("Int")
    default: return TypeName("String")
    }
}

I'd suggest considering an approach where parsing is separate from entities. I think it would be nice if the decoded entities didn't have additional baggage like these warnings. If I have time, I'll work on a prototype, see if you like it.

mattpolzin commented 2 years ago

RE your observation about the switch that still compiled but no longer works as intended: I'm really bummed about the fact that it's possible to write code that looks reasonable but in practice has the totally wrong effect (I also noted this while fixing code inside OpenAPIKit to work with the changes in this PR).

This might be worth a change. I'm not totally sure yet.

As for warnings being baggage, I definitely agree in the sense that they are the only reason I am lifting an enum into a struct. I recall some conversation in the past over at Swift Evolution on supporting stored values on enums and at the time I never felt very strongly about the idea but now I've really felt the pain!

There's one thing I actually quite like about the warnings being right there on the model, though. This way, the fact that the OpenAPI model is the way it is can always be traced back to a potential list of warnings encountered. In a sense, if OpenAPIKit warns that it encountered ambiguity and had to pick one of two interpretations, the warning holds onto the other representation without storing an entire other copy of the decoded result.

I still agree that another reasonable implementation would separate parsing out.

kean commented 2 years ago

Yeah, enum vs struct is a constant struggle. Struct is probably the best way moving forward, considering how much context is shared between the types, e.g. JSONSchemaContext. Using structs also opens an opportunity to move JSONSchemaContext content to the struct itself, removing computed properties: required, formatString, etc. Enums aren't my primary choice for scenarios like JSONSchema.

Whether to include the warnings is a separate issue. I have some time to give it a try just to see how it works. No pressure to merge it or anything. I'm just curious. I think it could a really nice and clear solution. There is a way to trace warnings back to the struct as well. Ideally, you should be able to trace them back to the source file.