Conversation
## Walkthrough
This update introduces comprehensive support for GraphQL union and interface types in the gRPC datasource engine, including enhancements to execution planning, marshaling, and testing. The changes add new proto messages, service methods, and mapping definitions to support unions and oneof fields, update the execution plan logic and visitor to handle composite types, and provide extensive new tests for interface and union scenarios in both queries and mutations.
## Changes
| Files/Paths | Change Summary |
|--------------------------------------------------------------------------------------------|---------------|
| v2/pkg/engine/datasource/grpc_datasource/execution_plan.go | Added `OneOfType` enum and related logic for distinguishing interface/union oneof fields; updated `RPCMessage` struct to use `OneOfType`, `FieldSelectionSet`, and `MemberTypes`; added `RPCFieldSelectionSet` type and methods for managing inline fragment selections. |
| v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go | Refactored composite type handling: replaced boolean interface check with `handleCompositeType` method; set oneof type and member types on messages; improved error handling for unresolved types; enhanced inline fragment field grouping and root field detection. |
| v2/pkg/engine/datasource/grpc_datasource/execution_plan_test.go | Centralized test mapping with `testMapping()`; merged and expanded interface/union execution plan tests; added mutation/union tests; updated all test cases to use new mapping and execution plan structures. |
| v2/pkg/engine/datasource/grpc_datasource/grpc_datasource.go | Improved marshaling logic for oneof fields and selection sets; now uses `IsOneOf()` and processes fields from `FieldSelectionSet` by data type; enhanced static value handling for union/interface member types. |
| v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.go | Activated and expanded tests for interface and union types; added new tests for union queries and mutations, including validation of polymorphic responses and inline fragments; introduced `setupTestGRPCServer` helper for server setup reuse. |
| v2/pkg/grpctest/mapping/mapping.go | Extended `DefaultGRPCMapping` with new RPCs (`randomSearchResult`, `search`, `performAction`), field mappings, and new types for union and action result scenarios. |
| v2/pkg/grpctest/mockservice.go | Added full mock implementations for new RPCs: `MutationPerformAction`, `QueryRandomSearchResult`, and `QuerySearch`; returns union/interface-like results for testing. |
| v2/pkg/grpctest/product.proto | Added proto definitions for new RPCs and union/oneof messages: `SearchResult`, `ActionResult`, `ActionSuccess`, `ActionError`, input messages, and updated service interface. |
| v2/pkg/grpctest/testdata/products.graphqls | Extended GraphQL schema with union types (`SearchResult`, `ActionResult`), new object and input types, and expanded `Query` and `Mutation` root types for union scenarios. |
| v2/pkg/engine/datasource/grpc_datasource/mapping_test_helper.go | Added `testMapping()` helper function returning a comprehensive GRPCMapping for use in tests, centralizing mapping configuration for queries, mutations, entities, enums, and fields. |📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 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 (2)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
v2/pkg/grpctest/mockservice.go (1)
20-64: Consider extracting action type strings as constants.The implementation correctly handles different action results. For better maintainability and discoverability of test cases, consider extracting the action type strings.
Add constants at the package level:
const ( ActionTypeError = "error_action" ActionTypeInvalid = "invalid_action" )Then update the switch statement:
switch actionType { -case "error_action": +case ActionTypeError: // Return an error result result = &productv1.ActionResult{ Value: &productv1.ActionResult_ActionError{ ActionError: &productv1.ActionError{ Message: "Action failed due to validation error", Code: "VALIDATION_ERROR", }, }, } -case "invalid_action": +case ActionTypeInvalid:v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.go (1)
498-527: Consider using bufconn for consistency with other tests.While the test implementation is comprehensive and correct, using a real TCP listener instead of bufconn could cause issues:
- Port conflicts when running tests in parallel
- Potential firewall/network issues in CI environments
- Inconsistency with other tests in the file
Consider using bufconn like other tests:
-// 1. Start a gRPC server with our mock implementation -lis, err := net.Listen("tcp", "localhost:0") -require.NoError(t, err) - -// Get the server address -serverAddr := fmt.Sprintf("localhost:%d", lis.Addr().(*net.TCPAddr).Port) +// 1. Start a gRPC server with our mock implementation +lis := bufconn.Listen(1024 * 1024)And update the connection:
-// 2. Connect to the gRPC server -// see https://github.com/grpc/grpc-go/issues/7091 -// nolint: staticcheck -conn, err := grpc.Dial( - serverAddr, - grpc.WithTransportCredentials(insecure.NewCredentials()), - grpc.WithLocalDNSResolution(), -) +// Create a buffer-based dialer +bufDialer := func(context.Context, string) (net.Conn, error) { + return lis.Dial() +} + +// 2. Connect to the gRPC server +// see https://github.com/grpc/grpc-go/issues/7091 +// nolint: staticcheck +conn, err := grpc.Dial( + "bufnet", + grpc.WithTransportCredentials(insecure.NewCredentials()), + grpc.WithContextDialer(bufDialer), +)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
v2/pkg/grpctest/productv1/product.pb.gois excluded by!**/*.pb.gov2/pkg/grpctest/productv1/product_grpc.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (9)
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go(2 hunks)v2/pkg/engine/datasource/grpc_datasource/execution_plan_test.go(14 hunks)v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go(6 hunks)v2/pkg/engine/datasource/grpc_datasource/grpc_datasource.go(1 hunks)v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.go(1 hunks)v2/pkg/grpctest/mapping/mapping.go(4 hunks)v2/pkg/grpctest/mockservice.go(1 hunks)v2/pkg/grpctest/product.proto(5 hunks)v2/pkg/grpctest/testdata/products.graphqls(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
v2/pkg/grpctest/mapping/mapping.go (2)
v2/pkg/graphqlerrors/response.go (1)
Response(9-14)v2/pkg/engine/datasource/grpc_datasource/configuration.go (1)
RPCConfigMap(5-5)
v2/pkg/engine/datasource/grpc_datasource/grpc_datasource.go (2)
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go (1)
OneOfType(18-18)v2/pkg/engine/datasource/grpc_datasource/compiler.go (1)
Message(116-120)
v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go (2)
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go (4)
OneOfType(18-18)OneOfTypeInterface(39-39)OneOfTypeUnion(41-41)RPCFieldSelectionSet(91-91)pkg/ast/ast_node_kind.go (5)
NodeKindField(32-32)NodeKindInterfaceTypeDefinition(16-16)NodeKindUnionTypeDefinition(18-18)NodeKindInlineFragment(35-35)NodeKindOperationDefinition(30-30)
v2/pkg/grpctest/mockservice.go (2)
v2/pkg/grpctest/productv1/product_grpc.pb.go (17)
ProductServiceServer(287-311)UnimplementedProductServiceServer(318-318)UnimplementedProductServiceServer(320-322)UnimplementedProductServiceServer(323-325)UnimplementedProductServiceServer(326-328)UnimplementedProductServiceServer(329-331)UnimplementedProductServiceServer(332-334)UnimplementedProductServiceServer(335-337)UnimplementedProductServiceServer(338-340)UnimplementedProductServiceServer(341-343)UnimplementedProductServiceServer(344-346)UnimplementedProductServiceServer(347-349)UnimplementedProductServiceServer(350-352)UnimplementedProductServiceServer(353-355)UnimplementedProductServiceServer(356-358)UnimplementedProductServiceServer(359-361)UnimplementedProductServiceServer(362-364)v2/pkg/grpctest/productv1/product.pb.go (56)
MutationPerformActionRequest(1864-1869)MutationPerformActionRequest(1882-1882)MutationPerformActionRequest(1897-1899)MutationPerformActionResponse(1909-1914)MutationPerformActionResponse(1927-1927)MutationPerformActionResponse(1942-1944)ActionResult(3029-3038)ActionResult(3051-3051)ActionResult(3066-3068)ActionResult_ActionError(3103-3105)ActionResult_ActionError(3109-3109)ActionError(3651-3657)ActionError(3670-3670)ActionError(3685-3687)ActionResult_ActionSuccess(3099-3101)ActionResult_ActionSuccess(3107-3107)ActionSuccess(3599-3605)ActionSuccess(3618-3618)ActionSuccess(3633-3635)QueryRandomSearchResultRequest(1692-1696)QueryRandomSearchResultRequest(1709-1709)QueryRandomSearchResultRequest(1724-1726)QueryRandomSearchResultResponse(1729-1734)QueryRandomSearchResultResponse(1747-1747)QueryRandomSearchResultResponse(1762-1764)SearchResult(2835-2845)SearchResult(2858-2858)SearchResult(2873-2875)SearchResult_Product(2915-2917)SearchResult_Product(2927-2927)Product(1953-1960)Product(1973-1973)Product(1988-1990)SearchResult_User(2919-2921)SearchResult_User(2929-2929)User(2073-2079)User(2092-2092)User(2107-2109)SearchResult_Category(2923-2925)SearchResult_Category(2931-2931)Category(2589-2596)Category(2609-2609)Category(2624-2626)CategoryKind_CATEGORY_KIND_ELECTRONICS(29-29)QuerySearchRequest(1602-1607)QuerySearchRequest(1620-1620)QuerySearchRequest(1635-1637)QuerySearchResponse(1647-1652)QuerySearchResponse(1665-1665)QuerySearchResponse(1680-1682)CategoryKind(24-24)CategoryKind(62-64)CategoryKind(66-68)CategoryKind(75-77)CategoryKind_CATEGORY_KIND_BOOK(28-28)CategoryKind_CATEGORY_KIND_FURNITURE(30-30)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build and test (go 1.23 / windows-latest)
- GitHub Check: Build and test (go 1.23 / windows-latest)
🔇 Additional comments (21)
v2/pkg/grpctest/mapping/mapping.go (1)
79-88: LGTM! The mapping configuration correctly extends support for union types.The new RPC configurations and field mappings follow the established patterns and naming conventions. The mappings align with the GraphQL schema additions for union types (
SearchResult,ActionResult) and their associated operations.Also applies to: 96-100, 189-197, 206-211, 440-471
v2/pkg/engine/datasource/grpc_datasource/grpc_datasource.go (1)
158-173: Well-implemented field selection and static value handling for composite types.The enhanced logic correctly:
- Merges fields from both the message fields and the type-specific field selection set
- Handles static values differently based on whether MemberTypes is populated
- Sets the static value to the matching member type name for union/interface types
v2/pkg/grpctest/testdata/products.graphqls (1)
138-165: Well-structured GraphQL schema additions for union type support.The union types and related operations follow GraphQL best practices:
- Clear union type definitions with appropriate member types
- Consistent naming conventions
- Proper input types for the new operations
- Good use case examples (polymorphic search results and success/error patterns)
Also applies to: 191-193, 203-204
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go (1)
16-42: Excellent design for representing composite types in the execution plan.The enhancements provide a robust foundation for handling union and interface types:
- Clear
OneOfTypeenum with descriptive constants- Appropriate field names returned by
FieldName()matching protobuf conventions- Well-structured
RPCMessageenhancements with field selection support- Clean helper methods for managing field selections
Also applies to: 77-88, 90-110
v2/pkg/grpctest/mockservice.go (2)
14-14: Good practice to ensure interface implementation at compile time.The compile-time check helps catch interface mismatches early.
66-170: Well-implemented mock methods for testing union type queries.The implementations provide excellent test coverage:
QueryRandomSearchResultuses randomization to test different union membersQuerySearchreturns a good mix of all union member types- Proper handling of the limit parameter with a sensible default
- Correct population of oneof fields for each union member
v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go (5)
161-168: LGTM! Good error handling for composite types.The integration of
handleCompositeTypewith proper error propagation viaStopWithInternalErrensures that any issues with resolving composite types are handled gracefully.
178-212: Well-structured composite type handling.The method effectively handles both interface and union types with:
- Clear separation of concerns for each node type
- Descriptive error messages that include type names
- Proper setting of
OneOfTypeandMemberTypeson the response message
218-220: Correct handling of inline fragment traversal.The early return for inline fragments prevents incorrect field index tracking, which is essential for proper union/interface type handling.
252-267: Clean helper methods for AST navigation.Both helper methods are well-designed:
IsRootField: Simple and effective check for root-level fieldsIsInlineFragmentField: Returns both the fragment reference and existence flag, enabling efficient inline fragment handlingThe bounds checking in
IsInlineFragmentFieldprevents potential panics.
346-354: Excellent implementation of inline fragment field grouping.The code correctly groups fields by their inline fragment type condition name, which is essential for proper union/interface type resolution. The lazy initialization of
FieldSelectionSetand the use of the fragment's type condition name as the key are both well-implemented.v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.go (3)
644-662: Excellent polymorphic response validation.The test validation correctly handles the Animal interface resolution by:
- Checking for type-specific fields to determine the concrete type
- Validating the
__typenamefield matches the expected type- Ensuring all common and type-specific fields are present
- Providing a clear error message if neither expected type is found
703-796: Comprehensive table-driven test structure for union types.Excellent test design with:
- Clear test case names describing the scenario
- Separate validation functions for each test case
- Thorough coverage of union type scenarios including:
- Single union result queries
- Array results with mixed types
- Mutation success/error cases
- Partial fragment selection
The validation logic properly checks type distribution and field presence for each union member.
821-826: Correct validation of partial fragment selection.The test properly validates GraphQL's behavior when only some union member fragments are selected. The Category type correctly returns only
__typenamewithout other fields when its fragment is not included in the query, which aligns with GraphQL specification.v2/pkg/engine/datasource/grpc_datasource/execution_plan_test.go (4)
342-344: Good refactoring to centralize mapping configuration.Using
testMapping()instead of inline mappings improves maintainability and reduces duplication across tests.
570-576: Correct field naming convention update.The change from camelCase (
filterField1) to snake_case (filter_field_1) properly aligns with protobuf naming conventions.
1377-2003: Excellent test coverage for composite types.The test cases comprehensively cover:
- Interface types with type-specific fields
- Union types with multiple member types
- Partial selections
- Common field selections
- Both single and list responses
The execution plans correctly specify
OneOfType,MemberTypes, andFieldSelectionSetfor proper composite type handling.
2526-3020: Well-structured centralized mapping configuration.The
testMapping()helper effectively consolidates all mapping configurations including:
- RPC configurations for queries, mutations, and entity lookups
- Field mappings with proper snake_case conventions
- Enum value mappings
- Comprehensive coverage of all types including the new composite types
This significantly improves test maintainability.
v2/pkg/grpctest/product.proto (3)
13-13: New RPCs properly support composite types.The added RPCs enable:
MutationPerformAction: Mutation returning union type resultsQuerySearch: Query returning lists of union typesQueryRandomSearchResult: Query returning single union typeThese align well with the GraphQL schema additions.
Also applies to: 23-25
325-331: Proper implementation of union types using protobuf oneof.The
SearchResultandActionResultmessages correctly use protobuf'soneoffeature to represent GraphQL union types. This ensures only one variant can be set at a time, matching GraphQL union semantics.Also applies to: 342-347
100-237: Good addition of descriptive comments.Adding comments to all request/response messages improves API documentation and makes the proto file more maintainable.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.go (3)
498-782: Well-structured interface type test with comprehensive coverage.This test provides excellent coverage for GraphQL interface types with various fragment patterns. The test case matrix approach and validation functions are well-designed.
Consider extracting the common server setup code (lines 500-534) into a helper function to reduce duplication across test methods, as this pattern is repeated in multiple tests.
+func setupTestGRPCServer(t *testing.T) (*bufconn.Listener, *grpc.ClientConn, func()) { + lis := bufconn.Listen(1024 * 1024) + server := grpc.NewServer() + mockService := &grpctest.MockService{} + productv1.RegisterProductServiceServer(server, mockService) + + go func() { + if err := server.Serve(lis); err != nil { + t.Errorf("failed to serve: %v", err) + } + }() + + bufDialer := func(context.Context, string) (net.Conn, error) { + return lis.Dial() + } + + conn, err := grpc.Dial("bufnet", + grpc.WithTransportCredentials(insecure.NewCredentials()), + grpc.WithContextDialer(bufDialer), + grpc.WithLocalDNSResolution()) + require.NoError(t, err) + + cleanup := func() { + server.Stop() + conn.Close() + } + + return lis, conn, cleanup +}
784-1201: Comprehensive union type test with excellent validation logic.This test provides thorough coverage for GraphQL union types across queries and mutations. The test cases cover various scenarios including success/error cases and fragment selection patterns.
The inline mapping definition (lines 1049-1161) is quite large and could impact maintainability. Consider extracting it into a separate helper function or constant to improve readability.
+func unionTestMapping() *GRPCMapping { + return &GRPCMapping{ + Service: "ProductService", + // ... rest of mapping configuration + } +}
1204-1204: Update outdated comment.The comment mentions "Test_DataSource_Load_WithProductQueries" but the actual function name is different.
-// Test_DataSource_Load_WithProductQueries tests the product-related query operations +// Test_DataSource_Load_WithCategoryQueries tests the category-related query operationsv2/pkg/engine/datasource/grpc_datasource/execution_plan.go (1)
16-42: Well-designed enum for oneof field types.The
OneOfTypeenum provides a clean abstraction for distinguishing interface and union types. TheFieldName()method correctly maps to the expected protobuf field names.Consider adding a
String()method for better debugging and logging support:+// String returns a string representation of the OneOfType. +func (o OneOfType) String() string { + switch o { + case OneOfTypeInterface: + return "Interface" + case OneOfTypeUnion: + return "Union" + case OneOfTypeNone: + return "None" + default: + return "Unknown" + } +}v2/pkg/engine/datasource/grpc_datasource/execution_plan_test.go (1)
2580-3074: Comprehensive centralized mapping configuration.This helper function provides extensive mapping configuration covering:
- Complete RPC mappings for all operation types
- Proper enum value mappings with gRPC naming conventions
- Comprehensive field mappings with camelCase to snake_case conversions
- Full support for composite types and their member types
- Proper argument mappings between GraphQL and gRPC
The mappings appear consistent and well-structured to support the new composite type functionality.
Consider breaking this large function into smaller, domain-specific mapping functions for better maintainability:
func testMapping() *GRPCMapping { mapping := &GRPCMapping{ Service: "Products", } mapping.QueryRPCs = buildQueryRPCs() mapping.MutationRPCs = buildMutationRPCs() mapping.EnumValues = buildEnumMappings() mapping.Fields = buildFieldMappings() return mapping }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go(2 hunks)v2/pkg/engine/datasource/grpc_datasource/execution_plan_test.go(14 hunks)v2/pkg/engine/datasource/grpc_datasource/grpc_datasource.go(1 hunks)v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- v2/pkg/engine/datasource/grpc_datasource/grpc_datasource.go
🧰 Additional context used
🧠 Learnings (1)
v2/pkg/engine/datasource/grpc_datasource/execution_plan_test.go (1)
Learnt from: Noroth
PR: wundergraph/graphql-go-tools#1197
File: v2/pkg/grpctest/product.proto:88-95
Timestamp: 2025-07-01T15:22:04.552Z
Learning: In proto files, example comments that are generated from tools use generic placeholders (like "LookupUserByIdRequest") rather than context-specific names, and this is acceptable since they are template-generated.
🧬 Code Graph Analysis (1)
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go (1)
execution/federationtesting/accounts/graph/model/models_gen.go (1)
Name(43-46)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build and test (go 1.23 / windows-latest)
- GitHub Check: Build and test (go 1.23 / windows-latest)
🔇 Additional comments (7)
v2/pkg/engine/datasource/grpc_datasource/execution_plan.go (3)
77-83: Enhanced RPCMessage structure for composite types.The new fields appropriately support GraphQL interface and union type handling. The field names are descriptive and purpose is clear.
85-98: Correct implementation of oneof type methods.The
IsOneOf()method correctly checks the range of valid oneof types. TheSelectValidTypes()method properly handles the type selection logic for both the base type and member types.
100-134: Efficient field selection set implementation.The
RPCFieldSelectionSettype and its methods provide a clean way to manage field selections from inline fragments. The deduplication logic inSelectFieldsForTypes()is correct and prevents duplicate fields.The
Addmethod properly initializes the slice when needed and theSelectFieldsForTypesmethod correctly handles the deduplication using a map-based approach.v2/pkg/engine/datasource/grpc_datasource/execution_plan_test.go (4)
344-344: LGTM! Good refactoring to centralize mapping configuration.The consolidation of mapping configurations into a single
testMapping()function improves maintainability and reduces duplication across test cases.Also applies to: 395-395, 477-477, 541-541
2059-2324: Comprehensive mutation union type test coverage.This test function provides excellent coverage for mutation operations returning union types, including:
- Complete ActionResult union handling with both success and error cases
- Partial union member selections (success-only and error-only scenarios)
- Proper mutation RPC configuration and response handling
The ActionResult pattern is a common GraphQL mutation design, making this valuable test coverage.
2326-2578: Good enum handling test coverage.This test function provides solid coverage for enum type handling, including:
- Single enum parameters with CategoryKind
- Enum array parameters
- Complex enum filters with nested input types
- Proper enum field mappings with EnumName property
The test structure properly validates both schema and operation, ensuring robust enum type support.
1377-2057: Enum and structure definitions verified
type OneOfType uint8is defined in execution_plan.go:18- Constants
OneOfTypeInterfaceandOneOfTypeUnionare declared and usedtype RPCFieldSelectionSet map[string]RPCFieldsis defined in execution_plan.go:101RPCMessagestruct includesOneOfTypeandMemberTypes []stringfields as expectedAll new enums and data structures are properly declared and referenced.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
v2/pkg/engine/datasource/grpc_datasource/execution_plan_test.go (1)
1444-1508: Consolidate or clarify duplicate union testsThe two test cases in
v2/pkg/engine/datasource/grpc_datasource/execution_plan_test.go
use the exact same GraphQL selection set and expected plan, differing only in test name and operation name:• Lines 1444–1508:
name: “Should create an execution plan for a query with a random cat and dog”
query:query CatAndDogQuery { … }• Immediately below (~lines 1509–1560):
name: “Should create an execution plan for a query with a union type”
query:query UnionQuery { … }Action items:
- Either merge/remove the redundant test case if they cover the same scenario.
- Or rename one test (and its GraphQL operation) to reflect a distinct behavior—e.g., “for both Cat and Dog” vs. “for a GraphQL union type.”
- Ensure each test adds unique coverage.
♻️ Duplicate comments (1)
v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.go (1)
660-661: Address the shared mapping helper organization.Multiple tests now use
testMapping()which improves consistency, but this function is defined inexecution_plan_test.gorather than a shared location, making it less discoverable for maintainability.Consider moving
testMapping()to a dedicated test helper file (e.g.,mapping_test_helper.go) within the same directory to improve discoverability and maintainability across all test files that use it.Also applies to: 931-931, 1068-1068, 1150-1150, 1241-1241
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
v2/pkg/engine/datasource/grpc_datasource/execution_plan_test.go(13 hunks)v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.go(19 hunks)v2/pkg/engine/datasource/grpc_datasource/mapping_test_helper.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- v2/pkg/engine/datasource/grpc_datasource/mapping_test_helper.go
🧰 Additional context used
🧠 Learnings (2)
v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.go (1)
Learnt from: SkArchon
PR: wundergraph/graphql-go-tools#1203
File: v2/pkg/engine/resolve/loader.go:63-67
Timestamp: 2025-07-02T15:28:02.073Z
Learning: In the graphql-go-tools codebase, result structs are consistently initialized with non-nil bytes.Buffer instances, making additional nil checks for res.out unnecessary defensive programming.
v2/pkg/engine/datasource/grpc_datasource/execution_plan_test.go (1)
Learnt from: Noroth
PR: wundergraph/graphql-go-tools#1197
File: v2/pkg/grpctest/product.proto:88-95
Timestamp: 2025-07-01T15:22:04.552Z
Learning: In proto files, example comments that are generated from tools use generic placeholders (like "LookupUserByIdRequest") rather than context-specific names, and this is acceptable since they are template-generated.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and test (go 1.23 / windows-latest)
🔇 Additional comments (12)
v2/pkg/engine/datasource/grpc_datasource/grpc_datasource_test.go (4)
56-99: Well-designed test helper function.The
setupTestGRPCServerhelper effectively consolidates repetitive server setup code across multiple tests. The implementation correctly handles bufconn setup, service registration, connection management, and provides proper cleanup through the returned function.
448-698: Comprehensive interface type testing with excellent coverage.The
Test_DataSource_Load_WithAnimalInterfacetest provides thorough coverage of GraphQL interface scenarios:
- Tests common fields vs type-specific fields
- Validates
__typenameresolution for Cat/Dog implementations- Covers various inline fragment combinations
- Includes proper validation logic for each scenario
The table-driven approach with individual validation functions makes the test maintainable and easy to extend.
700-969: Excellent union type test coverage across queries and mutations.The
Test_Datasource_Load_WithUnionTypestest comprehensively validates union type handling:
- Covers
SearchResultunion with Product/User/Category variants- Tests
ActionResultunion for success/error cases in mutations- Validates field selection and
__typenameresolution- Includes input variable handling and pagination scenarios
The test cases effectively exercise the complex polymorphic response handling that union types require.
150-151: Good refactoring to use the shared test helper.The consistent adoption of
setupTestGRPCServer(t)across all relevant tests eliminates code duplication and ensures consistent server setup behavior. The cleanup management throught.Cleanup(cleanup)follows Go testing best practices.Also applies to: 230-231, 322-323, 974-975, 1112-1113, 1226-1227
v2/pkg/engine/datasource/grpc_datasource/execution_plan_test.go (8)
342-344: LGTM: Good refactoring to centralize mapping configuration.The replacement of inline mapping with
testMapping()improves test maintainability and reduces duplication across test cases.
570-575: LGTM: Consistent field naming conventions.The standardization to snake_case for target field names (
filter_field_1,filter_field_2) aligns with protobuf naming conventions and maintains consistency across the codebase.
1377-1441: LGTM: Well-structured interface type test case.The test correctly validates execution plan generation for interface types with proper OneOfType configuration and field selection handling. The structure appropriately includes:
OneOfType: OneOfTypeInterfacefor interface handlingMemberTypeslisting concrete implementationsFieldSelectionSetfor type-specific field selections
1643-1695: LGTM: Good coverage of interface fragment usage.This test case correctly validates interface fragment handling with
... on Animal { ... }syntax, properly setting theFieldSelectionSetwith the interface type key. The emptyFields: RPCFields{}and populatedFieldSelectionSetstructure is correct for this pattern.
1747-1851: LGTM: Comprehensive union type test with multiple member types.The SearchResult union test properly validates:
- Union type handling with
OneOfType: OneOfTypeUnion- Multiple member types (Product, User, Category)
- Type-specific field selections in
FieldSelectionSet- Enum field handling for CategoryKind
2059-2147: LGTM: Excellent mutation union test coverage.The ActionResult union mutation test properly validates mutation execution plans with union return types. The structure correctly handles both success and error cases with appropriate field selections.
2327-2444: LGTM: Well-structured enum handling tests.The product execution plan tests properly validate enum parameter and field handling with
DataTypeEnumandEnumNamespecifications. The categoriesByKind and categoriesByKinds tests provide good coverage of single and array enum parameters.
2555-2555: LGTM: Consistent use of centralized mapping.Good use of
testMapping()to maintain consistency with other test functions in the file.
🤖 I have created a release *beep* *boop* --- ## [2.0.0-rc.198](v2.0.0-rc.197...v2.0.0-rc.198) (2025-07-04) ### Features * add support for composite types ([#1197](#1197)) ([e9b9f19](e9b9f19)) ### Bug Fixes * fix collecting representation for fetches scoped to concrete types ([#1200](#1200)) ([bcf547d](bcf547d)) --- 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 * **New Features** * Added support for composite types. * **Bug Fixes** * Corrected the collection of representations for fetches scoped to concrete types. * **Documentation** * Updated the changelog with details for version 2.0.0-rc.198. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
3 participants
Summary by CodeRabbit
New Features
SearchResult,ActionResult), input types, and related fields for more flexible querying and mutation results.Bug Fixes
Tests
Checklist