v2/pkg/engine/resolve/loader.go (2)

950-999: Entity index removal logic looks correct; clarify numeric segment stringification

The implementation correctly drops the numeric index immediately after _entities and preserves non‑trailing @. Note it also stringifies all subsequent numeric segments (e.g., 1 -> "1"), which matches the tests but should be called out in the function docs for future readers.

Apply this diff to document the behavior:

-// rewriteErrorPaths rewrites the path field for all the values.
+// rewriteErrorPaths rewrites GraphQL error "path" arrays for subgraph errors routed via _entities:
+// - Prefixes with fetchItem.ResponsePathElements (trailing "@" removed).
+// - Drops the numeric index immediately following "_entities".
+// - Converts all subsequent numeric segments to strings (e.g., 1 -> "1").
+// - Skips non-string/non-number segments.

---

990-994: Avoid per-error marshal/parse if this becomes hot; otherwise fine

json.Marshal + astjson.ParseBytesWithoutCache per error allocates. If this path ever gets hot, consider constructing the array via an arena-backed builder (passing in an arena or the resolvable) to avoid extra allocations. Not required for now.

v2/pkg/engine/resolve/loader_test.go (1)

1438-1539: Strong table-driven coverage; consider a couple more edge cases

Good coverage including nested fields, boolean segments, and @ handling. Optionally add:

• Only _entities with no trailing segments (e.g., ["_entities"]).
• _entities followed by an index only (e.g., ["_entities", 2]) to assert the result is exactly the fetch path prefix.

You could also use require.NoError for the parse copy to fail fast.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Learnt from: Noroth
PR: wundergraph/graphql-go-tools#1262
File: v2/pkg/engine/datasource/grpc_datasource/json_builder.go:0-0
Timestamp: 2025-08-08T09:43:07.433Z
Learning: In v2/pkg/engine/datasource/grpc_datasource/json_builder.go, mergeEntities intentionally uses the loop index when calling indexMap.getResultIndex because the index map is type-aware, making per-type counters unnecessary under the current assumptions. Avoid suggesting per-type ordinal counters for this path in future reviews.

v2/pkg/engine/resolve/loader.go (3)

956-963: Avoid unnecessary allocation when building pathPrefix

You can re-slice instead of copy; no mutation occurs.

-	pathPrefix := make([]string, len(fetchItem.ResponsePathElements))
-	copy(pathPrefix, fetchItem.ResponsePathElements)
-	// remove the trailing @ in case we're in an array as it looks weird in the path
-	// errors, like fetches, are attached to objects, not arrays
-	if len(fetchItem.ResponsePathElements) != 0 && fetchItem.ResponsePathElements[len(fetchItem.ResponsePathElements)-1] == "@" {
-		pathPrefix = pathPrefix[:len(pathPrefix)-1]
-	}
+	pathPrefix := fetchItem.ResponsePathElements
+	// remove the trailing @ in case we're in an array as it looks weird in the path
+	// errors, like fetches, are attached to objects, not arrays
+	if n := len(pathPrefix); n > 0 && pathPrefix[n-1] == "@" {
+		pathPrefix = pathPrefix[:n-1]
+	}

---

976-976: Guard the string comparison with a type check

Slightly clearer and avoids converting non-strings to empty string before compare.

-			if unsafebytes.BytesToString(item.GetStringBytes()) == "_entities" {
+			if item.Type() == astjson.TypeString && unsafebytes.BytesToString(item.GetStringBytes()) == "_entities" {

---

982-984: Minor readability: use pathItems[j] instead of pathItems[i+1]

Same logic, less cognitive overhead inside the loop.

-					// If the item after _entities is an index (number), we should ignore it.
-					if j == i+1 && pathItems[i+1].Type() == astjson.TypeNumber {
+					// If the item after _entities is an index (number), we should ignore it.
+					if j == i+1 && pathItems[j].Type() == astjson.TypeNumber {
 						continue
 					}

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Learnt from: Noroth
PR: wundergraph/graphql-go-tools#1262
File: v2/pkg/engine/datasource/grpc_datasource/json_builder.go:0-0
Timestamp: 2025-08-08T09:43:07.433Z
Learning: In v2/pkg/engine/datasource/grpc_datasource/json_builder.go, mergeEntities intentionally uses the loop index when calling indexMap.getResultIndex because the index map is type-aware, making per-type counters unnecessary under the current assumptions. Avoid suggesting per-type ordinal counters for this path in future reviews.

v2/pkg/engine/resolve/loader.go (2)

956-962: Confirm whether to drop the leading response-path element

renderErrorsInvalidInput removes the first element of ResponsePathElements before emitting a path (Lines 712-714). Should the same apply here to avoid including a root segment (e.g., "data")?

If yes, apply:

 pathPrefix := make([]string, len(fetchItem.ResponsePathElements))
 copy(pathPrefix, fetchItem.ResponsePathElements)
 // remove the trailing @ in case we're in an array as it looks weird in the path
 // errors, like fetches, are attached to objects, not arrays
 if len(fetchItem.ResponsePathElements) != 0 && fetchItem.ResponsePathElements[len(fetchItem.ResponsePathElements)-1] == "@" {
   pathPrefix = pathPrefix[:len(pathPrefix)-1]
 }
+// Optional: mirror renderErrorsInvalidInput behavior and drop the first element
+if len(pathPrefix) > 0 {
+  pathPrefix = pathPrefix[1:]
+}

---

996-1002: Avoid JSON round‑trip for building the new path (perf nit)

json.Marshal + ParseBytesWithoutCache allocates on every error. If this sits on a hot path, consider constructing the array via an arena (would require passing it in or keeping this as a method) to avoid the marshal/parse cycle.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro