fix: include search and prefix when computing level for list v1 - #1339
fix: include search and prefix when computing level for list v1#1339itslenny wants to merge 3 commits into
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — small, well-targeted fix.
Extended reasoning...
Overview
One-line logic fix in src/storage/database/pg.ts (searchObjects): the levels parameter passed to the storage.search SQL function is now computed from prefix + search concatenated, instead of prefix alone. Four new integration tests in object.test.ts cover trailing-slash search, full-filename search via search, prefix+search combination, and a no-match negative case.
Security risks
None. This is a read-only list/search path; no new user input reaches SQL unescaped (the same escapeLike/parameterization as before is preserved), and no auth/permission logic is touched.
Level of scrutiny
Low-to-medium is appropriate: single-line behavioral change confined to one query-parameter computation, in a non-critical (search/list) read path, with direct test coverage added for the exact scenarios being fixed.
Other factors
I verified the fix against the actual storage.search SQL function (migrations/tenant/0056-fix-optimized-search-function.sql), which internally computes v_prefix := coalesce(prefix, '') || coalesce(search, '') and filters objects by array_length(path_tokens, 1) = levels. The TS-side fix now matches that internal computation exactly, so levels is consistent with what the SQL function expects. The new tests exercise the trailing-slash/no-trailing-slash split-length equivalence and a negative case, which is the right coverage for this change.
d820940 to
1c92dbf
Compare
Coverage Report for CI Build 32294472576Coverage decreased (-0.005%) to 81.175%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
…n relying on passed in level value
d9b9264 to
9e33dfc
Compare
|
@claude review always |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes a core, non-trivial SQL listing/search algorithm (folder vs. file detection, hybrid skip-scan pagination, and a migration path for both new and pre-migration tenants), a human look would still be worthwhile before merging.
What was reviewed: the new migration's name-relative-to-prefix logic for both the name-sort (substring-based) and non-name-sort (path_tokens slice) branches, and the pg.ts dual-path wiring (migrated vs. pre-migration levels calculation) against the existing deployed 0056 SQL function's split_part semantics — confirmed the pre-migration fallback's new (prefix+search).split('/').length value is consistent with what the already-deployed function expects, so it isn't a regression for un-migrated tenants.
Extended reasoning...
Overview
This PR fixes a bug in storage.search() (list v1) where slashes in the search parameter were not accounted for when computing the folder/file "level" to extract, causing incorrect names to be returned when search contained nested path segments. It adds migration 0063 with a new 7-arg storage.search SQL function that computes prefix+search internally and always returns names relative to prefix (not prefix+search), updates src/storage/database/pg.ts to conditionally call either the new or legacy (8-arg) function depending on whether the tenant has the migration, and adds several new integration tests covering nested search paths, prefix+search combination, non-matching search, multi-level folder search, and non-name sort ordering.
Security risks
None identified. This is purely a listing/search correctness fix; no new user input is interpolated unsafely — all values are still passed as bound parameters, and the pre-existing escapeLike/ILIKE escaping for non-name-sort paths is preserved.
Level of scrutiny
This touches a hot, critical path (object listing/search, used by list() API and console) and contains genuinely complex logic: a hybrid peek-then-batch skip-scan algorithm re-implemented in the new SQL function, plus dual-path selection logic in the JS layer for pre- vs. post-migration tenants. I independently traced through both the name-sort and non-name-sort branches, and the pre-migration fallback's level calculation, and found the logic sound and consistent with the existing (already deployed) 0056 function's split_part-based semantics. However, given the algorithmic complexity, the fact that it ships a new tenant-wide SQL migration, and that it affects both migrated and legacy code paths simultaneously, I believe a human review of the SQL logic is still warranted despite finding no bugs and having good new test coverage.
Other factors
The PR includes 7 new integration tests covering the specific bug scenarios (nested search with/without trailing slash, prefix+search combination, no-match case, multi-level folder search, and non-name-column sorting), which is solid coverage for the described bug. The migration correctly drops only the specific old 8-arg function overload before creating the new 7-arg one, avoiding signature collisions for tenants that haven't migrated yet.
There was a problem hiding this comment.
Looks good — this addresses a real bug in the level computation, and the rolling-deploy compatibility concern raised by ferhatelmas has already been resolved in the final commit (921917b), which keeps the 8-arg storage.search signature with levels present-but-unused rather than dropping it.
Extended reasoning...
Overview
This PR fixes storage.search (list v1) so that when a search param containing slashes is combined with prefix, the function computes pagination "levels" using the combined prefix+search string (matching the function's internal v_prefix := coalesce(prefix,'') || coalesce(search,'')), and always returns object names relative to prefix rather than relative to prefix+search. It ships as tenant migration 0063, replacing the SQL function body, plus a small change in pg.ts's searchObjects to compute the level client-side, and six new integration tests covering nested search paths, prefix+search combination, no-match, folder-relative-name, and non-name sort ordering.
Rolling deploy concern already addressed
A reviewer (ferhatelmas) flagged that an earlier revision of this PR dropped the levels parameter from the SQL function signature entirely, which would break during a rolling deploy (old app code calling the new 8-arg-shaped function, or vice versa). Looking at the git history, the final commit on this PR (921917b, 'keep storage.search signature to maintain backward compatibility') directly resolves this: the function signature keeps all 8 original parameters including levels (now unused internally, computed as v_combined_levels from prefix+search instead), and the now-unnecessary hasMigration('fix-search-name-relative-to-prefix') branching in pg.ts was removed since the call shape is unconditionally compatible either way. I verified via grep that levels is declared in the signature but never read in the function body, confirming it is truly inert and safe to pass any value during a mixed-version deploy window.
Security risks
None. This is a pure read-path bug fix in a SECURITY INVOKER SQL function; no new user input reaches string concatenation in an unsafe way beyond what already existed (prefix/search were already interpolated into ILIKE patterns using parameterized queries, unchanged by this PR).
Level of scrutiny
Moderate — this touches a widely-used, performance-sensitive listing function, but the change is narrowly scoped (one internal calculation and the return-name substring math), guarded by a new migration, and backed by 6 new integration tests that directly exercise the previously-broken scenarios (nested slash search, prefix+search combination, folder-relative naming, non-name sort). The author already iterated in response to a legitimate reviewer concern about rolling-deploy safety, which is a good signal.
Other factors
No CODEOWNERS or auth/permission logic is touched. The only open PR comment (rolling-deploy compatibility) has been substantively addressed by a follow-up commit rather than left outstanding.
3 participants
What kind of change does this PR introduce?
Bug fix
What is the current behavior?
Slashes in
searchare not included when computing level. This causesstorage.search()to return the name from the incorrect level when the search param includes slashesWhen search params are included the
nameis returned relative to the search param (instead of the prefix)What is the new behavior?
Include prefix+search when computing level so it matches the internal behavior of storage.search (
v_prefix := coalesce(prefix, '') || coalesce(search, ''))Always return object name relative to the prefix. This maintains the pattern of prefix+name = full object path
Eliminate externally passed level param
Additional context
This issue/change does not impact
list v2as it does not include the separatesearchparam