Skip to content

fix: concurrent access for internal proto map#1369

Closed
Noroth wants to merge 21 commits intomasterfrom
ludwig/fix-fetch-access
Closed

fix: concurrent access for internal proto map#1369
Noroth wants to merge 21 commits intomasterfrom
ludwig/fix-fetch-access

Conversation

@Noroth
Copy link
Copy Markdown
Contributor

@Noroth Noroth commented Jan 26, 2026

Summary by CodeRabbit

  • New Features

    • Added new category fields with conditional resolution support to the schema.
    • Enhanced field resolver capabilities for complex nested and recursive resolution patterns.
  • Tests

    • Expanded test coverage for advanced field resolver scenarios including nested, optional, and aliased resolvers.

✏️ Tip: You can customize this high-level summary in your review settings.

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.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Jan 26, 2026

Caution

Review failed

The pull request is closed.

📝 Walkthrough

Walkthrough

This PR introduces ID-based tracking for gRPC RPC calls across the execution planning and data fetching pipeline. It adds an ID field to ServiceCall and RPCCall, implements protobuf cloning on ServiceCall output, refactors the DependencyGraph to use synchronized access with fetch items keyed by call ID, and overhauls the execution plan visitors to properly correlate calls with field resolvers. Test infrastructure is extended with new category resolver scenarios.

Changes

Cohort / File(s) Summary
Core Execution Plan & Compiler
v2/pkg/engine/datasource/grpc_datasource/compiler.go, v2/pkg/engine/datasource/grpc_datasource/execution_plan.go
Added ID field to ServiceCall and RPCCall for call identification. Added CloneOutput() method to ServiceCall for protobuf message cloning. Expanded RPCField with ResolvePath handling. Removed AppendTypeNameField helper. Added internal resolverField tracking with id, contextPath, and list nesting support.
Dependency Graph & Fetch
v2/pkg/engine/datasource/grpc_datasource/fetch.go
Replaced ServiceCall field in FetchItem with generic Output (protoreflect.Message). Added sync.RWMutex to DependencyGraph for thread-safe access. Changed initialization to map calls by call.ID instead of array index. Updated SetFetchData signature to accept outputMessage under write lock. Modified Fetch() to acquire read lock and TopologicalSortResolve to retrieve items via g.Fetch().
Execution Plan Visitors
v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go, v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go
Replaced manual call indexing with global callIndex counter. Consolidated planning state with operationFieldRefs slice and fieldResolverAncestors stack. Reworked EnterSelectionSet/LeaveSelectionSet to manage responseMessageAncestors. Enhanced enterFieldResolver to construct unique field paths and assign IDs. Added federation-specific resolver field scaffolding for entity lookups.
Data Source & JSON Building
v2/pkg/engine/datasource/grpc_datasource/grpc_datasource.go, v2/pkg/engine/datasource/grpc_datasource/json_builder.go
Removed plan field from DataSource struct. Updated Load() to call SetFetchData(serviceCall.ID, serviceCall.CloneOutput()) after gRPC invocation. Added guard in jsonBuilder.mergeWithPath to return early on empty resolved values.
Test Data & Protobuf Schema
v2/pkg/engine/datasource/grpc_datasource/execution_plan_test.go, v2/pkg/engine/datasource/grpc_datasource/execution_plan_federation_test.go, v2/pkg/engine/datasource/grpc_datasource/execution_plan_field_resolvers_test.go, v2/pkg/engine/datasource/grpc_datasource/fetch_test.go, v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.go
Added ID field to RPCCall entries in test fixtures. Expanded execution_plan_field_resolvers_test.go with 1600+ lines of new test cases for category resolvers (nested, optional, aliased, recursive). Added extensive test cases in grpc_datasource_test.go with t.Parallel() for field resolver scenarios.
Mock Service & Protobuf Definitions
v2/pkg/grpctest/mockservice_resolve.go, v2/pkg/grpctest/product.proto
Added two new RPC handlers: ResolveCategoryChildCategories and ResolveCategoryOptionalCategories with logic for emitting child/optional categories. Added corresponding protobuf messages and RPC service methods. Renamed field in ResolveCategoryMetricsNormalizedScoreContext from metricType to metric_type.
Test Configuration & GraphQL Schema
v2/pkg/grpctest/mapping.go, v2/pkg/grpctest/mapping_test_helper.go, v2/pkg/grpctest/testdata/products.graphqls
Added field mappings for childCategories and optionalCategories under category resolvers. Extended ResolveRPCs.Category with new resolver entries. Added GraphQL field definitions with @connect__fieldResolver directives.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant Visitor as Execution<br/>Plan Visitor
    participant Compiler
    participant DS as Data Source
    participant Graph as Dependency<br/>Graph
    participant GRPC as gRPC<br/>Service

    Client->>Visitor: Build Execution Plan
    Visitor->>Compiler: Compile Calls & Fetches
    Compiler->>Compiler: Assign ID to each Call
    Visitor->>Compiler: Store Node ID in ServiceCall.ID
    Compiler-->>Visitor: RPCExecutionPlan
    Visitor->>Graph: NewDependencyGraph(plan)
    Graph->>Graph: Initialize nodes/fetches<br/>keyed by call.ID
    
    Note over DS,GRPC: Fetch Phase
    DS->>GRPC: Execute RPC Call
    GRPC-->>DS: Response (protobuf message)
    DS->>DS: cloneOutput()<br/>(deep clone via proto.Clone)
    DS->>Graph: SetFetchData(call.ID, clonedOutput)
    Graph->>Graph: Lock (write)<br/>FetchItem[call.ID].Output = clonedOutput
    
    Note over Graph,Client: Resolution Phase
    Graph->>Graph: TopologicalSortResolve()
    Graph->>Graph: Lock (read)<br/>Fetch(callIndex)
    Graph-->>Graph: FetchItem with Output
    Graph->>Client: Resolved Dependencies
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

The changes span multiple interconnected components with significant structural modifications: introduction of global call indexing and ID-based correlation across visitors, replacement of field types in public structs (FetchItem.ServiceCall → Output), addition of mutex-protected synchronization, refactored planning state with new tracking mechanisms (callIndex, fieldResolverAncestors, operationFieldRefs), and 1600+ lines of heterogeneous test additions combining new test cases with extensive mock service extensions and protobuf schema changes.

Possibly related PRs

✨ Finishing touches
  • 📝 Generate docstrings

Comment @coderabbitai help to get the list of available commands and usage tips.

@Noroth Noroth closed this Jan 26, 2026
@Noroth Noroth deleted the ludwig/fix-fetch-access branch January 26, 2026 11:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant