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

Check the timestamp format at compile time #359

Closed
lopezator opened this issue May 22, 2020 · 4 comments · Fixed by #765
Closed

Check the timestamp format at compile time #359

lopezator opened this issue May 22, 2020 · 4 comments · Fixed by #765
Assignees
Labels
enhancement New feature or request

Comments

@lopezator
Copy link
Contributor

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.

@TristonianJones
Copy link
Collaborator

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.

@lopezator
Copy link
Contributor Author

lopezator commented May 23, 2020

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.

@TristonianJones
Copy link
Collaborator

@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. :)

@lopezator
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants