Exception when using 'audit-argument-checks'

leite08 commented 6 years ago

Upon testing the fix of #18 I've found this: when using Meteor's audit-argument-checks I got the exception below:

Exception from sub document.list id kiKy8ppmD7MG6D2ag Error: Did not check() all arguments during publisher 'document.list'
     at ArgumentChecker.throwUnlessAllArgumentsHaveBeenChecked (packages/check.js:483:13)
     at Object._failIfArgumentsAreNotAllChecked (packages/check.js:131:16)
     at maybeAuditArgumentChecks (packages/ddp-server/livedata_server.js:1765:18)
     at DDP._CurrentPublicationInvocation.withValue (packages/ddp-server/livedata_server.js:1043:15)
     at Meteor.EnvironmentVariable.EVp.withValue (packages/meteor.js:1134:15)
     at Subscription._runHandler (packages/ddp-server/livedata_server.js:1041:51)
     at Session._startSubscription (packages/ddp-server/livedata_server.js:859:9)
     at Session.sub (packages/ddp-server/livedata_server.js:625:12)
     at packages/ddp-server/livedata_server.js:559:43

It only happens after the first update of the joined collection (Companies). After this first error occurs, the exception keeps poping up even after a restart. Only stops after a meteor reset (I know, new database, but the code is configured to clean & repopulate the collections upon start).

To simulate this you can use this repo (audit-argument-checks is already installed):

Clone the new branch: git clone -b perak-joins-issue-missing-check git@github.com:leite08/Create-React-Meteor-App.git

Run: cd Create-React-Meteor-App/ && meteor npm install --save react react-dom && meteor

On the browser: http://localhost:3000/

It should run and show the home page.

Then you can run this database update, when the exception should be thrown: db.companies.update({},{$set:{name:'aaa'}});

perak commented 6 years ago

OK, I will fix it ASAP...

perak commented 6 years ago

@leite08 investigating...

Quick fix is:

import { check, Match } from 'meteor/check';

Meteor.publish('document.list', function(someId) {
  check(someId, String);
  check(arguments, [Match.Any]);

  ...
});

For some reason, when perak:joins is used, there is one extra argument to publication...

Try console.log(arguments); at the beginning of publish function, and I got:

{ '0': '123', '1': 'NSTDppJad5tzFtkZS' }

Instead:

{ '0': '123' }

🤔

perak commented 6 years ago

@leite08 Ah... OK, got it. That's normal behavior (now documented in README.md).

If { reactive: true } is used then publication is called with one extra argument internaly used by the package - you need to check() that extra argument too:

import { check, Match } from 'meteor/check';

Meteor.publish('document.list', function(someId, extraArgument) {
  check(someId, String);
  check(extraArgument, Match.Any);

  ...
});

Thanks again for perfect bug report! Beer for you: 🍺 (if you don't drink beer, please bring it back and I'll send you ice cream :D )

leite08 commented 6 years ago

Cool @perak! Thanks for the fix! Will implement it ASAP!

Thanks for the beer, I surely liked it! :smiley:

Congrats for the package! Have a beer on me: :beer:

perak / meteor-joins

Exception when using 'audit-argument-checks' #19