Handling of the MIP labels update from JoinSession and Fluid Signals from push and exposing them on container #19142
Conversation
…from push and exposing them on container
github-actions
bot
added
area: driver
github-actions
bot
removed
the
dependencies
⯅ @fluid-example/bundle-size-tests: +1.64 KB
Baseline commit: 084ced9 |
github-actions
bot
added
the
area: tests
| /** | ||
| * Emitted when the some of the properties related to the container are initialized or updated. | ||
| */ | ||
| (event: "metadataUpdate", listener: (metadata?: Record<string, unknown> | undefined) => void); |
There was a problem hiding this comment.
Why can it be undefined? I'd rather see {} used in such case (if it implies - all properties are empty), not undefined.
Please add comments about semantics how it should be interpreted by consumer. I see two ways:
- Merging properties / communicates only what changed: If some property is missing on this payload, but existed before (in
containerMetadatastate below), then it did not change and stays the same. If property needs to be deleted, it's value will be set to undefined inmetadatavalue on this event. - Overwrite: If some property is missing in
metadata, it means it is removed from set.
There was a problem hiding this comment.
The join session will always return the labels and not just the updates even if they are empty. So as far as labels is concerned, we will have the state always. I want to go with this semantic for these updates as you previously mentioned:
Merging properties / communicates only what changed: If some property is missing on this payload, but existed before (in containerMetadata state below), then it did not change and stays the same. If property needs to be deleted, it's value will be set to undefined in metadata value on this event.
As then the update provider does not need to bother about all props and even container can itself add props to the metadata as the service might not know all the props, so complete overwrite semantics does not work. If someone wants to reset a prop, then they need to provide that in update otherwise it will remain same.
| @@ -1546,6 +1559,22 @@ export class Container | |||
| this._deltaManager.connect(args); | |||
| } | |||
|
|
|||
| private readonly metadataUpdateHandler = (metadata?: Record<string, unknown> | undefined) => { | |||
| this._containerMetadata = { ...this._containerMetadata, ...metadata }; | |||
There was a problem hiding this comment.
This looks fishy (in terms of merging old & new).
Assuming that we get data only through joinSession, I see not much difference between first call and second call to joinSession. I'd assume that it returns full set of properties (not just update from previous state), as otherwise we will not have full info (ever) - we have no initial state.
And if it's full set, then property missing in second update should be - it's gone. But if we merge like that, it will stay around.
Let's clarify (through comments and code actions) semantics of these updates.
There was a problem hiding this comment.
The join session will always return the labels and not just the updates even if they are empty. So as far as labels is concerned, we will have the state always. I want to go with this semantic for these updates as you previously mentioned:
Merging properties / communicates only what changed: If some property is missing on this payload, but existed before (in containerMetadata state below), then it did not change and stays the same. If property needs to be deleted, it's value will be set to undefined in metadata value on this event.
As then the update provider does not need to bother about all props and even container can itself add props to the metadata as the service might not know all the props, so complete overwrite semantics does not work. If someone wants to reset a prop, then they need to provide that in update otherwise it will remain same.
| @@ -14,6 +14,12 @@ export class DocumentServiceCompressionAdapter extends DocumentServiceProxy { | |||
| private readonly _config: ICompressionStorageConfig, | |||
| ) { | |||
| super(service); | |||
| // Back-compat Old driver | |||
There was a problem hiding this comment.
any other adapters that we should care about?
How do we ensure we do not miss any (and have no events flowing)?
It's probably fine, but I wonder, should loader be able to verify that driver is of same or higher version, and if so - assert that ability to subscribe to events is provided? It's rather weak check, and a bit ugly, so not sure if we want to go that route..
There was a problem hiding this comment.
It does not seem like we have more adapters as the compilation would have failed, I also scanned the code a bit.
I don't want to compare package version, as their format also keeps changing. We could also check that
if (service instanceOf EventEmiter), then listen to event. OR
we could expose some prop on IDocumentServicePolicies and check that and then subscribe to event.
Description
ADO Task Link: https://dev.azure.com/fluidframework/internal/_workitems/edit/6640
Handling of the MIP labels update from JoinSession and Fluid Signals from push and exposing them on container so that the apps can listen to the updates and take appropriate action like update their UX etc.
The details about the server side changes etc. are included in the design document.
1.) Add event " metadataUpdate " in IDocumentServiceEvents which are emitted on IDocumentService.
export interface IDocumentServiceEvents extends IEvent {
/
IContainer.containerMetadata./
(event: "metadataUpdate", listener: (metadata: Record<string, string>) => void);
}*
2.) DeltaConnection will extract the labels from the join session response and will also extract it from the fluid signals in the driver layer and whenever it is updated by either path, it will tell IDocumentService to emit the
metadataUpdateevent on the service object.3.) Container will listen to this event and then it will emit this info on itself. For that we will add the same event in IContainerEvents.
export interface IContainerEvents extends IEvent {
/
/
(event: " metadataUpdate ", listener: (metadata: Record<string, string>) => void);
}*
Also expose containerMetadata: Record<string, string> on IContainer, so that it can read at any time.
We are adding this prop on the container as user should be able to read it on load also, besides getting any update through events when the update happens. The emitted event will only have the props which are updated while the contianerMetadata will have full set of props.
5.) Apps can listen to this event to get the latest policy labels. Apps can also read this on load from the container object once the container is connected. It will be exposed like this on the event or in containerMetadata.
Container.containerMetadata = { sensitivityLabelsInfo: value(string)}
Value will be jsonified string. So, app needs to parse it and it will contain same structure as we received from the service which is:
{ labels: Object, timestamp: string } where labels is of structure:
[
{
"sensitivityLabelId": string,
"tenantId": string,
"assignmentMethod": string,
}
]