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.

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

Follow up to add nav parameter #23845

Merged
merged 13 commits into from
Feb 18, 2025
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,7 @@ export class OdspDriverUrlResolver implements IUrlResolver {

// @alpha
export class OdspDriverUrlResolverForShareLink implements IUrlResolver {
constructor(shareLinkFetcherProps?: ShareLinkFetcherProps | undefined, logger?: ITelemetryBaseLogger, appName?: string | undefined, getContext?: ((resolvedUrl: IOdspResolvedUrl, dataStorePath: string) => Promise<string | undefined>) | undefined);
constructor(shareLinkFetcherProps?: ShareLinkFetcherProps | undefined, logger?: ITelemetryBaseLogger, appName?: string | undefined, getContext?: ((resolvedUrl: IOdspResolvedUrl, dataStorePath: string) => Promise<string | undefined>) | undefined, containerPackageInfo?: IContainerPackageInfo | undefined);
appendDataStorePath(requestUrl: URL, pathToAppend: string): string | undefined;
appendLocatorParams(baseUrl: string, resolvedUrl: IResolvedUrl, dataStorePath: string, packageInfoSource?: IContainerPackageInfo): Promise<string>;
static createDocumentUrl(baseUrl: string, driverInfo: OdspFluidDataStoreLocator): string;
Expand Down
4 changes: 3 additions & 1 deletion packages/drivers/odsp-driver/src/createFile/createFile.ts
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,9 @@ export async function createNewFluidFile(

odspResolvedUrl.context = resolvedUrl?.context;
odspResolvedUrl.appName = resolvedUrl?.appName;
odspResolvedUrl.codeHint = resolvedUrl?.codeHint;
odspResolvedUrl.codeHint = odspResolvedUrl.codeHint?.containerPackageName
? odspResolvedUrl.codeHint
: resolvedUrl?.codeHint;

if (shareLinkInfo?.createLink?.link) {
let newWebUrl = shareLinkInfo.createLink.link.webUrl;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import {
} from "@fluidframework/driver-definitions/internal";
import { ISharingLinkKind } from "@fluidframework/odsp-driver-definitions/internal";

import { buildOdspShareLinkReqParams, getContainerPackageName } from "./odspUtils.js";
import { buildOdspShareLinkReqParams } from "./odspUtils.js";

/**
* Create the request object with url and headers for creating a new file on OneDrive Sharepoint
Expand All @@ -37,7 +37,7 @@ export function createOdspCreateContainerRequest(
const createNewRequest: IRequest = {
url: `${siteUrl}?driveId=${encodeURIComponent(driveId)}&path=${encodeURIComponent(
filePath,
)}${containerPackageInfo ? `&containerPackageName=${getContainerPackageName(containerPackageInfo)}` : ""}${shareLinkRequestParams ? `&${shareLinkRequestParams}` : ""}`,
)}${shareLinkRequestParams ? `&${shareLinkRequestParams}` : ""}`,
Copy link
Contributor

Choose a reason for hiding this comment

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

@MarioJGMsoft Linter might be unhappy about the unused containerPackageInfo - you can rename the parameter starting with an _ to avoid the issue

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Applied change

headers: {
[DriverHeader.createNew]: {
fileName,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,7 @@ export class OdspDocumentServiceFactoryCore
!!this.hostPolicy.sessionOptions?.forceAccessTokenViaAuthorizationHeader,
odspResolvedUrl.isClpCompliantApp,
this.hostPolicy.enableSingleRequestForShareLinkWithCreate,
odspResolvedUrl,
Copy link
Contributor

Choose a reason for hiding this comment

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

Even though it is not related to this PR, I really would like us to change the name of this variable from odspResolveUrl to something else that represents the original name. Something like resolvedUrlForCreateNew. Let me know if that doesn't sound right to you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that the name can and should be changed to something more related, but I also think that it might be best to leave that change to another PR. Will create the follow up task for it.

)
: module.createNewContainerOnExistingFile(
getAuthHeader,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ export class OdspDriverUrlResolverForShareLink implements IUrlResolver {
* navigates directly to the link.
* @param getContext - callback function which is used to get context for given resolved url. If context
* is returned then it will be embedded into url returned by getAbsoluteUrl() method.
* @param containerPackageInfo - container package information which will be used to extract the container package name.
*/
public constructor(
shareLinkFetcherProps?: ShareLinkFetcherProps | undefined,
Expand All @@ -82,6 +83,7 @@ export class OdspDriverUrlResolverForShareLink implements IUrlResolver {
resolvedUrl: IOdspResolvedUrl,
dataStorePath: string,
) => Promise<string | undefined>,
private readonly containerPackageInfo?: IContainerPackageInfo | undefined,
Copy link
Contributor

Choose a reason for hiding this comment

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

| undefined type shouldn't be needed here since the property is already optional right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm guessing it's there because the call to get context might return undefined, but the function can still exist.

) {
this.logger = createOdspLogger(logger);
if (shareLinkFetcherProps) {
Expand Down Expand Up @@ -155,6 +157,10 @@ export class OdspDriverUrlResolverForShareLink implements IUrlResolver {

odspResolvedUrl.appName = this.appName;

odspResolvedUrl.codeHint = odspResolvedUrl.codeHint?.containerPackageName
? odspResolvedUrl.codeHint
: { containerPackageName: getContainerPackageName(this.containerPackageInfo) };

if (isSharingLinkToRedeem) {
// We need to remove the nav param if set by host when setting the sharelink as otherwise the shareLinkId
// when redeeming the share link during the redeem fallback for trees latest call becomes greater than
Expand Down Expand Up @@ -257,7 +263,8 @@ export class OdspDriverUrlResolverForShareLink implements IUrlResolver {

const containerPackageName: string | undefined =
getContainerPackageName(packageInfoSource) ??
odspResolvedUrl.codeHint?.containerPackageName;
odspResolvedUrl.codeHint?.containerPackageName ??
getContainerPackageName(this.containerPackageInfo);
Copy link
Contributor

Choose a reason for hiding this comment

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

Add comment explaining why the chaining here is done.

Something like, "containerPackage information can be either provided via the constructor or to the method's params. We give priority to the locally provided params, and fallback to the constructor provided value" (re-phrase maybe to make it more understandable).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added change


odspResolvedUrl.appName = this.appName;

Expand Down
134 changes: 74 additions & 60 deletions packages/drivers/odsp-driver/src/test/createNewUtilsTests.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,24 @@ import {
SharingLinkRole,
SharingLinkScope,
} from "@fluidframework/odsp-driver-definitions/internal";
import { createChildLogger } from "@fluidframework/telemetry-utils/internal";
import { createChildLogger, MockLogger } from "@fluidframework/telemetry-utils/internal";

import { useCreateNewModule } from "../createFile/index.js";
import { createOdspCreateContainerRequest } from "../createOdspCreateContainerRequest.js";
import { EpochTracker } from "../epochTracker.js";
import { LocalPersistentCache } from "../odspCache.js";
import { OdspDocumentServiceFactory } from "../odspDocumentServiceFactory.js";
import {
OdspDriverUrlResolverForShareLink,
type ShareLinkFetcherProps,
} from "../odspDriverUrlResolverForShareLink.js";
import { getHashedDocumentId } from "../odspPublicUtils.js";
import { IExistingFileInfo, INewFileInfo, createCacheSnapshotKey } from "../odspUtils.js";
import {
IExistingFileInfo,
INewFileInfo,
createCacheSnapshotKey,
getOdspResolvedUrl,
} from "../odspUtils.js";

import { mockFetchOk } from "./mockFetch.js";

Expand Down Expand Up @@ -324,16 +335,15 @@ describe("Create New Utils Tests", () => {
});

// Extract the Base64 encoded value of `nav`
const base64Value = mockSharingLinkData.webUrl.match(/nav=([^&]*)/)?.[1] as string;
const base64Value = odspResolvedUrl.shareLinkInfo.createLink.link.webUrl.match(
/nav=([^&]*)/,
)?.[1] as string;
// Decode the Base64 value to UTF-8, \r�� is being stored at the end of the string so we slice it off
const decodedValue = fromBase64ToUtf8(base64Value).slice(0, -3);

// Compare the values to make sure that the nav parameter was added correctly
assert.equal(decodedValue, "s=%2FsiteUrl&d=driveId&f=mockItemId&c=%2F&fluid=1");

// Reset the webUrl to the original value
mockSharingLinkData.webUrl = "https://mock.url";

// Test that error message is set appropriately when it is received in the response from ODSP
const mockSharingError = {
error: {
Expand Down Expand Up @@ -471,70 +481,74 @@ describe("Create New Utils Tests", () => {
assert(!odspResolvedUrl2.isClpCompliantApp, "isClpCompliantApp should be falsy");
await epochTracker.removeEntries().catch(() => {});
});

it("Should set the appropriate nav param info when a resolved url is sent", async () => {
const mockOdspResolvedUrl: IOdspResolvedUrl = {
...resolvedUrl,
odspResolvedUrl: true,
summarizer: true,
dataStorePath: "/dataStorePath",
codeHint: {
containerPackageName: "mockContainerPackageName",
},
fileVersion: "mockFileVersion",
context: "mockContext",
appName: "mockAppName",
const odspDocumentServiceFactory = new OdspDocumentServiceFactory(
async (_options) => "token",
async (_options) => "token",
new LocalPersistentCache(2000),
{ snapshotOptions: { timeout: 2000 }, enableSingleRequestForShareLinkWithCreate: true },
);

const expectedResponse = {
context: "http://sp.devinstall/_api/v2.1/$metadata#",
Copy link
Contributor

@pragya91 pragya91 Feb 19, 2025

Choose a reason for hiding this comment

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

Any reason for using this exact URL? Could we use a dummy one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we could use a dummy one and I added the change.

sequenceNumber: 1,
sha: "shaxxshaxx",
itemUrl: `http://fake.microsoft.com/_api/v2.1/drives/${driveId}/items/${itemId}`,
driveId,
itemId,
id: "Summary Handle",
sharing: mockSharingData,
dataStorePath: "/path",
};
const odspResolvedUrl = await useCreateNewModule(createChildLogger(), async (module) =>
mockFetchOk(
async () =>
module.createNewFluidFile(
async (_options) => "token",
newFileParams,
createChildLogger(),
createSummary(),
epochTracker,
fileEntry,
false /* createNewCaching */,
false /* forceAccessTokenViaAuthorizationHeader */,
undefined /* isClpCompliantApp */,
true /* enableSingleRequestForShareLinkWithCreate */,
mockOdspResolvedUrl,
),
{
itemId: "mockItemId",
id: "mockId",
sharing: mockSharingData,
sharingLinkErrorReason: undefined,
},
{ "x-fluid-epoch": "epoch1" },
),

const fileName = "fileName";
const request = createOdspCreateContainerRequest(siteUrl, driveId, filePath, fileName);
const logger = new MockLogger();

const sharingLinkFetcherProps: ShareLinkFetcherProps = {
tokenFetcher: async () => "token",
identityType: "Enterprise",
};

const getContext = async (
Copy link
Contributor

Choose a reason for hiding this comment

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

The function is defined as async, but I don't see any promises used

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the constructor of OdspDriverUrlResolverForShareLink, getContext is a function call that returns a Promise of <strin | undefined>, so the getContext function has to be async.

url: IOdspResolvedUrl,
dataStorePath: string,
): Promise<string> => {
if (dataStorePath === "") {
return "context";
}
return url.dataStorePath ?? "context";
};
const shareLinkResolver = new OdspDriverUrlResolverForShareLink(
sharingLinkFetcherProps,
logger,
"appName",
getContext,
{ name: "containerPackageName" },
Copy link
Contributor

Choose a reason for hiding this comment

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

{ name: "containerPackageName" } /*IContainerPackageInfo*/,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

applied change

);

// 's=%2FsiteUrl&d=driveId&f=mockItemId&c=%2F&fluid=1&a=mockAppName&p=mockContainerPackageName&x=mockContext'
// Update the webUrl to the version that has the nav parameter that was supposed to be added
mockSharingLinkData.webUrl =
"https://mock.url/?nav=cz0lMkZzaXRlVXJsJmQ9ZHJpdmVJZCZmPW1vY2tJdGVtSWQmYz0lMkYmZmx1aWQ9MSZhPW1vY2tBcHBOYW1lJnA9bW9ja0NvbnRhaW5lclBhY2thZ2VOYW1lJng9bW9ja0NvbnRleHQ%3D";
const resolved = await shareLinkResolver.resolve(request);
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a comment above this like explaining that we are trying to emulate the 'container' behavior where we first create a resolved object for the provided request to invoke driver's createContainer request.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Applied change

const summary = createSummary();
const docService = await mockFetchOk(
async () => odspDocumentServiceFactory.createContainer(summary, resolved, logger),
expectedResponse,
{ "x-fluid-epoch": "epoch1" },
);

const finalResolverUrl = getOdspResolvedUrl(docService.resolvedUrl);

assert.deepStrictEqual(odspResolvedUrl.shareLinkInfo?.createLink, {
shareId: mockSharingData.shareId,
link: {
role: mockSharingData.sharingLink.type,
...mockSharingData.sharingLink,
},
error: undefined,
});
// Extract the Base64 encoded value of `nav`
const base64Value = mockSharingLinkData.webUrl.match(/nav=([^&]*)/)?.[1] as string;
// Decode the Base64 value to UTF-8, need to slice off last unnecessary character
const decodedValue = fromBase64ToUtf8(base64Value).slice(0, -1);
const base64Value = finalResolverUrl.shareLinkInfo?.createLink?.link?.webUrl.match(
/nav=([^&]*)/,
)?.[1] as string;
// Decode the Base64 value to UTF-8, \r�� is being stored at the end of the string so we slice it off
const decodedValue = fromBase64ToUtf8(base64Value).slice(0, -3);
Copy link
Contributor

Choose a reason for hiding this comment

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

Where is \r�� coming from?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure, it's being added when I decode the value of navParam, so I'm not sure at what point of the code flow it's being added to the encoded nav param. I don't think it can become an issue but let me know if you think otherwise.


// Compare the values to make sure that the nav parameter was added correctly
assert.equal(
decodedValue,
"s=%2FsiteUrl&d=driveId&f=mockItemId&c=%2F&fluid=1&a=mockAppName&p=mockContainerPackageName&x=mockContext",
"s=%2FsiteUrl&d=driveId&f=itemId&c=%2F&fluid=1&a=appName&p=containerPackageName&x=context",
Copy link
Contributor

Choose a reason for hiding this comment

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

@MarioJGMsoft , Let us also test for scenarios where appName is not provided, but all other params are. Same with containerPackageName. Just to ensure the nav param is not misconstructed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Applied change

);

// Reset the webUrl to the original value
mockSharingLinkData.webUrl = "https://mock.url";
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -457,20 +457,4 @@ describe("Odsp Driver Resolver", () => {
`https://placeholder/placeholder/${resolvedUrl.hashedDocumentId}/` + `${testFilePath}`;
assert.strictEqual(resolvedUrl.url, expectedResolvedUrl, "resolved url is wrong");
});
it("Should create request with containerPackageName and resolve it", async () => {
request = createOdspCreateContainerRequest(
siteUrl,
driveId,
filePath,
fileName,
undefined,
{ name: "testContainerPackageName" }, // Container package info variable,
);
const resolvedUrl = await resolver.resolve(request);
assert.strictEqual(
resolvedUrl.codeHint?.containerPackageName,
"testContainerPackageName",
"containerPackageName should match",
);
});
});
Loading