From 0193f2a54895da5c50d653386693796ea624acb1 Mon Sep 17 00:00:00 2001 From: George Goranov Date: Tue, 21 Jul 2026 16:57:43 +0300 Subject: [PATCH 1/2] Fix sharing methods failing with 404 due to double-applied query params All sharing.* methods (listAssetShares, getAssetShare, shareAsset, updateAssetShare, deleteAssetShare) baked assetType/assetId into the request URL and then passed queryParameters through to the HTTP layer, which re-applied them via axios params. This produced a duplicated, malformed query string and a leading double-slash, causing the API to return 404 Not Found. URL construction now follows the reports module pattern: a path-only url plus queryParameters serialized once by the requestor. Also adds a patch method to httpRequestor so updateAssetShare (PATCH) works, and adds mock API tests for all five sharing endpoints. Fixes #195 Co-Authored-By: Claude Opus 4.8 (1M context) --- CHANGELOG.md | 5 + lib/sharing/index.ts | 53 ++---- lib/utils/httpRequestor.js | 3 + .../mock-api/sharing/common_test_constants.ts | 62 +++++++ .../sharing/delete_asset_share.spec.ts | 112 ++++++++++++ test/mock-api/sharing/get_asset_share.spec.ts | 127 ++++++++++++++ .../sharing/list_asset_shares.spec.ts | 139 +++++++++++++++ test/mock-api/sharing/share_asset.spec.ts | 162 ++++++++++++++++++ .../sharing/update_asset_share.spec.ts | 138 +++++++++++++++ 9 files changed, 760 insertions(+), 41 deletions(-) create mode 100644 test/mock-api/sharing/common_test_constants.ts create mode 100644 test/mock-api/sharing/delete_asset_share.spec.ts create mode 100644 test/mock-api/sharing/get_asset_share.spec.ts create mode 100644 test/mock-api/sharing/list_asset_shares.spec.ts create mode 100644 test/mock-api/sharing/share_asset.spec.ts create mode 100644 test/mock-api/sharing/update_asset_share.spec.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 487ce9d6..4b22a313 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,11 @@ and this project adheres to [Semantic Versioning](http://semver.org/). ## [X.X.X] - Unreleased +### Fixed + +- Fixed all `sharing` methods (`listAssetShares`, `getAssetShare`, `shareAsset`, `updateAssetShare`, `deleteAssetShare`) failing with `404 Not Found` because query parameters (`assetType`/`assetId`) were baked into the request URL and then re-applied by the HTTP layer, producing a duplicated, malformed query string. Query parameters are now applied once. ([#195](https://github.com/smartsheet/smartsheet-javascript-sdk/issues/195)) +- Fixed `sharing.updateAssetShare` throwing at runtime because the HTTP requestor did not expose a `patch` method. + ## [5.2.0] - 2026-07-09 ### Added diff --git a/lib/sharing/index.ts b/lib/sharing/index.ts index fd1dbe55..89a9ad58 100644 --- a/lib/sharing/index.ts +++ b/lib/sharing/index.ts @@ -202,7 +202,7 @@ export const createSharing = (options: CreateOptions): SharingApi => { }; // Base URL for shares endpoints - const baseUrl = '/shares'; + const baseUrl = 'shares'; /** * List all shares for a specified asset @@ -214,27 +214,9 @@ export const createSharing = (options: CreateOptions): SharingApi => { options: ListSharesOptions, callback?: RequestCallback ): Promise => { - const { queryParameters } = options; - - // Build the base URL with required parameters - const urlParams = new URLSearchParams(); - urlParams.append('assetType', queryParameters.assetType.toString()); - urlParams.append('assetId', queryParameters.assetId.toString()); - - // Add optional query parameters - if (queryParameters.maxItems) { - urlParams.append('maxItems', queryParameters.maxItems.toString()); - } - if (queryParameters.lastKey) { - urlParams.append('lastKey', queryParameters.lastKey); - } - if (queryParameters.sharingInclude) { - urlParams.append('sharingInclude', queryParameters.sharingInclude); - } - - const url = `${baseUrl}?${urlParams.toString()}`; - - return requestor.get({ ...optionsToSend, url, ...options }, callback); + const urlOptions = { url: baseUrl }; + + return requestor.get({ ...optionsToSend, ...urlOptions, ...options }, callback); }; /** @@ -247,11 +229,9 @@ export const createSharing = (options: CreateOptions): SharingApi => { options: GetShareOptions, callback?: RequestCallback ): Promise => { - const { shareId, queryParameters } = options; - - const url = `${baseUrl}/${shareId}?assetType=${queryParameters.assetType}&assetId=${queryParameters.assetId}`; + const urlOptions = { url: `${baseUrl}/${options.shareId}` }; - return requestor.get({ ...optionsToSend, url, ...options }, callback); + return requestor.get({ ...optionsToSend, ...urlOptions, ...options }, callback); }; /** @@ -261,14 +241,9 @@ export const createSharing = (options: CreateOptions): SharingApi => { * @returns Promise with shares success result */ const shareAsset = (options: ShareAssetOptions, callback?: RequestCallback): Promise => { - const { body, queryParameters } = options; - - let url = `${baseUrl}?assetType=${queryParameters.assetType}&assetId=${queryParameters.assetId}`; - if (queryParameters.sendEmail) { - url += '&sendEmail=true'; - } + const urlOptions = { url: baseUrl }; - return requestor.post({ ...optionsToSend, url, body, ...options }, callback); + return requestor.post({ ...optionsToSend, ...urlOptions, ...options }, callback); }; /** @@ -281,11 +256,9 @@ export const createSharing = (options: CreateOptions): SharingApi => { options: UpdateShareOptions, callback?: RequestCallback ): Promise => { - const { shareId, queryParameters, body } = options; + const urlOptions = { url: `${baseUrl}/${options.shareId}` }; - const url = `${baseUrl}/${shareId}?assetType=${queryParameters.assetType}&assetId=${queryParameters.assetId}`; - - return requestor.patch({ ...optionsToSend, url, body, ...options }, callback); + return requestor.patch({ ...optionsToSend, ...urlOptions, ...options }, callback); }; /** @@ -295,11 +268,9 @@ export const createSharing = (options: CreateOptions): SharingApi => { * @returns Promise with success result */ const deleteAssetShare = (options: DeleteShareOptions, callback?: RequestCallback): Promise => { - const { shareId, queryParameters } = options; - - const url = `${baseUrl}/${shareId}?assetType=${queryParameters.assetType}&assetId=${queryParameters.assetId}`; + const urlOptions = { url: `${baseUrl}/${options.shareId}` }; - return requestor.delete({ ...optionsToSend, url, ...options }, callback); + return requestor.delete({ ...optionsToSend, ...urlOptions, ...options }, callback); }; return { diff --git a/lib/utils/httpRequestor.js b/lib/utils/httpRequestor.js index 3ec4ab93..3e42f0e0 100644 --- a/lib/utils/httpRequestor.js +++ b/lib/utils/httpRequestor.js @@ -96,6 +96,8 @@ export function create(requestorConfig) { const put = (options, callback) => methodRequest(options, request.put, 'PUT', callback, options.body); + const patch = (options, callback) => methodRequest(options, request.patch, 'PATCH', callback, options.body); + const methodRequest = (options, method, methodName, callback, body) => { const baseRequestOptions = { url: buildUrl(options), @@ -191,6 +193,7 @@ export function create(requestorConfig) { return { get: get, put: put, + patch: patch, post: post, postFile: postFile, delete: deleteFunc, diff --git a/test/mock-api/sharing/common_test_constants.ts b/test/mock-api/sharing/common_test_constants.ts new file mode 100644 index 00000000..6074e3f8 --- /dev/null +++ b/test/mock-api/sharing/common_test_constants.ts @@ -0,0 +1,62 @@ +import { AccessLevel, AssetType, ShareScope, ShareType } from '@smartsheet/sharing'; + +// Common share identifiers +export const TEST_SHARE_ID = 'AAAMCmYGFOeE'; +export const TEST_SHARE_EMAIL = 'test.email@smartsheet.com'; +// userId and groupId are returned as strings in the API JSON payload +export const TEST_SHARE_USER_ID = '9876543210'; +export const TEST_SHARE_GROUP_ID = '1234567890'; +export const TEST_SHARE_NAME = 'Example Name'; +export const TEST_LAST_KEY = 'abcDefGhIjKlMnOpQrStUvWxYz'; + +// Asset targeted by share operations +export const TEST_ASSET_ID = 9876543210; + +// Pagination / include query parameters +export const TEST_MAX_ITEMS = 25; +export const TEST_SHARING_INCLUDE = 'email'; + +// Common success response values +export const TEST_SUCCESS_MESSAGE = 'SUCCESS'; +export const TEST_SUCCESS_RESULT_CODE = 0; + +// Share object with all properties present (matches all-response-body-properties mappings) +export const EXPECTED_SHARE_ALL_PROPERTIES = { + id: TEST_SHARE_ID, + email: TEST_SHARE_EMAIL, + userId: TEST_SHARE_USER_ID, + groupId: TEST_SHARE_GROUP_ID, + name: TEST_SHARE_NAME, + type: ShareType.USER, + accessLevel: AccessLevel.ADMIN, + scope: ShareScope.ITEM, +}; + +// Share object with only required properties (matches required-response-body-properties mappings) +export const EXPECTED_SHARE_REQUIRED_PROPERTIES = { + id: TEST_SHARE_ID, + email: TEST_SHARE_EMAIL, + userId: TEST_SHARE_USER_ID, + groupId: TEST_SHARE_GROUP_ID, + type: ShareType.USER, + accessLevel: AccessLevel.ADMIN, + scope: ShareScope.ITEM, +}; + +// Reusable query parameters targeting a sheet asset +export const TEST_SHEET_ASSET_QUERY = { + assetType: AssetType.SHEET, + assetId: TEST_ASSET_ID, +}; + +// Expected query parameters after URLSearchParams serialization (all values become strings) +export const EXPECTED_SHEET_ASSET_QUERY = { + assetType: AssetType.SHEET, + assetId: TEST_ASSET_ID.toString(), +}; + +// Common error status codes and messages +export const ERROR_500_STATUS_CODE = 500; +export const ERROR_500_MESSAGE = 'Internal Server Error'; +export const ERROR_400_STATUS_CODE = 400; +export const ERROR_400_MESSAGE = 'Malformed Request'; diff --git a/test/mock-api/sharing/delete_asset_share.spec.ts b/test/mock-api/sharing/delete_asset_share.spec.ts new file mode 100644 index 00000000..6fb28e95 --- /dev/null +++ b/test/mock-api/sharing/delete_asset_share.spec.ts @@ -0,0 +1,112 @@ +import crypto from 'crypto'; +import { createClient, findWireMockRequest } from '../utils/utils'; +import { expect } from '@jest/globals'; +import { AssetType } from '@smartsheet/sharing'; +import { + TEST_SHARE_ID, + TEST_ASSET_ID, + EXPECTED_SHEET_ASSET_QUERY, + TEST_SUCCESS_MESSAGE, + TEST_SUCCESS_RESULT_CODE, + ERROR_500_STATUS_CODE, + ERROR_500_MESSAGE, + ERROR_400_STATUS_CODE, + ERROR_400_MESSAGE, +} from './common_test_constants'; + +describe('Sharing - deleteAssetShare endpoint tests', () => { + const client = createClient(); + + const expectedAllResponseProperties = { + message: TEST_SUCCESS_MESSAGE, + resultCode: TEST_SUCCESS_RESULT_CODE, + }; + + it('deleteAssetShare generated url is correct', async () => { + const requestId = crypto.randomUUID(); + const options = { + shareId: TEST_SHARE_ID, + queryParameters: { + assetType: AssetType.SHEET, + assetId: TEST_ASSET_ID, + }, + customProperties: { + 'x-request-id': requestId, + 'x-test-name': '/sharing/delete-asset-share/all-response-body-properties', + }, + }; + await client.sharing.deleteAssetShare(options); + const matchedRequest = await findWireMockRequest(requestId); + + const parsedUrl = new URL(matchedRequest.absoluteUrl); + expect(parsedUrl.pathname).toEqual(`/2.0/shares/${TEST_SHARE_ID}`); + expect(matchedRequest.method).toEqual('DELETE'); + + const queryParamsObject = Object.fromEntries(parsedUrl.searchParams); + expect(queryParamsObject).toEqual(EXPECTED_SHEET_ASSET_QUERY); + }); + + it('deleteAssetShare all response body properties', async () => { + const requestId = crypto.randomUUID(); + const options = { + shareId: TEST_SHARE_ID, + queryParameters: { + assetType: AssetType.SHEET, + assetId: TEST_ASSET_ID, + }, + customProperties: { + 'x-request-id': requestId, + 'x-test-name': '/sharing/delete-asset-share/all-response-body-properties', + }, + }; + const response = await client.sharing.deleteAssetShare(options); + const matchedRequest = await findWireMockRequest(requestId); + + expect(matchedRequest.body).toEqual(''); + expect(response).toEqual(expectedAllResponseProperties); + }); + + it('deleteAssetShare error 400 response', async () => { + const requestId = crypto.randomUUID(); + const options = { + shareId: TEST_SHARE_ID, + queryParameters: { + assetType: AssetType.SHEET, + assetId: TEST_ASSET_ID, + }, + customProperties: { + 'x-request-id': requestId, + 'x-test-name': '/errors/400-response', + }, + }; + try { + await client.sharing.deleteAssetShare(options); + expect(true).toBe(false); // Expected an error to be thrown + } catch (error: any) { + expect(error.statusCode).toBe(ERROR_400_STATUS_CODE); + expect(error.message).toBe(ERROR_400_MESSAGE); + } + }); + + it('deleteAssetShare error 500 response', async () => { + const requestId = crypto.randomUUID(); + const options = { + shareId: TEST_SHARE_ID, + queryParameters: { + assetType: AssetType.SHEET, + assetId: TEST_ASSET_ID, + }, + customProperties: { + 'x-request-id': requestId, + 'x-test-name': '/errors/500-response', + }, + }; + try { + await client.sharing.deleteAssetShare(options); + expect(true).toBe(false); // Expected an error to be thrown + } catch (error: any) { + expect(error.statusCode).toBe(ERROR_500_STATUS_CODE); + expect(error.message).toBe(ERROR_500_MESSAGE); + } + }); +}); diff --git a/test/mock-api/sharing/get_asset_share.spec.ts b/test/mock-api/sharing/get_asset_share.spec.ts new file mode 100644 index 00000000..0ef3f9cd --- /dev/null +++ b/test/mock-api/sharing/get_asset_share.spec.ts @@ -0,0 +1,127 @@ +import crypto from 'crypto'; +import { createClient, findWireMockRequest } from '../utils/utils'; +import { expect } from '@jest/globals'; +import { AssetType } from '@smartsheet/sharing'; +import { + TEST_SHARE_ID, + TEST_ASSET_ID, + EXPECTED_SHARE_ALL_PROPERTIES, + EXPECTED_SHARE_REQUIRED_PROPERTIES, + EXPECTED_SHEET_ASSET_QUERY, + ERROR_500_STATUS_CODE, + ERROR_500_MESSAGE, + ERROR_400_STATUS_CODE, + ERROR_400_MESSAGE, +} from './common_test_constants'; + +describe('Sharing - getAssetShare endpoint tests', () => { + const client = createClient(); + + it('getAssetShare generated url is correct', async () => { + const requestId = crypto.randomUUID(); + const options = { + shareId: TEST_SHARE_ID, + queryParameters: { + assetType: AssetType.SHEET, + assetId: TEST_ASSET_ID, + }, + customProperties: { + 'x-request-id': requestId, + 'x-test-name': '/sharing/get-asset-share/all-response-body-properties', + }, + }; + await client.sharing.getAssetShare(options); + const matchedRequest = await findWireMockRequest(requestId); + + const parsedUrl = new URL(matchedRequest.absoluteUrl); + expect(parsedUrl.pathname).toEqual(`/2.0/shares/${TEST_SHARE_ID}`); + expect(matchedRequest.method).toEqual('GET'); + + const queryParamsObject = Object.fromEntries(parsedUrl.searchParams); + expect(queryParamsObject).toEqual(EXPECTED_SHEET_ASSET_QUERY); + }); + + it('getAssetShare all response body properties', async () => { + const requestId = crypto.randomUUID(); + const options = { + shareId: TEST_SHARE_ID, + queryParameters: { + assetType: AssetType.SHEET, + assetId: TEST_ASSET_ID, + }, + customProperties: { + 'x-request-id': requestId, + 'x-test-name': '/sharing/get-asset-share/all-response-body-properties', + }, + }; + const response = await client.sharing.getAssetShare(options); + const matchedRequest = await findWireMockRequest(requestId); + + expect(matchedRequest.body).toEqual(''); + expect(response).toEqual(EXPECTED_SHARE_ALL_PROPERTIES); + }); + + it('getAssetShare required response body properties', async () => { + const requestId = crypto.randomUUID(); + const options = { + shareId: TEST_SHARE_ID, + queryParameters: { + assetType: AssetType.SHEET, + assetId: TEST_ASSET_ID, + }, + customProperties: { + 'x-request-id': requestId, + 'x-test-name': '/sharing/get-asset-share/required-response-body-properties', + }, + }; + const response = await client.sharing.getAssetShare(options); + const matchedRequest = await findWireMockRequest(requestId); + + expect(matchedRequest.body).toEqual(''); + expect(response).toEqual(EXPECTED_SHARE_REQUIRED_PROPERTIES); + }); + + it('getAssetShare error 400 response', async () => { + const requestId = crypto.randomUUID(); + const options = { + shareId: TEST_SHARE_ID, + queryParameters: { + assetType: AssetType.SHEET, + assetId: TEST_ASSET_ID, + }, + customProperties: { + 'x-request-id': requestId, + 'x-test-name': '/errors/400-response', + }, + }; + try { + await client.sharing.getAssetShare(options); + expect(true).toBe(false); // Expected an error to be thrown + } catch (error: any) { + expect(error.statusCode).toBe(ERROR_400_STATUS_CODE); + expect(error.message).toBe(ERROR_400_MESSAGE); + } + }); + + it('getAssetShare error 500 response', async () => { + const requestId = crypto.randomUUID(); + const options = { + shareId: TEST_SHARE_ID, + queryParameters: { + assetType: AssetType.SHEET, + assetId: TEST_ASSET_ID, + }, + customProperties: { + 'x-request-id': requestId, + 'x-test-name': '/errors/500-response', + }, + }; + try { + await client.sharing.getAssetShare(options); + expect(true).toBe(false); // Expected an error to be thrown + } catch (error: any) { + expect(error.statusCode).toBe(ERROR_500_STATUS_CODE); + expect(error.message).toBe(ERROR_500_MESSAGE); + } + }); +}); diff --git a/test/mock-api/sharing/list_asset_shares.spec.ts b/test/mock-api/sharing/list_asset_shares.spec.ts new file mode 100644 index 00000000..eac07c37 --- /dev/null +++ b/test/mock-api/sharing/list_asset_shares.spec.ts @@ -0,0 +1,139 @@ +import crypto from 'crypto'; +import { createClient, findWireMockRequest } from '../utils/utils'; +import { expect } from '@jest/globals'; +import { AssetType } from '@smartsheet/sharing'; +import { + TEST_ASSET_ID, + TEST_MAX_ITEMS, + TEST_SHARING_INCLUDE, + TEST_LAST_KEY, + EXPECTED_SHARE_ALL_PROPERTIES, + EXPECTED_SHARE_REQUIRED_PROPERTIES, + ERROR_500_STATUS_CODE, + ERROR_500_MESSAGE, + ERROR_400_STATUS_CODE, + ERROR_400_MESSAGE, +} from './common_test_constants'; + +describe('Sharing - listAssetShares endpoint tests', () => { + const client = createClient(); + + const expectedAllResponseProperties = { + lastKey: TEST_LAST_KEY, + items: [EXPECTED_SHARE_ALL_PROPERTIES], + }; + + const expectedRequiredResponseProperties = { + items: [EXPECTED_SHARE_REQUIRED_PROPERTIES], + }; + + it('listAssetShares generated url is correct', async () => { + const requestId = crypto.randomUUID(); + const options = { + queryParameters: { + assetType: AssetType.SHEET, + assetId: TEST_ASSET_ID, + maxItems: TEST_MAX_ITEMS, + sharingInclude: TEST_SHARING_INCLUDE, + }, + customProperties: { + 'x-request-id': requestId, + 'x-test-name': '/sharing/list-asset-shares/all-response-body-properties', + }, + }; + await client.sharing.listAssetShares(options); + const matchedRequest = await findWireMockRequest(requestId); + + const parsedUrl = new URL(matchedRequest.absoluteUrl); + expect(parsedUrl.pathname).toEqual('/2.0/shares'); + expect(matchedRequest.method).toEqual('GET'); + + const queryParamsObject = Object.fromEntries(parsedUrl.searchParams); + expect(queryParamsObject).toEqual({ + assetType: AssetType.SHEET, + assetId: TEST_ASSET_ID.toString(), + maxItems: TEST_MAX_ITEMS.toString(), + sharingInclude: TEST_SHARING_INCLUDE, + }); + }); + + it('listAssetShares all response body properties', async () => { + const requestId = crypto.randomUUID(); + const options = { + queryParameters: { + assetType: AssetType.SHEET, + assetId: TEST_ASSET_ID, + }, + customProperties: { + 'x-request-id': requestId, + 'x-test-name': '/sharing/list-asset-shares/all-response-body-properties', + }, + }; + const response = await client.sharing.listAssetShares(options); + const matchedRequest = await findWireMockRequest(requestId); + + expect(matchedRequest.body).toEqual(''); + expect(response).toEqual(expectedAllResponseProperties); + }); + + it('listAssetShares required response body properties', async () => { + const requestId = crypto.randomUUID(); + const options = { + queryParameters: { + assetType: AssetType.SHEET, + assetId: TEST_ASSET_ID, + }, + customProperties: { + 'x-request-id': requestId, + 'x-test-name': '/sharing/list-asset-shares/required-response-body-properties', + }, + }; + const response = await client.sharing.listAssetShares(options); + const matchedRequest = await findWireMockRequest(requestId); + + expect(matchedRequest.body).toEqual(''); + expect(response).toEqual(expectedRequiredResponseProperties); + }); + + it('listAssetShares error 400 response', async () => { + const requestId = crypto.randomUUID(); + const options = { + queryParameters: { + assetType: AssetType.SHEET, + assetId: TEST_ASSET_ID, + }, + customProperties: { + 'x-request-id': requestId, + 'x-test-name': '/errors/400-response', + }, + }; + try { + await client.sharing.listAssetShares(options); + expect(true).toBe(false); // Expected an error to be thrown + } catch (error: any) { + expect(error.statusCode).toBe(ERROR_400_STATUS_CODE); + expect(error.message).toBe(ERROR_400_MESSAGE); + } + }); + + it('listAssetShares error 500 response', async () => { + const requestId = crypto.randomUUID(); + const options = { + queryParameters: { + assetType: AssetType.SHEET, + assetId: TEST_ASSET_ID, + }, + customProperties: { + 'x-request-id': requestId, + 'x-test-name': '/errors/500-response', + }, + }; + try { + await client.sharing.listAssetShares(options); + expect(true).toBe(false); // Expected an error to be thrown + } catch (error: any) { + expect(error.statusCode).toBe(ERROR_500_STATUS_CODE); + expect(error.message).toBe(ERROR_500_MESSAGE); + } + }); +}); diff --git a/test/mock-api/sharing/share_asset.spec.ts b/test/mock-api/sharing/share_asset.spec.ts new file mode 100644 index 00000000..c8d94d02 --- /dev/null +++ b/test/mock-api/sharing/share_asset.spec.ts @@ -0,0 +1,162 @@ +import crypto from 'crypto'; +import { createClient, findWireMockRequest } from '../utils/utils'; +import { expect } from '@jest/globals'; +import { AccessLevel, AssetType } from '@smartsheet/sharing'; +import { + TEST_ASSET_ID, + TEST_SHARE_EMAIL, + TEST_SHARE_GROUP_ID, + EXPECTED_SHARE_ALL_PROPERTIES, + EXPECTED_SHARE_REQUIRED_PROPERTIES, + TEST_SUCCESS_MESSAGE, + TEST_SUCCESS_RESULT_CODE, + ERROR_500_STATUS_CODE, + ERROR_500_MESSAGE, + ERROR_400_STATUS_CODE, + ERROR_400_MESSAGE, +} from './common_test_constants'; + +describe('Sharing - shareAsset endpoint tests', () => { + const client = createClient(); + + const testShareRequestBody = [ + { + email: TEST_SHARE_EMAIL, + accessLevel: AccessLevel.ADMIN, + subject: 'Shared with you', + message: 'Please review this asset', + ccMe: true, + }, + { + groupId: Number(TEST_SHARE_GROUP_ID), + accessLevel: AccessLevel.VIEWER, + }, + ]; + + const expectedAllResponseProperties = { + message: TEST_SUCCESS_MESSAGE, + resultCode: TEST_SUCCESS_RESULT_CODE, + result: [EXPECTED_SHARE_ALL_PROPERTIES], + }; + + const expectedRequiredResponseProperties = { + message: TEST_SUCCESS_MESSAGE, + resultCode: TEST_SUCCESS_RESULT_CODE, + result: [EXPECTED_SHARE_REQUIRED_PROPERTIES], + }; + + it('shareAsset generated url is correct', async () => { + const requestId = crypto.randomUUID(); + const options = { + body: testShareRequestBody, + queryParameters: { + assetType: AssetType.SHEET, + assetId: TEST_ASSET_ID, + sendEmail: true, + }, + customProperties: { + 'x-request-id': requestId, + 'x-test-name': '/sharing/share-asset/all-response-body-properties', + }, + }; + await client.sharing.shareAsset(options); + const matchedRequest = await findWireMockRequest(requestId); + + const parsedUrl = new URL(matchedRequest.absoluteUrl); + expect(parsedUrl.pathname).toEqual('/2.0/shares'); + expect(matchedRequest.method).toEqual('POST'); + + const queryParamsObject = Object.fromEntries(parsedUrl.searchParams); + expect(queryParamsObject).toEqual({ + assetType: AssetType.SHEET, + assetId: TEST_ASSET_ID.toString(), + sendEmail: 'true', + }); + }); + + it('shareAsset all response body properties', async () => { + const requestId = crypto.randomUUID(); + const options = { + body: testShareRequestBody, + queryParameters: { + assetType: AssetType.SHEET, + assetId: TEST_ASSET_ID, + }, + customProperties: { + 'x-request-id': requestId, + 'x-test-name': '/sharing/share-asset/all-response-body-properties', + }, + }; + const response = await client.sharing.shareAsset(options); + const matchedRequest = await findWireMockRequest(requestId); + + const body = JSON.parse(matchedRequest.body); + expect(body).toEqual(testShareRequestBody); + expect(response).toEqual(expectedAllResponseProperties); + }); + + it('shareAsset required response body properties', async () => { + const requestId = crypto.randomUUID(); + const options = { + body: testShareRequestBody, + queryParameters: { + assetType: AssetType.SHEET, + assetId: TEST_ASSET_ID, + }, + customProperties: { + 'x-request-id': requestId, + 'x-test-name': '/sharing/share-asset/required-response-body-properties', + }, + }; + const response = await client.sharing.shareAsset(options); + const matchedRequest = await findWireMockRequest(requestId); + + const body = JSON.parse(matchedRequest.body); + expect(body).toEqual(testShareRequestBody); + expect(response).toEqual(expectedRequiredResponseProperties); + }); + + it('shareAsset error 400 response', async () => { + const requestId = crypto.randomUUID(); + const options = { + body: testShareRequestBody, + queryParameters: { + assetType: AssetType.SHEET, + assetId: TEST_ASSET_ID, + }, + customProperties: { + 'x-request-id': requestId, + 'x-test-name': '/errors/400-response', + }, + }; + try { + await client.sharing.shareAsset(options); + expect(true).toBe(false); // Expected an error to be thrown + } catch (error: any) { + expect(error.statusCode).toBe(ERROR_400_STATUS_CODE); + expect(error.message).toBe(ERROR_400_MESSAGE); + } + }); + + it('shareAsset error 500 response', async () => { + const requestId = crypto.randomUUID(); + const options = { + body: testShareRequestBody, + queryParameters: { + assetType: AssetType.SHEET, + assetId: TEST_ASSET_ID, + }, + customProperties: { + 'x-request-id': requestId, + 'x-test-name': '/errors/500-response', + }, + }; + try { + await client.sharing.shareAsset(options); + expect(true).toBe(false); // Expected an error to be thrown + } catch (error: any) { + expect(error.statusCode).toBe(ERROR_500_STATUS_CODE); + expect(error.message).toBe(ERROR_500_MESSAGE); + } + }); +}); diff --git a/test/mock-api/sharing/update_asset_share.spec.ts b/test/mock-api/sharing/update_asset_share.spec.ts new file mode 100644 index 00000000..6ea0d193 --- /dev/null +++ b/test/mock-api/sharing/update_asset_share.spec.ts @@ -0,0 +1,138 @@ +import crypto from 'crypto'; +import { createClient, findWireMockRequest } from '../utils/utils'; +import { expect } from '@jest/globals'; +import { AccessLevel, AssetType } from '@smartsheet/sharing'; +import { + TEST_SHARE_ID, + TEST_ASSET_ID, + EXPECTED_SHARE_ALL_PROPERTIES, + EXPECTED_SHARE_REQUIRED_PROPERTIES, + EXPECTED_SHEET_ASSET_QUERY, + ERROR_500_STATUS_CODE, + ERROR_500_MESSAGE, + ERROR_400_STATUS_CODE, + ERROR_400_MESSAGE, +} from './common_test_constants'; + +describe('Sharing - updateAssetShare endpoint tests', () => { + const client = createClient(); + + const testUpdateRequestBody = { + accessLevel: AccessLevel.EDITOR, + }; + + it('updateAssetShare generated url is correct', async () => { + const requestId = crypto.randomUUID(); + const options = { + shareId: TEST_SHARE_ID, + body: testUpdateRequestBody, + queryParameters: { + assetType: AssetType.SHEET, + assetId: TEST_ASSET_ID, + }, + customProperties: { + 'x-request-id': requestId, + 'x-test-name': '/sharing/update-asset-share/all-response-body-properties', + }, + }; + await client.sharing.updateAssetShare(options); + const matchedRequest = await findWireMockRequest(requestId); + + const parsedUrl = new URL(matchedRequest.absoluteUrl); + expect(parsedUrl.pathname).toEqual(`/2.0/shares/${TEST_SHARE_ID}`); + expect(matchedRequest.method).toEqual('PATCH'); + + const queryParamsObject = Object.fromEntries(parsedUrl.searchParams); + expect(queryParamsObject).toEqual(EXPECTED_SHEET_ASSET_QUERY); + }); + + it('updateAssetShare all response body properties', async () => { + const requestId = crypto.randomUUID(); + const options = { + shareId: TEST_SHARE_ID, + body: testUpdateRequestBody, + queryParameters: { + assetType: AssetType.SHEET, + assetId: TEST_ASSET_ID, + }, + customProperties: { + 'x-request-id': requestId, + 'x-test-name': '/sharing/update-asset-share/all-response-body-properties', + }, + }; + const response = await client.sharing.updateAssetShare(options); + const matchedRequest = await findWireMockRequest(requestId); + + const body = JSON.parse(matchedRequest.body); + expect(body).toEqual(testUpdateRequestBody); + expect(response).toEqual(EXPECTED_SHARE_ALL_PROPERTIES); + }); + + it('updateAssetShare required response body properties', async () => { + const requestId = crypto.randomUUID(); + const options = { + shareId: TEST_SHARE_ID, + body: testUpdateRequestBody, + queryParameters: { + assetType: AssetType.SHEET, + assetId: TEST_ASSET_ID, + }, + customProperties: { + 'x-request-id': requestId, + 'x-test-name': '/sharing/update-asset-share/required-response-body-properties', + }, + }; + const response = await client.sharing.updateAssetShare(options); + const matchedRequest = await findWireMockRequest(requestId); + + const body = JSON.parse(matchedRequest.body); + expect(body).toEqual(testUpdateRequestBody); + expect(response).toEqual(EXPECTED_SHARE_REQUIRED_PROPERTIES); + }); + + it('updateAssetShare error 400 response', async () => { + const requestId = crypto.randomUUID(); + const options = { + shareId: TEST_SHARE_ID, + body: testUpdateRequestBody, + queryParameters: { + assetType: AssetType.SHEET, + assetId: TEST_ASSET_ID, + }, + customProperties: { + 'x-request-id': requestId, + 'x-test-name': '/errors/400-response', + }, + }; + try { + await client.sharing.updateAssetShare(options); + expect(true).toBe(false); // Expected an error to be thrown + } catch (error: any) { + expect(error.statusCode).toBe(ERROR_400_STATUS_CODE); + expect(error.message).toBe(ERROR_400_MESSAGE); + } + }); + + it('updateAssetShare error 500 response', async () => { + const requestId = crypto.randomUUID(); + const options = { + shareId: TEST_SHARE_ID, + body: testUpdateRequestBody, + queryParameters: { + assetType: AssetType.SHEET, + assetId: TEST_ASSET_ID, + }, + customProperties: { + 'x-request-id': requestId, + 'x-test-name': '/errors/500-response', + }, + }; + try { + await client.sharing.updateAssetShare(options); + expect(true).toBe(false); // Expected an error to be thrown + } catch (error: any) { + expect(error.statusCode).toBe(ERROR_500_STATUS_CODE); + expect(error.message).toBe(ERROR_500_MESSAGE); + } + }); +}); From 59fe919f0d73b43e33692a8152fadd8a691c0dfa Mon Sep 17 00:00:00 2001 From: George Goranov Date: Tue, 21 Jul 2026 17:19:31 +0300 Subject: [PATCH 2/2] Assert query params as URLSearchParams to catch duplicate keys Object.fromEntries() collapses repeated query keys, so the original double-applied assetType/assetId bug would still satisfy the URL assertions. Comparing parsedUrl.searchParams against an expected URLSearchParams verifies key cardinality, making these real regression tests for #195. Addresses CodeRabbit review on PR #197. Co-Authored-By: Claude Opus 4.8 (1M context) --- test/mock-api/sharing/common_test_constants.ts | 7 ++++--- test/mock-api/sharing/delete_asset_share.spec.ts | 3 +-- test/mock-api/sharing/get_asset_share.spec.ts | 3 +-- test/mock-api/sharing/list_asset_shares.spec.ts | 4 ++-- test/mock-api/sharing/share_asset.spec.ts | 4 ++-- test/mock-api/sharing/update_asset_share.spec.ts | 3 +-- 6 files changed, 11 insertions(+), 13 deletions(-) diff --git a/test/mock-api/sharing/common_test_constants.ts b/test/mock-api/sharing/common_test_constants.ts index 6074e3f8..2ccacc74 100644 --- a/test/mock-api/sharing/common_test_constants.ts +++ b/test/mock-api/sharing/common_test_constants.ts @@ -49,11 +49,12 @@ export const TEST_SHEET_ASSET_QUERY = { assetId: TEST_ASSET_ID, }; -// Expected query parameters after URLSearchParams serialization (all values become strings) -export const EXPECTED_SHEET_ASSET_QUERY = { +// Expected query string for a sheet asset, built as URLSearchParams so assertions +// verify key cardinality (a duplicated assetType/assetId would fail the comparison). +export const EXPECTED_SHEET_ASSET_QUERY = new URLSearchParams({ assetType: AssetType.SHEET, assetId: TEST_ASSET_ID.toString(), -}; +}); // Common error status codes and messages export const ERROR_500_STATUS_CODE = 500; diff --git a/test/mock-api/sharing/delete_asset_share.spec.ts b/test/mock-api/sharing/delete_asset_share.spec.ts index 6fb28e95..cc04e19d 100644 --- a/test/mock-api/sharing/delete_asset_share.spec.ts +++ b/test/mock-api/sharing/delete_asset_share.spec.ts @@ -42,8 +42,7 @@ describe('Sharing - deleteAssetShare endpoint tests', () => { expect(parsedUrl.pathname).toEqual(`/2.0/shares/${TEST_SHARE_ID}`); expect(matchedRequest.method).toEqual('DELETE'); - const queryParamsObject = Object.fromEntries(parsedUrl.searchParams); - expect(queryParamsObject).toEqual(EXPECTED_SHEET_ASSET_QUERY); + expect(parsedUrl.searchParams).toEqual(EXPECTED_SHEET_ASSET_QUERY); }); it('deleteAssetShare all response body properties', async () => { diff --git a/test/mock-api/sharing/get_asset_share.spec.ts b/test/mock-api/sharing/get_asset_share.spec.ts index 0ef3f9cd..e8e68863 100644 --- a/test/mock-api/sharing/get_asset_share.spec.ts +++ b/test/mock-api/sharing/get_asset_share.spec.ts @@ -37,8 +37,7 @@ describe('Sharing - getAssetShare endpoint tests', () => { expect(parsedUrl.pathname).toEqual(`/2.0/shares/${TEST_SHARE_ID}`); expect(matchedRequest.method).toEqual('GET'); - const queryParamsObject = Object.fromEntries(parsedUrl.searchParams); - expect(queryParamsObject).toEqual(EXPECTED_SHEET_ASSET_QUERY); + expect(parsedUrl.searchParams).toEqual(EXPECTED_SHEET_ASSET_QUERY); }); it('getAssetShare all response body properties', async () => { diff --git a/test/mock-api/sharing/list_asset_shares.spec.ts b/test/mock-api/sharing/list_asset_shares.spec.ts index eac07c37..f5206f54 100644 --- a/test/mock-api/sharing/list_asset_shares.spec.ts +++ b/test/mock-api/sharing/list_asset_shares.spec.ts @@ -48,13 +48,13 @@ describe('Sharing - listAssetShares endpoint tests', () => { expect(parsedUrl.pathname).toEqual('/2.0/shares'); expect(matchedRequest.method).toEqual('GET'); - const queryParamsObject = Object.fromEntries(parsedUrl.searchParams); - expect(queryParamsObject).toEqual({ + const expectedQueryParams = new URLSearchParams({ assetType: AssetType.SHEET, assetId: TEST_ASSET_ID.toString(), maxItems: TEST_MAX_ITEMS.toString(), sharingInclude: TEST_SHARING_INCLUDE, }); + expect(parsedUrl.searchParams).toEqual(expectedQueryParams); }); it('listAssetShares all response body properties', async () => { diff --git a/test/mock-api/sharing/share_asset.spec.ts b/test/mock-api/sharing/share_asset.spec.ts index c8d94d02..6a14dce1 100644 --- a/test/mock-api/sharing/share_asset.spec.ts +++ b/test/mock-api/sharing/share_asset.spec.ts @@ -66,12 +66,12 @@ describe('Sharing - shareAsset endpoint tests', () => { expect(parsedUrl.pathname).toEqual('/2.0/shares'); expect(matchedRequest.method).toEqual('POST'); - const queryParamsObject = Object.fromEntries(parsedUrl.searchParams); - expect(queryParamsObject).toEqual({ + const expectedQueryParams = new URLSearchParams({ assetType: AssetType.SHEET, assetId: TEST_ASSET_ID.toString(), sendEmail: 'true', }); + expect(parsedUrl.searchParams).toEqual(expectedQueryParams); }); it('shareAsset all response body properties', async () => { diff --git a/test/mock-api/sharing/update_asset_share.spec.ts b/test/mock-api/sharing/update_asset_share.spec.ts index 6ea0d193..9ffdee4a 100644 --- a/test/mock-api/sharing/update_asset_share.spec.ts +++ b/test/mock-api/sharing/update_asset_share.spec.ts @@ -42,8 +42,7 @@ describe('Sharing - updateAssetShare endpoint tests', () => { expect(parsedUrl.pathname).toEqual(`/2.0/shares/${TEST_SHARE_ID}`); expect(matchedRequest.method).toEqual('PATCH'); - const queryParamsObject = Object.fromEntries(parsedUrl.searchParams); - expect(queryParamsObject).toEqual(EXPECTED_SHEET_ASSET_QUERY); + expect(parsedUrl.searchParams).toEqual(EXPECTED_SHEET_ASSET_QUERY); }); it('updateAssetShare all response body properties', async () => {