Add ODSP version attach to OdspContainerServices

[yunho-microsoft]

Description

Add attach function to OdspContainerServices. It is more like exposing the ODSP specific arguments to ODSP client. Previously, the attach is at the IFluidContainer level which is a generic interface. Some arguments like filePath and fileName are not available in the function. But internally, we already support these arguments, through an internal logic by reassigning the attach function at ODSPClient. This change is just to expose it at ODSP interface.

User will be aware of the available args for ODSP specifically and can call OdspContainerServices.attach(odspArgs) to use the ODSP version of the attach function. It will just call the same function as before.

Reviewer Guidance

The review process is outlined on this wiki page.

[Copilot]

Pull Request Overview

Adds an ODSP-specific attach method to the OdspContainerServices API, allowing callers to pass ODSP-only attach properties directly.

• Exposes attach(odspProps) on OdspContainerServices
• Updates getContainerServices to accept the fluid container instance and return an attach wrapper
• Adds end-to-end tests for successful attach via services.attach() and double-attach rejection

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
packages/service-clients/odsp-client/src/odspClient.ts	Pass fluidContainer into getContainerServices and add attach
packages/service-clients/odsp-client/src/interfaces.ts	Define attach in OdspContainerServices with JSDoc
packages/service-clients/odsp-client/api-report/odsp-client.beta.api.md	Reflect new attach in beta API report
packages/service-clients/odsp-client/api-report/odsp-client.alpha.api.md	Reflect new attach in alpha API report
packages/service-clients/end-to-end-tests/odsp-client/src/test/containerCreate.spec.ts	Add tests for services.attach() behavior

Comments suppressed due to low confidence (1)

packages/service-clients/odsp-client/src/odspClient.ts:254

• [nitpick] Consider adding a test that passes specific odspProps (e.g., filePath, fileName) to services.attach() to verify they’re correctly forwarded to the underlying attach call.

attach: async (odspProps?: ContainerAttachProps<OdspContainerAttachProps>) => {

[github-actions]

🔗 No broken links found! ✅

Your attention to detail is admirable.

linkcheck output

> fluid-framework-docs-site@0.0.0 ci:check-links /home/runner/work/FluidFramework/FluidFramework/docs
> start-server-and-test "npm run serve -- --no-open" 3000 check-links

1: starting server using command "npm run serve -- --no-open"
and when url "[ 'http://127.0.0.1:3000' ]" is responding with HTTP status code 200
running tests using command "npm run check-links"


> fluid-framework-docs-site@0.0.0 serve
> docusaurus serve --no-open

[SUCCESS] Serving "build" directory at: http://localhost:3000/

> fluid-framework-docs-site@0.0.0 check-links
> linkcheck http://localhost:3000 --skip-file skipped-urls.txt

Crawling...

Stats:
  195689 links
    1565 destination URLs
    1797 URLs ignored
       0 warnings
       0 errors

[Josmithr]

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).

[yunho-microsoft]

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.

[vladsud]

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.

[Josmithr]

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.

[Josmithr]

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.

[ChumpChief]

I'm not clear why this is needed? OdspContainerAttachProps supposedly supports passing filePath and fileName?

[yunho-microsoft]

@ChumpChief The reason of this change is that external user has no knowledge of this OdspContainerAttachProps in IFluidContainer's attach method. The old signature is attach(props?: ContainerAttachProps): Promise, where ContainerAttachProps is translated to unknown. To let user be aware of what can be passed to the attach function, I add it to ODSP services, since filePath/fileName only applies to ODSPClient, not AzureClient. The functionalities is working now but the signature is not what we want.
@vladsud please correct me if I am wrong.

[ChumpChief]

@ChumpChief The reason of this change is that external user has no knowledge of this OdspContainerAttachProps in IFluidContainer's attach method. The old signature is attach(props?: ContainerAttachProps): Promise, where ContainerAttachProps is translated to unknown. To let user be aware of what can be passed to the attach function, I add it to ODSP services, since filePath/fileName only applies to ODSPClient, not AzureClient. The functionalities is working now but the signature is not what we want. @vladsud please correct me if I am wrong.

If it's just a typing issue I would expect we'd try to fix the typing on IFluidContainer then rather than add a second way to call attach() - duplicating the functionality seems confusing to a user. Perhaps IFluidContainer should take a second generic argument for its attach props typing? @sonalideshpandemsft might also have some ideas here.

[vladsud]

@ChumpChief The reason of this change is that external user has no knowledge of this OdspContainerAttachProps in IFluidContainer's attach method. The old signature is attach(props?: ContainerAttachProps): Promise, where ContainerAttachProps is translated to unknown. To let user be aware of what can be passed to the attach function, I add it to ODSP services, since filePath/fileName only applies to ODSPClient, not AzureClient. The functionalities is working now but the signature is not what we want. @vladsud please correct me if I am wrong.

If it's just a typing issue I would expect we'd try to fix the typing on IFluidContainer then rather than add a second way to call attach() - duplicating the functionality seems confusing to a user. Perhaps IFluidContainer should take a second generic argument for its attach props typing? @sonalideshpandemsft might also have some ideas here.

@ChumpChief, I think you are giving contradicting guidance here: #22129 (comment)

The last time we talked about it, you suggested to go this route.

[vladsud]

@yunho-microsoft , I'd love to see two more additions for this change:

1. Make appropriate change such that passing anything to IFluidContainer.attach() would fail. See IFluidContainer.attach() test #22144.
2. If I remember correctly, actually path property is not used (even though present on API signature). Please trace the code / run experiments to confirm and make appropriate change - either remove it or make it work (if my claim is correct)

[sonalideshpandemsft]

@ChumpChief The reason of this change is that external user has no knowledge of this OdspContainerAttachProps in IFluidContainer's attach method. The old signature is attach(props?: ContainerAttachProps): Promise, where ContainerAttachProps is translated to unknown. To let user be aware of what can be passed to the attach function, I add it to ODSP services, since filePath/fileName only applies to ODSPClient, not AzureClient. The functionalities is working now but the signature is not what we want. @vladsud please correct me if I am wrong.

If it's just a typing issue I would expect we'd try to fix the typing on IFluidContainer then rather than add a second way to call attach() - duplicating the functionality seems confusing to a user. Perhaps IFluidContainer should take a second generic argument for its attach props typing? @sonalideshpandemsft might also have some ideas here.

I agree with @ChumpChief. The change: #18425 was intended to align fileName and filePath more closely with the attach flow for the ODSP client. While making this change, I wasn’t sure how to make the update without introducing a breaking change.

Ideally, the IFluidContainer.attach function should accept a type-safe props object, rather than requiring a separate attach function specifically for the ODSP client.

[ChumpChief]

@ChumpChief, I think you are giving contradicting guidance here: #22129 (comment)

The last time we talked about it, you suggested to go this route.

I don't recall how that conversation ended, but in your link I'm objecting to a new IOdspFluidContainer, not to a new generic on IFluidContainer.

Way back in 1.0 release I suggested that we keep attach() off the IFluidContainer but was ultimately overruled. My original proposal was for createContainer() to return { container, attach, services }, as this is what I had explored with the ModelLoader.

Whatever we do here we should keep our patterns consistent across service-clients.