Skip to content

Fix selection of DEDICATED_CHANNELS strategy #418

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Float07
Copy link
Contributor

@Float07 Float07 commented Mar 7, 2025

Issue

I noticed that the DEDICATED_CHANNELS strategy was being selected every time we were querying based on the _id field. Most of the time, that's fine. However, this apparently was creating problems when:

  • We tried to filter the _id field with an operator other than $in
  • We tried to filter using the _id field and another field at the same time

Solution

I modified the getStrategy function. Now it only selects the DEDICATED_CHANNELS strategy if both of these are true:

  • The only field being used in the query is _id
  • The _id field is either a string or an object with only the $in field

This solved all the problems I was having while trying to implement Redis Oplog in my project.

Notes

I'm a first-time contributor, so I don't know that much about the repo. Please, let me know if you find any problems in my solution and I'll try to improve it.

Also, we're still using Meteor 2.16, and I think the newest versions of Redis oplog aren't compatible with it. It would be great for us if this fix could be made available for versios of Redis Oplog that are compatible with Meteor 2.16. Would that be possible? Let me know if I can help with that.

@StorytellerCZ
Copy link
Collaborator

StorytellerCZ commented Mar 27, 2025

Let's see if the tests pass. If yes, then it will be fine for 3.x branch. I think we might want to backport this to 2.x as well.

@StorytellerCZ
Copy link
Collaborator

Tests failed. Maybe I did the conflict resolution wrong. @Float07 please check.

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