Skip to content

Analysis.en.md

Latest commit

 

History

History
261 lines (198 loc) · 9.28 KB

Analysis.en.md

File metadata and controls

261 lines (198 loc) · 9.28 KB
Raw

Bug Analysis: defineMutationMethods: false + aldeed:collection2 → Meteor 3.4.1 Crash

What's Happening (Verified by Source Code)

Call Stack

new Mongo.Collection('links', { defineMutationMethods: false })
  └→ setupMutationMethods(this, name, options)        ← mongo/collection/collection_utils.js:47
       └→ if (options.defineMutationMethods === false) return;   ← EARLY EXIT
            ↑ _defineMutationMethods() is never called

LinksCollection.attachSchema(schema)
  └→ collection.deny({ ... })                         ← aldeed:collection2/main.js:765
       └→ addValidator(this, 'deny', options)          ← allow-deny/allow-deny.js:546
            └→ collection._validators[validatorSyncName][allowOrDeny].push(...)  ← line 590
                                   ^^^^^^^^^^^^^^^^^^^
                                   undefined → TypeError

Why It Breaks

In allow-deny/allow-deny.js, the _defineMutationMethods function performs two independent tasks:

  1. Initializes data structures (lines 81–99):

    self._restricted = false;
self._insecure = undefined;
self._validators = {
  insert: {allow: [], deny: []},
  update: {allow: [], deny: []},
  remove: {allow: [], deny: []},
  insertAsync: {allow: [], deny: []},
  updateAsync: {allow: [], deny: []},
  removeAsync: {allow: [], deny: []},
  upsertAsync: {allow: [], deny: []},
  fetch: [],
  fetchAllFields: false
};

  2. Registers DDP methods (lines 101–244): /links/insertAsync, /links/updateAsync, etc.

defineMutationMethods: false should only disable task 2, but in 3.4.1, the early return in setupMutationMethods skips both tasks.

Regression compared to 3.4.1-beta

In the beta version (mongo@2.2.1-beta341.1), _defineMutationMethods was likely called unconditionally in the collection constructor, and the defineMutationMethods flag only affected the registration of DDP methods inside that function. In the stable 3.4.1, the logic was moved to setupMutationMethods with a fatal early return.

Fix Option Evaluation

Option A: Separate Initialization from DDP Registration

Move the _validators initialization to a separate method (e.g., _initValidators()), call it unconditionally from the constructor, and have setupMutationMethods skip only the method registration.

Pros:

  • Architecturally clean solution, separates concerns.
  • No code duplication.

Cons:

  • Requires changes in two packages (mongo + allow-deny).
  • Larger number of changes → higher risk of side effects.
  • Requires a new _initValidators() API — changes the public/semi-public surface.

Option B: Initialize _validators in the Constructor (✅ RECOMMENDED)

Move the initialization of _restricted, _insecure, and _validators directly into the Mongo.Collection constructor before calling setupMutationMethods. After this, _defineMutationMethods (and consequently setupMutationMethods) can focus solely on DDP method registration.

Why this is the best option:

  1. Minimal Intervention: Only one file changes — mongo/collection/collection_utils.js (and optionally collection.js).
  2. No Duplication: Initialization happens in one place.
  3. Conceptually Correct: _validators / _restricted structures are properties of the collection itself, not a side effect of DDP registration. They are needed even without DDP.
  4. Backward Compatibility: allow() / deny() continue to work even if defineMutationMethods: false.
  5. Smallest Diff, lowest risk.

Cons:

  • Initialization now occurs in mongo instead of allow-deny — a minor layer violation (though already compromised by the early return).

Option C: Fix setupMutationMethods — Initialize in the return Branch

export function setupMutationMethods(collection, name, options) {
  if (options.defineMutationMethods === false) {
    collection._restricted = false;
    collection._insecure = undefined;
    collection._validators = { /* ... */ };
    return;
  }
  collection._defineMutationMethods({ ... });
}

Pros:

  • Change in only one file.

Cons:

  • Explicit code duplication — _validators is initialized in two places.
  • Fragility: If the _validators structure changes in allow-deny, collection_utils.js must be updated synchronously.
  • Worst solution from a maintainability perspective.

Recommended Patch (Option B)

