Skip to content

Commit 5154906

Browse files
authored
fix: skip invalid locations (#1357)
<!-- Important: Before developing new features, please open an issue to discuss your ideas with the maintainers. This ensures project alignment and helps avoid unnecessary work for you. Thank you for your contribution! Please provide a detailed description below and ensure you've met all the requirements. Squashed commit messages must follow the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) standard to facilitate changelog generation. Please ensure your PR title follows the Conventional Commits specification, using the appropriate type (e.g., feat, fix, docs) and scope. Examples of good PR titles: - 💥feat!: change implementation in an non-backward compatible way - ✨feat(auth): add support for OAuth2 login - 🐞fix(router): add support for custom metrics - 📚docs(README): update installation instructions - 🧹chore(deps): bump dependencies to latest versions --> This PR does the following * If a location in the locations array have invalid values (i.e. :- every slice element's col and line are 0 or less), we will remove the location entry. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved error-location handling: invalid or incomplete location entries are filtered out; the locations field is removed when empty or when omission is requested, while preserving valid entries. * **Tests** * Added comprehensive tests covering omission-flag behavior, removal vs. preservation cases, non-array/empty objects, mixed valid/invalid locations, and large datasets. * **Chores** * Internal cleanup and small refactors to error-field handling. <sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub> <!-- end of auto-generated comment: release notes by coderabbit.ai --> ## Checklist - [ ] I have discussed my proposed changes in an issue and have received approval to proceed. - [ ] I have followed the coding standards of the project. - [ ] Tests or benchmarks have been added or updated. <!-- Please add any additional information or context regarding your changes here. -->
1 parent 2faaad3 commit 5154906

File tree

4 files changed

+608
-23
lines changed

4 files changed

+608
-23
lines changed

v2/pkg/engine/resolve/const.go

Lines changed: 5 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,10 @@ package resolve
22

33
import "errors"
44

5+
const (
6+
locationsField = "locations"
7+
)
8+
59
var (
610
lBrace = []byte("{")
711
rBrace = []byte("}")
@@ -16,7 +20,7 @@ var (
1620
literalFalse = []byte("false")
1721
literalErrors = []byte("errors")
1822
literalMessage = []byte("message")
19-
literalLocations = []byte("locations")
23+
literalLocations = []byte(locationsField)
2024
literalPath = []byte("path")
2125
literalUnderscoreEntities = []byte("_entities")
2226
literalExtensions = []byte("extensions")
@@ -35,19 +39,3 @@ var (
3539
errHeaderPathInvalid = errors.New("invalid header path: header variables must be of this format: .request.header.{{ key }} ")
3640
ErrUnableToResolve = errors.New("unable to resolve operation")
3741
)
38-
39-
var (
40-
errorPaths = [][]string{
41-
{"message"},
42-
{"locations"},
43-
{"path"},
44-
{"extensions"},
45-
}
46-
)
47-
48-
const (
49-
errorsMessagePathIndex = 0
50-
errorsLocationsPathIndex = 1
51-
errorsPathPathIndex = 2
52-
errorsExtensionsPathIndex = 3
53-
)

v2/pkg/engine/resolve/loader.go

Lines changed: 31 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -936,12 +936,38 @@ func (l *Loader) optionallyOmitErrorFields(values []*astjson.Value) {
936936

937937
// optionallyOmitErrorLocations removes the "locations" object from all values.
938938
func (l *Loader) optionallyOmitErrorLocations(values []*astjson.Value) {
939-
if !l.omitSubgraphErrorLocations {
940-
return
941-
}
939+
arena := astjson.Arena{}
940+
942941
for _, value := range values {
943-
if value.Exists("locations") {
944-
value.Del("locations")
942+
// If the flag is set, delete all locations
943+
if !value.Exists(locationsField) || l.omitSubgraphErrorLocations {
944+
value.Del(locationsField)
945+
continue
946+
}
947+
948+
// Create a new array via astjson we can append to the valid types
949+
validLocations := arena.NewArray()
950+
validIndex := 0
951+
952+
// GetArray will return nil if not an array which will not be ranged over
953+
allLocations := value.Get(locationsField)
954+
for _, loc := range allLocations.GetArray() {
955+
line := loc.Get("line")
956+
column := loc.Get("column")
957+
958+
// Keep location only if both line and column are > 0 (spec says 0 is invalid)
959+
// In case it is not an int, 0 will be returned which is invalid anyway
960+
if line.GetInt() > 0 && column.GetInt() > 0 {
961+
validLocations.SetArrayItem(validIndex, loc)
962+
validIndex++
963+
}
964+
}
965+
966+
// If all locations were invalid, delete the locations field
967+
if len(validLocations.GetArray()) > 0 {
968+
value.Set(locationsField, validLocations)
969+
} else {
970+
value.Del(locationsField)
945971
}
946972
}
947973
}

0 commit comments

Comments
 (0)