diff --git a/packages/drivers/odsp-driver/src/getFileLink.ts b/packages/drivers/odsp-driver/src/getFileLink.ts index 2e2f18e9e377..e49613edaa76 100644 --- a/packages/drivers/odsp-driver/src/getFileLink.ts +++ b/packages/drivers/odsp-driver/src/getFileLink.ts @@ -6,12 +6,12 @@ import type { ITelemetryBaseProperties } from "@fluidframework/core-interfaces"; import { assert } from "@fluidframework/core-utils/internal"; import { NonRetryableError, runWithRetry } from "@fluidframework/driver-utils/internal"; -import { hasRedirectionLocation } from "@fluidframework/odsp-doclib-utils/internal"; import { IOdspUrlParts, OdspErrorTypes, OdspResourceTokenFetchOptions, TokenFetcher, + type IOdspResolvedUrl, } from "@fluidframework/odsp-driver-definitions/internal"; import { ITelemetryLoggerExt, @@ -44,10 +44,10 @@ const fileLinkCache = new Map>(); */ export async function getFileLink( getToken: TokenFetcher, - odspUrlParts: IOdspUrlParts, + resolvedUrl: IOdspResolvedUrl, logger: ITelemetryLoggerExt, ): Promise { - const cacheKey = `${odspUrlParts.siteUrl}_${odspUrlParts.driveId}_${odspUrlParts.itemId}`; + const cacheKey = `${resolvedUrl.siteUrl}_${resolvedUrl.driveId}_${resolvedUrl.itemId}`; const maybeFileLinkCacheEntry = fileLinkCache.get(cacheKey); if (maybeFileLinkCacheEntry !== undefined) { return maybeFileLinkCacheEntry; @@ -61,7 +61,7 @@ export async function getFileLink( async () => runWithRetryForCoherencyAndServiceReadOnlyErrors( async () => - getFileLinkWithLocationRedirectionHandling(getToken, odspUrlParts, logger), + getFileLinkWithLocationRedirectionHandling(getToken, resolvedUrl, logger), "getFileLinkCore", logger, ), @@ -115,32 +115,41 @@ export async function getFileLink( */ async function getFileLinkWithLocationRedirectionHandling( getToken: TokenFetcher, - odspUrlParts: IOdspUrlParts, + resolvedUrl: IOdspResolvedUrl, logger: ITelemetryLoggerExt, ): Promise { // We can have chains of location redirection one after the other, so have a for loop // so that we can keep handling the same type of error. Set max number of redirection to 5. let lastError: unknown; + let locationRedirected = false; for (let count = 1; count <= 5; count++) { try { - return await getFileLinkCore(getToken, odspUrlParts, logger); + const fileItem = await getFileItemLite(getToken, resolvedUrl, logger, true); + // Sometimes the siteUrl in the actual file is different from the siteUrl in the resolvedUrl due to location + // redirection. This creates issues in the getSharingInformation call. So we need to update the siteUrl in the + // resolvedUrl to the siteUrl in the fileItem which is the updated siteUrl. + const oldSiteDomain = new URL(resolvedUrl.siteUrl).origin; + const newSiteDomain = new URL(fileItem.sharepointIds.siteUrl).origin; + if (oldSiteDomain !== newSiteDomain) { + locationRedirected = true; + logger.sendTelemetryEvent({ + eventName: "LocationRedirectionErrorForGetOdspFileLink", + retryCount: count, + }); + renameTenantInOdspResolvedUrl(resolvedUrl, newSiteDomain); + } + return await getFileLinkCore(getToken, resolvedUrl, logger, fileItem); } catch (error: unknown) { lastError = error; + // If the getSharingLink call fails with the 401/403/404 error, then it could be due to that the file has moved + // to another location. This could occur in case we have more than 1 tenant rename. In that case, we should retry + // the getFileItemLite call to get the updated fileItem. if ( isFluidError(error) && - error.errorType === OdspErrorTypes.fileNotFoundOrAccessDeniedError && - hasRedirectionLocation(error) && - error.redirectLocation !== undefined + locationRedirected && + (error.errorType === OdspErrorTypes.fileNotFoundOrAccessDeniedError || + error.errorType === OdspErrorTypes.authorizationError) ) { - const redirectLocation = error.redirectLocation; - logger.sendTelemetryEvent({ - eventName: "LocationRedirectionErrorForGetOdspFileLink", - retryCount: count, - }); - // Generate the new SiteUrl from the redirection location. - const newSiteDomain = new URL(redirectLocation).origin; - const newSiteUrl = `${newSiteDomain}${new URL(odspUrlParts.siteUrl).pathname}`; - odspUrlParts.siteUrl = newSiteUrl; continue; } throw error; @@ -153,9 +162,8 @@ async function getFileLinkCore( getToken: TokenFetcher, odspUrlParts: IOdspUrlParts, logger: ITelemetryLoggerExt, + fileItem: FileItemLite, ): Promise { - const fileItem = await getFileItemLite(getToken, odspUrlParts, logger, true); - // ODSP link requires extra call to return link that is resistant to file being renamed or moved to different folder return PerformanceEvent.timedExecAsync( logger, @@ -193,7 +201,6 @@ async function getFileLinkCore( headers: { "Content-Type": "application/json;odata=verbose", "Accept": "application/json;odata=verbose", - "redirect": "manual", ...headers, }, }; @@ -280,7 +287,6 @@ async function getFileItemLite( ); const headers = getHeadersWithAuth(authHeader); - headers.redirect = "manual"; const requestInit = { method, headers }; const response = await fetchHelper(url, requestInit); additionalProps = response.propsToLog; @@ -301,3 +307,37 @@ async function getFileItemLite( }, ); } + +/** + * It takes a resolved url with old siteUrl and patches resolved url with updated site url domain. + * @param odspResolvedUrl - Previous odsp resolved url with older site url. + * @param newSiteDomain - New site domain after the tenant rename. + */ +function renameTenantInOdspResolvedUrl( + odspResolvedUrl: IOdspResolvedUrl, + newSiteDomain: string, +): void { + const newSiteUrl = `${newSiteDomain}${new URL(odspResolvedUrl.siteUrl).pathname}`; + odspResolvedUrl.siteUrl = newSiteUrl; + + if (odspResolvedUrl.endpoints.attachmentGETStorageUrl) { + odspResolvedUrl.endpoints.attachmentGETStorageUrl = `${newSiteDomain}${ + new URL(odspResolvedUrl.endpoints.attachmentGETStorageUrl).pathname + }`; + } + if (odspResolvedUrl.endpoints.attachmentPOSTStorageUrl) { + odspResolvedUrl.endpoints.attachmentPOSTStorageUrl = `${newSiteDomain}${ + new URL(odspResolvedUrl.endpoints.attachmentPOSTStorageUrl).pathname + }`; + } + if (odspResolvedUrl.endpoints.deltaStorageUrl) { + odspResolvedUrl.endpoints.deltaStorageUrl = `${newSiteDomain}${ + new URL(odspResolvedUrl.endpoints.deltaStorageUrl).pathname + }`; + } + if (odspResolvedUrl.endpoints.snapshotStorageUrl) { + odspResolvedUrl.endpoints.snapshotStorageUrl = `${newSiteDomain}${ + new URL(odspResolvedUrl.endpoints.snapshotStorageUrl).pathname + }`; + } +} diff --git a/packages/drivers/odsp-driver/src/test/getFileLink.spec.ts b/packages/drivers/odsp-driver/src/test/getFileLink.spec.ts index e73e77882bbf..3ae2ecd0b76e 100644 --- a/packages/drivers/odsp-driver/src/test/getFileLink.spec.ts +++ b/packages/drivers/odsp-driver/src/test/getFileLink.spec.ts @@ -5,6 +5,7 @@ import { strict as assert } from "node:assert"; +import type { IOdspResolvedUrl } from "@fluidframework/odsp-driver-definitions/internal"; import { MockLogger } from "@fluidframework/telemetry-utils/internal"; import { getFileLink } from "../getFileLink.js"; @@ -27,7 +28,17 @@ describe("getFileLink", () => { const fileItemResponse = { webDavUrl: "fetchDavUrl", webUrl: "fetchWebUrl", - sharepointIds: { listItemUniqueId: "fetchFileId" }, + sharepointIds: { listItemUniqueId: "fetchFileId", siteUrl }, + }; + + const getOdspResolvedUrl = (itemId: string): IOdspResolvedUrl => { + return { + siteUrl, + driveId, + itemId, + odspResolvedUrl: true, + endpoints: {}, + } as unknown as IOdspResolvedUrl; }; afterEach(() => { @@ -39,7 +50,7 @@ describe("getFileLink", () => { async () => getFileLink( storageTokenFetcher, - { siteUrl, driveId, itemId: "itemId4" }, + getOdspResolvedUrl("itemId4"), logger.toTelemetryLogger(), ), [ @@ -60,7 +71,7 @@ describe("getFileLink", () => { async () => getFileLink( storageTokenFetcher, - { siteUrl, driveId, itemId: "itemId5" }, + getOdspResolvedUrl("itemId5"), logger.toTelemetryLogger(), ), [ @@ -78,7 +89,7 @@ describe("getFileLink", () => { mockFetchSingle(async () => { return getFileLink( storageTokenFetcher, - { siteUrl, driveId, itemId: "itemId6" }, + getOdspResolvedUrl("itemId6"), logger.toTelemetryLogger(), ); }, notFound), @@ -91,7 +102,7 @@ describe("getFileLink", () => { async () => getFileLink( storageTokenFetcher, - { siteUrl, driveId, itemId: "itemId7" }, + getOdspResolvedUrl("itemId7"), logger.toTelemetryLogger(), ), [ @@ -109,7 +120,7 @@ describe("getFileLink", () => { // Should be present in cache now and subsequent calls should fetch from cache. const sharelink2 = await getFileLink( storageTokenFetcher, - { siteUrl, driveId, itemId: "itemId7" }, + getOdspResolvedUrl("itemId7"), logger.toTelemetryLogger(), ); assert.strictEqual( @@ -125,7 +136,7 @@ describe("getFileLink", () => { async () => getFileLink( storageTokenFetcher, - { siteUrl, driveId, itemId: "itemId7" }, + getOdspResolvedUrl("itemId7"), logger.toTelemetryLogger(), ), [ @@ -150,21 +161,18 @@ describe("getFileLink", () => { async () => getFileLink( storageTokenFetcher, - { siteUrl, driveId, itemId: "itemId8" }, + getOdspResolvedUrl("itemId8"), logger.toTelemetryLogger(), ), [ async (): Promise => - createResponse( - { Location: newSiteUrl }, + okResponse( + {}, { - error: { - message: "locationMoved", - }, + ...fileItemResponse, + sharepointIds: { ...fileItemResponse.sharepointIds, siteUrl: newSiteUrl }, }, - 308, ), - async (): Promise => okResponse({}, fileItemResponse), async (): Promise => okResponse({}, { d: { directUrl: "sharelink" } }), ], ); @@ -176,7 +184,7 @@ describe("getFileLink", () => { // Should be present in cache now and subsequent calls should fetch from cache. const sharelink2 = await getFileLink( storageTokenFetcher, - { siteUrl, driveId, itemId: "itemId8" }, + getOdspResolvedUrl("itemId8"), logger.toTelemetryLogger(), ); assert.strictEqual( @@ -191,31 +199,27 @@ describe("getFileLink", () => { async () => getFileLink( storageTokenFetcher, - { siteUrl, driveId, itemId: "itemId9" }, + getOdspResolvedUrl("itemId9"), logger.toTelemetryLogger(), ), [ async (): Promise => - createResponse( - { Location: newSiteUrl }, + okResponse( + {}, { - error: { - message: "locationMoved", - }, + ...fileItemResponse, + sharepointIds: { ...fileItemResponse.sharepointIds, siteUrl: newSiteUrl }, }, - 302, ), + notFound, async (): Promise => - createResponse( - { Location: newSiteUrl }, + okResponse( + {}, { - error: { - message: "locationMoved", - }, + ...fileItemResponse, + sharepointIds: { ...fileItemResponse.sharepointIds, siteUrl }, }, - 307, ), - async (): Promise => okResponse({}, fileItemResponse), async (): Promise => okResponse({}, { d: { directUrl: "sharelink" } }), ], ); @@ -227,7 +231,7 @@ describe("getFileLink", () => { // Should be present in cache now and subsequent calls should fetch from cache. const sharelink2 = await getFileLink( storageTokenFetcher, - { siteUrl, driveId, itemId: "itemId9" }, + getOdspResolvedUrl("itemId9"), logger.toTelemetryLogger(), ); assert.strictEqual( @@ -242,70 +246,55 @@ describe("getFileLink", () => { mockFetchMultiple(async () => { return getFileLink( storageTokenFetcher, - { siteUrl, driveId, itemId: "itemId10" }, + getOdspResolvedUrl("itemId10"), logger.toTelemetryLogger(), ); }, [ async (): Promise => - createResponse( - { Location: newSiteUrl }, - { - error: { - message: "locationMoved", - }, - }, - 308, - ), - async (): Promise => - createResponse( - { Location: newSiteUrl }, + okResponse( + {}, { - error: { - message: "locationMoved", - }, + ...fileItemResponse, + sharepointIds: { ...fileItemResponse.sharepointIds, siteUrl: newSiteUrl }, }, - 308, ), + notFound, async (): Promise => - createResponse( - { Location: newSiteUrl }, + okResponse( + {}, { - error: { - message: "locationMoved", - }, + ...fileItemResponse, + sharepointIds: { ...fileItemResponse.sharepointIds, siteUrl }, }, - 308, ), + notFound, async (): Promise => - createResponse( - { Location: newSiteUrl }, + okResponse( + {}, { - error: { - message: "locationMoved", - }, + ...fileItemResponse, + sharepointIds: { ...fileItemResponse.sharepointIds, siteUrl: newSiteUrl }, }, - 308, ), + notFound, async (): Promise => - createResponse( - { Location: newSiteUrl }, + okResponse( + {}, { - error: { - message: "locationMoved", - }, + ...fileItemResponse, + sharepointIds: { ...fileItemResponse.sharepointIds, siteUrl }, }, - 308, ), + notFound, async (): Promise => - createResponse( - { Location: newSiteUrl }, + okResponse( + {}, { - error: { - message: "locationMoved", - }, + ...fileItemResponse, + sharepointIds: { ...fileItemResponse.sharepointIds, siteUrl: newSiteUrl }, }, - 308, ), + notFound, ]), "File link should reject when not found", );