dbt-labs / hologram

A library for automatically generating Draft 7 JSON Schemas from Python dataclasses
MIT License
9 stars 13 forks source link

Fix restricted variants to try multiple candidates #31

Closed beckjake closed 4 years ago

beckjake commented 4 years ago

Currently, if you define multiple variants that meet the same explicitly restricted fields, dbt will just try the first one and fail if it can't parse. This can have counter-intuitive consequences:

@dataclass
class ParsedModel(JsonSchemaMixin):
  resource_type: NodeType.Model = field(metadata=dict(restrict=[NodeType.Model]))

@dataclass
class CompiledModel(JsonSchemaMixin):
  resource_type: NodeType.Model = field(metadata=dict(restrict=[NodeType.Model]))
  compiled: True

@dataclass
class UnionedModel(JsonSchemaMixin):
    value: Union[ParsedModel, CompiledModel]

# good!
assert UnionedModel(ParsedModel(NodeType.Model)).to_dict() == \
    {'value': {'resource_type': 'model'}} 
# good!
assert UnionedModel.from_dict({'value': {'resource_type': 'model'}}) == \
    UnionedModel(ParsedModel(NodeType.Model))
# good!
assert UnionedModel(CompiledModel(NodeType.Model, True)).to_dict() == \
    {'value': {'resource_type': 'model', 'compiled': True}}

# horrible error, don't even need to assert!
# "'compiled' was unexpected" - only tried to parse as a ParsedModel
UnionedModel.from_dict({'value': {'resource_type': 'model', 'compiled': True}}) == \
    UnionedModel(CompiledModel(NodeType.Model, True))

Reversing the order of the union will reverse the issue.

This PR fixes that issue by trying each variant. I refactored the restricted/unrestricted logic a tiny bit since we're now handling them the same way for both restricted and unrestricted fields.

An alternative I considered was making a special restriction that says "this field must exist". That required a bunch of changes to validating and parsing restrictions, while this seemed fairly straightforward.