diff --git a/.github/release-notes/v0.3.0.md b/.github/release-notes/v0.3.0.md new file mode 100644 index 0000000..1be348e --- /dev/null +++ b/.github/release-notes/v0.3.0.md @@ -0,0 +1,28 @@ +serpapi is now predictable for scripts and AI agents, and safe on a terminal. + +- **One error contract.** Errors and warnings are JSON lines on stderr with the HTTP status. Exit 2 means fix the command; it fails before a billed search, except when a `--jq` filter fails on the response itself; 1 is an API or network error; 130 means Ctrl-C or SIGTERM stopped it before it finished, after printing what had arrived. +- **Markdown and other formats.** `output=md`, `html`, `plain` and `csv`, in any case, work for `search` and `archive` (#10). +- **Timeouts.** `--timeout` takes seconds, such as `120`, or a duration, such as `2m`; `SERPAPI_TIMEOUT` sets it for a shell; `0` waits indefinitely. The default is 60 s (#15, @strzibny). +- **`--all-pages`.** `--fields` no longer stops paging after page 1 (#9, @adarshdigievo). `no_cache`, `zero_trace` and `output` apply to every page. Paging stops at a repeated page or at a page a mistyped `--fields` matches nothing on. Pages already fetched are printed, through `--jq` too, when a later page fails or on Ctrl-C. Output streams, so 100 pages of 1 MiB peak at 187–188 MiB instead of 720–735 MiB (3 runs each against v0.2.3 on macOS arm64, from a local fake API). +- **Warnings that name the fix.** A large result lists its top-level keys by size and the free `serpapi archive ID --jq` next step, or says when `no_cache` or `zero_trace` makes a repeat a new, counted search. A `--jq` that prints only null lists the keys the response has. A parameter given twice says which value was sent. +- **Safer.** Terminal output escapes control characters from results and from the account email that `login` prints, so a page title can't set the clipboard or clear the screen. `serpapi account` masks the API key (#7, @ilyazub), and the key is never echoed from a stray argument or a broken config file. `login` saves the key only under an absolute HOME, through a new private temp file, so a file left at the old temp name can't widen its permissions or receive the key. Ctrl-C or SIGTERM at its prompts stops it at once with echo on. +- **Also:** `serpapi version`; `login --api-key`; `login` asks before replacing a saved key (#7); `SERPAPI_API_KEY` as a fallback for `SERPAPI_KEY`; `--help` documents exit codes, errors and costs. +- **Patched Go.** Binaries are built with the latest patched Go at release time; `go version -m "$(command -v serpapi)"` shows which. v0.2.3's binaries were built with go1.25.0, in which `govulncheck -mode=binary` finds 57 (darwin/arm64) and 58 (linux/amd64) known vulnerabilities; a release rehearsal with go1.27.2 found none. + +## Changes scripts may notice + +- These now exit 2 before any request: `q=` with bare words, no parameters, the API key passed as `key`, `apikey`, `serp_api_key` or `serpapi_key` (`api_key` already did), `--fields` outside `search`, `--max-pages 0`, `--all-pages` with `async=true`, `--jq` or `--all-pages` with `output=md`, an unknown `output=`, and `locations` without a query. +- A `--jq` filter that can't run, such as a syntax error, `input` or `debug`, fails before the search; one that fails on the response, such as `tonumber` on a title, still exits 2 after it. `halt` now works. +- Bare words are joined into one `q`: `serpapi search engine=google coffee shops` sends `q=coffee shops`, not `q=shops`. +- `--fields` adds `error` to `json_restrictor`, so an API error still arrives. `--fields ,,` sends no `json_restrictor`; it used to send `,,` and print `{}`. +- A `SERPAPI_KEY` padded with spaces or a newline is trimmed before it's sent. +- `serpapi account` prints `api_key` masked, as `abcd…wxyz` (#7). +- `--all-pages`: when a later page fails, or `serpapi_pagination.next` isn't a valid URL, the pages so far are printed before the error, exit 1; that invalid URL is now `api_error`, not `network_error`. A 200 response with an error on a later page is a warning after the pages so far, exit 0, not exit 1. +- A JSON array body is exit 1 `api_error`, not exit 0. +- `serpapi` with no command prints nothing on stdout, where its usage used to go; stderr has the JSON error, and the usage too on a terminal. It still exits 2. +- `serpapi help TOPIC` with an unknown topic exits 2 with a JSON `usage_error` naming the closest command; it exited 0 with the usage on stderr. +- `serpapi login` exits 2 when HOME (`%AppData%` on Windows) isn't an absolute path, and no command reads a config file then; v0.2.3 used `.config/serpapi` under the current directory. +- At the hidden key prompt, Ctrl-D exits 2 with "No API key given" (v0.2.3 kept waiting), and SIGTERM exits 130 with echo on (v0.2.3: 143, echo off). +- The default timeout is 60 s, not 30 s. + +Thanks to @adarshdigievo (#4, #9), @jvmvik (#6), @strzibny (#15) and @ilyazub (#7). diff --git a/.github/scripts/release-tag-test.py b/.github/scripts/release-tag-test.py new file mode 100755 index 0000000..d475cef --- /dev/null +++ b/.github/scripts/release-tag-test.py @@ -0,0 +1,186 @@ +#!/usr/bin/env python3 +"""Tests release-tag.sh in scratch git repos, offline: a stub gh answers the +two API calls the script makes, a merged pull request's labels and creating a +tag ref (a local git tag, refused when it exists, as the API refuses with 422). +Prints one line per case and exits 1 if any fails. CI runs it before tagging. +Usage: release-tag-test.py [SCRIPT]""" +import os +import shutil +import stat +import subprocess +import sys +import tempfile + +STUB = r"""#!/usr/bin/env bash +echo "gh $*" >> "$STUB_LOG" +if [ "$1 $2" = "api repos/serpapi/serpapi-cli/git/refs" ]; then + ref=${4#ref=refs/tags/}; sha=${6#sha=} + if [ "$ref" = "${STUB_EXISTS:-}" ] || git rev-parse -q --verify "refs/tags/$ref" >/dev/null; then echo "Reference already exists (HTTP 422)" >&2; exit 1; fi + [ -n "${STUB_FAIL_CREATE:-}" ] && { echo "HTTP 500" >&2; exit 1; } + git tag "$ref" "$sha"; exit 0 +fi +case "$2" in repos/serpapi/serpapi-cli/commits/*/pulls) + [ -n "${STUB_FAIL_LABELS:-}" ] && { echo "HTTP 502" >&2; exit 1; } + sha=${2#repos/serpapi/serpapi-cli/commits/}; sha=${sha%/pulls} + if [ -f "${STUB_DIR:-/nonexistent}/labels-$sha" ]; then cat "$STUB_DIR/labels-$sha"; else printf '%s\n' "${STUB_LABELS:-}"; fi + exit 0 ;; +esac +echo "unexpected gh call: $*" >&2; exit 3 +""" + + +def git(repo, *args): + return subprocess.run(["git", "-C", repo, *args], check=True, capture_output=True, text=True).stdout.strip() + + +def commit(repo, path, text="x"): + full = os.path.join(repo, path) + os.makedirs(os.path.dirname(full) or repo, exist_ok=True) + with open(full, "a") as f: + f.write(text + "\n") + git(repo, "add", "-A") + git(repo, "commit", "-q", "-m", f"change {path}") + return git(repo, "rev-parse", "HEAD") + + +def fresh(root, tags=True): + repo = tempfile.mkdtemp(dir=root) + git(repo, "init", "-q", "-b", "main") + git(repo, "config", "user.email", "t@example.com") + git(repo, "config", "user.name", "t") + base = commit(repo, "pkg/cmd/root.go", "package cmd") + if tags: + git(repo, "tag", "v0.2.2", base) + base = commit(repo, "pkg/cmd/search.go", "package cmd") + git(repo, "tag", "v0.2.3", base) + return repo, base + + +def run(script, repo, root, **env): + out = os.path.join(root, "out") + open(out, "w").close() + log = os.path.join(root, "gh.log") + open(log, "w").close() + e = {"PATH": f"{root}/bin:{os.environ['PATH']}", "GITHUB_OUTPUT": out, "GITHUB_REPOSITORY": "serpapi/serpapi-cli", + "STUB_LOG": log, "HOME": root} + e.update({k: v for k, v in env.items() if v is not None}) + p = subprocess.run(["bash", script], cwd=repo, env=e, capture_output=True, text=True) + outputs = dict(line.split("=", 1) for line in open(out).read().splitlines() if "=" in line) + return p.returncode, p.stdout.strip(), p.stderr.strip(), outputs, open(log).read() + + +def main(): + script = os.path.abspath(sys.argv[1] if len(sys.argv) > 1 else os.path.join(os.path.dirname(__file__), "release-tag.sh")) + root = tempfile.mkdtemp(prefix="release-tag-test-") + os.makedirs(f"{root}/bin") + with open(f"{root}/bin/gh", "w") as f: + f.write(STUB) + os.chmod(f"{root}/bin/gh", stat.S_IRWXU) + failures = 0 + + def case(name, want_code, want_reason, want_tag, repo, sha, created=None, **env): + nonlocal failures + before = set(git(repo, "tag", "--list").split()) + code, stdout, stderr, outputs, log = run(script, repo, root, SHA=sha, **env) + new_tags = sorted(set(git(repo, "tag", "--list").split()) - before) + ok = (code == want_code and want_reason in (stdout + stderr) and outputs.get("tag", "") == want_tag + and new_tags == ([created] if created else [])) + failures += not ok + print(f"{'ok ' if ok else 'FAIL'} {name:52} exit={code} tag={outputs.get('tag', '')!r} next={outputs.get('next', '')!r} " + f"created={new_tags} :: {(stdout or stderr)[:95]}") + return log + + repo, _ = fresh(root) + code = commit(repo, "pkg/api/client.go", "package api") + case("code change, no labels: patch", 0, "Tagged", "v0.2.4", repo, code, created="v0.2.4", LABELS_JSON="[]") + repo, _ = fresh(root); code = commit(repo, "pkg/api/client.go") + case("release:minor label", 0, "minor after v0.2.3", "v0.3.0", repo, code, created="v0.3.0", LABELS_JSON='["release:minor"]') + repo, _ = fresh(root); code = commit(repo, "pkg/api/client.go") + case("release:major label", 0, "major after v0.2.3", "v1.0.0", repo, code, created="v1.0.0", LABELS_JSON='["release:major"]') + repo, _ = fresh(root); code = commit(repo, "pkg/api/client.go") + case("release:skip label", 0, "release:skip", "", repo, code, LABELS_JSON='["release:skip","release:minor"]') + repo, _ = fresh(root); code = commit(repo, "pkg/api/client.go") + case("multi-word label beside release:minor", 0, "minor", "v0.3.0", repo, code, created="v0.3.0", + LABELS_JSON='["good first issue","release:minor"]') + repo, _ = fresh(root); code = commit(repo, "pkg/api/client.go") + case("label that only contains release:minor", 0, "patch after", "v0.2.4", repo, code, created="v0.2.4", + LABELS_JSON='["not release:minor"]') + repo, _ = fresh(root) + for path in ["README.md", ".github/workflows/ci.yml", "pkg/cmd/x_test.go", "tests/e2e_test.go", "pkg/cmd/testdata/a.json", "AGENTS.md"]: + docs = commit(repo, path) + case("only docs, tests, testdata and CI changed", 0, "nothing that ships", "", repo, docs, LABELS_JSON='["release:minor"]') + case("explicit BUMP=minor releases them anyway", 0, "minor after v0.2.3", "v0.3.0", repo, docs, created="v0.3.0", BUMP="minor") + repo, _ = fresh(root); sha = commit(repo, "go.mod", "module x") + case("go.mod change ships", 0, "Tagged", "v0.2.4", repo, sha, created="v0.2.4", LABELS_JSON="[]") + repo, _ = fresh(root); sha = commit(repo, ".goreleaser.yml", "version: 2") + case(".goreleaser.yml change ships", 0, "Tagged", "v0.2.4", repo, sha, created="v0.2.4", LABELS_JSON="[]") + repo, _ = fresh(root); old = commit(repo, "pkg/a.go"); new = commit(repo, "pkg/b.go"); git(repo, "tag", "v0.2.4", new) + case("commit older than the latest release", 0, "older than v0.2.4", "", repo, old, LABELS_JSON="[]") + case("commit already tagged", 0, "already tagged v0.2.4", "", repo, new, LABELS_JSON="[]") + repo, base = fresh(root); git(repo, "checkout", "-q", "-b", "side", base); side = commit(repo, "pkg/side.go") + git(repo, "tag", "v0.2.4", side); git(repo, "checkout", "-q", "main"); git(repo, "tag", "-d", "v0.2.4") + git(repo, "tag", "v0.2.4-not-a-release", side) + main_sha = commit(repo, "pkg/main.go") + case("pre-release style tags don't count", 0, "patch after v0.2.3", "v0.2.4", repo, main_sha, created="v0.2.4", LABELS_JSON="[]") + repo, base = fresh(root); git(repo, "checkout", "-q", "-b", "side", base); side = commit(repo, "pkg/side.go") + git(repo, "checkout", "-q", "main"); main_sha = commit(repo, "pkg/main.go") + git(repo, "tag", "v0.2.4", side) + case("latest release on a branch main left (go-rewrite)", 0, "comparing their files", "v0.2.5", repo, main_sha, created="v0.2.5", + LABELS_JSON="[]") + repo, base = fresh(root); git(repo, "checkout", "-q", "-b", "side", base); side = commit(repo, "pkg/side.go") + git(repo, "tag", "v0.2.4", side); git(repo, "checkout", "-q", "main"); git(repo, "merge", "-q", "--no-edit", "side") + docs = commit(repo, "README.md") + case("main has the release's files, plus docs", 0, "nothing that ships", "", repo, docs, LABELS_JSON="[]") + repo, _ = fresh(root); code = commit(repo, "pkg/api/client.go") + case("next tag made meanwhile by another run", 1, "already exists", "", repo, code, LABELS_JSON="[]", STUB_EXISTS="v0.2.4") + repo, _ = fresh(root); code = commit(repo, "pkg/api/client.go") + case("dry run creates nothing", 0, "Would release v0.3.0", "", repo, code, LABELS_JSON='["release:minor"]', DRY_RUN="true") + repo, _ = fresh(root, tags=False); code = commit(repo, "pkg/api/client.go") + case("no tags yet: first release", 0, "patch after v0.0.0", "v0.0.1", repo, code, created="v0.0.1", LABELS_JSON="[]") + repo, _ = fresh(root); code = commit(repo, "pkg/api/client.go") + log = case("labels looked up from the merged PR", 0, "minor after", "v0.3.0", repo, code, created="v0.3.0", STUB_LABELS="release:minor") + if f"commits/{code}/pulls" not in log: + failures += 1 + print("FAIL the label lookup didn't ask for the commit's pull requests:", log.strip()[:120]) + repo, _ = fresh(root); code = commit(repo, "pkg/api/client.go") + case("unknown BUMP", 2, "BUMP must be", "", repo, code, BUMP="bogus") + repo, _ = fresh(root); major_pr = commit(repo, "pkg/api/client.go"); head = commit(repo, "pkg/cmd/root.go") + with open(f"{root}/labels-{major_pr}", "w") as f: + f.write("release:major\n") + log = case("earlier release:major merge, unlabeled head", 0, "major after v0.2.3", "v1.0.0", repo, head, created="v1.0.0", + STUB_DIR=root) + if f"commits/{major_pr}/pulls" not in log: + failures += 1 + print("FAIL the earlier merge's labels weren't looked up:", log.strip()[:160]) + repo, _ = fresh(root); skip_pr = commit(repo, "pkg/api/client.go"); head = commit(repo, "pkg/cmd/root.go") + with open(f"{root}/labels-{skip_pr}", "w") as f: + f.write("release:skip\n") + case("earlier release:skip doesn't hold back the head", 0, "patch after v0.2.3", "v0.2.4", repo, head, created="v0.2.4", + STUB_DIR=root) + repo, _ = fresh(root); minor_pr = commit(repo, "pkg/api/client.go"); head = commit(repo, "pkg/cmd/root.go") + with open(f"{root}/labels-{minor_pr}", "w") as f: + f.write("release:minor\n") + case("preview: PR labels plus an earlier minor merge", 0, "Would release v0.3.0", "", repo, head, LABELS_JSON="[]", + DRY_RUN="true", STUB_DIR=root) + repo, _ = fresh(root); docs_major = commit(repo, "README.md"); head = commit(repo, "pkg/cmd/root.go") + with open(f"{root}/labels-{docs_major}", "w") as f: + f.write("release:major\n") + case("earlier docs-only release:major doesn't count", 0, "patch after v0.2.3", "v0.2.4", repo, head, created="v0.2.4", + STUB_DIR=root) + repo, _ = fresh(root); code = commit(repo, "pkg/api/client.go") + case("tag API failure fails the job", 1, "HTTP 500", "", repo, code, LABELS_JSON="[]", STUB_FAIL_CREATE="1") + repo, _ = fresh(root); code = commit(repo, "pkg/api/client.go") + case("label lookup failure fails the job", 1, "HTTP 502", "", repo, code, STUB_FAIL_LABELS="1") + repo, _ = fresh(root); code = commit(repo, "pkg/api/client.go") + log = case("dry run with labels makes no API call", 0, "Would release", "", repo, code, LABELS_JSON="[]", DRY_RUN="true") + if log.strip(): + failures += 1 + print("FAIL dry run called gh:", log.strip()[:120]) + + shutil.rmtree(root) + print(f"{failures} failed") + sys.exit(1 if failures else 0) + + +if __name__ == "__main__": + main() diff --git a/.github/scripts/release-tag.sh b/.github/scripts/release-tag.sh new file mode 100755 index 0000000..c4f769d --- /dev/null +++ b/.github/scripts/release-tag.sh @@ -0,0 +1,129 @@ +#!/usr/bin/env bash +# Decides whether a commit on main is released, and as which version, and +# tags it. ci.yml runs it after the tests pass on main; release-preview.yml +# runs it with DRY_RUN=true on a pull request's merge commit, to show what +# merging releases. +# +# Environment: +# SHA the commit +# BUMP auto (default), patch, minor or major. auto bumps minor or +# major when any pull request merged since the latest release +# has the release:minor or release:major label, else patch; +# releases nothing when the merged pull request has +# release:skip; and releases only when something that ships +# changed since the latest release. +# LABELS_JSON the pull request's labels as a JSON array; looked up from the +# commit's merged pull request when unset +# DRY_RUN true to only report +# GITHUB_REPOSITORY, GH_TOKEN for the API +# Outputs, to GITHUB_OUTPUT: tag, the tag created for release.yml (empty when +# nothing is released); next, the version this commit gets or would get; +# reason, one line on what happened. +set -euo pipefail + +sha=$(git rev-parse --verify "${SHA:?SHA is required}^{commit}") +bump=${BUMP:-auto} +dry_run=${DRY_RUN:-false} + +output() { + if [ -n "${GITHUB_OUTPUT:-}" ]; then printf '%s=%s\n' "$1" "$2" >>"$GITHUB_OUTPUT"; fi +} + +finish() { + echo "$1" + output reason "$1" + exit 0 +} + +# Releases are vMAJOR.MINOR.PATCH; pre-release tags such as v1.0.0-rc.1 don't count. +release_tags() { git tag --list 'v*' | grep -E '^v[0-9]+\.[0-9]+\.[0-9]+$' | sort -V || true; } + +existing=$(git tag --points-at "$sha" | grep -E '^v[0-9]+\.[0-9]+\.[0-9]+$' | sort -V | tail -n 1 || true) +if [ -n "$existing" ]; then + output next "$existing" + finish "Not released again: $sha is already tagged $existing" +fi + +pr_labels() { + gh api "repos/${GITHUB_REPOSITORY:?}/commits/$1/pulls" --jq '.[] | select(.merged_at != null) | .labels[].name' +} + +# The files between two commits that ship in the binary, tests excluded. +shipped() { + git diff --name-only "$1" "$2" -- cmd pkg internal go.mod go.sum .goreleaser.yml \ + ':(exclude)*_test.go' ':(exclude)*/testdata/*' +} + +latest=$(release_tags | tail -n 1) +since=$(git hash-object -t tree /dev/null) +earlier="" +if [ -n "$latest" ]; then + since=$(git rev-list -n 1 "$latest") + if git merge-base --is-ancestor "$sha" "$since"; then + finish "Not released: $sha is older than $latest" + fi + # v0.2.1 to v0.2.3 were tagged on go-rewrite after it merged, so main + # doesn't contain them; comparing trees doesn't need it to. + if ! git merge-base --is-ancestor "$since" "$sha"; then + echo "$latest is on a commit that $sha doesn't contain; comparing their files" + fi + if git rev-parse -q --verify "$sha^" >/dev/null; then + earlier=$(git rev-list --first-parent "$sha^" --not "$since") + fi +else + latest=v0.0.0 +fi + +if [ "$bump" = auto ]; then + if [ -n "${LABELS_JSON:-}" ]; then + labels=$(jq -r '.[]' <<<"$LABELS_JSON") + else + labels=$(pr_labels "$sha") + fi + if grep -qx 'release:skip' <<<"$labels"; then + finish "Not released: the pull request has the release:skip label" + fi + # A release:major or release:minor pull request merged since the latest + # release counts too, since its own run may have failed, but only if it + # changed what ships: a docs-only one released nothing. + while read -r c; do + if [ -n "$c" ] && [ -n "$(shipped "$c^" "$c")" ]; then labels+=$'\n'$(pr_labels "$c"); fi + done <<<"$earlier" + if grep -qx 'release:major' <<<"$labels"; then + bump="major" + elif grep -qx 'release:minor' <<<"$labels"; then + bump="minor" + else + bump="patch" + fi + if [ -z "$(shipped "$since" "$sha")" ]; then + finish "Not released: nothing that ships changed since $latest, only docs, tests or CI" + fi +fi + +IFS=. read -r major minor patch <<<"${latest#v}" +case $bump in +major) next="v$((major + 1)).0.0" ;; +minor) next="v$major.$((minor + 1)).0" ;; +patch) next="v$major.$minor.$((patch + 1))" ;; +*) + echo "BUMP must be auto, patch, minor or major, not $bump" >&2 + exit 2 + ;; +esac +output next "$next" + +if git rev-parse -q --verify "refs/tags/$next" >/dev/null; then + echo "::error::$next already exists on another commit; a published tag is never moved or reused" >&2 + exit 1 +fi + +if [ "$dry_run" = true ]; then + finish "Would release $next ($bump after $latest) at $sha" +fi + +# A lightweight tag, as the earlier releases have. The API refuses a tag that +# already exists, so two runs can't both make it. +gh api "repos/${GITHUB_REPOSITORY:?}/git/refs" -f ref="refs/tags/$next" -f sha="$sha" >/dev/null +output tag "$next" +finish "Tagged $sha as $next ($bump after $latest)" diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 6f2d9c2..6671e48 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -5,9 +5,27 @@ on: branches: [main, develop] pull_request: branches: [main] + workflow_dispatch: + inputs: + bump: + description: "Version for main's head: auto follows the merged PR's release:* label and releases only when something that ships changed" + type: choice + options: [auto, patch, minor, major] + default: auto + dry_run: + description: Only report what would be released + type: boolean + default: true permissions: {} +# Runs on main test, tag and release one at a time, in order; each waits its +# turn rather than replacing a waiting one. Other runs never wait. +concurrency: + group: ${{ github.ref == 'refs/heads/main' && 'release-main' || github.run_id }} + cancel-in-progress: false + queue: max + jobs: test: runs-on: ubuntu-latest @@ -39,7 +57,60 @@ jobs: go-version: stable check-latest: true # Fork PRs get no secrets, so only the offline contract tests run there. + # Anywhere else a skipped live test fails the job, since main's runs + # release when they pass. - name: Run E2E tests + shell: bash env: SERPAPI_KEY: ${{ secrets.SERPAPI_KEY }} - run: go test -v ./tests/... + REQUIRE_LIVE: ${{ github.event_name != 'pull_request' }} + run: | + go test -v ./tests/... | tee e2e.log + if [ "$REQUIRE_LIVE" = true ] && grep -q -- '--- SKIP' e2e.log; then + echo "::error::Live e2e tests were skipped: is the SERPAPI_KEY secret set?" + exit 1 + fi + + # After the tests pass on main, release-tag.sh tags the commit with the next + # version, and release.yml releases it. A tag pushed with GITHUB_TOKEN + # doesn't start release.yml's tag trigger, so this run calls it. + release-script: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 + with: + persist-credentials: false + - run: python3 .github/scripts/release-tag-test.py + + tag: + needs: [test, e2e, release-script] + if: github.ref == 'refs/heads/main' && github.event_name != 'pull_request' + runs-on: ubuntu-latest + permissions: + contents: write + pull-requests: read + outputs: + tag: ${{ steps.tag.outputs.tag }} + steps: + - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 + with: + fetch-depth: 0 + persist-credentials: false + - id: tag + env: + GH_TOKEN: ${{ github.token }} + SHA: ${{ github.sha }} + BUMP: ${{ inputs.bump || 'auto' }} + DRY_RUN: ${{ inputs.dry_run || false }} + run: bash .github/scripts/release-tag.sh + + release: + needs: tag + if: needs.tag.outputs.tag != '' + permissions: + contents: write + uses: ./.github/workflows/release.yml + with: + tag: ${{ needs.tag.outputs.tag }} + sha: ${{ github.sha }} + secrets: inherit diff --git a/.github/workflows/release-preview.yml b/.github/workflows/release-preview.yml new file mode 100644 index 0000000..f3ba4a0 --- /dev/null +++ b/.github/workflows/release-preview.yml @@ -0,0 +1,38 @@ +name: Release preview + +# Says on each pull request what merging it releases, so a missing +# release:minor label shows up before the merge. Reads nothing secret. +on: + pull_request: + branches: [main] + types: [opened, synchronize, reopened, labeled, unlabeled] + +permissions: {} + +jobs: + preview: + runs-on: ubuntu-latest + permissions: + contents: read + pull-requests: read + steps: + - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 + with: + fetch-depth: 0 + persist-credentials: false + - id: tag + env: + # gh needs a token in Actions; this one only reads. + GH_TOKEN: ${{ github.token }} + SHA: ${{ github.sha }} + LABELS_JSON: ${{ toJSON(github.event.pull_request.labels.*.name) }} + DRY_RUN: "true" + run: bash .github/scripts/release-tag.sh + - env: + REASON: ${{ steps.tag.outputs.reason }} + NEXT: ${{ steps.tag.outputs.next }} + run: | + notes="none: GoReleaser will list the commits" + if [ -n "$NEXT" ] && [ -f ".github/release-notes/$NEXT.md" ]; then notes=".github/release-notes/$NEXT.md"; fi + echo "::notice title=Merging releases::$REASON; release notes: $notes" + printf '### Merging this pull request\n\n%s\n\nRelease notes: %s\n' "$REASON" "$notes" >> "$GITHUB_STEP_SUMMARY" diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 30d63e7..f662379 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -4,38 +4,119 @@ on: push: tags: - "v*" + # ci.yml calls this after it tags a commit on main. + workflow_call: + inputs: + tag: + description: The tag to release + type: string + required: true + sha: + description: The tested commit the tag must point at + type: string + required: true + # To release an existing tag again, e.g. after a failed run. + workflow_dispatch: + inputs: + tag: + description: An existing v* tag to release + type: string + required: true permissions: {} jobs: - goreleaser: + # One job builds, uploads and updates Homebrew while holding the lock, so + # releases from CI, a tag push or a dispatch never interleave. + publish: runs-on: ubuntu-latest permissions: contents: write + concurrency: + group: release-publish + cancel-in-progress: false + queue: max + env: + TAG: ${{ inputs.tag || github.ref_name }} + SHA: ${{ inputs.sha || (github.event_name == 'push' && github.sha) || '' }} steps: + # TAG may come from a dispatch input; check it before it reaches a ref, + # a path or GoReleaser's arguments. + - name: Check the tag + run: | + if ! [[ $TAG =~ ^v[0-9]+\.[0-9]+\.[0-9]+(-[0-9A-Za-z.-]+)?$ ]]; then + echo "::error::$TAG is not a vX.Y.Z tag" + exit 1 + fi - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 with: + ref: refs/tags/${{ env.TAG }} fetch-depth: 0 + - name: Check what the tag points at + run: | + if ! git rev-parse -q --verify "refs/tags/$TAG" >/dev/null; then + echo "::error::$TAG is not a tag" + exit 1 + fi + at=$(git rev-parse "$TAG^{commit}") + if [ "$(git rev-parse HEAD)" != "$at" ]; then + echo "::error::checked out $(git rev-parse HEAD), not $TAG at $at" + exit 1 + fi + if [ -n "$SHA" ] && [ "$at" != "$SHA" ]; then + echo "::error::$TAG points at $at, not at the tested $SHA" + exit 1 + fi + newest=$(git tag --list 'v*' | grep -E '^v[0-9]+\.[0-9]+\.[0-9]+$' | sort -V | tail -n 1) + if [ "$(printf '%s\n%s\n' "$newest" "$TAG" | sort -V | tail -n 1)" != "$TAG" ]; then + echo "::error::$newest is newer than $TAG; releasing $TAG would mark it latest and point Homebrew back at it" + exit 1 + fi + # A published version is never built again: a newer Go would change + # binaries users may already have. A complete release only gets its + # Homebrew formula refreshed; a partial one is left for a fix forward. + - id: existing + env: + GH_TOKEN: ${{ github.token }} + run: | + if ! assets=$(gh api "repos/$GITHUB_REPOSITORY/releases/tags/$TAG" --jq '.assets[].name' 2>&1); then + if grep -q 'HTTP 404' <<<"$assets"; then exit 0; fi + echo "::error::Couldn't check for a $TAG release: $assets" + exit 1 + fi + sums=$(gh release download "$TAG" --repo "$GITHUB_REPOSITORY" --pattern 'serpapi_*_checksums.txt' --output - 2>/dev/null || true) + missing=$(awk '{print $2}' <<<"$sums" | grep -vxF -e "$assets" || true) + if [ -n "$sums" ] && [ -z "$missing" ]; then + echo "skip=true" >> "$GITHUB_OUTPUT" + echo "::notice::$TAG is already released with every file in its checksums; only Homebrew is refreshed" + else + echo "::error::$TAG has an incomplete release, which is never rebuilt; release the next version instead (Actions, CI, Run workflow on main with bump patch)" + exit 1 + fi - uses: actions/setup-go@d35c59abb061a4a6fb18e82ac0862c26744d6ab5 # v5.5.0 with: # Not go.mod: its go line is the minimum for go install users, while # release binaries need the latest patched Go (v0.2.3 shipped go1.25.0). go-version: stable check-latest: true + # .github/release-notes/vX.Y.Z.md, when the release has one, replaces + # GoReleaser's list of commits. + - id: notes + run: | + if [ -f ".github/release-notes/$TAG.md" ]; then + echo "args=--release-notes .github/release-notes/$TAG.md" >> "$GITHUB_OUTPUT" + fi - uses: goreleaser/goreleaser-action@9ed2f89a662bf1735a48bc8557fd212fa902bebf # v6.2.1 + if: steps.existing.outputs.skip != 'true' with: version: ~> v2 - args: release --clean + args: release --clean ${{ steps.notes.outputs.args }} env: GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + GORELEASER_CURRENT_TAG: ${{ env.TAG }} - homebrew: - runs-on: ubuntu-latest - needs: goreleaser - steps: - name: Update Homebrew formula env: - TAG: ${{ github.ref_name }} HOMEBREW_TAP_DEPLOY_KEY: ${{ secrets.HOMEBREW_TAP_DEPLOY_KEY }} run: | VERSION="${TAG#v}" diff --git a/AGENTS.md b/AGENTS.md index 28c7aa5..cfeddd2 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -2,20 +2,20 @@ ## Releases -- A push or merge to `main` releases nothing. It runs CI (`.github/workflows/ci.yml`), whose e2e job makes real searches with the `SERPAPI_KEY` secret. -- Only a pushed `v*` tag releases. `.github/workflows/release.yml` then: - 1. runs GoReleaser (`.goreleaser.yml`), which publishes a GitHub release with 5 archives (darwin and linux on amd64 and arm64, windows on amd64) and `serpapi__checksums.txt`, and stamps the version into `serpapi --version`; +- A merge to `main` releases the CLI once CI passes on the merge commit, live e2e included; outside pull requests a skipped live test fails CI, so a release never rests on skipped tests. The CI `tag` job runs `.github/scripts/release-tag.sh`, which tags the commit with the next version, then calls `.github/workflows/release.yml` for that tag. +- The next version is a patch after the highest `vX.Y.Z` tag. Label the pull request `release:minor` or `release:major` for those; the label counts for every later merge until a release goes out, so a failed release run doesn't lose it. `release:skip` releases nothing for that merge. A merge releases nothing either when nothing that ships changed since the latest release: `cmd/`, `pkg/`, `go.mod`, `go.sum` and `.goreleaser.yml`, tests excluded. The "Release preview" check on each pull request says what merging it releases. +- A tag reaches every Homebrew and `go install` user, so merging to `main` is the release decision: label the pull request first. +- `release.yml`: + 1. runs GoReleaser (`.goreleaser.yml`), which publishes a GitHub release with 5 archives (darwin and linux on amd64 and arm64, windows on amd64) and `serpapi__checksums.txt`, and stamps the version into `serpapi --version`. `.github/release-notes/.md`, when it exists, is the release body; otherwise GoReleaser lists the commits; 2. writes `Formula/serpapi-cli.rb` in `serpapi/homebrew-tap` from those checksums and pushes it as a `serpapi-cli ` commit. -- A tag reaches every Homebrew and `go install` user, so tagging is a maintainer's call. Tag the merge commit on `main` once its CI has passed, live e2e included: the live tests skip without `SERPAPI_KEY`, so the e2e log must have no `--- SKIP` line. Tags here are lightweight: - ```bash - git fetch origin - git tag v0.3.0 - git rev-list -n1 v0.3.0 # must print that sha - git push origin v0.3.0 - ``` +- To release by hand, run the CI workflow on `main` from the Actions tab with a bump (`dry_run` is on by default), or push a lightweight `v*` tag on a `main` commit whose CI passed with no `--- SKIP` in the e2e log; `release.yml` runs on the pushed tag. +- If a release job fails after the tag was made, re-run the failed jobs of that CI run, or, once the cause is fixed on `main`, run the Release workflow from the Actions tab with that tag. The tag stays. +- `release.yml` refuses a tag that doesn't point at the tested commit or is older than the newest release (it would become "latest" and roll Homebrew back), and publishes one release at a time. A published version is never built again, since a newer Go would change binaries users may already have: a complete release only gets its Homebrew formula refreshed, and a partial one, from a failed upload, stops it. Release the next version instead: run CI on `main` from the Actions tab with bump `patch`. +- `.github/scripts/release-tag-test.py` tests `release-tag.sh` offline in scratch repos; CI runs it before the `tag` job. - Never move, delete or reuse a published tag: Go's module proxy and checksum database keep the first content they saw for each version, so a moved tag never reaches most `go install` users and fails checksum verification for the rest. -- The release body is GoReleaser's list of commits. To publish written notes instead, run `gh release edit v0.3.0 -R serpapi/serpapi-cli --notes-file ` after the Release run. +- v0.2.1 to v0.2.3 are tagged on `go-rewrite`, which `main` doesn't contain; `release-tag.sh` compares files with the latest release, not history. +- To change a published release's notes: `gh release edit v0.3.0 -R serpapi/serpapi-cli --notes-file `. - Release binaries build with the latest stable Go (`go-version: stable`); the `go` line in `go.mod` is the oldest Go that `go install` users need. - The formula's `test do` block in `release.yml` runs the released binary, e.g. `serpapi account` without a key must exit 2 with "No API key". Change it together with the behavior it checks. -- Check a release: the Release run is green, the release has the 5 archives and the checksums file, the tap has the `serpapi-cli ` commit, and `brew upgrade serpapi-cli && brew test serpapi-cli` passes. +- Check a release: the CI run's `tag` and `release` jobs (or, for a pushed tag, the Release run) are green, the release has the 5 archives and the checksums file, the tap has the `serpapi-cli ` commit, and `brew upgrade serpapi-cli && brew test serpapi-cli` passes. - To pull a bad release, revert the formula commit in the tap so new installs get the previous version, and fix forward: release the next patch version with the bad one retracted in its `go.mod` (`retract v0.3.0` in v0.3.1), since `go install ...@latest` takes the highest tag that isn't retracted. diff --git a/README.md b/README.md index db9f2bd..9bf506c 100644 --- a/README.md +++ b/README.md @@ -131,7 +131,7 @@ The API key comes from the first of: Without a key, `search` still runs, but the API answers only a few demo queries such as `q=coffee` and returns 401 for the rest. `account` and `archive` need a key. -`serpapi login` saves the key, readable only by you, to `serpapi/config.toml` in the OS config directory: `$XDG_CONFIG_HOME` or `~/.config` on Linux, `~/Library/Application Support` on macOS, `%AppData%` on Windows. +`serpapi login` saves the key, readable only by you, to `serpapi/config.toml` in the OS config directory: `$XDG_CONFIG_HOME` or `~/.config` on Linux, `~/Library/Application Support` on macOS, `%AppData%` on Windows. Without an absolute `HOME` (`%AppData%` on Windows) there is no config file: `login` exits 2, and other commands need `--api-key` or `SERPAPI_KEY`. ```toml api_key = "your_serpapi_key_here" @@ -158,9 +158,9 @@ A config file that can't be read or isn't valid TOML is reported by path and lin | 2 | `usage_error` | The command is wrong; fix it before retrying. | | 1 | `api_error` | `status` is the HTTP status: 200 the search ran but reported a problem, such as no results; 400 invalid parameters; 401 invalid API key; 403 account not allowed; 429 rate limited or out of searches; 5xx SerpApi failed. No `status`: the response was unreadable. | | 1 | `network_error` | No answer, such as a timeout. | - | 130 | `interrupted` | Stopped by Ctrl-C or SIGTERM after printing what had arrived, such as the `--all-pages` pages fetched so far. | + | 130 | `interrupted` | Ctrl-C or SIGTERM stopped it before it finished, after printing what had arrived, such as the `--all-pages` pages fetched so far. | -- **Cost**: each successful search counts against your monthly searches, including each `--all-pages` page; cached, errored and failed ones don't, though errored ones count toward the hourly limit ([pricing FAQ](https://serpapi.com/pricing)). An identical search, `--fields` included, repeated within an hour comes from the cache. `no_cache=true` forces a new, counted search; cached results are at most an hour old, so add it only when that is too old. `account`, `locations` and `archive` are free. +- **Cost**: each successful search counts against your monthly searches, including each `--all-pages` page; cached, errored and failed ones don't, though errored ones count toward the hourly limit ([pricing FAQ](https://serpapi.com/pricing)). An identical search, `--fields` included, repeated within an hour comes from the cache. `no_cache=true` forces a new, counted search; cached results are at most an hour old, so add it only when that is too old. `zero_trace=true` (Enterprise) stores nothing, so it skips the cache too and leaves no search for `archive` to read. `account`, `locations` and `archive` are free. - **No prompts without a terminal.** Only `serpapi login` reads stdin, and it waits for the first line, so pipe the key in (`printf %s "$KEY" | serpapi login`) or, simpler, set `SERPAPI_KEY`. ## Links diff --git a/pkg/cmd/cmd_test.go b/pkg/cmd/cmd_test.go index eada884..830d6cd 100644 --- a/pkg/cmd/cmd_test.go +++ b/pkg/cmd/cmd_test.go @@ -9,10 +9,12 @@ import ( "net/http" "net/http/httptest" "net/url" + "os" "strconv" "strings" "sync" "testing" + "time" "github.com/spf13/cobra" "github.com/spf13/pflag" @@ -102,12 +104,18 @@ func execute(t *testing.T, stdin string, args ...string) result { // executeContext is execute with a context the test can cancel, as Ctrl-C does. func executeContext(t *testing.T, ctx context.Context, stdin string, args ...string) result { + t.Helper() + return executeInput(t, ctx, strings.NewReader(stdin), args...) +} + +// executeInput is executeContext with any stdin, such as a pipe that stays open. +func executeInput(t *testing.T, ctx context.Context, stdin io.Reader, args ...string) result { t.Helper() resetFlags(rootCmd) var stdout, stderr bytes.Buffer rootCmd.SetOut(&stdout) rootCmd.SetErr(&stderr) - rootCmd.SetIn(strings.NewReader(stdin)) + rootCmd.SetIn(stdin) // A nil slice would make cobra parse the test binary's own arguments. rootCmd.SetArgs(append([]string{}, args...)) code := run(ctx) @@ -606,6 +614,28 @@ func TestLoginWithoutKey(t *testing.T) { wantError(t, execute(t, "\n", "login"), 2, "usage_error", "No API key given") } +// An agent's timeout sends SIGTERM to a login still waiting on an open pipe; +// it has to stop then, not when the grace period runs out. +func TestLoginStopsAtOnceWhenInterrupted(t *testing.T) { + fakeAPI(t, nil) + stdin, writer := io.Pipe() + t.Cleanup(func() { writer.Close() }) + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + time.AfterFunc(50*time.Millisecond, cancel) + + start := time.Now() + res := executeInput(t, ctx, stdin, "login") + + wantError(t, res, 130, "interrupted", "Stopped while waiting for the API key on stdin; nothing was saved") + if elapsed := time.Since(start); elapsed > time.Second { + t.Errorf("login took %v to stop, want well under the %v grace period", elapsed, interruptGrace) + } + if _, err := os.Stat(config.Path()); !os.IsNotExist(err) { + t.Errorf("config file after an interrupted login: %v", err) + } +} + // login shows the account's email on a terminal, so it must be as safe there // as a search result: a real PTY run printed ESC ] 52 and ESC [ 2J from it. func TestAccountEmailIsSafeOnATerminal(t *testing.T) { @@ -682,7 +712,9 @@ func TestHelpAndVersionGoToStdout(t *testing.T) { }{ {[]string{"--help"}, "SERPAPI_KEY"}, {[]string{"search", "--help"}, "--all-pages"}, + {[]string{"help", "search"}, "--all-pages"}, {[]string{"--version"}, "serpapi version "}, + {[]string{"help", "help"}, "Help provides help for any command"}, } { res := execute(t, "", tt.args...) if res.code != 0 || res.stderr != "" || !strings.Contains(res.stdout, tt.want) { @@ -690,3 +722,35 @@ func TestHelpAndVersionGoToStdout(t *testing.T) { } } } + +// A help topic that doesn't exist is a wrong command: exit 2 and one JSON +// line, not the usage as plain text with exit 0. +func TestHelpWithAnUnknownTopic(t *testing.T) { + fakeAPI(t, nil) + + for topic, want := range map[string]string{ + "serach": `Unknown help topic "serach"; did you mean search?`, + "nope": `Unknown help topic "nope"; see 'serpapi --help'`, + } { + res := execute(t, "", "help", topic) + + errObj := wantError(t, res, 2, "usage_error", want) + if errObj["message"] != want { + t.Errorf("help %s: message = %q, want %q", topic, errObj["message"], want) + } + if res.stdout != "" { + t.Errorf("help %s: stdout = %q, want empty", topic, res.stdout) + } + } +} + +// The help command still completes command names, and only them. +func TestHelpCompletesCommands(t *testing.T) { + fakeAPI(t, nil) + + res := execute(t, "", "__complete", "help", "se") + + if res.code != 0 || !strings.HasPrefix(res.stdout, "search\t") || !strings.Contains(res.stdout, "\n:4\n") { + t.Errorf("exit %d, stdout %q; want search and directive 4 (no file completion)", res.code, res.stdout) + } +} diff --git a/pkg/cmd/edge_test.go b/pkg/cmd/edge_test.go index a344ca9..8c29590 100644 --- a/pkg/cmd/edge_test.go +++ b/pkg/cmd/edge_test.go @@ -64,6 +64,13 @@ func searchWith(args ...string) []string { return append(append([]string{}, sear func edgeCases() []edgeCase { invalidKey := `{"error":"Invalid API key. Your API key should be here: https://serpapi.com/manage-api-key"}` pages := func(w http.ResponseWriter, r *http.Request) { fmt.Fprint(w, page(pageNumber(r), pageNumber(r) < 3)) } + bigPages := func(w http.ResponseWriter, r *http.Request) { + next := "" + if pageNumber(r) == 1 { + next = `"serpapi_pagination":{"next":"https://serpapi.com/search.json?engine=google&q=coffee&start=10"},` + } + fmt.Fprint(w, bigResult(next+`"inline_images":[{"position":1}],`, 700)) + } return []edgeCase{ {name: "argv/no parameters", args: []string{"search"}, exit: 2, code: "usage_error", msg: "No search parameters given"}, {name: "argv/q and bare words", args: searchWith("tea"), exit: 2, code: "usage_error", msg: "Got both q=... and bare words"}, @@ -202,17 +209,33 @@ func edgeCases() []edgeCase { warn: "keys by size: organic_results 52 KB, search_metadata 24 B. To print only what you need, read the stored search for free: serpapi archive abc123 --jq '.KEY'"}, {name: "output/large result without an id", args: search, api: respond(bigResult("", 1500)), warn: "repeat the command with --jq '.KEY': the same search within an hour comes from the cache"}, + {name: "output/large no_cache result promises no cache hit", args: searchWith("no_cache=true"), api: respond(bigResult("", 1500)), + warn: "repeat the command with --jq '.KEY'; no_cache=true makes the repeat a new, counted search"}, + {name: "output/large no_cache result keeps the free archive", args: searchWith("no_cache=true"), api: respond(bigResult(`"search_metadata":{"id":"abc123"},`, 1500)), + warn: "read the stored search for free: serpapi archive abc123 --jq '.KEY'"}, + {name: "output/large no_cache in the API's true words", args: searchWith("no_cache=YES"), api: respond(bigResult("", 1500)), + warn: "no_cache=YES makes the repeat a new, counted search"}, + {name: "output/large no_cache in the API's false words", args: searchWith("no_cache=False"), api: respond(bigResult("", 1500)), + warn: "repeat the command with --jq '.KEY': the same search within an hour comes from the cache"}, + {name: "output/large no_cache in neither list", args: searchWith("no_cache=maybe"), api: respond(bigResult("", 1500)), + warn: "repeat the command with --jq '.KEY'; with no_cache=maybe the repeat may be a new, counted search"}, + {name: "output/large zero_trace result has no archive", args: searchWith("zero_trace=true"), api: respond(bigResult(`"search_metadata":{"id":"abc123"},`, 1500)), + warn: "repeat the command with --jq '.KEY'; zero_trace=true stores nothing and makes the repeat a new, counted search"}, + {name: "output/large zero_trace=0 keeps the archive", args: searchWith("zero_trace=0"), api: respond(bigResult(`"search_metadata":{"id":"abc123"},`, 1500)), + warn: "read the stored search for free: serpapi archive abc123 --jq '.KEY'"}, + {name: "output/large unsure zero_trace with no_cache=true", args: searchWith("zero_trace=maybe", "no_cache=true"), + api: respond(bigResult(`"search_metadata":{"id":"abc123"},`, 1500)), + warn: "repeat the command with --jq '.KEY'; no_cache=true makes the repeat a new, counted search"}, {name: "output/large result with --jq is quiet", args: searchWith("--jq", ".organic_results | length"), api: respond(bigResult("", 1500)), stdout: "1500\n", quiet: true}, {name: "output/large result with --fields still hints", args: searchWith("--fields", "organic_results"), api: respond(bigResult("", 1500)), warn: "Large result: 52 KB"}, - {name: "output/large merged pages", args: searchWith("--all-pages"), reqs: 2, api: func(w http.ResponseWriter, r *http.Request) { - next := "" - if pageNumber(r) == 1 { - next = `"serpapi_pagination":{"next":"https://serpapi.com/search.json?engine=google&q=coffee&start=10"},` - } - fmt.Fprint(w, bigResult(next+`"inline_images":[{"position":1}],`, 700)) - }, warn: "Large result: 48 KB; top-level keys by size: organic_results 48 KB, inline_images 70 B. To print only what you need, repeat the command"}, + {name: "output/large merged pages", args: searchWith("--all-pages"), reqs: 2, api: bigPages, + warn: "Large result: 48 KB; top-level keys by size: organic_results 48 KB, inline_images 70 B. To print only what you need, repeat the command"}, + {name: "output/large no_cache merged pages", args: searchWith("no_cache=true", "--all-pages"), reqs: 2, api: bigPages, + warn: "no_cache=true makes the repeat a new, counted search for each page"}, + {name: "output/large zero_trace merged pages", args: searchWith("zero_trace=1", "--all-pages"), reqs: 2, api: bigPages, + warn: "zero_trace=1 stores nothing and makes the repeat a new, counted search for each page"}, {name: "output/large archive", args: []string{"archive", "abc123"}, api: respond(bigResult(`"search_metadata":{"id":"abc123"},`, 1500)), warn: "add --jq '.KEY'; reading the archive is free"}, {name: "output/jq printing only null lists the keys", args: searchWith("--jq", ".organic"), api: oneResult, stdout: "\n", @@ -411,6 +434,58 @@ func TestInterruptDuringAllPagesPrintsFetchedPages(t *testing.T) { } } +// interruptAtPage3 serves pages 1 and 2 and cancels ctx once page 3 is asked +// for, as Ctrl-C during a slow page would. +func interruptAtPage3(t *testing.T, cancel context.CancelFunc) { + t.Helper() + reached := make(chan struct{}) + fakeAPI(t, func(w http.ResponseWriter, r *http.Request) { + if n := pageNumber(r); n < 3 { + fmt.Fprint(w, page(n, true)) + return + } + close(reached) + <-r.Context().Done() + }) + t.Setenv("SERPAPI_KEY", "envkey") + go func() { + <-reached + cancel() + }() +} + +// The pages fetched before Ctrl-C were billed, so --jq still runs on them. +func TestInterruptDuringAllPagesWithJQPrintsFetchedPages(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + interruptAtPage3(t, cancel) + + res := executeContext(t, ctx, "", searchWith("--all-pages", "--jq", ".organic_results | length")...) + + wantError(t, res, 130, "interrupted", "Stopped while fetching page 3; pages 1-2 were printed") + if res.stdout != "2\n" { + t.Errorf("stdout = %q, want --jq's result on the 2 fetched pages", res.stdout) + } +} + +// --jq on the fetched pages gets interruptGrace after the signal, no more. +func TestInterruptDuringAllPagesBoundsAnEndlessJQ(t *testing.T) { + grace := interruptGrace + interruptGrace = 100 * time.Millisecond + t.Cleanup(func() { interruptGrace = grace }) + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + interruptAtPage3(t, cancel) + + start := time.Now() + res := executeContext(t, ctx, "", searchWith("--all-pages", "--jq", "reduce range(1e12) as $i (0; . + 1)")...) + + wantError(t, res, 130, "interrupted", "Stopped while running --jq") + if elapsed := time.Since(start); elapsed > 5*time.Second { + t.Errorf("took %v, want --jq stopped about interruptGrace after the signal", elapsed) + } +} + func TestInterruptBeforeTheResponse(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) defer cancel() @@ -445,6 +520,119 @@ func TestInterruptDuringJQKeepsWhatItPrinted(t *testing.T) { } } +// cancelAtEndOf cancels ctx as the client reads the last byte of page n: +// after the page arrived, before anything is printed. +func cancelAtEndOf(t *testing.T, n int, cancel context.CancelFunc) { + t.Helper() + orig := http.DefaultTransport + t.Cleanup(func() { http.DefaultTransport = orig }) + http.DefaultTransport = roundTripFunc(func(r *http.Request) (*http.Response, error) { + resp, err := orig.RoundTrip(r) + if err == nil && pageNumber(r) == n { + resp.Body = &onEOF{ReadCloser: resp.Body, do: cancel} + } + return resp, err + }) +} + +type roundTripFunc func(*http.Request) (*http.Response, error) + +func (f roundTripFunc) RoundTrip(r *http.Request) (*http.Response, error) { return f(r) } + +// onEOF calls do once the body has been read to the end. +type onEOF struct { + io.ReadCloser + do func() +} + +func (b *onEOF) Read(p []byte) (int, error) { + n, err := b.ReadCloser.Read(p) + if err == io.EOF { + b.do() + } + return n, err +} + +// A signal after the last page arrived, before it was printed, still prints +// every page: all of them were billed. +func TestInterruptAfterTheLastPagePrintsThemAll(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + fakeAPI(t, func(w http.ResponseWriter, r *http.Request) { fmt.Fprint(w, page(pageNumber(r), pageNumber(r) < 3)) }) + t.Setenv("SERPAPI_KEY", "envkey") + cancelAtEndOf(t, 3, cancel) + + res := executeContext(t, ctx, "", searchWith("--all-pages", "--jq", ".organic_results | length")...) + + wantOK(t, res) + if res.stdout != "3\n" || ctx.Err() == nil { + t.Errorf("stdout = %q, signaled = %v; want --jq's result on all 3 pages after the signal", res.stdout, ctx.Err() != nil) + } +} + +func TestInterruptAfterTheResponseStillRunsJQ(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + fakeAPI(t, func(w http.ResponseWriter, _ *http.Request) { fmt.Fprint(w, page(1, false)) }) + t.Setenv("SERPAPI_KEY", "envkey") + cancelAtEndOf(t, 1, cancel) + + res := executeContext(t, ctx, "", searchWith("--jq", ".organic_results[0].position")...) + + wantOK(t, res) + if res.stdout != "1\n" || ctx.Err() == nil { + t.Errorf("stdout = %q, signaled = %v; want --jq's result after the signal", res.stdout, ctx.Err() != nil) + } +} + +// --jq that fails on the pages fetched before a signal reports its own error: +// those pages did arrive. +func TestJQErrorAfterAnInterruptIsReported(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + interruptAtPage3(t, cancel) + + res := executeContext(t, ctx, "", searchWith("--all-pages", "--jq", ".organic_results[].position, (.missing | keys)")...) + + wantError(t, res, 2, "usage_error", "keys cannot be applied to: null") + if res.stdout != "1\n2\n" { + t.Errorf("stdout = %q, want what --jq printed before its error", res.stdout) + } +} + +// After a signal, --jq on the fetched pages stops, and flushes what it +// printed, before the process would exit at interruptGrace. +func TestInterruptedJQStopsBeforeTheProcessExits(t *testing.T) { + grace := interruptGrace + interruptGrace = time.Second + t.Cleanup(func() { interruptGrace = grace }) + ctx, cancel := context.WithCancelCause(context.Background()) + defer cancel(nil) + interruptAtPage3(t, func() { cancel(&signaled{at: time.Now()}) }) + var watchdog lockedBuffer + done, returned := make(chan struct{}), make(chan struct{}) + go func() { + exitAfterGrace(ctx, done, func() {}, &watchdog, func(int) {}) + close(returned) + }() + + res := executeContext(t, ctx, "", searchWith("--all-pages", "--jq", ".organic_results[0].position, (reduce range(1e12) as $i (0; . + 1))")...) + close(done) + select { + case <-returned: + case <-time.After(10 * time.Second): + t.Fatal("exitAfterGrace never returned") + } + + if got := watchdog.String(); got != "" { + t.Errorf("the process would have exited before --jq stopped: %s", got) + } + wantError(t, res, 130, "interrupted", "Stopped while running --jq") + if res.stdout != "1\n" { + t.Errorf("stdout = %q, want the value --jq printed before it was stopped", res.stdout) + } +} + // lockedBuffer is a bytes.Buffer safe to write from another goroutine. type lockedBuffer struct { mu sync.Mutex @@ -496,3 +684,67 @@ func TestExitAfterGrace(t *testing.T) { func(code int) { t.Errorf("exit(%d) without a signal", code) }) }) } + +// The process exits interruptGrace after the signal, however long the +// command took to notice it. +func TestExitAfterGraceCountsFromTheSignal(t *testing.T) { + grace := interruptGrace + interruptGrace = 2 * time.Second + t.Cleanup(func() { interruptGrace = grace }) + ctx, cancel := context.WithCancelCause(context.Background()) + cancel(&signaled{at: time.Now().Add(-interruptGrace)}) + exited := make(chan struct{}) + var stderr lockedBuffer + start := time.Now() + + go exitAfterGrace(ctx, make(chan struct{}), func() {}, &stderr, func(int) { close(exited) }) + + select { + case <-exited: + if elapsed := time.Since(start); elapsed > time.Second { + t.Errorf("exited %v after the watchdog woke, want at once: the grace had run out", elapsed) + } + case <-time.After(5 * time.Second): + t.Fatal("never exited") + } + // The message has a goroutine of its own; outliving this test, it would + // race with tests that swap output.IsTerminal. + for deadline := time.Now().Add(5 * time.Second); !strings.Contains(stderr.String(), "Stopped 2s after the signal"); time.Sleep(time.Millisecond) { + if time.Now().After(deadline) { + t.Fatalf("stderr = %q, want the watchdog's message", stderr.String()) + } + } +} + +// --jq after a signal stops a quarter of interruptGrace before the process +// exits, counted from the signal, so a slow decode before printing doesn't +// push it past the exit. +func TestPrintDeadlineCountsFromTheSignal(t *testing.T) { + ctx, cancel := context.WithCancelCause(context.Background()) + at := time.Now().Add(-time.Second) + cancel(&signaled{at: at}) + + printCtx, stop := printContext(ctx) + defer stop() + + want := at.Add(interruptGrace - interruptGrace/4) + if d, ok := printCtx.Deadline(); !ok || !d.Equal(want) { + t.Errorf("deadline %v (set: %v), want %v", d, ok, want) + } +} + +// net/http reports a canceled request with the context's cause, so a signal +// still reads as a stop before the response. +func TestSignalBeforeTheResponse(t *testing.T) { + ctx, cancel := context.WithCancelCause(context.Background()) + defer cancel(nil) + fakeAPI(t, func(_ http.ResponseWriter, r *http.Request) { + cancel(&signaled{at: time.Now()}) + <-r.Context().Done() + }) + t.Setenv("SERPAPI_KEY", "envkey") + + res := executeContext(t, ctx, "", search...) + + wantError(t, res, 130, "interrupted", "Stopped before the response arrived") +} diff --git a/pkg/cmd/hint.go b/pkg/cmd/hint.go index 3ca433d..671feab 100644 --- a/pkg/cmd/hint.go +++ b/pkg/cmd/hint.go @@ -112,13 +112,60 @@ func warnRepeated(cmd *cobra.Command, parsed []params.Param) { // archiveHint is the next step for a large stored search. func archiveHint(string) string { return "add --jq '.KEY'; reading the archive is free" } -// searchHint is the next step for a large search result: re-reading the -// stored search is free, and so is the same search within the cache hour. -func searchHint(id string) string { - if id != "" { - return fmt.Sprintf("read the stored search for free: serpapi archive %s --jq '.KEY'", id) +// searchHint returns the next step for a large result of the search sent +// with p: re-reading the stored search is free, and so is the same search +// within the cache hour, unless no_cache asked for a new one. ZeroTrace +// stores nothing and skips the cache. +func searchHint(p map[string]string) func(id string) string { + return func(id string) string { + zeroTrace, noCache := apiBool(p, "zero_trace"), apiBool(p, "no_cache") + if id != "" && zeroTrace == paramOff { + return fmt.Sprintf("read the stored search for free: serpapi archive %s --jq '.KEY'", id) + } + hint := "repeat the command with --jq '.KEY'" + switch { + case zeroTrace == paramOn: + hint += "; zero_trace=" + p["zero_trace"] + " stores nothing and makes the repeat a new, counted search" + case noCache == paramOn: + hint += "; no_cache=" + p["no_cache"] + " makes the repeat a new, counted search" + case zeroTrace == paramUnsure: + hint += "; with zero_trace=" + p["zero_trace"] + " the repeat may be a new, counted search" + case noCache == paramUnsure: + hint += "; with no_cache=" + p["no_cache"] + " the repeat may be a new, counted search" + default: + return hint + ": the same search within an hour comes from the cache" + } + if allPagesFlag { + hint += " for each page" + } + return hint + } +} + +type paramBool int + +const ( + paramOff paramBool = iota + paramOn + paramUnsure +) + +// apiBool reads a boolean parameter as the API does, through Mongoid 7.5's +// Boolean.evolve: its true words, in any case, are on, and anything else is +// off. Mongoid 8 keeps any other word as a string, which Ruby takes as true, +// so only its false words are sure to stay off. +func apiBool(p map[string]string, key string) paramBool { + v, ok := p[key] + if !ok { + return paramOff + } + switch strings.ToLower(v) { + case "true", "t", "yes", "y", "on", "1", "1.0": + return paramOn + case "false", "f", "no", "n", "off", "0", "0.0": + return paramOff } - return "repeat the command with --jq '.KEY': the same search within an hour comes from the cache" + return paramUnsure } func sizeText(n int) string { diff --git a/pkg/cmd/hint_test.go b/pkg/cmd/hint_test.go index daa1264..28d1314 100644 --- a/pkg/cmd/hint_test.go +++ b/pkg/cmd/hint_test.go @@ -28,7 +28,7 @@ func TestLargeResultHintListsTwelveKeysLargestFirst(t *testing.T) { cmd := &cobra.Command{} cmd.SetOut(&stdout) cmd.SetErr(&stderr) - hintLargeJSON(cmd, raw, searchHint) + hintLargeJSON(cmd, raw, searchHint(nil)) var got struct { Warning struct { diff --git a/pkg/cmd/login.go b/pkg/cmd/login.go index 648c613..2f7edc6 100644 --- a/pkg/cmd/login.go +++ b/pkg/cmd/login.go @@ -2,6 +2,7 @@ package cmd import ( "bufio" + "bytes" "context" "encoding/json" "errors" @@ -67,30 +68,45 @@ func runLogin(cmd *cobra.Command, _ []string) error { } // readAPIKey prompts on a terminal and otherwise reads the first line of -// stdin. It returns "" and no error when the user keeps the saved key. +// stdin. It returns "" and no error when the user keeps the saved key. Ctrl-C +// or SIGTERM stops it at once with an interrupted error. func readAPIKey(cmd *cobra.Command) (string, error) { + ctx := cmd.Context() stderr := cmd.ErrOrStderr() var key string if f, ok := cmd.InOrStdin().(*os.File); ok && term.IsTerminal(int(f.Fd())) { if saved, err := config.LoadAPIKey(); err == nil && saved != "" { - if email, err := accountEmail(cmd.Context(), saved); err == nil { + if email, err := accountEmail(ctx, saved); err == nil { fmt.Fprintf(stderr, "Already logged in as %s. Replace the saved key? [y/N] ", email) - line, _ := bufio.NewReader(f).ReadString('\n') + line, err := readInterruptibly(ctx, func() (string, error) { return bufio.NewReader(f).ReadString('\n') }, + "Stopped at the prompt; the saved key is unchanged") + if isInterrupted(err) { + fmt.Fprintln(stderr) + return "", err + } if !strings.EqualFold(strings.TrimSpace(line), "y") { fmt.Fprintln(stderr, "Kept the saved key.") return "", nil } } } + const stopped = "Stopped at the API key prompt; nothing was saved" + if ctx.Err() != nil { + return "", &clierrors.InterruptedError{Message: stopped} + } fmt.Fprint(stderr, "SerpApi API key (input hidden): ") - b, err := term.ReadPassword(int(f.Fd())) - fmt.Fprintln(stderr) + line, err := readHidden(ctx, f, stderr, stopped) if err != nil { - return "", &clierrors.UsageError{Message: "Failed to read the API key: " + err.Error()} + return "", err } - key = string(b) + key = line } else { - line, err := bufio.NewReader(cmd.InOrStdin()).ReadString('\n') + in := bufio.NewReader(cmd.InOrStdin()) + line, err := readInterruptibly(ctx, func() (string, error) { return in.ReadString('\n') }, + "Stopped while waiting for the API key on stdin; nothing was saved") + if isInterrupted(err) { + return "", err + } if err != nil && !errors.Is(err, io.EOF) { return "", &clierrors.UsageError{Message: "Failed to read the API key: " + err.Error()} } @@ -102,6 +118,88 @@ func readAPIKey(cmd *cobra.Command) (string, error) { return key, nil } +// readHidden reads a line from the terminal f without showing it. Raw mode is +// set and undone here, not in the goroutine that reads, so a read that a +// signal leaves running can't turn echo off again after it was restored. In +// raw mode Ctrl-C is a key rather than a signal; it stops the run as the +// signal would, even after Enter in the same read. Ctrl-D gives no key. +func readHidden(ctx context.Context, f *os.File, stderr io.Writer, stopped string) (string, error) { + fd := int(f.Fd()) + state, err := term.MakeRaw(fd) + if err != nil { + return "", &clierrors.UsageError{Message: "Failed to read the API key: " + err.Error()} + } + in := &ctrlCWatch{r: f} + t := term.NewTerminal(struct { + io.Reader + io.Writer + }{in, stderr}, "") + line, err := readInterruptibly(ctx, func() (string, error) { return t.ReadPassword("") }, stopped) + _ = term.Restore(fd, state) + if !isInterrupted(err) && in.ctrlC { + stopRun(ctx) + err = &clierrors.InterruptedError{Message: stopped} + } + if errors.Is(err, term.ErrPasteIndicator) { + err = nil + } + if err != nil { + fmt.Fprintln(stderr) + } + switch { + case err == nil: + return line, nil + case isInterrupted(err): + return "", err + case errors.Is(err, io.EOF): + return "", nil + } + return "", &clierrors.UsageError{Message: "Failed to read the API key: " + err.Error()} +} + +// ctrlCWatch notes a Ctrl-C, which raw mode passes on as a key. Only the +// reading goroutine writes ctrlC; it is read after that goroutine returned. +type ctrlCWatch struct { + r io.Reader + ctrlC bool +} + +func (w *ctrlCWatch) Read(p []byte) (int, error) { + n, err := w.r.Read(p) + if bytes.IndexByte(p[:n], 3) >= 0 { + w.ctrlC = true + } + return n, err +} + +// readInterruptibly returns what read returns, or an interrupted error with +// the stopped message as soon as ctx is canceled. A read from a terminal or a +// pipe can't be canceled itself; it ends with the process. read outlives an +// interrupted call, so it must not touch the command. +func readInterruptibly[T any](ctx context.Context, read func() (T, error), stopped string) (T, error) { + type result struct { + v T + err error + } + done := make(chan result, 1) + go func() { + v, err := read() + done <- result{v, err} + }() + select { + case r := <-done: + return r.v, r.err + case <-ctx.Done(): + var zero T + return zero, &clierrors.InterruptedError{Message: stopped} + } +} + +func isInterrupted(err error) bool { + var interrupted *clierrors.InterruptedError + return errors.As(err, &interrupted) +} + func accountEmail(ctx context.Context, apiKey string) (string, error) { raw, err := newClient(apiKey).Account(ctx) if err != nil { diff --git a/pkg/cmd/root.go b/pkg/cmd/root.go index 373697b..c3e980b 100644 --- a/pkg/cmd/root.go +++ b/pkg/cmd/root.go @@ -79,16 +79,17 @@ limited or out of searches; 5xx SerpApi failed. No status means no usable answer: a network_error, such as a timeout, or an unreadable response. Requests time out after 60s: --timeout or SERPAPI_TIMEOUT sets seconds, 0 waits indefinitely. -Exit 130, code interrupted: stopped by Ctrl-C or SIGTERM, after printing what -had arrived. +Exit 130, code interrupted: Ctrl-C or SIGTERM stopped it before it finished, +after printing what had arrived. Cost: each successful search counts against your monthly searches, including each page of --all-pages; cached, errored and failed ones don't, though errored ones count toward the hourly limit (https://serpapi.com/pricing). An identical search, --fields included, repeated within an hour comes from the cache. no_cache=true forces a new, counted search; cached results are at most an -hour old, so add it only when that is too old. account, locations and archive -are free.`, +hour old, so add it only when that is too old. zero_trace=true (Enterprise) +stores nothing, so it skips the cache too and leaves no search for archive to +read. account, locations and archive are free.`, Example: ` serpapi search engine=google q=coffee --fields "organic_results[0:3]" serpapi search engine=google coffee shops --jq ".organic_results[].link" serpapi search engine=google q=coffee output=md @@ -111,21 +112,60 @@ are free.`, // signal before the process exits anyway. var interruptGrace = 3 * time.Second +// signaled is the cause Execute cancels the command's context with, so code +// that sees the cancellation knows when the signal came. +type signaled struct{ at time.Time } + +func (*signaled) Error() string { return "stopped by Ctrl-C or SIGTERM" } + +// Unwrap keeps errors.Is(err, context.Canceled) true for a request it +// stopped: net/http reports the context's cause, not context.Canceled. +func (*signaled) Unwrap() error { return context.Canceled } + +// exitTime is when the process exits after the first signal: interruptGrace +// after it. A context canceled without a signal time counts from now. +func exitTime(ctx context.Context) time.Time { + var s *signaled + if errors.As(context.Cause(ctx), &s) { + return s.at.Add(interruptGrace) + } + return time.Now().Add(interruptGrace) +} + +// stopRunKey holds, in the command's context, the func that cancels it as a +// signal does. login calls it for a Ctrl-C that raw mode turns into a key. +type stopRunKey struct{} + +func stopRun(ctx context.Context) { + if stop, ok := ctx.Value(stopRunKey{}).(func()); ok { + stop() + } +} + // Execute runs the root command. The first Ctrl-C or SIGTERM cancels the // command's context so it can print what it has; a second one kills it. func Execute() { - ctx, stop := signal.NotifyContext(context.Background(), os.Interrupt, syscall.SIGTERM) + ctx, cancel := context.WithCancelCause(context.Background()) + stop := func() { cancel(&signaled{at: time.Now()}) } + sigs := make(chan os.Signal, 1) + signal.Notify(sigs, os.Interrupt, syscall.SIGTERM) + go func() { + <-sigs + stop() + }() + restoreDefault := func() { signal.Stop(sigs) } done := make(chan struct{}) - go exitAfterGrace(ctx, done, stop, rootCmd.ErrOrStderr(), os.Exit) - code := run(ctx) + go exitAfterGrace(ctx, done, restoreDefault, rootCmd.ErrOrStderr(), os.Exit) + code := run(context.WithValue(ctx, stopRunKey{}, stop)) close(done) - stop() + restoreDefault() os.Exit(code) } // exitAfterGrace waits for the first signal, restores the default handling // so that a second one kills the process, and exits 130 if the command is -// still running interruptGrace later, as it is when blocked on a full pipe. +// still running interruptGrace after the signal, as it is when blocked on a +// full pipe. func exitAfterGrace(ctx context.Context, done <-chan struct{}, stop func(), stderr io.Writer, exit func(int)) { select { case <-done: @@ -135,7 +175,7 @@ func exitAfterGrace(ctx context.Context, done <-chan struct{}, stop func(), stde stop() select { case <-done: - case <-time.After(interruptGrace): + case <-time.After(time.Until(exitTime(ctx))): // stderr may be the full pipe, so the message gets a moment, not a promise. go clierrors.FprintError(stderr, &clierrors.InterruptedError{ Message: fmt.Sprintf("Stopped %s after the signal; the output may be cut off", interruptGrace), @@ -146,20 +186,45 @@ func exitAfterGrace(ctx context.Context, done <-chan struct{}, stop func(), stde } // run executes the root command, reports any error on its stderr as JSON and -// returns the exit code. A command stopped by canceling ctx exits 130. +// returns the exit code. A request stopped by canceling ctx exits 130; an +// error from what had already arrived, such as a failing --jq, keeps its own. func run(ctx context.Context) int { + initHelpCommand() err := rootCmd.ExecuteContext(ctx) if err == nil { return 0 } - var interrupted *clierrors.InterruptedError - if ctx.Err() != nil && !errors.As(err, &interrupted) { + if ctx.Err() != nil && errors.Is(err, context.Canceled) { err = &clierrors.InterruptedError{Message: "Stopped before the response arrived"} } clierrors.FprintError(rootCmd.ErrOrStderr(), err) return clierrors.ExitCode(err) } +// initHelpCommand keeps cobra's help command, with its text and completion, +// but makes an unknown topic a usage error; cobra prints the usage and exits 0. +// It runs once every command has been added, which cobra needs. +func initHelpCommand() { + rootCmd.InitDefaultHelpCmd() + if help, _, err := rootCmd.Find([]string{"help"}); err == nil && help != rootCmd { + help.RunE = runHelp + } +} + +func runHelp(c *cobra.Command, args []string) error { + target, _, err := c.Root().Find(args) + if len(args) > 0 && (err != nil || target == c.Root()) { + msg := fmt.Sprintf("Unknown help topic %q", strings.Join(args, " ")) + if s := c.Root().SuggestionsFor(args[0]); len(s) > 0 { + return &clierrors.UsageError{Message: msg + "; did you mean " + strings.Join(s, " or ") + "?"} + } + return &clierrors.UsageError{Message: msg + "; see 'serpapi --help'"} + } + target.InitDefaultHelpFlag() + target.InitDefaultVersionFlag() + return target.Help() +} + func init() { pf := rootCmd.PersistentFlags() pf.StringVar(&apiKeyFlag, "api-key", "", "SerpApi API key; prefer SERPAPI_KEY, since flags show up in ps and shell history") @@ -318,12 +383,26 @@ func handleOutput(cmd *cobra.Command, raw []byte, next func(id string) string) e if err := dec.Decode(&v); err != nil { return &clierrors.APIError{Message: "Failed to parse response: " + err.Error()} } - return printJQ(cmd, v) + ctx, cancel := printContext(cmd.Context()) + defer cancel() + return printJQ(ctx, cmd, v) +} + +// printContext is the context for printing a result that has arrived. After +// a signal the search was billed, so the result still prints, with --jq +// stopped a quarter of interruptGrace before the process exits: that leaves +// it time to flush and report what it printed. +func printContext(ctx context.Context) (context.Context, context.CancelFunc) { + if ctx == nil || ctx.Err() == nil { + return ctx, func() {} + } + return context.WithDeadline(context.WithoutCancel(ctx), exitTime(ctx).Add(-interruptGrace/4)) } -// printJQ runs --jq on v and prints each result as it is produced. A filter -// that prints only null gets a warning listing the keys v has. -func printJQ(cmd *cobra.Command, v any) error { +// printJQ runs --jq on v until ctx ends, printing each result as it is +// produced. A filter that prints only null gets a warning listing the keys v +// has. +func printJQ(ctx context.Context, cmd *cobra.Command, v any) error { code, err := jq.Compile(jqFlag) if err != nil { return &clierrors.UsageError{Message: err.Error()} @@ -333,7 +412,6 @@ func printJQ(cmd *cobra.Command, v any) error { bw := bufio.NewWriter(w) var writeErr error printed, nulls := 0, 0 - ctx := cmd.Context() if ctx == nil { ctx = context.Background() } diff --git a/pkg/cmd/search.go b/pkg/cmd/search.go index 6514da6..b080502 100644 --- a/pkg/cmd/search.go +++ b/pkg/cmd/search.go @@ -3,6 +3,7 @@ package cmd import ( "bufio" "bytes" + "context" "encoding/json" "errors" "fmt" @@ -167,7 +168,7 @@ func sendSearch(cmd *cobra.Command, client *api.Client, p map[string]string, raw if restricted && isEmptyObject(result) { warnNothingMatched(cmd) } - return handleOutput(cmd, result, searchHint) + return handleOutput(cmd, result, searchHint(p)) } // explainMissingKey adds to a 401 for a search sent without a key how to give @@ -290,7 +291,10 @@ func searchAllPages(cmd *cobra.Command, client *api.Client, first map[string]str warnNothingMatched(cmd) } - if err := printMerged(cmd, merged); err != nil { + // Pages fetched before Ctrl-C were billed, so --jq still runs on them. + printCtx, cancel := printContext(ctx) + defer cancel() + if err := printMerged(printCtx, cmd, merged, searchHint(first)("")); err != nil { return err } if pageErr == nil { @@ -310,12 +314,13 @@ func searchAllPages(cmd *cobra.Command, client *api.Client, first map[string]str return annotatePageError(pageErr, pages) } -// printMerged prints the merged pages. --jq runs on them directly, and a -// pipe gets them encoded as they are written, so no encoded copy of every -// page is ever held in memory. -func printMerged(cmd *cobra.Command, merged map[string]any) error { +// printMerged prints the merged pages. --jq runs on them directly until ctx +// ends, and a pipe gets them encoded as they are written, so no encoded copy +// of every page is ever held in memory. next is the large-result hint's next +// step. +func printMerged(ctx context.Context, cmd *cobra.Command, merged map[string]any, next string) error { if jqFlag != "" { - return printJQ(cmd, merged) + return printJQ(ctx, cmd, merged) } w := cmd.OutOrStdout() if output.IsTerminal(w) { @@ -334,7 +339,7 @@ func printMerged(cmd *cobra.Command, merged map[string]any) error { return &clierrors.APIError{Message: "Failed to encode result: " + err.Error()} } sortKeySizes(keys) - hintLargeResult(cmd, int(size), keys, searchHint("")) + hintLargeResult(cmd, int(size), keys, next) bw := bufio.NewWriterSize(w, 64<<10) if _, err := output.WriteObject(bw, merged, nil); err != nil { return outputError(err) diff --git a/pkg/config/config.go b/pkg/config/config.go index 8d8c306..7e7709a 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -7,6 +7,7 @@ import ( "io/fs" "os" "path/filepath" + "runtime" "strings" "github.com/BurntSushi/toml" @@ -17,21 +18,27 @@ type configFile struct { APIKey string `toml:"api_key"` } -// Dir returns the directory where the CLI stores its config. +// Dir returns the directory where the CLI stores its config, or "" when +// neither the user config directory nor the home directory is an absolute +// path: a relative one would put the key in, or take it from, wherever the +// command runs. func Dir() string { - if dir, err := os.UserConfigDir(); err == nil { + if dir, err := os.UserConfigDir(); err == nil && filepath.IsAbs(dir) { return filepath.Join(dir, "serpapi") } - home, err := os.UserHomeDir() - if err != nil { - return filepath.Join(".config", "serpapi") + if home, err := os.UserHomeDir(); err == nil && filepath.IsAbs(home) { + return filepath.Join(home, ".config", "serpapi") } - return filepath.Join(home, ".config", "serpapi") + return "" } -// Path returns the full path to the config file. +// Path returns the full path to the config file, or "" when Dir is "". func Path() string { - return filepath.Join(Dir(), "config.toml") + dir := Dir() + if dir == "" { + return "" + } + return filepath.Join(dir, "config.toml") } // ErrNoAPIKey is returned by ResolveAPIKey when no source has a key. @@ -43,7 +50,11 @@ var ErrNoAPIKey = &clierrors.UsageError{ // is none. A file that can't be read or parsed is an error naming the file // and line; it never quotes the file, which holds the key. func LoadAPIKey() (string, error) { - data, err := os.ReadFile(Path()) + path := Path() + if path == "" { + return "", nil + } + data, err := os.ReadFile(path) if errors.Is(err, fs.ErrNotExist) { return "", nil } @@ -64,10 +75,20 @@ func LoadAPIKey() (string, error) { return strings.TrimSpace(cfg.APIKey), nil } -// SaveConfig writes the API key to the config file atomically. +// SaveConfig writes the API key to the config file atomically, through a +// temporary file that only this call creates, so a file left or planted next +// to the config can't widen its permissions or receive the key. func SaveConfig(apiKey string) error { dir := Dir() - if err := mkdirSecure(dir); err != nil { + if dir == "" { + home := "HOME" + if runtime.GOOS == "windows" { + home = "%AppData%" + } + return &clierrors.UsageError{Message: "No directory to save the API key in: " + home + + " is not set to an absolute path; set it, or pass the key with SERPAPI_KEY instead of saving it"} + } + if err := os.MkdirAll(dir, 0o700); err != nil { return &clierrors.UsageError{Message: "Failed to create config dir: " + err.Error()} } @@ -77,23 +98,37 @@ func SaveConfig(apiKey string) error { return &clierrors.UsageError{Message: err.Error()} } - finalPath := Path() - tmpPath := finalPath + ".tmp" - - if err := writeFileSecure(tmpPath, content); err != nil { - return &clierrors.UsageError{Message: err.Error()} + // CreateTemp opens a new file with O_EXCL and mode 0600. + tmp, err := os.CreateTemp(dir, "config.toml.*.tmp") + if err != nil { + return &clierrors.UsageError{Message: "Failed to save config: " + err.Error()} + } + if err := writeSynced(tmp, content); err != nil { + os.Remove(tmp.Name()) + return &clierrors.UsageError{Message: "Failed to save config: " + err.Error()} } // os.Rename replaces an existing file on Windows too (MoveFileEx with // MOVEFILE_REPLACE_EXISTING), so the old key stays until the new one lands. - if err := os.Rename(tmpPath, finalPath); err != nil { - os.Remove(tmpPath) + if err := os.Rename(tmp.Name(), Path()); err != nil { + os.Remove(tmp.Name()) return &clierrors.UsageError{Message: "Failed to save config: " + err.Error()} } return nil } +func writeSynced(f *os.File, data []byte) error { + _, err := f.Write(data) + if err == nil { + err = f.Sync() + } + if closeErr := f.Close(); err == nil { + err = closeErr + } + return err +} + func tomlMarshal(cfg configFile) ([]byte, error) { buf := new(bytes.Buffer) enc := toml.NewEncoder(buf) diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go index 664163f..cbf753d 100644 --- a/pkg/config/config_test.go +++ b/pkg/config/config_test.go @@ -5,7 +5,9 @@ import ( "os" "path/filepath" "runtime" + "slices" "strings" + "sync" "testing" clierrors "github.com/serpapi/serpapi-cli/pkg/errors" @@ -68,7 +70,151 @@ func TestSaveConfigRoundTripAndOverwrite(t *testing.T) { if runtime.GOOS != "windows" && info.Mode().Perm() != 0o600 { t.Errorf("config mode = %v, want 0600", info.Mode().Perm()) } - if _, err := os.Stat(Path() + ".tmp"); !os.IsNotExist(err) { - t.Errorf("temporary file left behind: %v", err) + wantNoTempFiles(t) +} + +func wantNoTempFiles(t *testing.T) { + t.Helper() + left, err := filepath.Glob(filepath.Join(Dir(), "config.toml*tmp")) + if err != nil { + t.Fatal(err) + } + if len(left) > 0 { + t.Errorf("temporary files left behind: %v", left) + } +} + +// A file someone else left or planted at the old fixed temporary name must +// not decide the saved key's permissions or where the key is written. +func TestSaveConfigIgnoresFilesAtTheOldTempName(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("file modes and symlinks differ on Windows") + } + isolate(t) + if err := os.MkdirAll(Dir(), 0o700); err != nil { + t.Fatal(err) + } + stale := Path() + ".tmp" + if err := os.WriteFile(stale, nil, 0o644); err != nil { + t.Fatal(err) + } + if err := os.Chmod(stale, 0o644); err != nil { + t.Fatal(err) + } + + if err := SaveConfig("stale-key"); err != nil { + t.Fatal(err) + } + if info, err := os.Stat(Path()); err != nil || info.Mode().Perm() != 0o600 { + t.Fatalf("config = %v, %v; want mode 0600 despite a 0644 %s", info, err, filepath.Base(stale)) + } + + victim := filepath.Join(t.TempDir(), "victim") + if err := os.WriteFile(victim, []byte("untouched\n"), 0o644); err != nil { + t.Fatal(err) + } + if err := os.Remove(stale); err != nil { + t.Fatal(err) + } + if err := os.Symlink(victim, stale); err != nil { + t.Fatal(err) + } + + if err := SaveConfig("symlink-key"); err != nil { + t.Fatal(err) + } + if b, _ := os.ReadFile(victim); string(b) != "untouched\n" { + t.Errorf("symlink target = %q, want it untouched", b) + } + if info, err := os.Lstat(Path()); err != nil || !info.Mode().IsRegular() { + t.Errorf("config = %v, %v; want a regular file", info, err) + } + if got, err := ResolveAPIKey(""); err != nil || got != "symlink-key" { + t.Errorf("ResolveAPIKey = %q, %v; want symlink-key", got, err) + } +} + +func TestSaveConfigConcurrently(t *testing.T) { + isolate(t) + keys := []string{"key-0", "key-1", "key-2", "key-3", "key-4", "key-5", "key-6", "key-7"} + errs := make(chan error, len(keys)) + var wg sync.WaitGroup + for _, key := range keys { + wg.Add(1) + go func() { + defer wg.Done() + errs <- SaveConfig(key) + }() + } + wg.Wait() + close(errs) + for err := range errs { + if err != nil { + t.Errorf("SaveConfig: %v", err) + } + } + got, err := ResolveAPIKey("") + if err != nil || !slices.Contains(keys, got) { + t.Errorf("ResolveAPIKey = %q, %v; want one of the saved keys", got, err) + } + wantNoTempFiles(t) +} + +// Without HOME there is no config directory, and a relative one would put the +// key in, or take it from, whatever directory the command runs in. +// Without an absolute home directory, the key is neither read from nor saved +// to a directory relative to wherever the command runs. +func TestNoConfigDirWithoutAnAbsoluteHome(t *testing.T) { + for _, home := range []string{"", ".", filepath.Join("relative", "home")} { + for _, name := range []string{"HOME", "XDG_CONFIG_HOME", "AppData", "USERPROFILE"} { + t.Setenv(name, home) + } + cwd := t.TempDir() + t.Chdir(cwd) + var planted []string + for _, dir := range []string{filepath.Join(home, ".config"), filepath.Join(home, "Library", "Application Support"), home} { + path := filepath.Join(dir, "serpapi", "config.toml") + if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(path, []byte(`api_key = "planted-key"`+"\n"), 0o600); err != nil { + t.Fatal(err) + } + planted = append(planted, path) + } + before := filesUnder(t, cwd) + + if Dir() != "" || Path() != "" { + t.Errorf("home %q: Dir() = %q, Path() = %q; want both empty", home, Dir(), Path()) + } + if key, err := ResolveAPIKey(""); err != ErrNoAPIKey { + t.Errorf("home %q: ResolveAPIKey = %q, %v; want ErrNoAPIKey, not a key under the current directory", home, key, err) + } + err := SaveConfig("new-key") + var usageErr *clierrors.UsageError + if !errors.As(err, &usageErr) || !strings.Contains(usageErr.Message, "absolute") || !strings.Contains(usageErr.Message, "SERPAPI_KEY") { + t.Errorf("home %q: SaveConfig = %v; want a UsageError that asks for an absolute home or SERPAPI_KEY", home, err) + } + for _, path := range planted { + if b, _ := os.ReadFile(path); !strings.Contains(string(b), "planted-key") { + t.Errorf("home %q: %s was overwritten: %q", home, path, b) + } + } + if after := filesUnder(t, cwd); !slices.Equal(before, after) { + t.Errorf("home %q: files under the current directory changed from %v to %v", home, before, after) + } + } +} + +func filesUnder(t *testing.T, root string) []string { + t.Helper() + var files []string + err := filepath.WalkDir(root, func(path string, _ os.DirEntry, err error) error { + files = append(files, path) + return err + }) + if err != nil { + t.Fatal(err) } + return files } diff --git a/pkg/config/config_unix.go b/pkg/config/config_unix.go deleted file mode 100644 index e7c72ef..0000000 --- a/pkg/config/config_unix.go +++ /dev/null @@ -1,33 +0,0 @@ -//go:build !windows - -package config - -import ( - "fmt" - "os" -) - -func mkdirSecure(path string) error { - return os.MkdirAll(path, 0700) -} - -func writeFileSecure(path string, data []byte) error { - f, err := os.OpenFile(path, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0600) - if err != nil { - return err - } - n, err := f.Write(data) - if err != nil { - f.Close() - return err - } - if n != len(data) { - f.Close() - return fmt.Errorf("incomplete write: wrote %d of %d bytes", n, len(data)) - } - if err := f.Sync(); err != nil { - f.Close() - return err - } - return f.Close() -} diff --git a/pkg/config/config_windows.go b/pkg/config/config_windows.go deleted file mode 100644 index 2be07cc..0000000 --- a/pkg/config/config_windows.go +++ /dev/null @@ -1,13 +0,0 @@ -//go:build windows - -package config - -import "os" - -func mkdirSecure(path string) error { - return os.MkdirAll(path, 0700) -} - -func writeFileSecure(path string, data []byte) error { - return os.WriteFile(path, data, 0600) -}