Skip to content

Commit dfd6523

Browse files
authored
fix: add an option to ignore skip/include directives (#1261)
During normalization this option enforces directiveIncludeSkipVisitor to keep nodes and delete directives instead. <!-- 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 * **New Features** * Added a configurable option to control whether nodes affected by `@skip` and `@include` directives are preserved or removed during operation normalization. * **Tests** * Introduced new test cases to verify the behavior of preserving nodes when processing `@skip` and `@include` directives. <!-- 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 d9fda8a commit dfd6523

File tree

3 files changed

+110
-11
lines changed

3 files changed

+110
-11
lines changed

v2/pkg/astnormalization/astnormalization.go

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -150,6 +150,7 @@ type options struct {
150150
removeUnusedVariables bool
151151
removeNotMatchingOperationDefinitions bool
152152
normalizeDefinition bool
153+
ignoreSkipInclude bool
153154
}
154155

155156
type Option func(options *options)
@@ -190,6 +191,12 @@ func WithNormalizeDefinition() Option {
190191
}
191192
}
192193

194+
func WithIgnoreSkipInclude() Option {
195+
return func(options *options) {
196+
options.ignoreSkipInclude = true
197+
}
198+
}
199+
193200
func (o *OperationNormalizer) setupOperationWalkers() {
194201
o.operationWalkers = make([]walkerStage, 0, 9)
195202

@@ -209,7 +216,7 @@ func (o *OperationNormalizer) setupOperationWalkers() {
209216

210217
directivesIncludeSkip := astvisitor.NewWalker(8)
211218
preventFragmentCycles(&directivesIncludeSkip)
212-
directiveIncludeSkip(&directivesIncludeSkip)
219+
directiveIncludeSkipKeepNodes(&directivesIncludeSkip, o.options.ignoreSkipInclude)
213220

214221
cleanup := astvisitor.NewWalker(8)
215222
deduplicateFields(&cleanup)

v2/pkg/astnormalization/directive_include_skip.go

Lines changed: 21 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -10,9 +10,18 @@ import (
1010
"github.com/wundergraph/graphql-go-tools/v2/pkg/lexer/literal"
1111
)
1212

13+
// directiveIncludeSkip registers a visitor to handle @include and @skip directives.
14+
// It deletes nodes that are evaluated as unused by the directives.
1315
func directiveIncludeSkip(walker *astvisitor.Walker) {
16+
directiveIncludeSkipKeepNodes(walker, false)
17+
}
18+
19+
// directiveIncludeSkipKeepNodes registers a visitor to handle @include and @skip directives.
20+
// If keepNodes is true, it unconditionally removes the directives and keeps parent nodes.
21+
func directiveIncludeSkipKeepNodes(walker *astvisitor.Walker, keepNodes bool) {
1422
visitor := directiveIncludeSkipVisitor{
15-
Walker: walker,
23+
Walker: walker,
24+
keepNodes: keepNodes,
1625
}
1726
walker.RegisterEnterDocumentVisitor(&visitor)
1827
walker.RegisterEnterDirectiveVisitor(&visitor)
@@ -21,6 +30,7 @@ func directiveIncludeSkip(walker *astvisitor.Walker) {
2130
type directiveIncludeSkipVisitor struct {
2231
*astvisitor.Walker
2332
operation, definition *ast.Document
33+
keepNodes bool
2434
}
2535

2636
func (d *directiveIncludeSkipVisitor) EnterDocument(operation, definition *ast.Document) {
@@ -62,8 +72,8 @@ func (d *directiveIncludeSkipVisitor) handleSkip(ref int) {
6272
default:
6373
return
6474
}
65-
if skip {
66-
d.handleRemoveNode()
75+
if !d.keepNodes && bool(skip) {
76+
d.removeParentNode()
6777
} else {
6878
d.operation.RemoveDirectiveFromNode(d.Ancestors[len(d.Ancestors)-1], ref)
6979
}
@@ -91,10 +101,10 @@ func (d *directiveIncludeSkipVisitor) handleInclude(ref int) {
91101
default:
92102
return
93103
}
94-
if include {
104+
if d.keepNodes || bool(include) {
95105
d.operation.RemoveDirectiveFromNode(d.Ancestors[len(d.Ancestors)-1], ref)
96106
} else {
97-
d.handleRemoveNode()
107+
d.removeParentNode()
98108
}
99109
}
100110

@@ -114,17 +124,19 @@ func (d *directiveIncludeSkipVisitor) getVariableValue(name string) (value, vali
114124
return false, false
115125
}
116126

117-
func (d *directiveIncludeSkipVisitor) handleRemoveNode() {
127+
func (d *directiveIncludeSkipVisitor) removeParentNode() {
118128
if len(d.Ancestors) < 2 {
119129
return
120130
}
121131

122-
removed := d.operation.RemoveNodeFromSelectionSetNode(d.Ancestors[len(d.Ancestors)-1], d.Ancestors[len(d.Ancestors)-2])
132+
parent := d.Ancestors[len(d.Ancestors)-1]
133+
grandParent := d.Ancestors[len(d.Ancestors)-2]
134+
removed := d.operation.RemoveNodeFromSelectionSetNode(parent, grandParent)
123135
if !removed {
124136
return
125137
}
126138

127-
if d.Ancestors[len(d.Ancestors)-2].Kind != ast.NodeKindSelectionSet {
139+
if grandParent.Kind != ast.NodeKindSelectionSet {
128140
return
129141
}
130142

@@ -133,7 +145,7 @@ func (d *directiveIncludeSkipVisitor) handleRemoveNode() {
133145
// So we have to add a __typename selection to the selection set,
134146
// but as this selection was not added by user it should not be added to resolved data
135147

136-
selectionSetRef := d.Ancestors[len(d.Ancestors)-2].Ref
148+
selectionSetRef := grandParent.Ref
137149

138150
if d.operation.SelectionSetIsEmpty(selectionSetRef) {
139151
selectionRef, _ := d.typeNameSelection()

v2/pkg/astnormalization/directive_include_skip_test.go

Lines changed: 81 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,10 @@
11
package astnormalization
22

3-
import "testing"
3+
import (
4+
"testing"
5+
6+
"github.com/wundergraph/graphql-go-tools/v2/pkg/astvisitor"
7+
)
48

59
func TestDirectiveIncludeVisitor(t *testing.T) {
610
t.Run("remove static include true on inline fragment", func(t *testing.T) {
@@ -316,4 +320,80 @@ func TestDirectiveIncludeVisitor(t *testing.T) {
316320
}
317321
}`, `{"yes":true,"no":false}`)
318322
})
323+
324+
t.Run("keepNodes", func(t *testing.T) {
325+
keepNodes := func(walker *astvisitor.Walker) {
326+
directiveIncludeSkipKeepNodes(walker, true)
327+
}
328+
329+
t.Run("skip should keep nodes", func(t *testing.T) {
330+
runWithVariables(t, keepNodes, testDefinition, `
331+
query($yes: Boolean = false, $no: Boolean = true) {
332+
dog {
333+
... @skip(if: $yes) {
334+
includeName: name
335+
}
336+
}
337+
withAlias: dog {
338+
name @skip(if: $no)
339+
}
340+
}`, `
341+
query($yes: Boolean = false, $no: Boolean = true) {
342+
dog {
343+
... {
344+
includeName: name
345+
}
346+
}
347+
withAlias: dog {
348+
name
349+
}
350+
}`, `{"yes":true,"no":false}`)
351+
})
352+
t.Run("include should keep nodes", func(t *testing.T) {
353+
runWithVariables(t, keepNodes, testDefinition, `
354+
query($yes: Boolean = false, $no: Boolean = true) {
355+
dog {
356+
... @include(if: $yes) {
357+
includeName: name
358+
}
359+
}
360+
withAlias: dog {
361+
name @include(if: $no)
362+
}
363+
}`, `
364+
query($yes: Boolean = false, $no: Boolean = true) {
365+
dog {
366+
... {
367+
includeName: name
368+
}
369+
}
370+
withAlias: dog {
371+
name
372+
}
373+
}`, `{"yes":true,"no":false}`)
374+
})
375+
t.Run("include/skip should keep nodes using default values", func(t *testing.T) {
376+
runWithVariables(t, keepNodes, testDefinition, `
377+
query($yes: Boolean = false, $no: Boolean = true) {
378+
dog {
379+
... @include(if: $yes) {
380+
includeName: name
381+
}
382+
}
383+
withAlias: dog {
384+
name @skip(if: $no)
385+
}
386+
}`, `
387+
query($yes: Boolean = false, $no: Boolean = true) {
388+
dog {
389+
... {
390+
includeName: name
391+
}
392+
}
393+
withAlias: dog {
394+
name
395+
}
396+
}`, `{}`)
397+
})
398+
})
319399
}

0 commit comments

Comments
 (0)