Meteor-Community-Packages / meteor-collection2

A Meteor package that extends Mongo.Collection to provide support for specifying a schema and then validating against that schema when inserting and updating.
https://packosphere.com/aldeed/collection2
MIT License
1.02k stars 108 forks source link

Meteor 3.0 potential bug #445

Open harryadel opened 7 months ago

harryadel commented 7 months ago

As reported in Slack by @StorytellerCZ :


W20240122-18:59:31.199(1)? (STDERR) meteor://💻app/packages/mongo.js:3584
W20240122-18:59:31.200(1)? (STDERR)             throw new Error("".concat(m, " +  is not available on the server. Please use ").concat(getAsyncMethodName(m), "() instead."));
W20240122-18:59:31.200(1)? (STDERR)                   ^
W20240122-18:59:31.200(1)? (STDERR) 
W20240122-18:59:31.200(1)? (STDERR) Error: update +  is not available on the server. Please use updateAsync() instead.
W20240122-18:59:31.200(1)? (STDERR)     at Object.ret.<computed> [as update] (packages/mongo/remote_collection_driver.js:52:15)
W20240122-18:59:31.200(1)? (STDERR)     at Collection.update (packages/mongo/collection.js:959:31)
W20240122-18:59:31.200(1)? (STDERR)     at Collection.Mongo.Collection.<computed> [as update] (packages/aldeed:collection2/main.js:236:23)
W20240122-18:59:31.200(1)? (STDERR)     at packages/accounts-oauth/oauth_server.js:94:41
W20240122-18:59:31.200(1)? (STDERR)     at AsynchronousCursor.forEach (packages/mongo/mongo_driver.js:1115:22)
W20240122-18:59:31.200(1)? (STDERR)     at processTicksAndRejections (node:internal/process/task_queues:95:5)
W20240122-18:59:31.200(1)? (STDERR) 
W20240122-18:59:31.200(1)? (STDERR) Node.js v20.9.0

If I add

if (Meteor.isServer && Meteor.isFibersDisabled) {
          return null
        }

to line 236 in main.js it goes away. Probably not the best approach though.

github-actions[bot] commented 7 months ago

Thank you for submitting this issue!

We, the Members of Meteor Community Packages take every issue seriously. Our goal is to provide long-term lifecycles for packages and keep up with the newest changes in Meteor and the overall NodeJs/JavaScript ecosystem.

However, we contribute to these packages mostly in our free time. Therefore, we can't guarantee you issues to be solved within certain time.

If you think this issue is trivial to solve, don't hesitate to submit a pull request, too! We will accompany you in the process with reviews and hints on how to get development set up.

Please also consider sponsoring the maintainers of the package. If you don't know who is currently maintaining this package, just leave a comment and we'll let you know

harryadel commented 7 months ago

Here's the relevant code associated with your problem:

// If async is supported and fibers are disabled
// wait for db operation promise to resolve 
// else just run the sync version as is
if (async && !Meteor.isFibersDisabled) {
        try {
          this[methodName.replace('Async', '')].isCalledFromAsync = true;
          _super.isCalledFromAsync = true;
          return Promise.resolve(_super.apply(this, args)); <---  wait for db operation here
        } catch (err) {
          const addValidationErrorsPropName =
            typeof validationContext.addValidationErrors === 'function'
              ? 'addValidationErrors'
              : 'addInvalidKeys';
          parsingServerError([err], validationContext, addValidationErrorsPropName);
          error = getErrorObject(validationContext, err.message, err.code);
          return Promise.reject(error);
        }
      } else {
        return _super.apply(this, args); <--- run sync version as is
      }

With that being said I don't see an problem inherently with an error being thrown out. You're not meant to be using sync DB calls on Meteor 3.0, duh! Moreover, it's in your best interest for an error to be thrown out so you can tell what's DB call failed instead of failing silently.

The best we can strive for is to beautify the error but IMO the error is informant as is, it doesn't get better than this.

Error: update +  is not available on the server. Please use updateAsync() instead.

Let me know what you think, otherwise I'm closing this issue.

StorytellerCZ commented 7 months ago

The problem though is that we weren't using any sync call on the server or client. Unless there was something in a package that we couldn't locate.

harryadel commented 7 months ago

hmmm, can you provide a replication of this bug? I was going simply by the error message you provided.

StorytellerCZ commented 7 months ago

I'm now thinking this could be related to this: https://github.com/meteor/meteor/pull/12978

While the current solution is technically correct, I don't think that it is a best behavior for collection2. I'll look more into this as I work on reproduction.

harryadel commented 7 months ago

Ok, keep me posted.

jankapunkt commented 1 month ago

@harryadel is this something that needs to be fixed for 3.0 or is this an edge case?