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

Sync fixes #103

Merged
merged 8 commits into from
Jun 1, 2023
Merged

Sync fixes #103

merged 8 commits into from
Jun 1, 2023

Conversation

mistermoe
Copy link
Contributor

@mistermoe mistermoe commented Jun 1, 2023

This PR fixes two issues related to sync (specifically push):

  1. account for how record updates work.
    • updating a record actually creates another RecordsWrite with the same recordId. only the first and most recent RecordsWrite messages are kept for a given recordId. any in between are outright nuked from everywhere. Prior to this PR, sync would attempt to push these non-existent records which would end up resulting in Sync failed due to error: Error: TODO: message not found
  2. account for scenario where a record is written and deleted before a push occurs.
    • RecordsDelete messages keep the initial RecordsWrite and simply delete the data associated to that message. Prior to this PR, sync would attempt to fetch data for a deleted record which would end up resulting in Sync failed due to error: Error: (404) Failed to read data associated with record <record_id>

💡 Note that decentralized-identity/dwn-server#31 needs to be merged in order for the 2nd issue to be fixed

This PR will also be used to bump the relevant package versions to get the fix published the NPMjs.com

@mistermoe
Copy link
Contributor Author

fix can be tested by doing the following:

  1. add the following to packages/web5/examples:
<!DOCTYPE html>
<html lang="en">

<head>
  <meta charset="UTF-8">
  <meta http-equiv="X-UA-Compatible" content="IE=edge">
  <meta name="viewport" content="width=device-width, initial-scale=1.0">
  <title>Document</title>
</head>

<body>
  <script type="module">
    import { Web5 } from '/node_modules/@tbd54566975/web5/dist/browser.mjs';

    const { web5, did: aliceDid } = await Web5.connect({
      techPreview: {
        dwnUrls: ['http://localhost:3000']
      }
    });
    console.log(aliceDid);

    const { record, status } = await web5.dwn.records.create({
      data: "Hello Web5",
      message: {
        dataFormat: 'text/plain',
      },
    });

    console.log('WRITE RESULT', status);

    const readResult = await record.data.text();
    console.log('READ RESULT', readResult);

    const updateResult = await record.update({ data: "Hello, I'm updated!" });
    console.log('UPDATE RESULT', updateResult);

    const deleteResult = await record.delete();
    console.log('DELETE RESULT', updateResult);
  </script>
</body>

</html>
  1. clone dwn-server. checkout tombstones branch. npm install && npm run server
  2. run npx http-server . from root of web5-js repo. fire up browser and head over to http://localhost:8080. load html

@michaelneale
Copy link
Contributor

Nice 👍

@codecov
Copy link

codecov bot commented Jun 1, 2023

Codecov Report

Merging #103 (2576969) into main (e6892e8) will increase coverage by 0.14%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #103      +/-   ##
==========================================
+ Coverage   65.50%   65.64%   +0.14%     
==========================================
  Files          31       31              
  Lines        3383     3397      +14     
  Branches      195      195              
==========================================
+ Hits         2216     2230      +14     
  Misses       1165     1165              
  Partials        2        2              
Components Coverage Δ
crypto 0.00% <ø> (ø)
dids 48.61% <ø> (ø)
web5 81.18% <ø> (ø)
web5-agent 0.00% <ø> (ø)
web5-proxy-agent 0.00% <ø> (ø)
web5-user-agent 79.34% <100.00%> (+0.23%) ⬆️

@frankhinek frankhinek added the bug Something isn't working label Jun 1, 2023
@frankhinek frankhinek merged commit 2ac756f into main Jun 1, 2023
@frankhinek frankhinek deleted the sync-bug branch June 2, 2023 15:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Regression: Sync failure after delete record (when using quickstart) (v 0.7.5)
3 participants