grafana / thema

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

Fix invalid unassignability check for lists of structs #194

Closed radiohead closed 1 year ago

radiohead commented 1 year ago

What

Fix list checking logic to include default values in list expression length check (i.e. check that there are two values when a default one is present), since the underlying issue in CUE has been resolved - calling .Expr no longer drops the default value.

Why

The reason is above, the underlying issue in CUE seems to have been resolved. This fixes a weird issue when using Thema with https://github.com/grafana/grafana-app-sdk and the following schema:

package kinds

test: {
    name:  "Test"
    group: "some"

    crd: {
        scope: "Namespaced"
    }

    codegen: {
        backend:  true
        frontend: false
    }

    lineage: {
        schemas: [{
            version: [0, 0]
            schema: {
                #Struct: {
                    foo: string
                }

                #AnotherStruct: {
                    foo: #Struct
                }

                spec: {
                    foo: string
                }

                status: {
                    foo: [...#Struct]
                    bar: [...#AnotherStruct]
                }
            }
        }]
    }
}

Which results in a false-positive unassignability error:

panic: status.foo: schema is a complex disjunction of list types, may only correspond to interface{}/any
status.bar: schema is a complex disjunction of list types, may only correspond to interface{}/any
joanlopez commented 1 year ago

... ...

Which results in a false-positive unassignability error:

panic: status.foo: schema is a complex disjunction of list types, may only correspond to interface{}/any
status.bar: schema is a complex disjunction of list types, may only correspond to interface{}/any

I don't see any default value on the example above, what I'm missing? 🤔 Thanks!

radiohead commented 1 year ago

... ... Which results in a false-positive unassignability error:

panic: status.foo: schema is a complex disjunction of list types, may only correspond to interface{}/any
status.bar: schema is a complex disjunction of list types, may only correspond to interface{}/any

I don't see any default value on the example above, what I'm missing? 🤔 Thanks!

CUE assigns an implicit default value of an empty array to the expression, at least that's what it looks like in the code.

Could we add / extend the existing test suite to cover this, please?

I've tried adding an case to the tests in assignable_test.go, but it doesn't seem to support a use case with named structs. I'd appreciate any guidance / help.