fix: distinguish 404 in GetAllFilesInPath, make uploads idempotent #33

Merged
rodin merged 3 commits from fix/8-10-error-handling-idempotent-upload into main 2026-05-02 17:07:23 +00:00
3 changed files with 144 additions and 5 deletions
+16 -2
View File
@@ -69,14 +69,28 @@ jobs:
echo "Release ID: ${RELEASE_ID}"
# Upload each asset
# Upload each asset (idempotent: delete existing asset with same name first)
for file in dist/*; do
filename=$(basename "$file")
echo "Uploading ${filename}..."
# Check if asset already exists and delete it
Review

[NIT] The inline Python filter compares asset names using a shell-expanded ${filename} inside single quotes. If a filename ever contained a single quote, this would break the command. While current artifact names are safe, consider more robust quoting or using a tool like jq.

**[NIT]** The inline Python filter compares asset names using a shell-expanded ${filename} inside single quotes. If a filename ever contained a single quote, this would break the command. While current artifact names are safe, consider more robust quoting or using a tool like jq.
EXISTING_ID=$(export ASSET_NAME="${filename}"; curl -sS \
Review

[MINOR] The asset name is interpolated directly into the URL query string without URL-encoding (…/assets?name=${filename}). While current filenames are controlled and simple, not URL-encoding can cause unexpected behavior or edge-case injection via special characters. Consider URL-encoding the filename when building the query parameter.

**[MINOR]** The asset name is interpolated directly into the URL query string without URL-encoding (…/assets?name=${filename}). While current filenames are controlled and simple, not URL-encoding can cause unexpected behavior or edge-case injection via special characters. Consider URL-encoding the filename when building the query parameter.
-H "Authorization: token ${GITEA_TOKEN}" \
"${GITEA_URL}/api/v1/repos/${REPO}/releases/${RELEASE_ID}/assets" \
| python3 -c "import json,sys,os; name=os.environ['ASSET_NAME']; assets=json.load(sys.stdin); print(next((str(a['id']) for a in assets if a['name']==name),''))" 2>/dev/null)
if [ -n "$EXISTING_ID" ]; then
echo " Asset ${filename} already exists (id=${EXISTING_ID}), deleting..."
curl -sSf -X DELETE \
-H "Authorization: token ${GITEA_TOKEN}" \
"${GITEA_URL}/api/v1/repos/${REPO}/releases/${RELEASE_ID}/assets/${EXISTING_ID}"
fi
curl -sSf -X POST \
-H "Authorization: token ${GITEA_TOKEN}" \
-H "Content-Type: application/octet-stream" \
"${GITEA_URL}/api/v1/repos/${REPO}/releases/${RELEASE_ID}/assets?name=${filename}" \
"${GITEA_URL}/api/v1/repos/${REPO}/releases/${RELEASE_ID}/assets?name=$(printf '%s' "${filename}" | jq -sRr @uri)" \
--data-binary "@${file}"
done
+31 -3
View File
@@ -7,6 +7,7 @@ import (
"bytes"
"context"
"encoding/json"
"errors"
"fmt"
"io"
"log"
@@ -16,6 +17,28 @@ import (
"time"
)
// APIError represents an HTTP error response from the Gitea API.
Review

[NIT] APIError.Error includes the entire response body in error strings. If upstream returns verbose bodies, this could leak detailed server messages into logs. Not a direct vulnerability here, but consider truncating or sanitizing bodies in errors if upstream may include sensitive details.

**[NIT]** APIError.Error includes the entire response body in error strings. If upstream returns verbose bodies, this could leak detailed server messages into logs. Not a direct vulnerability here, but consider truncating or sanitizing bodies in errors if upstream may include sensitive details.
// It carries the status code so callers can distinguish between
// different failure modes (e.g. 404 vs 500).
type APIError struct {
StatusCode int
Body string
}
func (e *APIError) Error() string {
body := e.Body
if len(body) > 200 {
body = body[:200] + "...(truncated)"
}
return fmt.Sprintf("HTTP %d: %s", e.StatusCode, body)
}
// IsNotFound reports whether an error is an API 404 response.
func IsNotFound(err error) bool {
var apiErr *APIError
return errors.As(err, &apiErr) && apiErr.StatusCode == http.StatusNotFound
}
// Client interacts with the Gitea API.
// A Client is safe for concurrent use by multiple goroutines.
type Client struct {
@@ -201,7 +224,7 @@ func (c *Client) doGet(ctx context.Context, reqURL string) ([]byte, error) {
if resp.StatusCode < 200 || resp.StatusCode >= 300 {
body, _ := io.ReadAll(resp.Body)
return nil, fmt.Errorf("HTTP %d: %s", resp.StatusCode, string(body))
return nil, &APIError{StatusCode: resp.StatusCode, Body: string(body)}
}
return io.ReadAll(resp.Body)
}
@@ -254,10 +277,15 @@ func (c *Client) GetAllFilesInPath(ctx context.Context, owner, repo, path string
// Try listing as directory first
entries, err := c.ListContents(ctx, owner, repo, path)
if err != nil {
// Might be a file, try fetching directly
// Only fall back to single-file fetch on 404 (path is a file, not a dir).
// Propagate all other errors (auth failures, server errors, rate limits).
if !IsNotFound(err) {
return nil, fmt.Errorf("list contents %q: %w", path, err)
}
Outdated
Review

[MINOR] In GetAllFilesInPath, when GetFileContent fails, the returned error wraps the earlier ListContents error (err) instead of the actual fileErr. This loses the specific cause of the file fetch failure. Prefer wrapping fileErr for better diagnostics.

**[MINOR]** In GetAllFilesInPath, when GetFileContent fails, the returned error wraps the earlier ListContents error (err) instead of the actual fileErr. This loses the specific cause of the file fetch failure. Prefer wrapping fileErr for better diagnostics.
// 404 means the path might be a file — try fetching directly
content, fileErr := c.GetFileContent(ctx, owner, repo, path)
if fileErr != nil {
return nil, fmt.Errorf("path %q is neither a file nor directory: %w", path, err)
return nil, fmt.Errorf("path %q is neither a file nor directory: %w", path, fileErr)
}
results[path] = content
return results, nil
+97
View File
@@ -3,6 +3,7 @@ package gitea
import (
"context"
"encoding/json"
"errors"
"fmt"
"net/http"
"net/http/httptest"
@@ -505,3 +506,99 @@ func TestGetTimelineReviewCommentID_NotFound(t *testing.T) {
t.Fatal("expected error when sentinel not found")
}
}
func TestGetAllFilesInPath_404FallsBackToFile(t *testing.T) {
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
switch r.URL.Path {
case "/api/v1/repos/owner/repo/contents/README.md":
// Contents API returns 404 for files (not a directory)
w.WriteHeader(http.StatusNotFound)
w.Write([]byte(`{"message":"not found"}`))
case "/api/v1/repos/owner/repo/raw/README.md":
w.Write([]byte("# Hello\n"))
default:
w.WriteHeader(http.StatusNotFound)
w.Write([]byte(`{"message":"not found"}`))
}
}))
defer server.Close()
client := NewClient(server.URL, "test-token")
files, err := client.GetAllFilesInPath(context.Background(), "owner", "repo", "README.md")
if err != nil {
t.Fatalf("expected fallback to file on 404, got error: %v", err)
}
if len(files) != 1 {
t.Fatalf("expected 1 file, got %d", len(files))
}
if files["README.md"] != "# Hello\n" {
t.Errorf("unexpected content: %q", files["README.md"])
}
}
func TestGetAllFilesInPath_500Propagates(t *testing.T) {
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
// Simulate a server error from ListContents
w.WriteHeader(http.StatusInternalServerError)
w.Write([]byte(`{"message":"internal server error"}`))
}))
defer server.Close()
client := NewClient(server.URL, "test-token")
_, err := client.GetAllFilesInPath(context.Background(), "owner", "repo", "somepath")
if err == nil {
t.Fatal("expected error to propagate for 500, got nil")
}
// Should NOT fall back to file fetch — error should propagate
var apiErr *APIError
if !errors.As(err, &apiErr) {
t.Fatalf("expected APIError in chain, got: %v", err)
}
if apiErr.StatusCode != http.StatusInternalServerError {
t.Errorf("expected status 500, got %d", apiErr.StatusCode)
}
}
func TestGetAllFilesInPath_403Propagates(t *testing.T) {
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(http.StatusForbidden)
w.Write([]byte(`{"message":"token has insufficient scope"}`))
}))
defer server.Close()
client := NewClient(server.URL, "test-token")
_, err := client.GetAllFilesInPath(context.Background(), "owner", "repo", "private/stuff")
if err == nil {
t.Fatal("expected error to propagate for 403, got nil")
}
var apiErr *APIError
if !errors.As(err, &apiErr) {
t.Fatalf("expected APIError in chain, got: %v", err)
}
if apiErr.StatusCode != http.StatusForbidden {
t.Errorf("expected status 403, got %d", apiErr.StatusCode)
}
}
func TestIsNotFound(t *testing.T) {
tests := []struct {
name string
err error
want bool
}{
{"nil error", nil, false},
{"non-API error", fmt.Errorf("network timeout"), false},
{"404 APIError", &APIError{StatusCode: 404, Body: "not found"}, true},
{"500 APIError", &APIError{StatusCode: 500, Body: "server error"}, false},
{"wrapped 404", fmt.Errorf("list contents: %w", &APIError{StatusCode: 404, Body: "not found"}), true},
{"wrapped 500", fmt.Errorf("list contents: %w", &APIError{StatusCode: 500, Body: "err"}), false},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
got := IsNotFound(tt.err)
if got != tt.want {
t.Errorf("IsNotFound(%v) = %v, want %v", tt.err, got, tt.want)
}
})
}
}