Skip to content
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

Can't update the RequestBodyCreator for WebSocketTransport #1444

Closed
lowki93 opened this issue Oct 9, 2020 · 3 comments
Closed

Can't update the RequestBodyCreator for WebSocketTransport #1444

lowki93 opened this issue Oct 9, 2020 · 3 comments

Comments

@lowki93
Copy link

lowki93 commented Oct 9, 2020

Bug report

We use WebSocketTransport and we need to override the requestBodyCreator parameters.
We define CustomBodyCreator:

struct CustomBodyCreator: RequestBodyCreator {
  
  public func requestBody<Operation: GraphQLOperation>(
    for operation: Operation,
    sendOperationIdentifiers: Bool
  ) -> GraphQLMap {
    .....
  }
  
}

we put it when we initialize the WebSocketTransport:

 WebSocketTransport(request: request, requestBodyCreator: CustomBodyCreator())

The method call in WebSocketTransport don't use the method define in the procotol :

 func sendHelper<Operation: GraphQLOperation>(operation: Operation, resultHandler: @escaping (_ result: Result<JSONObject, Error>) -> Void) -> String? {
    let body = requestBodyCreator.requestBody(for: operation, sendOperationIdentifiers: self.sendOperationIdentifiers)
    ....
    return sequenceNumber
  }

But requestBody from our struct is never call.

There is a an extension for RequestBodyCreator that define the function in the protocol :

extension RequestBodyCreator {
  /// Creates a `GraphQLMap` out of the passed-in operation
  ///
  /// - Parameters:
  ///   - operation: The operation to use
  ///   - sendOperationIdentifiers: Whether or not to send operation identifiers. Defaults to false.
  ///   - sendQueryDocument: Whether or not to send the full query document. Defaults to true.
  ///   - autoPersistQuery: Whether to use auto-persisted query information. Defaults to false.
  /// - Returns: The created `GraphQLMap`
  public func requestBody<Operation: GraphQLOperation>(for operation: Operation,
                                                       sendOperationIdentifiers: Bool = false,
                                                       sendQueryDocument: Bool = true,
                                                       autoPersistQuery: Bool = false) -> GraphQLMap {
.....
}

Why can put this default implementation in ApolloRequestBodyCreator directly ?

// Helper struct to create requests independently of HTTP operations.
public struct ApolloRequestBodyCreator: RequestBodyCreator {
  // Internal init methods cannot be used in public methods
  public init() { }
}

Why we can't override this ?

Versions

Please fill in the versions you're currently using:

  • apollo-ios SDK version: 0.34.1
  • Xcode version: 12.0.1
  • Swift version: 5.2

Further details

We need to override this to make WebSocketTransportwith AppSyncfrom AWS

@designatednerd
Copy link
Contributor

Hi, apologies for the delay, I was under the weather.

You can't override ApolloRequestBodyCreator because it's not intended to be overridden - that's why RequestBodyCreator is a protocol with a default implementation: That way your custom implementation can use the default implementation without having to implement it yourself if you don't need to.

That being said, there've been some changes to this class recently (mostly, removing all the stuff that was going on for uploads) that might make changes make sense.

I'm not sure why your custom implementation wouldn't be getting called - I would double check that the request creator is being passed in everywhere you expect it to be by setting some breakpoints and validating whether you've got your CustomRequestCreator or an ApolloRequestBodyCreator, and then working backwards to try to figure out where things are going wrong. If it is the custom request creator, that's definitely a bug.

@designatednerd
Copy link
Contributor

OK updating a test, I am now seeing the issue you're having - looks like something I gave literally an entire talk on just came to bite me because I have angered the gods of iOS development 🤦‍♀️

The issue is the default implementation is getting called even if it's overridden when the only type the compiler knows about is the protocol itself.

Unfortunately I think the approach in #1448 is going to break things too hard - it removes the ability for anyone to use the default implementation at all. I will make some changes with some tests to validate it works though.

@designatednerd
Copy link
Contributor

This has shipped with 0.35.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants