cel-go icon indicating copy to clipboard operation
cel-go copied to clipboard

Check the timestamp format at compile time

Open lopezator opened this issue 5 years ago • 4 comments

Correct me if I'm wrong, but the only way to know if the timestamp format is right, is to evaluate the expression?

Wouldn't be nice to know that before, at parse/check time?

e.g.

func TestTimestamp(t *testing.T) {
	// create env
	env, err := cel.NewEnv(
		cel.Declarations(
			decls.NewVar("date", decls.Timestamp)))
	if err != nil {
		t.Error(err)
	}

	// compile (parse+check)
	_, issues := env.Compile(`date > timestamp('wrong timestamp')`)
	if issues != nil && issues.Err() != nil {
		t.Fatal("I want to err here!")
	}
}

We are using parse+check to generate an AST, using that AST as an input to a filter component, who generates SQL from it.

This would allow us to support timestamp without having to evalute the expression just for that, or introducing custom code to post-check the AST, if the format passed to timestamp("wrong") is wrong.

I suppose this could apply for duration as well.

lopezator avatar May 22 '20 11:05 lopezator

Hi @lopezator thanks for filing the request. There's sort of a general category of lint-style checks that we've been thinking about adding for CEL. Checking string literal inputs to regexes, timestamps, durations, etc. definitely fall into this category.

These sort of checks can be a bit brittle when new functionality is introduced which is often why they're considered warnings rather than hard errors; however, I can see the cases you mentioned being errors.

I'm happy to consider this a future enhancement, but I don't have a delivery timeline in mind just yet.

TristonianJones avatar May 22 '20 23:05 TristonianJones

Thank you for your response @TristonianJones !!

I think we could apply that logic in our end the meantime, I just wanted to be sure I wasn't missing something obvious.

Just to be sure, what do you exactly mean by warnings? Do CEL return any warning in use cases like this? How and where?

Thank you for your work in this exceptional library, it's being very useful for us.

Best.

lopezator avatar May 23 '20 13:05 lopezator

@lopezator This is a good feature request, so thank you for filing it. You're not missing anything obvious at all.

The cel.Issues object has an Err() method exposed which can be tested to check for problems in the syntactic or semantic correctness. The object also contains a list of errors, but in theory could contain a list of non-error severity issues such as warnings and deprecation notices. It's very likely the literal checks you mentioned would still have error severity, but I can see other cases where a less severe issue should be returned and then the application, such as yours, could decide whether the warning should be a hard-stop fail or an intended use of the library.

I'm so glad you've enjoyed using CEL. Your team was one of the first to pick it up and play with it. :)

TristonianJones avatar May 23 '20 23:05 TristonianJones

The object also contains a list of errors, but in theory could contain a list of non-error severity issues such as warnings and deprecation notices.

Wow, this is a great idea, and would be more than enough for our use case I think.

I'm so glad you've enjoyed using CEL. Your team was one of the first to pick it up and play with it. :)

We do! Everytime we add new functionality on our side and therefore take the opportunity to upgrade the library to latest is like... Wow! So much evolution in a short time.

lopezator avatar May 25 '20 07:05 lopezator