You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@opendal.apache.org by er...@apache.org on 2026/08/16 12:39:42 UTC
(opendal) branch main updated: fix(services/onedrive): fix upload and listing behavior (#8080)
This is an automated email from the ASF dual-hosted git repository.
erickguan pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/opendal.git
The following commit(s) were added to refs/heads/main by this push:
new 614d77e41 fix(services/onedrive): fix upload and listing behavior (#8080)
614d77e41 is described below
commit 614d77e4117663c6740a258c0ac94494f8af80d0
Author: Erick Guan <29...@users.noreply.github.com>
AuthorDate: Sun Aug 16 20:39:35 2026 +0800
fix(services/onedrive): fix upload and listing behavior (#8080)
---
.../java/org/apache/opendal/ServiceConfig.java | 4 +-
bindings/python/python/opendal/config.py | 2 +-
core/services/onedrive/src/backend.rs | 25 ++++----
core/services/onedrive/src/config.rs | 4 +-
core/services/onedrive/src/core.rs | 67 ++++++++++------------
core/services/onedrive/src/graph_model.rs | 61 ++++----------------
core/services/onedrive/src/lister.rs | 59 +++++++++++++------
core/services/onedrive/src/reader.rs | 11 ++++
website/data/services.json | 4 +-
9 files changed, 111 insertions(+), 126 deletions(-)
diff --git a/bindings/java/src/main/java/org/apache/opendal/ServiceConfig.java b/bindings/java/src/main/java/org/apache/opendal/ServiceConfig.java
index ff1b9def6..11c833cbf 100644
--- a/bindings/java/src/main/java/org/apache/opendal/ServiceConfig.java
+++ b/bindings/java/src/main/java/org/apache/opendal/ServiceConfig.java
@@ -2320,9 +2320,9 @@ public interface ServiceConfig {
*/
public final String clientSecret;
/**
- * <p>Deprecated: OneDrive versioning capability is enabled by default.</p>
+ * <p>Deprecated: OneDrive supports version listing without this option.</p>
*
- * @deprecated OneDrive versioning capability is enabled by default and this option is no longer needed.
+ * @deprecated OneDrive supports version listing without this option.
*/
public final Boolean enableVersioning;
/**
diff --git a/bindings/python/python/opendal/config.py b/bindings/python/python/opendal/config.py
index 3e67dd98a..bfb04e09d 100644
--- a/bindings/python/python/opendal/config.py
+++ b/bindings/python/python/opendal/config.py
@@ -576,7 +576,7 @@ class OnedriveConfig(TypedDict):
client_secret: NotRequired[str]
"""Microsoft Graph API Application client secret that is in the Azure's app registration portal"""
enable_versioning: NotRequired[bool]
- """Deprecated: OneDrive versioning capability is enabled by default. [Deprecated since 0.57.0] OneDrive versioning capability is enabled by default and this option is no longer needed."""
+ """Deprecated: OneDrive supports version listing without this option. [Deprecated since 0.57.0] OneDrive supports version listing without this option."""
refresh_token: NotRequired[str]
"""Microsoft Graph API (also OneDrive API) refresh token"""
root: NotRequired[str | os.PathLike[str]]
diff --git a/core/services/onedrive/src/backend.rs b/core/services/onedrive/src/backend.rs
index d2f8597e2..492101d94 100644
--- a/core/services/onedrive/src/backend.rs
+++ b/core/services/onedrive/src/backend.rs
@@ -24,6 +24,7 @@ use opendal_core::*;
use super::core::OneDriveCore;
use super::core::parse_error;
+use super::core::parse_error_with_retry;
use super::deleter::OneDriveDeleter;
use super::lister::OneDriveLister;
use super::reader::*;
@@ -108,10 +109,10 @@ impl OnedriveBuilder {
self
}
- /// Deprecated: OneDrive versioning capability is enabled by default.
+ /// Deprecated: OneDrive supports version listing without this option.
#[deprecated(
since = "0.57.0",
- note = "OneDrive versioning capability is enabled by default and this option is no longer needed."
+ note = "OneDrive supports version listing without this option."
)]
pub fn enable_versioning(self, _enabled: bool) -> Self {
self
@@ -133,23 +134,22 @@ impl Builder for OnedriveBuilder {
write: true,
write_with_if_match: true,
- // OneDrive supports the file size up to 250GB
// Read more at https://support.microsoft.com/en-us/office/restrictions-and-limitations-in-onedrive-and-sharepoint-64883a5d-228e-48f5-b3d2-eb39e07630fa#individualfilesize
- // However, we can't enable this, otherwise OpenDAL behavior tests will try to test creating huge
- // file up to this size.
- // write_total_max_size: Some(250 * 1024 * 1024 * 1024),
+ write_total_max_size: Some(250 * 1024 * 1024 * 1024), // 250GB
copy: true,
rename: true,
stat: true,
stat_with_if_none_match: true,
- stat_with_version: true,
-
+ // Microsoft Graph doesn't preserve complete metadata for previous
+ // file versions, so OneDrive can't implement stat_with_version.
+ // See https://learn.microsoft.com/en-us/graph/api/driveitem-list-versions?view=graph-rest-1.0#remarks
delete: true,
create_dir: true,
list: true,
list_with_limit: true,
+ list_with_start_after: true,
list_with_versions: true,
shared: true,
@@ -245,6 +245,7 @@ impl Service for OnedriveBackend {
let response = self.core.onedrive_create_dir(ctx, path).await?;
match response.status() {
StatusCode::CREATED | StatusCode::OK => Ok(RpCreateDir::default()),
+ StatusCode::BAD_REQUEST => Err(parse_error_with_retry(response)),
_ => Err(parse_error(response)),
}
}
@@ -329,13 +330,7 @@ impl Service for OnedriveBackend {
fn list(&self, ctx: &OperationContext, path: &str, args: OpList) -> Result<Self::Lister> {
let output: oio::PageLister<OneDriveLister> = {
- let l = OneDriveLister::new(
- path.to_string(),
- self.core.clone(),
- ctx.clone(),
- self.core.capability,
- &args,
- );
+ let l = OneDriveLister::new(path.to_string(), self.core.clone(), ctx.clone(), &args);
Ok(oio::PageLister::new(l))
}?;
diff --git a/core/services/onedrive/src/config.rs b/core/services/onedrive/src/config.rs
index 5979688fd..d921d7274 100644
--- a/core/services/onedrive/src/config.rs
+++ b/core/services/onedrive/src/config.rs
@@ -38,10 +38,10 @@ pub struct OnedriveConfig {
pub client_id: Option<String>,
/// Microsoft Graph API Application client secret that is in the Azure's app registration portal
pub client_secret: Option<String>,
- /// Deprecated: OneDrive versioning capability is enabled by default.
+ /// Deprecated: OneDrive supports version listing without this option.
#[deprecated(
since = "0.57.0",
- note = "OneDrive versioning capability is enabled by default and this option is no longer needed."
+ note = "OneDrive supports version listing without this option."
)]
pub enable_versioning: bool,
}
diff --git a/core/services/onedrive/src/core.rs b/core/services/onedrive/src/core.rs
index 849b19547..510af2f4f 100644
--- a/core/services/onedrive/src/core.rs
+++ b/core/services/onedrive/src/core.rs
@@ -132,6 +132,9 @@ impl OneDriveCore {
serde_json::from_reader(bytes.reader())
.map_err(new_json_deserialize_error)?
}
+ StatusCode::BAD_REQUEST => {
+ return Err(parse_error_with_retry(response));
+ }
_ => return Err(parse_error(response)),
}
}
@@ -164,24 +167,14 @@ const MONITOR_WAIT_SECOND: u64 = 1;
// `services-onedrive` uses the file path based API for simplicity.
// Read more at https://learn.microsoft.com/en-us/graph/onedrive-addressing-driveitems
impl OneDriveCore {
- /// Send a stat request about a particular path, including:
- ///
- /// - Get stat object only if ETag not matches
- /// - whether to get the object version
- ///
- /// See also [`onedrive_get_stat_plain()`].
+ /// Send a stat request about a particular path.
pub(crate) async fn onedrive_stat(
&self,
ctx: &OperationContext,
path: &str,
args: OpStat,
) -> Result<Metadata> {
- let mut url: String = self.onedrive_item_url(path, true);
- if args.version().is_some() {
- url += "?$expand=versions(";
- url += VERSION_SELECT_PARAM;
- url += ")";
- }
+ let url: String = self.onedrive_item_url(path, true);
let mut request = Request::get(&url);
if let Some(etag) = args.if_none_match() {
@@ -214,22 +207,6 @@ impl OneDriveCore {
.with_etag(decoded_response.e_tag)
.with_content_length(decoded_response.size.max(0) as u64);
- if let Some(version) = args.version() {
- for item_version in decoded_response.versions.as_deref().unwrap_or_default() {
- if item_version.id == version {
- meta.set_version(version);
- break; // early exit
- }
- }
-
- if meta.version().is_none() {
- return Err(Error::new(
- ErrorKind::NotFound,
- "cannot find this version of the item",
- ));
- }
- }
-
let last_modified = decoded_response.last_modified_date_time;
let date_utc_last_modified = last_modified.parse::<Timestamp>()?;
meta.set_last_modified(date_utc_last_modified);
@@ -263,6 +240,9 @@ impl OneDriveCore {
self.sign(ctx, &mut request).await?;
let response = ctx.http_transport().send(request).await?;
+ if !response.status().is_success() {
+ return Err(parse_error(response));
+ }
let decoded_response: GraphApiOneDriveVersionsResponse =
serde_json::from_reader(response.into_body().reader())
.map_err(new_json_deserialize_error)?;
@@ -453,11 +433,10 @@ impl OneDriveCore {
path: &str,
args: &OpWrite,
) -> Result<Response<Buffer>> {
- let parent_path = get_parent(path);
let file_name = get_basename(path);
let url = format!(
"{}:/createUploadSession",
- self.onedrive_item_url(parent_path, true),
+ self.onedrive_item_url(path, true),
);
let mut request = Request::post(url).header(header::CONTENT_TYPE, "application/json");
@@ -610,6 +589,7 @@ impl OneDriveCore {
)
})
.map(String::from),
+ StatusCode::BAD_REQUEST => Err(parse_error_with_retry(response)),
_ => Err(parse_error(response)),
}
}
@@ -896,13 +876,7 @@ mod tests {
async fn list_root_returns_entries() {
let core = test_core("/");
let ctx = test_ctx();
- let lister = OneDriveLister::new(
- "/".to_string(),
- core,
- ctx,
- Capability::default(),
- &OpList::default(),
- );
+ let lister = OneDriveLister::new("/".to_string(), core, ctx, &OpList::default());
let mut lister = oio::PageLister::new(lister);
let mut entries = Vec::new();
@@ -921,6 +895,7 @@ mod error {
use http::Response;
use http::StatusCode;
+ use crate::graph_model::GraphErrorResponse;
use opendal_core::raw::*;
use opendal_core::*;
@@ -962,6 +937,24 @@ mod error {
err
}
+
+ /// Parse a consistency-lag error and mark it temporary.
+ ///
+ /// Other errors retain the classification from [`parse_error`].
+ pub(crate) fn parse_error_with_retry(response: Response<Buffer>) -> Error {
+ let retryable = is_consistency_lag_error(&response.body().to_bytes());
+ let err = parse_error(response);
+
+ if retryable { err.set_temporary() } else { err }
+ }
+
+ fn is_consistency_lag_error(body: &[u8]) -> bool {
+ let Ok(response) = serde_json::from_slice::<GraphErrorResponse>(body) else {
+ return false;
+ };
+
+ response.error.code == "invalidRequest"
+ }
}
pub(super) use error::*;
diff --git a/core/services/onedrive/src/graph_model.rs b/core/services/onedrive/src/graph_model.rs
index 3fe407c0a..abc3cc2fb 100644
--- a/core/services/onedrive/src/graph_model.rs
+++ b/core/services/onedrive/src/graph_model.rs
@@ -18,6 +18,16 @@
use serde::Deserialize;
use serde::Serialize;
+#[derive(Debug, Deserialize)]
+pub struct GraphErrorResponse {
+ pub error: GraphError,
+}
+
+#[derive(Debug, Deserialize)]
+pub struct GraphError {
+ pub code: String,
+}
+
#[derive(Debug, Deserialize)]
pub struct GraphOAuthRefreshTokenResponseBody {
pub access_token: String,
@@ -56,7 +66,6 @@ pub struct OneDriveItem {
pub parent_reference: ParentReference,
#[serde(flatten)]
pub item_type: ItemType,
- pub versions: Option<Vec<OneDriveItemVersion>>,
}
#[derive(Debug, Clone, Serialize, Deserialize)]
@@ -327,7 +336,6 @@ mod tests {
assert_eq!(item.e_tag, "\"{3B131E1C-7D81-20AF-80D0-720000000000},2\"");
assert_eq!(item.size, 3);
assert_eq!(item.parent_reference.id, "A0AA0A000A000A0A!113");
- assert!(item.versions.is_none());
if let ItemType::File { file, .. } = &item.item_type {
assert_eq!(file.mime_type, "text/plain");
} else {
@@ -335,55 +343,6 @@ mod tests {
}
}
- #[test]
- fn test_parse_one_drive_file_with_version_json() {
- let data = r#"{
- "@odata.context": "https://graph.microsoft.com/v1.0/$metadata#users('opendal%40outlook.com')/drive/root(id,name,lastModifiedDateTime,eTag,size,parentReference,folder,file)/$entity",
- "@odata.etag": "\"{3B131E1C-7D81-20AF-80D0-720000000000},2\"",
- "eTag": "\"{3B131E1C-7D81-20AF-80D0-720000000000},2\"",
- "id": "A0AA0A000A000A0A!114",
- "lastModifiedDateTime": "2025-02-16T19:49:05Z",
- "name": "filename.txt",
- "size": 3,
- "parentReference": {
- "driveType": "personal",
- "driveId": "A0AA0A000A000A0A",
- "id": "A0AA0A000A000A0A!113",
- "name": "folder_a",
- "path": "/drive/root:/folder_a",
- "siteId": "5f1f11f3-a6b4-4414-aee0-215c774f80db"
- },
- "file": {
- "mimeType": "text/plain",
- "hashes": {
- "quickXorHash": "79jFLwAAAAAAAAAAAwAAAAAAAAA=",
- "sha1Hash": "57218C316B6921E2CD61027A2387EDC31A2D9471",
- "sha256Hash": "F1945CD6C19E56B3C1C78943EF5EC18116907A4CA1EFC40A57D48AB1DB7ADFC5"
- }
- },
- "versions@odata.context": "https://graph.microsoft.com/v1.0/$metadata#users('opendal%40outlook.com')/drive/root/versions",
- "versions": [
- {
- "@microsoft.graph.downloadUrl": "https://my.microsoftpersonalcontent.com/personal/A0AA0A000A000A0A/_layouts/15/download.aspx?UniqueId=3b131e1c-7d81-20af-80d0-720000000000&Translate=false&tempauth=v1e.a&ApiVersion=2.0",
- "id": "1.0",
- "lastModifiedDateTime": "2025-02-16T19:49:05Z",
- "size": 3,
- "lastModifiedBy": {
- "user": {
- "email": "erickgdev@outlook.com",
- "displayName": "erickgdev@outlook.com"
- }
- }
- }
- ]
- }"#;
-
- let item: OneDriveItem = serde_json::from_str(data).unwrap();
- let versions = item.versions.expect("Versions present");
- assert_eq!("1.0", versions[0].id);
- assert_eq!("2025-02-16T19:49:05Z", versions[0].last_modified_date_time);
- }
-
#[test]
fn test_parse_one_drive_monitor_status_json() {
let data = r#"{
diff --git a/core/services/onedrive/src/lister.rs b/core/services/onedrive/src/lister.rs
index 8dce0d07b..5493c241f 100644
--- a/core/services/onedrive/src/lister.rs
+++ b/core/services/onedrive/src/lister.rs
@@ -31,7 +31,6 @@ use super::graph_model::ItemType;
pub struct OneDriveLister {
core: Arc<OneDriveCore>,
ctx: OperationContext,
- capability: Capability,
path: String,
op: OpList,
}
@@ -43,17 +42,21 @@ impl OneDriveLister {
path: String,
core: Arc<OneDriveCore>,
ctx: OperationContext,
- capability: Capability,
args: &OpList,
) -> Self {
Self {
core,
ctx,
- capability,
path,
op: args.clone(),
}
}
+
+ fn is_after_start(&self, path: &str) -> bool {
+ self.op
+ .start_after()
+ .is_none_or(|start_after| path > start_after)
+ }
}
impl oio::PageList for OneDriveLister {
@@ -81,7 +84,7 @@ impl oio::PageList for OneDriveLister {
let decoded_response: GraphApiOneDriveListResponse =
serde_json::from_reader(bytes.reader()).map_err(new_json_deserialize_error)?;
- let list_with_versions = self.capability.list_with_versions;
+ let list_with_versions = self.op.versions();
// Include the current directory itself when handling the first page of the listing.
if ctx.token.is_empty() && !ctx.done {
@@ -100,8 +103,10 @@ impl oio::PageList for OneDriveLister {
// skip `list_with_versions` intentionally because a folder doesn't have versions
- let entry = oio::Entry::new(&path, meta);
- ctx.entries.push_back(entry);
+ if self.is_after_start(&path) {
+ let entry = oio::Entry::new(&path, meta);
+ ctx.entries.push_back(entry);
+ }
}
if let Some(next_link) = decoded_response.next_link {
@@ -129,28 +134,50 @@ impl oio::PageList for OneDriveLister {
normalized_path.push('/');
}
+ if !self.is_after_start(&normalized_path) {
+ continue;
+ }
+
let mut meta = Metadata::new(entry_mode)
.with_etag(drive_item.e_tag)
.with_content_length(drive_item.size.max(0) as u64);
let last_modified = drive_item.last_modified_date_time.parse::<Timestamp>()?;
meta.set_last_modified(last_modified);
- // When listing a directory with `$expand=versions`, OneDrive returns 400 "Operation not supported".
- // Thus, `list_with_versions` induces N+1 requests. This N+1 is intentional.
- // N+1 is horrendous but we can't do any better without OneDrive's API support.
- // When OneDrive supports listing with versions API, remove this.
- if list_with_versions {
+ // OneDrive doesn't support expanding versions while listing a directory.
+ // Query each file separately when callers request versions.
+ if list_with_versions && entry_mode == EntryMode::FILE {
let versions = self
.core
.onedrive_list_versions(&self.ctx, &normalized_path)
.await?;
- if let Some(version) = versions.first() {
- meta.set_version(&version.id);
+
+ if versions.is_empty() {
+ ctx.entries
+ .push_back(oio::Entry::new(&normalized_path, meta));
+ continue;
}
- }
- let entry = oio::Entry::new(&normalized_path, meta);
- ctx.entries.push_back(entry)
+ for (index, version) in versions.into_iter().enumerate() {
+ let mut version_meta =
+ Metadata::new(entry_mode).with_content_length(version.size.max(0) as u64);
+ version_meta
+ .set_last_modified(version.last_modified_date_time.parse::<Timestamp>()?);
+ version_meta.set_version(&version.id);
+ version_meta.set_is_current(index == 0);
+
+ // OneDrive exposes the ETag only for the current item.
+ if index == 0 {
+ version_meta.set_etag(meta.etag().unwrap_or_default());
+ }
+
+ ctx.entries
+ .push_back(oio::Entry::new(&normalized_path, version_meta));
+ }
+ } else {
+ ctx.entries
+ .push_back(oio::Entry::new(&normalized_path, meta));
+ }
}
Ok(())
diff --git a/core/services/onedrive/src/reader.rs b/core/services/onedrive/src/reader.rs
index 0413b63ec..916bee447 100644
--- a/core/services/onedrive/src/reader.rs
+++ b/core/services/onedrive/src/reader.rs
@@ -51,6 +51,17 @@ impl oio::StreamRead for OnedriveReader {
let backend = &self.backend;
let path = self.path.as_str();
let args = self.args.clone();
+ let range = match range {
+ BytesRange::Suffix { size } => {
+ let content_length = backend
+ .core
+ .onedrive_stat(&self.ctx, path, OpStat::default())
+ .await?
+ .content_length();
+ BytesRange::new(content_length.saturating_sub(size), None)
+ }
+ range => range,
+ };
let response = backend
.core
.onedrive_get_content(&self.ctx, path, range, &args)
diff --git a/website/data/services.json b/website/data/services.json
index d08c7e4b0..3fe7d9993 100644
--- a/website/data/services.json
+++ b/website/data/services.json
@@ -3188,9 +3188,9 @@
"group": "General",
"deprecated": {
"since": "0.57.0",
- "note": "OneDrive versioning capability is enabled by default and this option is no longer needed."
+ "note": "OneDrive supports version listing without this option."
},
- "comments": "Deprecated: OneDrive versioning capability is enabled by default."
+ "comments": "Deprecated: OneDrive supports version listing without this option."
}
],
"examples": [