thema: Fix backwards compatibility checking bugs #193
Conversation
The entire fix here was: - Checking a closed def (`_#schema`) instead of the original, typically open `schema` field - Getting rid of cue.Schema() and cue.Final() as options to Subsume(), because they weren't doing what we thought they did Then all the backwards compatibility checks just started working, exactly like we'd originally hoped.
sdboyer
added
enhancement
| field aunion not present in {aunion:*"foo" | "bar" | "baz"}: | ||
| /cue.mod/pkg/github.com/grafana/thema/lineage.cue:234:12 | ||
| missing field "aunion" |
There was a problem hiding this comment.
This error absolutely sucks - the aunion field itself isn't missing, just some parts of its value!
But, it's still unambiguously better to be in a world where we have a true negative with a confusing error than a false positive with no error.
|
@AgnesToulet i'm really curious if the basic approach here could resolve the problems you encountered with |
| }] | ||
| -- out/bindfail -- | ||
| schema 0.1 is not backwards compatible with schema 0.0: | ||
| field concreteCross not present in {concreteCross:"foo" | "bar" | 42,concreteString:"foo" | "bar" | "baz",crossKind3:string | >=-2147483648 & <=2147483647 & int | bytes,crossKind2:string | >=-2147483648 & <=2147483647 & int}: |
There was a problem hiding this comment.
clearly we'll be reusing some of what we wrote to improve Validate() errors here, too
| @@ -29,4 +29,6 @@ lin: lenses: [{ | |||
| } | |||
| }] | |||
| -- out/bindfail -- | |||
| schema 0.1 must be backwards compatible with schema 0.0 | |||
| schema 0.1 is not backwards compatible with schema 0.0: | |||
| field not allowed in closed struct: getsRemoved | |||
There was a problem hiding this comment.
Here I guess we'll also need to pay attention when working on messages clarity, as closedness is probably something not explicitly managed by the schema author, thus even less familiar for a non-author user.
Could you list which edge cases do you have in mind? I'd be happy to help preparing such additional edge cases. |
I'm not sure i've ever been so delighted to put up a PR as this one!
We had three cases where the way we were invoking and constructing arguments to
cue.Value.Subsume()was not behaving as expected - removing required fields, removing optional fields, and changing a default value. Clearly, these are all significant cases that we should expect folks will routinely encounter.All three of these are now fixed. The only remaining exception cases involve additional lineage invariants that Thema is adding - nothing related to backwards compatibility.
The entire fix here was tweaking two lines:
_#schema) instead of the original, typically openschemafieldThen all the backwards compatibility checks just started working, exactly like we'd originally hoped! Even for defaults, which i'd written off months ago as something that would require some core CUE changes.
In retrospect, this makes a lot of sense - subsumption over open structs is always going to be weird to check, because openness is effectively like a
..._at the end of each struct, and_subsumes everything.We were trying to compensate for this by passing
cue.Final()toSubsume(). I don't know why that never worked as expected, but openness/closedness is really weird and subtle, so it's not hard to imagine there being something quirky there, even without bugs. But the crispness of the_#schemafield seems to have cut through that noise. I introduced it solely to give an internal-only handle that's always closed for Thema's Go runtime to work with as part ofValidate(), but now it's paying even more dividends.There's more edge case probing we should do here with additional test cases, but that's good for a follow-up.
All this said, the actual errors being emitted by the subsumption checks are quite confusing, more often than not. They're going to need even more love than validation errors. In the short term, it's more important that the checks are correct than that they're understandable - but we can't rest on that for long.