WIP: expression validation and evaluation with CEL #1666

davenewza · 2024-11-05T05:21:15Z

Expression parsing using CEL

📖 See Computed Fields product doc here: https://www.notion.so/keelhq/Computed-fields-fa1f91f188134ece9b2072e9b22e0c1e

Why?

Computed fields will introduce a lot more complexity to Keel expressions. We'll need to support:

Arithmetic operations, e.g. item.price - item.cost
Functions with arguments, e.g. ABS(value) or DURATION(start, end) or YEAR(date)
Aggregate functions which operate on 1:M data, e.g. SUM(items.price)
Expression nesting with proper operator/function precedence, e.g. SUM(account.transactions.value) + account.overdraft > 0 or account.isLocked

Our previous expression AST and parsing implementation would need to be significantly extended to support these new features and could would become quite a substantial amount of code to write and maintain, especially if we want to have really good type checking and validations.

The cel-go package parses, validates and evaluates CEL expressions and also supports everything raised above. CEL is almost identical to Keel expressions, and we've proposed adopting it so not to spend precious energy on extending our codebase to do almost exactly what cel-go would do for us.

Technical overview

The main objective of this PR is to reach feature parity and not to introduce anything new yet to our expressions language.

These points mostly cover what has changed in this PR:

Our AST doesn't describe expressions by their distinct parts anymore.
Each attribute has its own cel-go configuration used for parsing.
Expression validation is now done by cel-go. We do not need to interrogate the AST anymore.
Validation errors from cel-go are intercepted and made more friendly. Fortunately we still support discrete token positions with these validation messages.
Expression evaluation (for example, to generate SQL) is done using visitors.
SQL gen tests for @where, @set and @permission all remain mostly unchanged.

Primary packages

I think it's useful to briefly explain the main packages involved.

expressions: The abstraction over CEL which can validate expressions with Validate(*parser.Expression)

expressions/options: A library of options which can be used to configure the validator. That's important because each attribute would configure their expressions with more or less context and features. For example, a @where expression would be configured with its action input variables and with ctx whereas a @default attribute would not.

expressions/visitor: An expression visitor runner which can be used to interrogate expressions if need be. For example, this can be used to generate SQL or read idents (see next).

expressions/resolve: Various ways to resolve expressions down to values, idents, ident arrays, etc. Sometimes we actually only care about constants and not evaluation, for example, @unique([sku, supplierCode]).

parser/attributes: Parser configuration for each attribute type. Our validation code only needs to really call on these to validate their expressions. E.g. issues, err := attributes.ValidateWhereExpression(asts, action, expression)

What's still to do?

Validation messages need to be improved as they are less useful than what they used to be.
Validation message hints need to be reintroduced.
The functions runtime permission generation needs to be supported. This should be simple enough to do by implementing a new visitor.
DateTime and Date comparison support needs to be added to the cel-go configuration options.
Optional inputs and null fields need stricter validation checking still.
not in is currently not supported.
Migrating to using && and || - breaking change - more to follow.
There is a bug with token positions.
Fix the rest of the schema validation tests

jonbretman

I do worry we're going to make the validation errors a lot less friendly, which will hurt our less technical users more maybe if they mostly work in the schema.

Do you have a plan for how we can make them better?

jonbretman · 2024-12-11T12:48:21Z

expressions/parser_test.go

