[sc-35836] Add initial oneOf schema code gen support #26
Conversation
This uses shrubbery untagged unions to implement oneOf code gen.
We could use tagged unions here but I'm not sure if doing so adds any
meaningful information.
Remaining notes and questions:
- I don't fully understand `SchemaMap`s, so I think they may be handled incorrectly,
what is there indended use, and should we try to use them here
instead of inferType? If yes, is there a test case that would reveal
an issue with not using them
- Related to the above, should we extract/share code between
`mkInlineOneOfSchema` and `mkInlineBodySchema`?
- This does not implement openapi discriminators, which is where I
imagine using tagged unions might become useful
- This does not implement inline objects as choices for `oneOf`
- Are there any other test cases we want for the initial
implementation?
- If we run into `anyOf` being used, it would probably be simple to
have it just call out to the `oneOf` code (though hopefully no one
ever uses `anyOf`...)
Co-Authored-By: Tyler <[email protected]>
Co-Authored-By: Janus <[email protected]>
There was a problem hiding this comment.
SchemaMaps should be used to resolve the $ref references to other schemas within the schema. The test case to expose this isn't one that the test suite currently
makes easy to write, because the it's a matter of when it fails. We would like to fail at
the code gen step if a $ref refers to a non-existent schema. Inferring the type instead will let the code generate, but then it will fail to compile because the type
it's trying to reference does not exist.
Yes, it would be nice to avoid duplicating similar code code in mkInlineOneOfSchema and mkInlineBodySchema?
Yes, shrubbery tagged unions would be used with OpenAPI discriminator properties. Notably, the cannot always be used for oneOf without discriminator properties anyhow, so using a regular Unino type for oneOf is correct.
Not implementing inline objects as choices for oneOf for now seems fine. We should just make sure that the error message the code gen produces makes it reasonably clear that this is the case so that when we hit the case in an actual OpenAPI spec we can come and implement it then.
The test case you have for the initial implementation looks sufficient to me.
I also hope we never run into an anyOf usage in the real world. If we do, I agree just treating it as oneOf is probably the best we can. Possibly we should consider a warning, or making that behavior opt-in just to avoid surprising users who expected something else.
|
|
||
| let | ||
| header = | ||
| schemaTypeModuleHeader moduleName typeName extraExports | ||
|
|
||
| pragmas = | ||
| HC.lines $ | ||
| ["{-# LANGUAGE DataKinds #-}" | formatRequiresDataKinds format] |
There was a problem hiding this comment.
Let's please not use list comprehension syntax for this conditional
| case mbPrefixFn of | ||
| Just _ -> taggedUnion | ||
| Nothing -> union | ||
| <> " '[" |
There was a problem hiding this comment.
Technically this ' is only required when there is a single member and is now discouraged otherwise. I don't think it's worth a conditional here right now to deal with that, but it's a rather unfortunate aspect of DataKinds
There was a problem hiding this comment.
Yeah, I was aware of this, and it's a little awkward to imagine a spec with oneOf with only option, but it's worth supporting. I didn't realize the tick was discouraged when not needed (but I suppose that makes more sense in non-generated code where you always know your list is a certain length)
| Just OA.OpenApiArray -> | ||
| case OA._schemaItems schema of | ||
| Just (OA.OpenApiItemsObject (OA.Ref ref)) -> do | ||
| (_modName, refTypeName) <- CGU.inferTypeForInputName CGU.Type $ OA.getReference ref |
There was a problem hiding this comment.
I'm guessing this should use the schemaMap to resolve the ref
| schema | ||
| pure (schemaTypeInfoDependencies typeInfoWithDeps, schemaTypeInfoDependent typeInfoWithDeps) | ||
| OA.Ref ref -> do | ||
| (_modName, refTypeName) <- CGU.inferTypeForInputName CGU.Type $ OA.getReference ref |
There was a problem hiding this comment.
This ref should probably be resolved via a SchemaMap as well
| unionName = | ||
| fleeceCoreVar "unionNamed" | ||
| <> " (" | ||
| <> fleeceCoreVar "unqualifiedName" |
There was a problem hiding this comment.
Since we know the module that we're generating the type in we should really use qualifiedName here and give that module name
This uses shrubbery untagged unions to implement oneOf code gen. We could use tagged unions here but I'm not sure if doing so adds any meaningful information.
Remaining notes and questions:
SchemaMaps, so I think they may be handled incorrectly, what is there indended use, and should we try to use them here instead of inferType? If yes, is there a test case that would reveal an issue with not using themmkInlineOneOfSchemaandmkInlineBodySchema?oneOfanyOfbeing used, it would probably be simple to have it just call out to theoneOfcode (though hopefully no one ever usesanyOf...)