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

parse: include location of rule ref in AST #5790

Closed
anderseknert opened this issue Mar 23, 2023 · 4 comments
Closed

parse: include location of rule ref in AST #5790

anderseknert opened this issue Mar 23, 2023 · 4 comments

Comments

@anderseknert
Copy link
Member

anderseknert commented Mar 23, 2023

AST information from a rule head currently only includes the location of the value, not the rule ref var itself.

package location

foo := "bar"
opa parse location.rego --format json --json-include locations
{
  "rules": [
    {
      "head": {
        "name": "foo",
        "value": {
          "location": {
            "file": "location.rego",
            "row": 3,
            "col": 8
          },
          "type": "string",
          "value": "bar"
        },
        "assign": true,
        "ref": [
          {
            "type": "var",
            "value": "foo"
          }
        ]
      }
    }
  ]
}

For tooling that works with the AST, this location information would be useful.

@charlieegan3
Copy link
Contributor

Hmm, interesting. I think that we might be able to dig into where NewHead and RefHead are used and make sure we're setting the location on the Head structs as they're initially created. It supports the JSON opts, but I don't think the location is getting set during parsing.

@Trolloldem
Copy link
Contributor

I have taken a look into the code of NewHead and RefHead as suggested by @charlieegan3, and noticed that when they are called in parseHead, the defer function should handle the process of setting the Location of the Head of a Rule.

While investigating the motivation that causes the output to have missing location, I tried to use the following code as an example:

package test

default allow = false
allow = true {
   input.method == "GET"
   input.path = ["getSalary", user]
   input.user == user
}

In this case the output relative to the line allow = true is the following:

      "head": {
        "name": "allow",
        "value": {
          "location": {
            "file": "t2.rego",
            "row": 4,
            "col": 9
          },
          "type": "boolean",
          "value": true
        },
        "ref": [
          {
            "type": "var",
            "value": "allow"
          }
        ],
        "location": {
          "file": "t2.rego",
          "row": 4,
          "col": 1
        }
      },
      "location": {
        "file": "t2.rego",
        "row": 4,
        "col": 1
      }

There is a similar part of the output for the default allow = false.

What I noticed after this, is that in parseModule, the statement used in the example of @anderseknert is handled by the Body case insted of the *Rule one. This causes the method ParseRuleFromBody to create a new Rule struct. The problem with this approach is that the code used to configure the correct JSON setting in Parse is never called on this new struct.

I have tried to add similar code in the switch statement mentioned above and this is the output for the example of @anderseknert:

"rules": [
    {
      "body": [
        {
          "index": 0,
          "location": {
            "file": "test.rego",
            "row": 3,
            "col": 8
          },
          "terms": {
            "location": {
              "file": "test.rego",
              "row": 3,
              "col": 8
            },
            "type": "boolean",
            "value": true
          }
        }
      ],
      "head": {
        "name": "foo",
        "value": {
          "location": {
            "file": "test.rego",
            "row": 3,
            "col": 8
          },
          "type": "string",
          "value": "bar"
        },
        "assign": true,
        "ref": [
          {
            "type": "var",
            "value": "foo"
          }
        ],
        "location": {
          "file": "test.rego",
          "row": 3,
          "col": 1
        }
      },
      "location": {
        "file": "test.rego",
        "row": 3,
        "col": 1
      }
    }
  ]

I can work on this to further investigate if only this addition fixes the problem and add some tests.

@anderseknert
Copy link
Member Author

That'd be amazing! Thanks @Trolloldem 😃

Trolloldem added a commit to Trolloldem/opa that referenced this issue Apr 3, 2023
Prior to this commit, when the AST information
were serialized to JSON, rules expressed as

discussed in open-policy-agent#5790 included only the location
of their value and not the one of the rule
ref var itself. Now the AST serialized to
JSON includes this information.

Fixes: open-policy-agent#5790
Signed-off-by: Gianluca Oldani <[email protected]>
Trolloldem added a commit to Trolloldem/opa that referenced this issue Apr 8, 2023
Prior to this commit, when the AST information
were serialized to JSON, rules expressed as

discussed in open-policy-agent#5790 included only the location
of their value and not the one of the rule
ref var itself. Now the AST serialized to
JSON includes this information.

Fixes: open-policy-agent#5790
Signed-off-by: Gianluca Oldani <[email protected]>
Trolloldem added a commit to Trolloldem/opa that referenced this issue Apr 8, 2023
Prior to this commit, when the AST information
were serialized to JSON, rules expressed as

discussed in open-policy-agent#5790 included only the location
of their value and not the one of the rule
ref var itself. Now the AST serialized to
JSON includes this information.

Fixes: open-policy-agent#5790
Signed-off-by: Gianluca Oldani <[email protected]>
Trolloldem added a commit to Trolloldem/opa that referenced this issue Apr 12, 2023
Prior to this commit, when the AST information
were serialized to JSON, rules expressed as

discussed in open-policy-agent#5790 included only the location
of their value and not the one of the rule
ref var itself. Now the AST serialized to
JSON includes this information.

Fixes: open-policy-agent#5790
Signed-off-by: Gianluca Oldani <[email protected]>
Trolloldem added a commit to Trolloldem/opa that referenced this issue Apr 12, 2023
Prior to this commit, when the AST information
were serialized to JSON, rules expressed as

discussed in open-policy-agent#5790 included only the location
of their value and not the one of the rule
ref var itself. Now the AST serialized to
JSON includes this information.

Fixes: open-policy-agent#5790
Signed-off-by: Gianluca Oldani <[email protected]>
Trolloldem added a commit to Trolloldem/opa that referenced this issue Apr 17, 2023
Prior to this commit, when the AST information
were serialized to JSON, rules expressed as

discussed in open-policy-agent#5790 included only the location
of their value and not the one of the rule
ref var itself. Now the AST serialized to
JSON includes this information.

Fixes: open-policy-agent#5790
Signed-off-by: Gianluca Oldani <[email protected]>
kjothen pushed a commit to kjothen/opa that referenced this issue Apr 25, 2023
Prior to this commit, when the AST information
were serialized to JSON, rules expressed as

discussed in open-policy-agent#5790 included only the location
of their value and not the one of the rule
ref var itself. Now the AST serialized to
JSON includes this information.

Fixes: open-policy-agent#5790
Signed-off-by: Gianluca Oldani <[email protected]>
@anderseknert
Copy link
Member Author

Seems to be missing still in some cases: #6860

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

No branches or pull requests

4 participants