grafana / kindsys

a kind system for schematizing objects
Apache License 2.0
12 stars 0 forks source link

Move `_specIsNonEmpty` check out of `joinSchema` to eliminate duplicative errors #27

Open sdboyer opened 1 year ago

sdboyer commented 1 year ago

We currently use this _specIsNonEmpty field in the main joinSchema that's injected into all core/custom kinds as a way of ensuring that all kinds have at least one field in their spec. However, this is causing error spam, as errors for schema violations within spec are being duplicated in that hidden field. Example:

<dashboard@v0.0>.spec.gnetId: validation failed, data is not an instance:
    schema expected `string`
        /github.com/grafana/kindsys/kinds/dashboard/dashboard_kind.cue:36:13
        /github.com/grafana/kindsys/cue.mod/pkg/github.com/grafana/thema/lineage.cue:234:20
    but data contained `null`
        dashboard.json:1:218

<dashboard@v0.0>._specIsNonEmpty.gnetId: validation failed, data is not an instance:
    schema expected `string`
        /github.com/grafana/kindsys/kinds/dashboard/dashboard_kind.cue:36:13
        /github.com/grafana/kindsys/kindcat_custom.cue:69:19
        /github.com/grafana/kindsys/kindcats.cue:101:46
        /github.com/grafana/kindsys/cue.mod/pkg/github.com/grafana/thema/lineage.cue:46:21
        /github.com/grafana/kindsys/cue.mod/pkg/github.com/grafana/thema/lineage.cue:234:12
    but data contained `null`
        dashboard.json:1:218
sdboyer commented 1 year ago

The solution here should be straightforward - for Core and Custom, we can move that constraint from the actual struct that's injected into the lineage via thema's joinSchema, over to some other place in the kind declaration itself. So, whereas right now the constraint ends up living at CUE path #Custom.lineage.joinSchema._specIsNonEmpty, we could express the same at #Custom._constraints.NonEmptySpec. Something like this:

#Custom: {
  // ... other fields elided
  lineage: thema.#Lineage
  _constraints: {
    NonEmptySpec: [for sch in lineage.schemas {
      sch.schema & { spec: struct.MinFields(1) }
    }]
  }
}

This should result in the same constraint being enforced, but only at bind-time. That may mean a CUE evaluator performance boost at runtime, because operations like Validate() will no longer have to consider the additional field - though my guess is that the gains there will be minimal, because avoiding duplicative work in a case like this is probably a trivial optimization that's already in the evaluator.