GraphQLSwift / GraphQL

The Swift GraphQL implementation for macOS and Linux
MIT License
938 stars 72 forks source link

Handle the case where a optional variable is omitted in a query #17

Closed sportlabsMike closed 7 years ago

sportlabsMike commented 7 years ago

A query containing an optional variable which is then omitted in 'variables' causes a crash at line 84 of Execution/Values.swift

For example:

query FetchHeroByEpisodeQuery($episode: String) { hero(episode: $episode) { name } }

The additional unit test exposed the crash.

I have fixed by returning Map.null from getVariableValue when the input is .null and !(inputType is GraphQLNonNull)

The crash was:

fatal error: unexpectedly found nil while unwrapping an Optional value 2017-06-15 11:27:52.233296+0100 xctest[66184:54165053] fatal error: unexpectedly found nil while unwrapping an Optional value Current stack trace: 0 libswiftCore.dylib 0x00000001052fd130 swift_reportError + 129 1 libswiftCore.dylib 0x0000000105319b50 _swift_stdlib_reportFatalError + 60 2 libswiftCore.dylib 0x000000010510a250 specialized specialized StaticString.withUTF8Buffer ((UnsafeBufferPointer) -> A) -> A + 342 3 libswiftCore.dylib 0x0000000105284e90 partial apply for (_fatalErrorMessage(StaticString, StaticString, file : StaticString, line : UInt, flags : UInt32) -> Never).(closure #2) + 109 4 libswiftCore.dylib 0x000000010510a250 specialized specialized StaticString.withUTF8Buffer ((UnsafeBufferPointer) -> A) -> A + 342 5 libswiftCore.dylib 0x00000001052379a0 specialized fatalErrorMessage(StaticString, StaticString, file : StaticString, line : UInt, flags : UInt32) -> Never + 96 6 GraphQL 0x000000010159ec10 getVariableValue(schema : GraphQLSchema, definitionAST : VariableDefinition, input : Map) throws -> Map + 4910 7 GraphQL 0x000000010159e5c0 (getVariableValues(schema : GraphQLSchema, definitionASTs : [VariableDefinition], inputs : [String : Map]) throws -> [String : Map]).(closure #1) + 922 8 GraphQL 0x00000001015a11b0 thunk + 56 9 GraphQL 0x00000001015a1290 partial apply for thunk + 103 10 libswiftCore.dylib 0x00000001051bc6b0 Sequence.reduce (A1, (A1, A.Iterator.Element) throws -> A1) throws -> A1 + 574 11 GraphQL 0x000000010159e3a0 getVariableValues(schema : GraphQLSchema, definitionASTs : [VariableDefinition], inputs : [String : Map]) throws -> [String : Map] + 390 12 GraphQL 0x00000001015896c0 buildExecutionContext(queryStrategy : FieldExecutionStrategy, mutationStrategy : FieldExecutionStrategy, subscriptionStrategy : FieldExecutionStrategy, schema : GraphQLSchema, documentAST : Document, rootValue : Any, contextValue : Any, rawVariableValues : [String : Map], operationName : String?) throws -> ExecutionContext + 5184 13 GraphQL 0x0000000101588230 execute(queryStrategy : FieldExecutionStrategy, mutationStrategy : FieldExecutionStrategy, subscriptionStrategy : FieldExecutionStrategy, schema : GraphQLSchema, documentAST : Document, rootValue : Any, contextValue : Any, variableValues : [String : Map], operationName : String?) throws -> Map + 842 14 GraphQL 0x00000001015791f0 graphql(queryStrategy : FieldExecutionStrategy, mutationStrategy : FieldExecutionStrategy, subscriptionStrategy : FieldExecutionStrategy, schema : GraphQLSchema, request : String, rootValue : Any, contextValue : Any, variableValues : [String : Map], operationName : String?) throws -> Map + 1444 15 GraphQLTests 0x0000000101431350 StarWarsQueryTests.testOptionalVariable() throws -> () + 1544 16 GraphQLTests 0x0000000101432a30 @objc StarWarsQueryTests.testOptionalVariable() throws -> () + 50 17 CoreFoundation 0x00007fffc93e8020 invoking + 140 18 CoreFoundation 0x00007fffc93e7e10 -[NSInvocation invoke] + 289 19 XCTest 0x00000001000bccfa 24-[XCTestCase invokeTest]_block_invoke.234 + 50 20 XCTest 0x0000000100102fc1 -[XCTMemoryChecker _assertInvalidObjectsDeallocatedAfterScope:] + 37 21 XCTest 0x00000001000bc657 __24-[XCTestCase invokeTest]_block_invoke_2 + 665 22 XCTest 0x00000001000fab4c -[XCTestContext performInScope:] + 190 23 XCTest 0x00000001000bc546 -[XCTestCase invokeTest] + 254 24 XCTest 0x00000001000bce7e -[XCTestCase performTest:] + 565 25 XCTest 0x00000001000b9efa 27-[XCTestSuite performTest:]_block_invoke + 300 26 XCTest 0x00000001000b9bf9 -[XCTestSuite _performProtectedSectionForTest:testSection:] + 29 27 XCTest 0x00000001000b9d26 -[XCTestSuite performTest:] + 214 28 XCTest 0x00000001000b9efa 27-[XCTestSuite performTest:]_block_invoke + 300 29 XCTest 0x00000001000b9bf9 -[XCTestSuite _performProtectedSectionForTest:testSection:] + 29 30 XCTest 0x00000001000b9d26 -[XCTestSuite performTest:] + 214 31 XCTest 0x00000001000b9efa __27-[XCTestSuite performTest:]_block_invoke + 300 32 XCTest 0x00000001000b9bf9 -[XCTestSuite _performProtectedSectionForTest:testSection:] + 29 33 XCTest 0x00000001000b9d26 -[XCTestSuite performTest:] + 214 34 XCTest 0x000000010010db24 44-[XCTTestRunSession runTestsAndReturnError:]_block_invoke + 40 35 XCTest 0x00000001000d050c -[XCTestObservationCenter _observeTestExecutionForBlock:] + 587 36 XCTest 0x000000010010d8d1 -[XCTTestRunSession runTestsAndReturnError:] + 281 37 XCTest 0x00000001000a59ac -[XCTestDriver runTestsAndReturnError:] + 254 38 XCTest 0x00000001000fcfb8 _XCTestMain + 773 39 xctest 0x000000010000148d + 5261 40 libdyld.dylib 0x00007fffdf101234 start + 1

codecov-io commented 7 years ago

Codecov Report

Merging #17 into master will increase coverage by 0.14%. The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #17      +/-   ##
==========================================
+ Coverage   79.98%   80.12%   +0.14%     
==========================================
  Files          73       73              
  Lines        8712     8755      +43     
==========================================
+ Hits         6968     7015      +47     
+ Misses       1744     1740       -4
Impacted Files Coverage Δ
Sources/GraphQL/Execution/Values.swift 49.6% <100%> (+2.4%) :arrow_up:
...raphQLTests/StarWarsTests/StarWarsQueryTests.swift 96.8% <100%> (+0.26%) :arrow_up:
Sources/GraphQL/Utilities/IsValidValue.swift 31.74% <0%> (+3.17%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 513c473...5c51a2e. Read the comment docs.

paulofaria commented 7 years ago

Thanks man!