+	"github.com/teamkeel/keel/schema/validation/errorhandling"
+)
+
+func TestParser_Variable(t *testing.T) {


Feels like these tests would really benefit from using fixtures, a lot of duplication in in this file. Would be nice to use the same/similar approach we use for validation tests with schema files that contain expected errors and just loop over them.

Any reason you didn't do this?

That's a great point. The only reason why I didn't do this is because I've been going about this PR with a bit of a test-driven approach and just happened to not use fixtures, but agreed that this needs to change. Will add it to the list

jonbretman · 2024-12-11T12:51:18Z

expressions/typing/provider.go

+type TypeProvider struct {
+	Schema  []*parser.AST
+	Model   string
+	Objects map[string]map[string]*types.Type


Would be good to add a comment here explaining what the keys of the maps are.

Will do - thanks

jonbretman · 2024-12-11T12:52:33Z

permissions/permissions.go

-	}
-
-	stmt.expression += ")"
+	// stmt.expression += "("


Is this the functions runtime stuff?

Yeah it is. I need to fit this code into a cel visitor which should be relatively straight forward.

jonbretman · 2024-12-11T12:55:34Z

runtime/actions/writeinputs.go

 		if err != nil {
 			return err
 		}
+		// fragments, err := lhsResolver.NormalisedFragments()


Delete this?

jonbretman · 2024-12-11T12:56:48Z

schema/attributes/composite_unique_test.go

+	"github.com/teamkeel/keel/schema/reader"
+)
+
+func TestUnique_Valid(t *testing.T) {


Again feels like these would be better using fixtures.

jonbretman · 2024-12-11T13:02:30Z

schema/parser/expressions.go

+// ToAssignmentExpression splits an assignment expression into two seperate expressions.
+// E.g. the expression `post.age = 1 + 1` will become `post.age` and `1 + 1`
+func (expr *Expression) ToAssignmentExpression() (*Expression, *Expression, error) {
+	parts := strings.Split(expr.String(), "=")


An edge case but wouldn't this break with an expression like model.field = "=== HELLO ==="

@jonbretman Ah yes, what I've done is quite crude. I should rather find the assignment operator by iterating through the tokens. Will do that 👍

jonbretman · 2024-12-11T13:07:24Z

schema/testdata/errors/attribute_expression_unresolvable_rhs.keel

    }

    actions {
        update updatePost(id) with (title) {
            //expect-error:18:28:ActionInputError:title is already being used as an input so cannot also be used in an expression
            @set(post.title = title)
        }
+        update updatePost2(id) with (title) {
+            //expect-error:18:44:AttributeExpressionError:undeclared reference to 'post' (in container '')


You're not wrong about the error messages being less helpful. The error here is that it's using post on the RHS which isn't allowed right? How can we make these errors better? in container '' is pretty cryptic

Ah yes, this is actually fixed - Im still in the progress of updating all these tests. It's actually erroring with unknown identifier 'post', but even that can be improved further

davenewza · 2024-12-11T13:29:12Z

I do worry we're going to make the validation errors a lot less friendly, which will hurt our less technical users more maybe if they mostly work in the schema.

Do you have a plan for how we can make them better?

I agree that this will be problem if not addressed. My plan is to expand on converting cel-go validation errors to customer-friendly ones. This is working pretty well so far here but as mentioned needs to be expanded on. I also need to reintroduce hints for each attribute type.

davenewza added 4 commits November 1, 2024 14:13

chore: cel poc

c9d5357

chore: first pass sql gen

7a27be2

chore: cel proof of concept

908c553

chore: improvs

45cc7d6

davenewza marked this pull request as draft November 5, 2024 05:55

davenewza added the WIP work in progress label Nov 5, 2024

chore: wip

4e12417

davenewza requested review from jonbretman and RutZap November 5, 2024 11:40

davenewza added 17 commits November 6, 2024 14:29

fix: orderby exp

8fbb1e3

chore: prototyping wip

5f93272

chore: enums, basic relationships and arrays in cel

b2669c2

chore: cleaned up things

8f6a933

chore: improved tests, added decimal type

16c34de

chore: fixed @set runtime code

4fdadfe

chore: @where expression validation wip

edca4b3

chore: sql generation wip

99cfff1

chore: sql gen wip

0439405

chore: where sql generation green tests

33898c8

chore: early evaluation wip

347d645

chore: minor cleanup

fe5f4b5

chore: deprecated early eval

d363114

chore: cel visitors for where and set

773314d

chore: we need early auth, fixed set

30589e9

chore: default attribute parsing and validations

98b7e2d

chore: permission expressions validations

27d60c0

davenewza changed the title ~~poc/cel~~ WIP: expression validation and evaluation with CEL Nov 27, 2024

davenewza added 3 commits November 27, 2024 15:22

chore: cleaning up

4608337

chore: refactored validators, introduced resolvers

01de6bf

chore: validation errors with node position

df52b07

davenewza added 9 commits December 3, 2024 11:47

chore: validation messages wip

4d39fe6

chore: runtime fixes

3287ce6

chore: unique lookup validations, other fixes

5f3dc46

chore: tovalue expression resolver, og parser tests fixed

2077de6

chore: working through validations improvements

f28cf62

chore: unique attribute wip

73fd9d9

chore: refactoring

38afd1f

chore: cleaning up

dd243aa

chore: fixed error handling, cleaning up

822d9fd

jonbretman reviewed Dec 11, 2024

View reviewed changes

davenewza added 2 commits December 12, 2024 08:45

chore: refactored cel overloads config, better typing for cel

2d7c557

chore: sql migrations

9cfe469

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: expression validation and evaluation with CEL #1666

WIP: expression validation and evaluation with CEL #1666

davenewza commented Nov 5, 2024 •

edited

Loading

jonbretman left a comment

jonbretman Dec 11, 2024

davenewza Dec 11, 2024

jonbretman Dec 11, 2024

davenewza Dec 11, 2024

jonbretman Dec 11, 2024

davenewza Dec 11, 2024

jonbretman Dec 11, 2024

davenewza Dec 11, 2024

jonbretman Dec 11, 2024

jonbretman Dec 11, 2024

davenewza Dec 11, 2024

jonbretman Dec 11, 2024

davenewza Dec 11, 2024

davenewza commented Dec 11, 2024

WIP: expression validation and evaluation with CEL #1666

Are you sure you want to change the base?

WIP: expression validation and evaluation with CEL #1666

Conversation

davenewza commented Nov 5, 2024 • edited Loading

Expression parsing using CEL

Why?

Technical overview

Primary packages

What's still to do?

jonbretman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davenewza commented Dec 11, 2024

davenewza commented Nov 5, 2024 •

edited

Loading