diff --git a/core/services/github/src/backend.rs b/core/services/github/src/backend.rs index 3a2f8e7e1f44..f7a265b5222e 100644 --- a/core/services/github/src/backend.rs +++ b/core/services/github/src/backend.rs @@ -132,6 +132,7 @@ impl Builder for GithubBuilder { write_can_empty: true, delete: true, + delete_with_if_match: true, list: true, list_with_recursive: true, diff --git a/core/services/github/src/core.rs b/core/services/github/src/core.rs index 0c68286d298a..6c3424c59fe1 100644 --- a/core/services/github/src/core.rs +++ b/core/services/github/src/core.rs @@ -211,7 +211,7 @@ impl GithubCore { self.send(ctx, req).await } - pub async fn delete(&self, ctx: &OperationContext, path: &str) -> Result<()> { + pub async fn delete(&self, ctx: &OperationContext, path: &str, args: &OpDelete) -> Result<()> { // If path is a directory, we should delete path/.gitkeep let formatted_path = format!("{path}.gitkeep"); let p = if path.ends_with('/') { @@ -220,8 +220,12 @@ impl GithubCore { path }; - let Some(sha) = self.get_file_sha(ctx, p).await? else { - return Ok(()); + let sha = match args.if_match() { + Some(sha) => sha.to_string(), + None => match self.get_file_sha(ctx, p).await? { + Some(sha) => sha, + None => return Ok(()), + }, }; let path = build_abs_path(&self.root, p); @@ -257,7 +261,15 @@ impl GithubCore { match resp.status() { StatusCode::OK => Ok(()), + StatusCode::NOT_FOUND if args.if_match().is_some() => Err(Error::new( + ErrorKind::ConditionNotMatch, + "delete precondition requires a live target", + )), StatusCode::NOT_FOUND => Ok(()), + StatusCode::CONFLICT if args.if_match().is_some() => Err(Error::new( + ErrorKind::ConditionNotMatch, + "delete sha does not match the current blob", + )), _ => Err(parse_error(resp)), } } @@ -412,7 +424,7 @@ mod error { let (kind, retryable) = match parts.status.as_u16() { 401 | 403 => (ErrorKind::PermissionDenied, false), 404 => (ErrorKind::NotFound, false), - 304 | 412 => (ErrorKind::ConditionNotMatch, false), + 304 | 409 | 412 => (ErrorKind::ConditionNotMatch, false), // https://github.com/apache/opendal/issues/4146 // https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/423 // We should retry it when we get 423 error. @@ -450,14 +462,24 @@ mod error { #[tokio::test] async fn test_parse_error() { - let err_res = vec![( - r#"{ + let err_res = vec![ + ( + r#"{ "message": "Not Found", "documentation_url": "https://docs.github.com/rest/repos/contents#get-repository-content" }"#, - ErrorKind::NotFound, - StatusCode::NOT_FOUND, - )]; + ErrorKind::NotFound, + StatusCode::NOT_FOUND, + ), + ( + r#"{ + "message": "is at abc123 but expected def456", + "documentation_url": "https://docs.github.com/rest/repos/contents#delete-a-file" + }"#, + ErrorKind::ConditionNotMatch, + StatusCode::CONFLICT, + ), + ]; for res in err_res { let bs = bytes::Bytes::from(res.0); diff --git a/core/services/github/src/deleter.rs b/core/services/github/src/deleter.rs index 0c7b1383454f..3c0d3d2e1035 100644 --- a/core/services/github/src/deleter.rs +++ b/core/services/github/src/deleter.rs @@ -33,10 +33,7 @@ impl GithubDeleter { } impl oio::OneShotDelete for GithubDeleter { - async fn delete_once(&self, path: String, _: OpDelete) -> Result<()> { - match self.core.delete(&self.ctx, &path).await { - Ok(_) => Ok(()), - Err(err) => Err(err), - } + async fn delete_once(&self, path: String, args: OpDelete) -> Result<()> { + self.core.delete(&self.ctx, &path, &args).await } } diff --git a/core/services/github/src/docs.md b/core/services/github/src/docs.md index 7858be0a0780..9261d386d354 100644 --- a/core/services/github/src/docs.md +++ b/core/services/github/src/docs.md @@ -15,6 +15,11 @@ Depending on its configuration and the backing system, this service can expose: Inspect the effective capability set with [`opendal_core::Operator::info`] and [`opendal_core::OperatorInfo::capability`] after building an operator. +Conditional delete with `if_match` sends the blob SHA from +`Metadata::etag` as the Contents API `sha`. A mismatched SHA returns +`ErrorKind::ConditionNotMatch` and leaves the file in place. Unconditional +delete still looks up the current SHA first. + ## Configuration Use [`crate::GithubConfig`] for serializable configuration and this builder's