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

Verify sequence when processing pushpull #66

Closed
wants to merge 5 commits into from

Conversation

humdrum
Copy link
Contributor

@humdrum humdrum commented Mar 30, 2023

What this PR does / why we need it:

  • Add sequence validation logic.
    • When applying changePack, the clientSeq sent by the server has to
      • '>=' the sequence of localChanges.first
      • '<=' the sequence of localChanges.last
      • If not, the client can't make a proper change pack.
    • When applying changePack, the serverSeq sent by the server has to equal to prevServerSeq + 1
  • Update serverSeq with the actual data.
  • Add Yorkie.sequenceCorrupted Error
    • Pause realtime sync when occurred.
  • Add DocEvent corrupted.
    • When a document has a bad sequence. DocEvent.corrupted event fired.
    • Any suggestion for this is welcome. (@chacha912 )

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:
CI test will be success after yorkie-team/yorkie#503 is merged.

Does this PR introduce a user-facing change?:


Additional documentation:


Checklist:

  • Added relevant tests or not required
  • Didn't break anything

@humdrum humdrum added the enhancement 🌟 New feature or request label Mar 30, 2023
@humdrum humdrum requested review from hackerwins and chacha912 March 30, 2023 05:48
@humdrum humdrum self-assigned this Mar 30, 2023
@codecov
Copy link

codecov bot commented Mar 30, 2023

Codecov Report

Merging #66 (7e21e98) into main (b5ec0be) will decrease coverage by 0.21%.
The diff coverage is 7.04%.

@@            Coverage Diff             @@
##             main      #66      +/-   ##
==========================================
- Coverage   56.93%   56.72%   -0.21%     
==========================================
  Files          92       92              
  Lines       13953    14007      +54     
==========================================
+ Hits         7944     7946       +2     
- Misses       6009     6061      +52     
Impacted Files Coverage Δ
Sources/Core/Client.swift 0.68% <0.00%> (-0.01%) ⬇️
Sources/Document/DocEvent.swift 0.00% <0.00%> (ø)
Sources/Document/Document.swift 32.75% <0.00%> (-7.79%) ⬇️
Sources/Document/Change/ChangeID.swift 93.02% <50.00%> (-6.98%) ⬇️
Sources/API/Converter.swift 60.39% <100.00%> (+0.05%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@humdrum
Copy link
Contributor Author

humdrum commented Jun 14, 2023

Since the sequence validation has been defined by the role of the server, this PR is closed.

@humdrum humdrum closed this Jun 14, 2023
@humdrum humdrum deleted the verify_changepack_sequence branch August 31, 2023 10:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement 🌟 New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant