fix: return parsing error for empty selection sets#1220
Conversation
jensneuse
requested review from
a team,
Noroth,
StarpTech and
devsergiy
as code owners
WalkthroughThis change set introduces additional tests for handling empty GraphQL fragments in federation integration, updates parser logic to explicitly report errors on empty selection sets, and adjusts related tests to align with the new behavior. Multiple test cases across validation, parsing, and JSON schema modules are updated to use non-empty selection sets, and new test data files for empty fragment queries are added. Changes
✨ 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: 1
🧹 Nitpick comments (1)
execution/engine/federation_integration_test.go (1)
367-385: Consider refactoring to reduce code duplication.Both empty fragment tests follow identical patterns except for the query file names. Consider using a table-driven test approach to reduce duplication.
- t.Run("empty fragment", func(t *testing.T) { - setup := federationtesting.NewFederationSetup(addGateway(false)) - t.Cleanup(setup.Close) - gqlClient := NewGraphqlClient(http.DefaultClient) - ctx, cancel := context.WithCancel(context.Background()) - t.Cleanup(cancel) - resp := gqlClient.QueryStatusCode(ctx, setup.GatewayServer.URL, testQueryPath("queries/empty_fragment.graphql"), nil, http.StatusInternalServerError, t) - assert.Len(t, resp, 0) - }) - - t.Run("empty fragment variant", func(t *testing.T) { - setup := federationtesting.NewFederationSetup(addGateway(false)) - t.Cleanup(setup.Close) - gqlClient := NewGraphqlClient(http.DefaultClient) - ctx, cancel := context.WithCancel(context.Background()) - t.Cleanup(cancel) - resp := gqlClient.QueryStatusCode(ctx, setup.GatewayServer.URL, testQueryPath("queries/empty_fragment_variant.graphql"), nil, http.StatusInternalServerError, t) - assert.Len(t, resp, 0) - }) + emptyFragmentTests := []struct { + name string + queryFile string + }{ + {"empty fragment", "queries/empty_fragment.graphql"}, + {"empty fragment variant", "queries/empty_fragment_variant.graphql"}, + } + + for _, tt := range emptyFragmentTests { + t.Run(tt.name, func(t *testing.T) { + setup := federationtesting.NewFederationSetup(addGateway(false)) + t.Cleanup(setup.Close) + gqlClient := NewGraphqlClient(http.DefaultClient) + ctx, cancel := context.WithCancel(context.Background()) + t.Cleanup(cancel) + resp := gqlClient.QueryStatusCode(ctx, setup.GatewayServer.URL, testQueryPath(tt.queryFile), nil, http.StatusInternalServerError, t) + assert.Len(t, resp, 0) + }) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
execution/engine/federation_integration_test.go(1 hunks)execution/federationtesting/testdata/queries/empty_fragment.graphql(1 hunks)execution/federationtesting/testdata/queries/empty_fragment_variant.graphql(1 hunks)v2/pkg/ast/ast_operation_definition_test.go(1 hunks)v2/pkg/astparser/parser.go(1 hunks)v2/pkg/astparser/parser_test.go(3 hunks)v2/pkg/astvalidation/operation_validation_test.go(1 hunks)v2/pkg/graphqljsonschema/jsonschema_test.go(18 hunks)
🧰 Additional context used
🧠 Learnings (1)
execution/engine/federation_integration_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.122Z
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.
🧬 Code Graph Analysis (2)
v2/pkg/astparser/parser.go (1)
v2/pkg/ast/ast.go (1)
InvalidRef(8-8)
v2/pkg/astparser/parser_test.go (1)
v2/pkg/astparser/parser.go (1)
ParseGraphqlDocumentString(37-45)
⏰ 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.23 / windows-latest)
- GitHub Check: Build and test (go 1.23 / windows-latest)
- GitHub Check: Build and test (go 1.23 / ubuntu-latest)
🔇 Additional comments (11)
execution/federationtesting/testdata/queries/empty_fragment.graphql (1)
1-5: LGTM! Test data file correctly demonstrates empty fragment case.This test file appropriately defines an empty fragment to validate the parser's error handling for empty selection sets. The fragment
Ahas no fields, which should trigger the new error reporting behavior in the parser.v2/pkg/astparser/parser.go (1)
1313-1316: Excellent improvement! Parser now explicitly reports empty selection set errors.This change transforms silent failures into proper error reporting. When encountering an empty selection set, the parser now:
- Calls
errUnexpectedTokenwith appropriate expected tokens (IDENT, SPREAD)- Returns
ast.InvalidRefinstead of 0 to indicate failureThis aligns perfectly with the PR objective of returning parsing errors for empty selection sets.
v2/pkg/graphqljsonschema/jsonschema_test.go (1)
53-53: Systematic and necessary updates to comply with new parser behavior.All test cases have been consistently updated to replace empty selection sets with
{ a }, ensuring compatibility with the new parser validation rules. These changes:
- Maintain test validity while adding minimal required field selections
- Are applied uniformly across all affected test cases
- Don't alter the core test logic or expected results
- Align with the PR objective of rejecting empty selection sets
The minimal field selection is appropriate since these tests focus on input validation rather than query execution.
Also applies to: 65-65, 93-93, 107-107, 123-123, 139-139, 152-152, 165-165, 176-176, 188-188, 204-204, 221-221, 240-240, 260-260, 276-276, 290-290, 297-297, 309-309, 322-322
v2/pkg/ast/ast_operation_definition_test.go (1)
33-33: Necessary test updates to maintain compatibility with stricter parser rules.The test cases have been properly updated to include non-empty selection sets while preserving the original test logic for operation name existence checking. The changes:
- Replace empty operation bodies with minimal field selections
- Maintain the test's core functionality
- Use appropriate field names for testing purposes
- Ensure compliance with the new parser validation rules
Also applies to: 39-39, 45-45
execution/federationtesting/testdata/queries/empty_fragment_variant.graphql (1)
1-9: LGTM! Additional test coverage for empty inline fragments.This test file provides valuable coverage for empty inline fragment scenarios. The fragment
Acontains an empty inline fragment... { }which should trigger the parser's error reporting for empty selection sets. This complements the other empty fragment test file and ensures comprehensive validation coverage.execution/engine/federation_integration_test.go (2)
367-375: LGTM! Proper integration test for empty fragment error handling.The test correctly verifies that empty fragments result in HTTP 500 Internal Server Error responses with empty bodies, which aligns with the PR objectives of making the parser explicitly report errors on empty selection sets.
377-385: LGTM! Consistent test pattern for empty fragment variant.The test follows the same structure as the previous empty fragment test and properly validates the error response.
v2/pkg/astvalidation/operation_validation_test.go (1)
3536-3544: LGTM! Necessary adaptation to new parser behavior.The addition of
{ a }field selections to these queries is a proper adjustment to maintain test functionality while implementing the new parser behavior that disallows empty selection sets. The test intent (validating variable input types) remains unchanged, and the validation expectations are preserved.v2/pkg/astparser/parser_test.go (3)
781-800: LGTM: Test inputs updated to use valid GraphQL syntaxThe modifications correctly update the test inputs to include minimal field definitions instead of empty braces, which aligns with the stricter parsing behavior for empty selection sets. The changes are appropriate:
- Type, interface, and input extensions now include
a: Intfield- Scalar extension correctly removes braces (scalars don't have selection sets)
- Union extension syntax remains unchanged as expected
2234-2245: LGTM: Index test updated with minimal valid contentThe changes appropriately update the test definitions to include minimal valid content instead of empty braces:
- Object types, interfaces, and input types now include
a: Intfield- Enums now include single values (
A,B) instead of empty braces- Scalars remain without braces as expected
These modifications ensure the test continues to validate the indexing functionality while conforming to the stricter parser behavior for empty selection sets.
2587-2601: LGTM: New test case correctly validates empty fragment error reportingThis test case effectively validates the new parser behavior for empty selection sets:
- Tests the specific case of empty fragment definition (
fragment A on Query {})- Verifies the parser reports the expected error message about unexpected RBRACE token
- Error message correctly indicates that IDENT or SPREAD tokens are expected instead
- Follows the established pattern of other error reporting tests in the same function
This directly supports the PR objective to "return parsing error for empty selection sets" and ensures the feature works as intended.
🤖 I have created a release *beep* *boop* --- ## [2.0.0-rc.202](v2.0.0-rc.201...v2.0.0-rc.202) (2025-07-09) ### Bug Fixes * return parsing error for empty selection sets ([#1220](#1220)) ([726c0d2](726c0d2)) --- 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 an issue where empty selection sets during parsing now correctly return a parsing error. * **Documentation** * Updated the changelog with details for version 2.0.0-rc.202. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
2 participants
Summary by CodeRabbit
Tests
Bug Fixes