Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: Creating new Loop pages after SharePoint domain change #19443

Closed
wants to merge 13 commits into from
Closed

Fix: Creating new Loop pages after SharePoint domain change #19443

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
6c6dd6f
Fix file link after redirect
Philip-Scott Feb 1, 2024
0123021
Prettier fix
Philip-Scott Feb 1, 2024
496fdfd
Revert eslint ignore
Philip-Scott Feb 1, 2024
8c9f544
Merge branch 'main' into philip-scott/fix-get-file-line-after-redirect
Philip-Scott Feb 1, 2024
73700ba
Throw the error and mimic resolveWithLocationRedirectionHandling
Philip-Scott Feb 9, 2024
b688feb
Merge remote-tracking branch 'refs/remotes/origin/philip-scott/fix-ge…
Philip-Scott Feb 9, 2024
eeac31d
prettier:fix
Philip-Scott Feb 9, 2024
cc50bab
Remove that ugly hack
Philip-Scott Feb 9, 2024
fec84e8
Merge branch 'main' into philip-scott/fix-get-file-line-after-redirect
Philip-Scott Feb 9, 2024
ab6ccca
Use OdspRedirectError instead
Philip-Scott Feb 12, 2024
dcea3ec
Merge branch 'philip-scott/fix-get-file-line-after-redirect' of https…
Philip-Scott Feb 12, 2024
d2816ab
Remove extra space
Philip-Scott Feb 12, 2024
bfd5f75
Merge branch 'main' into philip-scott/fix-get-file-line-after-redirect
Philip-Scott Feb 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
119 changes: 86 additions & 33 deletions packages/drivers/odsp-driver/src/getFileLink.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,11 @@ import {
OdspResourceTokenFetchOptions,
TokenFetcher,
} from "@fluidframework/odsp-driver-definitions";
import { DriverErrorTypes } from "@fluidframework/driver-definitions";
import { getUrlAndHeadersWithAuth } from "./getUrlAndHeadersWithAuth";
import {
IOdspResponse,
TokenFetchOptionsEx,
fetchHelper,
getWithRetryForTokenRefresh,
toInstrumentedOdspTokenFetcher,
Expand Down Expand Up @@ -90,6 +93,83 @@ export async function getFileLink(
return fileLink;
}

async function getRequestInformation(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Recommend adding code docs to explain the purpose of this function.

getToken: TokenFetcher<OdspResourceTokenFetchOptions>,
odspUrlParts: IOdspUrlParts,
logger: ITelemetryLoggerExt,
options: TokenFetchOptionsEx,
fileItem: FileItemLite,
) {
const storageTokenFetcher = toInstrumentedOdspTokenFetcher(
logger,
odspUrlParts,
getToken,
true /* throwOnNullToken */,
);
const storageToken = await storageTokenFetcher(options, "GetFileLinkCore");
assert(
storageToken !== null,
0x2bb /* "Instrumented token fetcher with throwOnNullToken = true should never return null" */,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Every place that calls storageTokenFetcher() asserts that non-null result is returned. Given that's a wrapper produced by driver, can we at least move this assert into toInstrumentedOdspTokenFetcher() itself?
Or better, TokenFetcher type and force callers to change / assert? That's a bit bigger change, but having null on the type does not help.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mmmm that is actually part of the original code before moving it out to its own function. We can probably follow that up on a separate PR to keep this one small, is that alright?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

raised #20409 to fix it

);

// IMPORTANT: In past we were using GetFileByUrl() API to get to the list item that was corresponding
// to the file. This was intentionally replaced with GetFileById() to solve the following issue:
// GetFileByUrl() uses webDavUrl to locate list item. This API does not work for Consumer scenarios
// where webDavUrl is constructed using legacy ODC format for backward compatibility reasons.
// GetFileByUrl() does not understand that format and thus fails. GetFileById() relies on file item
// unique guid (sharepointIds.listItemUniqueId) and it works uniformly across Consumer and Commercial.
const { url, headers } = getUrlAndHeadersWithAuth(
`${
odspUrlParts.siteUrl
}/_api/web/GetFileById(@a1)/ListItemAllFields/GetSharingInformation?@a1=guid${encodeURIComponent(
`'${fileItem.sharepointIds.listItemUniqueId}'`,
)}`,
storageToken,
true,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add comments with arg names where it's not very clear what is the arg semantics

true, // forceAccessTokenViaAuthorizationHeader

);
const requestInit = {
method: "POST",
headers: {
"Content-Type": "application/json;odata=verbose",
"Accept": "application/json;odata=verbose",
...headers,
},
};

return { url, requestInit };
}

async function fetchWithLocationRedirectionHandling<T>(
getToken: TokenFetcher<OdspResourceTokenFetchOptions>,
odspUrlParts: IOdspUrlParts,
logger: ITelemetryLoggerExt,
options: TokenFetchOptionsEx,
fileItem: FileItemLite,
): Promise<IOdspResponse<Response>> {
for (;;) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the reason for this loop? My understanding from the changes in the code here is that we only want to perform a retry once after we get the 403 error. If this is true, can we structure the code such that we perform retry within the catch block?

let { url, requestInit } = await getRequestInformation...
try {
    response = await fetchHelper();
}catch(e){
    // 403 error occured and we need to retry with correct headers here
    if(error.redirectLocation){
        { url, requestInit } = = await getRequestInformation...
         response = await fetchHelper();
    }
}
return response;

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redirects can actually be chained together! One redirect may point to another redirect and so on.

This was actually a pattern taken from packages/loader/container-loader/src/location-redirection-utilities/resolveWithLocationRedirection.ts

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add comments to explain this logic.

const { url, requestInit } = await getRequestInformation(
getToken,
odspUrlParts,
logger,
options,
fileItem,
);

try {
return await fetchHelper(url, requestInit);
} catch (error: any) {
if (error.errorType !== DriverErrorTypes.locationRedirection) {
throw error;
}

const newSiteDomain = new URL(error.redirectUrl).origin;
const oldSiteDomain = new URL(odspUrlParts.siteUrl).origin;

odspUrlParts.siteUrl = odspUrlParts.siteUrl.replace(oldSiteDomain, newSiteDomain);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing input parameters of a function is not the best programming practice. (Although I do see it in some parts of the existing code already, but let's avoid adding any new ones.) If possible, always return the new value to the caller, and let the caller assign the value as it may.
(Might be irrelevant if we changed the code here to what I suggested above)

}
}
}

async function getFileLinkCore(
getToken: TokenFetcher<OdspResourceTokenFetchOptions>,
odspUrlParts: IOdspUrlParts,
Expand All @@ -106,42 +186,15 @@ async function getFileLinkCore(
let additionalProps;
const fileLink = await getWithRetryForTokenRefresh(async (options) => {
attempts++;
const storageTokenFetcher = toInstrumentedOdspTokenFetcher(
logger,
odspUrlParts,

const response = await fetchWithLocationRedirectionHandling(
getToken,
true /* throwOnNullToken */,
);
const storageToken = await storageTokenFetcher(options, "GetFileLinkCore");
assert(
storageToken !== null,
0x2bb /* "Instrumented token fetcher with throwOnNullToken = true should never return null" */,
odspUrlParts,
logger,
options,
fileItem,
);

// IMPORTANT: In past we were using GetFileByUrl() API to get to the list item that was corresponding
// to the file. This was intentionally replaced with GetFileById() to solve the following issue:
// GetFileByUrl() uses webDavUrl to locate list item. This API does not work for Consumer scenarios
// where webDavUrl is constructed using legacy ODC format for backward compatibility reasons.
// GetFileByUrl() does not understand that format and thus fails. GetFileById() relies on file item
// unique guid (sharepointIds.listItemUniqueId) and it works uniformly across Consumer and Commercial.
const { url, headers } = getUrlAndHeadersWithAuth(
`${
odspUrlParts.siteUrl
}/_api/web/GetFileById(@a1)/ListItemAllFields/GetSharingInformation?@a1=guid${encodeURIComponent(
`'${fileItem.sharepointIds.listItemUniqueId}'`,
)}`,
storageToken,
true,
);
const requestInit = {
method: "POST",
headers: {
"Content-Type": "application/json;odata=verbose",
"Accept": "application/json;odata=verbose",
...headers,
},
};
const response = await fetchHelper(url, requestInit);
additionalProps = response.propsToLog;

const sharingInfo = await response.content.json();
Expand Down
8 changes: 8 additions & 0 deletions packages/drivers/odsp-driver/src/odspUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import {
RetryableError,
NonRetryableError,
NetworkErrorBasic,
RequestRedirectionError,
} from "@fluidframework/driver-utils";
import { performance } from "@fluid-internal/client-utils";
import { assert } from "@fluidframework/core-utils";
Expand Down Expand Up @@ -122,6 +123,13 @@ export async function fetchHelper(
{ driverVersion },
);
}

if (!response.ok && response.redirected) {
throw new RequestRedirectionError("ODSP fetch error: Redirection", response.url, {
Philip-Scott marked this conversation as resolved.
Show resolved Hide resolved
driverVersion,
});
}

if (!response.ok || response.status < 200 || response.status >= 300) {
throwOdspNetworkError(
// pre-0.58 error message prefix: odspFetchError
Expand Down
11 changes: 11 additions & 0 deletions packages/loader/driver-utils/api-report/driver-utils.api.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -346,6 +346,17 @@ export function readAndParse<T>(storage: Pick<IDocumentStorageService, "readBlob
// @internal
export function requestOps(get: (from: number, to: number, telemetryProps: ITelemetryProperties) => Promise<IDeltasFetchResult>, concurrency: number, fromTotal: number, toTotal: number | undefined, payloadSize: number, logger: ITelemetryLoggerExt, signal?: AbortSignal, scenarioName?: string): IStream<ISequencedDocumentMessage[]>;

// @internal (undocumented)
export class RequestRedirectionError extends LoggingError {
constructor(message: string, redirectUrl: string, props: DriverErrorTelemetryProps);
// (undocumented)
readonly canRetry = true;
// (undocumented)
readonly errorType: "locationRedirection";
// (undocumented)
readonly redirectUrl: string;
}

// @internal (undocumented)
export class RetryableError<T extends string> extends NetworkErrorBasic<T> {
constructor(message: string, errorType: T, props: DriverErrorTelemetryProps);
Expand Down
1 change: 1 addition & 0 deletions packages/loader/driver-utils/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ export {
getRetryDelaySecondsFromError,
isOnline,
LocationRedirectionError,
RequestRedirectionError,
Philip-Scott marked this conversation as resolved.
Show resolved Hide resolved
NetworkErrorBasic,
NonRetryableError,
OnlineStatus,
Expand Down
17 changes: 17 additions & 0 deletions packages/loader/driver-utils/src/network.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,23 @@ export class LocationRedirectionError
}
}

/**
* @internal
*/
export class RequestRedirectionError extends LoggingError {
readonly errorType = DriverErrorTypes.locationRedirection;
readonly canRetry = true;

constructor(
message: string,
readonly redirectUrl: string,
props: DriverErrorTelemetryProps,
) {
super(message, props, new Set(["redirectUrl"]));
this.redirectUrl = redirectUrl;
}
}

/**
* @internal
*/
Expand Down
Loading