Change 1: mongo/collection/collection_utils.js

 export function setupMutationMethods(collection, name, options) {
-  if (options.defineMutationMethods === false) return;
-
+  if (options.defineMutationMethods === false) {
+    // Initialize allow/deny validator data structures even when DDP mutation
+    // methods are not registered. Other packages (e.g. aldeed:collection2)
+    // may call collection.deny() which depends on _validators being present.
+    collection._restricted = false;
+    collection._insecure = undefined;
+    collection._validators = {
+      insert:      { allow: [], deny: [] },
+      update:      { allow: [], deny: [] },
+      remove:      { allow: [], deny: [] },
+      insertAsync: { allow: [], deny: [] },
+      updateAsync: { allow: [], deny: [] },
+      removeAsync: { allow: [], deny: [] },
+      upsertAsync: { allow: [], deny: [] },
+      fetch: [],
+      fetchAllFields: false,
+    };
+    return;
+  }
+
   try {
     collection._defineMutationMethods({
       useExisting: options._suppressSameNameError === true
     });

Note: Technically, this is "Option C" implementation-wise, but semantically it's the minimally invasive version of Option B where initialization is moved to a single additional location instead of refactoring the entire architecture.

Where to Apply the Change

The file is located in the local Meteor package cache:

~/.meteor/packages/mongo/.2.3.0.1wew2r2a301++os+web.browser+web.browser.legacy+web.cordova/
  os/collection/collection_utils.js                 ← line 47
  web.browser/collection/collection_utils.js        ← same file, different target
  web.browser.legacy/collection/collection_utils.js ← same file, different target
  web.cordova/collection/collection_utils.js        ← same file, different target

⚠️ These are cached binary packages — changes will be overwritten upon Meteor or package updates. The correct path is an upstream PR to meteor/meteor.

For Upstream PR

Files in the meteor/meteor Repository

File What to change
packages/mongo/collection/collection_utils.js setupMutationMethods — add initialization to the return branch

Proper PR Title

fix(mongo): initialize _validators when defineMutationMethods is false

Regression Test

// tests/main.js or separate test
import { Mongo } from 'meteor/mongo';
import SimpleSchema from 'meteor/aldeed:simple-schema';
import 'meteor/aldeed:collection2/static';

it('should not crash when defineMutationMethods: false is used with attachSchema', function() {
  const TestCollection = new Mongo.Collection('test_no_mutation_methods', {
    defineMutationMethods: false
  });
  const schema = new SimpleSchema({ name: { type: String, optional: true } });
  
  // This should NOT throw TypeError
  expect(() => TestCollection.attachSchema(schema)).not.toThrow();
  
  // Validators should be initialized
  expect(TestCollection._validators).toBeDefined();
  expect(TestCollection._validators.insert).toBeDefined();
});

Local Workaround (If You Can't Wait for a PR)

Instead of defineMutationMethods: false, use one of these two workarounds:

Workaround 1: Do Not Use defineMutationMethods: false

// Create collection without the flag
export const LinksCollection = new Mongo.Collection('links');

// Explicitly deny all client-side mutations on the server
if (Meteor.isServer) {
  LinksCollection.deny({
    insert() { return true; },
    update() { return true; },
    remove() { return true; },
  });
}

Drawback: DDP methods are still registered, they just always return deny. Extra code, but functional.

Workaround 2: Manually Fix _validators After Collection Creation

export const LinksCollection = new Mongo.Collection('links', {
  defineMutationMethods: false
});

// Manually initialize structures that _defineMutationMethods should have created
LinksCollection._restricted = false;
LinksCollection._insecure = undefined;
LinksCollection._validators = {
  insert:      { allow: [], deny: [] },
  update:      { allow: [], deny: [] },
  remove:      { allow: [], deny: [] },
  insertAsync: { allow: [], deny: [] },
  updateAsync: { allow: [], deny: [] },
  removeAsync: { allow: [], deny: [] },
  upsertAsync: { allow: [], deny: [] },
  fetch: [],
  fetchAllFields: false,
};

const schema = new SimpleSchema({});
LinksCollection.attachSchema(schema);

Drawback: "Hackish" and depends on internal package structures. But it works.

Summary

Criterion Option A Option B Option C
Diff Minimality ❌ High ✅ One file ✅ One file
Code Duplication ✅ None ⚠️ Minimal ❌ Yes
Architectural Purity ✅ ✅ ❌
Side Effect Risk ⚠️ Medium ✅ Low ✅ Low
Recommendation ✅ CHOOSE