-
Notifications
You must be signed in to change notification settings - Fork 749
Added correct way of default parameters #804
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 5 commits
e8085f0
6e4cc7f
4ef637c
04fd12f
8db2e1c
686fb75
28bfb0b
3036c97
8574ab9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,8 +10,9 @@ import XCTest | |
| @testable import Apollo | ||
| import StarWarsAPI | ||
|
|
||
| class MultipartFormDataTests: XCTestCase { | ||
| private let requestCreator = ApolloRequestCreator() | ||
| class RequestCreatorTests: XCTestCase { | ||
| private let customRequestCreator = TestCustomRequestCreator() | ||
| private let apolloRequestCreator = ApolloRequestCreator() | ||
|
|
||
| private func checkString(_ string: String, | ||
| includes expectedString: String, | ||
|
|
@@ -158,15 +159,15 @@ Charlie file content. | |
| XCTAssertEqual(stringToCompare, expectedString) | ||
| } | ||
|
|
||
| func testSingleFileWithRequestCreator() throws { | ||
| func testSingleFileWithApolloRequestCreator() throws { | ||
| let alphaFileUrl = self.fileURLForFile(named: "a", extension: "txt") | ||
|
|
||
| let alphaFile = GraphQLFile(fieldName: "upload", | ||
| originalName: "a.txt", | ||
| mimeType: "text/plain", | ||
| fileURL: alphaFileUrl) | ||
|
|
||
| let data = try requestCreator.requestMultipartFormData( | ||
| let data = try apolloRequestCreator.requestMultipartFormData( | ||
| for: HeroNameQuery(), | ||
| files: [alphaFile!], | ||
| sendOperationIdentifiers: false, | ||
|
|
@@ -213,8 +214,8 @@ Alpha file content. | |
| self.checkString(stringToCompare, includes: expectedEndString) | ||
| } | ||
| } | ||
| func testMultipleFilesWithRequestCreator() throws { | ||
|
|
||
| func testMultipleFilesWithApolloRequestCreator() throws { | ||
| let alphaFileURL = self.fileURLForFile(named: "a", extension: "txt") | ||
| let alphaFile = GraphQLFile(fieldName: "uploads", | ||
| originalName: "a.txt", | ||
|
|
@@ -228,7 +229,7 @@ Alpha file content. | |
| fileURL: betaFileURL)! | ||
|
|
||
|
|
||
| let data = try requestCreator.requestMultipartFormData( | ||
| let data = try apolloRequestCreator.requestMultipartFormData( | ||
| for: HeroNameQuery(), | ||
| files: [alphaFile, betaFile], | ||
| sendOperationIdentifiers: false, | ||
|
|
@@ -283,4 +284,82 @@ Bravo file content. | |
| self.checkString(stringToCompare, includes: endString) | ||
| } | ||
| } | ||
|
|
||
| func testRequestBodyWithApolloRequestCreator() { | ||
| let query = HeroNameQuery() | ||
| let req = apolloRequestCreator.requestBody(for: query, sendOperationIdentifiers: false) | ||
|
|
||
| XCTAssertEqual(query.queryDocument, req["query"] as? String) | ||
| } | ||
|
|
||
| // MARK: - Custom request creator tests | ||
|
|
||
| func testSingleFileWithCustomRequestCreator() throws { | ||
| let alphaFileUrl = self.fileURLForFile(named: "a", extension: "txt") | ||
|
|
||
| let alphaFile = GraphQLFile(fieldName: "upload", | ||
| originalName: "a.txt", | ||
| mimeType: "text/plain", | ||
| fileURL: alphaFileUrl) | ||
|
|
||
| let data = try customRequestCreator.requestMultipartFormData( | ||
| for: HeroNameQuery(), | ||
| files: [alphaFile!], | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Probably worth
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is just copy/paste from other code made by you 🙈
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤦♀️Nah I'll fix it in a follow-up PR.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sowwy 🙈
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Haha, I'm the moron who did it first, no worries |
||
| sendOperationIdentifiers: false, | ||
| serializationFormat: JSONSerializationFormat.self, | ||
| manualBoundary: "TEST.BOUNDARY" | ||
| ) | ||
|
|
||
| let stringToCompare = try self.string(from: data) | ||
|
|
||
| print(stringToCompare) | ||
|
|
||
| if #available(iOS 11, macOS 13, tvOS 11, watchOS 4, *) { | ||
| let expectedString = """ | ||
| --TEST.BOUNDARY | ||
| Content-Disposition: form-data; name="test_variables" | ||
|
|
||
| {"episode":null} | ||
| --TEST.BOUNDARY | ||
| Content-Disposition: form-data; name="test_query" | ||
|
|
||
| query HeroName($episode: Episode) { hero(episode: $episode) { __typename name } } | ||
| --TEST.BOUNDARY | ||
| Content-Disposition: form-data; name="test_operationName" | ||
|
|
||
| HeroName | ||
| --TEST.BOUNDARY | ||
| Content-Disposition: form-data; name="upload"; filename="a.txt" | ||
| Content-Type: text/plain | ||
|
|
||
| Alpha file content. | ||
|
|
||
| --TEST.BOUNDARY-- | ||
| """ | ||
| XCTAssertEqual(stringToCompare, expectedString) | ||
| } else { | ||
| // Operation parameters may be in weird order, so let's at least check that the files and single parameter got encoded properly. | ||
| let expectedEndString = """ | ||
| --TEST.BOUNDARY | ||
| Content-Disposition: form-data; name="map" | ||
|
|
||
| {"0":["variables.upload"]} | ||
| --TEST.BOUNDARY | ||
| Content-Disposition: form-data; name="0"; filename="a.txt" | ||
| Content-Type: text/plain | ||
|
|
||
| Alpha file content. | ||
|
|
||
| --TEST.BOUNDARY-- | ||
| """ | ||
| self.checkString(stringToCompare, includes: expectedEndString) | ||
| } | ||
| } | ||
|
|
||
| func testRequestBodyWithCustomRequestCreator() { | ||
| let query = HeroNameQuery() | ||
| let req = customRequestCreator.requestBody(for: query, sendOperationIdentifiers: false) | ||
|
|
||
| XCTAssertEqual(query.queryDocument, req["test_query"] as? String) | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,62 @@ | ||
| // | ||
| // TestCustomRequestCreator.swift | ||
| // Apollo | ||
| // | ||
| // Created by Kim de Vos on 02/10/2019. | ||
| // Copyright © 2019 Apollo GraphQL. All rights reserved. | ||
| // | ||
|
|
||
| import Apollo | ||
|
|
||
| fileprivate struct TestCustomRequestCreator: RequestCreator { | ||
| public func requestBody<Operation: GraphQLOperation>(for operation: Operation, sendOperationIdentifiers: Bool) -> GraphQLMap { | ||
| var body: GraphQLMap = [ | ||
| "test_variables": operation.variables, | ||
| "test_operationName": operation.operationName, | ||
| ] | ||
|
|
||
| if sendOperationIdentifiers { | ||
| guard let operationIdentifier = operation.operationIdentifier else { | ||
| preconditionFailure("To send operation identifiers, Apollo types must be generated with operationIdentifiers") | ||
| } | ||
|
|
||
| body["test_id"] = operationIdentifier | ||
| } else { | ||
| body["test_query"] = operation.queryDocument | ||
| } | ||
|
|
||
| return body | ||
| } | ||
|
|
||
| public func requestMultipartFormData<Operation: GraphQLOperation>(for operation: Operation, | ||
| files: [GraphQLFile], | ||
| sendOperationIdentifiers: Bool, | ||
| serializationFormat: JSONSerializationFormat.Type, | ||
| manualBoundary: String?) throws -> MultipartFormData { | ||
| let formData: MultipartFormData | ||
|
|
||
| if let boundary = manualBoundary { | ||
| formData = MultipartFormData(boundary: boundary) | ||
| } else { | ||
| formData = MultipartFormData() | ||
| } | ||
|
|
||
| let fields = requestBody(for: operation, sendOperationIdentifiers: false) | ||
| for (name, data) in fields { | ||
| if let data = data as? GraphQLMap { | ||
| let data = try serializationFormat.serialize(value: data) | ||
| formData.appendPart(data: data, name: name) | ||
| } else if let data = data as? String { | ||
| try formData.appendPart(string: data, name: name) | ||
| } else { | ||
| try formData.appendPart(string: data.debugDescription, name: name) | ||
| } | ||
| } | ||
|
|
||
| files.forEach { | ||
| formData.appendPart(inputStream: $0.inputStream, contentLength: $0.contentLength, name: $0.fieldName, contentType: $0.mimeType, filename: $0.originalName) | ||
| } | ||
|
|
||
| return formData | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably worth adding one test to this file where
sendOperationIdentifersistruejust to make sure that's actually including the operation identifierThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, the default implementation here was actually false.
Maybe that is for another PR?
I can open an issue to you can track it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just not to add something that is not related to this PR and RequestCreator? 😁
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I'll add it in a follow-up PR - you don't need to open an issue for it, I'll leave this comment open