jaboc83 / ynab-sdk-dotnetcore

YNAB API .Net Core Library https://api.youneedabudget.com
https://www.nuget.org/packages/YNAB.SDK/
Apache License 2.0
25 stars 9 forks source link

Nullable types not supported by spec and that makes .NET generator unhappy #5

Open jaboc83 opened 4 years ago

jaboc83 commented 4 years ago

The swagger spec doesn't have support for nullable types using "type": ["uuid", "null"] as I'd hoped. After talking with YNAB team sounds like until an OpenAPI 3.0 implementation is ready there isn't a good way to determine this that doesn't break at least some generators. For now the recommendation is to use the "required" field as a guide and either override the mustache templates or the spec itself prior to generation. see this topic for more detail: https://support.youneedabudget.com/t/y75vv1/swagger-error

Also, details from my conversation with Brady (08/23/2019): "OpenAPI 2.0 (Swagger) has some real issues with specifying and handling nullable types. It's compounded by the fact that language generators handle it differently. We've had other developers tell us this and we have make some tweaks along the way to help but there are still some issues. First off, JSON Schema does allow for specifying 2 different types for a single filed (e.g. "type": ["string", "null"]) but Swagger, not fully respecting JSON Schema does not seem to like this or expect it. If you paste your changed schema file in the Swagger editor/validator you'll see errors like: "Structural error at definitions.Category.properties.original_category_group_id.type should be equal to one of the allowed values allowedValues: array, boolean, integer, number, object, string". We actually used to use that format and it broke other generators. The closest we've come is relying on the "required" array in the spec to signal which fields should not be null. If a field is not in the "required" array it means it could be null. Most generators seem to handle this properly. We actually have a pending change to the spec we'll release next week that removes a few more fields from the required properties to help with this. We actually had a problem with this in the TypeScript client because it was generating fields not in "required" as optional (i.e memo?: string), not nullable. This meant they might not be defined in objects, not that they could be null. But, we were able to override the template for the class generator and add make it use a union type like "string | null" for non-required, nullable fields. Anyway, you might be able to achieve the same thing by overriding the template in the .NET Core generator. It's unfortunate we've had these problems and I don't think there's a perfect fix."

jaboc83 commented 4 years ago

I'm not sure we have enough information in the template to make the generated CS happy for properties in a way that will be selective enough. Might just stick with a manually managed spec / API for the time being until YNAB OpenAPI 3 version is available. If I get more time in the future I'll come back and consider a more robust automated approach to the code generation. Ideally the YNAB spec won't change an awful lot so this generation should be pretty easy to keep up with. Time will tell. If anyone else wants to take a crack at it be my guest.