Access sharingLinkToRedeem from IOdspResolvedUrl.shareLinkInfo

[sonalivdeshpande]

No description provided.

[sonalivdeshpande]

Reopening this issue. The changes have been reverted in 0.52 release and main. The property can be replaced only when host/apps are using loader version >= 0.50.

[ssimic2]

@sdeshpande3 could you share the comments here around the conversations you had with @agarwal-navin. I'm not sure how the comment above relates to the compatibility matrix: https://github.com/microsoft/FluidFramework/wiki/Compatibility-and-Versioning

[ssimic2]

@agarwal-navin could you please help us clarify this item. I see IOdspResolvedUrl being contained to odsp driver. Loader treats any ODSP resolver as generic IUrlResolver. And I don't see how this touches loader/runtime boundary. So, I'm not sure about the meaning behind this "The property can be replaced only when host/apps are using loader version >= 0.50." reference.

[agarwal-navin]

@sdeshpande3 Can you please add some description to this issue. It's not clear what this issue is trying to do.

@ssimic2 I believe you are right - we need to only maintain the compat of IUrlResolver and hence IResolvedUrl across loader-driver boundary. The problem with IOdspResolvedUrl and OdspDriverUrlResolverForShareLink is that it is used directly by FFX and I think they are doing it in the loader layer. This is what I had originally thought as the issue that they should not be using this in the loader layer.
+@vladsud who has more context and can help clarify things here.

[ssimic2]

@agarwal-navin are you referring to the use cases where host (for example office web host) is passing resolver & documentServiceFactory to the Loader constructor, instead of a driver type? Or these are some references to custom loaders FFX created?

On the runtime side I see Bohemia is using IOdspResolvedUrl and OdspDriverUrlResolverForShareLink to create URLs or inspect URLs that were already resolved by the container. But I don't see how this plays out: "The property can be replaced only when host/apps are using loader version >= 0.50.", which implies that change is touching the boundary, or loader has dependency on knowing about odsp specifics.

@sdeshpande3 can you please update the description.

[sonalivdeshpande]

The PR for this issue was reverted back when we came across this bug: https://teams.microsoft.com/l/message/19:53328301da384b33bcc81111124e3ab4@thread.skype/1637275433228?tenantId=72f988bf-86f1-41af-91ab-2d7cd011db47&groupId=422665a0-7ad3-4d0d-9171-e8881d0397d9&parentMessageId=1637275433228&teamName=FFX%20(Fluid%20Framework%20and%20Experiences)&channelName=Bugs%20%F0%9F%90%9B&createdTime=1637275433228

While chatting with @jatgarg , it seemed a back compat issue where the resolver was packaged with loader and had version where the share link was only set in outer object (odspResolvedUrl.shareLinkToRedeem). But the driver had version which was relying on the inner object (odspResolvedUrl.sharelinkInfo.shareLinkToRedeem) to be set and hence it was causing the issue.

[vladsud]

