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

centralize space id util functions #2816

Merged
merged 1 commit into from
May 3, 2022

Conversation

C0rby
Copy link
Contributor

@C0rby C0rby commented May 2, 2022

Merged the utility functions regarding space ids or references into one package. Also updated the functions to support the newly added provider id in the spaces id.

@C0rby C0rby self-assigned this May 2, 2022
@C0rby C0rby force-pushed the merge-space-id-functions branch 2 times, most recently from 373ee83 to d28813e Compare May 2, 2022 18:17
butonic
butonic previously requested changes May 2, 2022
internal/grpc/services/storageprovider/storageprovider.go Outdated Show resolved Hide resolved
Comment on lines +126 to +131
// "storage_id!opaque_id/path"
// "storage_id/path"
Copy link
Contributor

@butonic butonic May 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the opaque_id might contain /, eg for the localfs, which uses the path ins the fileid. we may have to change localfs to use another form of id ... it already uses sqlite. might as well use an identifier for fileid to path lookup.

I currently only see FormatReference used in the share cache ... if so make this code a private function there. I fear people might expect to be able to parse FormatReference back, which might not always be the case.

edit: I see FormatReference used in owncloud/ocis#3648 ... I hope that only ends up as some form of cache key as well...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we have more places in the code where we manually build a storage space reference. Usually like path.Join(storageid + opaqueid, relPath). But I think it would be beneficial to have that logic in a function so that changing the format of the reference string is easy. That is what this function should be for. If we don't need/want it fine. 🤷

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, that makes sense. Maybe add this intention to the doc string?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add this intention to the doc string?

I don't think we need a comment saying "Use this instead of building your own." Or do you mean something else?

@C0rby C0rby force-pushed the merge-space-id-functions branch from d28813e to d0b9eff Compare May 2, 2022 19:01
@C0rby C0rby force-pushed the merge-space-id-functions branch from d0b9eff to b3d192b Compare May 2, 2022 20:03
@C0rby C0rby requested a review from butonic May 2, 2022 20:04
@sonarcloud
Copy link

sonarcloud bot commented May 2, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

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 this pull request may close these issues.

None yet

3 participants