Skip to content
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

Use tenant domain instead of siteUrl in /shares api in ODSP driver #20738

Merged
merged 5 commits into from
Apr 21, 2024

Conversation

jatgarg
Copy link
Contributor

@jatgarg jatgarg commented Apr 18, 2024

Description

Task Link

To load a Fluid container, host/app provides a URL which is used to talk to service to load the container. That URL can be a share link which host/app specifies explicitly to Fluid that the URL is a share link.
Fluid then uses that link to fetch the snapshot for the container. User sometimes don’t have permissions initially, so to save COGS, Fluid supply this share link in body of trees latest call to the service, so that in 1 network call, we can do the redeem and also fetch the snapshot. Sometimes this redeem on service side fails (in case it is first time sharing with that user), then the service throws on the trees latest call. Fluid catches this exception and determines/guess if this is because of redeem failure at service side and then fallback to doing redeem by itself using the /shares API. Once the redeem fails even in /shares API, the container load fails.

In this issue, for outlook Win32 the /shares API (Documentation link for API: Link) call is failing because of the length limitation on the API. There are 2 ways to call the /shares API(for Fluid use),

Method 1: site-url/_api/v2.0/shares/{shareID}
This has length limitation of about 400 characters.

Method 2: tenant-domain/_api/v2.0/shares/{shareID}
Then the length limitation is about 2048 characters.
where this, share ID is generated by encoding the share link supplied by the app in the above load request. So, encoding longer URL will give longer shareID.

Tenant-domain can be extracted from site-url by doing:
Const tenant-domain = new URL(site-url).origin

This issue occurred for Outlook win 32, because MDO team started adding some, additional long query params to the share link.
So, we are switching to use Method 1 instead of Method 2.

@jatgarg jatgarg self-assigned this Apr 18, 2024
@github-actions github-actions bot added area: driver Driver related issues area: odsp-driver base: main PRs targeted against main branch labels Apr 18, 2024
@vladsud vladsud requested a review from pragya91 April 19, 2024 20:01
@msfluid-bot
Copy link
Collaborator

msfluid-bot commented Apr 19, 2024

@fluid-example/bundle-size-tests: +1.39 KB
Metric NameBaseline SizeCompare SizeSize Diff
aqueduct.js 453.02 KB 453.02 KB No change
azureClient.js 548.79 KB 548.79 KB No change
connectionState.js 680 Bytes 680 Bytes No change
containerRuntime.js 256.09 KB 256.09 KB No change
fluidFramework.js 339.61 KB 339.61 KB No change
loader.js 132.34 KB 132.34 KB No change
map.js 41.46 KB 41.46 KB No change
matrix.js 143.45 KB 143.45 KB No change
odspClient.js 516.68 KB 517.15 KB +477 Bytes
odspDriver.js 97.06 KB 97.52 KB +477 Bytes
odspPrefetchSnapshot.js 41.93 KB 42.39 KB +470 Bytes
sharedString.js 161.23 KB 161.23 KB No change
sharedTree.js 339.61 KB 339.61 KB No change
Total Size 3.15 MB 3.15 MB +1.39 KB

Baseline commit: 55b0f70

Generated by 🚫 dangerJS against 22a7932

@vladsud
Copy link
Contributor

vladsud commented Apr 19, 2024

Method 1: /_api/v2.0/shares/{shareID}
Method 2: /_api/v2.0/shares/{shareID}

I do not see any difference :)

@jatgarg
Copy link
Contributor Author

jatgarg commented Apr 19, 2024

Method 1: /_api/v2.0/shares/{shareID}
Method 2: /_api/v2.0/shares/{shareID}

I do not see any difference :)

Haha. It was in < > which made them disappear.

Co-authored-by: Navin Agarwal <45832642+agarwal-navin@users.noreply.github.com>
@jatgarg jatgarg merged commit 39b74f4 into microsoft:main Apr 21, 2024
30 checks passed
jatgarg added a commit to jatgarg/FluidFramework-1 that referenced this pull request Apr 21, 2024
…icrosoft#20738)

