feat(apiclient): Client interface + V1Client wrapper#679
Open
jhagberg wants to merge 7 commits into
Open
Conversation
This was referenced Apr 23, 2026
jhagberg
added a commit
that referenced
this pull request
Apr 23, 2026
After the apiclient abstraction in #679 landed, list.datasetFiles and list.Datasets returned raw V1Client errors on HTTP / transport failures, dropping the 'failed to get files/datasets, reason: ...' prefix that callers saw before. This restores the legacy prefix for both commands while leaving the bare 'invalid base URL' error untouched (TestListDatasetNoUrl and TestListDatasetsNoUrl assert on that string directly). Caught by code review on #675. download already had the wrap in getFileIDURL but list lost it during the abstraction refactor.
jhagberg
force-pushed
the
feat/v2-client-abstraction
branch
from
368286e to
b711dd3
Compare
jhagberg
added a commit
that referenced
this pull request
Apr 24, 2026
After the apiclient abstraction in #679 landed, list.datasetFiles and list.Datasets returned raw V1Client errors on HTTP / transport failures, dropping the 'failed to get files/datasets, reason: ...' prefix that callers saw before. This restores the legacy prefix for both commands while leaving the bare 'invalid base URL' error untouched (TestListDatasetNoUrl and TestListDatasetsNoUrl assert on that string directly). Caught by code review on #675. download already had the wrap in getFileIDURL but list lost it during the abstraction refactor.
jhagberg
force-pushed
the
feat/v2-client-abstraction
branch
from
b711dd3 to
60a9c6b
Compare
jhagberg
added a commit
that referenced
this pull request
Apr 24, 2026
After the apiclient abstraction in #679 landed, list.datasetFiles and list.Datasets returned raw V1Client errors on HTTP / transport failures, dropping the 'failed to get files/datasets, reason: ...' prefix that callers saw before. This restores the legacy prefix for both commands while leaving the bare 'invalid base URL' error untouched (TestListDatasetNoUrl and TestListDatasetsNoUrl assert on that string directly). Caught by code review on #675. download already had the wrap in getFileIDURL but list lost it during the abstraction refactor.
jhagberg
force-pushed
the
feat/v2-client-abstraction
branch
from
60a9c6b to
0ead8d5
Compare
jhagberg
added a commit
that referenced
this pull request
Apr 24, 2026
After the apiclient abstraction in #679 landed, list.datasetFiles and list.Datasets returned raw V1Client errors on HTTP / transport failures, dropping the 'failed to get files/datasets, reason: ...' prefix that callers saw before. This restores the legacy prefix for both commands while leaving the bare 'invalid base URL' error untouched (TestListDatasetNoUrl and TestListDatasetsNoUrl assert on that string directly). Caught by code review on #675. download already had the wrap in getFileIDURL but list lost it during the abstraction refactor.
This was referenced Apr 24, 2026
jhagberg
added a commit
that referenced
this pull request
Apr 24, 2026
datasetCase/recursiveCase/fileCase now call a shared downloadOne helper that goes through client.DownloadFile, followed by the new writeBodyToDisk helper (progress-bar + .part atomic rename) extracted from the old downloadFile. Removes the #679 backward-compat wrappers (download.GetDatasets, download.GetFilesInfo, download.File alias) and the legacy helpers that moved to apiclient in #679 (setupCookieJar, cookieJar, cookiePath, getBody) plus getFileIDURL and downloadFile — UserArg (path or fileId) is now resolved inside V1Client.DownloadFile / V2Client.DownloadFile. recursiveCase uses v2's pathPrefix filter server-side when --api-version=v2; v1 still filters client-side since it has no server-side filter. Adds a test for the v2-missing-pubkey guard and replaces the old downloadFile-cleanup test with a writeBodyToDisk-cleanup test driven by an errReader that fails mid-stream. Tests for deleted helpers (TestGetDatasets, TestGetFilesInfo, TestFileIdUrl, TestGetBody*, TestSetupCookiejar) are removed — their behavior is covered by apiclient/v1_test.go (TestV1Client_ListDatasets_*, TestV1Client_DownloadFile_*).
nanjiangshu
reviewed
May 11, 2026
nanjiangshu
reviewed
May 11, 2026
nanjiangshu
reviewed
May 11, 2026
Contributor
nanjiangshu
left a comment
nanjiangshu
left a comment
There was a problem hiding this comment.
Works great when I tested. I just left a minor comment regarding potential naming confusion for version. Otherwise, it looks good.
kostas-kou
approved these changes
May 12, 2026
Contributor
kostas-kou
left a comment
kostas-kou
left a comment
There was a problem hiding this comment.
Tested it and is working. Good job
nanjiangshu
reviewed
May 12, 2026
New sibling package apiclient/ with: - Client interface (ListDatasets, ListFiles, DatasetInfo) - Shared types (File, Checksum, DatasetInfo, ListFilesOptions) - ErrNotSupportedOnV1 for v2-only operations hit on a V1Client - V1Client with v1 HTTP logic ported from download/download.go (setupCookieJar + getBody — same headers, same cookie path, same persistent jar configuration) - New() factory returning V1Client for 'v1' and an error for 'v2' (#675 wires up V2Client) DownloadFile is intentionally NOT in this interface yet. It joins in #677 alongside v2 download implementation, to avoid coupling this PR to the progress-bar write-to-disk refactor. Unit tests cover V1Client methods via httptest.Server, factory dispatch, and the v2-option-rejection path.
Thin wrappers preserve exact outward behavior including the 'failed to get files, reason:' error prefix asserted by TestInvalidUrl. File is now a type alias of apiclient.File. Wrappers and alias are removed in #677 once callers migrate to apiclient.Client directly.
- datasetCase / recursiveCase / fileCase accept (ctx, client, ...) - getFileIDURL accepts (ctx, client, ...) and calls client.ListFiles instead of GetFilesInfo - Download() constructs the client via apiclient.New using --api-version (default v1) - Added V1Client.SetHTTPClientForTest helper for hermetic unit tests DownloadFile itself still uses the legacy download/downloadFile HTTP code. That path moves onto the client in #677.
Exercises the apiclient.New factory by setting all other required inputs (datasetID, URL, pubKey) and observing the factory's error for v2 before the command attempts any HTTP.
Datasets and datasetFiles now construct an apiclient.Client via apiclient.New(cfg, apiVersionFlag) and call its methods. Same output behavior; v1 routes through V1Client which delegates to the same /metadata/datasets endpoints as before. --api-version v2 returns 'not yet implemented' until #675.
Addresses Task 5 code-quality follow-ups: - Add a NOTE explaining why Datasets does per-dataset ListFiles on v1 (no DatasetInfo endpoint; #676 replaces on v2). - Switch the new v2-error test to require.Error for consistency with the parallel test in download_test.go.
Three changes from review comments (nanjiangshu): - Rename Config.Version to Config.ClientVersion to disambiguate from the --api-version flag. Doc comment updated with an example. - Rename the version parameter on New() and ValidateVersion() to apiVersion, matching the --api-version flag name. - Widen File.DecryptedFileSize from int to int64 to match sda-download's wire type and avoid truncation above 2 GiB on 32-bit clients. list.go (datasetSize, formatFileSizeOutput) and the v1_test.go assertion follow.
jhagberg
force-pushed
the
feat/v2-client-abstraction
branch
from
0ead8d5 to
dfcea63
Compare
jhagberg
added a commit
that referenced
this pull request
May 13, 2026
After the apiclient abstraction in #679 landed, list.datasetFiles and list.Datasets returned raw V1Client errors on HTTP / transport failures, dropping the 'failed to get files/datasets, reason: ...' prefix that callers saw before. This restores the legacy prefix for both commands while leaving the bare 'invalid base URL' error untouched (TestListDatasetNoUrl and TestListDatasetsNoUrl assert on that string directly). Caught by code review on #675. download already had the wrap in getFileIDURL but list lost it during the abstraction refactor.
jhagberg
added a commit
that referenced
this pull request
May 13, 2026
Three changes from review comments (nanjiangshu): - v2.ListDatasets uses url.JoinPath(BaseURL, "datasets") instead of string concatenation. Handles trailing slashes correctly and surfaces a clear error on a malformed BaseURL. - v2 getJSON sends SDA-Client-Version alongside the existing User-Agent header so log pipelines keyed on the v1 header still see the version on v2 traffic. - v2_types.go toFile() drops the int(f.DecryptedSize) cast now that apiclient.File.DecryptedFileSize is int64 (from the #679 amend).
jhagberg
added a commit
that referenced
this pull request
May 13, 2026
Three follow-ups after rebasing on the amended #679 and #681: - v2_test.go: int64 assertion on the widened File.DecryptedFileSize - v2.go: blank line before paginate return (lint nlreturn) - list.go: drop now-redundant int(info.Size) cast on DatasetInfo.Size (both formatFileSizeOutput and DatasetInfo.Size are int64)
7 tasks
jhagberg
added a commit
that referenced
this pull request
May 13, 2026
datasetCase/recursiveCase/fileCase now call a shared downloadOne helper that goes through client.DownloadFile, followed by the new writeBodyToDisk helper (progress-bar + .part atomic rename) extracted from the old downloadFile. Removes the #679 backward-compat wrappers (download.GetDatasets, download.GetFilesInfo, download.File alias) and the legacy helpers that moved to apiclient in #679 (setupCookieJar, cookieJar, cookiePath, getBody) plus getFileIDURL and downloadFile — UserArg (path or fileId) is now resolved inside V1Client.DownloadFile / V2Client.DownloadFile. recursiveCase uses v2's pathPrefix filter server-side when --api-version=v2; v1 still filters client-side since it has no server-side filter. Adds a test for the v2-missing-pubkey guard and replaces the old downloadFile-cleanup test with a writeBodyToDisk-cleanup test driven by an errReader that fails mid-stream. Tests for deleted helpers (TestGetDatasets, TestGetFilesInfo, TestFileIdUrl, TestGetBody*, TestSetupCookiejar) are removed — their behavior is covered by apiclient/v1_test.go (TestV1Client_ListDatasets_*, TestV1Client_DownloadFile_*).
jhagberg
added a commit
that referenced
this pull request
May 13, 2026
Renames the new V1Client.DownloadFile and V2Client.DownloadFile call sites from c.cfg.Version to c.cfg.ClientVersion, mirrors the v2 "SDA-Client-Version alongside User-Agent" header pair that #681 picked up, and updates the downloadOneWithClient test fixture.
KarlG-nbis
reviewed
May 13, 2026
| @@ -0,0 +1,75 @@ | |||
| package apiclient | |||
Contributor
There was a problem hiding this comment.
would it make more sense to name this package "downloadclient", etc to be more specific in which client this package is an interface for? Just to reduce risk of mix up with the "sda-api", etc?
KarlG-nbis
reviewed
May 13, 2026
|
|
||
| // New returns a Client for the requested apiVersion. | ||
| // Today accepts "v1" only; "v2" errors. Extended in #675 to return a V2Client. | ||
| func New(cfg Config, apiVersion string, opts ...Option) (Client, error) { |
Contributor
There was a problem hiding this comment.
Would we also want validation of the cfg Config values here, or are we just assuming that the caller has validated those already?
nanjiangshu
approved these changes
May 13, 2026
Contributor
nanjiangshu
left a comment
nanjiangshu
left a comment
There was a problem hiding this comment.
Looks good to me now! Well done!
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related issue(s) and PR(s)
Closes #674. First of six PRs for #663 (SDA download v2 support); #681, #685, #686, #687 stack on top.
Description
Introduces
apiclient.Clientas a version-agnostic interface for the download HTTP surface, withV1Clientwrapping the existing v1 code andV2Clientleft as a stub for #681+. Alllistanddownloadlist-family calls route through the client; file transfer stays on the legacy path until #686.A new
--api-version v1|v2flag (defaultv1) picks the implementation.v2errors with "not yet implemented" until #681 wires upV2Client.ListDatasets.V1 wire behavior is preserved: same headers (
SDA-Client-Version,Authorization,Content-Type, optionalClient-Public-Key), same cookie path (${UserCacheDir}/sda-cli/sda_cookie), same"failed to get files, reason:"error prefix asserted byTestInvalidUrl, same HTTP 412 handling.DownloadFilejoins the interface in #686 to avoid coupling this PR to the progress-bar refactor.Updated from review (nanjiangshu):
Config.Version→Config.ClientVersion, theversionparameter onNew/ValidateVersion→apiVersion, andFile.DecryptedFileSizewidened toint64(matches sda-download's wire type; avoids truncation above 2 GiB on 32-bit clients).How to test
go test ./...(169 pass),go vet ./...,golangci-lint run— all cleansda-cli list --datasetsandsda-cli download --dataset-id X ...work as before on--api-version v1--api-version v2errors early with "not yet implemented"--api-version v9errors withunsupported --api-version "v9" (v1 or v2)Test plan
go test ./...: 169 passgo vet ./...: cleangolangci-lint run: cleangofmt -l .: clean