Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix invalid unassignability check for lists of structs #194

Merged
merged 1 commit into from Sep 20, 2023

Conversation

radiohead
Copy link
Contributor

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

Signed-off-by: Igor Suleymanov <igor.suleymanov@grafana.com>
Copy link
Contributor

@joanlopez joanlopez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@joanlopez
Copy link
Contributor

...
...

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
Copy link
Contributor Author

...
...
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.

@radiohead radiohead merged commit fb9338e into main Sep 20, 2023
2 checks passed
@radiohead radiohead deleted the fix/struct-list branch September 20, 2023 11:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants