Apollo-ios: Expose URL response in HTTPNetworkTransportGraphQLErrorDelegate

[erudel]

Hi,

I am trying to implement a delegate to intercept responses that contain a GraphQL error and it seems that HTTPNetworkTransportGraphQLErrorDelegate would be the perfect place to do that.
In my particular use case, it would be also helpful to have access to the headers from the (HTTP) response to perform some additional logging. However, I noticed that the response is not exposed in the delegate method and instead other higher level objects are passed.

Do you think it would be reasonable to also expose the response? Or is there another way to get this information?

If you think this makes sense, I would be happy to start working on a PR to address this issue.

[designatednerd]

I think what you're looking for is the HTTPNetworkTransportTaskCompletedDelegate - that gives you the raw data back from the request.

[erudel]

Thank you for your reply @designatednerd. I looked into that delegate, and as you mentioned, you get the raw data at that point so we don't know yet whether the response contains a GraphQL error or not.
Is there a way to construct the result and inspect errors from the task completed delegate?

[designatednerd]

The design of these delegates assumes that if you have a GraphQL error, the information you'd need to deal with a GraphQL error is actually included in the error itself. This is a best practice if you're going to be propagating errors using the GraphQL error system.

I'm looking longer-term at a different system that would allow easier hooking into responses at any point in the stack, but that's going to be...a while.

[erudel]

I agree with you on the idea that the information provided by these delegates should be enough to deal with error handling on the client side.

The reason I was looking to expose the response is that it would allow to perform error tracing (by looking at the HTTP headers) and help pinpoint the potential breakage on the backend side.

Maybe a better way to solve this issue would be to include the trace id in the error, rather than exposing the raw response and looking it up there. I'm curious to hear your thoughts on this approach.

[designatednerd]

Yep, that sort of thing is exactly why the error can contain arbitrary JSON.

[erudel]

Sounds good, thank you very much for taking the time to answer this and steer me in the right direction. Feel free to close the issue since the original question is not relevant anymore

[designatednerd]

You're welcome!

[xma000]

@erudel I believe you can implement a delegate that implements both HTTPNetworkTransportTaskCompletedDelegate and HTTPNetworkTransportGraphQLErrorDelegate. You will have the both the headers and the GraphQL error available for logging.

@designatednerd Can you confirm this solution will work?

[designatednerd]

That would work to a point - the problem is that with multiple in-flight requests it may not be obvious which headers correspond with the GraphQL error.

This is part of why it's better to keep the error information returned from the server within the actual error object: It prevents any confusion in terms of what data is related to which request.

[xma000]

Got it. The single delegate instance is set to HTTPNetworkTransport which may process many requests in parallel so it won't work.

[bharath2020]

the problem is that with multiple in-flight requests it may not be obvious which headers correspond with the GraphQL error.

@designatednerd You brought up the right point. This is indeed the limitation with the current API design of the delegates.

For the particular case of "Error tracing" which @erudel mentioned, it is fair to have "traceId" in the error payload. However, there are cases like "request tracing" in the event a request was a success (but incorrect data) or failure. Given that the HTTPNetworkTransportGraphQLErrorDelegate delegate does not provide its corresponding response headers, it poses a limitation where we could not access the metadata about the entire request. Given that Response Format in GraphQL specification only has the option to include two top-level fields "errors" and "data", we would have to rely on HTTP Response headers to store meta-information about the overall request and we would need to have it accessible when we are looking GraphQL result in both successes as well as failure cases.

HTTPNetworkTransportTaskCompletedDelegate allows us to access the metadata of the request, we would only have access to raw payload and we definitely would not want to parse the payload just to understand if the result was a success or a failure.

[designatednerd]

There's an extensions property Apollo supports which allows you to include arbitrary JSON along with your error. This is where we'd recommend placing any additional information.

[xma000]

I have opened a new issue https://github.com/apollographql/apollo-ios/issues/1132 with a proposal.

I don't like the extensions idea because the information we wanted to access belongs to the HTTP layer and is already sent to the client. We just need a proper mechanism to access it.

[bharath2020]

@designatednerd I second what @xma000 said. The metadata corresponds to the entire request irrespective of its success or failure. "extension" property as you suggested would be available only when there is a failure in the request. We would still be left with no option when the request succeeds, in which case "errors" will be null by design.