## Description

[Task
Link](https://dev.azure.com/fluidframework/internal/_workitems/edit/7650)

To load a Fluid container, host/app provides a URL which is used to talk
to service to load the container. That URL can be a share link which
host/app specifies explicitly to Fluid that the URL is a share link.
Fluid then uses that link to fetch the snapshot for the container. User
sometimes don’t have permissions initially, so to save COGS, Fluid
supply this share link in body of trees latest call to the service, so
that in 1 network call, we can do the redeem and also fetch the
snapshot. Sometimes this redeem on service side fails (in case it is
first time sharing with that user), then the service throws on the trees
latest call. Fluid catches this exception and determines/guess if this
is because of redeem failure at service side and then fallback to doing
redeem by itself using the /shares API. Once the redeem fails even in
/shares API, the container load fails.

In this issue, for outlook Win32 the /shares API (Documentation link for
API:
[Link](https://docs.microsoft.com/en-us/onedrive/developer/rest-api/api/shares_get))
call is failing because of the length limitation on the API. There are 2
ways to call the /shares API(for Fluid use),

Method 1: site-url/_api/v2.0/shares/{shareID}
This has length limitation of about 400 characters.

Method 2: tenant-domain/_api/v2.0/shares/{shareID}
Then the length limitation is about 2048 characters.
where this, share ID is generated by encoding the share link supplied by
the app in the above load request. So, encoding longer URL will give
longer shareID.

Tenant-domain can be extracted from site-url by doing:
Const tenant-domain = new URL(site-url).origin

This issue occurred for Outlook win 32, because MDO team started adding
some, additional long query params to the share link.
So, we are switching to use Method 1 instead of Method 2.

---------

Co-authored-by: Jatin Garg <jatingarg@Jatins-MacBook-Pro-2.local>
Co-authored-by: Navin Agarwal <45832642+agarwal-navin@users.noreply.github.com>
@agarwal-navin
Copy link
Contributor

@jatgarg I just realized that the tests you added don't use real ODSP service. We should add tests that use the real service with a long URL that has these extra long query params (just less than 2048 maybe) and ensure they work. This way we will know if they start failing for some reason (say a bug in the shares API service).

jatgarg added a commit that referenced this pull request Apr 22, 2024
…/shares api in ODSP driver (#20738) (#20771)

## Description

[Task
Link](https://dev.azure.com/fluidframework/internal/_workitems/edit/7650)

To load a Fluid container, host/app provides a URL which is used to talk
to service to load the container. That URL can be a share link which
host/app specifies explicitly to Fluid that the URL is a share link.
Fluid then uses that link to fetch the snapshot for the container. User
sometimes don’t have permissions initially, so to save COGS, Fluid
supply this share link in body of trees latest call to the service, so
that in 1 network call, we can do the redeem and also fetch the
snapshot. Sometimes this redeem on service side fails (in case it is
first time sharing with that user), then the service throws on the trees
latest call. Fluid catches this exception and determines/guess if this
is because of redeem failure at service side and then fallback to doing
redeem by itself using the /shares API. Once the redeem fails even in
/shares API, the container load fails.

In this issue, for outlook Win32 the /shares API (Documentation link for
API:

[Link](https://docs.microsoft.com/en-us/onedrive/developer/rest-api/api/shares_get))
call is failing because of the length limitation on the API. There are 2
ways to call the /shares API(for Fluid use),

Method 1: site-url/_api/v2.0/shares/{shareID}
This has length limitation of about 400 characters.

Method 2: tenant-domain/_api/v2.0/shares/{shareID} Then the length
limitation is about 2048 characters. where this, share ID is generated
by encoding the share link supplied by the app in the above load
request. So, encoding longer URL will give longer shareID.

Tenant-domain can be extracted from site-url by doing: Const
tenant-domain = new URL(site-url).origin

This issue occurred for Outlook win 32, because MDO team started adding
some, additional long query params to the share link. So, we are
switching to use Method 1 instead of Method 2.
jatgarg added a commit that referenced this pull request Apr 22, 2024
…/shares api in ODSP driver (#20738) (#20769)

## Description

[Task
Link](https://dev.azure.com/fluidframework/internal/_workitems/edit/7650)

To load a Fluid container, host/app provides a URL which is used to talk
to service to load the container. That URL can be a share link which
host/app specifies explicitly to Fluid that the URL is a share link.
Fluid then uses that link to fetch the snapshot for the container. User
sometimes don’t have permissions initially, so to save COGS, Fluid
supply this share link in body of trees latest call to the service, so
that in 1 network call, we can do the redeem and also fetch the
snapshot. Sometimes this redeem on service side fails (in case it is
first time sharing with that user), then the service throws on the trees
latest call. Fluid catches this exception and determines/guess if this
is because of redeem failure at service side and then fallback to doing
redeem by itself using the /shares API. Once the redeem fails even in
/shares API, the container load fails.

In this issue, for outlook Win32 the /shares API (Documentation link for
API:

[Link](https://docs.microsoft.com/en-us/onedrive/developer/rest-api/api/shares_get))
call is failing because of the length limitation on the API. There are 2
ways to call the /shares API(for Fluid use),

Method 1: site-url/_api/v2.0/shares/{shareID}
This has length limitation of about 400 characters.

Method 2: tenant-domain/_api/v2.0/shares/{shareID} Then the length
limitation is about 2048 characters. where this, share ID is generated
by encoding the share link supplied by the app in the above load
request. So, encoding longer URL will give longer shareID.

Tenant-domain can be extracted from site-url by doing: Const
tenant-domain = new URL(site-url).origin

This issue occurred for Outlook win 32, because MDO team started adding
some, additional long query params to the share link. So, we are
switching to use Method 1 instead of Method 2.

Co-authored-by: Jatin Garg <jatingarg@Jatins-MacBook-Pro-2.local>
Co-authored-by: Navin Agarwal <45832642+agarwal-navin@users.noreply.github.com>
jatgarg added a commit that referenced this pull request Apr 22, 2024
…/shares api in ODSP driver (#20738) (#20770)

## Description

[Task
Link](https://dev.azure.com/fluidframework/internal/_workitems/edit/7650)

To load a Fluid container, host/app provides a URL which is used to talk
to service to load the container. That URL can be a share link which
host/app specifies explicitly to Fluid that the URL is a share link.
Fluid then uses that link to fetch the snapshot for the container. User
sometimes don’t have permissions initially, so to save COGS, Fluid
supply this share link in body of trees latest call to the service, so
that in 1 network call, we can do the redeem and also fetch the
snapshot. Sometimes this redeem on service side fails (in case it is
first time sharing with that user), then the service throws on the trees
latest call. Fluid catches this exception and determines/guess if this
is because of redeem failure at service side and then fallback to doing
redeem by itself using the /shares API. Once the redeem fails even in
/shares API, the container load fails.

In this issue, for outlook Win32 the /shares API (Documentation link for
API:

[Link](https://docs.microsoft.com/en-us/onedrive/developer/rest-api/api/shares_get))
call is failing because of the length limitation on the API. There are 2
ways to call the /shares API(for Fluid use),

Method 1: site-url/_api/v2.0/shares/{shareID}
This has length limitation of about 400 characters.

Method 2: tenant-domain/_api/v2.0/shares/{shareID} Then the length
limitation is about 2048 characters. where this, share ID is generated
by encoding the share link supplied by the app in the above load
request. So, encoding longer URL will give longer shareID.

Tenant-domain can be extracted from site-url by doing: Const
tenant-domain = new URL(site-url).origin

This issue occurred for Outlook win 32, because MDO team started adding
some, additional long query params to the share link. So, we are
switching to use Method 1 instead of Method 2.
@jatgarg jatgarg deleted the shares branch April 23, 2024 23:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: driver Driver related issues area: odsp-driver base: main PRs targeted against main branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants