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

wasm: only returns first binding #3281

Closed
srenatus opened this issue Mar 16, 2021 · 3 comments · Fixed by #3282
Closed

wasm: only returns first binding #3281

srenatus opened this issue Mar 16, 2021 · 3 comments · Fixed by #3282
Assignees

Comments

@srenatus
Copy link
Contributor

$ opa eval -t wasm 'xs := ["foo", "bar"]; xs[x]' --format=pretty
+---+---------------+-------+
| x |      xs       | xs[x] |
+---+---------------+-------+
| 1 | ["foo","bar"] | "bar" |
+---+---------------+-------+

$ opa eval -t rego 'xs := ["foo", "bar"]; xs[x]' --format=pretty
+---+---------------+-------+
| x |      xs       | xs[x] |
+---+---------------+-------+
| 0 | ["foo","bar"] | "foo" |
| 1 | ["foo","bar"] | "bar" |
+---+---------------+-------+
@tsandall
Copy link
Member

Aha, here's the offending line:

https://github.com/open-policy-agent/opa/blob/master/rego/rego.go#L1927

@srenatus srenatus self-assigned this Mar 16, 2021
srenatus added a commit to srenatus/opa that referenced this issue Mar 16, 2021
Fixes open-policy-agent#3281.

Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
tsandall pushed a commit that referenced this issue Mar 16, 2021
Fixes #3281.

Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
@tsandall tsandall reopened this Mar 16, 2021
@tsandall
Copy link
Member

tsandall commented Mar 16, 2021

@srenatus for tomorrow...

Looks like there's another case that's not handled:

torin:~$ opa eval -t wasm 'x := [1,1]; x[_]'
{
  "result": [
    {
      "expressions": [
        {
          "value": true,
          "text": "x := [1,1]",
          "location": {
            "row": 1,
            "col": 1
          }
        },
        {
          "value": 1,
          "text": "x[_]",
          "location": {
            "row": 1,
            "col": 13
          }
        }
      ],
      "bindings": {
        "x": [
          1,
          1
        ]
      }
    }
  ]
}
torin:~$ opa eval -t rego 'x := [1,1]; x[_]'
{
  "result": [
    {
      "expressions": [
        {
          "value": true,
          "text": "x := [1,1]",
          "location": {
            "row": 1,
            "col": 1
          }
        },
        {
          "value": 1,
          "text": "x[_]",
          "location": {
            "row": 1,
            "col": 13
          }
        }
      ],
      "bindings": {
        "x": [
          1,
          1
        ]
      }
    },
    {
      "expressions": [
        {
          "value": true,
          "text": "x := [1,1]",
          "location": {
            "row": 1,
            "col": 1
          }
        },
        {
          "value": 1,
          "text": "x[_]",
          "location": {
            "row": 1,
            "col": 13
          }
        }
      ],
      "bindings": {
        "x": [
          1,
          1
        ]
      }
    }
  ]
}

OPA version:

$ opa version
Version: 0.28.0-dev
Build Commit: 1d747866
Build Timestamp: 2021-03-16T20:40:39Z
Build Hostname: bigbox.localdomain
Go Version: go1.15.2
WebAssembly: available

Not sure if this one can be traced back to the rego package or if it's happening deeper. E.g., I think we use the opa_set_t implementation from the C library for the results...are the results getting deduped? Technically the query results are sets though so perhaps we don't treat this as a bug...

EDIT: Didn't mean to re-open.

@srenatus
Copy link
Contributor Author

💭 Hmm. So, we'll definitely call result set add, and that will call opa_set_add,

case *ir.ResultSetAdd:
instrs = append(instrs, instruction.GetLocal{Index: c.lrs})
instrs = append(instrs, instruction.GetLocal{Index: c.local(stmt.Value)})
instrs = append(instrs, instruction.Call{Index: c.function(opaSetAdd)})

which then deduplicates:

opa/wasm/src/value.c

Lines 1477 to 1480 in bd5c572

if (opa_value_compare(curr->v, v) == 0)
{
return;
}

I suppose we could start a list of known differences that we're not going to address (at least not right away). This one and #3252 are on my radar so far...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants