feat: drop bucketid_objname and add version list test coverage - #1353
feat: drop bucketid_objname and add version list test coverage#1353TylerHillery wants to merge 18 commits into
Conversation
This comment has been minimized.
This comment has been minimized.
f7c3141 to
6a22625
Compare
Coverage Report for CI Build 34526960402Coverage increased (+0.1%) to 82.803%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats💛 - Coveralls |
6a22625 to
f3648fc
Compare
f3648fc to
e4b2e9c
Compare
8f764a9 to
5e4bb18
Compare
There was a problem hiding this comment.
Pull request overview
This PR advances the version-aware object listing rollout by removing the legacy bucketid_objname index and adding extensive test coverage around versioned listing behavior (v1 and v2), including pagination edge cases and filter interactions.
Changes:
- Adds v1 list tests covering
noncurrentVersions/deleteMarkersfilters and multi-version pagination/batch-boundary behaviors. - Adds a comprehensive v2 versioning test suite covering filter matrices, ordering within keys, pagination correctness, delimiter behavior, and continuation-token constraints.
- Introduces DB migration
0069and registers it to drop thestorage.bucketid_objnameindex concurrently.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/test/object.test.ts | Adds v1 versioned listing tests for filters and pagination/batch-boundary behaviors. |
| src/test/object-list-v2.test.ts | Adds v2 versioning-focused integration tests and DB seeding helpers. |
| src/internal/database/migrations/types.ts | Registers the new migration identifier and version number. |
| migrations/tenant/0069-drop-bucketid-objname-index.sql | Drops the legacy index concurrently (non-transactional migration). |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
b49d8d3 to
4a6020e
Compare
535a32d to
abc9035
Compare
6ada077 to
83476c7
Compare
83476c7 to
6f544af
Compare
a952113 to
716645a
Compare
df3fc9e to
21c3b4b
Compare
aebe2d8 to
a0b61ff
Compare
a0b61ff to
b9f8e77
Compare
|
realized, we should address it in this one https://github.com/supabase/storage/pull/1346/changes#r3981465011 |
| const markCompletedMigration = options.markCompletedTillMigration | ||
| ? DBMigration[options.markCompletedTillMigration] | ||
| : undefined | ||
| const completedThroughMigration = markCompletedMigration ?? localMigration | ||
| const unsafeReset = MIGRATION_RESET_FLOORS.find( | ||
| ({ migration, activatedBy }) => | ||
| currentTenantMigrations.some( | ||
| (currentMigration) => currentMigration.id >= DBMigration[activatedBy] | ||
| ) && completedThroughMigration < DBMigration[migration] | ||
| ) | ||
|
|
||
| if (unsafeReset && !options.skipResetFloorValidation) { | ||
| throw new Error(`Cannot replay ${unsafeReset.migration} after ${unsafeReset.activatedBy}`) | ||
| } | ||
|
|
||
| // This tenant migration is already at the desired migration | ||
| if (currentLastMigration.id === localMigration) { | ||
| if ( | ||
| currentLastMigration.id === localMigration && | ||
| (markCompletedMigration === undefined || currentLastMigration.id >= markCompletedMigration) | ||
| ) { |
There was a problem hiding this comment.
if it's reset 72 -> 71 for example, then reset again, there is no information to know 72 but it actually run
There was a problem hiding this comment.
good point let me know what you think of this approach: 42db58f
b9f8e77 to
e9a7ce5
Compare
What kind of change does this PR introduce?
Part of the two phase roll out in #1346 to drop the
bucketid_objnameand add tests