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

internal/planner: Insert general ref head objects starting from the leaves, not root #6401

Merged
merged 1 commit into from Nov 15, 2023

Conversation

srenatus
Copy link
Contributor

@srenatus srenatus commented Nov 13, 2023

This way the object insert operations can return a new object instance.

Before, the object construction for a rule like

p[a][b] := ...

would look like this:

*ir.BlockStmt BlockStmt (1 blocks)
  *ir.Block Block (3 statements)
    *ir.BlockStmt BlockStmt (1 blocks)
      *ir.Block Block (2 statements)
        *ir.DotStmt &{Source:{Value:Local<2>} Key:{Value:Local<10>} Target:Local<14>}
        *ir.BreakStmt &{Index:1}
    *ir.MakeObjectStmt &{Target:Local<14>}
    *ir.ObjectInsertOnceStmt &{Key:{Value:Local<10>} Value:{Value:Local<14>} Object:Local<2>}
*ir.ObjectInsertOnceStmt &{Key:{Value:Local<11>} Value:{Value:Local<13>} Object:Local<14>}

Now, it'll look like

*ir.BlockStmt BlockStmt (1 blocks)
  *ir.Block Block (2 statements)
    *ir.BlockStmt BlockStmt (1 blocks)
      *ir.Block Block (2 statements)
        *ir.DotStmt &{Source:{Value:Local<2>} Key:{Value:Local<10>} Target:Local<14>}
        *ir.BreakStmt &{Index:1}
    *ir.MakeObjectStmt &{Target:Local<14>}
*ir.ObjectInsertOnceStmt &{Key:{Value:Local<11>} Value:{Value:Local<13>} Object:Local<14>}
*ir.ObjectInsertStmt &{Key:{Value:Local<10>} Value:{Value:Local<14>} Object:Local<2>}

so the object in Local<14> is built first, and the added to object Local<2>.

@srenatus srenatus marked this pull request as ready for review November 13, 2023 10:35
johanfylling
johanfylling previously approved these changes Nov 13, 2023
Copy link
Contributor

@johanfylling johanfylling left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes more sense 👍.

It would seem the original implementation was relying on an implementation detail in the Wasm engine/transpilation it shouldn't have.

I recall there being an issue with an earlier, similar approach to this, that this change doesn't appear to suffer from. But that earlier approach did some other things differently also, so I think those issues came from elsewhere.

There are yaml tests covering this; but I see we don't have tests for more than two vars in the ref. Maybe we should add at least one more case there covering a longer ref with more vars; e.g. p[a][b][c][d] := ..., where each var enumerates to more than one value.

internal/planner/planner.go Outdated Show resolved Hide resolved
@srenatus
Copy link
Contributor Author

Thanks for the review, I'll add some tests there tomorrow

…eaves, not root.

This way the object insert operations can return a new object instance.

Before, the object construction for a rule like

    p[a][b] := ...

would look like this:

    *ir.BlockStmt BlockStmt (1 blocks)
      *ir.Block Block (3 statements)
        *ir.BlockStmt BlockStmt (1 blocks)
          *ir.Block Block (2 statements)
            *ir.DotStmt &{Source:{Value:Local<2>} Key:{Value:Local<10>} Target:Local<14>}
            *ir.BreakStmt &{Index:1}
        *ir.MakeObjectStmt &{Target:Local<14>}
        *ir.ObjectInsertOnceStmt &{Key:{Value:Local<10>} Value:{Value:Local<14>} Object:Local<2>}
    *ir.ObjectInsertOnceStmt &{Key:{Value:Local<11>} Value:{Value:Local<13>} Object:Local<14>}

Now, it'll look like

    *ir.BlockStmt BlockStmt (1 blocks)
      *ir.Block Block (2 statements)
        *ir.BlockStmt BlockStmt (1 blocks)
          *ir.Block Block (2 statements)
            *ir.DotStmt &{Source:{Value:Local<2>} Key:{Value:Local<10>} Target:Local<14>}
            *ir.BreakStmt &{Index:1}
        *ir.MakeObjectStmt &{Target:Local<14>}
    *ir.ObjectInsertOnceStmt &{Key:{Value:Local<11>} Value:{Value:Local<13>} Object:Local<14>}
    *ir.ObjectInsertStmt &{Key:{Value:Local<10>} Value:{Value:Local<14>} Object:Local<2>}

so the object in Local<14> is built first, and the added to object Local<2>.

Co-authored-by: Teemu Koponen <koponen@styra.com>
Signed-off-by: Stephan Renatus <stephan@styra.com>
@srenatus
Copy link
Contributor Author

@johanfylling if you could have another look, I'd appreciate it 😃

Copy link
Contributor

@johanfylling johanfylling left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! LGTM 👍

@srenatus srenatus merged commit e71e519 into open-policy-agent:main Nov 15, 2023
24 checks passed
@srenatus srenatus deleted the sr/planner-outer-in branch November 15, 2023 14:24
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 this pull request may close these issues.

None yet

3 participants