From 6bf2570ba2aa7149b10415b5560cb7245285e05a Mon Sep 17 00:00:00 2001 From: Jay Herron Date: Wed, 16 Jun 2021 10:56:56 -0600 Subject: [PATCH] Preserves result order Fixes https://github.com/GraphQLSwift/GraphQL/issues/60 --- Package.resolved | 9 +++ Package.swift | 2 + Sources/GraphQL/Execution/Execute.swift | 31 ++++----- Sources/GraphQL/Map/Map.swift | 65 +++++++++++++++---- Sources/GraphQL/Map/MapCoder.swift | 20 +++--- Sources/GraphQL/Map/MapSerialization.swift | 15 +++-- Sources/GraphQL/Subscription/Subscribe.swift | 3 +- .../GraphQL/Utilities/NIO+Extensions.swift | 19 ++++++ Sources/GraphQL/Utilities/ValueFromAST.swift | 4 +- .../StarWarsTests/StarWarsQueryTests.swift | 48 ++++++++++++++ 10 files changed, 171 insertions(+), 45 deletions(-) diff --git a/Package.resolved b/Package.resolved index f3e6804f..98071054 100644 --- a/Package.resolved +++ b/Package.resolved @@ -1,6 +1,15 @@ { "object": { "pins": [ + { + "package": "swift-collections", + "repositoryURL": "https://github.com/apple/swift-collections", + "state": { + "branch": null, + "revision": "d45e63421d3dff834949ac69d3c37691e994bd69", + "version": "0.0.3" + } + }, { "package": "swift-nio", "repositoryURL": "https://github.com/apple/swift-nio.git", diff --git a/Package.swift b/Package.swift index 317bda88..e1afde27 100644 --- a/Package.swift +++ b/Package.swift @@ -8,12 +8,14 @@ let package = Package( ], dependencies: [ .package(url: "https://github.com/apple/swift-nio.git", .upToNextMajor(from: "2.10.1")), + .package(url: "https://github.com/apple/swift-collections", .upToNextMajor(from: "0.0.3")), ], targets: [ .target( name: "GraphQL", dependencies: [ .product(name: "NIO", package: "swift-nio"), + .product(name: "OrderedCollections", package: "swift-collections"), ] ), .testTarget(name: "GraphQLTests", dependencies: ["GraphQL"]), diff --git a/Sources/GraphQL/Execution/Execute.swift b/Sources/GraphQL/Execution/Execute.swift index 26105f79..6ab5c1aa 100644 --- a/Sources/GraphQL/Execution/Execute.swift +++ b/Sources/GraphQL/Execution/Execute.swift @@ -1,5 +1,6 @@ import Dispatch import NIO +import OrderedCollections /** * Terminology @@ -97,8 +98,8 @@ public protocol FieldExecutionStrategy { parentType: GraphQLObjectType, sourceValue: Any, path: IndexPath, - fields: [String: [Field]] - ) throws -> Future<[String: Any]> + fields: OrderedDictionary + ) throws -> Future> } public protocol MutationFieldExecutionStrategy: FieldExecutionStrategy {} @@ -117,9 +118,9 @@ public struct SerialFieldExecutionStrategy: QueryFieldExecutionStrategy, Mutatio parentType: GraphQLObjectType, sourceValue: Any, path: IndexPath, - fields: [String: [Field]] - ) throws -> Future<[String: Any]> { - var results = [String: Future]() + fields: OrderedDictionary + ) throws -> Future> { + var results = OrderedDictionary>() try fields.forEach { field in let fieldASTs = field.value @@ -169,15 +170,16 @@ public struct ConcurrentDispatchFieldExecutionStrategy: QueryFieldExecutionStrat parentType: GraphQLObjectType, sourceValue: Any, path: IndexPath, - fields: [String: [Field]] - ) throws -> Future<[String: Any]> { + fields: OrderedDictionary + ) throws -> Future> { let resultsQueue = DispatchQueue( label: "\(dispatchQueue.label) results", qos: dispatchQueue.qos ) let group = DispatchGroup() - var results: [String: Future] = [:] + // preserve field order by assigning to null and filtering later + var results: OrderedDictionary?> = fields.mapValues { _ -> Future? in return nil } var err: Error? = nil fields.forEach { field in @@ -213,7 +215,7 @@ public struct ConcurrentDispatchFieldExecutionStrategy: QueryFieldExecutionStrat throw error } - return results.flatten(on: exeContext.eventLoopGroup) + return results.compactMapValues({ $0 }).flatten(on: exeContext.eventLoopGroup) } } @@ -323,7 +325,6 @@ func execute( // errors: executeErrors, // result: result // ) - return result } } catch let error as GraphQLError { @@ -417,9 +418,9 @@ func executeOperation( exeContext: ExecutionContext, operation: OperationDefinition, rootValue: Any -) throws -> Future<[String: Any]> { +) throws -> Future> { let type = try getOperationRootType(schema: exeContext.schema, operation: operation) - var inputFields: [String : [Field]] = [:] + var inputFields: OrderedDictionary = [:] var visitedFragmentNames: [String : Bool] = [:] let fields = try collectFields( @@ -494,9 +495,9 @@ func collectFields( exeContext: ExecutionContext, runtimeType: GraphQLObjectType, selectionSet: SelectionSet, - fields: inout [String: [Field]], + fields: inout OrderedDictionary, visitedFragmentNames: inout [String: Bool] -) throws -> [String: [Field]] { +) throws -> OrderedDictionary { var visitedFragmentNames = visitedFragmentNames for selection in selectionSet.selections { @@ -1109,7 +1110,7 @@ func completeObjectValue( } // Collect sub-fields to execute to complete this value. - var subFieldASTs: [String: [Field]] = [:] + var subFieldASTs: OrderedDictionary = [:] var visitedFragmentNames: [String: Bool] = [:] for fieldAST in fieldASTs { diff --git a/Sources/GraphQL/Map/Map.swift b/Sources/GraphQL/Map/Map.swift index 482fabea..8ea1690c 100644 --- a/Sources/GraphQL/Map/Map.swift +++ b/Sources/GraphQL/Map/Map.swift @@ -1,3 +1,5 @@ +import OrderedCollections + // MARK: MapError public enum MapError : Error { @@ -14,7 +16,7 @@ public enum Map { case number(Number) case string(String) case array([Map]) - case dictionary([String: Map]) + case dictionary(OrderedDictionary) public static func int(_ value: Int) -> Map { return .number(Number(value)) @@ -58,7 +60,7 @@ extension Map { self = .array(array) } - public init(_ dictionary: [String: Map]) { + public init(_ dictionary: OrderedDictionary) { self = .dictionary(dictionary) } @@ -86,7 +88,7 @@ extension Map { self = array.map({ Map($0) }) ?? .null } - public init(_ dictionary: [String: Map]?) { + public init(_ dictionary: OrderedDictionary?) { self = dictionary.map({ Map($0) }) ?? .null } } @@ -107,8 +109,8 @@ public func map(from value: Any?) throws -> Map { } if - let value = value as? [String: Any], - let dictionary: [String: Map] = try? value.reduce(into: [:], { result, pair in + let value = value as? OrderedDictionary, + let dictionary: OrderedDictionary = try? value.reduce(into: [:], { result, pair in result[pair.key] = try map(from: pair.value) }) { @@ -152,7 +154,7 @@ extension Map { self = .string(string) case let array as [Map]: self = .array(array) - case let dictionary as [String: Map]: + case let dictionary as OrderedDictionary: self = .dictionary(dictionary) default: throw MapError.incompatibleType @@ -243,7 +245,7 @@ extension Map { return try? arrayValue() } - public var dictionary: [String: Map]? { + public var dictionary: OrderedDictionary? { return try? dictionaryValue() } } @@ -372,7 +374,7 @@ extension Map { } } - public func dictionaryValue(converting: Bool = false) throws -> [String: Map] { + public func dictionaryValue(converting: Bool = false) throws -> OrderedDictionary { guard converting else { return try get() } @@ -487,7 +489,7 @@ extension Map { if let existingDictionary = dictionary[key]?.dictionary, let newDictionary = newValue.dictionary, merging { - var combinedDictionary: [String: Map] = [:] + var combinedDictionary: OrderedDictionary = [:] for (key, value) in existingDictionary { combinedDictionary[key] = value @@ -617,8 +619,20 @@ extension Map : Codable { else if let array = try? container.decode([Map].self) { self = .array(array) } - - else if let dictionary = try? container.decode([String: Map].self) { + + else if let _ = try? container.decode([String: Map].self) { + // Override OrderedDictionary default (unkeyed alternating key-value) + // Instead decode as a keyed container (like normal Dictionary) but use the order of the input + let container = try decoder.container(keyedBy: _DictionaryCodingKey.self) + var orderedDictionary: OrderedDictionary = [:] + for key in container.allKeys { + let value = try container.decode(Map.self, forKey: key) + orderedDictionary[key.stringValue] = value + } + self = .dictionary(orderedDictionary) + } + + else if let dictionary = try? container.decode(OrderedDictionary.self) { self = .dictionary(dictionary) } @@ -642,9 +656,32 @@ extension Map : Codable { case let .array(array): try container.encode(array) case let .dictionary(dictionary): - try container.encode(dictionary) + // Override OrderedDictionary default (unkeyed alternating key-value) + // Instead decode as a keyed container (like normal Dictionary) in the order of our OrderedDictionary + var container = encoder.container(keyedBy: _DictionaryCodingKey.self) + for (key, value) in dictionary { + let codingKey = _DictionaryCodingKey(stringValue: key)! + try container.encode(value, forKey: codingKey) + } } } + + /// A wrapper for dictionary keys which are Strings or Ints. + /// This is copied from Swift core: https://github.com/apple/swift/blob/256a9c5ad96378daa03fa2d5197b4201bf16db27/stdlib/public/core/Codable.swift#L5508 + internal struct _DictionaryCodingKey: CodingKey { + internal let stringValue: String + internal let intValue: Int? + + internal init?(stringValue: String) { + self.stringValue = stringValue + self.intValue = Int(stringValue) + } + + internal init?(intValue: Int) { + self.stringValue = "\(intValue)" + self.intValue = intValue + } + } } // MARK: Equatable @@ -738,7 +775,7 @@ extension Map : ExpressibleByArrayLiteral { extension Map : ExpressibleByDictionaryLiteral { public init(dictionaryLiteral elements: (String, Map)...) { - var dictionary = [String: Map](minimumCapacity: elements.count) + var dictionary = OrderedDictionary(minimumCapacity: elements.count) for (key, value) in elements { dictionary[key] = value @@ -847,7 +884,7 @@ extension Map { } } - func serialize(dictionary: [String: Map]) -> String { + func serialize(dictionary: OrderedDictionary) -> String { var string = "{" var index = 0 diff --git a/Sources/GraphQL/Map/MapCoder.swift b/Sources/GraphQL/Map/MapCoder.swift index 94787444..f80301ac 100644 --- a/Sources/GraphQL/Map/MapCoder.swift +++ b/Sources/GraphQL/Map/MapCoder.swift @@ -1,14 +1,16 @@ import CoreFoundation import Foundation +import OrderedCollections -/// A marker protocol used to determine whether a value is a `String`-keyed `Dictionary` + +/// A marker protocol used to determine whether a value is a `String`-keyed `OrderedDictionary` /// containing `Encodable` values (in which case it should be exempt from key conversion strategies). /// fileprivate protocol _MapStringDictionaryEncodableMarker { } -extension Dictionary : _MapStringDictionaryEncodableMarker where Key == String, Value: Encodable { } +extension OrderedDictionary : _MapStringDictionaryEncodableMarker where Key == String, Value: Encodable { } -/// A marker protocol used to determine whether a value is a `String`-keyed `Dictionary` +/// A marker protocol used to determine whether a value is a `String`-keyed `OrderedDictionary` /// containing `Decodable` values (in which case it should be exempt from key conversion strategies). /// /// The marker protocol also provides access to the type of the `Decodable` values, @@ -18,7 +20,7 @@ fileprivate protocol _MapStringDictionaryDecodableMarker { static var elementType: Decodable.Type { get } } -extension Dictionary : _MapStringDictionaryDecodableMarker where Key == String, Value: Decodable { +extension OrderedDictionary : _MapStringDictionaryDecodableMarker where Key == String, Value: Decodable { static var elementType: Decodable.Type { return Value.self } } @@ -798,7 +800,7 @@ extension _MapEncoder { } } - fileprivate func box(_ dict: [String : Encodable]) throws -> NSObject? { + fileprivate func box(_ dict: OrderedDictionary) throws -> NSObject? { let depth = self.storage.count let result = self.storage.pushKeyedContainer() do { @@ -845,7 +847,7 @@ extension _MapEncoder { // MapSerialization can consume NSDecimalNumber values. return NSDecimalNumber(decimal: value as! Decimal) } else if value is _MapStringDictionaryEncodableMarker { - return try box((value as Any) as! [String : Encodable]) + return try box((value as Any) as! OrderedDictionary) } #else @@ -862,7 +864,7 @@ extension _MapEncoder { // MapSerialization can consume NSDecimalNumber values. return NSDecimalNumber(decimal: value as! Decimal) } else if value is _MapStringDictionaryEncodableMarker { - return try box((value as Any) as! [String : Encodable]) + return try box((value as Any) as! OrderedDictionary) } #endif @@ -2405,11 +2407,11 @@ extension _MapDecoder { return Decimal(doubleValue) } } - + fileprivate func unbox(_ value: Any, as type: _MapStringDictionaryDecodableMarker.Type) throws -> T? { guard !(value is NSNull) else { return nil } - var result = [String : Any]() + var result: OrderedDictionary = [:] guard let dict = value as? NSDictionary else { throw DecodingError._typeMismatch(at: self.codingPath, expectation: type, reality: value) } diff --git a/Sources/GraphQL/Map/MapSerialization.swift b/Sources/GraphQL/Map/MapSerialization.swift index c1bc912e..b3ddda51 100644 --- a/Sources/GraphQL/Map/MapSerialization.swift +++ b/Sources/GraphQL/Map/MapSerialization.swift @@ -1,4 +1,5 @@ import Foundation +import OrderedCollections public struct MapSerialization { static func map(with object: NSObject) throws -> Map { @@ -13,14 +14,13 @@ public struct MapSerialization { let array: [Map] = try array.map { value in try self.map(with: value as! NSObject) } - return .array(array) case let dictionary as NSDictionary: - let dictionary: [String : Map] = try dictionary.reduce(into: [:]) { (dictionary, pair) in + // Extract from an unordered dictionary, using NSDictionary extraction order + let orderedDictionary: OrderedDictionary = try dictionary.reduce(into: [:]) { (dictionary, pair) in dictionary[pair.key as! String] = try self.map(with: pair.value as! NSObject) } - - return .dictionary(dictionary) + return .dictionary(orderedDictionary) default: throw EncodingError.invalidValue( object, @@ -45,7 +45,12 @@ public struct MapSerialization { case let .array(array): return try array.map({ try object(with: $0) }) as NSArray case let .dictionary(dictionary): - return try dictionary.mapValues({ try object(with: $0) }) as NSDictionary + // Coerce to an unordered dictionary + var unorderedDictionary: [String: NSObject] = [:] + for (key, value) in dictionary { + try unorderedDictionary[key] = object(with: value) + } + return unorderedDictionary as NSDictionary } } } diff --git a/Sources/GraphQL/Subscription/Subscribe.swift b/Sources/GraphQL/Subscription/Subscribe.swift index 030736af..a7a79394 100644 --- a/Sources/GraphQL/Subscription/Subscribe.swift +++ b/Sources/GraphQL/Subscription/Subscribe.swift @@ -1,4 +1,5 @@ import NIO +import OrderedCollections /** * Implements the "Subscribe" algorithm described in the GraphQL specification. @@ -168,7 +169,7 @@ func executeSubscription( // Get the first node let type = try getOperationRootType(schema: context.schema, operation: context.operation) - var inputFields: [String:[Field]] = [:] + var inputFields: OrderedDictionary = [:] var visitedFragmentNames: [String:Bool] = [:] let fields = try collectFields( exeContext: context, diff --git a/Sources/GraphQL/Utilities/NIO+Extensions.swift b/Sources/GraphQL/Utilities/NIO+Extensions.swift index 63edf3c6..4f5b200d 100644 --- a/Sources/GraphQL/Utilities/NIO+Extensions.swift +++ b/Sources/GraphQL/Utilities/NIO+Extensions.swift @@ -7,6 +7,7 @@ import Foundation import NIO +import OrderedCollections public typealias Future = EventLoopFuture @@ -38,6 +39,24 @@ extension Dictionary where Value : FutureType { } } } + +extension OrderedDictionary where Value : FutureType { + func flatten(on eventLoopGroup: EventLoopGroup) -> Future> { + let keys = self.keys + // create array of futures with (key,value) tuple + let futures: [Future<(Key, Value.Expectation)>] = self.map { element in + element.value.map(file: #file, line: #line) { (key: element.key, value: $0) } + } + // when all futures have succeeded convert tuple array back to dictionary + return EventLoopFuture.whenAllSucceed(futures, on: eventLoopGroup.next()).map { unorderedResult in + var result: OrderedDictionary = [:] + for key in keys { + result[key] = unorderedResult.first(where: { $0.0 == key })!.1 + } + return result + } + } +} extension Future { internal func flatMap( to type: T.Type = T.self, diff --git a/Sources/GraphQL/Utilities/ValueFromAST.swift b/Sources/GraphQL/Utilities/ValueFromAST.swift index 71b88a0b..65474fc0 100644 --- a/Sources/GraphQL/Utilities/ValueFromAST.swift +++ b/Sources/GraphQL/Utilities/ValueFromAST.swift @@ -1,3 +1,5 @@ +import OrderedCollections + /** * Produces a Map value given a GraphQL Value AST. * @@ -64,7 +66,7 @@ func valueFromAST(valueAST: Value?, type: GraphQLInputType, variables: [String: let fieldASTs = objectValue.fields.keyMap({ $0.name.value }) - return try .dictionary(fields.keys.reduce([:] as [String: Map]) { obj, fieldName in + return try .dictionary(fields.keys.reduce([:] as OrderedDictionary) { obj, fieldName in var obj = obj let field = fields[fieldName] let fieldAST = fieldASTs[fieldName] diff --git a/Tests/GraphQLTests/StarWarsTests/StarWarsQueryTests.swift b/Tests/GraphQLTests/StarWarsTests/StarWarsQueryTests.swift index 8510e464..5ce678fd 100644 --- a/Tests/GraphQLTests/StarWarsTests/StarWarsQueryTests.swift +++ b/Tests/GraphQLTests/StarWarsTests/StarWarsQueryTests.swift @@ -661,4 +661,52 @@ class StarWarsQueryTests : XCTestCase { let result = try graphql(schema: schema, request: query, eventLoopGroup: eventLoopGroup).wait() XCTAssertEqual(result, expected) } + + func testFieldOrderQuery() throws { + let eventLoopGroup = MultiThreadedEventLoopGroup(numberOfThreads: System.coreCount) + + defer { + XCTAssertNoThrow(try eventLoopGroup.syncShutdownGracefully()) + } + + XCTAssertEqual(try graphql( + schema: starWarsSchema, + request: """ + query HeroNameQuery { + hero { + id + name + } + } + """, + eventLoopGroup: eventLoopGroup + ).wait(), GraphQLResult( + data: [ + "hero": [ + "id": "2001", + "name": "R2-D2" + ], + ] + )) + + XCTAssertNotEqual(try graphql( + schema: starWarsSchema, + request: """ + query HeroNameQuery { + hero { + id + name + } + } + """, + eventLoopGroup: eventLoopGroup + ).wait(), GraphQLResult( + data: [ + "hero": [ + "name": "R2-D2", + "id": "2001" + ], + ] + )) + } }