Skip to content

Conversation

@mateoguzmana
Copy link
Collaborator

@mateoguzmana mateoguzmana commented Nov 25, 2024

Summary:

While trying to write some test cases for ReactOkHttpNetworkFetcher, I found that the cache control headers are not being sent over the network request correctly. These cache control headers are always being overwritten by the rest of the headers hence all the logic to set this custom cache control doesn't seem to be working as expected.

As per the Request headers docs, this seems to be the explanation:

/** Removes all headers on this builder and adds [headers]. */
open fun headers(headers: Headers) = commonHeaders(headers)

With the new approach by setting the headers first, we ensure that the cache control headers don't get overwritten but they would get added on top of the passed headers.

Notice that currently it seems to be overwriting these headers even if there are not headers passed from the Image component (AKA null/empty).

See my reproduction example in the test plan.

Changelog:

[ANDROID] [FIXED] - ReactOkHttpNetworkFetcher – cache control headers getting overwritten by the rest of the headers

Test Plan:

By creating the following component, we log the request headers using FLog with the previous and the new approach. See the difference on the outputs below:

Component:

<Image
  source={{
    uri: 'https://www.facebook.com/assets/fb_lite_messaging/E2EE-settings@3x.png?cacheBust=reload',
    cache: 'reload',
    headers: {
      'some-header': 'some-header-value',
    },
  }}
/>

Setting the headers last (current approach):

val request =
        Request.Builder()
            .cacheControl(cacheControlBuilder.build())
            .url(uri.toString())
            .headers(headers)
            .get()
            .build()
FLog.w("RequestHeaders", request.headers.toString())

Output (notice that the Cache-Control header is not present):

RequestHeaders  com.facebook.react.uiapp  W  some-header: some-header-value
image

New approach by setting the headers first:

val request =
        Request.Builder()
            .headers(headers)
            .cacheControl(cacheControlBuilder.build())
            .url(uri.toString())
            .get()
            .build()
FLog.w("RequestHeaders", request.headers.toString())

Output (Cache-Control is present now along with the passed headers):

RequestHeaders  com.facebook.react.uiapp   W  some-header: some-header-value
Cache-Control: no-cache, no-store
image

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Nov 25, 2024
@mateoguzmana mateoguzmana marked this pull request as ready for review November 25, 2024 02:48
@facebook-github-bot facebook-github-bot added the Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. label Nov 25, 2024
@facebook-github-bot
Copy link
Contributor

@javache has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot facebook-github-bot added the Merged This PR has been merged. label Nov 25, 2024
@facebook-github-bot
Copy link
Contributor

@javache merged this pull request in 81cb166.

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @mateoguzmana in 81cb166

When will my fix make it into a release? | How to file a pick request?

facebook-github-bot pushed a commit that referenced this pull request Dec 9, 2024
…ts (#47953)

Summary:
This is a follow up for the new cache control options for the Android Image component introduced in #47182, #47348 & #47426. And to make sure the cache control header works as expected and avoid missing the issue fixed in #47922, this PR introduces test cases to make sure this is getting applied as expected in the `ReactOkHttpNetworkFetcher`.

## Changelog:

[INTERNAL] [ADDED] - `ReactOkHttpNetworkFetcher` cache control tests

Pull Request resolved: #47953

Test Plan:
```bash
yarn test-android
```

Reviewed By: rshest

Differential Revision: D66498305

Pulled By: javache

fbshipit-source-id: 7a9a0cc596e49964943e59189614743ca8a472a1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants