-
Notifications
You must be signed in to change notification settings - Fork 6
[WIP] Permanently delete files #24
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -226,13 +226,13 @@ export interface RegisterOptions { | |
* Using an improper one will results in the registration being rejected. | ||
*/ | ||
deviceDesc?: | ||
| "desktop-windows" | ||
| "desktop-macos" | ||
| "desktop-linux" | ||
| "mobile-android" | ||
| "mobile-ios" | ||
| "browser-chrome" | ||
| "remarkable"; | ||
| "desktop-windows" | ||
| "desktop-macos" | ||
| "desktop-linux" | ||
| "mobile-android" | ||
| "mobile-ios" | ||
| "browser-chrome" | ||
| "remarkable"; | ||
/** | ||
* the unique id of this device | ||
* | ||
|
@@ -634,6 +634,18 @@ export interface RemarkableApi { | |
*/ | ||
delete(hash: string, refresh?: boolean): Promise<HashEntry>; | ||
|
||
|
||
/** | ||
* permanently delete an entry from all devices connected to the account | ||
* | ||
* @example | ||
* ```ts | ||
* await api.purge(file.hash); | ||
* ``` | ||
* @param hash - the hash of the entry to delete | ||
*/ | ||
purge(hash: string, refresh?: boolean): Promise<void>; | ||
|
||
/** | ||
* rename an entry | ||
* | ||
|
@@ -694,6 +706,21 @@ export interface RemarkableApi { | |
refresh?: boolean, | ||
): Promise<HashesEntry>; | ||
|
||
/** | ||
* delete many entries permanently from all devices connected to the account | ||
* | ||
* @example | ||
* ```ts | ||
* await api.bulkPurge([file.hash]); | ||
* ``` | ||
* | ||
* @param hashes - the hashes of the entries to delete | ||
*/ | ||
bulkPurge( | ||
hashes: readonly string[], | ||
refresh?: boolean, | ||
): Promise<[SimpleEntry[], string]>; | ||
|
||
/** | ||
* get the current cache value as a string | ||
* | ||
|
@@ -1308,6 +1335,11 @@ class Remarkable implements RemarkableApi { | |
async delete(hash: string, refresh: boolean = false): Promise<HashEntry> { | ||
return await this.move(hash, "trash", refresh); | ||
} | ||
|
||
/** permanently delete an entry */ | ||
async purge(hash: string, refresh: boolean = false): Promise<void> { | ||
await this.bulkPurge([hash], refresh); | ||
} | ||
|
||
/** rename an entry */ | ||
async rename( | ||
|
@@ -1381,6 +1413,26 @@ class Remarkable implements RemarkableApi { | |
return await this.bulkMove(hashes, "trash", refresh); | ||
} | ||
|
||
/** permanent delete many hashes */ | ||
async bulkPurge( | ||
petero-dk marked this conversation as resolved.
Show resolved
Hide resolved
|
||
hashes: readonly string[], | ||
refresh: boolean = false, | ||
): Promise<[SimpleEntry[], string]> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the similar methods return the entries that were modified. Similarly it seems to make sense to return something relating to the entries that were actually deleted. These don't tend to return the root hash. This API has evolved over a bit of time, so I'm open to an argument of why it helps to return something different, but baring that, I lean towards a consistent interface. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have no strong feelings on the return type. I was thinking about what could be useful data to return in this scenario and the only thing I could come up with was the root hash. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One benefit of doing something like the the other returns is that it'll tell you which were actually removed, which would indicate that maybe things were already purged. |
||
const [rootHash, generation] = await this.#getRootHash(refresh); | ||
// Get the raw text of the root entry | ||
const entries = await this.raw.getEntries(rootHash) | ||
const newEntries = entries.filter( | ||
(entry) => !hashes.includes(entry.hash)); | ||
Comment on lines
+1424
to
+1425
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would make hashes a |
||
// If we didn't delete anything, just return the current root | ||
if (newEntries.length < entries.length) { | ||
const hash = this.raw.makeListHash(newEntries); | ||
await this.#putRootHash(hash, generation); | ||
Comment on lines
+1428
to
+1429
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. from this, it looks like your setting the roothash to a raw entries file, rather than the hash to a file. I've never seen a rootHash specified this way. It may work, but if writing the entries first and then writing that hash to that file also works, I strongly lean towards that option. I also want you to be careful since this api while somewaht being non-destructive, can be destructive if you're not careful, also ideally keep backups of any documents, and a backup of the current rootHash before attempting any of this. In short, I'm skeptical that this is the right thing to do. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am doing this exactly like the remarkable app does, I added fiddler to my system in order to make sure I did it the same way. I can double check and possibly make a dump of the files so there is nothing I have missed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah, I guess try double checking. Does the rest of the api work as intended? If so, I can't imagine that the root hash would change from a hash to a raw file. Looking at the beginning of your function, the rootHash is a normal hash. You pass that into |
||
return [newEntries, hash]; | ||
} | ||
return [newEntries, rootHash]; | ||
|
||
} | ||
|
||
/** dump the raw cache */ | ||
dumpCache(): string { | ||
return this.raw.dumpCache(); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -840,6 +840,12 @@ export interface RawRemarkableApi { | |
metadata: Metadata, | ||
): Promise<[RawFileEntry, Promise<void>]>; | ||
|
||
/** | ||
* create a list hash from a set of entries | ||
* @param entries - the entries to hash | ||
*/ | ||
makeListHash(entries: RawEntry[]): string; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think there's much utility in exposing this, and it doesn't rely on the state of raw, so I'd move to a utility function. It could still be placed within this file, I just wouldn't put it in the RawApi. That said, I wouldn't make any changes until there's agreement on the rootHash, as this would be moot. |
||
|
||
/** | ||
* put a set of entries to make an entry list file | ||
* | ||
|
@@ -1145,6 +1151,14 @@ export class RawRemarkable implements RawRemarkableApi { | |
} | ||
} | ||
|
||
makeListHash(entries: RawEntry[]): string { | ||
const records = ["3\n"]; | ||
for (const { hash, type, id, subfiles, size } of entries) { | ||
records.push(`${hash}:${type}:${id}:${subfiles}:${size}\n`); | ||
} | ||
return records.join(""); | ||
} | ||
|
||
async putEntries( | ||
id: string, | ||
entries: RawEntry[], | ||
|
@@ -1161,10 +1175,8 @@ export class RawRemarkable implements RawRemarkableApi { | |
const hash = await digest(hashBuff); | ||
const size = entries.reduce((acc, ent) => acc + ent.size, 0); | ||
|
||
const records = ["3\n"]; | ||
for (const { hash, type, id, subfiles, size } of entries) { | ||
records.push(`${hash}:${type}:${id}:${subfiles}:${size}\n`); | ||
} | ||
const listHash = this.makeListHash(entries); | ||
|
||
const res: RawListEntry = { | ||
id, | ||
hash, | ||
|
@@ -1176,7 +1188,7 @@ export class RawRemarkable implements RawRemarkableApi { | |
return [ | ||
res, | ||
// NOTE when monitoring requests, this had the extension .docSchema appended, but I'm not entirely sure why | ||
this.#putFile(hash, `${id}.docSchema`, enc.encode(records.join(""))), | ||
this.#putFile(hash, `${id}.docSchema`, enc.encode(listHash)), | ||
]; | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would make this have a similar return type, e.g.
[SimpleEntry, string]
although I left a comment on the bulk purge return type as well.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason I did not return the simpleentry is because it no longer exists, but it would be a simple change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, lets wait until the more important details are hashed out.