Echo records_format back on search responses - #15
Open
sagargg wants to merge 1 commit into
Open
Conversation
`datastore_search` accepts `records_format` (objects / lists / csv / tsv) but the response never said which shape it returned, so a client holding only the body had to remember its own query string to parse `records`. Emit `result.records_format` next to `records`. The value comes from the writer itself — each `stream_*` function passes its own literal to `_stream_envelope` — so `datastore_search_sql`, which always streams through `stream_objects`, reports `objects` without any plumbing in the service layer. Also corrects a stale claim in the docs: `records_format=csv` / `tsv` emit data rows only, with column names on `result.fields`. There is no header row inside the records string (`test_csv_format_streams_data_rows` has always pinned this).
📝 WalkthroughWalkthroughThe datastore response envelope now echoes ChangesRecords format response contract
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
datastore/schemas/responses.py (1)
163-163: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConstrain
records_formatto the supported values.Using
strleaves the response contract open-ended in generated API documentation. UseLiteral["objects", "lists", "csv", "tsv"]while retaining the"objects"default so clients can discover and validate the four supported formats.Proposed type change
- records_format: str = "objects" + records_format: Literal["objects", "lists", "csv", "tsv"] = "objects"Add the corresponding
Literalimport.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@datastore/schemas/responses.py` at line 163, Update the records_format field in the response schema to use Literal with the supported values "objects", "lists", "csv", and "tsv", preserving "objects" as the default. Add the required Literal import and leave other response fields unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@API.md`:
- Around line 299-302: Update the pagination guidance in API.md lines 299-302
and CLAUDE.md lines 703-705 to distinguish empty-page representations:
object/list records use empty arrays, while CSV/TSV records are JSON strings and
use an empty string. Apply the equivalent clarification at both documented
sites.
---
Nitpick comments:
In `@datastore/schemas/responses.py`:
- Line 163: Update the records_format field in the response schema to use
Literal with the supported values "objects", "lists", "csv", and "tsv",
preserving "objects" as the default. Add the required Literal import and leave
other response fields unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b38aa26e-c9cd-44b5-883c-ca1d13717b42
📒 Files selected for processing (10)
API.mdCLAUDE.mddatastore/schemas/responses.pydatastore/services/streaming.pyexample_payload/datastore_search/records_format_csv.jsonexample_payload/datastore_search/records_format_lists.jsonpostman/collection.jsonpostman/generate_postman.pytests/test_datastore_search.pytests/test_datastore_search_sql.py
Comment on lines
+299
to
+302
| - `records_format=csv` / `tsv` → `records` is a single text body of data rows, | ||
| still inside the JSON envelope; column names are on `fields`, not in the text. | ||
| - `result.records_format` echoes the format that was applied, so a client can tell | ||
| which `records` shape it got without re-reading its own query string. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Document empty CSV/TSV pages as strings. Delimited records values are JSON strings, so their empty-page representation is "", not [].
API.md#L299-L302: update the adjacent pagination guidance to distinguish object/list empty arrays from CSV/TSV empty strings.CLAUDE.md#L703-L705: make the equivalent distinction in the datastore search pagination guidance.
📍 Affects 2 files
API.md#L299-L302(this comment)CLAUDE.md#L703-L705
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@API.md` around lines 299 - 302, Update the pagination guidance in API.md
lines 299-302 and CLAUDE.md lines 703-705 to distinguish empty-page
representations: object/list records use empty arrays, while CSV/TSV records are
JSON strings and use an empty string. Apply the equivalent clarification at both
documented sites.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
datastore_searchacceptsrecords_format(objects/lists/csv/tsv), but the response never said which shape it returned. A client holding only the response body had to remember its own query string to know how to parserecords.This adds
result.records_formatto the search envelope, next torecords:How
The value comes from the writer, not the service. Each
stream_*function indatastore/services/streaming.pyalready knows its own format, so it passes a literal to_stream_envelope.datastore_search_sqlstreams throughstream_objectsand therefore reportsobjectsfor free — no change needed inservices/read.py.datastore/services/streaming.py—records_formatparam on_stream_envelope, emitted before"records"; the four writers pass their own value.datastore/schemas/responses.py—records_format: str = "objects"onDatastoreSearchResponse.Resultso OpenAPI documents it.datastore_searchrequests (records_format=lists,records_format=csv) from newexample_payload/files;collection.jsonregenerated (27 requests, was 25).API.md+CLAUDE.md§6.2 / §6.4 response examples.Additive and backwards compatible — no existing field changes shape.
Docs correction
The docs claimed
records_format=csv/tsvput a header row first. They don't —_records_delimited_stringemits data rows only, andtest_csv_format_streams_data_rowshas always pinned that ("144,DCL,47.82\n…"). Column names live onresult.fields. Corrected in that function's docstring,API.md, andCLAUDE.md. No behaviour change — flagging in case the header was actually intended, which would be a separate PR.Testing
pytest— 368 passed. Assertions added to the fourrecords_formatrouting tests plus thesearch_sqlhappy path.ruff checkreports one pre-existing import-order error intests/test_cors.py, untouched here (its fix lives on another branch).Summary by CodeRabbit
New Features
records_format(objects,lists,csv, ortsv).Documentation