Add readonly API to IFluidContainer

[yunho-microsoft]

Description

Add readonly API to IFluidContainer. This makes users of ODSPClient able to get the readonly status of the container. Also expose the readonly event handler.

Testing: add a new test to check readonly flag after container attachment. Also test the readonly event handler by triggering the forceReadonly function in IContainer

Reviewer Guidance

The review process is outlined on this wiki page.

[Copilot]

Pull Request Overview

This PR introduces a readonly API to IFluidContainer, allowing users to inspect and enforce readonly mode on containers. Key changes include:

• Addition of a new readonly property and forceReadonly method to IFluidContainer and its implementation.
• Emitting a new "readonly" event when the container’s readonly state changes.
• Updates to API reports and end-to-end tests to support the new readonly functionality.

Reviewed Changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
packages/service-clients/end-to-end-tests/odsp-client/src/test/containerCreate.spec.ts	Adds a new test validating the readonly flag and event firing.
packages/loader/container-loader/src/connectionManager.ts	Inserts a debug log in connectCore; likely a leftover debugging aid.
packages/framework/fluid-static/src/fluidContainer.ts	Implements and wires up the readonly API changes.
API Report files	Updates API definitions for public, beta, alpha, and legacy releases.
packages/dds/tree/api-report/tree.alpha.api.md	Updates readonly API documentation in tree API reports.

[vladsud]

@jason-ha, could you please help Yunpeng to get it in? I.e. guide on what's required to expose new functionality (and not break any rules around versioning). Thanks!

[jason-ha]

While this would be different than others, I recommend that all new "properties" that are dynamic (and especially if they will be implemented with logic) be written a method. This could be isReadOnly() or getReadOnlyState().
cc @microsoft/fluid-cr-api

[yunho-microsoft]

Fixed and use getReadOnlyState

[jason-ha]

That update works for me. I would like API v-team to confirm this. (unresolving for visibility)
@microsoft/fluid-cr-api?

[ChumpChief]

I'm personally OK with any of the above - afaik we haven't established a strict rule about whether dynamic members should be properties/methods. My slight preference is for readOnly > isReadOnly() > getReadOnlyState(). "State" seems to imply a more complex return type than Boolean, and "is" is always a nice prefix for Boolean.

My personal rule is to switch to a method if it requires computation (particularly potentially expensive computation).
Rather than switching to a method just for being dynamic.

Here's what we have currently:
https://github.com/microsoft/FluidFramework/wiki/Coding-Guidelines#naming

[jason-ha]

There is compute. At least on the API surface (though it is informative internally too), a method helps callers know that it is not a simple property (maybe it is dynamic or maybe complex) and they shouldn't capture it simply and expect it to remain the same.
Interestingly for connected the compute appears light but can be incredibly deep. I think I counted something around 20-25 frames when debugging once to get to the real source of state.

[ChumpChief]

I don't mind a method here, and of course "how much compute is too much for a property" is a little fuzzy. I also have mixed feelings about our immutable-but-lazily-instantiated properties that might be expensive on first access.

There are plenty of examples of dynamic values in native JS APIs that represent as properties though (e.g. Array.length), so while I respect the desire to hint that capturing is not appropriate I think it can be OK to explain this in documentation in some cases.

Acknowledged that this is probably a good topic for API council to discuss in more detail and come up with something consistent that we can recommend.

[yunho-microsoft]

Based on discussion, I decide to use isReadOnly as the member function name and the event name is readonlyChanged. @jason-ha , @ChumpChief Please take a look

[ChumpChief]

Can we make this a plain boolean? "undefined" is confusing here, it would be nice to have a clear signal to the customer "should I try writing or not"

[yunho-microsoft]

@ChumpChief So you mean return true if the flag is actually undefined? Internally we have the flag as type boolean | undefined. And as the comment states, the readOnly flag is unknown when connection is not established yet. I believe the write will fail if client tries to write when readOnly is undefined since no connection yet. So I prefer keep it same as the interal type. @vladsud Any comments

[ChumpChief]

I think this is proving my point that it is confusing :) By returning undefined, it pushes the work to the customer to interpret what that means and what they should do in response. A friendlier API just lets them know "go ahead and write" vs. "don't try to write".

[yunho-microsoft]

Fixed. Make it return boolean. Undefined -> true for read only value. Please take a look. @ChumpChief

[vladsud]

I'd rather make it a tri-state (enum), i.e. push to caller to decide what to do when we have no information (undefined means - we have no clue if file is read-only or not). While we should provide some guidance (like - in most cases if you do not have this info, you can assume the container is writable, and handling of situation is not much different from other places where client learns about write -> read changes with delay and needs to deal with it), I think we should not make a call for the caller on what to do here, especially that it would be (statistically speaking) the majority of cases where client would operate with read-only files without learning yet that it's read-only file.

[ChumpChief]

I don't understand what you are saying, and so I don't think a customer would either.

What would you put in the documentation to a customer for the "undefined" state? Should they permit write attempts or not? The customer's decision of what to do is boolean at the end of the day, I don't see why we should make them deduce what the correct boolean decision is from a tri-state.

[jason-ha]

Can I ask, what might be done with container that is not yet connected and we don't know compared to when known and read-only or writable?
Maybe what should/should not be done when unknown? If caller does attempt to write before connection and then there is no write access, what happens?
If we can or want to treat the unknown case like one of the others, then fine we can just use a boolean. If special consideration is needed, then we should use a tri-state and the property may need to be renamed to not appear as binary.

[yunho-microsoft]

@jason-ha I prefer to use tri-state. And the documentation can clarify that if readonly is undefined/unknown, it means the flag is unknown and container disconnected, please check container's connected state.
And in this way we give whatever we know about the flag and let developers decide what to do next. It also simplify the logic between IFluidContainer layer and IContainer layer for flag translation (from undefined to true/false) and saves the potential event changes since we raise event when flag turns from undefined -> true/false (we need to omit the event if we decide to make undefined -> true/false). Actually I am not sure what value to put there (true or false?) when the internal state is unknown.

[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:
  210260 links
    1663 destination URLs
    1895 URLs ignored
       0 warnings
       0 errors

[ChumpChief]

Should match camel casing between the event and property - either isReadOnly/readOnlyChanged or isReadonly/readonlyChanged. Otherwise this naming and use of method looks good to me!

[ChumpChief]

It looks like undefined is more commonly equated with false in container.ts - probably check with @vladsud what he'd recommend doing in the undefined case.

Also, you should make sure that the readonlyChanged event is only emitted when the computed value of this property actually changes (so, for example it shouldn't emit for container.readOnlyInfo.readonly going from undefined -> true as currently written here since isReadOnly() would return true both before and after that state change). I see some guards in connectionManager.ts that suppress raising the event if the state didn't change, but it's not immediately clear if that will prevent the above possibility.

[yunho-microsoft]

That's part of the reason I prefer keep undefined. This PR is to exposing the readonly information to IFluidContainer rather than fixing any existing logic. And internally we emit the event when trying to set the readonly flag. undefined -> true is a readonly state change (from unknown to true/false).

[ChumpChief]

We should be able to resolve this here though, one direction or the other. The public API surface has a higher quality bar than the legacy API surface given its longer support/compat contract, and also because it needs to be consumable by third parties who we may not be able to directly tutorialize on the quirks of the underlying system.

[vladsud]

Left a comment in above comment - undefined means we have no clue. We need socket connection to learn if user has write access to a file.

[yunho-microsoft]

Changes moved to clients' repo