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

Add ExcludeLocationFile JSON marshalling option #6398

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions ast/json/json.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,9 @@ type MarshalOptions struct {
IncludeLocation NodeToggle
// IncludeLocationText additionally/optionally includes the text of the location
IncludeLocationText bool
// ExcludeLocationFile additionally/optionally excludes the file of the location
// Note that this is inverted (i.e. not "include" as the default needs to remain false)
ExcludeLocationFile bool
}

// NodeToggle is a generic struct to allow the toggling of
Expand Down
21 changes: 19 additions & 2 deletions ast/location/location.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ type Location struct {
Col int `json:"col"` // The column in the row.
Offset int `json:"-"` // The byte offset for the location in the source.

// JSONOptions specifies options for marshaling and unmarshaling of locations
// JSONOptions specifies options for marshaling and unmarshalling of locations
JSONOptions astJSON.Options
}

Expand Down Expand Up @@ -96,15 +96,32 @@ func (loc *Location) Compare(other *Location) int {

func (loc *Location) MarshalJSON() ([]byte, error) {
// structs are used here to preserve the field ordering of the original Location struct
if loc.JSONOptions.MarshalOptions.ExcludeLocationFile {
Copy link
Contributor

Choose a reason for hiding this comment

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

Doing something like

data := struct {
	File string `json:"file,omitempty"`
	Row  int    `json:"row"`
	Col  int    `json:"col"`
	Text []byte `json:"text,omitempty"`
}{
	Row: loc.Row,
	Col: loc.Col,
}

if !loc.JSONOptions.MarshalOptions.ExcludeLocationFile {
	data.File = loc.File
}

would eliminate some redundancy here without changing the behavior, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried that, and I could swear I had empty "" show up still, but now that I try again, it seems to work. If CI is happy too, then let's have this merged :)

Copy link
Member Author

Choose a reason for hiding this comment

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

I had edited code in the wrong location 😬 It doesn't work as I'm still seeing tests fail with "file": "". Not sure why omitempty doesn't bite.

data := struct {
Row int `json:"row"`
Col int `json:"col"`
Text []byte `json:"text,omitempty"`
}{
Row: loc.Row,
Col: loc.Col,
}

if loc.JSONOptions.MarshalOptions.IncludeLocationText {
data.Text = loc.Text
}

return json.Marshal(data)
}

data := struct {
File string `json:"file"`
Row int `json:"row"`
Col int `json:"col"`
Text []byte `json:"text,omitempty"`
}{
File: loc.File,
Row: loc.Row,
Col: loc.Col,
File: loc.File,
}

if loc.JSONOptions.MarshalOptions.IncludeLocationText {
Expand Down
13 changes: 13 additions & 0 deletions ast/location/location_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,19 @@ func TestLocationMarshal(t *testing.T) {
},
exp: `{"file":"file","row":1,"col":1,"text":"dGV4dA=="}`,
},
"excluding file": {
loc: &Location{
File: "file",
Row: 1,
Col: 1,
JSONOptions: astJSON.Options{
MarshalOptions: astJSON.MarshalOptions{
ExcludeLocationFile: true,
},
},
},
exp: `{"row":1,"col":1}`,
},
}

for id, tc := range testCases {
Expand Down
22 changes: 22 additions & 0 deletions ast/marshal_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,28 @@ func TestGeneric_MarshalWithLocationJSONOptions(t *testing.T) {
}(),
ExpectedJSON: `{"location":{"file":"example.rego","row":1,"col":2,"text":"dGhpbmdz"},"type":"string","value":"example"}`,
},
"location included, location text included, file excluded": {
Term: func() *Term {
v, _ := InterfaceToValue("example")
t := &Term{
Value: v,
Location: NewLocation([]byte("things"), "example.rego", 1, 2),
}
t.setJSONOptions(
astJSON.Options{
MarshalOptions: astJSON.MarshalOptions{
IncludeLocation: astJSON.NodeToggle{
Term: true,
},
IncludeLocationText: true,
ExcludeLocationFile: true,
},
},
)
return t
}(),
ExpectedJSON: `{"location":{"row":1,"col":2,"text":"dGhpbmdz"},"type":"string","value":"example"}`,
},
}

for name, data := range testCases {
Expand Down
29 changes: 29 additions & 0 deletions ast/parser_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3959,6 +3959,35 @@ func TestRuleFromBodyJSONOptions(t *testing.T) {
}
}

func TestRuleFromBodyJSONOptionsLocationOptions(t *testing.T) {
parserOpts := ParserOptions{ProcessAnnotation: true}
parserOpts.JSONOptions = &astJSON.Options{
MarshalOptions: astJSON.MarshalOptions{
IncludeLocation: astJSON.NodeToggle{
Term: true,
Package: true,
Comment: true,
Import: true,
Rule: true,
Head: true,
Expr: true,
SomeDecl: true,
Every: true,
With: true,
Annotations: true,
AnnotationsRef: true,
},
IncludeLocationText: true,
ExcludeLocationFile: true,
},
}

module := `package a.b.c
foo := "bar"
`
assertParseModuleJSONOptions(t, `foo := "bar"`, module, parserOpts)
}

func TestRuleModulePtr(t *testing.T) {
mod := `package test

Expand Down