apache / opendal

Apache OpenDAL: access data freely.
https://opendal.apache.org
Apache License 2.0
3.26k stars 454 forks source link

servcices/supabase: Tracking issues of not fixed issues at storage-api side #2199

Closed Xuanwo closed 3 months ago

Xuanwo commented 1 year ago

Visit https://github.com/apache/incubator-opendal/pull/2190 for more context.

This issue has been converted as a tracking issue of not fixed issues at storage-api side:

We are keeping this issue to ensure that we have a valid context for contacting the Supabase development team.

xyjixyjixyji commented 1 year ago

Please assign this to me, it is almost done xD

xyjixyjixyji commented 1 year ago

Also, I found that many tests require read_with_range capabilities, but the Supabase seems do not support the http RANGE header.

Is it reasonable to add

    if !op.info().capability().read_with_range {
        return Ok(());
    }

to tests that call into the op.range_reader()?

Xuanwo commented 1 year ago

Also, I found that many tests require read_with_range capabilities, but the Supabase seems do not support the http RANGE header.

Supabase seems handled range header already:

https://github.com/supabase/storage-api/blob/1794d04ab0e7259d9058293e257f442015252ec3/src/storage/renderer/asset.ts#L14-L20

  getAsset(request: FastifyRequest, options: RenderOptions) {
    return this.backend.getObject(options.bucket, options.key, options.version, {
      ifModifiedSince: request.headers['if-modified-since'],
      ifNoneMatch: request.headers['if-none-match'],
      range: request.headers.range,
    })
  }
Xuanwo commented 1 year ago

Supabase has two different storage backend, s3 supports range, but file not. It's another issue from supabase storage side :rofl:

  async getObject(
    bucketName: string,
    key: string,
    version: string | undefined
  ): Promise<ObjectResponse> {
    const file = path.resolve(this.filePath, withOptionalVersion(`${bucketName}/${key}`, version))
    const body = fs.createReadStream(file)
    const data = await fs.stat(file)
    const { cacheControl, contentType } = await this.getFileMetadata(file)
    const lastModified = new Date(0)
    lastModified.setUTCMilliseconds(data.mtimeMs)

    const checksum = await fileChecksum(file)

    return {
      metadata: {
        cacheControl: cacheControl || 'no-cache',
        mimetype: contentType || 'application/octet-stream',
        lastModified: lastModified,
        // contentRange: data.ContentRange, @todo: support range requests
        httpStatusCode: 200,
        size: data.size,
        eTag: checksum,
        contentLength: data.size,
      },
      body,
    }
  }
Xuanwo commented 1 year ago

Also, I found that many tests require read_with_range capabilities, but the Supabase seems do not support the http RANGE header.

Is it reasonable to add

    if !op.info().capability().read_with_range {
        return Ok(());
    }

to tests that call into the op.range_reader()?

Yes, we should ignore tests for services that doesn't support range. Please make sure all services have this capability been set correctly.

xyjixyjixyji commented 1 year ago

Of course. I will patch that alongside my Supabase CI fix.

xyjixyjixyji commented 1 year ago

Supabase has two different storage backend, s3 supports range, but file not. It's another issue from supabase storage side 🤣

  async getObject(
    bucketName: string,
    key: string,
    version: string | undefined
  ): Promise<ObjectResponse> {
    const file = path.resolve(this.filePath, withOptionalVersion(`${bucketName}/${key}`, version))
    const body = fs.createReadStream(file)
    const data = await fs.stat(file)
    const { cacheControl, contentType } = await this.getFileMetadata(file)
    const lastModified = new Date(0)
    lastModified.setUTCMilliseconds(data.mtimeMs)

    const checksum = await fileChecksum(file)

    return {
      metadata: {
        cacheControl: cacheControl || 'no-cache',
        mimetype: contentType || 'application/octet-stream',
        lastModified: lastModified,
        // contentRange: data.ContentRange, @todo: support range requests
        httpStatusCode: 200,
        size: data.size,
        eTag: checksum,
        contentLength: data.size,
      },
      body,
    }
  }

What do you think, should we raise an issue there? 🤣

Xuanwo commented 1 year ago

What do you think, should we raise an issue there?

Yes, please raise an issue to supbase storage-api and link here. We can add read_with_range for supabase when they addressed it.

Xuanwo commented 1 year ago

This issue will be converted as a tracking issue of not fixed issues at storage-api side:

Xuanwo commented 1 year ago

Thanks to @Ji-Xinyou, most work at opendal side is done!

Xuanwo commented 3 months ago

We are going to remove supabase support, let's close.