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

How to use ODSP url resolver to extract drive and item ids from ODSP Fluid file link? #4338

Closed
AndreiZe opened this issue Nov 13, 2020 · 6 comments · Fixed by #4375
Closed
Assignees
Milestone

Comments

@AndreiZe
Copy link
Contributor

ODSP url resolver space is confusing and needs some documentation.

Issue #1:
There are two url resolver implementations: OdspDriverUrlResolver and OdspDriverUrlResolverForShareLink. Which one should be used? OdspDriverUrlResolver does not know how to handle real ODSP file links and only handles url in private format. On the other hand OdspDriverUrlResolverForShareLink knows how to handle real ODSP file links but the name makes it sound that it should be used only with share links. Is that right? Clear guidance on what url resolver should be used and when is lacking.

Issue #2:
Most file APIs require drive and item ids and these are hard to extract from ODSP file share links. It seems that IOdspUrlResolver.resolve() can be used to return IOdspResolveUrl which exposes driveId and itemId. However, the problem is that OdspDriverUrlResolver.resolve() does not know how to handle ODSP file link and OdspDriverUrlResolverForShareLink.resolve() seem to work but 1) it requires to pass a number of parameters to constructor which makes creating OdspDriverUrlResolverForShareLink instance non-trivial and 2) it results in network call to /GetSharingInformation which does not make much sense if all that I need is to resolve url which already contains all necessary information. I see that there is helper method getLocatorFromOdspUrl() which takes ODSP url and returns necessary information. Is that the official way to extract drive and item ids from ODSP Fluid file link? Is it going to be supported going forward or should I use OdspDriverUrlResolverForShareLink.resolve()?

@ghost ghost added the triage label Nov 13, 2020
@AndreiZe
Copy link
Contributor Author

Tagging @jatgarg as he did most of code changes in this space.

@jatgarg
Copy link
Contributor

jatgarg commented Nov 17, 2020

@AndreiZe What is the difference between sharelink and file link?
1.) OdspDriverUrlResolver is for resolving url created with driver format which is same as url created by exposed api, createOdspUrl() on driver. OdspdriverResolvedUrlForShareLink is named as such because it creates share link or use share link if supplied in url to be resolved.
As far as usage for new resolver is concerned, why can't an app provide the token just like it supplies token for storage, push etc. The driver does not generate them. It asks from the app. Maybe the utility to provide the token needs to reside in FFX layer so that other hosts trying to use the new resolver can supply that argument. Also tagging @christiango

@christiango
Copy link
Member

@jatgarg - we can provide the sharing link token in cases where it makes sense. For the scenario we are describing we should not need to provide a sharing token, as Andrei said we just want To extract drive and item ID from the URl, this should not require a sharing token

@AndreiZe
Copy link
Contributor Author

No need for additional utility code as we already have getLocatorFromOdspUrl(). I was just asking if this is intended to be used for what I described. Maybe we should add documentation comments stating the purpose of this method?

#1 was about a confusion caused by the fact that odsp-driver has two different url resolvers: OdspDriverUrlResolver and OdspDriverUrlResolverForShareLink. We lack documentation on expected usage for these resolvers. Moreover it might be appropriate to consolidate resolver logic in single class.

@jatgarg
Copy link
Contributor

jatgarg commented Nov 18, 2020

Actually I don't think we should merge those in a single class. OdspdriverUrlResolver is just a simple url resolver which was there to test our internal scenarios and have internal driver url format. It does not know how to resolve any app url links.
The other resolver for share link, is recently taken from FFX repo which involves geneartion of share link and other utilities that app needs right now. We don't know whether same would be required for 3P scenarios etc. We also have a discussion going on whether we should have taken it or not.
I have added comments in my PR describing where each url resolver should be used. I will add a comment in getLocatorFromOdspUrl also.

@jatgarg
Copy link
Contributor

jatgarg commented Nov 18, 2020

@AndreiZe Changed my PR to add appropriate comments and removed the utility.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants