fix(services/github): support env-driven construction, honor range reads, add read-only behavior tests

.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

.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 }}

.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 }}

core/services/github/src/config.rs

@@ -63,36 +63,46 @@ impl Configurator for GithubConfig {
     type Builder = GithubBuilder;
 
     fn from_uri(uri: &OperatorUri) -> Result<Self> {
-        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://<owner>/<repo>/<root>` 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"));
+    }
 }

core/services/github/src/reader.rs

@@ -53,17 +53,36 @@ 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<dyn oio::ReadStreamDyn>,
+                    )
+                } 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<dyn oio::ReadStreamDyn>,
+                    )
+                }
+            }
             _ => {
                 let (part, mut body) = resp.into_parts();
                 let buf = body.to_buffer().await?;
                 return Err(parse_error(Response::from_parts(part, buf)));
             }
         };
 
-        Ok((rp, Box::new(stream) as Box<dyn oio::ReadStreamDyn>))
+        Ok((rp, stream))
     }
 }