feat(deps): upgrade expr from 1.16.0 to 1.16.9 to support concat function. Fixes #13175

[jiachengxu]

Fixes #13175

Verification

Before this PR, creating the following workflow in the original issue:

apiVersion: argoproj.io/v1alpha1
kind: Workflow
metadata:
  generateName: test-expression-
  namespace: argo
spec:
  entrypoint: main
  arguments:
    parameters:
      - name: expr
        value: "{{= concat(['a', 'b'], ['c', 'd']) | join('\\n') }}"
  templates:
    - name: main
      inputs:
        parameters:
          - name: expr
      script:
        image: alpine:3.6
        command: ["sh"]
        source: |
          echo result: '{{ inputs.parameters.expr }}'

it logs in the main container:

result: {{= concat([a, b], [c, d]) | join(n) }}

With this PR, it logs:

result: a
b
c
d

[agilgur5]

Looks like some of the tests are failing (unit and E2E) as the error message format is ever so slightly different. It seems like expr isn't giving a line number back anymore?

Error:      	Not equal: 
        	            	expected: "failed to dispatch event: failed to evaluate workflow template expression: unexpected token EOF (1:1)"
        	            	actual  : "failed to dispatch event: failed to evaluate workflow template expression: unexpected token EOF"

[jiachengxu]

#13194 (comment)

Yes, right. Debugging the test locally and it seems like the line number, e.g., (1:1) is not there anymore

[jiachengxu]

Is it OK if we just update the error message to make it match what expr currently returns?

[agilgur5]

Yes I think that's fine. This actually might be a bug in expr, as it doesn't seem to be mentioned in the release notes anywhere. cc @antonmedv

[antonmedv]

Will take a look. Is there an example what reproduces the error?

[jiachengxu]

Will take a look. Is there an example what reproduces the error?

@antonmedv Thanks for taking a look at this. You can use the following program to reproduce the issue:

package main

import (
	"log"

	"github.com/expr-lang/expr"
)

func main() {
	input := ""
	env := map[string]interface{}{
		"key": "value",
	}
	_, err := expr.Compile(input, expr.Env(env))
	if err != nil {
		log.Fatal(err)
	}
}

I also did some experiments and I narrowed the issue down to v1.16.8, before v1.16.8, the error is:

unexpected token EOF (1:1)

For version >= v1.16.8, the line number in the error is not available anymore, and the error message becomes the following:

unexpected token EOF

[antonmedv]

I took a look on why this error is here. Looks like in this case, as input is empty "", Expr does not add any more location information (I don't think it is useful in the case anyway).

I can just append this string to the error message + " (1, 1)" or we can the test (remote location part, or use Contains(t, "unexpected token EOF", err.Error())).

What do you think?

[jiachengxu]

I took a look on why this error is here. Looks like in this case, as input is empty "", Expr does not add any more location information (I don't think it is useful in the case anyway).

I can just append this string to the error message + " (1, 1)" or we can the test (remote location part, or use Contains(t, "unexpected token EOF", err.Error())).

What do you think?

@antonmedv thank you for taking a look at this. Yes, I agree with you that in this case the location info doesn't help that much, I think we can just update our test by removing the location from the expected error message. @agilgur5 Please let me know your thought on this, if you agree, I can update tests this PR.

[agilgur5]

I would say yes to both actually 😅

I don't think tests, in general, should rely on exact errors, as error specifics can change (and improving an error message, for instance, is usually not considered a breaking change). Especially if the source code does not rely on that specific error message; if it changes a bit, it's often not impactful. So @jiachengxu I think that change makes sense to do either way.

But I also think expr should have consistent errors, and it's not clear to me why this behavior changed. It is empty so the line number isn't very useful, but it makes sense to have it for consistency's sake. I also imagine there might be other users who's tests might break, so I think it would be good to add that back as well @antonmedv. Unless you had a good reason to remove it, but it seems like it was unintentional?

[antonmedv]

Expr provides errors during compilation from a few sources, some comes with a "snippet" |----------^.

My idea was to make this consistent across all errors. If an error contains a snippet |----^, add (line, column) to the error.

[antonmedv]

Also I was refactoring error location information from line and column for offsets start and end. This way it is possible to select the whole token with error.

[tooptoop4]

i wonder if this helps with #13061 do u happen to use SSO RBAC @jiachengxu ?

[jiachengxu]

i wonder if this helps with #13061 do u happen to use SSO RBAC @jiachengxu ?

No, I am not using SSO RBAC feature for my setup

[jiachengxu]

I have updated the tests to use Contains instead of Equal: bcbfef5

[agilgur5]

I have updated the tests to use Contains instead of Equal: bcbfef5

Yea that's what I usually do with these kinds of error message based tests.

LGTM, thanks for the update!

i wonder if this helps with #13061 do u happen to use SSO RBAC @jiachengxu ?

@tooptoop4 we'll want someone to test with :latest once this is merged and the image is released. The expr changelog didn't mention anything that I thought would affect it, but 🤷 It might've been something more innocuous like this change or could be a bug in the Argo portion of the templating logic