URI resolver belongs to host layer (i.e. host packages however they want URI resolvers).
Same for Loader.
(If they chose to dynamically deliver any of these parts, it's their problem to own).

We promise that (ODSP) Driver factor can ship dynamically. I.e. driver interface (exposed to loader & runtime) and interop between resolved URI and driver factory crosses compat line - we need to ensure that newer versions of driver work fine with IResolveUrl and driver-specific extensions of it.

I think (but worth double checking) that URI resolver and resolve URI are not available to runtime.

[ssimic2]

Thanks @vladsud. If I understood this properly, the host could have used odsp-urlResolver that was referring to some older odsp driver's OdspDriverUrlResolver. While at the same time using OdspDocumentService factory from a new odsp driver's package? I'm assuming driver factory simply means document service factory.

[agarwal-navin]

The interop between resolved URL and driver layer is for IResolvedUrl right. However, in this issue, the interface in question is IOdspResolvedUrl which is derived from IResolvedUrl and seems to be a driver layer detail that is not exposed to the loader layer.
IMO, we should guarantee that IResolvedUrl supports back-compat across driver-loader but not IOdspResolvedUrl.

[jatgarg]

I think it is actually the back-compat between older and newer odsp resolver. Maybe we can just fix it when creating the driver instance from factory and assign to the newer field if only present in the older field within the resolved url.

[ssimic2]

The issue here seems that documentService (via fetchSnapshot and subsequently redeemSharingLink) is using IOdspResolvedUrl given by an old resolver. So this particular issue was between new odsp driver and an old odsp resolver.

@agarwal-navin @jatgarg what are the compatibility guidelines here? any old resolver needs to work with new driver (forever sort of compatibility) or min version as is the current case with loader/runtime? I would be curious what sort of use cases drove this separation, but its whole another topic.

Also do we test against this sort of compatibility regressions?

[vladsud]

There is no different in IResolvedUrl vs IOdspResolvedUrl when talking about ODSP layer. These constructs exist only at compile time. The resulting object satisfies both interfaces.

Resolved URI maybe coming from older code and used by newer driver factory (and thus driver instance). That's the only versioning concern I'm aware relates to it.

@ssimic2, I'd not look at internal APIs if driver like fetchSnapshot (unless you do trace up the stack to public / exported API).
If you trace it, you should see that actual usage (of this API and any other) falls into one of two buckets:

• It's code accessible from driver factory, and thus any usage or resolved URI can be by earlier code, but resolved URI produced by older code.
• It's used in one of the functions that have no cross-compat layer usage, like snapshot prefetching, or some other APIs that are directly called by host - in this bucket all APIs are shipped simultaneously together.

Everything that exists in ods-driver-definitions package represents APIs that directly or indirectly are on the fault line between driver factory and the rest of the eco-system. These things need to change very carefully, with back-compat in mind (API shape and corresponding implementation on both sides of fault line). IOdspResolvedUrl is in this package for that reason.

[ssimic2]

Thanks @vladsud. And yes, fetchSnapshot traces back to the first bucket, since the IResolvedUrl/IOdspResolvedUrl (that is uses) has been passed via driver factory (OdspDocumentServiceFactoryCore.createDocumentService). So, essentially, resolved URI can be coming from older code.

And I see what you are saying about prefetchLatestSnapshot, this is great info.

We should be able to learn from these sort of tickets, so I may bug you on the bigger picture around this model, and use-cases there.

The immediate questions are on existing compatibility/versioning guidelines:

1. I am assuming this is backwards compatibility only ? Meaning new odsp drivers need to be compatible with older resolved odsp URLs, but not the other way around.
2. At this point in time (pre 1.0), how do we determine how far back do we go with back compatibility in this case?

[vladsud]

We support what we test. I believe LTS=0.47 (if not, probably time to bump to it)
I think we test all directions, though practically odsp driver is not older than loader
And testing odsp driver is hard, I think (Curtis will have more info here) such testing with odsp driver happens outside of ci loop because it’s not stable / there are failures. Basically we run all the same tests we run in ci loop, but against real service - that would never be as stable as ci loop

pre 1.0 does not matter here that much, we support already production scenarios!

[ssimic2]

Thanks @vladsud. Since host (like OWH) can import resolvers (via odsp drivers packages in the case of OWH) separately from loaders, I'm guessing you are assuming whenever they bump loader dependency, they would also bump dependencies for all other fluid packages (including driver package). Therefore reference to loader/driver compatibility, although this is really compat. between older resolver (that came in through older driver's package) and newer driver.

In regards to LTS, here is some usage data on the older loaders (in the last 60 days):
Data_loaderVersion Data_hostName count_
0.32.1 Fluid Preview App 2
0.33.3 Fluid Preview App 1
0.34.1 Fluid Preview App 2
0.37.4 Fluid Preview App 5
0.42.3 Fluid Preview App 16
0.45.2 Demo Host 11547
0.46.0 OneNoteMeetingNotes 3
0.46.1 OneNoteMeetingNotes 25
0.46.1 OneNoteNotes 33
0.47.0 OneNote_Android 159
...

Notes: Fluid Preview App data seem to have come in the last few days. I did not see it in the earlier queries. Demo Host should be our tests, coming from LTS being ^0.45.0