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

createObject does not set cacheControl #18

Closed
horizon0708 opened this issue May 9, 2021 · 7 comments · Fixed by supabase/storage-js#8
Closed

createObject does not set cacheControl #18

horizon0708 opened this issue May 9, 2021 · 7 comments · Fixed by supabase/storage-js#8
Labels
bug Something isn't working

Comments

@horizon0708
Copy link

horizon0708 commented May 9, 2021

Bug report

Describe the bug

TL;DR When I upload an object to storage via storage-js, the API doesn't care about the cache-control value.

At the moment, when you call upload() from the Supabase client, it sets the default cache-control field to 3600 (https://github.com/supabase/storage-js/blob/main/src/lib/StorageFileApi.ts#L15). The API should take this field and set that as max-age (https://github.com/supabase/storage-api/blob/master/src/routes/object/createObject.ts#L59). If this value is not present, it is set to no-cache.

When I upload an object to a bucket, I can see that cache-control is set in the request (see screenshot below). This should mean that the server should respond with cache-control header value set tomax-age=3600 when I request for the object later on.
image

However, when I download the object afterward, the header is set to no-cache.
image

This is not desirable behavior, as we want to cache as many things as possible for a good user experience (and probably also lessen the load for Supabase too).

To Reproduce

  1. Using the supabase client, upload any object, with or without an optional cacheControl value set.
  2. Download the object using the supabase client, refresh the page after the download finishes.
  3. on refresh, check the network tab to see that (1) the browser requested and downloaded the object again (2) and the response has no-cache header set.

Expected behavior

Supabase API sets cache-control headers that we pass in.

Screenshots

If applicable, add screenshots to help explain your problem.

System information

  • OS: Windows
  • Browser (if applies): chrome
  • Version of supabase-js: "1.7.7",
  • Version of Node.js: v12.0.0
@horizon0708 horizon0708 added the bug Something isn't working label May 9, 2021
@horizon0708
Copy link
Author

The way I'm getting around this atm is to implement/use caching on your app. I'm using react query. I have something like:

export const fetchAvatar: QueryFunction<string | undefined> = async ({ queryKey }) => {
    const [_key, path] = queryKey
    const { data, error } = await supabaseClient.storage
        .from(AVATAR_BUCKET)
        .download(path as string)
    
    if(error) {
        throw new Error(error.message)
    }

    if(!data) {
        return undefined
    }

    return URL.createObjectURL(data)
}

then I pass the query function to react-query with options (note staleTime).

export const UserAvartar:React.FC<UserAvatarProps> = ({ name, path, className }) => {
    const { data } = useQuery(
        ['avatar', path], 
        fetchAvatar, 
        { 
            enabled: !!path,
            staleTime: 1000 * 60 * 60, // 1 hour
        }
    )
    return <Avatar className={className} alt={name} src={data} />
}

This means that the fetch for an object is cached for an hour (as long as the user doesn't reload the app), so the user won't have to redownload the same image over and over again.

@horizon0708
Copy link
Author

horizon0708 commented May 19, 2021

Checked after updating Supabase to 1.11.3. Still happening.

@horizon0708
Copy link
Author

horizon0708 commented May 19, 2021

Did some digging just now. The issue is because the controller is calling .file()

const data = await request.file()

This calls this part of fastify-multipart

 async function getMultipartFile (options) {
    const parts = this[kMultipartHandler](options)
    let part
    while ((part = await parts()) != null) {
      if (part.file) {
        // part.file.truncated is true when a configured file size limit is reached
        if (part.file.truncated && throwFileSizeLimit) {
          throw new RequestFileTooLargeError()
        }
        return part
      }
    }
  }
// https://github.com/fastify/fastify-multipart/blob/master/index.js#L185
fastify.decorateRequest('file', getMultipartFile)

As you can see, this only loads the first file it sees, never loading field - thus

const cacheTime = data.fields.cacheControl?.value

will never have a value.

The solution would be to use parts() API that returns an AysncIterable of Multipart and process that or give the value as a header to make life easier. If I have more time I may try to make a PR, but no promises 🙈

@inian
Copy link
Member

inian commented May 19, 2021

Thanks for digging into this @horizon0708. Looks like the non file values need to be sent before the file fields as mentioned here and it is the intended behaviour. Fixing in storage-js

@inian
Copy link
Member

inian commented May 19, 2021

Can you try with supabase-js 1.11.14 James?

@horizon0708
Copy link
Author

horizon0708 commented May 19, 2021

It's working! Thanks for a quick update! Looks like I was completely off the mark with https://github.com/supabase/storage-api/issues/18#issuecomment-843717897😅
image

@inian
Copy link
Member

inian commented May 19, 2021

I thought it might have to do with something with Fastify as well till I realised that it was working with when I sent the request via Postman but not via supabase-js

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants