You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@opendal.apache.org by "jorgehermo9 (via GitHub)" <gi...@apache.org> on 2024/11/03 15:44:38 UTC
[PR] feat(core/services-gcs): support user defined metadata [opendal]
jorgehermo9 opened a new pull request, #5276:
URL: https://github.com/apache/opendal/pull/5276
Relates to https://github.com/apache/opendal/issues/4842
--
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(core/services-gcs): support user defined metadata [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on PR #5276:
URL: https://github.com/apache/opendal/pull/5276#issuecomment-2456058342
See https://github.com/apache/opendal/pull/5284
--
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(core/services-gcs): support user defined metadata [opendal]
Posted by "jorgehermo9 (via GitHub)" <gi...@apache.org>.
jorgehermo9 commented on code in PR #5276:
URL: https://github.com/apache/opendal/pull/5276#discussion_r1827027270
##########
core/src/services/gcs/core.rs:
##########
@@ -267,6 +276,12 @@ impl GcsCore {
let mut req = Request::post(&url);
+ if let Some(user_metadata) = op.user_metadata() {
+ for (key, value) in user_metadata {
+ req = req.header(format!("{X_GOOG_META_PREFIX}{key}"), value)
Review Comment:
In the details section of the first documentation you depicted, it says:
> Any custom metadata set in the [initial request](https://cloud.google.com/storage/docs/performing-resumable-uploads#initiate-session) that uses the same KEY string is overwritten using the value provided in the X-Goog-Meta- header.
And in https://cloud.google.com/storage/docs/performing-resumable-uploads#initiate-session It seems that it is allowed in single chunks upload, as I think it is done here:
As you raised this, in multipart uploads I think we should include the headers in the `gcs_complete_multipart_upload`
In the behaviour tests are there test for both multipart and single chunk uploads with gcs? I would like to check that both `gcs_complete_multipart_upload` and `gcs_insert_object_request` set the metadata successfully.
--
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(core/services-gcs): support user defined metadata [opendal]
Posted by "jorgehermo9 (via GitHub)" <gi...@apache.org>.
jorgehermo9 commented on PR #5276:
URL: https://github.com/apache/opendal/pull/5276#issuecomment-2457746340
Hi @Xuanwo so nice! Thank you very much.
I'm open for any other suggestions!
--
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(core/services-gcs): support user defined metadata [opendal]
Posted by "jorgehermo9 (via GitHub)" <gi...@apache.org>.
jorgehermo9 commented on code in PR #5276:
URL: https://github.com/apache/opendal/pull/5276#discussion_r1827023390
##########
core/src/services/gcs/backend.rs:
##########
@@ -424,6 +426,10 @@ impl Access for GcsBackend {
m.set_last_modified(parse_datetime_from_rfc3339(&meta.updated)?);
+ if let Some(user_metadata) = meta.metadata {
+ m.with_user_metadata(user_metadata);
Review Comment:
should I include a `!user_meta.is_empty()` check before adding the metadata? as done in other services.
I don't know if gcs can send an empty metadata in the response
--
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(core/services-gcs): support user defined metadata [opendal]
Posted by "jorgehermo9 (via GitHub)" <gi...@apache.org>.
jorgehermo9 commented on code in PR #5276:
URL: https://github.com/apache/opendal/pull/5276#discussion_r1827027270
##########
core/src/services/gcs/core.rs:
##########
@@ -267,6 +276,12 @@ impl GcsCore {
let mut req = Request::post(&url);
+ if let Some(user_metadata) = op.user_metadata() {
+ for (key, value) in user_metadata {
+ req = req.header(format!("{X_GOOG_META_PREFIX}{key}"), value)
Review Comment:
In the details section of the first documentation you depicted, it says:
> Any custom metadata set in the [initial request](https://cloud.google.com/storage/docs/performing-resumable-uploads#initiate-session) that uses the same KEY string is overwritten using the value provided in the X-Goog-Meta- header.
And in https://cloud.google.com/storage/docs/performing-resumable-uploads#single-chunk-upload It seems that it is allowed in single chunks upload, as I think it is done here:
As you raised this, in multipart uploads I think we should include the headers in the `gcs_complete_multipart_upload`
In the behaviour tests are there test for both multipart and single chunk uploads with gcs? I would like to check that both `gcs_complete_multipart_upload` and `gcs_insert_object_request` set the metadata successfully.
--
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(core/services-gcs): support user defined metadata [opendal]
Posted by "jorgehermo9 (via GitHub)" <gi...@apache.org>.
jorgehermo9 commented on code in PR #5276:
URL: https://github.com/apache/opendal/pull/5276#discussion_r1828475116
##########
core/src/services/gcs/core.rs:
##########
@@ -269,37 +276,28 @@ impl GcsCore {
req = req.header(CONTENT_LENGTH, size.unwrap_or_default());
- if metadata.is_empty() {
- if let Some(content_type) = op.content_type() {
- req = req.header(CONTENT_TYPE, content_type);
- }
-
+ if request_metadata.is_empty() {
Review Comment:
I don't know if we should handle this case, or always use `uploadType=multipart`. It can happen that the `request_metadata` serialization is an empty json (`{ }`) and this check does not use multipart if thats the case.
I'm not sure if it would be better to use always `uploadType=multipart`. I don't know if there are any problem at GCS if we send an empty metadata though... If the request can fail if the metadata is empty, then we must do this check
--
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(core/services-gcs): support user defined metadata [opendal]
Posted by "jorgehermo9 (via GitHub)" <gi...@apache.org>.
jorgehermo9 commented on code in PR #5276:
URL: https://github.com/apache/opendal/pull/5276#discussion_r1827645793
##########
core/src/services/gcs/core.rs:
##########
@@ -267,6 +276,12 @@ impl GcsCore {
let mut req = Request::post(&url);
+ if let Some(user_metadata) = op.user_metadata() {
+ for (key, value) in user_metadata {
+ req = req.header(format!("{X_GOOG_META_PREFIX}{key}"), value)
Review Comment:
Hi, I understand what you mean. Include the user metadata in https://github.com/apache/opendal/blob/330bd74f3ace74f15966dfb2465191aa7c1ae5a6/core/src/services/gcs/core.rs#L303.
(and I also have to change https://github.com/apache/opendal/blob/330bd74f3ace74f15966dfb2465191aa7c1ae5a6/core/src/services/gcs/core.rs#L268 to `multipart/related`)
I have doubts about how this will behave if we mix _system_ metadata such as `storageClass` and `cacheControl` with user-defined metadata in that request.
I can't propertly test it thought... The command `OPENDAL_TEST=gcs cargo test behavior --features tests` is failing for me in local. It worked with `OPENDAL_TEST=azblob`...
--
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(core/services-gcs): support user defined metadata [opendal]
Posted by "jorgehermo9 (via GitHub)" <gi...@apache.org>.
jorgehermo9 commented on code in PR #5276:
URL: https://github.com/apache/opendal/pull/5276#discussion_r1827645793
##########
core/src/services/gcs/core.rs:
##########
@@ -267,6 +276,12 @@ impl GcsCore {
let mut req = Request::post(&url);
+ if let Some(user_metadata) = op.user_metadata() {
+ for (key, value) in user_metadata {
+ req = req.header(format!("{X_GOOG_META_PREFIX}{key}"), value)
Review Comment:
Hi, I understand what you mean. Include the user metadata in https://github.com/apache/opendal/blob/330bd74f3ace74f15966dfb2465191aa7c1ae5a6/core/src/services/gcs/core.rs#L303.
(and I also have to change https://github.com/apache/opendal/blob/330bd74f3ace74f15966dfb2465191aa7c1ae5a6/core/src/services/gcs/core.rs#L268 to `multipart/related`)
I have doubts about how this will behave if we mix _system_ metadata such as `storageClass` and `cacheControl` with user-defined metadata in that request.
I will try to run some test in local.
--
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(core/services-gcs): support user defined metadata [opendal]
Posted by "jorgehermo9 (via GitHub)" <gi...@apache.org>.
jorgehermo9 commented on code in PR #5276:
URL: https://github.com/apache/opendal/pull/5276#discussion_r1827766308
##########
core/src/services/gcs/core.rs:
##########
@@ -267,6 +276,12 @@ impl GcsCore {
let mut req = Request::post(&url);
+ if let Some(user_metadata) = op.user_metadata() {
+ for (key, value) in user_metadata {
+ req = req.header(format!("{X_GOOG_META_PREFIX}{key}"), value)
Review Comment:
I understand it better with that type. The hashmap that is currently in the code was confusing me!
--
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(core/services-gcs): support user defined metadata [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo merged PR #5276:
URL: https://github.com/apache/opendal/pull/5276
--
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(core/services-gcs): support user defined metadata [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #5276:
URL: https://github.com/apache/opendal/pull/5276#discussion_r1827762415
##########
core/src/services/gcs/core.rs:
##########
@@ -267,6 +276,12 @@ impl GcsCore {
let mut req = Request::post(&url);
+ if let Some(user_metadata) = op.user_metadata() {
+ for (key, value) in user_metadata {
+ req = req.header(format!("{X_GOOG_META_PREFIX}{key}"), value)
Review Comment:
We can improve this part by writing this type directly without using `json!()`, if you prefer.
For example:
```rust
#[serde(default, rename_all = "camelCase")]
struct InsertRequestBody {
#[serde(skip_serializing_if = "Option::is_none")]
content_type: Option<String>,
#[serde(skip_serializing_if = "HashMap::is_empty")]
metadata: HashMap<String, String>
}
```
--
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(core/services-gcs): support user defined metadata [opendal]
Posted by "jorgehermo9 (via GitHub)" <gi...@apache.org>.
jorgehermo9 commented on code in PR #5276:
URL: https://github.com/apache/opendal/pull/5276#discussion_r1829721908
##########
core/src/services/gcs/core.rs:
##########
@@ -269,37 +276,28 @@ impl GcsCore {
req = req.header(CONTENT_LENGTH, size.unwrap_or_default());
- if metadata.is_empty() {
- if let Some(content_type) = op.content_type() {
- req = req.header(CONTENT_TYPE, content_type);
- }
-
+ if request_metadata.is_empty() {
Review Comment:
Agree with that!
--
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(core/services-gcs): support user defined metadata [opendal]
Posted by "jorgehermo9 (via GitHub)" <gi...@apache.org>.
jorgehermo9 commented on code in PR #5276:
URL: https://github.com/apache/opendal/pull/5276#discussion_r1827028305
##########
core/src/services/gcs/backend.rs:
##########
@@ -424,6 +426,10 @@ impl Access for GcsBackend {
m.set_last_modified(parse_datetime_from_rfc3339(&meta.updated)?);
+ if let Some(user_metadata) = meta.metadata {
+ m.with_user_metadata(user_metadata);
Review Comment:
done in [#a6478ef](https://github.com/apache/opendal/pull/5276/commits/a6478efa70ac655a3e1f0ae796c79a7eac75537b)
##########
core/src/services/gcs/backend.rs:
##########
@@ -424,6 +426,10 @@ impl Access for GcsBackend {
m.set_last_modified(parse_datetime_from_rfc3339(&meta.updated)?);
+ if let Some(user_metadata) = meta.metadata {
+ m.with_user_metadata(user_metadata);
Review Comment:
done in https://github.com/apache/opendal/pull/5276/commits/a6478efa70ac655a3e1f0ae796c79a7eac75537b
--
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(core/services-gcs): support user defined metadata [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on PR #5276:
URL: https://github.com/apache/opendal/pull/5276#issuecomment-2453494232
> I didn't see the CI for GCS behaviour tests.
Thank you, @jorgehermo9, for working on this.
We do have tests for GCS, but GitHub has security reasons for not allowing PRs from forks to use them. (This can be addressed once you become an OpenDAL committer!)
I will try to trigger the test for you.
--
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(core/services-gcs): support user defined metadata [opendal]
Posted by "jorgehermo9 (via GitHub)" <gi...@apache.org>.
jorgehermo9 commented on code in PR #5276:
URL: https://github.com/apache/opendal/pull/5276#discussion_r1827010176
##########
core/src/services/gcs/core.rs:
##########
@@ -318,16 +333,22 @@ impl GcsCore {
let mut req = Request::put(&url);
+ if let Some(user_metadata) = args.user_metadata() {
+ for (key, value) in user_metadata {
+ req = req.header(format!("{X_GOOG_META_PREFIX}{key}"), value)
+ }
+ }
+
if let Some(content_type) = args.content_type() {
req = req.header(CONTENT_TYPE, content_type);
}
if let Some(acl) = &self.predefined_acl {
- req = req.header("x-goog-acl", acl);
+ req = req.header(X_GOOG_ACL, acl);
Review Comment:
refactored these constants in order to be consistent with `s3`, `azblob` and `oss` services
--
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(core/services-gcs): support user defined metadata [opendal]
Posted by "jorgehermo9 (via GitHub)" <gi...@apache.org>.
jorgehermo9 commented on code in PR #5276:
URL: https://github.com/apache/opendal/pull/5276#discussion_r1827764692
##########
core/src/services/gcs/core.rs:
##########
@@ -267,6 +276,12 @@ impl GcsCore {
let mut req = Request::post(&url);
+ if let Some(user_metadata) = op.user_metadata() {
+ for (key, value) in user_metadata {
+ req = req.header(format!("{X_GOOG_META_PREFIX}{key}"), value)
Review Comment:
Oh, I understand now. Crystal clear 😀
It is my first time reading gcs documentation and I'm sorry for the inconveniences
thanks! I'll take a look into 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: [PR] feat(core/services-gcs): support user defined metadata [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #5276:
URL: https://github.com/apache/opendal/pull/5276#discussion_r1827825347
##########
core/src/services/gcs/core.rs:
##########
@@ -267,6 +276,12 @@ impl GcsCore {
let mut req = Request::post(&url);
+ if let Some(user_metadata) = op.user_metadata() {
+ for (key, value) in user_metadata {
+ req = req.header(format!("{X_GOOG_META_PREFIX}{key}"), value)
Review Comment:
> It is my first time reading gcs documentation and I'm sorry for the inconveniences
Never mind. The GCS documentation is really poor; I've read it many times to find those details.
> I understand it better with that type. The hashmap that is currently in the code was confusing me!
Welcome to improve them!
--
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(core/services-gcs): support user defined metadata [opendal]
Posted by "jorgehermo9 (via GitHub)" <gi...@apache.org>.
jorgehermo9 commented on code in PR #5276:
URL: https://github.com/apache/opendal/pull/5276#discussion_r1828478748
##########
core/src/services/gcs/core.rs:
##########
@@ -267,6 +276,12 @@ impl GcsCore {
let mut req = Request::post(&url);
+ if let Some(user_metadata) = op.user_metadata() {
+ for (key, value) in user_metadata {
+ req = req.header(format!("{X_GOOG_META_PREFIX}{key}"), value)
Review Comment:
I think I addressed it at c2074104d55118645770a48e92a509c07ad5e548. I hope it looks more like what you meant.
Can you trigger CI and see how it breaks..?
--
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(core/services-gcs): support user defined metadata [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #5276:
URL: https://github.com/apache/opendal/pull/5276#discussion_r1827025740
##########
core/src/services/gcs/backend.rs:
##########
@@ -424,6 +426,10 @@ impl Access for GcsBackend {
m.set_last_modified(parse_datetime_from_rfc3339(&meta.updated)?);
+ if let Some(user_metadata) = meta.metadata {
+ m.with_user_metadata(user_metadata);
Review Comment:
> should I include a `!user_meta.is_empty()` check before adding the metadata? as done in other services.
I'm thinking about storing `HashMap<String, String>` always to avoid those duplicated checks.
--
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(core/services-gcs): support user defined metadata [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #5276:
URL: https://github.com/apache/opendal/pull/5276#discussion_r1827022045
##########
core/src/services/gcs/core.rs:
##########
@@ -318,16 +333,22 @@ impl GcsCore {
let mut req = Request::put(&url);
+ if let Some(user_metadata) = args.user_metadata() {
Review Comment:
Yes, I think it should. Presigned requests are just normal read/write requests, but signed in a different way.
--
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(core/services-gcs): support user defined metadata [opendal]
Posted by "jorgehermo9 (via GitHub)" <gi...@apache.org>.
jorgehermo9 commented on code in PR #5276:
URL: https://github.com/apache/opendal/pull/5276#discussion_r1829730940
##########
core/src/services/gcs/core.rs:
##########
@@ -269,37 +274,28 @@ impl GcsCore {
req = req.header(CONTENT_LENGTH, size.unwrap_or_default());
- if metadata.is_empty() {
- if let Some(content_type) = op.content_type() {
- req = req.header(CONTENT_TYPE, content_type);
- }
-
+ if request_metadata.is_empty() {
+ // If the metadata is empty, we do not set any `Content-Type` header,
+ // since if we had it in the `op.content_type()`, it would be already set in the
+ // `multipart` metadata body and this branch won't be executed.
let req = req.body(body).map_err(new_request_build_error)?;
Ok(req)
} else {
let mut multipart = Multipart::new();
-
- multipart = multipart.part(
- FormDataPart::new("metadata")
- .header(
- CONTENT_TYPE,
- "application/json; charset=UTF-8".parse().unwrap(),
- )
- .content(json!(metadata).to_string()),
- );
-
- let mut media_part = FormDataPart::new("media").content(body);
-
- if let Some(content_type) = op.content_type() {
- media_part = media_part.header(
+ let metadata_part = FormDataPart::new("metadata")
+ .header(
CONTENT_TYPE,
- content_type
- .parse()
- .map_err(|_| Error::new(ErrorKind::Unexpected, "invalid header value"))?,
+ "application/json; charset=UTF-8".parse().unwrap(),
+ )
+ .content(
+ serde_json::to_string(&request_metadata)
Review Comment:
addressed in https://github.com/apache/opendal/pull/5276/commits/5cd78085d24ac96f8faccb412bffdf441ed23de3
--
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(core/services-gcs): support user defined metadata [opendal]
Posted by "jorgehermo9 (via GitHub)" <gi...@apache.org>.
jorgehermo9 commented on code in PR #5276:
URL: https://github.com/apache/opendal/pull/5276#discussion_r1827010400
##########
core/src/services/gcs/core.rs:
##########
@@ -318,16 +333,22 @@ impl GcsCore {
let mut req = Request::put(&url);
+ if let Some(user_metadata) = args.user_metadata() {
Review Comment:
I'm not sure if this metadata should be set in the headers of presign requests. Any thoughts about this?
--
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(core/services-gcs): support user defined metadata [opendal]
Posted by "jorgehermo9 (via GitHub)" <gi...@apache.org>.
jorgehermo9 commented on code in PR #5276:
URL: https://github.com/apache/opendal/pull/5276#discussion_r1828476964
##########
core/src/services/gcs/core.rs:
##########
@@ -318,16 +316,22 @@ impl GcsCore {
let mut req = Request::put(&url);
+ if let Some(user_metadata) = args.user_metadata() {
+ for (key, value) in user_metadata {
Review Comment:
Should we keep this? In the `gcs_insert_object_request` we are using the multipart body now... I don't know if this would work
--
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(core/services-gcs): support user defined metadata [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #5276:
URL: https://github.com/apache/opendal/pull/5276#discussion_r1827028270
##########
core/src/services/gcs/core.rs:
##########
@@ -267,6 +276,12 @@ impl GcsCore {
let mut req = Request::post(&url);
+ if let Some(user_metadata) = op.user_metadata() {
+ for (key, value) in user_metadata {
+ req = req.header(format!("{X_GOOG_META_PREFIX}{key}"), value)
Review Comment:
> As you raised this, in multipart uploads I think we should include the headers in the
Hi, the `multipart upload` is different from google's `uploadType=multipart`. Using `uploadType=multipart` inside `insert` means upload metadata with data at the same time via HTTP [Multipart Form](https://developer.mozilla.org/en-US/docs/Web/HTTP/Methods/POST#multipart_form_submission). It's a different thing from s3's multipart upload which is in different requests.
--
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(core/services-gcs): support user defined metadata [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #5276:
URL: https://github.com/apache/opendal/pull/5276#discussion_r1827025251
##########
core/src/services/gcs/core.rs:
##########
@@ -267,6 +276,12 @@ impl GcsCore {
let mut req = Request::post(&url);
+ if let Some(user_metadata) = op.user_metadata() {
+ for (key, value) in user_metadata {
+ req = req.header(format!("{X_GOOG_META_PREFIX}{key}"), value)
Review Comment:
Hi, this seems only apply to [when used the final request of a resumable upload](https://cloud.google.com/storage/docs/json_api/v1/parameters#xgoogmeta).
To set custom metadata, we need to use `insert`'s `uploadType=multipart`. Read https://cloud.google.com/storage/docs/uploading-objects 's `JSON API multipart upload (a single-request upload that includes object metadata)` for more information.
--
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(core/services-gcs): support user defined metadata [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #5276:
URL: https://github.com/apache/opendal/pull/5276#discussion_r1827752963
##########
core/src/services/gcs/core.rs:
##########
@@ -267,6 +276,12 @@ impl GcsCore {
let mut req = Request::post(&url);
+ if let Some(user_metadata) = op.user_metadata() {
+ for (key, value) in user_metadata {
+ req = req.header(format!("{X_GOOG_META_PREFIX}{key}"), value)
Review Comment:
Hi, @jorgehermo9, the `metadata` json we sent to gcs will be like:
```json
{
"contentType": "application/pdf",
"metadata": {
"location": "anywhere"
}
}
```
As shown as https://cloud.google.com/storage/docs/json_api/v1/objects/insert#request-body.
So we are not mixing them with `user-defined metadata`.
--
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(core/services-gcs): support user defined metadata [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #5276:
URL: https://github.com/apache/opendal/pull/5276#discussion_r1828619928
##########
core/src/services/gcs/core.rs:
##########
@@ -269,37 +274,28 @@ impl GcsCore {
req = req.header(CONTENT_LENGTH, size.unwrap_or_default());
- if metadata.is_empty() {
- if let Some(content_type) = op.content_type() {
- req = req.header(CONTENT_TYPE, content_type);
- }
-
+ if request_metadata.is_empty() {
+ // If the metadata is empty, we do not set any `Content-Type` header,
+ // since if we had it in the `op.content_type()`, it would be already set in the
+ // `multipart` metadata body and this branch won't be executed.
let req = req.body(body).map_err(new_request_build_error)?;
Ok(req)
} else {
let mut multipart = Multipart::new();
-
- multipart = multipart.part(
- FormDataPart::new("metadata")
- .header(
- CONTENT_TYPE,
- "application/json; charset=UTF-8".parse().unwrap(),
- )
- .content(json!(metadata).to_string()),
- );
-
- let mut media_part = FormDataPart::new("media").content(body);
-
- if let Some(content_type) = op.content_type() {
- media_part = media_part.header(
+ let metadata_part = FormDataPart::new("metadata")
+ .header(
CONTENT_TYPE,
- content_type
- .parse()
- .map_err(|_| Error::new(ErrorKind::Unexpected, "invalid header value"))?,
+ "application/json; charset=UTF-8".parse().unwrap(),
+ )
+ .content(
+ serde_json::to_string(&request_metadata)
Review Comment:
We can use `serde_json::to_vec` here to avoid extra cost of UTF-8 check.
##########
core/src/services/gcs/core.rs:
##########
@@ -318,16 +316,22 @@ impl GcsCore {
let mut req = Request::put(&url);
+ if let Some(user_metadata) = args.user_metadata() {
+ for (key, value) in user_metadata {
Review Comment:
XML API is different from JSON API; we need to keep this.
##########
core/src/services/gcs/core.rs:
##########
@@ -269,37 +276,28 @@ impl GcsCore {
req = req.header(CONTENT_LENGTH, size.unwrap_or_default());
- if metadata.is_empty() {
- if let Some(content_type) = op.content_type() {
- req = req.header(CONTENT_TYPE, content_type);
- }
-
+ if request_metadata.is_empty() {
Review Comment:
`Multipart` involves additional handling of multipart encoding, so it's better to avoid those costs when users only want to write data.
--
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(core/services-gcs): support user defined metadata [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on PR #5276:
URL: https://github.com/apache/opendal/pull/5276#issuecomment-2456061033
Nice work!
![Uploading image.png…]()
--
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