Skip to content

Commit 03189bc

Browse files
devsergiydevsergiy
authored
fix: fix conflict of rewriter and required fields (#1338)
<!-- Important: Before developing new features, please open an issue to discuss your ideas with the maintainers. This ensures project alignment and helps avoid unnecessary work for you. Thank you for your contribution! Please provide a detailed description below and ensure you've met all the requirements. Squashed commit messages must follow the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) standard to facilitate changelog generation. Please ensure your PR title follows the Conventional Commits specification, using the appropriate type (e.g., feat, fix, docs) and scope. Examples of good PR titles: - 💥feat!: change implementation in an non-backward compatible way - ✨feat(auth): add support for OAuth2 login - 🐞fix(router): add support for custom metrics - 📚docs(README): update installation instructions - 🧹chore(deps): bump dependencies to latest versions --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved error handling by gracefully skipping missing nodes instead of crashing during query planning. * **New Features** * Enhanced federated schema query planning with improved dependency tracking across subgraphs. * Refined field resolution and selection handling for more accurate query execution in complex federation scenarios. * **Improvements** * Better error diagnostics with enhanced context in planning output. <!-- end of auto-generated comment: release notes by coderabbit.ai --> ## 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. <!-- Please add any additional information or context regarding your changes here. -->
1 parent af46be6 commit 03189bc

14 files changed

+983
-152
lines changed

‎v2/pkg/astprinter/astprinter.go‎

Lines changed: 43 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -269,8 +269,17 @@ func (p *printVisitor) EnterArgument(ref int) {
269269
}
270270

271271
func (p *printVisitor) LeaveArgument(ref int) {
272-
if len(p.document.ArgumentsAfter(p.Ancestors[len(p.Ancestors)-1], ref)) == 0 {
273-
p.write(literal.RPAREN)
272+
if len(p.document.ArgumentsAfter(p.Ancestors[len(p.Ancestors)-1], ref)) > 0 {
273+
// if not the last argument
274+
return
275+
}
276+
277+
// last argument
278+
p.write(literal.RPAREN)
279+
280+
a := p.Ancestors[len(p.Ancestors)-1]
281+
if p.debug && a.Kind == ast.NodeKindField && !p.document.FieldHasSelections(a.Ref) && !p.document.FieldHasDirectives(a.Ref) {
282+
p.printFieldInfo(a.Ref, true)
274283
}
275284
}
276285

@@ -319,14 +328,23 @@ func (p *printVisitor) LeaveOperationDefinition(ref int) {
319328
}
320329

321330
func (p *printVisitor) EnterSelectionSet(ref int) {
331+
p.write(literal.LBRACE)
332+
322333
if p.debug {
323-
p.writeIndented(literal.LINETERMINATOR)
324-
p.writeIndented([]byte("# SelectionSet ref:"))
334+
p.write([]byte(" # setRef:"))
325335
p.write([]byte(strconv.Itoa(ref)))
326-
p.write(literal.LINETERMINATOR)
327-
p.writeIndented(literal.LBRACE)
328-
} else {
329-
p.write(literal.LBRACE)
336+
337+
if l := len(p.SimpleWalker.Ancestors); l > 0 {
338+
a := p.SimpleWalker.Ancestors[l-1]
339+
switch a.Kind {
340+
case ast.NodeKindField:
341+
p.printFieldInfo(a.Ref, false)
342+
case ast.NodeKindInlineFragment:
343+
p.write([]byte(" fragmentRef:"))
344+
p.write([]byte(strconv.Itoa(a.Ref)))
345+
default:
346+
}
347+
}
330348
}
331349

332350
if p.indent != nil {
@@ -342,15 +360,6 @@ func (p *printVisitor) LeaveSelectionSet(ref int) {
342360
}
343361

344362
func (p *printVisitor) EnterField(ref int) {
345-
if p.debug {
346-
p.writeIndented([]byte("# Field ref:"))
347-
p.write([]byte(strconv.Itoa(ref)))
348-
if p.fieldCallback != nil {
349-
p.fieldCallback(ref, p.out)
350-
}
351-
p.write(literal.LINETERMINATOR)
352-
}
353-
354363
if p.document.Fields[ref].Alias.IsDefined {
355364
p.writeIndented(p.document.Input.ByteSlice(p.document.Fields[ref].Alias.Name))
356365
p.write(literal.COLON)
@@ -362,6 +371,23 @@ func (p *printVisitor) EnterField(ref int) {
362371
if !p.document.FieldHasArguments(ref) && (p.document.FieldHasSelections(ref) || p.document.FieldHasDirectives(ref)) {
363372
p.write(literal.SPACE)
364373
}
374+
375+
if p.debug && !p.document.FieldHasArguments(ref) && !p.document.FieldHasSelections(ref) && !p.document.FieldHasDirectives(ref) {
376+
p.printFieldInfo(ref, true)
377+
}
378+
}
379+
380+
func (p *printVisitor) printFieldInfo(ref int, comment bool) {
381+
if p.debug {
382+
if comment {
383+
p.write([]byte(" #"))
384+
}
385+
p.write([]byte(" fieldRef:"))
386+
p.write([]byte(strconv.Itoa(ref)))
387+
if p.fieldCallback != nil {
388+
p.fieldCallback(ref, p.out)
389+
}
390+
}
365391
}
366392

367393
func (p *printVisitor) LeaveField(ref int) {
@@ -399,12 +425,6 @@ func (p *printVisitor) LeaveFragmentSpread(ref int) {
399425
}
400426

401427
func (p *printVisitor) EnterInlineFragment(ref int) {
402-
if p.debug {
403-
p.write(literal.LINETERMINATOR)
404-
p.writeIndented([]byte("# InlineFragment ref:"))
405-
p.write([]byte(strconv.Itoa(ref)))
406-
p.write(literal.LINETERMINATOR)
407-
}
408428
p.writeIndented(literal.SPREAD)
409429

410430
if p.document.InlineFragmentHasTypeCondition(ref) && !p.document.InlineFragmentIsOfTheSameType(ref) {

‎v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1372,7 +1372,7 @@ func (p *Planner[T]) debugPrintQueryPlan(operation *ast.Document) {
13721372
if p.hasFederationRoot && p.dataSourcePlannerConfig.HasRequiredFields() { // IsRepresentationsQuery
13731373
args = append(args, "Representations:\n")
13741374
for _, cfg := range p.dataSourcePlannerConfig.RequiredFields {
1375-
key, report := plan.RequiredFieldsFragment(cfg.TypeName, cfg.SelectionSet, cfg.FieldName == "")
1375+
key, report := plan.QueryPlanRequiredFieldsFragment(cfg.TypeName, cfg.FieldName, cfg.SelectionSet)
13761376
if report.HasErrors() {
13771377
continue
13781378
}
@@ -1407,7 +1407,7 @@ func (p *Planner[T]) generateQueryPlansForFetchConfiguration(operation *ast.Docu
14071407
if p.hasFederationRoot && p.dataSourcePlannerConfig.HasRequiredFields() { // IsRepresentationsQuery
14081408
representations = make([]resolve.Representation, len(p.dataSourcePlannerConfig.RequiredFields))
14091409
for i, cfg := range p.dataSourcePlannerConfig.RequiredFields {
1410-
fragmentAst, report := plan.QueryPlanRequiredFieldsFragment(cfg.FieldName, cfg.TypeName, cfg.SelectionSet)
1410+
fragmentAst, report := plan.QueryPlanRequiredFieldsFragment(cfg.TypeName, cfg.FieldName, cfg.SelectionSet)
14111411
if report.HasErrors() {
14121412
p.stopWithError(errors.WithStack(fmt.Errorf("generateQueryPlansForFetchConfiguration: failed to build fragment for required fields: %w", report)))
14131413
return
@@ -1446,7 +1446,7 @@ func (p *Planner[T]) printOperation() (operationBytes []byte, variablesBytes []b
14461446
// create empty operation and definition documents
14471447
definition, err := p.config.UpstreamSchema()
14481448
if err != nil {
1449-
p.stopWithError(errors.WithStack(fmt.Errorf("printOperation: failed to read upstream schema: %w", err)))
1449+
p.stopWithError(errors.WithStack(fmt.Errorf("printOperation planner id: %d: failed to read upstream schema: %w", p.id, err)))
14501450
return nil, nil
14511451
}
14521452

@@ -1457,7 +1457,7 @@ func (p *Planner[T]) printOperation() (operationBytes []byte, variablesBytes []b
14571457
// normalize upstream operation
14581458
kit.normalizer.NormalizeOperation(p.upstreamOperation, definition, kit.report)
14591459
if kit.report.HasErrors() {
1460-
p.stopWithError(errors.WithStack(fmt.Errorf("printOperation: normalization failed: %w", kit.report)))
1460+
p.stopWithError(errors.WithStack(fmt.Errorf("printOperation planner id: %d: normalization failed: %w", p.id, kit.report)))
14611461
return nil, nil
14621462
}
14631463

@@ -1468,7 +1468,7 @@ func (p *Planner[T]) printOperation() (operationBytes []byte, variablesBytes []b
14681468

14691469
kit.validator.Validate(p.upstreamOperation, definition, kit.report)
14701470
if kit.report.HasErrors() {
1471-
p.stopWithError(errors.WithStack(fmt.Errorf("printOperation: validation failed: %w", kit.report)))
1471+
p.stopWithError(errors.WithStack(fmt.Errorf("printOperation planner id: %d: validation failed: %w", p.id, kit.report)))
14721472
return nil, nil
14731473
}
14741474

@@ -1478,7 +1478,7 @@ func (p *Planner[T]) printOperation() (operationBytes []byte, variablesBytes []b
14781478
// print upstream operation
14791479
err = kit.printer.Print(p.upstreamOperation, kit.buf)
14801480
if err != nil {
1481-
p.stopWithError(errors.WithStack(fmt.Errorf("printOperation: failed to print: %w", err)))
1481+
p.stopWithError(errors.WithStack(fmt.Errorf("printOperation planner id: %d: failed to print: %w", p.id, err)))
14821482
return nil, nil
14831483
}
14841484

@@ -1493,7 +1493,7 @@ func (p *Planner[T]) printOperation() (operationBytes []byte, variablesBytes []b
14931493
Pretty: false,
14941494
}, kit.buf)
14951495
if err != nil {
1496-
p.stopWithError(errors.WithStack(fmt.Errorf("printOperation: failed to minify: %w", err)))
1496+
p.stopWithError(errors.WithStack(fmt.Errorf("printOperation planner id: %d: failed to minify: %w", p.id, err)))
14971497
return nil, nil
14981498
}
14991499
if madeReplacements && kit.buf.Len() < len(rawOperationBytes) {

‎v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_federation_entity_interfaces_test.go‎

Lines changed: 28 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -4587,16 +4587,17 @@ func TestGraphQLDataSourceFederationEntityInterfaces(t *testing.T) {
45874587
DependsOnFetchIDs: []int{0},
45884588
},
45894589
FetchConfiguration: resolve.FetchConfiguration{
4590-
Input: `{"method":"POST","url":"http://localhost:4001/graphql","body":{"query":"query($representations: [_Any!]!){_entities(representations: $representations){... on Admin {__typename} ... on Moderator {__typename title} ... on User {__typename title}}}","variables":{"representations":[$$0$$]}}}`,
4590+
Input: `{"method":"POST","url":"http://localhost:4004/graphql","body":{"query":"query($representations: [_Any!]!){_entities(representations: $representations){... on Account {age}}}","variables":{"representations":[$$0$$]}}}`,
45914591
Variables: []resolve.Variable{
45924592
&resolve.ResolvableObjectVariable{
45934593
Renderer: resolve.NewGraphQLVariableResolveRenderer(&resolve.Object{
45944594
Nullable: true,
45954595
Fields: []*resolve.Field{
45964596
{
45974597
Name: []byte("__typename"),
4598-
Value: &resolve.String{
4599-
Path: []string{"__typename"},
4598+
Value: &resolve.StaticString{
4599+
Path: []string{"__typename"},
4600+
Value: "Account",
46004601
},
46014602
OnTypeNames: [][]byte{[]byte("Admin"), []byte("Account")},
46024603
},
@@ -4609,8 +4610,9 @@ func TestGraphQLDataSourceFederationEntityInterfaces(t *testing.T) {
46094610
},
46104611
{
46114612
Name: []byte("__typename"),
4612-
Value: &resolve.String{
4613-
Path: []string{"__typename"},
4613+
Value: &resolve.StaticString{
4614+
Path: []string{"__typename"},
4615+
Value: "Account",
46144616
},
46154617
OnTypeNames: [][]byte{[]byte("Moderator"), []byte("Account")},
46164618
},
@@ -4623,8 +4625,9 @@ func TestGraphQLDataSourceFederationEntityInterfaces(t *testing.T) {
46234625
},
46244626
{
46254627
Name: []byte("__typename"),
4626-
Value: &resolve.String{
4627-
Path: []string{"__typename"},
4628+
Value: &resolve.StaticString{
4629+
Path: []string{"__typename"},
4630+
Value: "Account",
46284631
},
46294632
OnTypeNames: [][]byte{[]byte("User"), []byte("Account")},
46304633
},
@@ -4652,17 +4655,16 @@ func TestGraphQLDataSourceFederationEntityInterfaces(t *testing.T) {
46524655
DependsOnFetchIDs: []int{0},
46534656
},
46544657
FetchConfiguration: resolve.FetchConfiguration{
4655-
Input: `{"method":"POST","url":"http://localhost:4004/graphql","body":{"query":"query($representations: [_Any!]!){_entities(representations: $representations){... on Account {age}}}","variables":{"representations":[$$0$$]}}}`,
4658+
Input: `{"method":"POST","url":"http://localhost:4001/graphql","body":{"query":"query($representations: [_Any!]!){_entities(representations: $representations){... on Admin {__typename} ... on Moderator {__typename title} ... on User {__typename title}}}","variables":{"representations":[$$0$$]}}}`,
46564659
Variables: []resolve.Variable{
46574660
&resolve.ResolvableObjectVariable{
46584661
Renderer: resolve.NewGraphQLVariableResolveRenderer(&resolve.Object{
46594662
Nullable: true,
46604663
Fields: []*resolve.Field{
46614664
{
46624665
Name: []byte("__typename"),
4663-
Value: &resolve.StaticString{
4664-
Path: []string{"__typename"},
4665-
Value: "Account",
4666+
Value: &resolve.String{
4667+
Path: []string{"__typename"},
46664668
},
46674669
OnTypeNames: [][]byte{[]byte("Admin"), []byte("Account")},
46684670
},
@@ -4675,9 +4677,8 @@ func TestGraphQLDataSourceFederationEntityInterfaces(t *testing.T) {
46754677
},
46764678
{
46774679
Name: []byte("__typename"),
4678-
Value: &resolve.StaticString{
4679-
Path: []string{"__typename"},
4680-
Value: "Account",
4680+
Value: &resolve.String{
4681+
Path: []string{"__typename"},
46814682
},
46824683
OnTypeNames: [][]byte{[]byte("Moderator"), []byte("Account")},
46834684
},
@@ -4690,9 +4691,8 @@ func TestGraphQLDataSourceFederationEntityInterfaces(t *testing.T) {
46904691
},
46914692
{
46924693
Name: []byte("__typename"),
4693-
Value: &resolve.StaticString{
4694-
Path: []string{"__typename"},
4695-
Value: "Account",
4694+
Value: &resolve.String{
4695+
Path: []string{"__typename"},
46964696
},
46974697
OnTypeNames: [][]byte{[]byte("User"), []byte("Account")},
46984698
},
@@ -4718,7 +4718,7 @@ func TestGraphQLDataSourceFederationEntityInterfaces(t *testing.T) {
47184718
resolve.SingleWithPath(&resolve.SingleFetch{
47194719
FetchDependencies: resolve.FetchDependencies{
47204720
FetchID: 3,
4721-
DependsOnFetchIDs: []int{0, 1},
4721+
DependsOnFetchIDs: []int{0, 2},
47224722
},
47234723
FetchConfiguration: resolve.FetchConfiguration{
47244724
Input: `{"method":"POST","url":"http://localhost:4003/graphql","body":{"query":"query($representations: [_Any!]!){_entities(representations: $representations){... on Admin {__typename title}}}","variables":{"representations":[$$0$$]}}}`,
@@ -4755,7 +4755,7 @@ func TestGraphQLDataSourceFederationEntityInterfaces(t *testing.T) {
47554755
resolve.SingleWithPath(&resolve.SingleFetch{
47564756
FetchDependencies: resolve.FetchDependencies{
47574757
FetchID: 4,
4758-
DependsOnFetchIDs: []int{0, 1, 3},
4758+
DependsOnFetchIDs: []int{0, 2, 3},
47594759
},
47604760
FetchConfiguration: resolve.FetchConfiguration{
47614761
Input: `{"method":"POST","url":"http://localhost:4002/graphql","body":{"query":"query($representations: [_Any!]!){_entities(representations: $representations){... on Account {uniqueTitle}}}","variables":{"representations":[$$0$$]}}}`,
@@ -4770,65 +4770,65 @@ func TestGraphQLDataSourceFederationEntityInterfaces(t *testing.T) {
47704770
Path: []string{"__typename"},
47714771
Value: "Account",
47724772
},
4773-
OnTypeNames: [][]byte{[]byte("Admin"), []byte("Account")},
4773+
OnTypeNames: [][]byte{[]byte("Moderator"), []byte("Account")},
47744774
},
47754775
{
47764776
Name: []byte("title"),
47774777
Value: &resolve.String{
47784778
Path: []string{"title"},
47794779
},
4780-
OnTypeNames: [][]byte{[]byte("Admin"), []byte("Account")},
4780+
OnTypeNames: [][]byte{[]byte("Moderator"), []byte("Account")},
47814781
},
47824782
{
47834783
Name: []byte("id"),
47844784
Value: &resolve.Scalar{
47854785
Path: []string{"id"},
47864786
},
4787-
OnTypeNames: [][]byte{[]byte("Admin"), []byte("Account")},
4787+
OnTypeNames: [][]byte{[]byte("Moderator"), []byte("Account")},
47884788
},
47894789
{
47904790
Name: []byte("__typename"),
47914791
Value: &resolve.StaticString{
47924792
Path: []string{"__typename"},
47934793
Value: "Account",
47944794
},
4795-
OnTypeNames: [][]byte{[]byte("Moderator"), []byte("Account")},
4795+
OnTypeNames: [][]byte{[]byte("User"), []byte("Account")},
47964796
},
47974797
{
47984798
Name: []byte("title"),
47994799
Value: &resolve.String{
48004800
Path: []string{"title"},
48014801
},
4802-
OnTypeNames: [][]byte{[]byte("Moderator"), []byte("Account")},
4802+
OnTypeNames: [][]byte{[]byte("User"), []byte("Account")},
48034803
},
48044804
{
48054805
Name: []byte("id"),
48064806
Value: &resolve.Scalar{
48074807
Path: []string{"id"},
48084808
},
4809-
OnTypeNames: [][]byte{[]byte("Moderator"), []byte("Account")},
4809+
OnTypeNames: [][]byte{[]byte("User"), []byte("Account")},
48104810
},
48114811
{
48124812
Name: []byte("__typename"),
48134813
Value: &resolve.StaticString{
48144814
Path: []string{"__typename"},
48154815
Value: "Account",
48164816
},
4817-
OnTypeNames: [][]byte{[]byte("User"), []byte("Account")},
4817+
OnTypeNames: [][]byte{[]byte("Admin"), []byte("Account")},
48184818
},
48194819
{
48204820
Name: []byte("title"),
48214821
Value: &resolve.String{
48224822
Path: []string{"title"},
48234823
},
4824-
OnTypeNames: [][]byte{[]byte("User"), []byte("Account")},
4824+
OnTypeNames: [][]byte{[]byte("Admin"), []byte("Account")},
48254825
},
48264826
{
48274827
Name: []byte("id"),
48284828
Value: &resolve.Scalar{
48294829
Path: []string{"id"},
48304830
},
4831-
OnTypeNames: [][]byte{[]byte("User"), []byte("Account")},
4831+
OnTypeNames: [][]byte{[]byte("Admin"), []byte("Account")},
48324832
},
48334833
},
48344834
}),

0 commit comments

Comments
 (0)