From 4405d325e6c7bc88c47a28b6729e163d803c5096 Mon Sep 17 00:00:00 2001 From: Milinda Dias Date: Thu, 18 Dec 2025 18:32:50 +0530 Subject: [PATCH 1/8] fix: skip invalid locations --- v2/pkg/engine/resolve/errors.go | 7 +++++++ v2/pkg/engine/resolve/loader.go | 34 +++++++++++++++++++++++++++++---- 2 files changed, 37 insertions(+), 4 deletions(-) diff --git a/v2/pkg/engine/resolve/errors.go b/v2/pkg/engine/resolve/errors.go index 93b43f8c01..81edd05e47 100644 --- a/v2/pkg/engine/resolve/errors.go +++ b/v2/pkg/engine/resolve/errors.go @@ -19,6 +19,13 @@ type Location struct { Column uint32 `json:"column"` } +// ValidationLocation We define a separate type for validation to avoid confusion with the Location type in GraphQLError +// where the attributes adhere to the spec +type ValidationLocation struct { + Line int32 `json:"line"` + Column int32 `json:"column"` +} + type SubgraphError struct { DataSourceInfo DataSourceInfo Path string diff --git a/v2/pkg/engine/resolve/loader.go b/v2/pkg/engine/resolve/loader.go index 263f997a7e..e8c9c97ef8 100644 --- a/v2/pkg/engine/resolve/loader.go +++ b/v2/pkg/engine/resolve/loader.go @@ -936,11 +936,37 @@ 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 - } for _, value := range values { - if value.Exists("locations") { + if !value.Exists("locations") { + continue + } + + // If the flag is set, delete all locations + if l.omitSubgraphErrorLocations { + value.Del("locations") + continue + } + + // Parse locations into typed structure for validation + locationsJSON := value.Get("locations").MarshalTo(nil) + var parsedLocations []ValidationLocation + if err := json.Unmarshal(locationsJSON, &parsedLocations); err != nil { + // If we can't parse, skip validation, since it will be handled in the current flow + continue + } + + // Check if all locations are invalid (line <= 0 or column <= 0) + allInvalid := true + for _, loc := range parsedLocations { + if loc.Line > 0 || loc.Column > 0 { + allInvalid = false + break + } + } + + // Only delete locations field if ALL locations are invalid + // Otherwise keep locations as is, to error out later + if allInvalid { value.Del("locations") } } From 631437a51bc68f828d8db16228c0ef9354fdf268 Mon Sep 17 00:00:00 2001 From: Milinda Dias Date: Thu, 18 Dec 2025 19:57:37 +0530 Subject: [PATCH 2/8] fix: current changes --- v2/pkg/engine/resolve/errors.go | 7 - v2/pkg/engine/resolve/loader.go | 56 +++-- v2/pkg/engine/resolve/loader_test.go | 350 +++++++++++++++++++++++++++ 3 files changed, 387 insertions(+), 26 deletions(-) diff --git a/v2/pkg/engine/resolve/errors.go b/v2/pkg/engine/resolve/errors.go index 81edd05e47..93b43f8c01 100644 --- a/v2/pkg/engine/resolve/errors.go +++ b/v2/pkg/engine/resolve/errors.go @@ -19,13 +19,6 @@ type Location struct { Column uint32 `json:"column"` } -// ValidationLocation We define a separate type for validation to avoid confusion with the Location type in GraphQLError -// where the attributes adhere to the spec -type ValidationLocation struct { - Line int32 `json:"line"` - Column int32 `json:"column"` -} - type SubgraphError struct { DataSourceInfo DataSourceInfo Path string diff --git a/v2/pkg/engine/resolve/loader.go b/v2/pkg/engine/resolve/loader.go index e8c9c97ef8..e0774a7909 100644 --- a/v2/pkg/engine/resolve/loader.go +++ b/v2/pkg/engine/resolve/loader.go @@ -937,37 +937,55 @@ func (l *Loader) optionallyOmitErrorFields(values []*astjson.Value) { // optionallyOmitErrorLocations removes the "locations" object from all values. func (l *Loader) optionallyOmitErrorLocations(values []*astjson.Value) { for _, value := range values { - if !value.Exists("locations") { - continue - } - // If the flag is set, delete all locations - if l.omitSubgraphErrorLocations { + if !value.Exists("locations") || l.omitSubgraphErrorLocations { value.Del("locations") continue } - // Parse locations into typed structure for validation - locationsJSON := value.Get("locations").MarshalTo(nil) - var parsedLocations []ValidationLocation - if err := json.Unmarshal(locationsJSON, &parsedLocations); err != nil { - // If we can't parse, skip validation, since it will be handled in the current flow + // Filter out invalid location entries (line <= 0 or column <= 0) + locations := value.Get("locations") + if locations.Type() != astjson.TypeArray { continue } - // Check if all locations are invalid (line <= 0 or column <= 0) - allInvalid := true - for _, loc := range parsedLocations { - if loc.Line > 0 || loc.Column > 0 { - allInvalid = false - break + locationsArray := locations.GetArray() + validLocations := make([]*astjson.Value, 0, len(locationsArray)) + + for _, loc := range locationsArray { + line := loc.Get("line") + column := loc.Get("column") + + // Skip invalid locations: nil values or values <= 0 + if line == nil || column == nil { + continue + } + + lineInt := line.GetInt() + columnInt := column.GetInt() + + // Keep location only if both line and column are > 0 + if lineInt > 0 && columnInt > 0 { + validLocations = append(validLocations, loc) } } - // Only delete locations field if ALL locations are invalid - // Otherwise keep locations as is, to error out later - if allInvalid { + // If all locations were invalid, delete the locations field + if len(validLocations) == 0 { value.Del("locations") + } else if len(validLocations) < len(locationsArray) { + // Some locations were invalid - rebuild the array with only valid ones + newLocations := astjson.MustParseBytes([]byte(`[]`)) + + start := time.Now() + for i, validLoc := range validLocations { + newLocations.SetArrayItem(i, validLoc) + } + elapsed := time.Since(start) + + fmt.Println(elapsed) + + value.Set("locations", newLocations) } } } diff --git a/v2/pkg/engine/resolve/loader_test.go b/v2/pkg/engine/resolve/loader_test.go index b19aaafec5..e72700f489 100644 --- a/v2/pkg/engine/resolve/loader_test.go +++ b/v2/pkg/engine/resolve/loader_test.go @@ -1527,3 +1527,353 @@ 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: "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 - unchanged", + omitSubgraphErrorLocations: false, + inputJSON: `[ + { + "message": "Field error", + "locations": "invalid", + "path": ["field"] + } + ]`, + expectedJSON: `[ + { + "message": "Field error", + "locations": "invalid", + "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"] + } + ]`, + }, + } + + 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)) + }) + } +} From 5b65f9c9d8412c24b3348308ca702672dcb6b06f Mon Sep 17 00:00:00 2001 From: Milinda Dias Date: Thu, 18 Dec 2025 20:23:18 +0530 Subject: [PATCH 3/8] fix: updates --- v2/pkg/engine/resolve/loader.go | 35 ++--- v2/pkg/engine/resolve/loader_test.go | 222 +++++++++++++++++++++++++++ 2 files changed, 235 insertions(+), 22 deletions(-) diff --git a/v2/pkg/engine/resolve/loader.go b/v2/pkg/engine/resolve/loader.go index e0774a7909..ad91644bc1 100644 --- a/v2/pkg/engine/resolve/loader.go +++ b/v2/pkg/engine/resolve/loader.go @@ -943,49 +943,40 @@ func (l *Loader) optionallyOmitErrorLocations(values []*astjson.Value) { continue } - // Filter out invalid location entries (line <= 0 or column <= 0) locations := value.Get("locations") if locations.Type() != astjson.TypeArray { continue } locationsArray := locations.GetArray() - validLocations := make([]*astjson.Value, 0, len(locationsArray)) + locationsClone := slices.Clone(locationsArray) - for _, loc := range locationsArray { + deletedEntries := 0 + locationsArrayLength := len(locationsArray) + + for i, loc := range locationsClone { line := loc.Get("line") column := loc.Get("column") // Skip invalid locations: nil values or values <= 0 if line == nil || column == nil { + locations.Del(strconv.Itoa(i - deletedEntries)) + deletedEntries++ continue } - lineInt := line.GetInt() - columnInt := column.GetInt() - // Keep location only if both line and column are > 0 - if lineInt > 0 && columnInt > 0 { - validLocations = append(validLocations, loc) + // In case it is not an int, 0 will be returned which is invalid anyway + isValid := line.GetInt() > 0 && column.GetInt() > 0 + if !isValid { + locations.Del(strconv.Itoa(i - deletedEntries)) + deletedEntries++ } } // If all locations were invalid, delete the locations field - if len(validLocations) == 0 { + if locationsArrayLength == deletedEntries { value.Del("locations") - } else if len(validLocations) < len(locationsArray) { - // Some locations were invalid - rebuild the array with only valid ones - newLocations := astjson.MustParseBytes([]byte(`[]`)) - - start := time.Now() - for i, validLoc := range validLocations { - newLocations.SetArrayItem(i, validLoc) - } - elapsed := time.Since(start) - - fmt.Println(elapsed) - - value.Set("locations", newLocations) } } } diff --git a/v2/pkg/engine/resolve/loader_test.go b/v2/pkg/engine/resolve/loader_test.go index e72700f489..2bbf56be35 100644 --- a/v2/pkg/engine/resolve/loader_test.go +++ b/v2/pkg/engine/resolve/loader_test.go @@ -1568,6 +1568,80 @@ func TestLoader_OptionallyOmitErrorLocations(t *testing.T) { } ]`, }, + { + 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, @@ -1852,6 +1926,154 @@ func TestLoader_OptionallyOmitErrorLocations(t *testing.T) { } ]`, }, + { + 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 { From 02c6203f00d9e12546ae9f2b4290ae9b4c1cd8ba Mon Sep 17 00:00:00 2001 From: Milinda Dias Date: Thu, 18 Dec 2025 20:25:04 +0530 Subject: [PATCH 4/8] fix: comments --- v2/pkg/engine/resolve/loader.go | 1 + 1 file changed, 1 insertion(+) diff --git a/v2/pkg/engine/resolve/loader.go b/v2/pkg/engine/resolve/loader.go index ad91644bc1..bd4ddd8657 100644 --- a/v2/pkg/engine/resolve/loader.go +++ b/v2/pkg/engine/resolve/loader.go @@ -954,6 +954,7 @@ func (l *Loader) optionallyOmitErrorLocations(values []*astjson.Value) { deletedEntries := 0 locationsArrayLength := len(locationsArray) + // We loop on a clone since we delete elements inline for i, loc := range locationsClone { line := loc.Get("line") column := loc.Get("column") From 4ccfd56ba3073efa074aee27dc5fcbe455ac2bfe Mon Sep 17 00:00:00 2001 From: Milinda Dias Date: Thu, 18 Dec 2025 20:40:21 +0530 Subject: [PATCH 5/8] fix: const added --- v2/pkg/engine/resolve/const.go | 22 +++++----------------- v2/pkg/engine/resolve/loader.go | 8 ++++---- v2/pkg/engine/resolve/resolve.go | 2 +- 3 files changed, 10 insertions(+), 22 deletions(-) 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 bd4ddd8657..bfbbe32404 100644 --- a/v2/pkg/engine/resolve/loader.go +++ b/v2/pkg/engine/resolve/loader.go @@ -938,12 +938,12 @@ func (l *Loader) optionallyOmitErrorFields(values []*astjson.Value) { func (l *Loader) optionallyOmitErrorLocations(values []*astjson.Value) { for _, value := range values { // If the flag is set, delete all locations - if !value.Exists("locations") || l.omitSubgraphErrorLocations { - value.Del("locations") + if !value.Exists(locationsField) || l.omitSubgraphErrorLocations { + value.Del(locationsField) continue } - locations := value.Get("locations") + locations := value.Get(locationsField) if locations.Type() != astjson.TypeArray { continue } @@ -977,7 +977,7 @@ func (l *Loader) optionallyOmitErrorLocations(values []*astjson.Value) { // If all locations were invalid, delete the locations field if locationsArrayLength == deletedEntries { - value.Del("locations") + value.Del(locationsField) } } } 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 { From baa8e095fb7013337047f6318e81693e4be4f01e Mon Sep 17 00:00:00 2001 From: Milinda Dias Date: Thu, 18 Dec 2025 21:35:03 +0530 Subject: [PATCH 6/8] fix: review comments --- v2/pkg/engine/resolve/loader.go | 15 +++------------ 1 file changed, 3 insertions(+), 12 deletions(-) diff --git a/v2/pkg/engine/resolve/loader.go b/v2/pkg/engine/resolve/loader.go index bfbbe32404..f4e0217b5b 100644 --- a/v2/pkg/engine/resolve/loader.go +++ b/v2/pkg/engine/resolve/loader.go @@ -944,28 +944,19 @@ func (l *Loader) optionallyOmitErrorLocations(values []*astjson.Value) { } locations := value.Get(locationsField) - if locations.Type() != astjson.TypeArray { + locationsArray := locations.GetArray() + if len(locationsArray) == 0 { continue } - locationsArray := locations.GetArray() locationsClone := slices.Clone(locationsArray) - deletedEntries := 0 - locationsArrayLength := len(locationsArray) // We loop on a clone since we delete elements inline for i, loc := range locationsClone { line := loc.Get("line") column := loc.Get("column") - // Skip invalid locations: nil values or values <= 0 - if line == nil || column == nil { - locations.Del(strconv.Itoa(i - deletedEntries)) - deletedEntries++ - continue - } - // Keep location only if both line and column are > 0 // In case it is not an int, 0 will be returned which is invalid anyway isValid := line.GetInt() > 0 && column.GetInt() > 0 @@ -976,7 +967,7 @@ func (l *Loader) optionallyOmitErrorLocations(values []*astjson.Value) { } // If all locations were invalid, delete the locations field - if locationsArrayLength == deletedEntries { + if len(locations.GetArray()) == 0 { value.Del(locationsField) } } From 977208d25e2c1f48c473f244207cba2dacdfb0a3 Mon Sep 17 00:00:00 2001 From: Milinda Dias Date: Thu, 18 Dec 2025 22:22:54 +0530 Subject: [PATCH 7/8] fix: updates --- v2/pkg/engine/resolve/loader.go | 32 +++++++++++++--------------- v2/pkg/engine/resolve/loader_test.go | 3 +-- 2 files changed, 16 insertions(+), 19 deletions(-) diff --git a/v2/pkg/engine/resolve/loader.go b/v2/pkg/engine/resolve/loader.go index f4e0217b5b..732300d9d2 100644 --- a/v2/pkg/engine/resolve/loader.go +++ b/v2/pkg/engine/resolve/loader.go @@ -943,31 +943,29 @@ func (l *Loader) optionallyOmitErrorLocations(values []*astjson.Value) { continue } - locations := value.Get(locationsField) - locationsArray := locations.GetArray() - if len(locationsArray) == 0 { - continue - } - - locationsClone := slices.Clone(locationsArray) - deletedEntries := 0 - - // We loop on a clone since we delete elements inline - for i, loc := range locationsClone { + // Create a new array via astjson we can append to the valid types + arena := astjson.Arena{} + 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 + // 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 - isValid := line.GetInt() > 0 && column.GetInt() > 0 - if !isValid { - locations.Del(strconv.Itoa(i - deletedEntries)) - deletedEntries++ + if line.GetInt() > 0 && column.GetInt() > 0 { + validLocations.SetArrayItem(validIndex, loc) + validIndex++ } } // If all locations were invalid, delete the locations field - if len(locations.GetArray()) == 0 { + 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 2bbf56be35..999ae87c98 100644 --- a/v2/pkg/engine/resolve/loader_test.go +++ b/v2/pkg/engine/resolve/loader_test.go @@ -1843,7 +1843,7 @@ func TestLoader_OptionallyOmitErrorLocations(t *testing.T) { ]`, }, { - name: "locations is not an array - unchanged", + name: "locations is not an array - removes locations field", omitSubgraphErrorLocations: false, inputJSON: `[ { @@ -1855,7 +1855,6 @@ func TestLoader_OptionallyOmitErrorLocations(t *testing.T) { expectedJSON: `[ { "message": "Field error", - "locations": "invalid", "path": ["field"] } ]`, From 1354921df0c042a5781e5cb8c0c1d742ad0a9c30 Mon Sep 17 00:00:00 2001 From: Milinda Dias Date: Fri, 19 Dec 2025 16:18:08 +0530 Subject: [PATCH 8/8] fix: arenas --- v2/pkg/engine/resolve/loader.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/v2/pkg/engine/resolve/loader.go b/v2/pkg/engine/resolve/loader.go index 732300d9d2..7d32962665 100644 --- a/v2/pkg/engine/resolve/loader.go +++ b/v2/pkg/engine/resolve/loader.go @@ -936,6 +936,8 @@ func (l *Loader) optionallyOmitErrorFields(values []*astjson.Value) { // optionallyOmitErrorLocations removes the "locations" object from all values. func (l *Loader) optionallyOmitErrorLocations(values []*astjson.Value) { + arena := astjson.Arena{} + for _, value := range values { // If the flag is set, delete all locations if !value.Exists(locationsField) || l.omitSubgraphErrorLocations { @@ -944,7 +946,6 @@ func (l *Loader) optionallyOmitErrorLocations(values []*astjson.Value) { } // Create a new array via astjson we can append to the valid types - arena := astjson.Arena{} validLocations := arena.NewArray() validIndex := 0