Split OkHttpNetworkFetcher's fetch method to make part of it reusable

[rigdern]

Subclasses overriding the fetch method might want to reuse some of its functionality. This change splits the fetch method into 2 methods to make this reusability possible: fetch and fetchWithRequest.

This Fresco change will be consumed by React Native PR facebook/react-native#7791 which enables React Native apps to specify headers to include in the HTTP request when fetching a remote image. For example, one might leverage this when fetching an image from an endpoint that requires authentication.

Adam Comella
Microsoft Corp.

[kirwan]

Thanks for the PR @rigdern.

We just need to add to this doc (and maybe where you declare mHeaders too) to make it clear that support for these added headers is dependent on the NetworkFetcher being used.

Ideally we'd add support to the basic HttpUrlConnectionNetworkFetcher too but I can live without that as long as this is well enough documented.

[rigdern]

@kriwan, thanks for the feedback! I added some comments to document this.

[ghost]

@rigdern updated the pull request - view changes

[ghost]

@rigdern updated the pull request - view changes

[kirwan]

The doc you've added wasn't quite what I meant, which was that the headers are only supported if you use a NetworkFetcher implementation which consumes them. This PR only adds support for one of the two NetworkFetchers in the open source library and anyone using their own would have to add support.

That said I don't want to delay this over a doc which I can fix myself - particularly as we have a new release coming which this could be included in - so I'm going to import it then make the slight change myself.

@facebook-github-bot import

[ghost]

Thanks for importing. If you are an FB employee go to Phabricator to review internal test results.

[rigdern]

Thanks, @kirwan. Sorry, I wrote this change a while ago and I forgot that NetworkFetcher was just an interface 😞.

[npomfret]

Any chance of getting this into the project? It would be very useful.

[lambdapioneer]

Hi @rigdern and @npomfret,

Thanks again for the pull request. While the changes certainly would unblock the React Native feature, I do not feel too comfortable adding HTTP headers to the ImageRequest class. There are three arguments that I have for this:

First, the HTTP headers would only apply to a certain URI scheme (http) and is not relevant e.g. for local file requests. After adding headers, we might also want to add support for auth tokens in cookies and so on leading to some confusing parts in the core api.

Second, sending tokens from a higher level seems to be a specific use case due to the nature of React Native. Most clients would use an OkHttp interceptor (see code below [1]) to solve this. Passing down auth tokens from the UI would look like an anti-pattern there.

Finally, so far the URI was able to fully describe the source of the content. If the image might change depending on the used headers, this would require us to rethink cache keys.

Proposals

Nevertheless, I see that this is crucial for writing applications in React Native that require auth headers. I'd like to propose the following alternatives:

(1) We find a way to leverage the OkHttp3 interceptor in ReactNative. Frankly, I don't know how feasible this is to do. However, the big benefit would be that one can properly do the rewriting of all request parameters. In addition, client logic could handle a 401 itself, re-auth and then re-issue the original request. Completely transparent to the image and UI code.

(2) In the React Native Android code, we create a custom ReactNativeImageRequest extends ImageRequest in those cases where one wants to pass down HTTP modifiers. React Native would also implement its own NetworkFetcher down in the stack and there check for instanceof ReactNativeImageRequest for modifying the request on the lower level where necessary. This would come close to the current proposal with a lot of freedom, without modifying the ImageRequest class.

(3) Refactor the Fresco code to have a NetworkImageRequestWithHeaders extends ImageRequest. This would not change much for other clients, but make it available for users who really need this.

I'm looking forward to hear what you think about these ideas :)

Maybe @oprisnik or @kirwan also chime in. Personally, I feel that (2) would be something that we can get working quite soon. I'm more than happy to help you working on those so that we get them in soon.

Cheers,
Daniel

Code attachments

[1] Best-practice of adding auth headers using interceptor of okhttp

final OkHttpClient.Builder okHttpClientBuilder = new OkHttpClient.Builder()
    .addInterceptor(new Interceptor() {
        @Override
        public Response intercept(Chain chain) throws IOException {
            if (!chain.request().url().host().equals("myhost.com")) {
                return chain.proceed(chain.request());
            }
            final Request.Builder requestBuilder = chain.request().newBuilder();
            requestBuilder.addHeader("authtoken", "foobar42");
            return chain.proceed(requestBuilder.build());
        }
    });
final ImagePipelineConfig imagePipelineConfig = OkHttpImagePipelineConfigFactory.newBuilder(
        getApplicationContext(),
        okHttpClientBuilder.build())
    .build();

[npomfret]

Thanks for the details. I agree that option 2 sounds best. Are you considering that the user could provide an optional "NetworkFetcher" from their JS code in the form of a fetch request? From there they are free to provide custom headers and cache keys.

I don't fully understand option 3 - how would it manifest in the client code?

[rigdern]

@lambdapioneer, thanks for the feedback. Option (2) sounds very similar to the change I originally submitted to the react-native repo (rigdern/react-native@2743ba2) but in this comment @dmmiller suggested this alternative approach of putting part of the change into Fresco.

@dmmiller do you have any thoughts on @lambdapioneer proposals?

[dmmiller]

I am fine with (2). The fresco team certainly knows better if this doesn't really belong in Fresco and if there is a better way to put it in. I suggested it since it looked like just a copy/paste + a new method in our code. If we can do it through inheritance that might be better.

[rigdern]

Based on @dmmiller's response, I will continue with (2). @lambdapioneer, can you provide feedback on my implementation of (2) at rigdern/react-native@2743ba2?

[lambdapioneer]

@rigdern, I've added my comments to the implementation at rigdern/react-native@2743ba2. Looks very good and happy to support from Fresco side

[facebook-github-bot]

@rigdern updated the pull request - view changes - changes since last import

[rigdern]

@lambdapioneer Thank you for your suggestions. I applied them and updated the Fresco and React Native PRs.

[lambdapioneer]

Awesome, just two small style remarks :)

Will already import and then update with when the code style is updated

[lambdapioneer]

nit: move this method below the other public methods and re-format:

protected void fetchWithRequest(
    final OkHttpNetworkFetchState fetchState,
    final Callback callback,
    final Request request) {

[lambdapioneer]

👍

[facebook-github-bot]

@rigdern updated the pull request - view changes - changes since last import

[rigdern]

@lambdapioneer I made the styling tweaks you requested. I also retitled the PR to better match the approach we're now taking.