From 2cc0630d8ecbc313e7bd78942458519f96bce782 Mon Sep 17 00:00:00 2001 From: Jay Herron Date: Tue, 12 Sep 2023 00:05:58 -0600 Subject: [PATCH 1/2] fix: Fixes NoUnusedVariablesRule https://github.com/GraphQLSwift/GraphQL/issues/61 --- Sources/GraphQL/Language/AST.swift | 21 ++++++++++++- Sources/GraphQL/Language/Kinds.swift | 2 +- Sources/GraphQL/Language/Visitor.swift | 10 +++---- .../Rules/NoUnusedVariablesRule.swift | 30 +++++-------------- Sources/GraphQL/Validation/Validate.swift | 9 +++--- .../NoUnusedVariablesRuleTests.swift | 10 +++++++ 6 files changed, 47 insertions(+), 35 deletions(-) diff --git a/Sources/GraphQL/Language/AST.swift b/Sources/GraphQL/Language/AST.swift index 22da2a76..44c285c0 100644 --- a/Sources/GraphQL/Language/AST.swift +++ b/Sources/GraphQL/Language/AST.swift @@ -301,7 +301,6 @@ public func == (lhs: Definition, rhs: Definition) -> Bool { public enum OperationType: String { case query case mutation - // Note: subscription is an experimental non-spec addition. case subscription } @@ -949,6 +948,15 @@ public final class ObjectValue { self.loc = loc self.fields = fields } + + public func get(key: String) -> NodeResult? { + switch key { + case "fields": + return .array(fields) + default: + return nil + } + } } extension ObjectValue: Equatable { @@ -968,6 +976,17 @@ public final class ObjectField { self.name = name self.value = value } + + public func get(key: String) -> NodeResult? { + switch key { + case "name": + return .node(name) + case "value": + return .node(value) + default: + return nil + } + } } extension ObjectField: Equatable { diff --git a/Sources/GraphQL/Language/Kinds.swift b/Sources/GraphQL/Language/Kinds.swift index 448db3a7..4c4fd0f2 100644 --- a/Sources/GraphQL/Language/Kinds.swift +++ b/Sources/GraphQL/Language/Kinds.swift @@ -1,4 +1,4 @@ -public enum Kind { +public enum Kind: CaseIterable { case name case document case operationDefinition diff --git a/Sources/GraphQL/Language/Visitor.swift b/Sources/GraphQL/Language/Visitor.swift index 86c10b5c..293fd113 100644 --- a/Sources/GraphQL/Language/Visitor.swift +++ b/Sources/GraphQL/Language/Visitor.swift @@ -268,7 +268,7 @@ final class Stack { * If a prior visitor edits a node, no following visitors will see that node. */ func visitInParallel(visitors: [Visitor]) -> Visitor { - var skipping = [Node?](repeating: nil, count: visitors.count) + var skipping = [VisitResult?](repeating: nil, count: visitors.count) return Visitor( enter: { node, key, parent, path, ancestors in @@ -283,9 +283,9 @@ func visitInParallel(visitors: [Visitor]) -> Visitor { ) if case .skip = result { - skipping[i] = node + skipping[i] = .node(node) } else if case .break = result { -// skipping[i] = BREAK + skipping[i] = .break } else if case .node = result { return result } @@ -306,11 +306,11 @@ func visitInParallel(visitors: [Visitor]) -> Visitor { ) if case .break = result { -// skipping[i] = BREAK + skipping[i] = .break } else if case .node = result { return result } - } // else if skipping[i] == node { + } // else if case let .node(skippedNode) = skipping[i], skippedNode == node { // skipping[i] = nil // } } diff --git a/Sources/GraphQL/Validation/Rules/NoUnusedVariablesRule.swift b/Sources/GraphQL/Validation/Rules/NoUnusedVariablesRule.swift index 3af733a7..44cf2992 100644 --- a/Sources/GraphQL/Validation/Rules/NoUnusedVariablesRule.swift +++ b/Sources/GraphQL/Validation/Rules/NoUnusedVariablesRule.swift @@ -5,38 +5,23 @@ * are used, either directly or within a spread fragment. */ func NoUnusedVariablesRule(context: ValidationContext) -> Visitor { - var variableDefs: [VariableDefinition] = [] - return Visitor( - enter: { node, _, _, _, _ in - if node is OperationDefinition { - variableDefs = [] - return .continue - } - - if let def = node as? VariableDefinition { - variableDefs.append(def) - return .continue - } - - return .continue + enter: { _, _, _, _, _ in + .continue }, leave: { node, _, _, _, _ -> VisitResult in guard let operation = node as? OperationDefinition else { return .continue } - var variableNameUsed: [String: Bool] = [:] let usages = context.getRecursiveVariableUsages(operation: operation) + let variableNameUsed = Set(usages.map { usage in + usage.node.name.value + }) - for usage in usages { - variableNameUsed[usage.node.name.value] = true - } - - for variableDef in variableDefs { + for variableDef in operation.variableDefinitions { let variableName = variableDef.variable.name.value - - if variableNameUsed[variableName] != true { + if !variableNameUsed.contains(variableName) { context.report( error: GraphQLError( message: operation.name.map { @@ -47,7 +32,6 @@ func NoUnusedVariablesRule(context: ValidationContext) -> Visitor { ) } } - return .continue } ) diff --git a/Sources/GraphQL/Validation/Validate.swift b/Sources/GraphQL/Validation/Validate.swift index 5805f4c6..7559b16e 100644 --- a/Sources/GraphQL/Validation/Validate.swift +++ b/Sources/GraphQL/Validation/Validate.swift @@ -240,11 +240,10 @@ public final class ValidationContext { } if let variable = node as? Variable { - usages - .append(VariableUsage( - node: variable, - type: typeInfo.inputType - )) + usages.append(VariableUsage( + node: variable, + type: typeInfo.inputType + )) } return .continue diff --git a/Tests/GraphQLTests/ValidationTests/NoUnusedVariablesRuleTests.swift b/Tests/GraphQLTests/ValidationTests/NoUnusedVariablesRuleTests.swift index 0f26f03f..78adf0fc 100644 --- a/Tests/GraphQLTests/ValidationTests/NoUnusedVariablesRuleTests.swift +++ b/Tests/GraphQLTests/ValidationTests/NoUnusedVariablesRuleTests.swift @@ -256,4 +256,14 @@ class NoUnusedVariablesRuleTests: ValidationTestCase { message: #"Variable "$a" is never used in operation "Bar"."# ) } + + func testVariableUsedInsideObject() throws { + try assertValid( + """ + query Foo($a: String) { + field(object: { a: $a }) + } + """ + ) + } } From 8a1c8f32146ba54d08e201dfca9fe3c335b5f71e Mon Sep 17 00:00:00 2001 From: Jay Herron Date: Mon, 6 Nov 2023 12:52:39 -0700 Subject: [PATCH 2/2] test: Adds test of unhappy path --- .../NoUnusedVariablesRuleTests.swift | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/Tests/GraphQLTests/ValidationTests/NoUnusedVariablesRuleTests.swift b/Tests/GraphQLTests/ValidationTests/NoUnusedVariablesRuleTests.swift index 78adf0fc..9de2a193 100644 --- a/Tests/GraphQLTests/ValidationTests/NoUnusedVariablesRuleTests.swift +++ b/Tests/GraphQLTests/ValidationTests/NoUnusedVariablesRuleTests.swift @@ -266,4 +266,20 @@ class NoUnusedVariablesRuleTests: ValidationTestCase { """ ) } + + func testVariableUnusedInsideObject() throws { + let errors = try assertInvalid( + errorCount: 1, + query: """ + query Foo($a: String, $b: String) { + field(object: { a: $a }) + } + """ + ) + + try assertValidationError( + error: errors[0], line: 1, column: 23, + message: #"Variable "$b" is never used in operation "Foo"."# + ) + } }