JSONPath filter expression is invalid

[dibyom]

Expected Behavior

Given the expression: $(body.taskRun.spec.outputs.resources[?(@.name == 'bucket')].resourceRef.name), the expression should be valid

Actual Behavior

User gets a "unterminated expression" error

{"level":"error","logger":"eventlistener","caller":"sink/sink.go:144","msg":"failed to ApplyEventValuesToParams: &{%!w(string=failed to replace JSONPath value for param bucket: {string $(body.taskRun.spec.outputs.resources[?(@.name == 'bucket')].resourceRef.name) []}: &{%!w(string=unterminated filter)})}","knative.dev/controller":"eventlistener","/triggers-eventid":"958bw","/trigger":"log-collection","stacktrace":"github.com/tektoncd/triggers/pkg/sink.Sink.processTrigger\n\t/workspace/go/src/github.com/tektoncd/triggers/pkg/sink/sink.go:144\ngithub.com/tektoncd/triggers/pkg/sink.Sink.HandleEvent.func1\n\t/workspace/go/src/github.com/tektoncd/triggers/pkg/sink/sink.go:93"}

[afrittoli]

I tried a few more, but no luck

$(body.taskRun.spec.outputs.resources[?(@.name=='bucket')].resourceRef.name)
$(body.taskRun.spec.outputs.resources[?(@.name=="bucket")].resourceRef.name)
$(body.taskRun.spec.outputs.resources[?(@.name==\"bucket\")].resourceRef.name)

When I drop the filter it works fine:

$(body.taskRun.spec.outputs.resources[0].resourceRef.name)
$(body.taskRun.spec.outputs.resources[*].resourceRef.name)

[dibyom]

/kind bug

[afrittoli]

I tried on the latest codebase, adding a unit test in jsonpath_test.go with array filtering works fine:

{
		name: "Array filters",
		in:   `{"body":{"child":[{"a": "b", "w": "1"}, {"a": "c", "w": "2"}, {"a": "d", "w": "3"}]}}`,
		expr: "$(body.child[?(@.a == 'd')].w)",
		want: "3",
}

[dibyom]

Now that's interesting...I'd have expected that to fail

[dibyom]

The issue is with the tektonVar regex that is being used in applyEventValuesToParams

The regex is \$\(?([^\)]+)\) -- it keeps matching all chars that are not a ) so when it sees the brackets in the filter expression it returns prematurely leading to the unterminated expression.

Maybe we could use \$\((.*)\) which will match everything until the last ). This will work for the scenario but will break in the case that there are multiple JSONPath expressions in the same string.

[afrittoli]

The need for a regex comes from the fact that we support embedding the jsonpath filter into text.
One simple solution could be to not support embedding jsonpath in text.
Alternatively we could try using recursion in regex to find balanced parenthesis - see https://www.regular-expressions.info/recurse.html#balanced - I've not tried that and I don't know if the golang regex library supports that?

[dibyom]

The need for a regex comes from the fact that we support embedding the jsonpath filter into text.

yes, and that we support multiple jsonpath expressions in the same string

One simple solution could be to not support embedding jsonpath in text.

Yeah, especially since you can do that in templates already. I proposed something similar in #253 (Also related, #367)

One issue here though is that we'd be making a breaking change in a patch release

Alternatively we could try using recursion in regex to find balanced parenthesis - see https://www.regular-expressions.info/recurse.html#balanced - I've not tried that and I don't know if the golang regex library supports that?

Heh. Yeah I did try a couple on regex101. Though I think that needs recursion support which needs a PCRE implementation while the Go regex implementation uses Re2

Perhaps, we can get away with something hacky like splitting on $( and then using a regex to extract each expression?

[afrittoli]

The need for a regex comes from the fact that we support embedding the jsonpath filter into text.

yes, and that we support multiple jsonpath expressions in the same string

One simple solution could be to not support embedding jsonpath in text.

Yeah, especially since you can do that in templates already. I proposed something similar in #253 (Also related, #367)

One issue here though is that we'd be making a breaking change in a patch release

Alternatively we could try using recursion in regex to find balanced parenthesis - see https://www.regular-expressions.info/recurse.html#balanced - I've not tried that and I don't know if the golang regex library supports that?

Heh. Yeah I did try a couple on regex101. Though I think that needs recursion support which needs a PCRE implementation while the Go regex implementation uses Re2

Perhaps, we can get away with something hacky like splitting on $( and then using a regex to extract each expression?

We could have some code - not based on regex - that looks for balanced parenthesis after a $( - but that same code code could then return the simplified jsonpath expression - I don't think we would need regex anymore? Alternatively the code that looks for balanced parenthesis could prepend/append some TOKEN e.g. __JSONPATHSTART__ and __JSONPATHEND__ that we then could use in the regex to get the jsonpath as regex groups like today.

[dibyom]

/assign

[dibyom]

We could have some code - not based on regex - that looks for balanced parenthesis after a $( - but that same code code could then return the simplified jsonpath expression - I don't think we would need regex anymore?

Yeah, since we are only really trying to remove the portion of the expression that tekton adds i.e. the leading $( and the last ) I think something like this might also work:

	multi = "$(body.child[?(@.a == 'd')].w)-$(asd)-blahblah"
	s := strings.Split(multi, "$(")
	for _, i := range s {
		if strings.Contains(i, ")") {
			 i = i[:strings.LastIndex(i, ")")]
			 fmt.Println(i)
		}
	}

[dibyom]

wip fix in https://github.com/tektoncd/triggers/compare/master...dibyom:issues/365?expand=1