diff --git a/.github/services/github/read_only/action.yml b/.github/services/github/read_only/action.yml new file mode 100644 index 000000000000..20937bce916c --- /dev/null +++ b/.github/services/github/read_only/action.yml @@ -0,0 +1,42 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +name: read_only +description: 'Behavior test for github service in read-only mode' + +runs: + using: "composite" + steps: + - name: Setup + shell: bash + run: | + # Run the read-only behavior tests against the fixtures in + # `core/tests/data` of the apache/opendal repository itself, so no + # extra backend or write permission is required. + cat << EOF >> $GITHUB_ENV + OPENDAL_GITHUB_OWNER=apache + OPENDAL_GITHUB_REPO=opendal + OPENDAL_GITHUB_ROOT=core/tests/data + OPENDAL_DISABLE_RANDOM_ROOT=true + OPENDAL_TEST_CAPABILITY_OVERRIDES=write=false,delete=false,create_dir=false + EOF + + # GITHUB_TOKEN is injected by the caller workflow; forward it to raise + # the GitHub API rate limit for the read-only behavior tests. + if [ -n "${GITHUB_TOKEN:-}" ]; then + echo "OPENDAL_GITHUB_TOKEN=$GITHUB_TOKEN" >> $GITHUB_ENV + fi diff --git a/.github/workflows/test_behavior_binding_python.yml b/.github/workflows/test_behavior_binding_python.yml index 7e2caaf1cd77..0cd3493fe526 100644 --- a/.github/workflows/test_behavior_binding_python.yml +++ b/.github/workflows/test_behavior_binding_python.yml @@ -98,3 +98,7 @@ jobs: setup: ${{ matrix.cases.setup }} service: ${{ matrix.cases.service }} feature: ${{ matrix.cases.feature }} + env: + # Forward the workflow token so read-only service tests (e.g. the + # github service) can raise their API rate limit. + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} diff --git a/.github/workflows/test_behavior_core.yml b/.github/workflows/test_behavior_core.yml index 1dd1666d4ffd..31c2f995e80a 100644 --- a/.github/workflows/test_behavior_core.yml +++ b/.github/workflows/test_behavior_core.yml @@ -58,3 +58,7 @@ jobs: setup: ${{ matrix.cases.setup }} service: ${{ matrix.cases.service }} feature: ${{ matrix.cases.feature }} + env: + # Forward the workflow token so read-only service tests (e.g. the + # github service) can raise their API rate limit. + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} diff --git a/core/services/github/src/config.rs b/core/services/github/src/config.rs index 6ab87deedfe0..7471731305ba 100644 --- a/core/services/github/src/config.rs +++ b/core/services/github/src/config.rs @@ -63,36 +63,46 @@ impl Configurator for GithubConfig { type Builder = GithubBuilder; fn from_uri(uri: &OperatorUri) -> Result { - let owner = uri.name().ok_or_else(|| { - Error::new(ErrorKind::ConfigInvalid, "uri host must contain owner") - .with_context("service", GITHUB_SCHEME) - })?; - - let raw_path = uri.root().ok_or_else(|| { - Error::new(ErrorKind::ConfigInvalid, "uri path must contain repository") - .with_context("service", GITHUB_SCHEME) - })?; - - let (repo, remainder) = match raw_path.split_once('/') { - Some((repo, rest)) => (repo, Some(rest)), - None => (raw_path, None), - }; - - if repo.is_empty() { - return Err( - Error::new(ErrorKind::ConfigInvalid, "repository name is required") - .with_context("service", GITHUB_SCHEME), - ); + let mut map = uri.options().clone(); + + // `github:////` URIs provide owner, repo and root + // through the URI itself. A bare scheme like `github` (used by + // `Operator::via_iter`) must fall back to the options so that + // `OPENDAL_GITHUB_OWNER` / `OPENDAL_GITHUB_REPO` environment variables + // work like they do for other services. + if let Some(owner) = uri.name() { + map.insert("owner".to_string(), owner.to_string()); } - let mut map = uri.options().clone(); - map.insert("owner".to_string(), owner.to_string()); - map.insert("repo".to_string(), repo.to_string()); + if let Some(raw_path) = uri.root() { + let (repo, remainder) = match raw_path.split_once('/') { + Some((repo, rest)) => (repo, Some(rest)), + None => (raw_path, None), + }; + + if !repo.is_empty() { + map.insert("repo".to_string(), repo.to_string()); + } + + if let Some(rest) = remainder + && !rest.is_empty() + { + map.insert("root".to_string(), rest.to_string()); + } + } - if let Some(rest) = remainder - && !rest.is_empty() - { - map.insert("root".to_string(), rest.to_string()); + // Owner and repository must be provided either via the URI or the + // options; `#[serde(default)]` would otherwise silently turn a missing + // field into an empty string. + if map.get("owner").is_none_or(String::is_empty) { + return Err(Error::new(ErrorKind::ConfigInvalid, "owner is required") + .with_context("service", GITHUB_SCHEME)); + } + if map.get("repo").is_none_or(String::is_empty) { + return Err( + Error::new(ErrorKind::ConfigInvalid, "repository is required") + .with_context("service", GITHUB_SCHEME), + ); } Self::from_iter(map) @@ -129,4 +139,22 @@ mod tests { assert!(GithubConfig::from_uri(&uri).is_err()); } + + #[test] + fn from_uri_sets_owner_repo_and_root_from_options() { + let uri = OperatorUri::new( + "github", + vec![ + ("owner".to_string(), "apache".to_string()), + ("repo".to_string(), "opendal".to_string()), + ("root".to_string(), "core/tests/data".to_string()), + ], + ) + .unwrap(); + + let cfg = GithubConfig::from_uri(&uri).unwrap(); + assert_eq!(cfg.owner, "apache".to_string()); + assert_eq!(cfg.repo, "opendal".to_string()); + assert_eq!(cfg.root.as_deref(), Some("core/tests/data")); + } } diff --git a/core/services/github/src/reader.rs b/core/services/github/src/reader.rs index 0050553bbdc5..4791c88927dd 100644 --- a/core/services/github/src/reader.rs +++ b/core/services/github/src/reader.rs @@ -53,10 +53,29 @@ impl oio::StreamRead for GithubReader { let status = resp.status(); let (rp, stream) = match status { - StatusCode::OK | StatusCode::PARTIAL_CONTENT => ( - RpRead::new(parse_into_metadata(path, resp.headers())?), - resp.into_body(), - ), + StatusCode::OK | StatusCode::PARTIAL_CONTENT => { + let (part, mut body) = resp.into_parts(); + let meta = parse_into_metadata(path, &part.headers)?; + + // GitHub ignores the Range header on authenticated requests + // and returns the full content with 200, so slice the body + // client-side when the server did not honor the range. + if status == StatusCode::PARTIAL_CONTENT || range.is_full() { + ( + RpRead::new(meta), + Box::new(body) as Box, + ) + } else { + let bs = body.to_buffer().await?; + let total_size = bs.len() as u64; + let sliced = bs.slice(range.to_content_range(bs.len())?); + let meta = Metadata::new(EntryMode::FILE).with_content_length(total_size); + ( + RpRead::new(meta), + Box::new(sliced) as Box, + ) + } + } _ => { let (part, mut body) = resp.into_parts(); let buf = body.to_buffer().await?; @@ -64,6 +83,6 @@ impl oio::StreamRead for GithubReader { } }; - Ok((rp, Box::new(stream) as Box)) + Ok((rp, stream)) } }