Support for async simple-schema (3.0.0)#480
Support for async simple-schema (3.0.0)#480thumptech wants to merge 3 commits intoMeteor-Community-Packages:masterfrom
Conversation
|
hey @thumptech I don't want this to be forgotten. I assume this is a breaking change so we should target a 5.0.0 release with it, right? |
There was a problem hiding this comment.
Pull request overview
Updates aldeed:collection2 and its test harness to work with aldeed:simple-schema@3.0.0, including async-capable cleaning/validation and updated Meteor/test dependencies.
Changes:
- Bump test app to Meteor
3.3.2and update test dependencies/lockfiles. - Update collection2 to accept SimpleSchema
3.0.0and shift validation/cleaning paths toward async. - Migrate several tests to use
insertAsync/findOneAsyncand asyncautoValue.
Reviewed changes
Copilot reviewed 11 out of 13 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
package/collection2/main.js |
Core validation/wrapping logic updated for async clean/validate and Meteor 3 async allow/deny. |
package/collection2/package.js |
Expand SimpleSchema version constraint to include 3.0.0. |
tests/.meteor/release |
Bump test app Meteor release to 3.3.2. |
tests/.meteor/versions |
Refresh Meteor package versions for the test app (incl. SimpleSchema 3 / collection2). |
tests/.meteor/packages |
Update test app package pins to SimpleSchema 3 and newer Meteor packages. |
tests/autoValue.tests.js |
Convert autoValue and test inserts to async to exercise async SimpleSchema behaviors. |
tests/books.tests.js |
Test tweaks for async usage; adds a global side effect and some debug-ish scaffolding. |
tests/multi.tests.js |
Style normalization and client-path conversion to async collection APIs. |
tests/helper.js |
Minor helper tweak; adds commented debug logging. |
tests/package.json |
Bump @babel/runtime in tests. |
tests/package-lock.json |
Lockfile update for new @babel/runtime version. |
.jshintrc |
Set esversion: 11 to accommodate newer JS syntax. |
.gitignore |
Ignore packages/ directory. |
Files not reviewed (1)
- tests/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| function _methodMutation(async, methodName) { | ||
| const _super = Meteor.isFibersDisabled ? | ||
| Mongo.Collection.prototype[methodName] | ||
| : Mongo.Collection.prototype[methodName.replace('Async', '')]; | ||
|
|
||
| try { | ||
| return await _super.apply(this, validatedArgs); | ||
| } catch (err) { | ||
| if (!_super) return; | ||
| Mongo.Collection.prototype[methodName] = function(...args) { | ||
| const [validatedArgs, validationContext] = getArgumentsAndValidationContext.call(this, methodName, args, async); | ||
|
|
||
| if (async && !Meteor.isFibersDisabled) { | ||
| try { | ||
| this[methodName.replace('Async', '')].isCalledFromAsync = true; | ||
| _super.isCalledFromAsync = true; | ||
| return Promise.resolve(_super.apply(this, validatedArgs)); | ||
| } catch (err) { |
There was a problem hiding this comment.
getArgumentsAndValidationContext is now async, but _methodMutation calls it without await and immediately destructures the return value. This will yield a Promise (not an array) and will throw at runtime when insert/update/insertAsync/updateAsync go through this wrapper. Refactor _methodMutation to await validation (or provide a synchronous variant for fiber-enabled environments) before calling _super.
There was a problem hiding this comment.
I think I'd already made this change in the local copy. I've just commited the local changes and it adds the requested await statement.
| async function getArgumentsAndValidationContext(methodName, args, async) { | ||
| let options = isInsertType(methodName) ? args[1] : args[2]; | ||
|
|
||
| // Support missing options arg | ||
| if (!options || typeof options === 'function') { | ||
| options = {}; | ||
| } | ||
|
|
||
| function getArgumentsAndValidationContext(methodName, args, async) { | ||
| let options = isInsertType(methodName) ? args[1] : args[2]; | ||
| let validationContext = {}; | ||
| let validatedArgs = args; | ||
| if (this._c2 && options.bypassCollection2 !== true) { | ||
| let userId = null; | ||
| try { | ||
| // https://github.com/aldeed/meteor-collection2/issues/175 | ||
| userId = Meteor.userId(); | ||
| } catch (err) { | ||
| } | ||
|
|
||
| // Support missing options arg | ||
| if (!options || typeof options === 'function') { | ||
| options = {}; | ||
| try { | ||
| [validatedArgs, validationContext] = await doValidate( | ||
| this, | ||
| methodName, | ||
| args, | ||
| Meteor.isServer || this._connection === null, // getAutoValues | ||
| userId, | ||
| Meteor.isServer, // isFromTrustedCode | ||
| async | ||
| ); | ||
| } catch (e) { | ||
| throw e; | ||
| } | ||
|
|
||
| let validationContext = {}; | ||
| let validatedArgs = args; | ||
| if (this._c2 && options.bypassCollection2 !== true) { | ||
| let userId = null; | ||
| try { | ||
| // https://github.com/aldeed/meteor-collection2/issues/175 | ||
| userId = Meteor.userId(); | ||
| } catch (err) {} | ||
|
|
||
| [validatedArgs, validationContext] = doValidate( | ||
| this, | ||
| methodName, | ||
| args, | ||
| Meteor.isServer || this._connection === null, // getAutoValues | ||
| userId, | ||
| Meteor.isServer, // isFromTrustedCode | ||
| async | ||
| ); | ||
|
|
||
| if (!validatedArgs) { | ||
| // doValidate already called the callback or threw the error, so we're done. | ||
| // But insert should always return an ID to match core behavior. | ||
| return isInsertType(methodName) ? this._makeNewID() : undefined; | ||
| } | ||
| } else { | ||
| // We still need to adjust args because insert does not take options | ||
| if (isInsertType(methodName) && typeof validatedArgs[1] !== 'function') validatedArgs.splice(1, 1); | ||
| if (!validatedArgs) { | ||
| // doValidate already called the callback or threw the error, so we're done. | ||
| // But insert should always return an ID to match core behavior. | ||
| return isInsertType(methodName) ? this._makeNewID() : undefined; | ||
| } | ||
| } else { | ||
| // We still need to adjust args because insert does not take options | ||
| if (isInsertType(methodName) && typeof validatedArgs[1] !== 'function') validatedArgs.splice(1, 1); | ||
| } | ||
|
|
||
| return [validatedArgs, validationContext]; | ||
| } | ||
|
|
||
| function _methodMutation(async, methodName) { | ||
| const _super = Meteor.isFibersDisabled | ||
| ? Mongo.Collection.prototype[methodName] | ||
| : Mongo.Collection.prototype[methodName.replace('Async', '')]; | ||
|
|
||
| if (!_super) return; | ||
| Mongo.Collection.prototype[methodName] = function (...args) { | ||
| const [validatedArgs, validationContext] = getArgumentsAndValidationContext.call(this, methodName, args, async); | ||
|
|
||
| if (async && !Meteor.isFibersDisabled) { | ||
| try { | ||
| this[methodName.replace('Async', '')].isCalledFromAsync = true; | ||
| _super.isCalledFromAsync = true; | ||
| return Promise.resolve(_super.apply(this, validatedArgs)); | ||
| } catch (err) { | ||
| if (this._c2) { | ||
| const addValidationErrorsPropName = | ||
| typeof validationContext.addValidationErrors === 'function' | ||
| ? 'addValidationErrors' | ||
| : 'addInvalidKeys'; | ||
| parsingServerError([err], validationContext, addValidationErrorsPropName); | ||
| const error = getErrorObject(validationContext, err.message, err.code); | ||
| return Promise.reject(error); | ||
| } else { | ||
| // do not change error if collection isn't being validated by collection2 | ||
| return Promise.reject(err); | ||
| } | ||
| } | ||
| } else { | ||
| return _super.apply(this, validatedArgs); | ||
| } | ||
| }; | ||
| } | ||
| return [validatedArgs, validationContext]; | ||
| } |
There was a problem hiding this comment.
When doValidate signals an invalid operation by returning [] (callback-based inserts/updates), destructuring sets validatedArgs to undefined, triggering the if (!validatedArgs) path. This function then returns a scalar (newId/undefined) instead of a [args, context] tuple, which will break callers that always destructure the result. Consider returning a consistent shape (e.g., { validatedArgs, validationContext, earlyReturn } or [null, validationContext]) and handling the early-return behavior in the wrappers.
There was a problem hiding this comment.
I really have NFI what this is on about
There was a problem hiding this comment.
The comment here looks like this is the original intended behaviour:
// doValidate already called the callback or threw the error, so we're done. // But insert should always return an ID to match core behavior.
| @@ -725,12 +751,16 @@ function getArgumentsAndValidationContext(methodName, args, async) { | |||
| isLocalCollection | |||
| } | |||
| }); | |||
| } catch (e) { | |||
| throw e; | |||
| } | |||
|
|
|||
| return false; | |||
| }, | |||
| update: function (userId, doc, fields, modifier) { | |||
| // Referenced modifier is cleaned in place | |||
| c.simpleSchema(modifier).clean(modifier, { | |||
| return false; | |||
| }, | |||
| update: async function(userId, doc, fields, modifier) { | |||
| // Referenced modifier is cleaned in place | |||
| try { | |||
| await c.simpleSchema(modifier).clean(modifier, { | |||
| mutate: true, | |||
There was a problem hiding this comment.
defineDeny now registers async allow/deny callbacks (e.g., insert: async function(...) { await ... }). Meteor 3.x supports async allow/deny, but Meteor 2.x does not; in Meteor 2, an async function returns a Promise object which can be treated as a truthy return value and potentially deny/allow writes incorrectly. If this package still targets Meteor 2, gate async deny rules by Meteor version / Meteor.isFibersDisabled and use a synchronous implementation there (e.g., Fiber blocking via Promise.await), or update package constraints/docs to explicitly drop Meteor 2 support.
There was a problem hiding this comment.
We'll just lock this to Meteor 3 in package.js
| async function wrapCallbackForParsingServerErrors(validationContext, cb) { | ||
| const addValidationErrorsPropName = | ||
| typeof validationContext.addValidationErrors === 'function' ? | ||
| 'addValidationErrors' | ||
| : 'addInvalidKeys'; | ||
| return async function wrappedCallbackForParsingServerErrors(...args) { | ||
| parsingServerError(args, validationContext, addValidationErrorsPropName); | ||
| return await cb.apply(this, args); |
There was a problem hiding this comment.
wrapCallbackForParsingServerErrors is now async and returns an async wrapped callback. Meteor collection callbacks are not awaited; making the callback async can introduce unhandled promise rejections if the callback throws and can subtly change timing. This wrapper can stay synchronous: parse/replace args[0] and then call cb normally without async/await.
| async function wrapCallbackForParsingServerErrors(validationContext, cb) { | |
| const addValidationErrorsPropName = | |
| typeof validationContext.addValidationErrors === 'function' ? | |
| 'addValidationErrors' | |
| : 'addInvalidKeys'; | |
| return async function wrappedCallbackForParsingServerErrors(...args) { | |
| parsingServerError(args, validationContext, addValidationErrorsPropName); | |
| return await cb.apply(this, args); | |
| function wrapCallbackForParsingServerErrors(validationContext, cb) { | |
| const addValidationErrorsPropName = | |
| typeof validationContext.addValidationErrors === 'function' ? | |
| 'addValidationErrors' | |
| : 'addInvalidKeys'; | |
| return function wrappedCallbackForParsingServerErrors(...args) { | |
| parsingServerError(args, validationContext, addValidationErrorsPropName); | |
| return cb.apply(this, args); |
| globalThis.books = books; | ||
|
|
There was a problem hiding this comment.
globalThis.books = books; introduces a global side effect that can leak across tests and makes the test environment harder to reason about. If this was only for debugging, please remove it; otherwise, prefer exporting from a helper module or scoping it to the tests that need it.
| globalThis.books = books; |
| return new Promise((resolve, reject) => { | ||
| if (ASYNC_FRIENDLY) { | ||
| try { | ||
| //console.log(`calling method ${methodName} with args ${JSON.stringify(args)}`); |
There was a problem hiding this comment.
The added commented-out console.log looks like leftover debug code. Please remove it or guard logging behind an explicit env flag to keep the helper noise-free.
| //console.log(`calling method ${methodName} with args ${JSON.stringify(args)}`); |
| try{ | ||
| await callMongoMethod(books, 'update', [ | ||
| id, | ||
| { $set: { copies: 2 } }, | ||
| { bypassCollection2: true } | ||
| ]); | ||
| }catch(e){ | ||
| throw e; | ||
| } |
There was a problem hiding this comment.
This try/catch just rethrows the same error, which is a no-op and adds noise to the test. You can remove the try/catch entirely (or assert on the error if the test is meant to verify a failure mode).
| try{ | |
| await callMongoMethod(books, 'update', [ | |
| id, | |
| { $set: { copies: 2 } }, | |
| { bypassCollection2: true } | |
| ]); | |
| }catch(e){ | |
| throw e; | |
| } | |
| await callMongoMethod(books, 'update', [ | |
| id, | |
| { $set: { copies: 2 } }, | |
| { bypassCollection2: true } | |
| ]); |
Hey @jankapunkt , Yeah this will require a version bump, as will simple-schema. There are a lot of changes highlighted by the bot, I'll try to get through them as I can. The package is working in my app however, so I don't believe any of these are too severe. No need to worry about the simple-schema/collection2/autoform stack being forgotten, I need them to get my intranet app onto meteor 3 and have no choice but to work on them if nobody else is. Just keep pinging me with anything that needs done across the 3 pull requests and I'll try to commit the changes as I can to them. |
I've been working on the simple-schema package to for proper async support. This is the update of collection2 to work with it. All tests passing but I haven't tested it in my app yet, just wanted to ping that I am working on it.
This will probably break backwards compatibility.