Conversation
SkArchon
requested review from
Noroth,
StarpTech,
devsergiy,
jensneuse and
ysmolski
as code owners
WalkthroughValidate and selectively retain per-error Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
v2/pkg/engine/resolve/loader.go (1)
958-965: Comment is misleading: should say "AND" not "or".The comment says locations are invalid if "line <= 0 or column <= 0", but the actual logic (line 961) considers a location valid if
loc.Line > 0 || loc.Column > 0. By De Morgan's law, a location is only invalid when bothline <= 0 AND column <= 0.🔎 Apply this diff to fix the comment:
- // Check if all locations are invalid (line <= 0 or column <= 0) + // Check if all locations are invalid (line <= 0 AND column <= 0)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
v2/pkg/engine/resolve/errors.go(1 hunks)v2/pkg/engine/resolve/loader.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-09T15:30:57.980Z
Learnt from: SkArchon
Repo: wundergraph/graphql-go-tools PR: 1351
File: v2/pkg/engine/resolve/context.go:160-162
Timestamp: 2025-12-09T15:30:57.980Z
Learning: Ensure all Context instances in v2/pkg/engine/resolve are created with NewContext() instead of Context{} literals. The codebase avoids defensive nil checks for Context.subgraphErrors to enforce correct usage and fail fast on misuse. Do not add defensive nil checks for map fields in Context that are initialized by NewContext().
Applied to files:
v2/pkg/engine/resolve/loader.gov2/pkg/engine/resolve/errors.go
🧬 Code graph analysis (1)
v2/pkg/engine/resolve/loader.go (1)
v2/pkg/engine/resolve/errors.go (1)
ValidationLocation(24-27)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build and test (go 1.25 / windows-latest)
- GitHub Check: Build and test (go 1.25 / ubuntu-latest)
- GitHub Check: Build and test (go 1.25 / windows-latest)
- GitHub Check: Build and test (go 1.25 / ubuntu-latest)
🔇 Additional comments (2)
v2/pkg/engine/resolve/errors.go (1)
22-27: LGTM!Good design choice using
int32for the validation type. This allows proper detection of negative values from upstream responses, whereas usinguint32(like the existingLocationtype) would cause negative values to wrap around to large positive numbers, incorrectly passing validation.v2/pkg/engine/resolve/loader.go (1)
938-973: LGTM! Clean implementation of conditional location omission.The two-path control flow is clear:
- Unconditional deletion when the omit flag is set
- Content-based deletion when all locations are invalid
The graceful handling of parse errors (lines 953-956) by skipping validation and letting downstream processing handle malformed data is appropriate.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
v2/pkg/engine/resolve/loader.go (1)
938-982: Consider extracting "locations" as a constant.The string
"locations"is repeated 6 times throughout this function (lines 941, 942, 946, 963, 972, 979). Extracting it as a package-level constant would improve maintainability and reduce the risk of typos.Example refactor
At the package level, add:
const ( // ... existing constants errorLocationFieldName = "locations" )Then replace all occurrences of
"locations"in this function witherrorLocationFieldName.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
v2/pkg/engine/resolve/loader.go(1 hunks)v2/pkg/engine/resolve/loader_test.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-09T15:30:57.980Z
Learnt from: SkArchon
Repo: wundergraph/graphql-go-tools PR: 1351
File: v2/pkg/engine/resolve/context.go:160-162
Timestamp: 2025-12-09T15:30:57.980Z
Learning: Ensure all Context instances in v2/pkg/engine/resolve are created with NewContext() instead of Context{} literals. The codebase avoids defensive nil checks for Context.subgraphErrors to enforce correct usage and fail fast on misuse. Do not add defensive nil checks for map fields in Context that are initialized by NewContext().
Applied to files:
v2/pkg/engine/resolve/loader.gov2/pkg/engine/resolve/loader_test.go
🧬 Code graph analysis (2)
v2/pkg/engine/resolve/loader.go (2)
v2/pkg/ast/ast_type.go (1)
Type(21-29)v2/pkg/engine/jsonschema/schema.go (1)
TypeArray(12-12)
v2/pkg/engine/resolve/loader_test.go (1)
v2/pkg/engine/resolve/loader.go (1)
Loader(157-183)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build and test (go 1.25 / windows-latest)
- GitHub Check: Build and test (go 1.25 / ubuntu-latest)
- GitHub Check: Build and test (go 1.25 / ubuntu-latest)
🔇 Additional comments (3)
v2/pkg/engine/resolve/loader_test.go (1)
1531-2101: Excellent test coverage!The test suite comprehensively validates the new per-entry location validation logic across 21 scenarios, including:
- Flag-based omission
- Missing/invalid fields (nil, zero, negative values)
- Mixed valid and invalid entries
- Type mismatches (string values for line/column)
- Edge cases (empty arrays, non-array locations, multiple errors)
- Large datasets with various patterns
The test structure is clear and the expected outcomes align well with the PR objectives.
v2/pkg/engine/resolve/loader.go (2)
940-944: Early exit logic is clean and efficient.The condition correctly handles two cases:
- When the omit flag is set, all locations are removed
- When no locations field exists, deletion is a safe no-op
This avoids unnecessary validation work when locations should be removed anyway.
957-975: Per-entry validation logic is correct.The implementation properly:
- Checks for nil line/column fields
- Validates both line and column are positive integers
- Uses
GetInt()which returns 0 for non-integers, correctly treating them as invalid- Adjusts indices correctly during deletion (
i - deletedEntries)The approach of cloning the array to iterate while deleting from the original is sound and prevents index-related bugs.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
v2/pkg/engine/resolve/loader.go (2)
941-941: Simplify the condition per past review feedback.The condition checks if
locationsdoesn't exist before callingDel, butvalue.Del("locations")is a no-op if the field doesn't exist. You can simplify this as suggested in the past review:if !value.Exists("locations") || l.omitSubgraphErrorLocations {can become:
if l.omitSubgraphErrorLocations || !value.Exists("locations") {Or even simpler, merge with line 942:
🔎 Apply this diff to simplify the condition:
- // If the flag is set, delete all locations - if !value.Exists("locations") || l.omitSubgraphErrorLocations { + // If the flag is set or locations don't exist, delete and continue + if l.omitSubgraphErrorLocations || !value.Exists("locations") { value.Del("locations") continue }Based on past review comments, this simplification was suggested.
941-981: Extract repeated string literal to a constant.The string
"locations"is repeated multiple times throughout the function (lines 941, 942, 946, 964, 973, 980). Per past review feedback, this should be extracted to a constant for better maintainability.🔎 Suggested refactor:
At the package level (around line 32-36 where other constants are defined), add:
const ( // existing constants... errorLocationsField = "locations" )Then update the function to use this constant throughout.
Based on past review comments, extracting this string to a constant was recommended.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
v2/pkg/engine/resolve/loader.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-09T15:30:57.980Z
Learnt from: SkArchon
Repo: wundergraph/graphql-go-tools PR: 1351
File: v2/pkg/engine/resolve/context.go:160-162
Timestamp: 2025-12-09T15:30:57.980Z
Learning: Ensure all Context instances in v2/pkg/engine/resolve are created with NewContext() instead of Context{} literals. The codebase avoids defensive nil checks for Context.subgraphErrors to enforce correct usage and fail fast on misuse. Do not add defensive nil checks for map fields in Context that are initialized by NewContext().
Applied to files:
v2/pkg/engine/resolve/loader.go
🔇 Additional comments (2)
v2/pkg/engine/resolve/loader.go (2)
958-976: The in-place deletion logic is correct.The validation and deletion logic correctly handles:
- Invalid entries (nil or non-positive line/column values)
- Index adjustment via
deletedEntriestracking to account for shifting elements- Non-integer values treated as invalid (GetInt() returns 0)
The approach of cloning the array before iteration prevents modification-while-iterating issues.
978-981: Correctly deletes the locations field when all entries are invalid.The logic properly tracks whether all location entries were removed and deletes the
locationsfield accordingly, which aligns with the PR objectives.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
v2/pkg/engine/resolve/loader.go (1)
951-956: Consider building a filtered array instead of clone+delete for better performance.The current approach clones the entire array and then deletes invalid entries in-place with index adjustments. An alternative approach would be to iterate once and build a new array containing only valid entries, then replace the locations field. This could be more efficient, especially for arrays with many invalid entries.
Example alternative approach
locationsArray := locations.GetArray() validLocations := make([]*astjson.Value, 0, len(locationsArray)) for _, loc := range locationsArray { line := loc.Get("line") column := loc.Get("column") // Keep location only if both exist and are > 0 if line != nil && column != nil && line.GetInt() > 0 && column.GetInt() > 0 { validLocations = append(validLocations, loc) } } // If no valid locations remain, delete the field if len(validLocations) == 0 { value.Del(locationsField) } else if len(validLocations) < len(locationsArray) { // Rebuild the array with only valid entries // This would require astjson API support for replacing array contents }Note: This assumes astjson supports rebuilding arrays efficiently. If not, the current approach is reasonable.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
v2/pkg/engine/resolve/const.go(2 hunks)v2/pkg/engine/resolve/loader.go(1 hunks)v2/pkg/engine/resolve/resolve.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-09T15:30:57.980Z
Learnt from: SkArchon
Repo: wundergraph/graphql-go-tools PR: 1351
File: v2/pkg/engine/resolve/context.go:160-162
Timestamp: 2025-12-09T15:30:57.980Z
Learning: Ensure all Context instances in v2/pkg/engine/resolve are created with NewContext() instead of Context{} literals. The codebase avoids defensive nil checks for Context.subgraphErrors to enforce correct usage and fail fast on misuse. Do not add defensive nil checks for map fields in Context that are initialized by NewContext().
Applied to files:
v2/pkg/engine/resolve/loader.gov2/pkg/engine/resolve/resolve.gov2/pkg/engine/resolve/const.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build and test (go 1.25 / ubuntu-latest)
- GitHub Check: Build and test (go 1.25 / windows-latest)
- GitHub Check: Build and test (go 1.25 / ubuntu-latest)
- GitHub Check: Build and test (go 1.25 / windows-latest)
🔇 Additional comments (5)
v2/pkg/engine/resolve/const.go (1)
5-7: LGTM! Good refactoring to centralize the field name.Extracting the "locations" string into a constant improves maintainability and reduces duplication across the codebase.
Also applies to: 23-23
v2/pkg/engine/resolve/resolve.go (1)
204-204: LGTM! Consistent use of the centralized constant.The replacement of the literal string with
locationsFieldis correct and improves code consistency.v2/pkg/engine/resolve/loader.go (3)
940-944: LGTM! Early exit optimization is correct.The logic properly handles both the omit flag and missing locations field cases before proceeding with validation.
978-980: LGTM! Cleanup logic correctly removes the field when all entries are invalid.The comparison ensures that if all location entries were removed, the entire locations field is deleted as intended by the PR objectives.
958-976: The deletion index adjustment logic is mathematically sound and already has comprehensive test coverage. The code correctly accounts for previous deletions usingi - deletedEntries, and existing tests cover all edge cases including consecutive invalid entries, mixed valid/invalid locations, and various nil/invalid value scenarios. No changes needed.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
v2/pkg/engine/resolve/loader.go (1)
946-967: Consider simplifying the deletion logic for better maintainability.The current implementation with
i - deletedEntriesis functionally correct but difficult to reason about. Past reviewers flagged this pattern as "arcane magic." Consider these alternatives:Alternative 1: Build a new array with valid entries:
locations := value.Get(locationsField) locationsArray := locations.GetArray() if len(locationsArray) == 0 { continue } validLocations := make([]*astjson.Value, 0, len(locationsArray)) for _, loc := range locationsArray { line := loc.Get("line") column := loc.Get("column") if line.GetInt() > 0 && column.GetInt() > 0 { validLocations = append(validLocations, loc) } } if len(validLocations) == 0 { value.Del(locationsField) } else if len(validLocations) < len(locationsArray) { // Reconstruct array with valid entries only newLocations := astjson.MustParseBytes([]byte(`[]`)) for i, validLoc := range validLocations { newLocations.SetArrayItem(i, validLoc) } value.Set(locationsField, newLocations) }Alternative 2: Delete from end to beginning (avoids index shifting):
for i := len(locationsArray) - 1; i >= 0; i-- { loc := locationsArray[i] line := loc.Get("line") column := loc.Get("column") if line.GetInt() <= 0 && column.GetInt() <= 0 { locations.Del(strconv.Itoa(i)) } }Also, the comment on line 955 is slightly misleading—you loop on the clone but delete from the original.
As per coding guidelines and past review comments, simpler deletion logic improves code maintainability.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
v2/pkg/engine/resolve/loader.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-09T15:30:57.980Z
Learnt from: SkArchon
Repo: wundergraph/graphql-go-tools PR: 1351
File: v2/pkg/engine/resolve/context.go:160-162
Timestamp: 2025-12-09T15:30:57.980Z
Learning: Ensure all Context instances in v2/pkg/engine/resolve are created with NewContext() instead of Context{} literals. The codebase avoids defensive nil checks for Context.subgraphErrors to enforce correct usage and fail fast on misuse. Do not add defensive nil checks for map fields in Context that are initialized by NewContext().
Applied to files:
v2/pkg/engine/resolve/loader.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build and test (go 1.25 / windows-latest)
- GitHub Check: Build and test (go 1.25 / ubuntu-latest)
- GitHub Check: Build and test (go 1.25 / ubuntu-latest)
- GitHub Check: Build and test (go 1.25 / windows-latest)
🔇 Additional comments (2)
v2/pkg/engine/resolve/loader.go (2)
940-944: LGTM! Good use of early return and constant.The logic correctly handles both the omit flag and missing locations field. Using the
locationsFieldconstant improves maintainability. Based on learnings, this aligns with the constant-based key handling introduced in related files.
969-972: LGTM! Proper cleanup of empty locations.Correctly removes the locations field when all entries are invalid, which aligns with the PR objectives.
🤖 I have created a release *beep* *boop* --- ## [2.0.0-rc.242](v2.0.0-rc.241...v2.0.0-rc.242) (2025-12-19) ### Bug Fixes * skip invalid locations ([#1357](#1357)) ([5154906](5154906)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Fixed handling of invalid locations <sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub> <!-- end of auto-generated comment: release notes by coderabbit.ai -->
4 participants
This PR does the following
Summary by CodeRabbit
Bug Fixes
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.
Checklist