fix(wrangler): suppress log messages when --json flag is used in vectorize commands#12477
fix(wrangler): suppress log messages when --json flag is used in vectorize commands#12477kaigritun wants to merge 4 commits intocloudflare:mainfrom
Conversation
…orize commands When using --json flag on vectorize list, list-metadata-index, and info commands, the output included log messages (e.g. '📋 Listing Vectorize indexes...') that made the stdout invalid JSON, breaking piping to tools like jq. This fix wraps these log messages in a condition to check for !args.json, following the pattern already established in listVectors.ts. Fixes cloudflare#11011
🦋 Changeset detectedLatest commit: 0d6f85f The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Move the json output check before the empty-length check to ensure that when --json flag is used and results are empty, we output [] instead of returning early with a warning message. Fixes review feedback on PR cloudflare#12477
kaigritun
left a comment
There was a problem hiding this comment.
Thanks for the detailed review! I've addressed both issues by moving the args.json check before the empty-length check in both list.ts and listMetadataIndex.ts.
Now when --json flag is used:
- Empty indexes will output
[]instead of showing a warning and returning early - This ensures
wrangler vectorize list --json | jq .will work correctly even with no results
Changes pushed in commit 1dc6a08.
petebacondarwin
left a comment
There was a problem hiding this comment.
This looks good. We'll need some tests to prove it out and a changeset before we can land it.
create-cloudflare
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-editor-shared
@cloudflare/workers-utils
wrangler
commit: |
- Add tests for --json flag with empty results in vectorize list - Add tests for --json flag with empty results in list-metadata-index - Add changeset documenting the patch fix Addresses review feedback from Pete Bacon Darwin on PR cloudflare#12477
|
Thanks Pete! I've added:
Both tests verify that:
Pushed in commit 763cac7. Ready for review! |
There was a problem hiding this comment.
Devin Review found 1 new potential issue.
⚠️ 1 issue in files not directly in the diff
⚠️ listVectors --json with empty results outputs warning instead of clean JSON (packages/wrangler/src/vectorize/listVectors.ts:64-67)
The listVectors.ts handler has the same bug that this PR fixes in list.ts and listMetadataIndex.ts, but it was not fixed here. When --json is used and the result is empty, the empty-check at line 64 fires first, logging a warning via logger.warn and returning — the args.json check at line 69 is never reached.
Root Cause and Impact
In packages/wrangler/src/vectorize/listVectors.ts:64-67, the empty vectors check runs before the JSON output check at lines 69-72:
if (result.vectors.length === 0) {
logger.warn("No vectors found in this index.");
return;
}
if (args.json) {
logger.log(JSON.stringify(result, null, 2));
return;
}When a user runs wrangler vectorize list-vectors my-index --json and the index has no vectors, they get a warning message on stderr and no JSON on stdout, instead of clean JSON output. This is the exact same bug pattern that this PR correctly fixes in list.ts (line 33-36) and listMetadataIndex.ts (line 34-37) by moving the args.json check before the empty check.
Impact: Users piping wrangler vectorize list-vectors --json to tools like jq will get no stdout output when the index is empty, breaking automation pipelines.
View 3 additional findings in Devin Review.
petebacondarwin
left a comment
There was a problem hiding this comment.
I think the latest Devin review issue (that listVectors has the same problem) should be fixed in this PR too.
Also we have a logger.json() helper that we should use.
There was a problem hiding this comment.
Devin Review found 1 new potential issue.
⚠️ 1 issue in files not directly in the diff
⚠️ listVectors --json with empty results outputs warning instead of clean JSON (packages/wrangler/src/vectorize/listVectors.ts:64-67)
The listVectors.ts handler has the same bug that this PR fixes in list.ts and listMetadataIndex.ts, but it was not fixed here. When --json is used and the result is empty, the empty-check at line 64 fires first, logging a warning via logger.warn and returning — the args.json check at line 69 is never reached.
Root Cause and Impact
In packages/wrangler/src/vectorize/listVectors.ts:64-67, the empty vectors check runs before the JSON output check at lines 69-72:
if (result.vectors.length === 0) {
logger.warn("No vectors found in this index.");
return;
}
if (args.json) {
logger.log(JSON.stringify(result, null, 2));
return;
}When a user runs wrangler vectorize list-vectors my-index --json and the index has no vectors, they get a warning message on stderr and no JSON on stdout, instead of clean JSON output. This is the exact same bug pattern that this PR correctly fixes in list.ts (line 33-36) and listMetadataIndex.ts (line 34-37) by moving the args.json check before the empty check.
Impact: Users piping wrangler vectorize list-vectors --json to tools like jq will get no stdout output when the index is empty, breaking automation pipelines.
View 5 additional findings in Devin Review.
|
Can you also take a look at: The listVectors.ts handler has the same bug that this PR fixes in list.ts and listMetadataIndex.ts, but it was not fixed here. When --json is used and the result is empty, the empty-check at line 64 fires first, logging a warning via logger.warn and returning — the args.json check at line 69 is never reached. |
| it("should output clean JSON when there are no vectorize indexes with --json flag", async () => { | ||
| mockVectorizeV2RequestError(); | ||
| await runWrangler("vectorize list --json"); | ||
| expect(std.out).toMatchInlineSnapshot(`"[]"`); |
There was a problem hiding this comment.
Could you add a test when there are actual things to list, similar to it("should handle listing vectorize indexes" on l341 above
There was a problem hiding this comment.
Also using JSON.parse would allow testing that the output is actually valid JSON, i.e.:
expect(JSON.parse(std.out)).toMatchInlineSnapshot();| it("should output clean JSON when there are no metadata indexes with --json flag", async () => { | ||
| mockVectorizeV2RequestError(); | ||
| await runWrangler("vectorize list-metadata-index test-index --json"); | ||
| expect(std.out).toMatchInlineSnapshot(`"[]"`); |
|
I'm not making any claims about the quality of their work, but I wanted to let you know that @kaigritun is a fully-autonomous non-human actor |
|
Thanks @MichaelDeBoey - we saw the blog post 😄 |
Summary
When using
--jsonflag on vectorizelist,list-metadata-index, andinfocommands, the output included log messages (e.g.📋 Listing Vectorize indexes...) that made the stdout invalid JSON, breaking piping to tools likejq.Changes
Wrap progress log messages in a condition to check for
!args.json, following the pattern already established inlistVectors.ts(lines 49-51).Files modified:
packages/wrangler/src/vectorize/list.tspackages/wrangler/src/vectorize/listMetadataIndex.tspackages/wrangler/src/vectorize/info.tsBefore
$ wrangler vectorize list --json 📋 Listing Vectorize indexes... [{"name": "test-index", ...}]After
$ wrangler vectorize list --json [{"name": "test-index", ...}]Fixes #11011