diff --git a/v2/pkg/engine/resolve/const.go b/v2/pkg/engine/resolve/const.go index 8a259494ec..92ad42ab7c 100644 --- a/v2/pkg/engine/resolve/const.go +++ b/v2/pkg/engine/resolve/const.go @@ -2,6 +2,10 @@ package resolve import "errors" +const ( + locationsField = "locations" +) + var ( lBrace = []byte("{") rBrace = []byte("}") @@ -16,7 +20,7 @@ var ( literalFalse = []byte("false") literalErrors = []byte("errors") literalMessage = []byte("message") - literalLocations = []byte("locations") + literalLocations = []byte(locationsField) literalPath = []byte("path") literalUnderscoreEntities = []byte("_entities") literalExtensions = []byte("extensions") @@ -35,19 +39,3 @@ var ( errHeaderPathInvalid = errors.New("invalid header path: header variables must be of this format: .request.header.{{ key }} ") ErrUnableToResolve = errors.New("unable to resolve operation") ) - -var ( - errorPaths = [][]string{ - {"message"}, - {"locations"}, - {"path"}, - {"extensions"}, - } -) - -const ( - errorsMessagePathIndex = 0 - errorsLocationsPathIndex = 1 - errorsPathPathIndex = 2 - errorsExtensionsPathIndex = 3 -) diff --git a/v2/pkg/engine/resolve/loader.go b/v2/pkg/engine/resolve/loader.go index 263f997a7e..7d32962665 100644 --- a/v2/pkg/engine/resolve/loader.go +++ b/v2/pkg/engine/resolve/loader.go @@ -936,12 +936,38 @@ func (l *Loader) optionallyOmitErrorFields(values []*astjson.Value) { // optionallyOmitErrorLocations removes the "locations" object from all values. func (l *Loader) optionallyOmitErrorLocations(values []*astjson.Value) { - if !l.omitSubgraphErrorLocations { - return - } + arena := astjson.Arena{} + for _, value := range values { - if value.Exists("locations") { - value.Del("locations") + // If the flag is set, delete all locations + if !value.Exists(locationsField) || l.omitSubgraphErrorLocations { + value.Del(locationsField) + continue + } + + // Create a new array via astjson we can append to the valid types + validLocations := arena.NewArray() + validIndex := 0 + + // GetArray will return nil if not an array which will not be ranged over + allLocations := value.Get(locationsField) + for _, loc := range allLocations.GetArray() { + line := loc.Get("line") + column := loc.Get("column") + + // Keep location only if both line and column are > 0 (spec says 0 is invalid) + // In case it is not an int, 0 will be returned which is invalid anyway + if line.GetInt() > 0 && column.GetInt() > 0 { + validLocations.SetArrayItem(validIndex, loc) + validIndex++ + } + } + + // If all locations were invalid, delete the locations field + if len(validLocations.GetArray()) > 0 { + value.Set(locationsField, validLocations) + } else { + value.Del(locationsField) } } } diff --git a/v2/pkg/engine/resolve/loader_test.go b/v2/pkg/engine/resolve/loader_test.go index b19aaafec5..999ae87c98 100644 --- a/v2/pkg/engine/resolve/loader_test.go +++ b/v2/pkg/engine/resolve/loader_test.go @@ -1527,3 +1527,574 @@ func TestRewriteErrorPaths(t *testing.T) { }) } } + +func TestLoader_OptionallyOmitErrorLocations(t *testing.T) { + tests := []struct { + name string + omitSubgraphErrorLocations bool + inputJSON string + expectedJSON string + }{ + { + name: "omit flag is true - removes all locations", + omitSubgraphErrorLocations: true, + inputJSON: `[ + { + "message": "Field error", + "locations": [{"line": 1, "column": 5}], + "path": ["field"] + } + ]`, + expectedJSON: `[ + { + "message": "Field error", + "path": ["field"] + } + ]`, + }, + { + name: "no locations field - unchanged", + omitSubgraphErrorLocations: false, + inputJSON: `[ + { + "message": "Field error", + "path": ["field"] + } + ]`, + expectedJSON: `[ + { + "message": "Field error", + "path": ["field"] + } + ]`, + }, + { + name: "no locations field with omit flag true - calls Del safely", + omitSubgraphErrorLocations: true, + inputJSON: `[ + { + "message": "Field error", + "path": ["field"] + } + ]`, + expectedJSON: `[ + { + "message": "Field error", + "path": ["field"] + } + ]`, + }, + { + name: "empty object with no locations - safe to call Del", + omitSubgraphErrorLocations: false, + inputJSON: `[ + { + "message": "Error" + } + ]`, + expectedJSON: `[ + { + "message": "Error" + } + ]`, + }, + { + name: "empty object with omit flag - safe to call Del", + omitSubgraphErrorLocations: true, + inputJSON: `[ + { + "message": "Error" + } + ]`, + expectedJSON: `[ + { + "message": "Error" + } + ]`, + }, + { + name: "multiple errors without locations field", + omitSubgraphErrorLocations: false, + inputJSON: `[ + { + "message": "Error 1", + "path": ["field1"] + }, + { + "message": "Error 2", + "extensions": {"code": "SOME_ERROR"} + }, + { + "message": "Error 3" + } + ]`, + expectedJSON: `[ + { + "message": "Error 1", + "path": ["field1"] + }, + { + "message": "Error 2", + "extensions": {"code": "SOME_ERROR"} + }, + { + "message": "Error 3" + } + ]`, + }, + { + name: "all valid locations - unchanged", + omitSubgraphErrorLocations: false, + inputJSON: `[ + { + "message": "Field error", + "locations": [ + {"line": 1, "column": 5}, + {"line": 2, "column": 10} + ], + "path": ["field"] + } + ]`, + expectedJSON: `[ + { + "message": "Field error", + "locations": [ + {"line": 1, "column": 5}, + {"line": 2, "column": 10} + ], + "path": ["field"] + } + ]`, + }, + { + name: "all locations invalid (line <= 0) - removes locations field", + omitSubgraphErrorLocations: false, + inputJSON: `[ + { + "message": "Field error", + "locations": [ + {"line": 0, "column": 5}, + {"line": 1, "column": -2} + ], + "path": ["field"] + } + ]`, + expectedJSON: `[ + { + "message": "Field error", + "path": ["field"] + } + ]`, + }, + { + name: "all locations invalid (column <= 0) - removes locations field", + omitSubgraphErrorLocations: false, + inputJSON: `[ + { + "message": "Field error", + "locations": [ + {"line": 1, "column": 0}, + {"line": 2, "column": -1} + ], + "path": ["field"] + } + ]`, + expectedJSON: `[ + { + "message": "Field error", + "path": ["field"] + } + ]`, + }, + { + name: "mixed valid and invalid locations - keeps only valid", + omitSubgraphErrorLocations: false, + inputJSON: `[ + { + "message": "Field error", + "locations": [ + {"line": 1, "column": 5}, + {"line": 0, "column": 10}, + {"line": 3, "column": -2}, + {"line": 4, "column": 15} + ], + "path": ["field"] + } + ]`, + expectedJSON: `[ + { + "message": "Field error", + "locations": [ + {"line": 1, "column": 5}, + {"line": 4, "column": 15} + ], + "path": ["field"] + } + ]`, + }, + { + name: "location with missing line field - removes that location", + omitSubgraphErrorLocations: false, + inputJSON: `[ + { + "message": "Field error", + "locations": [ + {"line": 1, "column": 5}, + {"column": 10} + ], + "path": ["field"] + } + ]`, + expectedJSON: `[ + { + "message": "Field error", + "locations": [ + {"line": 1, "column": 5} + ], + "path": ["field"] + } + ]`, + }, + { + name: "location with missing column field - removes that location", + omitSubgraphErrorLocations: false, + inputJSON: `[ + { + "message": "Field error", + "locations": [ + {"line": 1, "column": 5}, + {"line": 2} + ], + "path": ["field"] + } + ]`, + expectedJSON: `[ + { + "message": "Field error", + "locations": [ + {"line": 1, "column": 5} + ], + "path": ["field"] + } + ]`, + }, + { + name: "all locations missing fields - removes locations field", + omitSubgraphErrorLocations: false, + inputJSON: `[ + { + "message": "Field error", + "locations": [ + {"line": 1}, + {"column": 5} + ], + "path": ["field"] + } + ]`, + expectedJSON: `[ + { + "message": "Field error", + "path": ["field"] + } + ]`, + }, + { + name: "multiple errors with different location scenarios", + omitSubgraphErrorLocations: false, + inputJSON: `[ + { + "message": "Error 1", + "locations": [{"line": 1, "column": 5}], + "path": ["field1"] + }, + { + "message": "Error 2", + "locations": [ + {"line": 0, "column": 0} + ], + "path": ["field2"] + }, + { + "message": "Error 3", + "locations": [ + {"line": 3, "column": 10}, + {"line": -1, "column": 5} + ], + "path": ["field3"] + } + ]`, + expectedJSON: `[ + { + "message": "Error 1", + "locations": [{"line": 1, "column": 5}], + "path": ["field1"] + }, + { + "message": "Error 2", + "path": ["field2"] + }, + { + "message": "Error 3", + "locations": [ + {"line": 3, "column": 10} + ], + "path": ["field3"] + } + ]`, + }, + { + name: "locations is not an array - removes locations field", + omitSubgraphErrorLocations: false, + inputJSON: `[ + { + "message": "Field error", + "locations": "invalid", + "path": ["field"] + } + ]`, + expectedJSON: `[ + { + "message": "Field error", + "path": ["field"] + } + ]`, + }, + { + name: "location with string line value - removes that location", + omitSubgraphErrorLocations: false, + inputJSON: `[ + { + "message": "Field error", + "locations": [ + {"line": 1, "column": 5}, + {"line": "invalid", "column": 10} + ], + "path": ["field"] + } + ]`, + expectedJSON: `[ + { + "message": "Field error", + "locations": [ + {"line": 1, "column": 5} + ], + "path": ["field"] + } + ]`, + }, + { + name: "location with string column value - removes that location", + omitSubgraphErrorLocations: false, + inputJSON: `[ + { + "message": "Field error", + "locations": [ + {"line": 1, "column": 5}, + {"line": 2, "column": "invalid"} + ], + "path": ["field"] + } + ]`, + expectedJSON: `[ + { + "message": "Field error", + "locations": [ + {"line": 1, "column": 5} + ], + "path": ["field"] + } + ]`, + }, + { + name: "all locations with string values - removes locations field", + omitSubgraphErrorLocations: false, + inputJSON: `[ + { + "message": "Field error", + "locations": [ + {"line": "invalid", "column": 5}, + {"line": 2, "column": "invalid"} + ], + "path": ["field"] + } + ]`, + expectedJSON: `[ + { + "message": "Field error", + "path": ["field"] + } + ]`, + }, + { + name: "large dataset - alternating valid and invalid locations", + omitSubgraphErrorLocations: false, + inputJSON: `[ + { + "message": "Complex error", + "locations": [ + {"line": 1, "column": 5}, + {"line": 0, "column": 10}, + {"line": 3, "column": 15}, + {"line": -1, "column": 20}, + {"line": 5, "column": 25}, + {"line": 6, "column": 0}, + {"line": 7, "column": 30}, + {"line": 8, "column": -5}, + {"line": 9, "column": 35} + ], + "path": ["field"] + } + ]`, + expectedJSON: `[ + { + "message": "Complex error", + "locations": [ + {"line": 1, "column": 5}, + {"line": 3, "column": 15}, + {"line": 5, "column": 25}, + {"line": 7, "column": 30}, + {"line": 9, "column": 35} + ], + "path": ["field"] + } + ]`, + }, + { + name: "large dataset - consecutive invalid entries at start and end", + omitSubgraphErrorLocations: false, + inputJSON: `[ + { + "message": "Edge case error", + "locations": [ + {"line": 0, "column": 1}, + {"line": -1, "column": 2}, + {"line": 0, "column": 0}, + {"line": 4, "column": 10}, + {"line": 5, "column": 20}, + {"line": 6, "column": 30}, + {"line": 7, "column": 40}, + {"column": 50}, + {"line": 9}, + {"line": -5, "column": -10} + ], + "path": ["field"] + } + ]`, + expectedJSON: `[ + { + "message": "Edge case error", + "locations": [ + {"line": 4, "column": 10}, + {"line": 5, "column": 20}, + {"line": 6, "column": 30}, + {"line": 7, "column": 40} + ], + "path": ["field"] + } + ]`, + }, + { + name: "large dataset - mixed types and values across multiple errors", + omitSubgraphErrorLocations: false, + inputJSON: `[ + { + "message": "Error 1", + "locations": [ + {"line": 1, "column": 1}, + {"line": 2, "column": 0}, + {"line": 3, "column": 3}, + {"line": "invalid", "column": 4}, + {"line": 5, "column": 5} + ], + "path": ["field1"] + }, + { + "message": "Error 2", + "locations": [ + {"line": 10, "column": 10}, + {"line": 0, "column": 20}, + {"line": 30, "column": 30}, + {"line": 40, "column": "bad"}, + {"line": 50, "column": 50}, + {"line": -1, "column": 60} + ], + "path": ["field2"] + }, + { + "message": "Error 3", + "locations": [ + {"column": 100}, + {"line": 200}, + {"line": 0, "column": 0} + ], + "path": ["field3"] + }, + { + "message": "Error 4", + "locations": [ + {"line": 100, "column": 100}, + {"line": 200, "column": 200}, + {"line": 300, "column": 300} + ], + "path": ["field4"] + } + ]`, + expectedJSON: `[ + { + "message": "Error 1", + "locations": [ + {"line": 1, "column": 1}, + {"line": 3, "column": 3}, + {"line": 5, "column": 5} + ], + "path": ["field1"] + }, + { + "message": "Error 2", + "locations": [ + {"line": 10, "column": 10}, + {"line": 30, "column": 30}, + {"line": 50, "column": 50} + ], + "path": ["field2"] + }, + { + "message": "Error 3", + "path": ["field3"] + }, + { + "message": "Error 4", + "locations": [ + {"line": 100, "column": 100}, + {"line": 200, "column": 200}, + {"line": 300, "column": 300} + ], + "path": ["field4"] + } + ]`, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + loader := &Loader{ + omitSubgraphErrorLocations: tt.omitSubgraphErrorLocations, + } + + // Parse input JSON into astjson values + inputValue, err := astjson.ParseBytesWithoutCache([]byte(tt.inputJSON)) + assert.NoError(t, err) + + values := inputValue.GetArray() + + // Call the function + loader.optionallyOmitErrorLocations(values) + + // Marshal back to JSON for comparison + actualJSON := inputValue.MarshalTo(nil) + + // Compare with expected + assert.JSONEq(t, tt.expectedJSON, string(actualJSON)) + }) + } +} diff --git a/v2/pkg/engine/resolve/resolve.go b/v2/pkg/engine/resolve/resolve.go index 093538e553..ee3538c32f 100644 --- a/v2/pkg/engine/resolve/resolve.go +++ b/v2/pkg/engine/resolve/resolve.go @@ -201,7 +201,7 @@ func New(ctx context.Context, options ResolverOptions) *Resolver { } if !options.OmitSubgraphErrorLocations { - allowedErrorFields["locations"] = struct{}{} + allowedErrorFields[locationsField] = struct{}{} } for _, field := range options.AllowedSubgraphErrorFields {