[Networking Stack] additionalErrorInterceptor(for:) doesn't work when subclassing LegacyInterceptorProvider

[TizianoCoroneo]

The problem

The problem is: the additionalErrorInterceptor(for:) function is never called, if the InterceptorProvider class in use is a subclass of any InterceptorProvider that does not declare the additionalErrorInterceptor function.

For example, consider this CustomInterceptor type:

class CustomInterceptor: LegacyInterceptorProvider {
  // Note that we don't use `override` here.
  func additionalErrorInterceptor<Operation>(for operation: Operation) -> ApolloErrorInterceptor? where Operation : GraphQLOperation {
    ErrorInterceptor()
  }
}

Its superclass, LegacyInterceptorProvider, does not provide an implementation of additionalErrorInterceptor(for:), relying instead on the default implementation from the protocol, that returns nil.
Since its superclass does not implement this function, the implementation in CustomInterceptor is not overriding it, but declaring it anew. For some 🤯 reason, this means that the implementation that provides the additionalErrorInterceptor is never called. The default implementation in the extension of the InterceptorProvider protocol is called instead, returning nil.

The solution

I suggest 2 small changes to solve this issue, and remove this trap.

• Remove the default implementation of additionalErrorInterceptor(for:) from extension InterceptorProvider. This will force base classes to implement it; the library expects the user to subclass one of the provided InterceptorProviders anyway, so I think it's acceptable.
• Implement additionalErrorInterceptor(for:) in both LegacyInterceptorProvider and CodableInterceptorProvider, so that subclasses will have something to override, but keeping the nice nil default.

These changes make for a pretty smooth solution, with only one small breaking change: InterceptorProviders that did implement additionalErrorInterceptor(for:) before this change will have to add the override keyword, if they inherited from one of the provided InterceptorProviders.

Double checking

You can check out the first commit of this PR to see that the Unit test I added fails, reproducing the issue. Add the other commit, and everything is green again ✅

[designatednerd]

Hi! Good point, though I think we can make this work without removing the default implementation.

[designatednerd]

(current test failure is an intermittent issue and unrelated, when you push up new commits I'll kick the CI server if needed)

[TizianoCoroneo]

Hi! Good point, though I think we can make this work without removing the default implementation.

I thought that removing the default implementation from the protocol it would create a footgun for users of the library that create their own InterceptorProvider without subclassing one of the main ones:
If the base class does not implement the function, then other custom subclasses will incur in the same issue.

import Apollo

class BaseIP: InterceptorProvider {
    // no implementation for additionalErrorInterceptor
}

class SubIP: BaseIP {
    // implements the function, but incurs in the same issue.
}

It's kind of a corner case 🤷‍♂️ we'll do fine either way.

I spent a couple days rewriting a network layer with the new networking stack and it's 🔥🔥🔥 keep up the good work! 👏

[TizianoCoroneo]

I've found why this specific issue happens: the subclass cannot override the default implementation of the protocol because... it needs to be implemented in the compiler 😅 it's one of the features mentioned in Swift's Generics Manifesto, under "Allowing subclasses to override requirements satisfied by defaults".

[designatednerd]

If someone's creating their own InterceptorProvider I'd be really surprised if they're creating something with subclasses.

This is probably worth reminding people of in documentation rather than not including the default implementation, IMO.

[TizianoCoroneo]

I'll put back the default implementation in the protocol extension, and remove the implementation from the various TestProviders in the tests.

I'll try to update the docs as well, but one warning: I'm a non-native speaker 😅