grafana / thema

A CUE-based framework for portable, evolvable schema
Apache License 2.0
229 stars 12 forks source link

Validation error messages correctness #186

Closed K-Phoen closed 1 year ago

K-Phoen commented 1 year ago

This PR follows up on #185 with a few more validation test cases, and more importantly some fixes in the error messages Thema generates.

We focused on improving four main areas:

The mungeValidateErr() function is still the beast that it was: we haven't found a clean way to rewrite it yet.

sdboyer commented 1 year ago

missing fields should be reported as such (note: depending on how we see this, it's either a bugfix or a BC break)

say more? there's definitely some issues we need to work on in the bc checker around missing fields, but as i think of it, all bc checking is a lineage-bind-time concern - it should all be decided by the time we get to data validation

K-Phoen commented 1 year ago

I guess my question was more: how should Validate() behave when some fields are missing from the data?

For example:

import "github.com/grafana/thema"

thema.#Lineage
name: "example"
schemas: [{
  version: [0, 0]
  schema: {
    notOptional: string
    optional?: string
  }
}]

Should the following input be considered valid?

{
}

Before this PR, it was. With this change, an error will be produced. Something along the lines of:

<example@v0.0>.notOptional: validation failed, data is not an instance:
    schema specifies that field exists with type `string`
    but field was absent from data
sdboyer commented 1 year ago

I'm going to test and make sure that that dashboard validation still works before approving (looking forward to having more automated tests for this!)

sdboyer commented 1 year ago

looking good on a dashboard - in it goes!