From de643c4f9e909ca5c9f57a9df7c2c44c3f0ed412 Mon Sep 17 00:00:00 2001 From: Mladen Jablanovic Date: Mon, 24 Aug 2026 10:24:13 +0200 Subject: [PATCH 1/2] fix(CLI): restore empty file on zero-match tag filter, fix cache ordering PR #1085 made copyToDestination fail when the API returns an empty body, which happens whenever a filter (e.g. --tags) matches zero translations. That's a valid empty export, not a failed download, so treat it as one: write an empty file instead of erroring. Also fix the cache write ordering in downloadSynchronously and PullParallel: the ETag/Last-Modified was being persisted before copyToDestination ran, so a failed write could still poison the cache into reporting 304 Not Modified for a locale that was never written. Co-Authored-By: Claude Sonnet 5 --- clients/cli/cmd/internal/pull.go | 27 +++++--- .../cli/cmd/internal/pull_parallel_test.go | 66 +++++++++++++++++++ clients/cli/spec/pull_spec.rb | 56 ++++++++++++++++ 3 files changed, 140 insertions(+), 9 deletions(-) diff --git a/clients/cli/cmd/internal/pull.go b/clients/cli/cmd/internal/pull.go index b609eeea2..f8810157f 100644 --- a/clients/cli/cmd/internal/pull.go +++ b/clients/cli/cmd/internal/pull.go @@ -273,16 +273,16 @@ func (target *Target) PullParallel(client *phrase.APIClient, cache *DownloadCach return dlErr } - if cache != nil { - updateCache(cache, cacheKey, response) - } - if err := copyToDestination(file, lf.Path); err != nil { err = fmt.Errorf("%s for %s", err, lf.Path) results[i] = downloadResult{errMsg: err.Error()} return err } + if cache != nil { + updateCache(cache, cacheKey, response) + } + results[i] = downloadResult{ message: lf.Message(), path: lf.RelPath(), @@ -445,23 +445,32 @@ func (target *Target) downloadSynchronously(client *phrase.APIClient, localeFile } } + if err := copyToDestination(file, localeFile.Path); err != nil { + return err + } + if cache != nil { updateCache(cache, cacheKey, response) } - return copyToDestination(file, localeFile.Path) + return nil } func copyToDestination(file *os.File, path string) error { - if file == nil { - return fmt.Errorf("no content to write to %s", path) - } - defer file.Close() destFile, err := os.Create(path) if err != nil { return err } defer destFile.Close() + + // The API returns a 200 with an empty body when a filter (e.g. --tags) + // matches no translations; decode() then leaves file nil with no error. + // That's a valid empty export, not a failed download, so write an empty + // file rather than treating it as an error. + if file == nil { + return nil + } + defer file.Close() _, err = io.Copy(destFile, file) return err } diff --git a/clients/cli/cmd/internal/pull_parallel_test.go b/clients/cli/cmd/internal/pull_parallel_test.go index 51dee7ff4..75a48c220 100644 --- a/clients/cli/cmd/internal/pull_parallel_test.go +++ b/clients/cli/cmd/internal/pull_parallel_test.go @@ -1,6 +1,8 @@ package internal import ( + "os" + "path/filepath" "testing" ) @@ -44,3 +46,67 @@ func TestBuildDownloadOpts_TagHandling(t *testing.T) { t.Errorf("expected tag to be empty string, got %q", opts.Tag.Value()) } } + +// A nil *os.File represents a 200 response with an empty body, which the API +// returns when a filter (e.g. --tags) matches no translations. copyToDestination +// must write an empty file for this case rather than failing, since it's a valid +// (if unusual) export result, not a failed download. +func TestCopyToDestination_NilFileWritesEmptyFile(t *testing.T) { + path := filepath.Join(t.TempDir(), "en.json") + + if err := copyToDestination(nil, path); err != nil { + t.Fatalf("unexpected error: %v", err) + } + + data, err := os.ReadFile(path) + if err != nil { + t.Fatalf("expected file to be created at %s: %v", path, err) + } + if len(data) != 0 { + t.Errorf("expected empty file, got %q", data) + } +} + +func TestCopyToDestination_NilFileTruncatesExistingContent(t *testing.T) { + path := filepath.Join(t.TempDir(), "en.json") + if err := os.WriteFile(path, []byte(`{"hello":"world"}`), 0o644); err != nil { + t.Fatalf("failed to seed existing file: %v", err) + } + + if err := copyToDestination(nil, path); err != nil { + t.Fatalf("unexpected error: %v", err) + } + + data, err := os.ReadFile(path) + if err != nil { + t.Fatalf("failed to read file: %v", err) + } + if len(data) != 0 { + t.Errorf("expected previously downloaded content to be replaced with empty file, got %q", data) + } +} + +func TestCopyToDestination_CopiesFileContent(t *testing.T) { + srcPath := filepath.Join(t.TempDir(), "src.json") + if err := os.WriteFile(srcPath, []byte(`{"hello":"world"}`), 0o644); err != nil { + t.Fatalf("failed to write source file: %v", err) + } + src, err := os.Open(srcPath) + if err != nil { + t.Fatalf("failed to open source file: %v", err) + } + defer src.Close() + + destPath := filepath.Join(t.TempDir(), "dest.json") + if err := copyToDestination(src, destPath); err != nil { + t.Fatalf("unexpected error: %v", err) + } + + data, err := os.ReadFile(destPath) + if err != nil { + t.Fatalf("failed to read dest file: %v", err) + } + if string(data) != `{"hello":"world"}` { + t.Errorf("expected copied content, got %q", data) + } +} diff --git a/clients/cli/spec/pull_spec.rb b/clients/cli/spec/pull_spec.rb index 0ae31bf0a..63c9efb08 100644 --- a/clients/cli/spec/pull_spec.rb +++ b/clients/cli/spec/pull_spec.rb @@ -248,6 +248,62 @@ end end + describe "pull with tag filter matching no translations" do + let(:config) do + <<~YAML + phrase: + host: #{ENV.fetch("BASE_URL")} + project_id: "#{project_id}" + access_token: "#{token}" + pull: + targets: + - file: "#{@tmpdir}/locales/.yml" + params: + file_format: yml + tags: "nonexistent-tag" + YAML + end + + before do + # The API responds with 200 and an empty body when a tag filter + # matches zero translations - this is not an error. + mock_set!("GET", "/projects/#{project_id}/locales/#{locale_en_id}/download", + status: 200, + body: "", + headers: { "content-type" => "application/x-yaml" } + ) + mock_set!("GET", "/projects/#{project_id}/locales/#{locale_de_id}/download", + status: 200, + body: "", + headers: { "content-type" => "application/x-yaml" } + ) + end + + it "succeeds and writes an empty file instead of failing" do + r = run_cli("pull", config: config) + + expect(r[:exit_code]).to eq(0) + + en_file_path = File.join(@tmpdir, "locales", "en.yml") + de_file_path = File.join(@tmpdir, "locales", "de.yml") + + expect(File.exist?(en_file_path)).to be true + expect(File.exist?(de_file_path)).to be true + expect(File.read(en_file_path)).to eq("") + expect(File.read(de_file_path)).to eq("") + end + + it "succeeds with --parallel too" do + r = run_cli("pull", "--parallel", config: config) + + expect(r[:exit_code]).to eq(0) + + en_file_path = File.join(@tmpdir, "locales", "en.yml") + expect(File.exist?(en_file_path)).to be true + expect(File.read(en_file_path)).to eq("") + end + end + describe "pull with locale_mapping" do let(:config) do <<~YAML From 7f1791b28e5330a8d8f39d37aa942884293e4cd9 Mon Sep 17 00:00:00 2001 From: Mladen Jablanovic Date: Mon, 24 Aug 2026 11:41:28 +0200 Subject: [PATCH 2/2] ci(CLI): quiet noisy generator output, show rspec test names Redirect the openapi-generator/npm output from the generate step to a log file that's only dumped on failure, and switch rspec to the documentation formatter so CI logs show test names instead of dots. --- .github/workflows/test-cli.yml | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/.github/workflows/test-cli.yml b/.github/workflows/test-cli.yml index 88ef6f393..49946d116 100644 --- a/.github/workflows/test-cli.yml +++ b/.github/workflows/test-cli.yml @@ -26,15 +26,20 @@ jobs: uses: actions/setup-go@v2 with: go-version: '1.24.4' - - name: Generate CLI and run unit tests + - name: Generate CLI env: GOPRIVATE: github.com/phrase/phrase-go run: | - npm install - npm run generate.go + { + npm install + npm run generate.go + cd ./clients/cli + go mod edit -replace github.com/phrase/phrase-go/v4=../go + npm run generate.cli + } > generate.log 2>&1 || { cat generate.log; exit 1; } + - name: Build and run unit tests + run: | cd ./clients/cli - go mod edit -replace github.com/phrase/phrase-go/v4=../go - npm run generate.cli go build . go test -v ./... - name: Install Ruby and rspec @@ -46,7 +51,7 @@ jobs: - name: Run integration tests run: | cd ./clients/cli - bundle exec rspec + bundle exec rspec --format documentation - name: License check uses: phrase/actions/lawa-ci@v1 with: