You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@opendal.apache.org by "wlinna (via GitHub)" <gi...@apache.org> on 2025/03/03 20:54:35 UTC
[PR] feat(services/gcs): Implement multipart/related and use it with Gcs [opendal]
wlinna opened a new pull request, #5691:
URL: https://github.com/apache/opendal/pull/5691
The status of this PR:
With this PR applied, I am able to set metadata such as `content_encoding` and `content_type` in write operations with `Gcs`. I have tested it with the official Storage emulator provided by `firebase-tools`.
What's missing:
- I haven't tested this with the actual GCS yet
- Unit tests for `RelatedPart`
# Which issue does this PR close?
Closes https://github.com/apache/opendal/issues/5624
TL;DR: Improves GCS API compliance
# Rationale for this change
The rationale for the change is discussed in the issue.
# What changes are included in this PR?
- Implement support for `multipart/related` (`RelatedPart`)
- Use `RelatedPart` in `gcs_insert_object_request` instead of `FormDataPart`
# Are there any user-facing changes?
None
--
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: [PR] feat(services/gcs): Implement multipart/related and use it with Gcs [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #5691:
URL: https://github.com/apache/opendal/pull/5691#discussion_r1978825142
##########
core/src/raw/http_util/multipart.rs:
##########
@@ -523,6 +523,111 @@ impl Part for MixedPart {
}
}
+/// RelatedPart is a builder for multipart/related part.
+pub struct RelatedPart {
+ /// Common
+ headers: HeaderMap,
+ content: Buffer,
+}
+
+impl Default for RelatedPart {
+ fn default() -> Self {
+ Self::new()
+ }
+}
+
+impl RelatedPart {
+ /// Create a new related
+ pub fn new() -> Self {
+ Self {
+ headers: HeaderMap::new(),
+ content: Buffer::new(),
+ }
+ }
+
+ /// Build a mixed part from a request.
+ pub fn from_request(req: Request<Buffer>) -> Self {
+ let (parts, content) = req.into_parts();
+
+ Self {
+ headers: parts.headers,
+ content,
+ }
+ }
+
+ /// Consume a mixed part to build a response.
+ pub fn into_response(mut self) -> Response<Buffer> {
+ let mut builder = Response::builder();
+
+ // Swap headers directly instead of copy the entire map.
+ mem::swap(builder.headers_mut().unwrap(), &mut self.headers);
+
+ builder
+ .body(self.content)
+ .expect("a related part must be valid response")
+ }
+
+ /// Insert a header into part.
+ pub fn header(mut self, key: HeaderName, value: HeaderValue) -> Self {
+ self.headers.insert(key, value);
+ self
+ }
+
+ /// Set the content for this part.
+ pub fn content(mut self, content: impl Into<Buffer>) -> Self {
+ self.content = content.into();
+ self
+ }
+}
+
+impl Part for RelatedPart {
+ const TYPE: &'static str = "related";
+
+ fn format(self) -> Buffer {
+ // This is what multipart/related body might look for an insert in GCS
+ // https://cloud.google.com/storage/docs/uploading-objects
+ /*
+ --separator_string
+ Content-Type: application/json; charset=UTF-8
+
+ {"name":"my-document.txt"}
+
+ --separator_string
+ Content-Type: text/plain
+
+ This is a text file.
+ --separator_string--
+ */
+
+ let mut bufs = Vec::with_capacity(3);
+ let mut bs = BytesMut::new();
+
+ // Write request headers.
+ for (k, v) in self.headers.iter() {
+ bs.extend_from_slice(k.as_str().as_bytes());
+ bs.extend_from_slice(b": ");
+ bs.extend_from_slice(v.as_bytes());
+ bs.extend_from_slice(b"\r\n");
+ }
+ bs.extend_from_slice(b"\r\n");
+ bufs.push(Buffer::from(bs.freeze()));
+
+ if !self.content.is_empty() {
+ bufs.push(self.content);
+ bufs.push(Buffer::from("\r\n"))
+ }
+
+ bufs.into_iter().flatten().collect()
+ }
+
+ fn parse(_s: &str) -> Result<Self> {
+ Err(Error::new(
+ ErrorKind::Unsupported,
+ "parsing multipart/related is not supported",
+ ))
+ }
+}
+
Review Comment:
Would you like to add a unit test for related parts?
--
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: [PR] feat(services/gcs): Implement multipart/related and use it with Gcs [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo merged PR #5691:
URL: https://github.com/apache/opendal/pull/5691
--
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: [PR] feat(services/gcs): Implement multipart/related and use it with Gcs [opendal]
Posted by "wlinna (via GitHub)" <gi...@apache.org>.
wlinna commented on code in PR #5691:
URL: https://github.com/apache/opendal/pull/5691#discussion_r1984635584
##########
core/src/raw/http_util/multipart.rs:
##########
@@ -523,6 +523,111 @@ impl Part for MixedPart {
}
}
+/// RelatedPart is a builder for multipart/related part.
+pub struct RelatedPart {
+ /// Common
+ headers: HeaderMap,
+ content: Buffer,
+}
+
+impl Default for RelatedPart {
+ fn default() -> Self {
+ Self::new()
+ }
+}
+
+impl RelatedPart {
+ /// Create a new related
+ pub fn new() -> Self {
+ Self {
+ headers: HeaderMap::new(),
+ content: Buffer::new(),
+ }
+ }
+
+ /// Build a mixed part from a request.
+ pub fn from_request(req: Request<Buffer>) -> Self {
+ let (parts, content) = req.into_parts();
+
+ Self {
+ headers: parts.headers,
+ content,
+ }
+ }
+
+ /// Consume a mixed part to build a response.
+ pub fn into_response(mut self) -> Response<Buffer> {
+ let mut builder = Response::builder();
+
+ // Swap headers directly instead of copy the entire map.
+ mem::swap(builder.headers_mut().unwrap(), &mut self.headers);
+
+ builder
+ .body(self.content)
+ .expect("a related part must be valid response")
+ }
+
+ /// Insert a header into part.
+ pub fn header(mut self, key: HeaderName, value: HeaderValue) -> Self {
+ self.headers.insert(key, value);
+ self
+ }
+
+ /// Set the content for this part.
+ pub fn content(mut self, content: impl Into<Buffer>) -> Self {
+ self.content = content.into();
+ self
+ }
+}
+
+impl Part for RelatedPart {
+ const TYPE: &'static str = "related";
+
+ fn format(self) -> Buffer {
+ // This is what multipart/related body might look for an insert in GCS
+ // https://cloud.google.com/storage/docs/uploading-objects
+ /*
+ --separator_string
+ Content-Type: application/json; charset=UTF-8
+
+ {"name":"my-document.txt"}
+
+ --separator_string
+ Content-Type: text/plain
+
+ This is a text file.
+ --separator_string--
+ */
+
+ let mut bufs = Vec::with_capacity(3);
+ let mut bs = BytesMut::new();
+
+ // Write request headers.
+ for (k, v) in self.headers.iter() {
+ bs.extend_from_slice(k.as_str().as_bytes());
+ bs.extend_from_slice(b": ");
+ bs.extend_from_slice(v.as_bytes());
+ bs.extend_from_slice(b"\r\n");
+ }
+ bs.extend_from_slice(b"\r\n");
+ bufs.push(Buffer::from(bs.freeze()));
+
+ if !self.content.is_empty() {
+ bufs.push(self.content);
+ bufs.push(Buffer::from("\r\n"))
+ }
+
+ bufs.into_iter().flatten().collect()
+ }
+
+ fn parse(_s: &str) -> Result<Self> {
+ Err(Error::new(
+ ErrorKind::Unsupported,
+ "parsing multipart/related is not supported",
+ ))
+ }
+}
+
Review Comment:
I will add unit tests
--
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: [PR] feat(services/gcs): Implement multipart/related and use it with Gcs [opendal]
Posted by "wlinna (via GitHub)" <gi...@apache.org>.
wlinna commented on code in PR #5691:
URL: https://github.com/apache/opendal/pull/5691#discussion_r1984635584
##########
core/src/raw/http_util/multipart.rs:
##########
@@ -523,6 +523,111 @@ impl Part for MixedPart {
}
}
+/// RelatedPart is a builder for multipart/related part.
+pub struct RelatedPart {
+ /// Common
+ headers: HeaderMap,
+ content: Buffer,
+}
+
+impl Default for RelatedPart {
+ fn default() -> Self {
+ Self::new()
+ }
+}
+
+impl RelatedPart {
+ /// Create a new related
+ pub fn new() -> Self {
+ Self {
+ headers: HeaderMap::new(),
+ content: Buffer::new(),
+ }
+ }
+
+ /// Build a mixed part from a request.
+ pub fn from_request(req: Request<Buffer>) -> Self {
+ let (parts, content) = req.into_parts();
+
+ Self {
+ headers: parts.headers,
+ content,
+ }
+ }
+
+ /// Consume a mixed part to build a response.
+ pub fn into_response(mut self) -> Response<Buffer> {
+ let mut builder = Response::builder();
+
+ // Swap headers directly instead of copy the entire map.
+ mem::swap(builder.headers_mut().unwrap(), &mut self.headers);
+
+ builder
+ .body(self.content)
+ .expect("a related part must be valid response")
+ }
+
+ /// Insert a header into part.
+ pub fn header(mut self, key: HeaderName, value: HeaderValue) -> Self {
+ self.headers.insert(key, value);
+ self
+ }
+
+ /// Set the content for this part.
+ pub fn content(mut self, content: impl Into<Buffer>) -> Self {
+ self.content = content.into();
+ self
+ }
+}
+
+impl Part for RelatedPart {
+ const TYPE: &'static str = "related";
+
+ fn format(self) -> Buffer {
+ // This is what multipart/related body might look for an insert in GCS
+ // https://cloud.google.com/storage/docs/uploading-objects
+ /*
+ --separator_string
+ Content-Type: application/json; charset=UTF-8
+
+ {"name":"my-document.txt"}
+
+ --separator_string
+ Content-Type: text/plain
+
+ This is a text file.
+ --separator_string--
+ */
+
+ let mut bufs = Vec::with_capacity(3);
+ let mut bs = BytesMut::new();
+
+ // Write request headers.
+ for (k, v) in self.headers.iter() {
+ bs.extend_from_slice(k.as_str().as_bytes());
+ bs.extend_from_slice(b": ");
+ bs.extend_from_slice(v.as_bytes());
+ bs.extend_from_slice(b"\r\n");
+ }
+ bs.extend_from_slice(b"\r\n");
+ bufs.push(Buffer::from(bs.freeze()));
+
+ if !self.content.is_empty() {
+ bufs.push(self.content);
+ bufs.push(Buffer::from("\r\n"))
+ }
+
+ bufs.into_iter().flatten().collect()
+ }
+
+ fn parse(_s: &str) -> Result<Self> {
+ Err(Error::new(
+ ErrorKind::Unsupported,
+ "parsing multipart/related is not supported",
+ ))
+ }
+}
+
Review Comment:
I will
--
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: [PR] feat(services/gcs): Implement multipart/related and use it with Gcs [opendal]
Posted by "wlinna (via GitHub)" <gi...@apache.org>.
wlinna commented on PR #5691:
URL: https://github.com/apache/opendal/pull/5691#issuecomment-2705807896
I noticed that this doesn't actually work correctly on the emulator yet. For some reason in some places the content-type is taken as a path for example. So if I set `Content-Type: application/p21`, the emulator will see that I want to write a file in path `application/p21`.
--
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: [PR] feat(services/gcs): Implement multipart/related and use it with Gcs [opendal]
Posted by "wlinna (via GitHub)" <gi...@apache.org>.
wlinna commented on code in PR #5691:
URL: https://github.com/apache/opendal/pull/5691#discussion_r1986028763
##########
core/src/raw/http_util/multipart.rs:
##########
@@ -523,6 +523,111 @@ impl Part for MixedPart {
}
}
+/// RelatedPart is a builder for multipart/related part.
+pub struct RelatedPart {
+ /// Common
+ headers: HeaderMap,
+ content: Buffer,
+}
+
+impl Default for RelatedPart {
+ fn default() -> Self {
+ Self::new()
+ }
+}
+
+impl RelatedPart {
+ /// Create a new related
+ pub fn new() -> Self {
+ Self {
+ headers: HeaderMap::new(),
+ content: Buffer::new(),
+ }
+ }
+
+ /// Build a mixed part from a request.
+ pub fn from_request(req: Request<Buffer>) -> Self {
+ let (parts, content) = req.into_parts();
+
+ Self {
+ headers: parts.headers,
+ content,
+ }
+ }
+
+ /// Consume a mixed part to build a response.
+ pub fn into_response(mut self) -> Response<Buffer> {
+ let mut builder = Response::builder();
+
+ // Swap headers directly instead of copy the entire map.
+ mem::swap(builder.headers_mut().unwrap(), &mut self.headers);
+
+ builder
+ .body(self.content)
+ .expect("a related part must be valid response")
+ }
+
+ /// Insert a header into part.
+ pub fn header(mut self, key: HeaderName, value: HeaderValue) -> Self {
+ self.headers.insert(key, value);
+ self
+ }
+
+ /// Set the content for this part.
+ pub fn content(mut self, content: impl Into<Buffer>) -> Self {
+ self.content = content.into();
+ self
+ }
+}
+
+impl Part for RelatedPart {
+ const TYPE: &'static str = "related";
+
+ fn format(self) -> Buffer {
+ // This is what multipart/related body might look for an insert in GCS
+ // https://cloud.google.com/storage/docs/uploading-objects
+ /*
+ --separator_string
+ Content-Type: application/json; charset=UTF-8
+
+ {"name":"my-document.txt"}
+
+ --separator_string
+ Content-Type: text/plain
+
+ This is a text file.
+ --separator_string--
+ */
+
+ let mut bufs = Vec::with_capacity(3);
+ let mut bs = BytesMut::new();
+
+ // Write request headers.
+ for (k, v) in self.headers.iter() {
+ bs.extend_from_slice(k.as_str().as_bytes());
+ bs.extend_from_slice(b": ");
+ bs.extend_from_slice(v.as_bytes());
+ bs.extend_from_slice(b"\r\n");
+ }
+ bs.extend_from_slice(b"\r\n");
+ bufs.push(Buffer::from(bs.freeze()));
+
+ if !self.content.is_empty() {
+ bufs.push(self.content);
+ bufs.push(Buffer::from("\r\n"))
+ }
+
+ bufs.into_iter().flatten().collect()
+ }
+
+ fn parse(_s: &str) -> Result<Self> {
+ Err(Error::new(
+ ErrorKind::Unsupported,
+ "parsing multipart/related is not supported",
+ ))
+ }
+}
+
Review Comment:
I added a simple unit tests. I just used the example from Google's documentation. I'm not aware of a more complex example
--
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