ramezrafla / redis-oplog

Using Redis for pub / subs in Meteor for infinite scalability
MIT License
29 stars 5 forks source link

MinimongoError: The positional operator did not find the match needed from the query #9

Open jasongrishkoff opened 3 years ago

jasongrishkoff commented 3 years ago

Here's the code throwing this error:

        Meteor.defer(() => {
            Campaigns.update(
                {_id:nowPlaying,detail:{$elemMatch:detail}},
                {$set:{'detail.$.listened_for':length}}
            )
        })

This is executed server-side, not on client.

Here's the full log:

gxbsm
2021-02-11 19:40:53+02:00Exception in defer callback: MinimongoError: The positional operator did not find the match needed from the querygxbsm
2021-02-11 19:40:53+02:00 at MinimongoError (packages/minimongo/common.js:1087:17)gxbsm
2021-02-11 19:40:53+02:00 at findModTarget (packages/minimongo/local_collection.js:1947:17)gxbsm
2021-02-11 19:40:53+02:00 at packages/minimongo/local_collection.js:1160:24gxbsm
2021-02-11 19:40:53+02:00 at Array.forEach (<anonymous>)gxbsm
2021-02-11 19:40:53+02:00 at packages/minimongo/local_collection.js:1144:28gxbsm
2021-02-11 19:40:53+02:00 at Array.forEach (<anonymous>)gxbsm
2021-02-11 19:40:53+02:00 at Function.LocalCollection._modify (packages/minimongo/local_collection.js:1134:27)gxbsm
2021-02-11 19:40:53+02:00 at packages/zegenie:redis-oplog/lib/mongo/mutator.js:119:27gxbsm
2021-02-11 19:40:53+02:00 at Array.forEach (<anonymous>)gxbsm
2021-02-11 19:40:53+02:00 at Mongo.Collection.update (packages/zegenie:redis-oplog/lib/mongo/mutator.js:117:14)gxbsm
2021-02-11 19:40:53+02:00 at Mongo.Collection.update (packages/zegenie:redis-oplog/lib/mongo/extendMongoCollection.js:60:29)gxbsm
2021-02-11 19:40:53+02:00 at server/methods/listens.jsx:129:23gxbsm
2021-02-11 19:40:53+02:00 at Meteor.EnvironmentVariable.EVp.withValue (packages/meteor.js:1234:12)gxbsm
2021-02-11 19:40:53+02:00 at packages/meteor.js:550:25gxbsm
2021-02-11 19:40:53+02:00 at runWithEnvironment (packages/meteor.js:1286:24)
ramezrafla commented 3 years ago

Aaaah - positional operators! Meteor has an incomplete implementation in mini-mongo How come it worked in dev, that call was never made?

ramezrafla commented 3 years ago

I am taking a closer look as we speak - FYI

jasongrishkoff commented 3 years ago

Great, thanks! Gonna add a few more errors. I didn't actually test this one properly it seems. I've been laser-focused on the chats not updating, and those were all working pretty smooth hehe.

ramezrafla commented 3 years ago

Can you check your packages folder for the commit your are using of this lib? mutator.js:117 has nothing offending in it in the current version Thanks

ramezrafla commented 3 years ago

Sorry spoke too soon -- this is the offending line LocalCollection._modify(after, modifier)

jasongrishkoff commented 3 years ago

Currently on 2.0.15. Only installed it for the first time a few hours ago :)

icellan commented 3 years ago

This is blocking me from using this package in a project. The offending query on my side is:

collection.update({
      _id: {
        $in: notificationIds,
      },
      'notifications.userId': this.userId,
    }, {
      $set: {
        'notifications.$.readOn': new Date(),
      },
    },{
      multi: true,
    });

Also tried with $elemMatch in the query:

collection.update({
      _id: {
        $in: notificationIds,
      },
      notifications: {
        $elemMatch: {
          userId: this.userId,
          readOn: {
            $exists: false
          }
        }
      }
    },{
      $set: {
        'notifications.$.readOn': new Date()
      }
    }, {
      multi: true
    });

Without the _id part in the query, so only the notifications part, it does work properly.

Had to revert to cultofcoders:redis-oplog to make this work.

Any progress on this issue yet @ramezrafla ?

Error in my case:

