feat: version aware listing - #1346
Conversation
8480320 to
30fd898
Compare
Coverage Report for CI Build 33525890038Coverage decreased (-0.3%) to 81.063%Details
Uncovered Changes
Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
There was a problem hiding this comment.
I have parsed out all the functions from before this change and after this change for an easier git diff to review:
tyler/chore/function-diffs-before...tyler/chore/function-diffs-after
There was a problem hiding this comment.
benchmarking code can be found here: tyler/feat/object-versioning-wave-2a...tyler/chore/object-versioning-benchmark
Note, this was primarily AI generated benchmark I still need to validate results. This benchmarking code will not be committed.
There was a problem hiding this comment.
thanks for sharing!
From the look of it, it is missing an index:
| list_objects_with_delimiter: deleteMarkers=only | — | — | 3175.92ms | 4081.87ms | 2425.58ms |
8534d33 to
c2009f2
Compare
8104dfa to
1b461d6
Compare
There was a problem hiding this comment.
Pull request overview
Adds version-aware listing capabilities to the Storage object list endpoints (v1 and v2), enabling clients to include/exclude noncurrent versions and delete markers and to perform exact key matches (useful for retrieving all versions of a single object).
Changes:
- Add
noncurrentVersions,deleteMarkers(tri-state) andexactMatchto list endpoints and wire them through storage/database layers. - Extend V2 continuation token pagination to support multi-version key pagination using
(archived_at, version)as additional cursor tiebreakers. - Introduce tenant migrations updating DB listing/search functions to return versioning metadata, and drop the legacy
(bucket_id, name)uniqueness index to support multi-version testing/groundwork.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/test/object.test.ts | Adds v1 route coverage for versioning filters and exact-match behavior. |
| src/test/object-list-v2.test.ts | Adds extensive v2 versioning + pagination tests and DB seeding helpers. |
| src/storage/schemas/object.ts | Introduces objectListEntrySchema/ObjectListEntry for list responses (nullable versioning fields). |
| src/storage/object.ts | Threads new list options, adjusts delimiter collapsing for exactMatch, and extends continuation token fields. |
| src/storage/database/pg.ts | Implements filtering/exactMatch and pagination tiebreaks in PG list/search queries and function calls. |
| src/storage/database/adapter.ts | Updates DB interfaces to accept new list/search options and return ObjectListEntry[]. |
| src/internal/database/migrations/types.ts | Registers new migrations: version-aware listing and index drop. |
| src/http/routes/object/listObjectsV2.ts | Adds request params + migration gating for versioning filters on v2 endpoint. |
| src/http/routes/object/listObjects.ts | Adds request params + migration gating and updates response schema for v1 endpoint. |
| migrations/tenant/0068-list-objects-with-versions.sql | Updates storage SQL functions to support version-aware listing outputs and filtering. |
| migrations/tenant/0069-drop-bucketid-objname-index.sql | Drops the legacy unique index concurrently to allow multi-version rows per key. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline finding on the timestamp-cursor tiebreak collapsing to '' for all rows, I also checked the upsertObject conflict-target switch (pg.ts, keyed on the pre-existing objects-current-version-index migration) and the noncurrentVersions/deleteMarkers cursor-locking in object.ts — both correctly gate on migration state and aren't affected by the same class of bug.
Extended reasoning...
A confirmed bug was already reported inline (storage.search_by_timestamp's version tiebreak collapsing to '' for all rows, not just the boundary row, when the previous page's last row had a NULL/empty version — a realistic scenario since version is nullable and defaults to NULL for pre-existing objects). Given that finding is present, per policy I only add a short note about what else was checked and ruled out, rather than a full approve/defer writeup.
I spot-checked the three ruled-out candidates: the upsertObject ON CONFLICT target switch in pg.ts (ties migration 0069's dropped unique index to the pre-existing partial index from migration 0066, consistent with the PR's stated intent to remove the bucket_id/name uniqueness constraint), and the cursor-locking/enum-validation logic in object.ts for noncurrentVersions/deleteMarkers. Nothing beyond the already-reported finding stood out as a new, independently-worth-flagging issue.
|
|
||
| export type Obj = FromSchema<typeof objectSchema> | ||
|
|
||
| export const objectListEntrySchema = { |
There was a problem hiding this comment.
any reason we don't want to add these properties directly to the Obj type instead of having a new one?
There was a problem hiding this comment.
The nullability only exists because list responses can return synthetic folder entries with no version/marker/versioned fields at all, so a separate schema keeps that "might be a folder" case scoped to listing instead of forcing null-checks onto call sites that can never hit it.
If you still prefer having on Obj let me know
| } | ||
| }, 5000) | ||
|
|
||
| test('v1 desc listing advances to the next key after exhausting a key at a batch boundary', async () => { |
There was a problem hiding this comment.
I think we are skipping some keys wrongly. Can you add an ascending version of this test while adding a sibling object between deepKey and targetKey?
There was a problem hiding this comment.
A test like this? (note I have moved the tests to new branch/PR as part of the 2 phase rollout): 8f764a9
This comment has been minimized.
This comment has been minimized.
1537989 to
0bea5a1
Compare
55dfa10 to
6249965
Compare
6249965 to
431f5c4
Compare
431f5c4 to
14275c7
Compare
14275c7 to
f869840
Compare
![depthfirst-app[bot]](https://avatars.githubusercontent.com/in/1021002?s=60&v=4){kind=small}
| -- CREATE OR REPLACE FUNCTION doesn't work when parameters or return value changes | ||
| DROP FUNCTION IF EXISTS storage.list_objects_with_delimiter(text, text, text, integer, text, text, text); | ||
|
|
||
| CREATE OR REPLACE FUNCTION storage.list_objects_with_delimiter( |
There was a problem hiding this comment.
Maybe it's time to refactor this big function into smaller more manageable one?
There was a problem hiding this comment.
Most of the function's size comes from planner-sensitive query variants. For example, we need explicit branches for sort direction, prefix bounds, multi-version pagination, and delete-marker filtering:
IF delete_markers = 'only' THEN
-- Literal is_delete_marker predicate so Postgres can use the partial index
ELSIF v_multi_row THEN
-- Version-aware keyset query
ELSE
-- Current-version query
END IF;It is difficult to collapse these into one parameterized static query because PostgreSQL's cached generic plan cannot reliably use the name bounds or partial index. Dynamic SQL would preserve those predicates, but it would be replanned on every peek.
I also investigated extracting helpers for tri-state normalization and prefix upper-bound calculation. Those blocks are small and used in only a few places, so I do not think it's worth additional schema-level functions.
Did you have some ideas around what the refactor would look like?
What kind of change does this PR introduce?
Feature
What is the current behavior?
Currently, listing (v1 and v2) doesn't allow including
deleteMarkersornoncurrentVersions, or performing anexactMatchon the prefix.What is the new behavior?
This change adds the above parameters to the list endpoints and wires them through all the necessary functions.
deleteMarkersandnoncurrentVersionsonly allowexclude,include, andonly, withexcludebeing the default.exactMatchis a Boolean flag that changes the predicate onnamefromname LIKE '$1%'toname = $1.exactMatchwill be useful when you want to find all versions of a particular object. Without it, you could have an object likefile.configandfile.config.backup, where the backup would be included in the list results.Additional context
It is important to note that this PR still does not change any of the write operations that would allow multiple versions per object, but lays the groundwork for #1347 when it lands. I removed the
bucket_id, nameuniqueness constraint so that we can fully test the new listing options under various conditions by seeding the database directly.