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

Add arbitrary key-value storage #217

Merged
merged 6 commits into from
Nov 3, 2023

Conversation

HerbCaudill
Copy link
Collaborator

@HerbCaudill HerbCaudill commented Oct 31, 2023

This PR adds support for arbitrary key-value storage to StorageSubsystem. The load, save, and remove methods are for generic key/value storage, and the loadDoc, saveDoc, and removeDoc methods are for Automerge documents.

For example, these will be used by the LocalFirstAuthProvider to persist the encrypted team graph that encodes group membership and permissions.

 const namespace = "MyCoolAdapter" 
 const key = "ABC123" 

 await storage.save(namespace, key, value) 
  
 const reloadedValue = await storage.load(namespace, key) 
  
 await storage.remove(namespace, key) 

The namespace parameter is to prevent collisions with other users of the storage subsystem. Typically this will be the name of the plug-in, adapter, or other system that is using it. For example, the LocalFirstAuthProvider uses the namespace LocalFirstAuthProvider.

* Removes the Automerge document with the given ID from storage
*/
async removeDoc(documentId: DocumentId) {
void this.storageAdapter.removeRange([documentId, "snapshot"])
Copy link
Member

Choose a reason for hiding this comment

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

is it worth doing an indexedDB migration to clean this up and stop squatting on the whole documentId namespace?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh I really don't think so. Maybe it would have been cleaner to do it that way from the beginning but if there's not a problem I would just leave it. As long as nobody but the storage subsystem talks directly to the storage adapters it should be fine.

return keyHash(headsbinary)
}
/** Flag to avoid compacting when a compaction is already underway */
#compacting = false
Copy link
Member

Choose a reason for hiding this comment

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

can't prevent another tab from doing this... still probably a good idea but worth testing concurrent compactions

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah - maybe create a separate issue to track that?

* This function returns the chunk type ("snapshot" or "incremental") if the key is in one of these
* forms.
*/
export function chunkTypeFromKey(key: StorageKey): ChunkType | null {
Copy link
Member

Choose a reason for hiding this comment

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

i'm not sure there's a good reason to differentiate these anymore. what do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's true that we're not doing anything with the information except to ensure that it's one or the other, but I think the additional clarity doesn't hurt anything & could be helpful for debugging etc.

@HerbCaudill HerbCaudill changed the title storage: documentation & minor refactoring Storage: documentation & minor refactoring Nov 1, 2023
@HerbCaudill HerbCaudill changed the title Storage: documentation & minor refactoring Add arbitrary key-value storage Nov 1, 2023
@HerbCaudill HerbCaudill marked this pull request as ready for review November 1, 2023 11:31
@pvh pvh merged commit 15d21cd into node-adapter-remove-from-cache Nov 3, 2023
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.

2 participants