From c7c4fab9b443c5a12b3adf7d92174456120219d5 Mon Sep 17 00:00:00 2001 From: Anders Eknert Date: Sat, 18 Jan 2025 18:59:26 +0100 Subject: [PATCH] Remove jsonOptions from AST nodes and terms MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This was originally added to allow Regal to get a serialized AST that included location data wherever that was possible. Regal is however no longer using OPA's JSON serialization but its own custom encoder. Attaching these options to every AST node and term comes with a cost attached, and without any known users of this feature vs. the many users who care about resource utilization, this feels like an easy choice. While it seems unlikely to be users depending on this functionality — in case someone needs it, the options for serializing AST nodes to JSON can now be set globally instead. Global state is always awkward, but since JSON marshalling methods only have access to the node being marshalled and of course, global state, there's not a whole lot of options if we intend to keep this feature. Signed-off-by: Anders Eknert --- cmd/parse.go | 5 +- internal/bundle/inspect/inspect.go | 36 +- internal/bundle/inspect/inspect_test.go | 11 + v1/ast/annotations.go | 29 +- v1/ast/json/json.go | 70 ++++ v1/ast/location/location.go | 10 +- v1/ast/location/location_test.go | 24 +- v1/ast/marshal.go | 11 - v1/ast/marshal_test.go | 448 +++++++++++++----------- v1/ast/parser.go | 28 +- v1/ast/parser_ext.go | 69 +--- v1/ast/parser_test.go | 133 +------ v1/ast/policy.go | 116 +----- v1/ast/term.go | 12 +- v1/bundle/bundle.go | 10 +- v1/loader/loader.go | 12 +- v1/loader/loader_test.go | 12 +- v1/tester/runner.go | 2 - 18 files changed, 435 insertions(+), 603 deletions(-) delete mode 100644 v1/ast/marshal.go diff --git a/cmd/parse.go b/cmd/parse.go index 8eddefdc362..0ac3fc2e17f 100644 --- a/cmd/parse.go +++ b/cmd/parse.go @@ -90,7 +90,7 @@ func parse(args []string, params *parseParams, stdout io.Writer, stderr io.Write RegoVersion: params.regoVersion(), } if exposeLocation { - parserOpts.JSONOptions = &astJSON.Options{ + astJSON.SetOptions(astJSON.Options{ MarshalOptions: astJSON.MarshalOptions{ IncludeLocationText: true, IncludeLocation: astJSON.NodeToggle{ @@ -108,7 +108,8 @@ func parse(args []string, params *parseParams, stdout io.Writer, stderr io.Write AnnotationsRef: true, }, }, - } + }) + defer astJSON.SetOptions(astJSON.Defaults()) } result, err := loader.RegoWithOpts(args[0], parserOpts) diff --git a/internal/bundle/inspect/inspect.go b/internal/bundle/inspect/inspect.go index 43f203faf86..ff9ceaa17b6 100644 --- a/internal/bundle/inspect/inspect.go +++ b/internal/bundle/inspect/inspect.go @@ -43,18 +43,20 @@ func FileForRegoVersion(regoVersion ast.RegoVersion, path string, includeAnnotat } func bundleOrDirInfoForRegoVersion(regoVersion ast.RegoVersion, path string, includeAnnotations bool) (*Info, error) { + json.SetOptions(json.Options{ + MarshalOptions: json.MarshalOptions{ + IncludeLocation: json.NodeToggle{ + // Annotation location data is only included if includeAnnotations is set + AnnotationsRef: includeAnnotations, + }, + }, + }) + defer json.SetOptions(json.Defaults()) + b, err := loader.NewFileLoader(). WithRegoVersion(regoVersion). WithSkipBundleVerification(true). WithProcessAnnotation(true). // Always process annotations, for enriching namespace listing - WithJSONOptions(&json.Options{ - MarshalOptions: json.MarshalOptions{ - IncludeLocation: json.NodeToggle{ - // Annotation location data is only included if includeAnnotations is set - AnnotationsRef: includeAnnotations, - }, - }, - }). AsBundle(path) if err != nil { return nil, err @@ -209,18 +211,20 @@ func (bi *Info) getBundleDataWasmAndSignatures(name string) error { } func fileInfoForRegoVersion(regoVersion ast.RegoVersion, path string, includeAnnotations bool) (*Info, error) { + json.SetOptions(json.Options{ + MarshalOptions: json.MarshalOptions{ + IncludeLocation: json.NodeToggle{ + // Annotation location data is only included if includeAnnotations is set + AnnotationsRef: includeAnnotations, + }, + }, + }) + defer json.SetOptions(json.Defaults()) + res, err := loader.NewFileLoader(). WithRegoVersion(regoVersion). WithSkipBundleVerification(true). WithProcessAnnotation(true). // Always process annotations, for enriching namespace listing - WithJSONOptions(&json.Options{ - MarshalOptions: json.MarshalOptions{ - IncludeLocation: json.NodeToggle{ - // Annotation location data is only included if includeAnnotations is set - AnnotationsRef: includeAnnotations, - }, - }, - }). All([]string{path}) if err != nil { return nil, err diff --git a/internal/bundle/inspect/inspect_test.go b/internal/bundle/inspect/inspect_test.go index 96319ecdfa0..1d399b4f2cd 100644 --- a/internal/bundle/inspect/inspect_test.go +++ b/internal/bundle/inspect/inspect_test.go @@ -17,6 +17,7 @@ import ( "github.com/open-policy-agent/opa/internal/file/archive" "github.com/open-policy-agent/opa/v1/ast" + astJSON "github.com/open-policy-agent/opa/v1/ast/json" "github.com/open-policy-agent/opa/v1/bundle" "github.com/open-policy-agent/opa/v1/util/test" ) @@ -94,6 +95,16 @@ p = 1`, t.Fatalf("expected %d annotation, but got: %d", exp, got) } + astJSON.SetOptions(astJSON.Options{ + MarshalOptions: astJSON.MarshalOptions{ + IncludeLocation: astJSON.NodeToggle{ + // Annotation location data is only included if includeAnnotations is set + AnnotationsRef: true, + }, + }, + }) + defer astJSON.SetOptions(astJSON.Defaults()) + bs, err := json.Marshal(info.Annotations[0]) if err != nil { t.Fatalf("Unexpected error: %v", err) diff --git a/v1/ast/annotations.go b/v1/ast/annotations.go index cfa1c5dabc4..a7db8199f1e 100644 --- a/v1/ast/annotations.go +++ b/v1/ast/annotations.go @@ -38,9 +38,8 @@ type ( Custom map[string]interface{} `json:"custom,omitempty"` Location *Location `json:"location,omitempty"` - comments []*Comment - node Node - jsonOptions astJSON.Options + comments []*Comment + node Node } // SchemaAnnotation contains a schema declaration for the document identified by the path. @@ -77,8 +76,6 @@ type ( Annotations *Annotations `json:"annotations,omitempty"` Location *Location `json:"location,omitempty"` // The location of the node the annotations are applied to - jsonOptions astJSON.Options - node Node // The node the annotations are applied to } @@ -181,13 +178,6 @@ func (a *Annotations) GetTargetPath() Ref { } } -func (a *Annotations) setJSONOptions(opts astJSON.Options) { - a.jsonOptions = opts - if a.Location != nil { - a.Location.JSONOptions = opts - } -} - func (a *Annotations) MarshalJSON() ([]byte, error) { if a == nil { return []byte(`{"scope":""}`), nil @@ -229,7 +219,7 @@ func (a *Annotations) MarshalJSON() ([]byte, error) { data["custom"] = a.Custom } - if a.jsonOptions.MarshalOptions.IncludeLocation.Annotations { + if astJSON.GetOptions().MarshalOptions.IncludeLocation.Annotations { if a.Location != nil { data["location"] = a.Location } @@ -249,7 +239,6 @@ func NewAnnotationsRef(a *Annotations) *AnnotationsRef { Path: a.GetTargetPath(), Annotations: a, node: a.node, - jsonOptions: a.jsonOptions, } } @@ -282,10 +271,20 @@ func (ar *AnnotationsRef) MarshalJSON() ([]byte, error) { data["annotations"] = ar.Annotations } - if ar.jsonOptions.MarshalOptions.IncludeLocation.AnnotationsRef { + if astJSON.GetOptions().MarshalOptions.IncludeLocation.AnnotationsRef { if ar.Location != nil { data["location"] = ar.Location } + + // The location set for the schema ref terms is wrong (always set to + // row 1) and not really useful anyway.. so strip it out bedfore marshalling + for _, schema := range ar.Annotations.Schemas { + if schema.Path != nil { + for _, term := range schema.Path { + term.Location = nil + } + } + } } return json.Marshal(data) diff --git a/v1/ast/json/json.go b/v1/ast/json/json.go index 565017d58e3..9081fe7039a 100644 --- a/v1/ast/json/json.go +++ b/v1/ast/json/json.go @@ -1,5 +1,26 @@ +// Copyright 2023 The OPA Authors. All rights reserved. +// Use of this source code is governed by an Apache2 +// license that can be found in the LICENSE file. + +// This package provides options for JSON marshalling of AST nodes, and location +// data in particular. Since location data occupies a significant portion of the +// AST when included, it is excluded by default. The options provided here allow +// changing that behavior — either for all nodes or for specific types. Since +// JSONMarshaller implementations have access only to the node being marshaled, +// our options are to either attach these settings to *all* nodes in the AST, or +// to provide them via global state. The former is perhaps a little more elegant, +// and is what we went with initially. The cost of attaching these settings to +// every node however turned out to be non-negligible, and given that the number +// of users who have an interest in AST serialization are likely to be few, we +// have since switched to using global state, as provided here. Note that this +// is mostly to provide an equivalent feature to what we had before, should +// anyone depend on that. Users who need fine-grained control over AST +// serialization are recommended to use external libraries for that purpose, +// such as `github.com/json-iterator/go`. package json +import "sync" + // Options defines the options for JSON operations, // currently only marshaling can be configured type Options struct { @@ -34,3 +55,52 @@ type NodeToggle struct { Annotations bool AnnotationsRef bool } + +// configuredJSONOptions synchronizes access to the global JSON options +type configuredJSONOptions struct { + options Options + lock sync.RWMutex +} + +var options = &configuredJSONOptions{ + options: Defaults(), +} + +// SetOptions sets the global options for marshalling AST nodes to JSON +func SetOptions(opts Options) { + options.lock.Lock() + defer options.lock.Unlock() + options.options = opts +} + +// GetOptions returns (a copy of) the global options for marshalling AST nodes to JSON +func GetOptions() Options { + options.lock.RLock() + defer options.lock.RUnlock() + return options.options +} + +// Defaults returns the default JSON options, which is to exclude location +// information in serialized JSON AST. +func Defaults() Options { + return Options{ + MarshalOptions: MarshalOptions{ + IncludeLocation: NodeToggle{ + Term: false, + Package: false, + Comment: false, + Import: false, + Rule: false, + Head: false, + Expr: false, + SomeDecl: false, + Every: false, + With: false, + Annotations: false, + AnnotationsRef: false, + }, + IncludeLocationText: false, + ExcludeLocationFile: false, + }, + } +} diff --git a/v1/ast/location/location.go b/v1/ast/location/location.go index d529ad72dcd..716aad6930f 100644 --- a/v1/ast/location/location.go +++ b/v1/ast/location/location.go @@ -18,9 +18,6 @@ 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 unmarshalling of locations - JSONOptions astJSON.Options - Tabs []int `json:"-"` // The column offsets of tabs in the source. } @@ -98,7 +95,8 @@ 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 { + jsonOptions := astJSON.GetOptions().MarshalOptions + if jsonOptions.ExcludeLocationFile { data := struct { Row int `json:"row"` Col int `json:"col"` @@ -108,7 +106,7 @@ func (loc *Location) MarshalJSON() ([]byte, error) { Col: loc.Col, } - if loc.JSONOptions.MarshalOptions.IncludeLocationText { + if jsonOptions.IncludeLocationText { data.Text = loc.Text } @@ -126,7 +124,7 @@ func (loc *Location) MarshalJSON() ([]byte, error) { File: loc.File, } - if loc.JSONOptions.MarshalOptions.IncludeLocationText { + if jsonOptions.IncludeLocationText { data.Text = loc.Text } diff --git a/v1/ast/location/location_test.go b/v1/ast/location/location_test.go index 2ad9a179042..e35cb6d97f5 100644 --- a/v1/ast/location/location_test.go +++ b/v1/ast/location/location_test.go @@ -90,8 +90,9 @@ func TestLocationCompare(t *testing.T) { func TestLocationMarshal(t *testing.T) { testCases := map[string]struct { - loc *Location - exp string + loc *Location + options astJSON.Options + exp string }{ "default json options": { loc: &Location{ @@ -108,10 +109,10 @@ func TestLocationMarshal(t *testing.T) { File: "file", Row: 1, Col: 1, - JSONOptions: astJSON.Options{ - MarshalOptions: astJSON.MarshalOptions{ - IncludeLocationText: true, - }, + }, + options: astJSON.Options{ + MarshalOptions: astJSON.MarshalOptions{ + IncludeLocationText: true, }, }, exp: `{"file":"file","row":1,"col":1,"text":"dGV4dA=="}`, @@ -121,10 +122,10 @@ func TestLocationMarshal(t *testing.T) { File: "file", Row: 1, Col: 1, - JSONOptions: astJSON.Options{ - MarshalOptions: astJSON.MarshalOptions{ - ExcludeLocationFile: true, - }, + }, + options: astJSON.Options{ + MarshalOptions: astJSON.MarshalOptions{ + ExcludeLocationFile: true, }, }, exp: `{"row":1,"col":1}`, @@ -133,6 +134,9 @@ func TestLocationMarshal(t *testing.T) { for id, tc := range testCases { t.Run(id, func(t *testing.T) { + astJSON.SetOptions(tc.options) + defer astJSON.SetOptions(astJSON.Defaults()) + bs, err := json.Marshal(tc.loc) if err != nil { t.Fatal(err) diff --git a/v1/ast/marshal.go b/v1/ast/marshal.go deleted file mode 100644 index dd8dff684c4..00000000000 --- a/v1/ast/marshal.go +++ /dev/null @@ -1,11 +0,0 @@ -package ast - -import ( - astJSON "github.com/open-policy-agent/opa/v1/ast/json" -) - -// customJSON is an interface that can be implemented by AST nodes that -// allows the parser to set options for JSON operations on that node. -type customJSON interface { - setJSONOptions(astJSON.Options) -} diff --git a/v1/ast/marshal_test.go b/v1/ast/marshal_test.go index 18de0877dd1..28c26b7f9af 100644 --- a/v1/ast/marshal_test.go +++ b/v1/ast/marshal_test.go @@ -8,9 +8,14 @@ import ( "github.com/open-policy-agent/opa/v1/util" ) +func resetJSONOptions() { + astJSON.SetOptions(astJSON.Defaults()) +} + func TestGeneric_MarshalWithLocationJSONOptions(t *testing.T) { testCases := map[string]struct { Term *Term + Options astJSON.Options ExpectedJSON string }{ "base case, no location options set": { @@ -24,62 +29,58 @@ func TestGeneric_MarshalWithLocationJSONOptions(t *testing.T) { ExpectedJSON: `{"type":"string","value":"example"}`, }, "location included, location text excluded": { + Options: astJSON.Options{ + MarshalOptions: astJSON.MarshalOptions{ + IncludeLocation: astJSON.NodeToggle{ + Term: true, + }, + IncludeLocationText: false, + }, + }, Term: func() *Term { v, _ := InterfaceToValue("example") return &Term{ Value: v, Location: NewLocation([]byte{}, "example.rego", 1, 2), - jsonOptions: astJSON.Options{ - MarshalOptions: astJSON.MarshalOptions{ - IncludeLocation: astJSON.NodeToggle{ - Term: true, - }, - IncludeLocationText: false, - }, - }, } }(), ExpectedJSON: `{"location":{"file":"example.rego","row":1,"col":2},"type":"string","value":"example"}`, }, "location included, location text also included": { + Options: astJSON.Options{ + MarshalOptions: astJSON.MarshalOptions{ + IncludeLocation: astJSON.NodeToggle{ + Term: true, + }, + IncludeLocationText: true, + }, + }, 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, - }, - }, - ) return t }(), ExpectedJSON: `{"location":{"file":"example.rego","row":1,"col":2,"text":"dGhpbmdz"},"type":"string","value":"example"}`, }, "location included, location text included, file excluded": { + Options: astJSON.Options{ + MarshalOptions: astJSON.MarshalOptions{ + IncludeLocation: astJSON.NodeToggle{ + Term: true, + }, + IncludeLocationText: true, + ExcludeLocationFile: true, + }, + }, 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"}`, @@ -88,6 +89,9 @@ func TestGeneric_MarshalWithLocationJSONOptions(t *testing.T) { for name, data := range testCases { t.Run(name, func(t *testing.T) { + astJSON.SetOptions(data.Options) + t.Cleanup(resetJSONOptions) + bs := util.MustMarshalJSON(data.Term) got := string(bs) exp := data.ExpectedJSON @@ -102,6 +106,7 @@ func TestGeneric_MarshalWithLocationJSONOptions(t *testing.T) { func TestTerm_MarshalJSON(t *testing.T) { testCases := map[string]struct { Term *Term + Options astJSON.Options ExpectedJSON string }{ "base case": { @@ -120,15 +125,15 @@ func TestTerm_MarshalJSON(t *testing.T) { return &Term{ Value: v, Location: NewLocation([]byte{}, "example.rego", 1, 2), - jsonOptions: astJSON.Options{ - MarshalOptions: astJSON.MarshalOptions{ - IncludeLocation: astJSON.NodeToggle{ - Term: false, - }, - }, - }, } }(), + Options: astJSON.Options{ + MarshalOptions: astJSON.MarshalOptions{ + IncludeLocation: astJSON.NodeToggle{ + Term: false, + }, + }, + }, ExpectedJSON: `{"type":"string","value":"example"}`, }, "location included": { @@ -137,21 +142,24 @@ func TestTerm_MarshalJSON(t *testing.T) { return &Term{ Value: v, Location: NewLocation([]byte{}, "example.rego", 1, 2), - jsonOptions: astJSON.Options{ - MarshalOptions: astJSON.MarshalOptions{ - IncludeLocation: astJSON.NodeToggle{ - Term: true, - }, - }, - }, } }(), + Options: astJSON.Options{ + MarshalOptions: astJSON.MarshalOptions{ + IncludeLocation: astJSON.NodeToggle{ + Term: true, + }, + }, + }, ExpectedJSON: `{"location":{"file":"example.rego","row":1,"col":2},"type":"string","value":"example"}`, }, } for name, data := range testCases { t.Run(name, func(t *testing.T) { + astJSON.SetOptions(data.Options) + t.Cleanup(resetJSONOptions) + bs := util.MustMarshalJSON(data.Term) got := string(bs) exp := data.ExpectedJSON @@ -212,6 +220,7 @@ func TestTerm_UnmarshalJSON(t *testing.T) { func TestPackage_MarshalJSON(t *testing.T) { testCases := map[string]struct { Package *Package + Options astJSON.Options ExpectedJSON string }{ "base case": { @@ -224,11 +233,11 @@ func TestPackage_MarshalJSON(t *testing.T) { Package: &Package{ Path: EmptyRef(), Location: NewLocation([]byte{}, "example.rego", 1, 2), - jsonOptions: astJSON.Options{ - MarshalOptions: astJSON.MarshalOptions{ - IncludeLocation: astJSON.NodeToggle{ - Package: false, - }, + }, + Options: astJSON.Options{ + MarshalOptions: astJSON.MarshalOptions{ + IncludeLocation: astJSON.NodeToggle{ + Package: false, }, }, }, @@ -238,11 +247,11 @@ func TestPackage_MarshalJSON(t *testing.T) { Package: &Package{ Path: EmptyRef(), Location: NewLocation([]byte{}, "example.rego", 1, 2), - jsonOptions: astJSON.Options{ - MarshalOptions: astJSON.MarshalOptions{ - IncludeLocation: astJSON.NodeToggle{ - Package: true, - }, + }, + Options: astJSON.Options{ + MarshalOptions: astJSON.MarshalOptions{ + IncludeLocation: astJSON.NodeToggle{ + Package: true, }, }, }, @@ -252,6 +261,9 @@ func TestPackage_MarshalJSON(t *testing.T) { for name, data := range testCases { t.Run(name, func(t *testing.T) { + astJSON.SetOptions(data.Options) + t.Cleanup(resetJSONOptions) + bs := util.MustMarshalJSON(data.Package) got := string(bs) exp := data.ExpectedJSON @@ -268,6 +280,7 @@ func TestPackage_MarshalJSON(t *testing.T) { func TestComment_MarshalJSON(t *testing.T) { testCases := map[string]struct { Comment *Comment + Options astJSON.Options ExpectedJSON string }{ "base case": { @@ -280,11 +293,11 @@ func TestComment_MarshalJSON(t *testing.T) { Comment: &Comment{ Text: []byte("comment"), Location: NewLocation([]byte{}, "example.rego", 1, 2), - jsonOptions: astJSON.Options{ - MarshalOptions: astJSON.MarshalOptions{ - IncludeLocation: astJSON.NodeToggle{ - Comment: false, // ignored - }, + }, + Options: astJSON.Options{ + MarshalOptions: astJSON.MarshalOptions{ + IncludeLocation: astJSON.NodeToggle{ + Comment: false, // ignored }, }, }, @@ -294,11 +307,11 @@ func TestComment_MarshalJSON(t *testing.T) { Comment: &Comment{ Text: []byte("comment"), Location: NewLocation([]byte{}, "example.rego", 1, 2), - jsonOptions: astJSON.Options{ - MarshalOptions: astJSON.MarshalOptions{ - IncludeLocation: astJSON.NodeToggle{ - Comment: true, // ignored - }, + }, + Options: astJSON.Options{ + MarshalOptions: astJSON.MarshalOptions{ + IncludeLocation: astJSON.NodeToggle{ + Comment: true, // ignored }, }, }, @@ -308,6 +321,9 @@ func TestComment_MarshalJSON(t *testing.T) { for name, data := range testCases { t.Run(name, func(t *testing.T) { + astJSON.SetOptions(data.Options) + t.Cleanup(resetJSONOptions) + bs := util.MustMarshalJSON(data.Comment) got := string(bs) exp := data.ExpectedJSON @@ -322,6 +338,7 @@ func TestComment_MarshalJSON(t *testing.T) { func TestImport_MarshalJSON(t *testing.T) { testCases := map[string]struct { Import *Import + Options astJSON.Options ExpectedJSON string }{ "base case": { @@ -345,15 +362,15 @@ func TestImport_MarshalJSON(t *testing.T) { return &Import{ Path: &term, Location: NewLocation([]byte{}, "example.rego", 1, 2), - jsonOptions: astJSON.Options{ - MarshalOptions: astJSON.MarshalOptions{ - IncludeLocation: astJSON.NodeToggle{ - Import: false, - }, - }, - }, } }(), + Options: astJSON.Options{ + MarshalOptions: astJSON.MarshalOptions{ + IncludeLocation: astJSON.NodeToggle{ + Import: false, + }, + }, + }, ExpectedJSON: `{"path":{"type":"string","value":"example"}}`, }, "location included": { @@ -366,21 +383,24 @@ func TestImport_MarshalJSON(t *testing.T) { return &Import{ Path: &term, Location: NewLocation([]byte{}, "example.rego", 1, 2), - jsonOptions: astJSON.Options{ - MarshalOptions: astJSON.MarshalOptions{ - IncludeLocation: astJSON.NodeToggle{ - Import: true, - }, - }, - }, } }(), + Options: astJSON.Options{ + MarshalOptions: astJSON.MarshalOptions{ + IncludeLocation: astJSON.NodeToggle{ + Import: true, + }, + }, + }, ExpectedJSON: `{"location":{"file":"example.rego","row":1,"col":2},"path":{"type":"string","value":"example"}}`, }, } for name, data := range testCases { t.Run(name, func(t *testing.T) { + astJSON.SetOptions(data.Options) + t.Cleanup(resetJSONOptions) + bs := util.MustMarshalJSON(data.Import) got := string(bs) exp := data.ExpectedJSON @@ -410,38 +430,33 @@ func TestRule_MarshalJSON(t *testing.T) { testCases := map[string]struct { Rule *Rule + Options astJSON.Options ExpectedJSON string }{ "base case": { - Rule: rule.Copy(), + Rule: rule, ExpectedJSON: `{"body":[{"index":0,"terms":{"type":"boolean","value":true}}],"head":{"name":"allow","value":{"type":"boolean","value":true},"ref":[{"type":"var","value":"allow"}]}}`, }, "location excluded": { - Rule: func() *Rule { - r := rule.Copy() - r.jsonOptions = astJSON.Options{ - MarshalOptions: astJSON.MarshalOptions{ - IncludeLocation: astJSON.NodeToggle{ - Rule: false, - }, + Rule: rule, + Options: astJSON.Options{ + MarshalOptions: astJSON.MarshalOptions{ + IncludeLocation: astJSON.NodeToggle{ + Rule: false, }, - } - return r - }(), + }, + }, ExpectedJSON: `{"body":[{"index":0,"terms":{"type":"boolean","value":true}}],"head":{"name":"allow","value":{"type":"boolean","value":true},"ref":[{"type":"var","value":"allow"}]}}`, }, "location included": { - Rule: func() *Rule { - r := rule.Copy() - r.jsonOptions = astJSON.Options{ - MarshalOptions: astJSON.MarshalOptions{ - IncludeLocation: astJSON.NodeToggle{ - Rule: true, - }, + Rule: rule, + Options: astJSON.Options{ + MarshalOptions: astJSON.MarshalOptions{ + IncludeLocation: astJSON.NodeToggle{ + Rule: true, }, - } - return r - }(), + }, + }, ExpectedJSON: `{"body":[{"index":0,"terms":{"type":"boolean","value":true}}],"head":{"name":"allow","value":{"type":"boolean","value":true},"ref":[{"type":"var","value":"allow"}]},"location":{"file":"example.rego","row":6,"col":2}}`, }, "annotations included": { @@ -464,6 +479,9 @@ func TestRule_MarshalJSON(t *testing.T) { for name, data := range testCases { t.Run(name, func(t *testing.T) { + astJSON.SetOptions(data.Options) + t.Cleanup(resetJSONOptions) + bs := util.MustMarshalJSON(data.Rule) got := string(bs) exp := data.ExpectedJSON @@ -493,6 +511,7 @@ func TestHead_MarshalJSON(t *testing.T) { testCases := map[string]struct { Head *Head + Options astJSON.Options ExpectedJSON string }{ "base case": { @@ -500,38 +519,34 @@ func TestHead_MarshalJSON(t *testing.T) { ExpectedJSON: `{"name":"allow","value":{"type":"boolean","value":true},"ref":[{"type":"var","value":"allow"}]}`, }, "location excluded": { - Head: func() *Head { - h := head.Copy() - h.jsonOptions = astJSON.Options{ - MarshalOptions: astJSON.MarshalOptions{ - IncludeLocation: astJSON.NodeToggle{ - Head: false, - }, + Head: head, + Options: astJSON.Options{ + MarshalOptions: astJSON.MarshalOptions{ + IncludeLocation: astJSON.NodeToggle{ + Head: false, }, - } - - return h - }(), + }, + }, ExpectedJSON: `{"name":"allow","value":{"type":"boolean","value":true},"ref":[{"type":"var","value":"allow"}]}`, }, "location included": { - Head: func() *Head { - h := head.Copy() - h.jsonOptions = astJSON.Options{ - MarshalOptions: astJSON.MarshalOptions{ - IncludeLocation: astJSON.NodeToggle{ - Head: true, - }, + Head: head, + Options: astJSON.Options{ + MarshalOptions: astJSON.MarshalOptions{ + IncludeLocation: astJSON.NodeToggle{ + Head: true, }, - } - return h - }(), + }, + }, ExpectedJSON: `{"name":"allow","value":{"type":"boolean","value":true},"ref":[{"type":"var","value":"allow"}],"location":{"file":"example.rego","row":6,"col":2}}`, }, } for name, data := range testCases { t.Run(name, func(t *testing.T) { + astJSON.SetOptions(data.Options) + t.Cleanup(resetJSONOptions) + bs := util.MustMarshalJSON(data.Head) got := string(bs) exp := data.ExpectedJSON @@ -552,16 +567,17 @@ ref.head[rule].test contains "value" if { rule := "rule" }` - jsonOptions := &astJSON.Options{ + astJSON.SetOptions(astJSON.Options{ MarshalOptions: astJSON.MarshalOptions{ IncludeLocation: astJSON.NodeToggle{ Head: true, Term: true, }, }, - } + }) + t.Cleanup(resetJSONOptions) - module, err := ParseModuleWithOpts("test.rego", policy, ParserOptions{JSONOptions: jsonOptions}) + module, err := ParseModuleWithOpts("test.rego", policy, ParserOptions{}) if err != nil { t.Fatal(err) } @@ -597,45 +613,42 @@ func TestExpr_MarshalJSON(t *testing.T) { testCases := map[string]struct { Expr *Expr + Options astJSON.Options ExpectedJSON string }{ "base case": { - Expr: expr.Copy(), + Expr: expr, ExpectedJSON: `{"index":0,"terms":{"type":"boolean","value":true}}`, }, "location excluded": { - Expr: func() *Expr { - e := expr.Copy() - e.jsonOptions = astJSON.Options{ - MarshalOptions: astJSON.MarshalOptions{ - IncludeLocation: astJSON.NodeToggle{ - Expr: false, - }, + Expr: expr, + Options: astJSON.Options{ + MarshalOptions: astJSON.MarshalOptions{ + IncludeLocation: astJSON.NodeToggle{ + Expr: false, }, - } - - return e - }(), + }, + }, ExpectedJSON: `{"index":0,"terms":{"type":"boolean","value":true}}`, }, "location included": { - Expr: func() *Expr { - e := expr.Copy() - e.jsonOptions = astJSON.Options{ - MarshalOptions: astJSON.MarshalOptions{ - IncludeLocation: astJSON.NodeToggle{ - Expr: true, - }, + Expr: expr, + Options: astJSON.Options{ + MarshalOptions: astJSON.MarshalOptions{ + IncludeLocation: astJSON.NodeToggle{ + Expr: true, }, - } - return e - }(), + }, + }, ExpectedJSON: `{"index":0,"location":{"file":"example.rego","row":6,"col":13},"terms":{"type":"boolean","value":true}}`, }, } for name, data := range testCases { t.Run(name, func(t *testing.T) { + astJSON.SetOptions(data.Options) + t.Cleanup(resetJSONOptions) + bs := util.MustMarshalJSON(data.Expr) got := string(bs) exp := data.ExpectedJSON @@ -712,6 +725,7 @@ func TestSomeDecl_MarshalJSON(t *testing.T) { testCases := map[string]struct { SomeDecl *SomeDecl + Options astJSON.Options ExpectedJSON string }{ "base case": { @@ -723,17 +737,21 @@ func TestSomeDecl_MarshalJSON(t *testing.T) { }, "location excluded": { SomeDecl: &SomeDecl{ - Symbols: []*Term{term}, - Location: NewLocation([]byte{}, "example.rego", 1, 2), - jsonOptions: astJSON.Options{MarshalOptions: astJSON.MarshalOptions{IncludeLocation: astJSON.NodeToggle{SomeDecl: false}}}, + Symbols: []*Term{term}, + Location: NewLocation([]byte{}, "example.rego", 1, 2), + }, + Options: astJSON.Options{ + MarshalOptions: astJSON.MarshalOptions{IncludeLocation: astJSON.NodeToggle{SomeDecl: false}}, }, ExpectedJSON: `{"symbols":[{"type":"string","value":"example"}]}`, }, "location included": { SomeDecl: &SomeDecl{ - Symbols: []*Term{term}, - Location: NewLocation([]byte{}, "example.rego", 1, 2), - jsonOptions: astJSON.Options{MarshalOptions: astJSON.MarshalOptions{IncludeLocation: astJSON.NodeToggle{SomeDecl: true}}}, + Symbols: []*Term{term}, + Location: NewLocation([]byte{}, "example.rego", 1, 2), + }, + Options: astJSON.Options{ + MarshalOptions: astJSON.MarshalOptions{IncludeLocation: astJSON.NodeToggle{SomeDecl: true}}, }, ExpectedJSON: `{"location":{"file":"example.rego","row":1,"col":2},"symbols":[{"type":"string","value":"example"}]}`, }, @@ -741,6 +759,9 @@ func TestSomeDecl_MarshalJSON(t *testing.T) { for name, data := range testCases { t.Run(name, func(t *testing.T) { + astJSON.SetOptions(data.Options) + t.Cleanup(resetJSONOptions) + bs := util.MustMarshalJSON(data.SomeDecl) got := string(bs) exp := data.ExpectedJSON @@ -776,6 +797,7 @@ allow if { testCases := map[string]struct { Every *Every + Options astJSON.Options ExpectedJSON string }{ "base case": { @@ -783,25 +805,24 @@ allow if { ExpectedJSON: `{"body":[{"index":0,"terms":[{"type":"ref","value":[{"type":"var","value":"equal"}]},{"type":"var","value":"e"},{"type":"number","value":1}]}],"domain":{"type":"array","value":[{"type":"number","value":1},{"type":"number","value":2},{"type":"number","value":3}]},"key":null,"value":{"type":"var","value":"e"}}`, }, "location excluded": { - Every: func() *Every { - e := every.Copy() - e.jsonOptions = astJSON.Options{MarshalOptions: astJSON.MarshalOptions{IncludeLocation: astJSON.NodeToggle{Every: false}}} - return e - }(), + Every: every, + Options: astJSON.Options{ + MarshalOptions: astJSON.MarshalOptions{IncludeLocation: astJSON.NodeToggle{Every: false}}, + }, ExpectedJSON: `{"body":[{"index":0,"terms":[{"type":"ref","value":[{"type":"var","value":"equal"}]},{"type":"var","value":"e"},{"type":"number","value":1}]}],"domain":{"type":"array","value":[{"type":"number","value":1},{"type":"number","value":2},{"type":"number","value":3}]},"key":null,"value":{"type":"var","value":"e"}}`, }, "location included": { - Every: func() *Every { - e := every.Copy() - e.jsonOptions = astJSON.Options{MarshalOptions: astJSON.MarshalOptions{IncludeLocation: astJSON.NodeToggle{Every: true}}} - return e - }(), + Every: every, + Options: astJSON.Options{MarshalOptions: astJSON.MarshalOptions{IncludeLocation: astJSON.NodeToggle{Every: true}}}, ExpectedJSON: `{"body":[{"index":0,"terms":[{"type":"ref","value":[{"type":"var","value":"equal"}]},{"type":"var","value":"e"},{"type":"number","value":1}]}],"domain":{"type":"array","value":[{"type":"number","value":1},{"type":"number","value":2},{"type":"number","value":3}]},"key":null,"location":{"file":"example.rego","row":5,"col":2},"value":{"type":"var","value":"e"}}`, }, } for name, data := range testCases { t.Run(name, func(t *testing.T) { + astJSON.SetOptions(data.Options) + t.Cleanup(resetJSONOptions) + bs := util.MustMarshalJSON(data.Every) got := string(bs) exp := data.ExpectedJSON @@ -833,34 +854,28 @@ b if { with := module.Rules[1].Body[0].With[0] testCases := map[string]struct { - With *With + Options astJSON.Options ExpectedJSON string }{ "base case": { - With: with, ExpectedJSON: `{"target":{"type":"ref","value":[{"type":"var","value":"input"}]},"value":{"type":"number","value":1}}`, }, "location excluded": { - With: func() *With { - w := with.Copy() - w.jsonOptions = astJSON.Options{MarshalOptions: astJSON.MarshalOptions{IncludeLocation: astJSON.NodeToggle{With: false}}} - return w - }(), + Options: astJSON.Options{MarshalOptions: astJSON.MarshalOptions{IncludeLocation: astJSON.NodeToggle{With: false}}}, ExpectedJSON: `{"target":{"type":"ref","value":[{"type":"var","value":"input"}]},"value":{"type":"number","value":1}}`, }, "location included": { - With: func() *With { - w := with.Copy() - w.jsonOptions = astJSON.Options{MarshalOptions: astJSON.MarshalOptions{IncludeLocation: astJSON.NodeToggle{With: true}}} - return w - }(), + Options: astJSON.Options{MarshalOptions: astJSON.MarshalOptions{IncludeLocation: astJSON.NodeToggle{With: true}}}, ExpectedJSON: `{"location":{"file":"example.rego","row":7,"col":4},"target":{"type":"ref","value":[{"type":"var","value":"input"}]},"value":{"type":"number","value":1}}`, }, } for name, data := range testCases { t.Run(name, func(t *testing.T) { - bs := util.MustMarshalJSON(data.With) + astJSON.SetOptions(data.Options) + t.Cleanup(resetJSONOptions) + + bs := util.MustMarshalJSON(with) got := string(bs) exp := data.ExpectedJSON @@ -875,6 +890,7 @@ func TestAnnotations_MarshalJSON(t *testing.T) { testCases := map[string]struct { Annotations *Annotations + Options astJSON.Options ExpectedJSON string }{ "base case": { @@ -902,11 +918,10 @@ func TestAnnotations_MarshalJSON(t *testing.T) { "foo": "bar", }, Location: NewLocation([]byte{}, "example.rego", 1, 4), - - jsonOptions: astJSON.Options{ - MarshalOptions: astJSON.MarshalOptions{ - IncludeLocation: astJSON.NodeToggle{Annotations: false}, - }, + }, + Options: astJSON.Options{ + MarshalOptions: astJSON.MarshalOptions{ + IncludeLocation: astJSON.NodeToggle{Annotations: false}, }, }, ExpectedJSON: `{"custom":{"foo":"bar"},"description":"My desc","entrypoint":true,"organizations":["org1"],"scope":"rule","title":"My rule"}`, @@ -922,11 +937,10 @@ func TestAnnotations_MarshalJSON(t *testing.T) { "foo": "bar", }, Location: NewLocation([]byte{}, "example.rego", 1, 4), - - jsonOptions: astJSON.Options{ - MarshalOptions: astJSON.MarshalOptions{ - IncludeLocation: astJSON.NodeToggle{Annotations: true}, - }, + }, + Options: astJSON.Options{ + MarshalOptions: astJSON.MarshalOptions{ + IncludeLocation: astJSON.NodeToggle{Annotations: true}, }, }, ExpectedJSON: `{"custom":{"foo":"bar"},"description":"My desc","entrypoint":true,"location":{"file":"example.rego","row":1,"col":4},"organizations":["org1"],"scope":"rule","title":"My rule"}`, @@ -935,6 +949,9 @@ func TestAnnotations_MarshalJSON(t *testing.T) { for name, data := range testCases { t.Run(name, func(t *testing.T) { + astJSON.SetOptions(data.Options) + t.Cleanup(resetJSONOptions) + bs := util.MustMarshalJSON(data.Annotations) got := string(bs) exp := data.ExpectedJSON @@ -950,6 +967,7 @@ func TestAnnotationsRef_MarshalJSON(t *testing.T) { testCases := map[string]struct { AnnotationsRef *AnnotationsRef + Options astJSON.Options ExpectedJSON string }{ "base case": { @@ -966,10 +984,10 @@ func TestAnnotationsRef_MarshalJSON(t *testing.T) { Path: []*Term{}, Annotations: &Annotations{}, Location: NewLocation([]byte{}, "example.rego", 1, 4), - jsonOptions: astJSON.Options{ - MarshalOptions: astJSON.MarshalOptions{ - IncludeLocation: astJSON.NodeToggle{AnnotationsRef: false}, - }, + }, + Options: astJSON.Options{ + MarshalOptions: astJSON.MarshalOptions{ + IncludeLocation: astJSON.NodeToggle{AnnotationsRef: false}, }, }, ExpectedJSON: `{"annotations":{"scope":""},"path":[]}`, @@ -979,11 +997,10 @@ func TestAnnotationsRef_MarshalJSON(t *testing.T) { Path: []*Term{}, Annotations: &Annotations{}, Location: NewLocation([]byte{}, "example.rego", 1, 4), - - jsonOptions: astJSON.Options{ - MarshalOptions: astJSON.MarshalOptions{ - IncludeLocation: astJSON.NodeToggle{AnnotationsRef: true}, - }, + }, + Options: astJSON.Options{ + MarshalOptions: astJSON.MarshalOptions{ + IncludeLocation: astJSON.NodeToggle{AnnotationsRef: true}, }, }, ExpectedJSON: `{"annotations":{"scope":""},"location":{"file":"example.rego","row":1,"col":4},"path":[]}`, @@ -992,6 +1009,9 @@ func TestAnnotationsRef_MarshalJSON(t *testing.T) { for name, data := range testCases { t.Run(name, func(t *testing.T) { + astJSON.SetOptions(data.Options) + t.Cleanup(resetJSONOptions) + bs := util.MustMarshalJSON(data.AnnotationsRef) got := string(bs) exp := data.ExpectedJSON @@ -1005,10 +1025,11 @@ func TestAnnotationsRef_MarshalJSON(t *testing.T) { func TestNewAnnotationsRef_JSONOptions(t *testing.T) { tests := []struct { - note string - module string - expected []string - options ParserOptions + note string + module string + expected []string + options ParserOptions + jsonOptions astJSON.Options }{ { note: "all JSON marshaller options set to true", @@ -1058,22 +1079,22 @@ package test p = 1`, options: ParserOptions{ ProcessAnnotation: true, - 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, - }, + }, + 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, }, }, }, @@ -1087,6 +1108,9 @@ p = 1`, for _, tc := range tests { t.Run(tc.note, func(t *testing.T) { + astJSON.SetOptions(tc.jsonOptions) + t.Cleanup(resetJSONOptions) + module := MustParseModuleWithOpts(tc.module, tc.options) if len(tc.expected) != len(module.Annotations) { diff --git a/v1/ast/parser.go b/v1/ast/parser.go index fef95751329..2054141d303 100644 --- a/v1/ast/parser.go +++ b/v1/ast/parser.go @@ -150,7 +150,6 @@ type ParserOptions struct { AllFutureKeywords bool FutureKeywords []string SkipRules bool - JSONOptions *astJSON.Options // RegoVersion is the version of Rego to parse for. RegoVersion RegoVersion unreleasedKeywords bool // TODO(sr): cleanup @@ -237,10 +236,11 @@ func (p *Parser) WithSkipRules(skip bool) *Parser { return p } -// WithJSONOptions sets the Options which will be set on nodes to configure -// their JSON marshaling behavior. -func (p *Parser) WithJSONOptions(jsonOptions *astJSON.Options) *Parser { - p.po.JSONOptions = jsonOptions +// WithJSONOptions sets the JSON options on the parser (now a no-op). +// +// Deprecated: Use SetOptions in the json package instead, where a longer description +// of why this is deprecated also can be found. +func (p *Parser) WithJSONOptions(_ *astJSON.Options) *Parser { return p } @@ -494,19 +494,6 @@ func (p *Parser) Parse() ([]Statement, []*Comment, Errors) { stmts = p.parseAnnotations(stmts) } - if p.po.JSONOptions != nil { - for i := range stmts { - vis := NewGenericVisitor(func(x interface{}) bool { - if x, ok := x.(customJSON); ok { - x.setJSONOptions(*p.po.JSONOptions) - } - return false - }) - - vis.Walk(stmts[i]) - } - } - return stmts, p.s.comments, p.s.errors } @@ -971,9 +958,8 @@ func (p *Parser) parseHead(defaultRule bool) (*Head, bool) { switch x := ref.Value.(type) { case Var: - // Modify the code to add the location to the head ref - // and set the head ref's jsonOptions. - head = VarHead(x, ref.Location, p.po.JSONOptions) + // TODO + head = VarHead(x, ref.Location, nil) case Ref: head = RefHead(x) case Call: diff --git a/v1/ast/parser_ext.go b/v1/ast/parser_ext.go index db1c3caedc1..9712cb611ad 100644 --- a/v1/ast/parser_ext.go +++ b/v1/ast/parser_ext.go @@ -18,7 +18,6 @@ import ( "unicode" "github.com/open-policy-agent/opa/v1/ast/internal/tokens" - astJSON "github.com/open-policy-agent/opa/v1/ast/json" ) // MustParseBody returns a parsed body. @@ -266,8 +265,7 @@ func ParseCompleteDocRuleFromEqExpr(module *Module, lhs, rhs *Term) (*Rule, erro if v, ok := lhs.Value.(Var); ok { // Modify the code to add the location to the head ref - // and set the head ref's jsonOptions. - head = VarHead(v, lhs.Location, &lhs.jsonOptions) + head = VarHead(v, lhs.Location, nil) } else if r, ok := lhs.Value.(Ref); ok { // groundness ? if _, ok := r[0].Value.(Var); !ok { return nil, fmt.Errorf("invalid rule head: %v", r) @@ -281,17 +279,14 @@ func ParseCompleteDocRuleFromEqExpr(module *Module, lhs, rhs *Term) (*Rule, erro } head.Value = rhs head.Location = lhs.Location - head.setJSONOptions(lhs.jsonOptions) body := NewBody(NewExpr(BooleanTerm(true).SetLocation(rhs.Location)).SetLocation(rhs.Location)) - setJSONOptions(body, &rhs.jsonOptions) return &Rule{ Location: lhs.Location, Head: head, Body: body, Module: module, - jsonOptions: lhs.jsonOptions, generatedBody: true, }, nil } @@ -308,18 +303,14 @@ func ParseCompleteDocRuleWithDotsFromTerm(module *Module, term *Term) (*Rule, er head := RefHead(ref, BooleanTerm(true).SetLocation(term.Location)) head.generatedValue = true head.Location = term.Location - head.jsonOptions = term.jsonOptions body := NewBody(NewExpr(BooleanTerm(true).SetLocation(term.Location)).SetLocation(term.Location)) - setJSONOptions(body, &term.jsonOptions) return &Rule{ Location: term.Location, Head: head, Body: body, Module: module, - - jsonOptions: term.jsonOptions, }, nil } @@ -341,17 +332,14 @@ func ParsePartialObjectDocRuleFromEqExpr(module *Module, lhs, rhs *Term) (*Rule, head.Key = ref[1] } head.Location = rhs.Location - head.jsonOptions = rhs.jsonOptions body := NewBody(NewExpr(BooleanTerm(true).SetLocation(rhs.Location)).SetLocation(rhs.Location)) - setJSONOptions(body, &rhs.jsonOptions) rule := &Rule{ - Location: rhs.Location, - Head: head, - Body: body, - Module: module, - jsonOptions: rhs.jsonOptions, + Location: rhs.Location, + Head: head, + Body: body, + Module: module, } return rule, nil @@ -376,22 +364,18 @@ func ParsePartialSetDocRuleFromTerm(module *Module, term *Term) (*Rule, error) { return nil, fmt.Errorf("%vs cannot be used for rule head", ValueName(term.Value)) } // Modify the code to add the location to the head ref - // and set the head ref's jsonOptions. - head = VarHead(v, ref[0].Location, &ref[0].jsonOptions) + head = VarHead(v, ref[0].Location, nil) head.Key = ref[1] } head.Location = term.Location - head.jsonOptions = term.jsonOptions body := NewBody(NewExpr(BooleanTerm(true).SetLocation(term.Location)).SetLocation(term.Location)) - setJSONOptions(body, &term.jsonOptions) rule := &Rule{ - Location: term.Location, - Head: head, - Body: body, - Module: module, - jsonOptions: term.jsonOptions, + Location: term.Location, + Head: head, + Body: body, + Module: module, } return rule, nil @@ -417,17 +401,14 @@ func ParseRuleFromCallEqExpr(module *Module, lhs, rhs *Term) (*Rule, error) { head := RefHead(ref, rhs) head.Location = lhs.Location head.Args = Args(call[1:]) - head.jsonOptions = lhs.jsonOptions body := NewBody(NewExpr(BooleanTerm(true).SetLocation(rhs.Location)).SetLocation(rhs.Location)) - setJSONOptions(body, &rhs.jsonOptions) rule := &Rule{ - Location: lhs.Location, - Head: head, - Body: body, - Module: module, - jsonOptions: lhs.jsonOptions, + Location: lhs.Location, + Head: head, + Body: body, + Module: module, } return rule, nil @@ -449,17 +430,14 @@ func ParseRuleFromCallExpr(module *Module, terms []*Term) (*Rule, error) { head := RefHead(ref, BooleanTerm(true).SetLocation(loc)) head.Location = loc head.Args = terms[1:] - head.jsonOptions = terms[0].jsonOptions body := NewBody(NewExpr(BooleanTerm(true).SetLocation(loc)).SetLocation(loc)) - setJSONOptions(body, &terms[0].jsonOptions) rule := &Rule{ - Location: loc, - Head: head, - Module: module, - Body: body, - jsonOptions: terms[0].jsonOptions, + Location: loc, + Head: head, + Module: module, + Body: body, } return rule, nil } @@ -655,7 +633,6 @@ func ParseStatementsWithOpts(filename, input string, popts ParserOptions) ([]Sta WithAllFutureKeywords(popts.AllFutureKeywords). WithCapabilities(popts.Capabilities). WithSkipRules(popts.SkipRules). - WithJSONOptions(popts.JSONOptions). WithRegoVersion(popts.RegoVersion). withUnreleasedKeywords(popts.unreleasedKeywords) @@ -777,16 +754,6 @@ func setRuleModule(rule *Rule, module *Module) { } } -func setJSONOptions(x interface{}, jsonOptions *astJSON.Options) { - vis := NewGenericVisitor(func(x interface{}) bool { - if x, ok := x.(customJSON); ok { - x.setJSONOptions(*jsonOptions) - } - return false - }) - vis.Walk(x) -} - // ParserErrorDetail holds additional details for parser errors. type ParserErrorDetail struct { Line string `json:"line"` diff --git a/v1/ast/parser_test.go b/v1/ast/parser_test.go index 00d74f9f9c9..37bcc2b5f4c 100644 --- a/v1/ast/parser_test.go +++ b/v1/ast/parser_test.go @@ -14,7 +14,6 @@ import ( "testing" "github.com/open-policy-agent/opa/v1/ast/internal/tokens" - astJSON "github.com/open-policy-agent/opa/v1/ast/json" ) const ( @@ -4096,9 +4095,9 @@ func TestParseMultiValueRuleGeneratedBodyLocationText(t *testing.T) { t.Parallel() mod := `package test - + import rego.v1 - + foo contains "bar" ` @@ -4114,85 +4113,6 @@ func TestParseMultiValueRuleGeneratedBodyLocationText(t *testing.T) { } } -func TestRuleFromBodyJSONOptions(t *testing.T) { - tests := []string{ - `pi = 3.14159`, - `p contains x if { x = 1 }`, - `greeting = "hello"`, - `cores = [{0: 1}, {1: 2}]`, - `wrapper = cores[0][1]`, - `pi = [3, 1, 4, x, y, z]`, - `foo["bar"] = "buz"`, - `foo["9"] = "10"`, - `foo.buz = "bar"`, - `foo.fizz contains "buzz"`, - `bar contains 1`, - `bar contains [{"foo":"baz"}]`, - `bar contains "qux"`, - `input = 1`, - `data = 2`, - `f(1) = 2`, - `f(1)`, - `d1 := 1234`, - } - - parserOpts := ParserOptions{ProcessAnnotation: true, AllFutureKeywords: 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, - }, - }, - } - - for _, tc := range tests { - t.Run(tc, func(t *testing.T) { - testModule := "package a.b.c\n" + tc - assertParseModuleJSONOptions(t, tc, testModule, parserOpts) - }) - } -} - -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 @@ -6134,7 +6054,7 @@ func TestAnnotationsAugmentedError(t *testing.T) { func TestAnnotationsAreParsedAsYamlv1_2(t *testing.T) { policy := `package p - + # METADATA # custom: # string: yes @@ -6537,53 +6457,6 @@ func assertParseModule(t *testing.T, msg string, input string, correct *Module, } -func assertParseModuleJSONOptions(t *testing.T, msg string, input string, opts ...ParserOptions) { - opt := ParserOptions{} - if len(opts) == 1 { - opt = opts[0] - } - m, err := ParseModuleWithOpts("", input, opt) - if err != nil { - t.Errorf("Error on test \"%s\": parse error on %s: %s", msg, input, err) - return - } - - if len(m.Rules) != 1 { - t.Fatalf("Error on test \"%s\": expected 1 rule but got %d", msg, len(m.Rules)) - } - - rule := m.Rules[0] - if rule.Head.jsonOptions != *opt.JSONOptions { - t.Fatalf("Error on test \"%s\": expected rule Head Options\n%v\n, got\n%v", msg, *opt.JSONOptions, rule.Head.jsonOptions) - } - if rule.Body[0].jsonOptions != *opt.JSONOptions { - t.Fatalf("Error on test \"%s\": expected rule Body Options\n%v\n, got\n%v", msg, *opt.JSONOptions, rule.Body[0].jsonOptions) - } - switch terms := rule.Body[0].Terms.(type) { - case []*Term: - for _, term := range terms { - if term.jsonOptions != *opt.JSONOptions { - t.Fatalf("Error on test \"%s\": expected body Term Options\n%v\n, got\n%v", msg, *opt.JSONOptions, term.jsonOptions) - } - } - case *SomeDecl: - if terms.jsonOptions != *opt.JSONOptions { - t.Fatalf("Error on test \"%s\": expected body Term Options\n%v\n, got\n%v", msg, *opt.JSONOptions, terms.jsonOptions) - } - case *Every: - if terms.jsonOptions != *opt.JSONOptions { - t.Fatalf("Error on test \"%s\": expected body Term Options\n%v\n, got\n%v", msg, *opt.JSONOptions, terms.jsonOptions) - } - case *Term: - if terms.jsonOptions != *opt.JSONOptions { - t.Fatalf("Error on test \"%s\": expected body Term Options\n%v\n, got\n%v", msg, *opt.JSONOptions, terms.jsonOptions) - } - } - if rule.jsonOptions != *opt.JSONOptions { - t.Fatalf("Error on test \"%s\": expected rule Options\n%v\n, got\n%v", msg, *opt.JSONOptions, rule.jsonOptions) - } -} - func assertParseModuleError(t *testing.T, msg, input string) { m, err := ParseModule("", input) if err == nil { diff --git a/v1/ast/policy.go b/v1/ast/policy.go index 0e0422a9f63..94dc25244bc 100644 --- a/v1/ast/policy.go +++ b/v1/ast/policy.go @@ -213,8 +213,6 @@ type ( // TODO: these fields have inconsistent JSON keys with other structs in this package. Text []byte Location *Location - - jsonOptions astJSON.Options } // Package represents the namespace of the documents produced @@ -222,8 +220,6 @@ type ( Package struct { Path Ref `json:"path"` Location *Location `json:"location,omitempty"` - - jsonOptions astJSON.Options } // Import represents a dependency on a document outside of the policy @@ -232,8 +228,6 @@ type ( Path *Term `json:"path"` Alias Var `json:"alias,omitempty"` Location *Location `json:"location,omitempty"` - - jsonOptions astJSON.Options } // Rule represents a rule as defined in the language. Rules define the @@ -253,7 +247,6 @@ type ( Module *Module `json:"-"` generatedBody bool - jsonOptions astJSON.Options } // Head represents the head of a rule. @@ -268,7 +261,6 @@ type ( keywords []tokens.Token generatedValue bool - jsonOptions astJSON.Options } // Args represents zero or more arguments to a rule. @@ -287,7 +279,6 @@ type ( Negated bool `json:"negated,omitempty"` Location *Location `json:"location,omitempty"` - jsonOptions astJSON.Options generatedFrom *Expr generates []*Expr } @@ -296,8 +287,6 @@ type ( SomeDecl struct { Symbols []*Term `json:"symbols"` Location *Location `json:"location,omitempty"` - - jsonOptions astJSON.Options } Every struct { @@ -306,8 +295,6 @@ type ( Domain *Term `json:"domain"` Body Body `json:"body"` Location *Location `json:"location,omitempty"` - - jsonOptions astJSON.Options } // With represents a modifier on an expression. @@ -315,8 +302,6 @@ type ( Target *Term `json:"target"` Value *Term `json:"value"` Location *Location `json:"location,omitempty"` - - jsonOptions astJSON.Options } ) @@ -514,15 +499,6 @@ func (c *Comment) Equal(other *Comment) bool { return c.Location.Equal(other.Location) && bytes.Equal(c.Text, other.Text) } -func (c *Comment) setJSONOptions(opts astJSON.Options) { - // Note: this is not used for location since Comments use default JSON marshaling - // behavior with struct field names in JSON. - c.jsonOptions = opts - if c.Location != nil { - c.Location.JSONOptions = opts - } -} - // Compare returns an integer indicating whether pkg is less than, equal to, // or greater than other. func (pkg *Package) Compare(other *Package) int { @@ -567,19 +543,12 @@ func (pkg *Package) String() string { return fmt.Sprintf("package %v", path) } -func (pkg *Package) setJSONOptions(opts astJSON.Options) { - pkg.jsonOptions = opts - if pkg.Location != nil { - pkg.Location.JSONOptions = opts - } -} - func (pkg *Package) MarshalJSON() ([]byte, error) { data := map[string]interface{}{ "path": pkg.Path, } - if pkg.jsonOptions.MarshalOptions.IncludeLocation.Package { + if astJSON.GetOptions().MarshalOptions.IncludeLocation.Package { if pkg.Location != nil { data["location"] = pkg.Location } @@ -680,13 +649,6 @@ func (imp *Import) String() string { return strings.Join(buf, " ") } -func (imp *Import) setJSONOptions(opts astJSON.Options) { - imp.jsonOptions = opts - if imp.Location != nil { - imp.Location.JSONOptions = opts - } -} - func (imp *Import) MarshalJSON() ([]byte, error) { data := map[string]interface{}{ "path": imp.Path, @@ -696,7 +658,7 @@ func (imp *Import) MarshalJSON() ([]byte, error) { data["alias"] = imp.Alias } - if imp.jsonOptions.MarshalOptions.IncludeLocation.Import { + if astJSON.GetOptions().MarshalOptions.IncludeLocation.Import { if imp.Location != nil { data["location"] = imp.Location } @@ -832,13 +794,6 @@ func (rule *Rule) isFunction() bool { return len(rule.Head.Args) > 0 } -func (rule *Rule) setJSONOptions(opts astJSON.Options) { - rule.jsonOptions = opts - if rule.Location != nil { - rule.Location.JSONOptions = opts - } -} - func (rule *Rule) MarshalJSON() ([]byte, error) { data := map[string]interface{}{ "head": rule.Head, @@ -853,7 +808,7 @@ func (rule *Rule) MarshalJSON() ([]byte, error) { data["else"] = rule.Else } - if rule.jsonOptions.MarshalOptions.IncludeLocation.Rule { + if astJSON.GetOptions().MarshalOptions.IncludeLocation.Rule { if rule.Location != nil { data["location"] = rule.Location } @@ -914,14 +869,11 @@ func NewHead(name Var, args ...*Term) *Head { return head } -// VarHead creates a head object, initializes its Name, Location, and Options, -// and returns the new head. -func VarHead(name Var, location *Location, jsonOpts *astJSON.Options) *Head { +// VarHead creates a head object, initializes its Name and Location and returns the new head. +// NOTE: The JSON options argument is no longer used, and kept only for backwards compatibility. +func VarHead(name Var, location *Location, _ *astJSON.Options) *Head { h := NewHead(name) h.Reference[0].Location = location - if jsonOpts != nil { - h.Reference[0].setJSONOptions(*jsonOpts) - } return h } @@ -1084,26 +1036,10 @@ func (head *Head) stringWithOpts(opts toStringOpts) string { return buf.String() } -func (head *Head) setJSONOptions(opts astJSON.Options) { - head.jsonOptions = opts - if head.Location != nil { - head.Location.JSONOptions = opts - } -} - func (head *Head) MarshalJSON() ([]byte, error) { var loc *Location - includeLoc := head.jsonOptions.MarshalOptions.IncludeLocation - if includeLoc.Head { - if head.Location != nil { - loc = head.Location - } - - for _, term := range head.Reference { - if term.Location != nil { - term.jsonOptions.MarshalOptions.IncludeLocation.Term = includeLoc.Term - } - } + if astJSON.GetOptions().MarshalOptions.IncludeLocation.Head && head.Location != nil { + loc = head.Location } // NOTE(sr): we do this to override the rendering of `head.Reference`. @@ -1655,13 +1591,6 @@ func (expr *Expr) String() string { return strings.Join(buf, " ") } -func (expr *Expr) setJSONOptions(opts astJSON.Options) { - expr.jsonOptions = opts - if expr.Location != nil { - expr.Location.JSONOptions = opts - } -} - func (expr *Expr) MarshalJSON() ([]byte, error) { data := map[string]interface{}{ "terms": expr.Terms, @@ -1680,7 +1609,7 @@ func (expr *Expr) MarshalJSON() ([]byte, error) { data["negated"] = true } - if expr.jsonOptions.MarshalOptions.IncludeLocation.Expr { + if astJSON.GetOptions().MarshalOptions.IncludeLocation.Expr { if expr.Location != nil { data["location"] = expr.Location } @@ -1794,19 +1723,12 @@ func (d *SomeDecl) Hash() int { return termSliceHash(d.Symbols) } -func (d *SomeDecl) setJSONOptions(opts astJSON.Options) { - d.jsonOptions = opts - if d.Location != nil { - d.Location.JSONOptions = opts - } -} - func (d *SomeDecl) MarshalJSON() ([]byte, error) { data := map[string]interface{}{ "symbols": d.Symbols, } - if d.jsonOptions.MarshalOptions.IncludeLocation.SomeDecl { + if astJSON.GetOptions().MarshalOptions.IncludeLocation.SomeDecl { if d.Location != nil { data["location"] = d.Location } @@ -1871,13 +1793,6 @@ func (q *Every) KeyValueVars() VarSet { return vis.vars } -func (q *Every) setJSONOptions(opts astJSON.Options) { - q.jsonOptions = opts - if q.Location != nil { - q.Location.JSONOptions = opts - } -} - func (q *Every) MarshalJSON() ([]byte, error) { data := map[string]interface{}{ "key": q.Key, @@ -1886,7 +1801,7 @@ func (q *Every) MarshalJSON() ([]byte, error) { "body": q.Body, } - if q.jsonOptions.MarshalOptions.IncludeLocation.Every { + if astJSON.GetOptions().MarshalOptions.IncludeLocation.Every { if q.Location != nil { data["location"] = q.Location } @@ -1953,20 +1868,13 @@ func (w *With) SetLoc(loc *Location) { w.Location = loc } -func (w *With) setJSONOptions(opts astJSON.Options) { - w.jsonOptions = opts - if w.Location != nil { - w.Location.JSONOptions = opts - } -} - func (w *With) MarshalJSON() ([]byte, error) { data := map[string]interface{}{ "target": w.Target, "value": w.Value, } - if w.jsonOptions.MarshalOptions.IncludeLocation.With { + if astJSON.GetOptions().MarshalOptions.IncludeLocation.With { if w.Location != nil { data["location"] = w.Location } diff --git a/v1/ast/term.go b/v1/ast/term.go index 1350150f1af..9abc29346a2 100644 --- a/v1/ast/term.go +++ b/v1/ast/term.go @@ -294,8 +294,6 @@ func MustInterfaceToValue(x interface{}) Value { type Term struct { Value Value `json:"value"` // the value of the Term as represented in Go Location *Location `json:"location,omitempty"` // the location of the Term in the source - - jsonOptions astJSON.Options } // NewTerm returns a new Term object. @@ -402,13 +400,6 @@ func (term *Term) IsGround() bool { return term.Value.IsGround() } -func (term *Term) setJSONOptions(opts astJSON.Options) { - term.jsonOptions = opts - if term.Location != nil { - term.Location.JSONOptions = opts - } -} - // MarshalJSON returns the JSON encoding of the term. // // Specialized marshalling logic is required to include a type hint for Value. @@ -417,7 +408,8 @@ func (term *Term) MarshalJSON() ([]byte, error) { "type": ValueName(term.Value), "value": term.Value, } - if term.jsonOptions.MarshalOptions.IncludeLocation.Term { + jsonOptions := astJSON.GetOptions().MarshalOptions + if jsonOptions.IncludeLocation.Term { if term.Location != nil { d["location"] = term.Location } diff --git a/v1/bundle/bundle.go b/v1/bundle/bundle.go index be320f6a731..54b28b2653f 100644 --- a/v1/bundle/bundle.go +++ b/v1/bundle/bundle.go @@ -444,7 +444,6 @@ type Reader struct { verificationConfig *VerificationConfig skipVerify bool processAnnotations bool - jsonOptions *astJSON.Options capabilities *ast.Capabilities files map[string]FileInfo // files in the bundle signature payload sizeLimitBytes int64 @@ -517,9 +516,11 @@ func (r *Reader) WithCapabilities(caps *ast.Capabilities) *Reader { return r } -// WithJSONOptions sets the JSONOptions to use when parsing policy files -func (r *Reader) WithJSONOptions(opts *astJSON.Options) *Reader { - r.jsonOptions = opts +// WithJSONOptions sets the JSON options on the parser (now a no-op). +// +// Deprecated: Use SetOptions in the json package instead, where a longer description +// of why this is deprecated also can be found. +func (r *Reader) WithJSONOptions(*astJSON.Options) *Reader { return r } @@ -571,7 +572,6 @@ func (r *Reader) ParserOptions() ast.ParserOptions { return ast.ParserOptions{ ProcessAnnotation: r.processAnnotations, Capabilities: r.capabilities, - JSONOptions: r.jsonOptions, RegoVersion: r.regoVersion, } } diff --git a/v1/loader/loader.go b/v1/loader/loader.go index 563f99efcab..5e2217473ae 100644 --- a/v1/loader/loader.go +++ b/v1/loader/loader.go @@ -100,6 +100,8 @@ type FileLoader interface { WithSkipBundleVerification(bool) FileLoader WithProcessAnnotation(bool) FileLoader WithCapabilities(*ast.Capabilities) FileLoader + // Deprecated: Use SetOptions in the json package instead, where a longer description + // of why this is deprecated also can be found. WithJSONOptions(*astJSON.Options) FileLoader WithRegoVersion(ast.RegoVersion) FileLoader WithFollowSymlinks(bool) FileLoader @@ -177,9 +179,11 @@ func (fl *fileLoader) WithCapabilities(caps *ast.Capabilities) FileLoader { return fl } -// WithJSONOptions sets the JSONOptions for use when parsing files -func (fl *fileLoader) WithJSONOptions(opts *astJSON.Options) FileLoader { - fl.opts.JSONOptions = opts +// WithJSONOptions sets the JSON options on the parser (now a no-op). +// +// Deprecated: Use SetOptions in the json package instead, where a longer description +// of why this is deprecated also can be found. +func (fl *fileLoader) WithJSONOptions(*astJSON.Options) FileLoader { return fl } @@ -269,7 +273,6 @@ func (fl fileLoader) AsBundle(path string) (*bundle.Bundle, error) { WithSkipBundleVerification(fl.skipVerify). WithProcessAnnotations(fl.opts.ProcessAnnotation). WithCapabilities(fl.opts.Capabilities). - WithJSONOptions(fl.opts.JSONOptions). WithFollowSymlinks(fl.followSymlinks). WithRegoVersion(fl.opts.RegoVersion) @@ -757,7 +760,6 @@ func loadBundleFile(path string, bs []byte, m metrics.Metrics, opts ast.ParserOp br := bundle.NewCustomReader(tl). WithRegoVersion(opts.RegoVersion). WithCapabilities(opts.Capabilities). - WithJSONOptions(opts.JSONOptions). WithProcessAnnotations(opts.ProcessAnnotation). WithMetrics(m). WithSkipBundleVerification(true). diff --git a/v1/loader/loader_test.go b/v1/loader/loader_test.go index 7cce0d8c39e..f9245866a47 100644 --- a/v1/loader/loader_test.go +++ b/v1/loader/loader_test.go @@ -1067,14 +1067,20 @@ func TestLoadWithJSONOptions(t *testing.T) { t.Fatalf("Unexpected error: %v", err) } - // load the file with JSON options set to include location data - loaded, err := NewFileLoader().WithFS(fsys).WithJSONOptions(&astJSON.Options{ + astJSON.SetOptions(astJSON.Options{ MarshalOptions: astJSON.MarshalOptions{ IncludeLocation: astJSON.NodeToggle{ Package: true, }, }, - }).All(paths) + }) + + t.Cleanup(func() { + astJSON.SetOptions(astJSON.Defaults()) + }) + + // load the file with JSON options set to include location data + loaded, err := NewFileLoader().WithFS(fsys).All(paths) if err != nil { t.Fatalf("Unexpected error: %v", err) } diff --git a/v1/tester/runner.go b/v1/tester/runner.go index 23350a9e4ad..1ddd60bfe6f 100644 --- a/v1/tester/runner.go +++ b/v1/tester/runner.go @@ -635,7 +635,6 @@ func LoadWithParserOptions(args []string, filter loader.Filter, popts ast.Parser WithRegoVersion(popts.RegoVersion). WithCapabilities(popts.Capabilities). WithProcessAnnotation(popts.ProcessAnnotation). - WithJSONOptions(popts.JSONOptions). Filtered(args, filter) if err != nil { return nil, nil, err @@ -702,7 +701,6 @@ func LoadBundlesWithParserOptions(args []string, filter loader.Filter, popts ast WithRegoVersion(popts.RegoVersion). WithCapabilities(popts.Capabilities). WithProcessAnnotation(popts.ProcessAnnotation). - WithJSONOptions(popts.JSONOptions). WithSkipBundleVerification(true). WithFilter(filter). AsBundle(bundleDir)