Exception while invoking method 'markNotificationRead' MinimongoError: The positional operator did not find the match needed from the query
    at MinimongoError (packages/minimongo/common.js:1087:17)
    at findModTarget (packages/minimongo/local_collection.js:1965:17)
    at packages/minimongo/local_collection.js:1160:24
    at Array.forEach (<anonymous>)
    at packages/minimongo/local_collection.js:1144:28
    at Array.forEach (<anonymous>)
    at Function.LocalCollection._modify (packages/minimongo/local_collection.js:1134:27)
    at packages/zegenie:redis-oplog/lib/mongo/mutator.js:116:27
    at Array.forEach (<anonymous>)
    at ns.Collection.update (packages/zegenie:redis-oplog/lib/mongo/mutator.js:114:14)
    at ns.Collection.update (packages/zegenie:redis-oplog/lib/mongo/extendMongoCollection.js:60:29)
    at ns.Collection.Mongo.Collection.<computed> [as update] (packages/aldeed:collection2/collection2.js:217:19)
    at MethodInvocation.markNotificationRead (imports/api/methods/notifications.js:12:31)
    at maybeAuditArgumentChecks (packages/ddp-server/livedata_server.js:1771:12)
    at packages/ddp-server/livedata_server.js:719:19
    at Meteor.EnvironmentVariable.EVp.withValue (packages/meteor.js:1234:12)
    at packages/ddp-server/livedata_server.js:717:46
    at Meteor.EnvironmentVariable.EVp.withValue (packages/meteor.js:1234:12)
    at packages/ddp-server/livedata_server.js:715:46
    at new Promise (<anonymous>)
    at Session.method (packages/ddp-server/livedata_server.js:689:23)
    at packages/ddp-server/livedata_server.js:559:43
RedisOplog: RaceDetectionManager started
ramezrafla commented 2 years ago

@jasongrishkoff Can you test pls with the new minimongo?

jasongrishkoff commented 2 years ago

Hi @ramezrafla I actually stopped using this package back in February - there were too many issues, so I just went back to plain old oplog and scaled everything up. Unfortunately I can't test this for you :(

Miraell commented 2 years ago

@ramezrafla it still does not work. I made a quick monkey patch (works for only simple case) for myself which looks like this:

mutator.js


116           let options = {};
117           
118           const positionalKey = Object.keys(updateSelector).filter(value => value.split('.').length > 1);
119           if (positionalKey.length === 1) {
120             const split = positionalKey[0].split('.');
121             const valueToSearch = updateSelector[positionalKey];
122             options.arrayIndices = [before[split[0]].findIndex(value => value[split[1]] === valueToSearch)];
123           }   
124             
125           LocalCollection._modify(after, modifier, options);
jasongrishkoff commented 2 years ago

FYI, was playing around with the package and can confirm this still isn't working for me either.

ramezrafla commented 2 years ago

@jasongrishkoff If I implement @Miraell 's fix above, will it work for you?

jasongrishkoff commented 2 years ago

Same error with the monkey patch. Code I'm firing looks kinda like this:

            var detail = {type:'listen',user:this.userId}
            History.update(
                {_id:id,detail:{$elemMatch:detail}},
                {$set:{
                    'detail.$.seconds':120,
                }}
            )
JackAdams commented 1 year ago

I'm having problems with the positional operator server side with:

Groups.update({"members.user_id": "asdfkjslea3q489"}, {$set: {"members.$.name": "New Name"}}, {multi: true})

The error is:

Error [MinimongoError]: The positional operator did not find the match needed from the query

