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

OptCheckStringFormat causes Program to fail generation if format argument has field access #690

Closed
2 of 3 tasks
rodaine opened this issue Apr 28, 2023 · 3 comments · Fixed by #694
Closed
2 of 3 tasks
Assignees

Comments

@rodaine
Copy link

rodaine commented Apr 28, 2023

Describe the bug

With ext.Strings, setting the OptCheckStringFormat option when generating the Program fails if the argument to the format string refers to a field on a message.

To Reproduce

  • parser
  • checker
  • interpreter

Test setup:

func Test(t *testing.T) {
	// message Msg {
	//   double fld = 1;
	// }
	msg := &Msg{Fld: 1.23}

	expr := "'%s'.format([msg.fld])"

	env, err := cel.NewEnv(
		ext.Strings(),
		cel.Types(msg),
		cel.Variable("msg",
			cel.ObjectType(string(msg.ProtoReflect().Descriptor().FullName()))),
	)
	require.NoError(t, err)

	ast, issues := env.Compile(expr)
	require.NoError(t, issues.Err())

	prog, err := env.Program(ast, cel.EvalOptions(
		cel.OptOptimize,
		cel.OptCheckStringFormat,
	))
	require.NoError(t, err)

	val, _, err := prog.Eval(map[string]any{"msg": msg})
	require.NoError(t, err)
	t.Logf("%#v", val)
}

Actual behavior

=== RUN   Test
    validator_test.go:59: 
        	Error:      	Received unexpected error:
        	            	error during formatting: string clause can only be used on strings, bools, bytes, ints, doubles, maps, lists, types, durations, and timestamps
--- FAIL: Test (0.00s)

Expected behavior

"1.23"

Additional context

Test passes with either modification:

  • Remove cel.OptCheckStringFormat
  • Replace msg type with cel.DynType
@TristonianJones TristonianJones self-assigned this Apr 29, 2023
@TristonianJones
Copy link
Collaborator

@rodaine I'm not actually able to repro the error. I am curious though whether disabling cel.OptOptimize has any effect as I notice that the tests don't check this case.

@rodaine
Copy link
Author

rodaine commented May 1, 2023

Removing just cel.OptOptimize from the above still fails with the same error. I think the above case is passing for you because the variable type is being set to Dyn. If you make it the explicit type of the message, you should see the same error.

@TristonianJones
Copy link
Collaborator

@rodaine Thank you, that was the piece I missed. My PR now reflects the fix and test case.

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 a pull request may close this issue.

2 participants