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

Filter out unsubscribed documents key in DocEvent #463

Merged
merged 5 commits into from
Feb 15, 2023

Conversation

chacha912
Copy link
Contributor

@chacha912 chacha912 commented Feb 10, 2023

What this PR does / why we need it:

image

When firing a DocEvent, instead of directly passing the documentKeys of the published event,
it is passed to the subscribed documentKey.
Previously, an error occurred in the following code when a client that only subscribed to docKey1
received a documentKeys of [docKey1, docKey2].

func (c *Client) Watch(
  ctx context.Context,
  docs ...*document.Document,
) (<-chan WatchResponse, error) {
  // ...
  // handle WatchDocumentsResponse
  handleResponse := func(pbResp *api.WatchDocumentsResponse) (*WatchResponse, error) {
    switch resp := pbResp.Body.(type) {
    case *api.WatchDocumentsResponse_Event:
      eventType, err := converter.FromEventType(resp.Event.Type)

      switch eventType {
      case types.DocumentsWatchedEvent, types.DocumentsUnwatchedEvent, types.PresenceChangedEvent:
        for _, k := range converter.FromDocumentKeys(resp.Event.DocumentKeys) {
           cli, err := converter.FromClient(resp.Event.Publisher)
           // 💣 The document Keys in WatchDocumentsResponse may contain document keys 
           // that the client is not subscribed to.  
           attachment := c.attachments[k.String()]				 
     }
}

Which issue(s) this PR fixes:

Address yorkie-team/yorkie-js-sdk#455

Special notes for your reviewer:

Does this PR introduce a user-facing change?:


Additional documentation:


Checklist:

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

@codecov
Copy link

codecov bot commented Feb 10, 2023

Codecov Report

Merging #463 (faafccc) into main (de20c70) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #463      +/-   ##
==========================================
+ Coverage   46.56%   46.60%   +0.03%     
==========================================
  Files          70       70              
  Lines        5723     5727       +4     
==========================================
+ Hits         2665     2669       +4     
  Misses       2764     2764              
  Partials      294      294              
Impacted Files Coverage Δ
server/backend/sync/memory/pubsub.go 47.57% <100.00%> (+2.11%) ⬆️

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

@hackerwins
Copy link
Member

As we discussed, I think it would be nice to add test code like below.

  1. clientA attaches doc1
  2. clientB attaches doc1
  3. clientA attaches doc2. At this time, checks whether information of doc2 is coming to clientB

@chacha912
Copy link
Contributor Author

@hackerwins Thank you for suggesting a testing procedure.
I added the test as follows:

  1. Client A attaches doc1
  2. Client A watches doc1
  3. Client B attaches doc1 and doc2
  4. Client B watches doc1 and doc2. <- At this point, check whether Client A receive the information of doc2

In the original logic, an error occurs at step 4, but the test is passed after modification.
panic runtime

@chacha912 chacha912 requested a review from hackerwins February 15, 2023 05:42
@chacha912 chacha912 marked this pull request as ready for review February 15, 2023 05:43
@chacha912 chacha912 marked this pull request as draft February 15, 2023 05:44
@chacha912 chacha912 marked this pull request as ready for review February 15, 2023 06:05
Copy link
Member

@hackerwins hackerwins left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution.
I left a trivial comment.

test/integration/peer_awareness_test.go Show resolved Hide resolved
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