Skip to content

Add ODSP version attach to OdspContainerServices #24595

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

Closed
Closed
Changes from all 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
@@ -79,14 +79,39 @@ describe("Container create scenarios", () => {
);
});

/**
* Scenario: Test attaching a container using ODSP service.
*
* Expected behavior: an error should not be thrown nor should a rejected promise
* be returned.
*/
it("can attach a container using OdspContainerServices", async () => {
const { container, services } = await client.createContainer(schema);
const itemId = await services.attach();

if (container.connectionState !== ConnectionState.Connected) {
await timeoutPromise((resolve) => container.once("connected", () => resolve()), {
durationMs: connectTimeoutMs,
errorMsg: "container connect() timeout",
});
}

assert.strictEqual(typeof itemId, "string", "Attach did not return a string ID");
assert.strictEqual(
container.attachState,
AttachState.Attached,
"Container is not attached after attach is called",
);
});

/**
* Scenario: Test if attaching a container twice fails.
*
* Expected behavior: an error should not be thrown nor should a rejected promise
* be returned.
*/
it("cannot attach a container twice", async () => {
const { container } = await client.createContainer(schema);
const { container, services } = await client.createContainer(schema);
const itemId = await container.attach();

if (container.connectionState !== ConnectionState.Connected) {
@@ -103,6 +128,11 @@ describe("Container create scenarios", () => {
"Container is attached after attach is called",
);
await assert.rejects(container.attach(), () => true, "Container should not attach twice");
await assert.rejects(
services.attach(),
() => true,
"Container should not attach twice via ODSP services",
);
});

/**
Original file line number Diff line number Diff line change
@@ -45,6 +45,7 @@ export interface OdspConnectionConfig {

// @beta
export interface OdspContainerServices {
attach(odspProps?: ContainerAttachProps<OdspContainerAttachProps>): Promise<string>;
audience: IOdspAudience;
}

Original file line number Diff line number Diff line change
@@ -45,6 +45,7 @@ export interface OdspConnectionConfig {

// @beta
export interface OdspContainerServices {
attach(odspProps?: ContainerAttachProps<OdspContainerAttachProps>): Promise<string>;
audience: IOdspAudience;
}

11 changes: 10 additions & 1 deletion packages/service-clients/odsp-client/src/interfaces.ts
Original file line number Diff line number Diff line change
@@ -7,7 +7,11 @@ import type {
IConfigProviderBase,
ITelemetryBaseLogger,
} from "@fluidframework/core-interfaces";
import type { IMember, IServiceAudience } from "@fluidframework/fluid-static";
import type {
IMember,
IServiceAudience,
ContainerAttachProps,
} from "@fluidframework/fluid-static";

import type { IOdspTokenProvider } from "./token.js";

@@ -87,6 +91,11 @@ export interface OdspContainerServices {
* Provides an object that facilitates obtaining information about users present in the Fluid session, as well as listeners for roster changes triggered by users joining or leaving the session.
*/
audience: IOdspAudience;

Copy link
Contributor

@Josmithr Josmithr May 13, 2025

Choose a reason for hiding this comment

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

Are users expected to ever construct or implement the OdspContainerServices interface? Or do we exclusively provide objects of this type to them?

If the latter, we should consider marking the interface as @sealed so that we can add new members in the future without it being considered a breaking change.

Since this API is @beta, we can make this change. But adding a non-optional property would normally be breaking (if not sealed).

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 think we return this interface in createContainer or getContainer call and very likely we need to add new stuff to this interface for ODSP specific features. I don't think clients need to implement this interface. @vladsud for any comments.
I will try to mark it as sealed.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is type of out parameter in our API, and we expect no custom implementations.
That said, it's totally normal to construct custom implementation as a mock in tests.
I think sealed is fine.

/**
* Attach function that is used to attach the container to the ODSP service. This function is specific to the ODSP service and accepts some ODSP specific properties.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I think this can be simplified, given the context (these are ODSP-specific types)

Suggested change
* Attach function that is used to attach the container to the ODSP service. This function is specific to the ODSP service and accepts some ODSP specific properties.
* Attaches the container to the ODSP service.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we also add a @param comment for odspProps? What does it mean for the user not to provide anything here? We should document the default policy.

*/
attach(odspProps?: ContainerAttachProps<OdspContainerAttachProps>): Promise<string>;
}

/**
12 changes: 9 additions & 3 deletions packages/service-clients/odsp-client/src/odspClient.ts
Original file line number Diff line number Diff line change
@@ -137,7 +137,7 @@ export class OdspClient {

const fluidContainer = await this.createFluidContainer(container, this.connectionConfig);

const services = await this.getContainerServices(container);
const services = await this.getContainerServices(container, fluidContainer);

return { container: fluidContainer as IFluidContainer<T>, services };
}
@@ -162,7 +162,7 @@ export class OdspClient {
container,
rootDataObject: await this.getContainerEntryPoint(container),
});
const services = await this.getContainerServices(container);
const services = await this.getContainerServices(container, fluidContainer);
return { container: fluidContainer as IFluidContainer<T>, services };
}

@@ -242,12 +242,18 @@ export class OdspClient {
return fluidContainer;
}

private async getContainerServices(container: IContainer): Promise<OdspContainerServices> {
private async getContainerServices(
container: IContainer,
fluidContainer: IFluidContainer,
): Promise<OdspContainerServices> {
return {
audience: createServiceAudience({
container,
createServiceMember: createOdspAudienceMember,
}),
attach: async (odspProps?: ContainerAttachProps<OdspContainerAttachProps>) => {
return fluidContainer.attach(odspProps);
},
};
}

Loading
Oops, something went wrong.