services/cos: Implement Write Returns Metadata for cos#5777
services/cos: Implement Write Returns Metadata for cos#5777Xuanwo merged 3 commits intoapache:mainfrom
Conversation
|
Hey, @meteorgan @Xuanwo! The behavior tests of metadata have been fixed, Would you have to take a look? :) |
|
There shouldn't be any failed behavior tests. If a feature hasn't been implemented yet, the corresponding test should not be executed. we have some conditions to control this. for example: |
|
|
||
| let resp = self.core.send(req).await?; | ||
|
|
||
| let meta = Self::parse_metadata(resp.headers())?; |
There was a problem hiding this comment.
In #5710 (comment), we found that cos may return null as the version id which could cause test_write_with_append_returns_metadata to fail. However this issue hasn't been addressed here. Did i miss something ? Also, when running behavior tests, did you enable OPENDAL_COS_ENABLE_VERSIONING ?
There was a problem hiding this comment.
I prefer we skip the version id when it's null. what do you think ? @Xuanwo
core/src/services/cos/writer.rs
Outdated
| .await?; | ||
|
|
||
| let result: CompleteMultipartUploadResult = | ||
| quick_xml::de::from_reader(resp.body().clone().reader()) |
There was a problem hiding this comment.
we can avoid this clone by moving resp.status() before this line.
There was a problem hiding this comment.
resp.body() is a shared reference. Clone or mutable reference will work.
core/src/services/cos/writer.rs
Outdated
| // COS return null as the version id when it's an appendable object. | ||
| // Refer to https://cloud.tencent.com/document/product/436/7741 | ||
| meta.set_version("null"); |
There was a problem hiding this comment.
I should've made it clear: we don't need to return null as version id since it doesn't make sense to users. If parsing version id returns in null, we simply shouldn't set it.
There was a problem hiding this comment.
the stat method in backend.rs may have the same issue.
Signed-off-by: Ziy1-Tan <[email protected]>
Signed-off-by: Ziy1-Tan <[email protected]>
Signed-off-by: Ziy1-Tan <[email protected]>
|
Thank you @Ziy1-Tan for the great work and thank you @meteorgan for the review! |
3 participants
Which issue does this PR close?
Part of #5557 .
Test result of
OPENDAL_COS_ENABLE_VERSIONING=true OPENDAL_TEST=cos cargo test behavior::test_write --features testsRationale for this change
What changes are included in this PR?
Are there any user-facing changes?