You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@opendal.apache.org by "czy-29 (via GitHub)" <gi...@apache.org> on 2024/07/16 02:08:02 UTC
[I] new feature: Automatically add `Content-Type` based on the extension in the path (or provide an API for automatic setting) [opendal]
czy-29 opened a new issue, #4902:
URL: https://github.com/apache/opendal/issues/4902
### Feature Description
If I understand correctly, in the current version of OpenDAL, no matter what file we upload to the object storage, its default `Content-Type` will always be `application/octet-stream`. If I want to modify this default behavior, I have to manually set the Content-Type myself using something like `Operator::write_with`. This is not very convenient, and it should not be the expected behavior. The expected `Content-Type` should automatically match the path extension. Of course, I can easily implement this capability at the application level using `mime_guess` myself, but I think it would be more convenient to sink this behavior into OpenDAL. Either change the default behavior of `Operator::write` directly to this, or add a method similar to `content_type_guessed` to FutureWrite. Of course, the specific API design can be discussed further, but I believe that no matter how we choose, adding this feature will make uploading files more convenient and intuitive.
### Problem and Solution
Allow `Operator::write` to automatically set the corresponding `Content-Type` based on the file extension in the path, or add a method to `FutureWrite` to automatically set this.
### Additional Context
_No response_
### Are you willing to contribute to the development of this feature?
- [X] Yes, I am willing to contribute to the development of this feature.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] new feature: Automatically add `Content-Type` based on the extension in the path (or provide an API for automatic setting) [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on issue #4902:
URL: https://github.com/apache/opendal/issues/4902#issuecomment-2235203343
Looks great, let's go!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] new feature: Automatically add `Content-Type` based on the extension in the path (or provide an API for automatic setting) [opendal]
Posted by "czy-29 (via GitHub)" <gi...@apache.org>.
czy-29 commented on issue #4902:
URL: https://github.com/apache/opendal/issues/4902#issuecomment-2230042604
Let me take some time to read the source code first, in order to better understand the overall architecture of Layer‘s implementation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] new feature: Automatically add `Content-Type` based on the extension in the path (or provide an API for automatic setting) [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on issue #4902:
URL: https://github.com/apache/opendal/issues/4902#issuecomment-2229876781
This layer could do more: detect the content_type if storage service doesn't provide it. For example, local fs doesn't store the content_type for file. We can use this layer to detect it when user call `stat`:
```rust
let op = op.layer(MimeGuessLayer::default());
let meta = op.stat("x.pdf").await?; <-- `meta.content_type()` will be `pdf`.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] new feature: Automatically add `Content-Type` based on the extension in the path (or provide an API for automatic setting) [opendal]
Posted by "czy-29 (via GitHub)" <gi...@apache.org>.
czy-29 commented on issue #4902:
URL: https://github.com/apache/opendal/issues/4902#issuecomment-2229893463
I am indeed willing to contribute, but I am still a novice in contributing to large open source projects and unsure if I can handle it well. Moreover, if we follow our newly designed concept, this feature would appear to require a significant amount of work, especially if we include the reading of file format magic headers (I'm not sure if there are any existing crates in the Rust community that can retrieve MIME information based on magic headers, and if not, we may need to create a large table).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] new feature: Automatically add `Content-Type` based on the extension in the path (or provide an API for automatic setting) [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo closed issue #4902: new feature: Automatically add `Content-Type` based on the extension in the path (or provide an API for automatic setting)
URL: https://github.com/apache/opendal/issues/4902
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] new feature: Automatically add `Content-Type` based on the extension in the path (or provide an API for automatic setting) [opendal]
Posted by "czy-29 (via GitHub)" <gi...@apache.org>.
czy-29 commented on issue #4902:
URL: https://github.com/apache/opendal/issues/4902#issuecomment-2229882163
Wow, that's a great idea! I think it's much better than my previous design, and it's indeed more useful and flexible! This design perfectly meets my expectations and needs, so I fully approve of it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] new feature: Automatically add `Content-Type` based on the extension in the path (or provide an API for automatic setting) [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on issue #4902:
URL: https://github.com/apache/opendal/issues/4902#issuecomment-2229884906
I see you've indicated a willingness to contribute. Given the recent design changes, are you still interested in implementing it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] new feature: Automatically add `Content-Type` based on the extension in the path (or provide an API for automatic setting) [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on issue #4902:
URL: https://github.com/apache/opendal/issues/4902#issuecomment-2229951015
> especially if we include the reading of file format magic headers
This is optional. We can implement the basic `mime_guess::from_path()` first.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] new feature: Automatically add `Content-Type` based on the extension in the path (or provide an API for automatic setting) [opendal]
Posted by "czy-29 (via GitHub)" <gi...@apache.org>.
czy-29 commented on issue #4902:
URL: https://github.com/apache/opendal/issues/4902#issuecomment-2234058995
I have finished reading the code related to `Layer` today, and I am not sure if I understand it correctly. Before implementing it, I would like to confirm with you that the following approach is correct:
Firstly, `MimeGuessLayer` should implement `Layer<A>` and return an internal type that implements `LayeredAccess` (such as `MimeGuessAccessor<A>`).
Then, in the `LayeredAccess` implementation of `MimeGuessAccessor<A>`, each associated type can directly use the associated type corresponding to `Inner` (actually type A).
Then, for all required methods except for `write`, simply call the corresponding method of `self.inner()` to implement it.
For `write`, first judge whether `args.content_type` (args is `OpWrite` type) is `None`. If yes, call `args.with_content_type`, and then call `self.inner().write` with the new args.
Among the remaining methods that provide default implementations, only `stat` needs to be implemented. The implementation of `stat` is to first call `self.inner().stat`, and then for the returned `RpStat`, call `map_metadata`. Then, the same process is used to determine whether the content type is `None`. If it is, call `with_content_type` and add the new content type and return.
Finally, there should be no need to add new logic to `list`, as I have read in the document that when calling `list`, if the user requests content type, it will automatically call `stat` internally to retrieve it. Therefore, as long as we implement this logic on `stat`, users will automatically have the ability to obtain the content type when calling `list`.
I don't know if my understanding above is correct?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] new feature: Automatically add `Content-Type` based on the extension in the path (or provide an API for automatic setting) [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on issue #4902:
URL: https://github.com/apache/opendal/issues/4902#issuecomment-2229872844
Thank you for submitting this pull request. I understand the feature request, but I don't think auto-detection is a good approach.
Perhaps we could implement a `MimeGuessLayer` that detects and sets the `content_type` when it's not specified by the user. In the `MimeGuessLayer`, we could detect the file extension or even read the magic bits to determine it based on user needs. This would be much more flexible and useful.
Users can use this layer like this:
```rust
let op = op.layer(MimeGuessLayer::default());
op.write("x.pdf", bs).await?; <-- content-type will be `pdf`.
```
What do you think?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org