From b4096e491d4762e508b44e6bf9964f13acba2d22 Mon Sep 17 00:00:00 2001 From: Deepesh Rai Date: Wed, 15 Jul 2026 23:31:43 +0530 Subject: [PATCH 1/2] feat(buckets): accept bucket id or name on file ops; add folderKey/folderPath to getAll MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extends 5 BucketService file-operation methods so their first parameter accepts `bucket: number | string` — either a numeric bucket Id or a bucket Name. When a name is supplied the SDK resolves it to an Id via an OData `$filter=Name eq '…'&$select=Id&$top=1` lookup on the folder-scoped Buckets collection. Methods updated: - `deleteFile` - `getFileMetaData` - `getFiles` - `getReadUri` - `uploadFile` Also extends `BucketGetAllOptions` with `folderKey?: string` and `folderPath?: string` so `getAll()` can be scoped by any of the three folder refs. `getAll()` with no folder options deliberately remains a cross-folder query — it does not fall back to the SDK init-time meta-tag folderKey (documented, non-breaking). Shared name-lookup logic (name validation + folder header resolution + OData filter + empty-result → NotFoundError) is extracted into a private `findByNameInFolder` helper on `FolderScopedService`; both `getByNameLookup` (full-resource fetch) and the new `resolveIdByName` (Id-only projection) delegate to it. Unit and integration coverage added for all name-path scenarios and for `folderKey` / `folderPath` on `getAll()`. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/models/orchestrator/buckets.models.ts | 107 +++++-- src/models/orchestrator/buckets.types.ts | 5 + src/services/folder-scoped.ts | 106 +++++-- src/services/orchestrator/buckets/buckets.ts | 164 ++++++++--- .../orchestrator/buckets.integration.test.ts | 81 ++++++ .../services/orchestrator/buckets.test.ts | 267 +++++++++++++++++- 6 files changed, 621 insertions(+), 109 deletions(-) diff --git a/src/models/orchestrator/buckets.models.ts b/src/models/orchestrator/buckets.models.ts index 83ed1536f..953001040 100644 --- a/src/models/orchestrator/buckets.models.ts +++ b/src/models/orchestrator/buckets.models.ts @@ -19,26 +19,37 @@ import { PaginatedResponse, NonPaginatedResponse, HasPaginationOptions } from '. */ export interface BucketServiceModel { /** - * Gets all buckets across folders with optional filtering - * + * Gets all buckets across folders, or scoped to a specific folder when + * folder context is provided. + * + * Folder scoping is optional and can be supplied as `folderId`, `folderKey`, + * or `folderPath` in the options. When none of them is provided the request + * is a cross-folder query. Unlike file-operation methods, `getAll()` does + * **not** fall back to the SDK's init-time folder key — no folder in options + * always means cross-folder. + * * The method returns either: * - A NonPaginatedResponse with data and totalCount (when no pagination parameters are provided) * - A paginated result with navigation cursors (when any pagination parameter is provided) - * - * @param options - Query options including optional folderId and pagination options + * + * @param options - Optional folder scoping (`folderId` / `folderKey` / `folderPath`), OData query options, and pagination options * @returns Promise resolving to either an array of buckets NonPaginatedResponse or a PaginatedResponse when pagination options are used. * {@link BucketGetResponse} * @example * ```typescript - * // Get all buckets across folders + * // Cross-folder — every bucket the caller can see * const allBuckets = await buckets.getAll(); * - * // Get buckets within a specific folder - * const folderBuckets = await buckets.getAll({ - * folderId: - * }); + * // Scoped by folder ID + * const folderBuckets = await buckets.getAll({ folderId: }); * - * // Get buckets with filtering + * // Scoped by folder key (GUID) + * await buckets.getAll({ folderKey: '5f6dadf1-3677-49dc-8aca-c2999dd4b3ba' }); + * + * // Scoped by folder path + * await buckets.getAll({ folderPath: 'Shared/Finance' }); + * + * // Filtering * const filteredBuckets = await buckets.getAll({ * filter: "name eq 'MyBucket'" * }); @@ -104,6 +115,11 @@ export interface BucketServiceModel { /** * Gets metadata for files in a bucket with optional filtering and pagination. * + * Accepts either a numeric bucket `Id` or a bucket `Name`. When a name is + * supplied, the bucket is resolved within the folder scope provided in + * `options` — the name lookup runs in the same folder used for the + * subsequent file listing. + * * Folder context can be supplied as `folderId`, `folderKey`, or `folderPath` * inside the options. * @@ -111,15 +127,18 @@ export interface BucketServiceModel { * - A NonPaginatedResponse with items array (when no pagination parameters are provided) * - A PaginatedResponse with navigation cursors (when any pagination parameter is provided) * - * @param bucketId - The ID of the bucket to get file metadata from + * @param bucket - The bucket's numeric ID or its name * @param options - Folder scoping (`folderId` / `folderKey` / `folderPath`) and optional parameters for filtering and pagination * @returns Promise resolving to either an array of files metadata NonPaginatedResponse or a PaginatedResponse when pagination options are used. * {@link BlobItem} * @example * ```typescript - * // By folder ID + * // By bucket ID * const fileMetadata = await buckets.getFileMetaData(, { folderId: }); * + * // By bucket name + * await buckets.getFileMetaData('MyBucket', { folderId: }); + * * // By folder key (GUID) * await buckets.getFileMetaData(, { folderKey: '5f6dadf1-3677-49dc-8aca-c2999dd4b3ba' }); * @@ -139,7 +158,7 @@ export interface BucketServiceModel { * ``` */ getFileMetaData( - bucketId: number, + bucket: number | string, options?: T, ): Promise< T extends HasPaginationOptions @@ -149,16 +168,16 @@ export interface BucketServiceModel { /** * Gets metadata for files in a bucket — positional `folderId` form. * - * @deprecated Use the options-object form: `getFileMetaData(bucketId, { folderId })`. See {@link BucketGetFileMetaDataWithPaginationOptions} for the supported options. + * @deprecated Use the options-object form: `getFileMetaData(bucket, { folderId })`. See {@link BucketGetFileMetaDataWithPaginationOptions} for the supported options. * - * @param bucketId - The ID of the bucket to get file metadata from + * @param bucket - The bucket's numeric ID or its name * @param folderId - Required folder ID (numeric) * @param options - Optional parameters for filtering and pagination * @returns Promise resolving to either an array of files metadata NonPaginatedResponse or a PaginatedResponse when pagination options are used. * {@link BlobItem} */ getFileMetaData( - bucketId: number, + bucket: number | string, folderId: number, options?: T, ): Promise< @@ -170,19 +189,26 @@ export interface BucketServiceModel { /** * Gets a direct download URL for a file in the bucket. * + * Accepts either a numeric bucket `Id` or a bucket `Name`. When a name is + * supplied, the bucket is resolved within the folder scope provided in + * `options`. + * * Folder context can be supplied as `folderId`, `folderKey`, or `folderPath` * in the options. * - * @param bucketId - The ID of the bucket + * @param bucket - The bucket's numeric ID or its name * @param path - The full path to the file * @param options - Folder scoping (`folderId` / `folderKey` / `folderPath`) and optional `expiryInMinutes` * @returns Promise resolving to blob file access information * {@link BucketGetUriResponse} * @example * ```typescript - * // By folder ID + * // By bucket ID * await buckets.getReadUri(, '/folder/file.pdf', { folderId: }); * + * // By bucket name + * await buckets.getReadUri('MyBucket', '/folder/file.pdf', { folderId: }); + * * // By folder key (GUID) * await buckets.getReadUri(, '/folder/file.pdf', { folderKey: '5f6dadf1-3677-49dc-8aca-c2999dd4b3ba' }); * @@ -191,14 +217,14 @@ export interface BucketServiceModel { * ``` */ getReadUri( - bucketId: number, + bucket: number | string, path: string, options?: BucketGetReadUriRequestOptions, ): Promise; /** * Gets a direct download URL for a file in the bucket — options-only form. * - * @deprecated Use the positional form: `getReadUri(bucketId, path, options?)`. See {@link BucketGetReadUriRequestOptions} for the supported options. + * @deprecated Use the positional form: `getReadUri(bucket, path, options?)`. See {@link BucketGetReadUriRequestOptions} for the supported options. * * @param options - Contains bucketId, folder scoping (`folderId` / `folderKey` / `folderPath`), file path and optional expiry time * @returns Promise resolving to blob file access information @@ -209,10 +235,14 @@ export interface BucketServiceModel { /** * Uploads a file to a bucket. * + * Accepts either a numeric bucket `Id` or a bucket `Name`. When a name is + * supplied, the bucket is resolved within the folder scope provided in + * `options`. + * * Folder context can be supplied as `folderId`, `folderKey`, or `folderPath` * in the options. * - * @param bucketId - The ID of the bucket to upload to + * @param bucket - The bucket's numeric ID or its name * @param path - Path where the file should be stored in the bucket * @param content - File content to upload * @param options - Folder scoping (`folderId` / `folderKey` / `folderPath`) @@ -220,10 +250,13 @@ export interface BucketServiceModel { * {@link BucketUploadResponse} * @example * ```typescript - * // By folder ID + * // By bucket ID * const file = new File(['file content'], 'example.txt'); * await buckets.uploadFile(, '/folder/example.txt', file, { folderId: }); * + * // By bucket name + * await buckets.uploadFile('MyBucket', '/folder/example.txt', file, { folderId: }); + * * // By folder key (GUID) * await buckets.uploadFile(, '/folder/example.txt', file, { folderKey: '5f6dadf1-3677-49dc-8aca-c2999dd4b3ba' }); * @@ -236,7 +269,7 @@ export interface BucketServiceModel { * ``` */ uploadFile( - bucketId: number, + bucket: number | string, path: string, content: Blob | Uint8Array | File, options?: BucketUploadFileRequestOptions, @@ -244,7 +277,7 @@ export interface BucketServiceModel { /** * Uploads a file to a bucket — options-only form. * - * @deprecated Use the positional form: `uploadFile(bucketId, path, content, options?)`. See {@link BucketUploadFileRequestOptions} for the supported options. + * @deprecated Use the positional form: `uploadFile(bucket, path, content, options?)`. See {@link BucketUploadFileRequestOptions} for the supported options. * * @param options - Options for file upload including bucket ID, folder scoping (`folderId` / `folderKey` / `folderPath`), path, and content * @returns Promise resolving bucket upload response @@ -255,21 +288,32 @@ export interface BucketServiceModel { /** * Deletes a file from a bucket * - * @param bucketId - The ID of the bucket + * Accepts either a numeric bucket `Id` or a bucket `Name`. When a name is + * supplied, the bucket is resolved within the folder scope provided in + * `options`. + * + * @param bucket - The bucket's numeric ID or its name * @param path - The full path to the file to delete * @param options - Folder scoping (`folderId` / `folderKey` / `folderPath`) * @returns Promise resolving when the file is deleted * @example * ```typescript - * // Delete a file from a bucket + * // By bucket ID * await buckets.deleteFile(, '/folder/file.pdf', { folderId: }); + * + * // By bucket name + * await buckets.deleteFile('MyBucket', '/folder/file.pdf', { folderId: }); * ``` */ - deleteFile(bucketId: number, path: string, options?: BucketDeleteFileOptions): Promise; + deleteFile(bucket: number | string, path: string, options?: BucketDeleteFileOptions): Promise; /** * Lists all files in a bucket. * + * Accepts either a numeric bucket `Id` or a bucket `Name`. When a name is + * supplied, the bucket is resolved within the folder scope provided in + * `options`. + * * Returns a flat, recursive listing of all files in the bucket. Supports regex filtering * and filter / orderby / select / expand. {@link BucketFile} entries include * `isDirectory` so callers can distinguish folders from files. @@ -278,16 +322,19 @@ export interface BucketServiceModel { * - A NonPaginatedResponse with items array (when no pagination parameters are provided) * - A PaginatedResponse with navigation cursors (when any pagination parameter is provided) * - * @param bucketId - The ID of the bucket + * @param bucket - The bucket's numeric ID or its name * @param options - Folder scoping (`folderId` / `folderKey` / `folderPath`) and optional parameters for regex filtering, query options, and pagination * {@link BucketGetFilesOptions} * @returns Promise resolving to either an array of files NonPaginatedResponse or a PaginatedResponse when pagination options are used. * {@link BucketFile} * @example * ```typescript - * // List all files in the bucket + * // By bucket ID * const files = await buckets.getFiles(, { folderId: }); * + * // By bucket name + * const filesByName = await buckets.getFiles('MyBucket', { folderId: }); + * * // Filter by regex pattern * const pdfs = await buckets.getFiles(, { * folderId: , @@ -311,7 +358,7 @@ export interface BucketServiceModel { * ``` */ getFiles( - bucketId: number, + bucket: number | string, options?: T ): Promise< T extends HasPaginationOptions diff --git a/src/models/orchestrator/buckets.types.ts b/src/models/orchestrator/buckets.types.ts index b2f052866..115704c24 100644 --- a/src/models/orchestrator/buckets.types.ts +++ b/src/models/orchestrator/buckets.types.ts @@ -24,7 +24,12 @@ export interface BucketGetResponse { } export type BucketGetAllOptions = RequestOptions & PaginationOptions & { + /** Numeric folder Id. Sent as `X-UIPATH-OrganizationUnitId`. */ folderId?: number; + /** Folder key (GUID). Sent as `X-UIPATH-FolderKey`. */ + folderKey?: string; + /** Slash-delimited folder path (e.g. `'Shared/Finance'`). Sent as `X-UIPATH-FolderPath-Encoded`. */ + folderPath?: string; } export interface BucketGetByIdOptions extends BaseOptions {} diff --git a/src/services/folder-scoped.ts b/src/services/folder-scoped.ts index 38816c491..194afcdbd 100644 --- a/src/services/folder-scoped.ts +++ b/src/services/folder-scoped.ts @@ -10,7 +10,8 @@ import { resolveFolderHeaders } from '../utils/folder/folder-headers'; /** * Matches single-quote characters in OData string literals — escaped to `''` - * inside the `$filter=Name eq '…'` clause built by `getByNameLookup`. + * inside the `$filter=Name eq '…'` clause built by the shared name-lookup + * helper. */ const SINGLE_QUOTE_RE = /'/g; @@ -93,41 +94,112 @@ export class FolderScopedService extends BaseService { transform: (raw: TRaw) => T, responseFieldMap?: FieldMapping, ): Promise { - const validatedName = validateName(resourceType, name); const { folderId, folderKey, folderPath, ...queryOptions } = options; - const headers = resolveFolderHeaders({ - folderId, - folderKey, - folderPath, - resourceType: `${resourceType}.getByName`, - fallbackFolderKey: this.config.folderKey, - }); - const apiFieldOptions = responseFieldMap ? transformOptions(queryOptions, responseFieldMap) : queryOptions; + const extraParams = addPrefixToKeys(apiFieldOptions, ODATA_PREFIX, Object.keys(apiFieldOptions)); + + return this.findByNameInFolder({ + resourceType, + endpoint, + name, + folder: { folderId, folderKey, folderPath }, + callerLabel: `${resourceType}.getByName`, + fallbackFolderKey: this.config.folderKey, + extraParams, + mapItem: transform, + }); + } + + /** + * Resolves a resource's numeric `Id` from its `Name` within a folder scope. + * + * Used by service methods that accept `: number | string` as their + * first argument. When the caller passes a string, the method calls this + * helper to obtain the numeric ID before continuing with the existing + * ID-based flow. + * + * Issues a lean OData request that projects only `Id` (`$select=Id&$top=1`), + * so the response is small and no field-rename transform is required. + * + * @param resourceType - Resource label used in validation + error messages (e.g. 'Bucket', 'Asset') + * @param endpoint - Folder-scoped OData collection endpoint + * @param name - Resource name to resolve + * @param folder - Folder scoping (`folderId` / `folderKey` / `folderPath`); at least one required unless SDK init-time `folderKey` is set + * @param callerLabel - Label used in header resolution (e.g. 'Buckets.deleteFile') for error context + * @throws ValidationError when the name is empty or the folder scope is unresolvable + * @throws NotFoundError when the OData response contains no matching resource + */ + protected async resolveIdByName( + resourceType: string, + endpoint: string, + name: string, + folder: { folderId?: number; folderKey?: string; folderPath?: string }, + callerLabel: string, + ): Promise { + return this.findByNameInFolder<{ Id: number }, number>({ + resourceType, + endpoint, + name, + folder, + callerLabel, + fallbackFolderKey: this.config.folderKey, + extraParams: { '$select': 'Id' }, + mapItem: (raw) => raw.Id, + }); + } + + /** + * Core name-lookup building block: validates the name, resolves folder + * headers, issues the `$filter=Name eq '…'&$top=1` OData query, maps the + * single result, and throws `NotFoundError` when the response is empty. + * + * Not exposed to services directly — callers go through `getByNameLookup` + * (full-resource fetch) or `resolveIdByName` (Id-only projection). Both + * differ only in `extraParams` (`$select`) and `mapItem`. + */ + private async findByNameInFolder(args: { + resourceType: string; + endpoint: string; + name: string; + folder: { folderId?: number; folderKey?: string; folderPath?: string }; + callerLabel: string; + fallbackFolderKey?: string; + extraParams?: Record; + mapItem: (raw: TRaw) => TOut; + }): Promise { + const validatedName = validateName(args.resourceType, args.name); + + const headers = resolveFolderHeaders({ + folderId: args.folder.folderId, + folderKey: args.folder.folderKey, + folderPath: args.folder.folderPath, + resourceType: args.callerLabel, + fallbackFolderKey: args.fallbackFolderKey, + }); - const apiOptions = { - ...addPrefixToKeys(apiFieldOptions, ODATA_PREFIX, Object.keys(apiFieldOptions)), + const params = { + ...args.extraParams, '$filter': `Name eq '${validatedName.replace(SINGLE_QUOTE_RE, "''")}'`, '$top': '1', }; - const response = await this.get>(endpoint, { + const response = await this.get>(args.endpoint, { headers, - params: apiOptions, + params, }); const items = response.data?.value; if (!items?.length) { - const folderHint = describeFolderForError(folderId, folderKey, folderPath); + const folderHint = describeFolderForError(args.folder.folderId, args.folder.folderKey, args.folder.folderPath); throw new NotFoundError({ - message: `${resourceType} '${validatedName}' not found${folderHint}.`, + message: `${args.resourceType} '${validatedName}' not found${folderHint}.`, }); } - return transform(items[0]); + return args.mapItem(items[0]); } } diff --git a/src/services/orchestrator/buckets/buckets.ts b/src/services/orchestrator/buckets/buckets.ts index b16f20694..ed172c5dc 100644 --- a/src/services/orchestrator/buckets/buckets.ts +++ b/src/services/orchestrator/buckets/buckets.ts @@ -82,29 +82,50 @@ export class BucketService extends FolderScopedService implements BucketServiceM : NonPaginatedResponse > { // Transformation function for buckets - const transformBucketResponse = (bucket: any) => + const transformBucketResponse = (bucket: any) => pascalToCamelCaseKeys(bucket) as BucketGetResponse; + // Pull folderKey/folderPath out of options — the pagination helper only + // recognizes folderId, so we resolve string-based folder scoping here and + // pass pre-built headers through. Passing them through as query options + // would leak them into the URL as OData params. + const { folderKey, folderPath, ...restOptions } = (options ?? {}) as + T & { folderKey?: string; folderPath?: string }; + const folderId = (restOptions as { folderId?: number }).folderId; + + const wantsFolder = folderId !== undefined || folderKey !== undefined || folderPath !== undefined; + + // No meta-tag folderKey fallback: `getAll()` with no folder options + // deliberately remains a cross-folder query, matching the pre-existing + // public contract. + const headers = wantsFolder + ? resolveFolderHeaders({ folderId, folderKey, folderPath, resourceType: 'Buckets.getAll' }) + : undefined; + return PaginationHelpers.getAll({ serviceAccess: this.createPaginationServiceAccess(), - getEndpoint: (folderId) => folderId ? BUCKET_ENDPOINTS.GET_BY_FOLDER : BUCKET_ENDPOINTS.GET_ALL, + // Endpoint choice is driven by `wantsFolder`, not the folderId the helper + // hands us — folderKey/folderPath don't surface as a numeric folderId, + // so we can't rely on the helper's default (folderId ? by-folder : all). + getEndpoint: () => wantsFolder ? BUCKET_ENDPOINTS.GET_BY_FOLDER : BUCKET_ENDPOINTS.GET_ALL, getByFolderEndpoint: BUCKET_ENDPOINTS.GET_BY_FOLDER, transformFn: transformBucketResponse, + headers, pagination: { paginationType: PaginationType.OFFSET, itemsField: ODATA_PAGINATION.ITEMS_FIELD, totalCountField: ODATA_PAGINATION.TOTAL_COUNT_FIELD, paginationParams: { - pageSizeParam: ODATA_OFFSET_PARAMS.PAGE_SIZE_PARAM, - offsetParam: ODATA_OFFSET_PARAMS.OFFSET_PARAM, - countParam: ODATA_OFFSET_PARAMS.COUNT_PARAM + pageSizeParam: ODATA_OFFSET_PARAMS.PAGE_SIZE_PARAM, + offsetParam: ODATA_OFFSET_PARAMS.OFFSET_PARAM, + countParam: ODATA_OFFSET_PARAMS.COUNT_PARAM } } - }, options) as any; + }, restOptions as T) as any; } getFileMetaData( - bucketId: number, + bucket: number | string, options?: T, ): Promise< T extends HasPaginationOptions @@ -112,7 +133,7 @@ export class BucketService extends FolderScopedService implements BucketServiceM : NonPaginatedResponse >; getFileMetaData( - bucketId: number, + bucket: number | string, folderId: number, options?: T, ): Promise< @@ -122,7 +143,7 @@ export class BucketService extends FolderScopedService implements BucketServiceM >; @track('Buckets.GetFileMetaData') async getFileMetaData( - bucketId: number, + bucket: number | string, optionsOrFolderId?: T | number, legacyOptions?: T, ): Promise< @@ -130,10 +151,6 @@ export class BucketService extends FolderScopedService implements BucketServiceM ? PaginatedResponse : NonPaginatedResponse > { - if (!bucketId) { - throw new ValidationError({ message: 'bucketId is required for getFileMetaData' }); - } - // Normalize the two overload forms into a single internal shape. let folderId: number | undefined; let folderKey: string | undefined; @@ -141,15 +158,21 @@ export class BucketService extends FolderScopedService implements BucketServiceM let restOptions: Omit; if (typeof optionsOrFolderId === 'number') { - // Deprecated positional form: getFileMetaData(bucketId, folderId, options?) + // Deprecated positional form: getFileMetaData(bucket, folderId, options?) folderId = optionsOrFolderId; restOptions = (legacyOptions ?? {}) as Omit; } else { - // Preferred form: getFileMetaData(bucketId, options?) + // Preferred form: getFileMetaData(bucket, options?) const opts = optionsOrFolderId ?? ({} as T); ({ folderId, folderKey, folderPath, ...restOptions } = opts); } + const bucketId = await this.coerceBucketId( + bucket, + { folderId, folderKey, folderPath }, + 'Buckets.getFileMetaData', + ); + const headers = resolveFolderHeaders({ folderId, folderKey, @@ -185,7 +208,7 @@ export class BucketService extends FolderScopedService implements BucketServiceM } uploadFile( - bucketId: number, + bucket: number | string, path: string, content: Blob | Uint8Array | File, options?: BucketUploadFileRequestOptions, @@ -193,35 +216,32 @@ export class BucketService extends FolderScopedService implements BucketServiceM uploadFile(options: BucketUploadFileOptions): Promise; @track('Buckets.UploadFile') async uploadFile( - bucketIdOrOptions: number | BucketUploadFileOptions, + bucketOrOptions: number | string | BucketUploadFileOptions, path?: string, content?: Blob | Uint8Array | File, options?: BucketUploadFileRequestOptions, ): Promise { // Normalize the two overload forms into a single internal shape. - let bucketId: number; + let bucket: number | string; let resolvedPath: string; let resolvedContent: Blob | Uint8Array | File; let folderId: number | undefined; let folderKey: string | undefined; let folderPath: string | undefined; - if (bucketIdOrOptions !== null && typeof bucketIdOrOptions === 'object') { + if (bucketOrOptions !== null && typeof bucketOrOptions === 'object') { // Deprecated options-only form: uploadFile({ bucketId, path, content, ... }) - ({ bucketId, path: resolvedPath, content: resolvedContent, folderId, folderKey, folderPath } = bucketIdOrOptions); + // Deprecated form remains numeric-only for bucketId. + ({ bucketId: bucket, path: resolvedPath, content: resolvedContent, folderId, folderKey, folderPath } = bucketOrOptions); } else { - // Preferred positional form: uploadFile(bucketId, path, content, options?) - bucketId = bucketIdOrOptions; + // Preferred positional form: uploadFile(bucket, path, content, options?) + bucket = bucketOrOptions; resolvedPath = path as string; resolvedContent = content as Blob | Uint8Array | File; const opts = options ?? ({} as BucketUploadFileRequestOptions); ({ folderId, folderKey, folderPath } = opts); } - if (!bucketId) { - throw new ValidationError({ message: 'bucketId is required for uploadFile' }); - } - if (!resolvedPath) { throw new ValidationError({ message: 'path is required for uploadFile' }); } @@ -230,6 +250,12 @@ export class BucketService extends FolderScopedService implements BucketServiceM throw new ValidationError({ message: 'content is required for uploadFile' }); } + const bucketId = await this.coerceBucketId( + bucket, + { folderId, folderKey, folderPath }, + 'Buckets.uploadFile', + ); + const headers = resolveFolderHeaders({ folderId, folderKey, @@ -254,19 +280,19 @@ export class BucketService extends FolderScopedService implements BucketServiceM } getReadUri( - bucketId: number, + bucket: number | string, path: string, options?: BucketGetReadUriRequestOptions, ): Promise; getReadUri(options: BucketGetReadUriOptions): Promise; @track('Buckets.GetReadUri') async getReadUri( - bucketIdOrOptions: number | BucketGetReadUriOptions, + bucketOrOptions: number | string | BucketGetReadUriOptions, path?: string, options?: BucketGetReadUriRequestOptions, ): Promise { // Normalize the two overload forms into a single internal shape. - let bucketId: number; + let bucket: number | string; let resolvedPath: string; let folderId: number | undefined; let folderKey: string | undefined; @@ -274,10 +300,11 @@ export class BucketService extends FolderScopedService implements BucketServiceM let expiryInMinutes: number | undefined; let restOptions: Record; - if (bucketIdOrOptions !== null && typeof bucketIdOrOptions === 'object') { + if (bucketOrOptions !== null && typeof bucketOrOptions === 'object') { // Deprecated options-only form: getReadUri({ bucketId, path, ... }) - const { bucketId: bid, path: p, expiryInMinutes: e, folderId: fid, folderKey: fkey, folderPath: fpath, ...rest } = bucketIdOrOptions; - bucketId = bid; + // Deprecated form remains numeric-only for bucketId. + const { bucketId: bid, path: p, expiryInMinutes: e, folderId: fid, folderKey: fkey, folderPath: fpath, ...rest } = bucketOrOptions; + bucket = bid; resolvedPath = p; expiryInMinutes = e; folderId = fid; @@ -285,13 +312,19 @@ export class BucketService extends FolderScopedService implements BucketServiceM folderPath = fpath; restOptions = rest; } else { - // Preferred positional form: getReadUri(bucketId, path, options?) - bucketId = bucketIdOrOptions; + // Preferred positional form: getReadUri(bucket, path, options?) + bucket = bucketOrOptions; resolvedPath = path as string; const opts = options ?? ({} as BucketGetReadUriRequestOptions); ({ expiryInMinutes, folderId, folderKey, folderPath, ...restOptions } = opts); } + const bucketId = await this.coerceBucketId( + bucket, + { folderId, folderKey, folderPath }, + 'Buckets.getReadUri', + ); + const headers = resolveFolderHeaders({ folderId, folderKey, @@ -400,19 +433,21 @@ export class BucketService extends FolderScopedService implements BucketServiceM @track('Buckets.GetFiles') async getFiles( - bucketId: number, + bucket: number | string, options?: T ): Promise< T extends HasPaginationOptions ? PaginatedResponse : NonPaginatedResponse > { - if (!bucketId) { - throw new ValidationError({ message: 'bucketId is required for getFiles' }); - } - const { folderId, folderKey, folderPath, ...restOptions } = options ?? {} as BucketGetFilesOptions; + const bucketId = await this.coerceBucketId( + bucket, + { folderId, folderKey, folderPath }, + 'Buckets.getFiles', + ); + const headers = resolveFolderHeaders({ folderId, folderKey, @@ -448,19 +483,23 @@ export class BucketService extends FolderScopedService implements BucketServiceM } @track('Buckets.DeleteFile') - async deleteFile(bucketId: number, path: string, options?: BucketDeleteFileOptions): Promise { - if (!bucketId) { - throw new ValidationError({ message: 'bucketId is required for deleteFile' }); - } - + async deleteFile(bucket: number | string, path: string, options?: BucketDeleteFileOptions): Promise { if (!path) { throw new ValidationError({ message: 'path is required for deleteFile' }); } + const { folderId, folderKey, folderPath } = options ?? {}; + + const bucketId = await this.coerceBucketId( + bucket, + { folderId, folderKey, folderPath }, + 'Buckets.deleteFile', + ); + const headers = resolveFolderHeaders({ - folderId: options?.folderId, - folderKey: options?.folderKey, - folderPath: options?.folderPath, + folderId, + folderKey, + folderPath, resourceType: 'Buckets.deleteFile', fallbackFolderKey: this.config.folderKey, }); @@ -499,4 +538,35 @@ export class BucketService extends FolderScopedService implements BucketServiceM queryOptions ); } + + /** + * Coerces a numeric bucket ID or a bucket name into a numeric ID. When a + * name is supplied, resolves it via an OData lookup on the folder-scoped + * Buckets collection. Callers pass the folder scope so name lookups run in + * the same folder used for the subsequent file operation. + */ + private async coerceBucketId( + bucket: number | string, + folder: { folderId?: number; folderKey?: string; folderPath?: string }, + callerLabel: string, + ): Promise { + if (bucket === null || bucket === undefined) { + throw new ValidationError({ message: `bucket is required for ${callerLabel}` }); + } + if (typeof bucket === 'number') { + if (bucket <= 0) { + throw new ValidationError({ + message: `bucket must be a positive numeric Id for ${callerLabel} (got ${bucket})`, + }); + } + return bucket; + } + return this.resolveIdByName( + 'Bucket', + BUCKET_ENDPOINTS.GET_BY_FOLDER, + bucket, + folder, + callerLabel, + ); + } } diff --git a/tests/integration/shared/orchestrator/buckets.integration.test.ts b/tests/integration/shared/orchestrator/buckets.integration.test.ts index b50ba634f..56b4d1f6d 100644 --- a/tests/integration/shared/orchestrator/buckets.integration.test.ts +++ b/tests/integration/shared/orchestrator/buckets.integration.test.ts @@ -91,6 +91,28 @@ describe.each(modes)('Orchestrator Buckets - Integration Tests [%s]', (mode) => expect(Array.isArray(result.items)).toBe(true); expect(result.items.length).toBeLessThanOrEqual(10); }); + + it('should accept folderKey to scope by folder', async () => { + const { buckets } = getServices(); + const config = getTestConfig(); + if (!config.folderKey) throw new Error('INTEGRATION_TEST_FOLDER_KEY must be configured for folderKey getAll test'); + + const result = await buckets.getAll({ folderKey: config.folderKey }); + + expect(result).toBeDefined(); + expect(Array.isArray(result.items)).toBe(true); + }); + + it('should accept folderPath to scope by folder', async () => { + const { buckets } = getServices(); + const config = getTestConfig(); + if (!config.folderPath) throw new Error('INTEGRATION_TEST_FOLDER_PATH must be configured for folderPath getAll test'); + + const result = await buckets.getAll({ folderPath: config.folderPath }); + + expect(result).toBeDefined(); + expect(Array.isArray(result.items)).toBe(true); + }); }); describe('getById', () => { @@ -317,6 +339,65 @@ describe.each(modes)('Orchestrator Buckets - Integration Tests [%s]', (mode) => }); }); + describe('File operations - by bucket name', () => { + let bucketName!: string; + let bucketId!: number; + let folderId!: number; + + beforeAll(async () => { + const { buckets } = getServices(); + folderId = getFolderId()!; + if (!folderId) throw new Error('INTEGRATION_TEST_FOLDER_ID must be configured for by-name tests'); + + const all = await buckets.getAll({ folderId, pageSize: 1 }); + if (!all.items.length) throw new Error('No buckets available for by-name tests'); + + bucketId = all.items[0].id; + bucketName = all.items[0].name; + }); + + it('uploadFile → getFileMetaData → getReadUri → deleteFile all accept a bucket name', async () => { + const { buckets } = getServices(); + const fileName = `/integration-byname-${mode}-${Date.now()}.txt`; + const buffer = Buffer.from(createTestFileContent(fileName), 'utf-8'); + + const uploadResult = await buckets.uploadFile(bucketName, fileName, buffer, { folderId }); + trackUploadedFile(bucketId, fileName, folderId); + expect(uploadResult.success).toBe(true); + + const metadata = await buckets.getFileMetaData(bucketName, { folderId, prefix: fileName }); + expect(metadata.items.some((f: any) => f.path === fileName)).toBe(true); + + const readUri = await buckets.getReadUri(bucketName, fileName, { folderId }); + expect(readUri.uri).toMatch(/^https?:\/\/.+/); + + await buckets.deleteFile(bucketName, fileName, { folderId }); + untrackUploadedFile(fileName); + }); + + it('getFiles accepts a bucket name', async () => { + const { buckets } = getServices(); + const result = await buckets.getFiles(bucketName, { folderId }); + expect(Array.isArray(result.items)).toBe(true); + }); + + it('should resolve the bucket name using folderKey scoping', async () => { + const { buckets } = getServices(); + const config = getTestConfig(); + if (!config.folderKey) throw new Error('INTEGRATION_TEST_FOLDER_KEY must be configured for the by-name folderKey test'); + + const result = await buckets.getFiles(bucketName, { folderKey: config.folderKey }); + expect(Array.isArray(result.items)).toBe(true); + }); + + it('should throw NotFoundError when a bucket name does not exist', async () => { + const { buckets } = getServices(); + const missing = `__missing-bucket-${mode}-${Date.now()}__`; + await expect(buckets.getFiles(missing, { folderId })) + .rejects.toSatisfy(isNotFoundError); + }); + }); + describe('File operations - folderKey scoping', () => { let bucketId!: number; let folderId!: number; diff --git a/tests/unit/services/orchestrator/buckets.test.ts b/tests/unit/services/orchestrator/buckets.test.ts index 67f52b47f..015b6465b 100644 --- a/tests/unit/services/orchestrator/buckets.test.ts +++ b/tests/unit/services/orchestrator/buckets.test.ts @@ -337,13 +337,58 @@ describe('BucketService Unit Tests', () => { totalCountField: ODATA_PAGINATION.TOTAL_COUNT_FIELD, }) }), - undefined + expect.anything(), ); + // With no folder in options, use the cross-folder endpoint and send no folder headers. + const [config] = vi.mocked(PaginationHelpers.getAll).mock.calls.at(-1)!; + expect((config as any).getEndpoint()).toBe(BUCKET_ENDPOINTS.GET_ALL); + expect((config as any).headers).toBeUndefined(); + expect(result).toEqual(mockResponse); expect(result.items).toHaveLength(3); }); + it('should route folderKey to the FolderKey header and use the folder-scoped endpoint', async () => { + vi.mocked(PaginationHelpers.getAll).mockResolvedValue({ items: [], totalCount: 0 }); + + await bucketService.getAll({ folderKey: BUCKET_TEST_CONSTANTS.FOLDER_KEY }); + + const [config, restOptions] = vi.mocked(PaginationHelpers.getAll).mock.calls.at(-1)!; + expect((config as any).headers).toMatchObject({ + [FOLDER_KEY]: BUCKET_TEST_CONSTANTS.FOLDER_KEY, + }); + expect((config as any).getEndpoint()).toBe(BUCKET_ENDPOINTS.GET_BY_FOLDER); + // Stripped from the OData pass-through options before delegation. + expect(restOptions).not.toHaveProperty('folderKey'); + }); + + it('should route folderPath to the encoded FolderPath header', async () => { + vi.mocked(PaginationHelpers.getAll).mockResolvedValue({ items: [], totalCount: 0 }); + + await bucketService.getAll({ folderPath: BUCKET_TEST_CONSTANTS.FOLDER_PATH_WITH_SPACE }); + + const [config, restOptions] = vi.mocked(PaginationHelpers.getAll).mock.calls.at(-1)!; + expect((config as any).headers).toMatchObject({ + [FOLDER_PATH_ENCODED]: BUCKET_TEST_CONSTANTS.FOLDER_PATH_WITH_SPACE_ENCODED, + }); + expect((config as any).getEndpoint()).toBe(BUCKET_ENDPOINTS.GET_BY_FOLDER); + expect(restOptions).not.toHaveProperty('folderPath'); + }); + + it('should NOT fall back to SDK init-time folderKey when no folder is in options (cross-folder preserved)', async () => { + const { instance } = createServiceTestDependencies({ folderKey: BUCKET_TEST_CONSTANTS.FOLDER_KEY }); + vi.mocked(ApiClient).mockImplementation(function () { return mockApiClient; }); + const scopedService = new BucketService(instance); + vi.mocked(PaginationHelpers.getAll).mockResolvedValue({ items: [], totalCount: 0 }); + + await scopedService.getAll(); + + const [config] = vi.mocked(PaginationHelpers.getAll).mock.calls.at(-1)!; + expect((config as any).headers).toBeUndefined(); + expect((config as any).getEndpoint()).toBe(BUCKET_ENDPOINTS.GET_ALL); + }); + it('should return paginated buckets when pagination options provided', async () => { const mockBuckets = createMockBuckets(10); const mockResponse = { @@ -592,9 +637,9 @@ describe('BucketService Unit Tests', () => { ); }); - it('should throw ValidationError when bucketId is missing', async () => { + it('should throw ValidationError when bucket is missing', async () => { await expect(bucketService.getFileMetaData(null as any, TEST_CONSTANTS.FOLDER_ID)) - .rejects.toThrow('bucketId is required for getFileMetaData'); + .rejects.toThrow('bucket is required for Buckets.getFileMetaData'); }); it('should throw ValidationError when no folder context can be resolved', async () => { @@ -850,13 +895,13 @@ describe('BucketService Unit Tests', () => { }); - it('should throw ValidationError when bucketId is missing', async () => { + it('should throw ValidationError when bucket is missing', async () => { await expect(bucketService.uploadFile({ bucketId: null as any, folderId: TEST_CONSTANTS.FOLDER_ID, path: BUCKET_TEST_CONSTANTS.FILE_PATH, content: new Blob([BUCKET_TEST_CONSTANTS.FILE_CONTENT]) - })).rejects.toThrow('bucketId is required for uploadFile'); + })).rejects.toThrow('bucket is required for Buckets.uploadFile'); }); it('should throw ValidationError when no folder context can be resolved', async () => { @@ -1062,7 +1107,7 @@ describe('BucketService Unit Tests', () => { } }); - it('should throw ValidationError when positional bucketId is missing', async () => { + it('should throw ValidationError when positional bucket is missing', async () => { await expect( bucketService.uploadFile( null as any, @@ -1070,7 +1115,7 @@ describe('BucketService Unit Tests', () => { new Blob([BUCKET_TEST_CONSTANTS.FILE_CONTENT]), { folderId: TEST_CONSTANTS.FOLDER_ID }, ), - ).rejects.toThrow('bucketId is required for uploadFile'); + ).rejects.toThrow('bucket is required for Buckets.uploadFile'); }); it('should throw ValidationError when positional path is missing', async () => { @@ -1129,12 +1174,12 @@ describe('BucketService Unit Tests', () => { expect(result.headers).toEqual(BUCKET_TEST_CONSTANTS.BLOB_HEADERS); }); - it('should throw ValidationError when bucketId is missing', async () => { + it('should throw ValidationError when bucket is missing', async () => { await expect(bucketService.getReadUri({ bucketId: null as any, folderId: TEST_CONSTANTS.FOLDER_ID, path: BUCKET_TEST_CONSTANTS.FILE_PATH - })).rejects.toThrow('bucketId is required for getUri'); + })).rejects.toThrow('bucket is required for Buckets.getReadUri'); }); it('should throw ValidationError when no folder context can be resolved', async () => { @@ -1310,14 +1355,14 @@ describe('BucketService Unit Tests', () => { ); }); - it('should throw ValidationError when positional bucketId is missing', async () => { + it('should throw ValidationError when positional bucket is missing', async () => { await expect( bucketService.getReadUri( null as any, BUCKET_TEST_CONSTANTS.FILE_PATH, { folderId: TEST_CONSTANTS.FOLDER_ID }, ), - ).rejects.toThrow('bucketId is required for getUri'); + ).rejects.toThrow('bucket is required for Buckets.getReadUri'); }); it('should throw ValidationError when positional path is missing', async () => { @@ -1371,12 +1416,12 @@ describe('BucketService Unit Tests', () => { ); }); - it('should throw ValidationError when bucketId is missing', async () => { + it('should throw ValidationError when bucket is missing', async () => { await expect(bucketService.deleteFile( null as any, BUCKET_TEST_CONSTANTS.FILE_PATH, { folderId: TEST_CONSTANTS.FOLDER_ID }, - )).rejects.toThrow('bucketId is required for deleteFile'); + )).rejects.toThrow('bucket is required for Buckets.deleteFile'); expect(mockApiClient.delete).not.toHaveBeenCalled(); }); @@ -1553,9 +1598,9 @@ describe('BucketService Unit Tests', () => { expect((file as any).fullPath).toBeUndefined(); }); - it('should throw ValidationError when bucketId is missing', async () => { + it('should throw ValidationError when bucket is missing', async () => { await expect(bucketService.getFiles(null as any, { folderId: TEST_CONSTANTS.FOLDER_ID })) - .rejects.toThrow('bucketId is required for getFiles'); + .rejects.toThrow('bucket is required for Buckets.getFiles'); expect(PaginationHelpers.getAll).not.toHaveBeenCalled(); }); @@ -1592,5 +1637,197 @@ describe('BucketService Unit Tests', () => { ); }); }); + + describe('bucket name resolution', () => { + const nameLookupResponse = { value: [{ Id: BUCKET_TEST_CONSTANTS.BUCKET_ID }] }; + const emptyLookupResponse = { value: [] }; + + it('deleteFile should resolve a bucket name to its Id before deleting', async () => { + mockApiClient.get.mockResolvedValueOnce(nameLookupResponse); + mockApiClient.delete.mockResolvedValue(undefined); + + await bucketService.deleteFile( + BUCKET_TEST_CONSTANTS.BUCKET_NAME, + BUCKET_TEST_CONSTANTS.FILE_PATH, + { folderId: TEST_CONSTANTS.FOLDER_ID }, + ); + + // Resolver hit /odata/Buckets with $filter/$select/$top + expect(mockApiClient.get).toHaveBeenCalledWith( + BUCKET_ENDPOINTS.GET_BY_FOLDER, + expect.objectContaining({ + params: expect.objectContaining({ + '$filter': `Name eq '${BUCKET_TEST_CONSTANTS.BUCKET_NAME}'`, + '$select': 'Id', + '$top': '1', + }), + headers: expect.objectContaining({ + [FOLDER_ID]: TEST_CONSTANTS.FOLDER_ID.toString(), + }), + }), + ); + + // Downstream DELETE used the resolved numeric Id + expect(mockApiClient.delete).toHaveBeenCalledWith( + BUCKET_ENDPOINTS.DELETE_FILE(BUCKET_TEST_CONSTANTS.BUCKET_ID), + expect.objectContaining({ + params: { path: BUCKET_TEST_CONSTANTS.FILE_PATH }, + }), + ); + }); + + it('deleteFile should throw NotFoundError when the bucket name is missing', async () => { + mockApiClient.get.mockResolvedValueOnce(emptyLookupResponse); + + await expect(bucketService.deleteFile( + BUCKET_TEST_CONSTANTS.MISSING_BUCKET_NAME, + BUCKET_TEST_CONSTANTS.FILE_PATH, + { folderId: TEST_CONSTANTS.FOLDER_ID }, + )).rejects.toBeInstanceOf(NotFoundError); + expect(mockApiClient.delete).not.toHaveBeenCalled(); + }); + + it('getFiles should resolve a bucket name to its Id before listing files', async () => { + mockApiClient.get.mockResolvedValueOnce(nameLookupResponse); + vi.mocked(PaginationHelpers.getAll).mockResolvedValue({ + items: [], + totalCount: 0, + hasNextPage: false, + } as any); + + await bucketService.getFiles( + BUCKET_TEST_CONSTANTS.BUCKET_NAME, + { folderId: TEST_CONSTANTS.FOLDER_ID }, + ); + + expect(mockApiClient.get).toHaveBeenCalledWith( + BUCKET_ENDPOINTS.GET_BY_FOLDER, + expect.objectContaining({ + params: expect.objectContaining({ + '$filter': `Name eq '${BUCKET_TEST_CONSTANTS.BUCKET_NAME}'`, + '$select': 'Id', + }), + }), + ); + + expect(PaginationHelpers.getAll).toHaveBeenCalled(); + const [config] = vi.mocked(PaginationHelpers.getAll).mock.calls[0]; + expect((config as any).getEndpoint()).toBe( + BUCKET_ENDPOINTS.GET_FILES(BUCKET_TEST_CONSTANTS.BUCKET_ID), + ); + }); + + it('getFileMetaData should resolve a bucket name to its Id before listing metadata', async () => { + mockApiClient.get.mockResolvedValueOnce(nameLookupResponse); + vi.mocked(PaginationHelpers.getAll).mockResolvedValue({ + items: [], + totalCount: 0, + hasNextPage: false, + } as any); + + await bucketService.getFileMetaData( + BUCKET_TEST_CONSTANTS.BUCKET_NAME, + { folderId: TEST_CONSTANTS.FOLDER_ID }, + ); + + const [config] = vi.mocked(PaginationHelpers.getAll).mock.calls[0]; + expect((config as any).getEndpoint()).toBe( + BUCKET_ENDPOINTS.GET_FILE_META_DATA(BUCKET_TEST_CONSTANTS.BUCKET_ID), + ); + }); + + it('getReadUri should resolve a bucket name to its Id before fetching URI', async () => { + // First get is the name lookup, second is _getUri + mockApiClient.get + .mockResolvedValueOnce(nameLookupResponse) + .mockResolvedValueOnce(createMockReadUriApiResponse()); + + await bucketService.getReadUri( + BUCKET_TEST_CONSTANTS.BUCKET_NAME, + BUCKET_TEST_CONSTANTS.FILE_PATH, + { folderId: TEST_CONSTANTS.FOLDER_ID }, + ); + + expect(mockApiClient.get).toHaveBeenNthCalledWith( + 2, + BUCKET_ENDPOINTS.GET_READ_URI(BUCKET_TEST_CONSTANTS.BUCKET_ID), + expect.objectContaining({ + params: expect.objectContaining({ path: BUCKET_TEST_CONSTANTS.FILE_PATH }), + }), + ); + }); + + it('uploadFile should resolve a bucket name to its Id before uploading', async () => { + // First get is the name lookup, second is _getWriteUri + mockApiClient.get + .mockResolvedValueOnce(nameLookupResponse) + .mockResolvedValueOnce(createMockWriteUriApiResponse()); + + const originalFetch = globalThis.fetch; + globalThis.fetch = vi.fn().mockResolvedValue({ status: 200 } as Response); + try { + await bucketService.uploadFile( + BUCKET_TEST_CONSTANTS.BUCKET_NAME, + BUCKET_TEST_CONSTANTS.FILE_PATH, + new Blob([BUCKET_TEST_CONSTANTS.FILE_CONTENT]), + { folderId: TEST_CONSTANTS.FOLDER_ID }, + ); + } finally { + globalThis.fetch = originalFetch; + } + + expect(mockApiClient.get).toHaveBeenNthCalledWith( + 2, + BUCKET_ENDPOINTS.GET_WRITE_URI(BUCKET_TEST_CONSTANTS.BUCKET_ID), + expect.objectContaining({ + params: expect.objectContaining({ path: BUCKET_TEST_CONSTANTS.FILE_PATH }), + }), + ); + }); + + it('should skip the name-lookup GET when a numeric bucket id is passed', async () => { + mockApiClient.delete.mockResolvedValue(undefined); + + await bucketService.deleteFile( + BUCKET_TEST_CONSTANTS.BUCKET_ID, + BUCKET_TEST_CONSTANTS.FILE_PATH, + { folderId: TEST_CONSTANTS.FOLDER_ID }, + ); + + // No resolver call: numeric ids bypass the OData lookup. + expect(mockApiClient.get).not.toHaveBeenCalled(); + expect(mockApiClient.delete).toHaveBeenCalledWith( + BUCKET_ENDPOINTS.DELETE_FILE(BUCKET_TEST_CONSTANTS.BUCKET_ID), + expect.any(Object), + ); + }); + + it('should reject non-positive numeric bucket ids with a specific message', async () => { + await expect(bucketService.deleteFile( + 0, + BUCKET_TEST_CONSTANTS.FILE_PATH, + { folderId: TEST_CONSTANTS.FOLDER_ID }, + )).rejects.toThrow('bucket must be a positive numeric Id for Buckets.deleteFile'); + + await expect(bucketService.deleteFile( + -5, + BUCKET_TEST_CONSTANTS.FILE_PATH, + { folderId: TEST_CONSTANTS.FOLDER_ID }, + )).rejects.toThrow('bucket must be a positive numeric Id for Buckets.deleteFile'); + }); + + it('should propagate the resolver NotFoundError from uploadFile', async () => { + mockApiClient.get.mockResolvedValueOnce(emptyLookupResponse); + + await expect(bucketService.uploadFile( + BUCKET_TEST_CONSTANTS.MISSING_BUCKET_NAME, + BUCKET_TEST_CONSTANTS.FILE_PATH, + new Blob([BUCKET_TEST_CONSTANTS.FILE_CONTENT]), + { folderId: TEST_CONSTANTS.FOLDER_ID }, + )).rejects.toBeInstanceOf(NotFoundError); + // _getWriteUri was never reached + expect(mockApiClient.get).toHaveBeenCalledTimes(1); + }); + }); }); From 8b80aeaf8fd6c033a220aa60b3740a3b713b653f Mon Sep 17 00:00:00 2001 From: Deepesh Rai Date: Wed, 15 Jul 2026 23:55:52 +0530 Subject: [PATCH 2/2] fix(buckets): address review feedback on PR #607 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - `coerceBucketId`: reject `NaN` / `Infinity` via `Number.isFinite`, not just `<= 0`. `parseInt('bad', 10)` and similar no longer leak into the API URL. - Add "numeric id skips name lookup" regression guard for all four sibling methods (`uploadFile`, `getReadUri`, `getFileMetaData`, `getFiles`) — previously only `deleteFile` was covered. - Widen the NaN/Infinity validation test alongside the existing 0/-5 cases. - Type integration-test callbacks as `BlobItem` instead of `any`. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/services/orchestrator/buckets/buckets.ts | 2 +- .../orchestrator/buckets.integration.test.ts | 5 +- .../services/orchestrator/buckets.test.ts | 90 ++++++++++++++++++- 3 files changed, 92 insertions(+), 5 deletions(-) diff --git a/src/services/orchestrator/buckets/buckets.ts b/src/services/orchestrator/buckets/buckets.ts index ed172c5dc..26b590db0 100644 --- a/src/services/orchestrator/buckets/buckets.ts +++ b/src/services/orchestrator/buckets/buckets.ts @@ -554,7 +554,7 @@ export class BucketService extends FolderScopedService implements BucketServiceM throw new ValidationError({ message: `bucket is required for ${callerLabel}` }); } if (typeof bucket === 'number') { - if (bucket <= 0) { + if (!Number.isFinite(bucket) || bucket <= 0) { throw new ValidationError({ message: `bucket must be a positive numeric Id for ${callerLabel} (got ${bucket})`, }); diff --git a/tests/integration/shared/orchestrator/buckets.integration.test.ts b/tests/integration/shared/orchestrator/buckets.integration.test.ts index 56b4d1f6d..1cf82cc75 100644 --- a/tests/integration/shared/orchestrator/buckets.integration.test.ts +++ b/tests/integration/shared/orchestrator/buckets.integration.test.ts @@ -9,6 +9,7 @@ import { import { registerResource } from '../../utils/cleanup'; import { createTestFileContent } from '../../utils/helpers'; import { isNotFoundError } from '../../../../src/core/errors'; +import type { BlobItem } from '../../../../src/models/orchestrator/buckets.types'; const modes: InitMode[] = ['v0', 'v1']; @@ -232,7 +233,7 @@ describe.each(modes)('Orchestrator Buckets - Integration Tests [%s]', (mode) => expect(uploadResult.success).toBe(true); const metadata = await buckets.getFileMetaData(bucket.bucketId, bucket.folderId); - const uploadedFile = metadata.items.find((f: any) => f.path === fileName); + const uploadedFile = metadata.items.find((f: BlobItem) => f.path === fileName); if (uploadedFile) { expect(uploadedFile.path).toBe(fileName); @@ -366,7 +367,7 @@ describe.each(modes)('Orchestrator Buckets - Integration Tests [%s]', (mode) => expect(uploadResult.success).toBe(true); const metadata = await buckets.getFileMetaData(bucketName, { folderId, prefix: fileName }); - expect(metadata.items.some((f: any) => f.path === fileName)).toBe(true); + expect(metadata.items.some((f: BlobItem) => f.path === fileName)).toBe(true); const readUri = await buckets.getReadUri(bucketName, fileName, { folderId }); expect(readUri.uri).toMatch(/^https?:\/\/.+/); diff --git a/tests/unit/services/orchestrator/buckets.test.ts b/tests/unit/services/orchestrator/buckets.test.ts index 015b6465b..064677a3b 100644 --- a/tests/unit/services/orchestrator/buckets.test.ts +++ b/tests/unit/services/orchestrator/buckets.test.ts @@ -1785,7 +1785,7 @@ describe('BucketService Unit Tests', () => { ); }); - it('should skip the name-lookup GET when a numeric bucket id is passed', async () => { + it('deleteFile should skip the name-lookup GET when a numeric bucket id is passed', async () => { mockApiClient.delete.mockResolvedValue(undefined); await bucketService.deleteFile( @@ -1802,7 +1802,81 @@ describe('BucketService Unit Tests', () => { ); }); - it('should reject non-positive numeric bucket ids with a specific message', async () => { + it('getFiles should skip the name-lookup GET when a numeric bucket id is passed', async () => { + vi.mocked(PaginationHelpers.getAll).mockResolvedValue({ + items: [], + totalCount: 0, + hasNextPage: false, + } as any); + + await bucketService.getFiles( + BUCKET_TEST_CONSTANTS.BUCKET_ID, + { folderId: TEST_CONSTANTS.FOLDER_ID }, + ); + + expect(mockApiClient.get).not.toHaveBeenCalled(); + expect(PaginationHelpers.getAll).toHaveBeenCalled(); + }); + + it('getFileMetaData should skip the name-lookup GET when a numeric bucket id is passed', async () => { + vi.mocked(PaginationHelpers.getAll).mockResolvedValue({ + items: [], + totalCount: 0, + hasNextPage: false, + } as any); + + await bucketService.getFileMetaData( + BUCKET_TEST_CONSTANTS.BUCKET_ID, + { folderId: TEST_CONSTANTS.FOLDER_ID }, + ); + + expect(mockApiClient.get).not.toHaveBeenCalled(); + expect(PaginationHelpers.getAll).toHaveBeenCalled(); + }); + + it('getReadUri should skip the name-lookup GET when a numeric bucket id is passed', async () => { + // Only _getUri hits mockApiClient.get; the resolver would be an extra call. + mockApiClient.get.mockResolvedValue(createMockReadUriApiResponse()); + + await bucketService.getReadUri( + BUCKET_TEST_CONSTANTS.BUCKET_ID, + BUCKET_TEST_CONSTANTS.FILE_PATH, + { folderId: TEST_CONSTANTS.FOLDER_ID }, + ); + + // Exactly one GET (the URI fetch), no resolver call ahead of it. + expect(mockApiClient.get).toHaveBeenCalledTimes(1); + expect(mockApiClient.get).toHaveBeenCalledWith( + BUCKET_ENDPOINTS.GET_READ_URI(BUCKET_TEST_CONSTANTS.BUCKET_ID), + expect.any(Object), + ); + }); + + it('uploadFile should skip the name-lookup GET when a numeric bucket id is passed', async () => { + // Only _getWriteUri hits mockApiClient.get; the resolver would be extra. + mockApiClient.get.mockResolvedValue(createMockWriteUriApiResponse()); + + const originalFetch = globalThis.fetch; + globalThis.fetch = vi.fn().mockResolvedValue({ status: 200 } as Response); + try { + await bucketService.uploadFile( + BUCKET_TEST_CONSTANTS.BUCKET_ID, + BUCKET_TEST_CONSTANTS.FILE_PATH, + new Blob([BUCKET_TEST_CONSTANTS.FILE_CONTENT]), + { folderId: TEST_CONSTANTS.FOLDER_ID }, + ); + } finally { + globalThis.fetch = originalFetch; + } + + expect(mockApiClient.get).toHaveBeenCalledTimes(1); + expect(mockApiClient.get).toHaveBeenCalledWith( + BUCKET_ENDPOINTS.GET_WRITE_URI(BUCKET_TEST_CONSTANTS.BUCKET_ID), + expect.any(Object), + ); + }); + + it('should reject non-positive or non-finite numeric bucket ids with a specific message', async () => { await expect(bucketService.deleteFile( 0, BUCKET_TEST_CONSTANTS.FILE_PATH, @@ -1814,6 +1888,18 @@ describe('BucketService Unit Tests', () => { BUCKET_TEST_CONSTANTS.FILE_PATH, { folderId: TEST_CONSTANTS.FOLDER_ID }, )).rejects.toThrow('bucket must be a positive numeric Id for Buckets.deleteFile'); + + await expect(bucketService.deleteFile( + Number.NaN, + BUCKET_TEST_CONSTANTS.FILE_PATH, + { folderId: TEST_CONSTANTS.FOLDER_ID }, + )).rejects.toThrow('bucket must be a positive numeric Id for Buckets.deleteFile'); + + await expect(bucketService.deleteFile( + Number.POSITIVE_INFINITY, + BUCKET_TEST_CONSTANTS.FILE_PATH, + { folderId: TEST_CONSTANTS.FOLDER_ID }, + )).rejects.toThrow('bucket must be a positive numeric Id for Buckets.deleteFile'); }); it('should propagate the resolver NotFoundError from uploadFile', async () => {