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

topdown.RegisterBuiltinFunc Doesn't provide standardized error wrapping #2101

Closed
patrick-east opened this issue Feb 13, 2020 · 0 comments · Fixed by #2110
Closed

topdown.RegisterBuiltinFunc Doesn't provide standardized error wrapping #2101

patrick-east opened this issue Feb 13, 2020 · 0 comments · Fixed by #2110

Comments

@patrick-east
Copy link
Contributor

Expected Behavior

When registering a builtin with the older topdown.RegisterFunctionalBuiltinX helpers the returned error is always checked and wrapped as needed with a topdown.Error. These errors have some nice additions like location info and a standard look/feel. All of the builtin functions should have error messages that have a similar look and feel.

Actual Behavior

The newer topdown.RegisterBuiltinFunc doesn't do any function wrapping, so any builtin functions that use it no longer get their errors wrapped unless they explicity return topdown.Error's with appropriate info.

The current result is that a there are a mix of error types returned by builtin functions and a user hitting errors will see different style of error messaging depending on the builtin.

Steps to Reproduce the Problem

{17:11} ~ ❯ cat /tmp/policy.rego
package foo

not_wrapped {
   net.cidr_expand(input.x)
}

wrapped {
   net.cidr_contains(input.x, "")
}
{17:11} ~ ❯ cat /tmp/input.json
{"x": 123}
{17:11} ~ ❯ opa eval -d /tmp/policy.rego -i /tmp/input.json 'data.foo.wrapped'
{
  "errors": [
    {
      "message": "net.cidr_contains: operand 1 must be string but got number",
      "code": "eval_type_error",
      "location": {
        "file": "/tmp/policy.rego",
        "row": 8,
        "col": 4
      }
    }
  ]
}
{17:11} ✘ ~ ❯ opa eval -d /tmp/policy.rego -i /tmp/input.json 'data.foo.not_wrapped'
{
  "errors": [
    {
      "message": "operand 1 must be string but got number"
    }
  ]
}

Additional Info

We should still be able to wrap the function call like we are doing now to normalize the error type returned.

patrick-east added a commit to patrick-east/opa that referenced this issue Feb 19, 2020
The older RegisterFunctionalBuiltinX functions would do this and it
gave the builtin error messaging/typing a nice uniform look and feel.

This changes the newer RegisterBuiltinFunc to do the same thing, so
any builtin will now either return a nil error or a `*topdown.Error`.
With an appropriate code, location, and message.

Fixes: open-policy-agent#2101
Signed-off-by: Patrick East <east.patrick@gmail.com>
tsandall pushed a commit that referenced this issue Feb 20, 2020
The older RegisterFunctionalBuiltinX functions would do this and it
gave the builtin error messaging/typing a nice uniform look and feel.

This changes the newer RegisterBuiltinFunc to do the same thing, so
any builtin will now either return a nil error or a `*topdown.Error`.
With an appropriate code, location, and message.

Fixes: #2101
Signed-off-by: Patrick East <east.patrick@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

1 participant