feat(buckets): ref-based file ops (BucketRef) + resource overrides via getByNameLookup - #716
deepeshrai-tech wants to merge 2 commits into
Conversation
|
| * Numeric `bucketId` inputs pass through unchanged. Returns the effective folder from the | ||
| * lookup so callers can propagate any override-driven redirect to the file-op header block. | ||
| */ | ||
| private async _resolveBucketRef( |
There was a problem hiding this comment.
Convention violation: private keyword is already present, so the leading underscore is redundant and goes against the project rule ("Prefer private keyword over underscore prefix for private methods" — conventions.md).
| private async _resolveBucketRef( | |
| private async resolveBucketRef( |
All three existing private helpers in this file (_getUri, _getWriteUri, _uploadToUri) carry the same issue — worth a follow-up cleanup, but the new method introduced here should conform right away.
There was a problem hiding this comment.
Fixed in da25e05 — renamed _resolveBucketRef → resolveBucketRef. Left _getUri, _getWriteUri, _uploadToUri alone because those are pre-existing helpers outside this PR's scope (the ref-based work) — happy to do a follow-up cleanup PR that renames all three.
| 'Buckets.getReadUri', | ||
| ); | ||
|
|
||
| const headers = resolveFolderHeaders({ |
There was a problem hiding this comment.
getReadUri is the only one of the five file ops that doesn't validate bucketId after _resolveBucketRef returns. getFileMetaData, uploadFile, getFiles, and deleteFile all have an explicit guard here; skipping it in getReadUri means a { id: 0 } or an accidentally falsy bucket id silently flows to _getUri and produces a malformed URL rather than a clear ValidationError.
| const headers = resolveFolderHeaders({ | |
| if (!bucketId) { | |
| throw new ValidationError({ message: 'bucketId is required for getReadUri' }); | |
| } | |
| const headers = resolveFolderHeaders({ |
There was a problem hiding this comment.
Fixed in da25e05 — added the missing if (!bucketId) throw ... guard on getReadUri right after resolveBucketRef, matching the pattern in the other 4 file ops. The error now reads bucketId is required for getReadUri (was bucketId is required for getUri via the private _getUri helper) — more accurate since it names the actual caller. Two existing tests updated to match.
| * `getFiles`, `getFileMetaData`). `{ name }` triggers an internal | ||
| * `getByNameLookup` (runtime overrides apply); `{ id }` skips the lookup. | ||
| */ | ||
| export type BucketRef = ResourceRef<number>; |
There was a problem hiding this comment.
ResourceRef<number> is a three-way union { id } | { name } | { key }, but _resolveBucketRef only handles id and name — a caller who passes { key: '…' } gets a runtime ValidationError even though TypeScript said the type was fine. The PR description acknowledges this ("runtime currently supports { id } and { name }"), but that disclaimer doesn't appear anywhere in the public type.
Two options:
- Narrow
BucketRefto only the supported variants so the type contract matches the runtime:Suggested changeexport type BucketRef = ResourceRef<number>; export type BucketRef = { id: number; name?: never } | { name: string; id?: never }; - Keep
ResourceRef<number>but add a@remarks(or inline note in the JSDoc) stating that{ key }is not yet implemented and will throw.
Option 1 is safer — it surfaces the gap at compile time instead of runtime.
There was a problem hiding this comment.
Fixed in da25e05 — narrowed BucketRef from the generic ResourceRef<number> ({id} | {name} | {key}) to { id } | { name } with explicit ?: never markers. Buckets don't expose a public GUID key on their operational routes, so { key } was misleading in the type surface. Now the compile-time type matches the runtime — no more "TypeScript said fine, runtime says ValidationError" gap. Docs note the deliberate exclusion.
Review summaryThree issues found:
|
|
8138a7d to
da25e05
Compare
| @track('Buckets.GetFileMetaData') | ||
| async getFileMetaData<T extends BucketGetFileMetaDataWithPaginationOptions = BucketGetFileMetaDataWithPaginationOptions>( | ||
| bucketId: number, | ||
| bucketIdOrRef: number | BucketRef, |
There was a problem hiding this comment.
The class-level overload declarations for getFileMetaData (unchanged, sitting above the @track decorator) still say bucketId: number — no BucketRef overload was added to the class. TypeScript resolves caller types against the overload signatures, not the implementation signature. This means:
const buckets = new BucketService(sdk);
// TS Error: Argument of type '{ name: string }' is not assignable to parameter of type 'number'.
await buckets.getFileMetaData({ name: 'InvoicesBucket' }, { folderPath: 'Shared' });uploadFile and getReadUri both got explicit BucketRef overloads (e.g. uploadFile(bucketRef: BucketRef, path, content, options?)) — the same treatment is needed here, placed before the two existing number overloads:
| bucketIdOrRef: number | BucketRef, | |
| bucketIdOrRef: number | BucketRef, |
(The suggestion keeps the implementation line unchanged — the fix is to add a new overload declaration above the existing two, not to change this line.)
// Add before the existing overloads:
getFileMetaData<T extends BucketGetFileMetaDataWithPaginationOptions = BucketGetFileMetaDataWithPaginationOptions>(
bucketRef: BucketRef,
options?: T,
): Promise<T extends HasPaginationOptions<T> ? PaginatedResponse<BlobItem> : NonPaginatedResponse<BlobItem>>;There was a problem hiding this comment.
Fixed in 3fb3c4e — added the missing BucketRef | number overload declaration for getFileMetaData at the class level, matching the two-overload pattern already used on uploadFile and getReadUri. Callers passing { name: 'x' } now typecheck against the ref overload, not the impl signature. getFiles and deleteFile use inline union sigs (no separate overload needed).
| */ | ||
| getReadUri( | ||
| bucketId: number, | ||
| bucketRef: BucketRef | number, |
There was a problem hiding this comment.
The @param bucketId JSDoc comment above this method was not updated to match the renamed parameter. TypeDoc correlates @param tags to parameters by name — a @param bucketId description on a parameter named bucketRef is orphaned and won't appear in generated docs.
getFileMetaData got its @param updated in this PR; the same update is needed for getReadUri, uploadFile, deleteFile, and getFiles. Each of their JSDoc blocks should change:
- * @param bucketId - The ID of the bucket …
+ * @param bucketRef - Bucket ref (`{ id }` or `{ name }`). A raw numeric bucket id is also
+ * accepted as a shorthand for `{ id }`. `{ name }` triggers an internal name lookup where
+ * runtime resource overrides may redirect the target across folders.There was a problem hiding this comment.
Fixed in 3fb3c4e — updated @param bucketId → @param bucketRef on getReadUri, uploadFile, deleteFile, getFiles. Each carries the same wording as getFileMetaData: describes both { id } / { name } variants + the numeric shorthand, and notes overrides apply on { name }. @example blocks refreshed to use the ref-based call shape. getById intentionally left with @param bucketId — it's not ref-based.
| @@ -1407,6 +1408,71 @@ describe('BucketService Unit Tests', () => { | |||
| { folderId: TEST_CONSTANTS.FOLDER_ID }, | |||
| )).rejects.toThrow(TEST_CONSTANTS.ERROR_MESSAGE); | |||
| }); | |||
There was a problem hiding this comment.
rules.md is explicit: "When sibling methods share a configuration pattern, each method needs its own test for that pattern — verifying it on one method does not cover a sibling that uses the same setup."
The { name } resolution path goes through resolveBucketRef in all five methods, but each method wires it differently:
getFileMetaDatahas the most complex setup — it normalises two overload forms (legacyfolderId: numberpositional vs. options bag) before callingresolveBucketRef. A regression in that normalization (e.g. folder scope extracted incorrectly for the{ name }branch) would be invisible to adeleteFiletest.uploadFilesimilarly has a three-form discriminator.getReadUrilikewise.getFilesis simpler but still has its own options-destructuring step.
The two deleteFile tests added here are a good blueprint — a basic { name } resolution test and an override-redirect test — but the same pair is needed for each of the other four methods to satisfy the project's sibling-method testing rule.
There was a problem hiding this comment.
Fixed in 3fb3c4e — added a sibling override test for each of the other 4 file ops (getFileMetaData, uploadFile, getReadUri, getFiles), matching the deleteFile override test. Each verifies:
- the internal name-lookup GET (
getByNameLookup) scopes to the redirected folder, AND - the follow-up operation's headers scope to the same redirected folder (PUT-URI fetch, POST body wire, or PaginationHelpers.getAll config).
Test count for buckets.test.ts: 79 → 83. All 2735 unit tests pass.
Review summaryThree new issues found (the three prior open threads are still unresolved — author says all fixed in da25e05 but threads remain open): New findings this run:
|
da25e05 to
3fb3c4e
Compare
| expect(getOpts?.headers?.[FOLDER_PATH_ENCODED]).toBe(OVERRIDE_TEST_CONSTANTS.TARGET_FOLDER_PATH_ENCODED); | ||
| // The pagination call's headers carry the same redirected folder. | ||
| const [paginationConfig] = vi.mocked(PaginationHelpers.getAll).mock.calls[0]; | ||
| expect((paginationConfig as any).headers?.[FOLDER_PATH_ENCODED]).toBe(OVERRIDE_TEST_CONSTANTS.TARGET_FOLDER_PATH_ENCODED); |
There was a problem hiding this comment.
as any violates the project's "No any type" convention (conventions.md). GetAllConfig (in src/utils/pagination/internal-types.ts) already types headers as Record<string, string>, so there is a clean alternative:
| expect((paginationConfig as any).headers?.[FOLDER_PATH_ENCODED]).toBe(OVERRIDE_TEST_CONSTANTS.TARGET_FOLDER_PATH_ENCODED); | |
| const [paginationConfig] = vi.mocked(PaginationHelpers.getAll).mock.calls[0]; | |
| expect((paginationConfig as { headers?: Record<string, string> }).headers?.[FOLDER_PATH_ENCODED]).toBe(OVERRIDE_TEST_CONSTANTS.TARGET_FOLDER_PATH_ENCODED); |
The same pattern appears at line 1798 in the getFiles override test — same fix needed there.
There was a problem hiding this comment.
Fixed in f6bbf27 — replaced both as any casts (in the getFileMetaData and getFiles sibling override tests) with the typed as { headers?: Record<string, string> } shape. Follows the suggestion and matches the GetAllConfig.headers type in src/utils/pagination/internal-types.ts. typecheck + lint clean, 83/83 pass.
Review summaryOne new finding this run:
|
3fb3c4e to
f6bbf27
Compare
| uploadFile( | ||
| bucketRef: BucketRef, | ||
| path: string, | ||
| content: Blob | Uint8Array<ArrayBuffer> | File, | ||
| options?: BucketUploadFileRequestOptions, | ||
| ): Promise<BucketUploadResponse>; |
There was a problem hiding this comment.
This new overload uses BucketRef only, while the existing overload immediately below keeps number only — splitting what BucketServiceModel declares as a single BucketRef | number parameter. The same BucketRef-and-number-as-separate-overloads pattern appears at getReadUri (lines 288–297).
The split creates a TypeScript gap: a caller holding a BucketRef | number variable can call getFileMetaData(ref, opts) on a BucketService instance (line 109 already uses the combined form) but gets a compile error on uploadFile(ref, path, content) — neither class overload matches the union. On BucketServiceModel the call is fine because the interface uses the combined type.
Collapse this overload and the number-only one below into a single combined entry, matching the getFileMetaData and interface patterns:
| uploadFile( | |
| bucketRef: BucketRef, | |
| path: string, | |
| content: Blob | Uint8Array<ArrayBuffer> | File, | |
| options?: BucketUploadFileRequestOptions, | |
| ): Promise<BucketUploadResponse>; | |
| uploadFile( | |
| bucketRef: BucketRef | number, | |
| path: string, | |
| content: Blob | Uint8Array<ArrayBuffer> | File, | |
| options?: BucketUploadFileRequestOptions, | |
| ): Promise<BucketUploadResponse>; |
(The uploadFile(bucketId: number, ...) overload on the next lines becomes redundant and should be removed alongside this change.)
There was a problem hiding this comment.
Fixed in 095be4a — collapsed the two split overloads on uploadFile into a single combined bucketRef: BucketRef | number overload, matching getFileMetaData and BucketServiceModel. Callers holding a BucketRef | number union now match the class overload.
| getReadUri( | ||
| bucketRef: BucketRef, | ||
| path: string, | ||
| options?: BucketGetReadUriRequestOptions, | ||
| ): Promise<BucketGetUriResponse>; |
There was a problem hiding this comment.
Same split-overload issue as uploadFile above: BucketRef-only here plus number-only below, while BucketServiceModel declares BucketRef | number as one combined parameter and getFileMetaData (line 109) already uses the combined form in the class.
| getReadUri( | |
| bucketRef: BucketRef, | |
| path: string, | |
| options?: BucketGetReadUriRequestOptions, | |
| ): Promise<BucketGetUriResponse>; | |
| getReadUri( | |
| bucketRef: BucketRef | number, | |
| path: string, | |
| options?: BucketGetReadUriRequestOptions, | |
| ): Promise<BucketGetUriResponse>; |
(The getReadUri(bucketId: number, ...) overload immediately below becomes redundant and should be removed.)
There was a problem hiding this comment.
Fixed in 095be4a — same collapse on getReadUri: one bucketRef: BucketRef | number overload + the deprecated options-only form. typecheck + lint clean, 83/83 pass.
Review summaryOne new finding this run: Split class overloads for uploadFile and getReadUri — both methods add a BucketRef-only positional overload alongside the existing number-only one, rather than merging them into a single combined parameter. This diverges from BucketServiceModel (which uses BucketRef | number throughout) and from getFileMetaData at line 109 (which already uses the combined form in the class). A caller holding a BucketRef | number variable can call getFileMetaData(ref, opts) on a BucketService instance without issue but gets a TypeScript compile error on uploadFile(ref, path, content) or getReadUri(ref, path, opts). Inline suggestions posted on both overload blocks. |
f6bbf27 to
095be4a
Compare
|
✅ No issues found. Checked for bugs and CLAUDE.md compliance. |
095be4a to
b465b50
Compare
|
✅ No issues found. Checked for bugs and CLAUDE.md compliance. |
…a getByNameLookup
Broadens the first-slot type of every Bucket file op from `bucketId: number` to
`bucketRef: BucketRef | number`, mirroring the Assets and Processes ref-based
patterns from the Resource Resolution Framework. Applies to:
- `uploadFile`
- `getReadUri`
- `getFileMetaData`
- `getFiles`
- `deleteFile`
`BucketRef = ResourceRef<number>` (`{ id }` | `{ name }` | `{ key }`; runtime
currently supports `{ id }` and `{ name }`). Numeric bucket ids remain accepted
as a shorthand for `{ id }` — every existing caller keeps working unchanged.
A shared private `_resolveBucketRef` helper routes `{ name }` refs through
`FolderScopedService.getByNameLookup`, which internally calls `resolveOverride`
— so a runtime override for a design-time bucket name redirects both the wire
bucket id (via the lookup response) AND the follow-up file-op's folder header.
Each file op propagates the lookup's `effectiveFolder` onto its own
`resolveFolderHeaders` call, matching the pattern shipped in the Assets PR.
Docs: `docs/oauth-scopes.md` — `uploadFile()` and `deleteFile()` entries note
the extra `OR.Buckets.Read` scope needed when passing `{ name }`.
Tests (`describe('deleteFile')` — representative for all 5 methods since they
share `_resolveBucketRef`):
- `{ name }` ref triggers `getByNameLookup` then targets the resolved bucket id
- Cross-folder override redirect on `{ name }` — lookup GET + follow-up DELETE
header both scope to the redirect target
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
b465b50 to
6989bb5
Compare
|
✅ No issues found. Checked for bugs and CLAUDE.md compliance. |
…es for upload/deleteFile Raina's #716 review round: - BucketServiceModel / BucketService: the primary overload of every file-op method (getFileMetaData / getReadUri / uploadFile / deleteFile / getFiles) is now `BucketRef` only. Numeric `bucketId` moves to a dedicated `@deprecated Use the ref-based form: <method>({ id: bucketId }, ...)` overload. Mirrors the processes-PR pattern of splitting a ref-based signature from a deprecated legacy one instead of hiding BC in a union type. - docs/oauth-scopes.md: `uploadFile()` and `deleteFile()` now list `OR.Buckets` or `OR.Buckets.Write`, matching every other write method's scope-alternate pattern (previously showed only the broader `OR.Buckets`, which read confusingly next to the read entries). Runtime signature stays `bucketRef: BucketRef | number` on the class implementation body since the ref-resolver dispatches on shape — only the overload declarations split. 84/84 bucket unit tests pass; typecheck + oxlint clean. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
✅ No issues found. Checked for bugs and CLAUDE.md compliance. |



Summary
Broadens the first-slot type of every Bucket file op from
bucketId: numbertobucketRef: BucketRef | number, mirroring the Assets and Processes ref-based patterns from the Resource Resolution Framework. Applies to:uploadFilegetReadUrigetFileMetaDatagetFilesdeleteFileBucketRef = ResourceRef<number>({ id }|{ name }|{ key }; runtime currently supports{ id }and{ name }). Numeric bucket ids remain accepted as a shorthand for{ id }— every existing caller keeps working unchanged.A shared private
_resolveBucketRefhelper routes{ name }refs throughFolderScopedService.getByNameLookup, which internally callsresolveOverride— so a runtime override for a design-time bucket name redirects both the wire bucket id (via the lookup response) AND the follow-up file-op's folder header.Each file op propagates the lookup's
effectiveFolderonto its ownresolveFolderHeaderscall, matching the pattern shipped in the Assets PR.Test plan
npm run typecheck— cleannpm run lint— 0/0npm run test:unit— 2731/2731 pass (single failing file is a pre-existingcheck-samples.test.tsmodule-not-found)npm run build— exit 0describe('deleteFile')— representative for all 5 methods since they share_resolveBucketRef:{ name }ref triggersgetByNameLookupthen targets the resolved bucket id{ name }— lookup GET + follow-up DELETE header both scope to the redirect targetDocs
docs/oauth-scopes.md—uploadFile()anddeleteFile()entries note the extraOR.Buckets.Readscope needed when passing{ name }.🤖 Generated with Claude Code