Page MenuHomePhabricator

[client-backup] add function to create backup-service URL
ClosedPublic

Authored by kamil on Aug 29 2023, 2:06 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Oct 27, 4:09 PM
Unknown Object (File)
Sep 27 2024, 10:56 PM
Unknown Object (File)
Sep 27 2024, 10:56 PM
Unknown Object (File)
Sep 27 2024, 10:56 PM
Unknown Object (File)
Sep 27 2024, 10:50 PM
Unknown Object (File)
Sep 20 2024, 10:11 AM
Unknown Object (File)
Sep 19 2024, 11:22 PM
Unknown Object (File)
Sep 19 2024, 6:59 PM
Subscribers

Details

Summary

Function to create backup endpoint path.

Depends D8990

Test Plan

Tested later in the stack.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

kamil held this revision as a draft.
lib/utils/backup-service.js
8 ↗(On Diff #30469)

As a consequence of the way BackupServiceHTTPEndpoint is defined we can call this function with a blob service endpoint and flow won't complain.
On the other hand makeBlobServiceEndpointURL has the same problem - it probably wasn't noticed since it was defined when blob service was the only service available.

I don't fell strong about requesting changes here since I am not sure how serious it is. However it doesn't look right to me that we are introducing type but the type system is able to detect incorrect usage.

lib/utils/backup-service.js
8 ↗(On Diff #30469)

Agree with that - the blob client endpoints were very simple and the introduced type system was more about useful constants than any verification. Even the replacePathParams function is very simple and replaces anything that starts with :.

In the long term, these helpers should be file-private and used only to build higher level APIs like these in D8993-D8996.
For now, I wouldn't block it either because it just works well, and the aforementioned 'higher level APIs' are also introduced.

If we really want to be more strict, we can replace type of endpoint.path from string to union of explicit strings: type BackupServiceEndpointPath = '/backups' | '/backups/:backup_id/user_keys' | ... - this should solve the issue (checked this quickly - flow will complain)

marcin added inline comments.
lib/utils/backup-service.js
8 ↗(On Diff #30469)

@kamil - if you can quickly introduce:

If we really want to be more strict, we can replace type of endpoint.path from string to union of explicit strings: type BackupServiceEndpointPath = '/backups' | '/backups/:backup_id/user_keys' | ... - this should solve the issue (checked this quickly - flow will complain)

then I think it is worth doing. If it is too much work then create follow up task.

This revision is now accepted and ready to land.Aug 29 2023, 5:53 AM
lib/utils/backup-service.js
8 ↗(On Diff #30469)

Done!