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

[UI] Support repo snapshots #462

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

senwang86
Copy link
Collaborator

Summary

  • Creating a new database table called YDocSnapshot that stores repo snapshots at the request of users.
  • The implement features a strict database consistency, once receiving a request, it reads the yDocBlob in the Repo table and inserts it into YDocSnapshot. It avoids conflicts between Repo.yDocBlob in the event of network interruption.
  • "Rename/restore/delete" operations will be added after discussion.

Test

  • Attach the shell of the api docker container
  • Run npx prisma migrate dev
  • Restart api container

add_snapshots

@senwang86 senwang86 requested a review from lihebi August 18, 2023 04:32
repoId: String
createdAt: String
message: String
yDocBlob: String
Copy link
Collaborator

Choose a reason for hiding this comment

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

This blob doesn’t need to be sent to the front end.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This blob doesn’t need to be sent to the front end.

SG, I was thinking about the "restore" operation, I am not exactly sure if it's better to pass the yDocBlob in a single query.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Restore should probably happen in the backend yjs server, and front end will retrieve from yjs. I’ll think about this.

id: await nanoid(),
yDocBlob: repo.yDocBlob,
message: message,
repoId: repoId,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use the connect clause to connect the two tables.

*/
async function getRepoSnapshots(_, { repoId }) {
const snapshots = await prisma.yDocSnapshot.findMany({
where: { repoId: repoId },
Copy link
Collaborator

Choose a reason for hiding this comment

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

{repo: {id: repoId}}

createdAt DateTime @default(now())
message String?
yDocBlob Bytes?
Repo Repo? @relation(fields: [repoId], references: [id])
Copy link
Collaborator

Choose a reason for hiding this comment

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

Lower case “repo” field name.

createdAt DateTime @default(now())
message String?
yDocBlob Bytes?
deleted Boolean @default(false)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't want to add deleted here since it adds complexity. We could reply on DB backup if we really want to ensure data safety.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am not so familiar with DB backup, any reference for its pros and cons? I am considering a scenario where users might decide to delete a snapshot, but then want to undo the deletion later. Furthermore, if there are 1,000 users performing these operations at different times, can DB backups gracefully address this scenario?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd skip this scenario for now. If the user deletes it, it is deleted.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'd skip this scenario for now. If the user deletes it, it is deleted.

OK, let me revert the change.

message String?
yDocBlob Bytes?
deleted Boolean @default(false)
repo Repo? @relation(fields: [repoId], references: [id])
Copy link
Collaborator

@lihebi lihebi Aug 18, 2023

Choose a reason for hiding this comment

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

yDocBlob and repo/repoId sound to be required, not optional. I.e.,

   yDocBlob Bytes
   repo: Repo
   repoId: String

Comment on lines 74 to 77
yDocBlob Bytes?
repo Repo? @relation(fields: [repoId], references: [id])
repoId String?
yDocBlob Bytes
repo Repo @relation(fields: [repoId], references: [id])
repoId String
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

You need to generate a new DB migration to reflect this change.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

bb35923 addes the prisma migration.

@lihebi
Copy link
Collaborator

lihebi commented Aug 19, 2023

One last comment: merge the two migrations into one? I.e. remove the migrations and re-generate one.

@senwang86
Copy link
Collaborator Author

One last comment: merge the two migrations into one? I.e. remove the migrations and re-generate one.

d291948 address this.

@lihebi
Copy link
Collaborator

lihebi commented Aug 19, 2023

Awesome, thanks!

@lihebi
Copy link
Collaborator

lihebi commented Aug 20, 2023

Since this PR involves DB schema change, I'd like to ensure that the restoring would work under this design before merging.

I was thinking about how to restore a snapshot. Two methods:

  • Option 1 (not ideal): Directly replacing the ydoc to a previous state. This isn't ideal, as the peers see a different yDoc with different history, impossible to sync.
  • Option 2 (ideal): Add a new "restoring" changeset on top of the latest ydoc. This should be the ideal way.

In terms of implementation, I found that Yjs has a "Y.Snapshot". So instead of storing a whole YDocBlob into snapshots, we could store a Y.Snapshot binary and use Y.snapshot APIs for restoring. But this snapshot seems to only support method 1, which is not ideal.

To support method 2, we might need to implement a "restoring" changeset, like mentioned here.

I'll need some time to think about it before we merge this PR and work on restoring.

@senwang86
Copy link
Collaborator Author

senwang86 commented Aug 30, 2023

Realized that the YDocSnapshot needs a field of userId to separate the snapshots among various collaborators. WDYT, @lihebi ?

@lihebi
Copy link
Collaborator

lihebi commented Aug 30, 2023

I don't think so. The snapshot is associated with the repo. Each repo has only one line of snapshots, regardless of which user triggered it.

@senwang86
Copy link
Collaborator Author

I don't think so. The snapshot is associated with the repo. Each repo has only one line of snapshots, regardless of which user triggered it.

Will the snapshot list show snapshots from all other collaborators?

@lihebi
Copy link
Collaborator

lihebi commented Aug 31, 2023

Yes. There's only one snapshot line for the repo; everyone sees and operates on it. Keep it simple.

@lihebi
Copy link
Collaborator

lihebi commented Aug 31, 2023

There are more things to consider.

I've been thinking about how to make the revision system to:

  1. play well with the Git system and
  2. to be able to function on scopes

Also, the revision system code wouldn't need to be included in the public-facing desktop app. It would be exclusive to SaaS app, meaning the code would be in another repo.

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