Skip to content

feat: add private method to get expected content type #210

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.

Sign up for GitHub

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

feat: add private method to get expected content type #210

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c6f7d9f
feat: Add private method to get expected content type
anonymousRecords Sep 29, 2024
2c07dd3
refactor: getContentType method to simplify the code
anonymousRecords Sep 29, 2024
15428c9
refactor: change return
anonymousRecords Sep 29, 2024
ccc4774
refactor: add mime-types
anonymousRecords Oct 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions src/packages/StorageFileApi.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,20 @@ export default class StorageFileApi {
return this.uploadOrUpdate('POST', path, fileBody, fileOptions)
}

private _getExpectedContentType(path: string): string {
const extension = path.split('.').pop()?.toLowerCase()
const mimeTypes: { [key: string]: string } = {
png: 'image/png',
jpg: 'image/jpeg',
jpeg: 'image/jpeg',
gif: 'image/gif',
pdf: 'application/pdf',
md: 'text/markdown',
}

return extension ? mimeTypes[extension] : 'application/octet-stream'
}

/**
* Upload a file with a token generated from `createSignedUploadUrl`.
* @param path The file path, including the file name. Should be of the format `folder/subfolder/filename.png`. The bucket must already exist before attempting to upload.
Expand Down Expand Up @@ -205,7 +219,15 @@ export default class StorageFileApi {
} else {
body = fileBody
headers['cache-control'] = `max-age=${options.cacheControl}`

const expectedContentType = this._getExpectedContentType(path)
headers['content-type'] = options.contentType as string

if (headers['content-type'] !== expectedContentType) {
throw new StorageError(
`Content-type mismatch. Expected: "${expectedContentType}", but received: "${headers['content-type']}" for file "${path}"`
)
}
Comment on lines +219 to +223
Copy link

@saqibameen saqibameen Sep 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't this lead to unwanted errors because expectedContentType is defaulting to application/octet-stream?

E.g., for .xls file, the mime type is application/vnd.ms-excel, but because of how _getExpectedContentType(...) works, it will return application/octet-stream and headers['content-type'] will be application/vnd.ms-excel

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've modified the code to address the issue you pointed out. The _getExpectedContentType function has been updated as follows:

  • For known extensions, it continues to return the corresponding MIME type as before.
  • For unknown extensions, it now returns an empty string ('') instead of 'application/octet-stream'.

This change prevents unnecessary errors for unknown file types like .xls, and allows the system to use the content-type provided by the user. Could you please review this change and confirm if it adequately addresses the concern you raised?

Copy link

@saqibameen saqibameen Oct 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! It does address the concern that I raised. However, I feel this method is not scalable and is only limited to handling the specified types e.g., .png, .jpg, .jpeg, .gif, .pdf, and .md. This seems very limiting.

Solutions that I'd expect:

  1. Get the mime type of the file using something like mime-types pkg. So it is not limited to the specified types. Alternatively, we can ask users to pass it while creating signedUploadUrl. Then just match the type when user uploads.
  2. Even better, detect the mime type from the incoming buffer, just like we do on frontend file.type. Not sure if that is possible, then match it.

Just thinking out loud here. Would be happy to contribute in any way.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have modified the code to use the suggested mime-types package for content type verification, allowing support for a wider range of file types.

While considering the option of detecting MIME types directly from file buffers, I decided to adopt the mime-types package approach for now, as accurately detecting all file types from buffers could be challenging.

If any ideas come up for implementing buffer-based MIME type detection in the future, I'll certainly explore that approach as well. Any additional thoughts or suggestions are welcome 😊

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me. Thank you.

}

const res = await this.fetch(url.toString(), {
Expand Down