Going to need to revert to cult-of-coders:redis-oplog for the time being, as there are too many positional operators used in my codebase. Pity, because zegenie:redis-oplog gets the merging of published documents with different sets of fields right (which cult-of-coders:redis-oplog doesn't).

ramezrafla commented 1 year ago

@JackAdams Could you test the monkey patch above (https://github.com/ramezrafla/redis-oplog/issues/9#issuecomment-985488735) I don't use positional operators so can't test them.

If it works, I'll make the chance and push to production

JackAdams commented 1 year ago

Seems to have fixed the problem for me; no error and correct update made in the document in the db. This being the case, I'm going to forge ahead with zegie:redis-oplog for now. Thanks for all the work on this @ramezrafla , I really appreciate it. (I notice that @Miraell mentioned it only works in simple cases. I think most/all of the cases I use the position operator in would be simple cases that the patch covers. It might be worth mentioning in the docs, though, that the positional operator only supports basic use cases. Feel free to use that line of code in my comment above as an example.)

ramezrafla commented 1 year ago

Great! We have been using this version of redis-oplog in production with tens of thousands of users simultaneously. So we know it works for our application. As new use cases come up, I'll try to incorporate without jeopardizing its mission to be scalable.

Will update and push to production shortly.

JackAdams commented 1 year ago

Out of interest, @ramezrafla , have you managed to have tens of thousands of concurrent users, with multiple subscriptions each, making frequent writes, and still use mongo unsharded?

ramezrafla commented 1 year ago

Yes! That's the purpose of this package. No sharding yet. The cache within zegenie:redis-oplog dramatically reduces hits to the DB. Since each user is connected via websocket, we can keep their recurrent data. When we write to the DB, we don't need to repull (like original redis-oplog), we simply update the local cache.

Which brings the next question, with positional operators in place, please check that the data in the client is also updated (not just DB).

JackAdams commented 1 year ago

Happy to confirm that changes are being correctly propagated to the client after an update using the positional operator.

JackAdams commented 1 year ago

Super happy to hear about the scalability. Even with cult-of-coders:redis-oplog the database was really hurting with a lot of users making frequent writes. (Full respect to Theo and his team, too, by the way -- redis-oplog was a huge step forward for scalability of Meteor apps with a lot of pub/sub.)

ramezrafla commented 1 year ago

Excellent! Theo handled a lot of the interfacing with redis and the internals of Meteor. There is a bit of hacking in there to override internal pub/subs. So he did all the heavy lifting.

JackAdams commented 1 year ago

@ramezrafla . Maybe hold off pushing that patch to production. I just did and have a lot of users reporting issues. Will update when I know more.

JackAdams commented 1 year ago

Been testing, and the fix for the issues I was seeing is to make sure the type is an array when trying to find the array index to use for the positional operator. (This is because I was using a string of dot separated fields to query nested objects, not arrays, which is a common enough practice, so the suggested fix to the patch below if (_.isArray(before[split[0]])) should definitely be used if this patch is going to production.)

let options = {};
const positionalKey = Object.keys(updateSelector).filter(value => value.split('.').length > 1);
if (positionalKey.length === 1) {
  const split = positionalKey[0].split('.');
  const valueToSearch = updateSelector[positionalKey];
  if (_.isArray(before[split[0]])) {
    options.arrayIndices = [before[split[0]].findIndex(value => value[split[1]] === valueToSearch)];
  }
}
LocalCollection._modify(after, modifier, options);

This code block replaces:

LocalCollection._modify(after, modifier)

Are you still using the original test suite, @ramezrafla ?

ramezrafla commented 1 year ago

Excellent @JackAdams If that works, we can push in production.

I am not using the test suites, to simplify things. In hindsight, maybe a mistake.

JackAdams commented 1 year ago

I've pushed that modified patch to production and so far, so good. I'll let you know if we run into any problems.

JackAdams commented 1 year ago

Okay. Better not put that patch into production. It seems to be giving some false positives for the old version of alanning:roles, which was a popular package and may still be in production in some apps. Given the way the data for the roles is structured in user docs I can sort of guess what's happening. I need to test a bit more. (It also means that any queries on nested docs using dot delimited strings probably aren't working as advertised!) Edit: possibly a false alarm. I'll let you know when I've finished investigating.

JackAdams commented 1 year ago

I don't think it was anything to do with the patch, in the end. But this is really weird. On the Mongo console:

meteor:PRIMARY> db.users.findOne({_id:"ZqRoBNapiGcQTtTBT", nonExistentField: "nonExistentValue"})
null

Which is what I expected. But in the Meteor shell and in my app code:

> Meteor.users.findOne({_id:"ZqRoBNapiGcQTtTBT", nonExistentField: "nonExistentValue"})
{
  _id: 'ZqRoBNapiGcQTtTBT',

Which shouldn't be happening at all. This only happens when I've got Meteor.users.startCaching() in the server code. Any idea what's going on? (Note: this happens with or without the patch.) I've just tested this with other collections and that seems to be consistent. As long as there's an _id that matches, it doesn't matter what other fields there are, the findOne method returns the document. This is not the case for find, which works as expected. This strange behaviour of the findOne method only happens on the server for collections with caching started; it works as expected on the client.

Edit: sorry, I'll split this out into a separate issue (#25)

The positional operator patch (with the guard condition with _.isArray) remains good for production, as far as I know. I've got a few too many findOne instances server side that I need to work consistently with mongo to keep using the zegenie:redis-oplog package in production though, as much as I'd like to. :-(