Skip to content
Open
Show file tree
Hide file tree
Changes from 40 commits
Commits
Show all changes
44 commits
Select commit Hold shift + click to select a range
6f65ec5
feat: allow short circuit of beforeFind
dblythy Jul 19, 2023
41bf1b7
improve test description
mtrezza Jul 23, 2023
21c9289
Update CloudCode.spec.js
dblythy Jul 23, 2023
ca74929
wip
dblythy Jul 24, 2023
207c3c0
wip
dblythy Jul 24, 2023
bc68816
wip
dblythy Jul 25, 2023
d66fa48
Merge branch 'alpha' into beforeFind-objects
mtrezza Sep 1, 2023
34f7741
feat: allow short circuit of beforeFind
dblythy Jul 19, 2023
5825abf
improve test description
mtrezza Jul 23, 2023
6daa039
Update CloudCode.spec.js
dblythy Jul 23, 2023
ec37789
wip
dblythy Jul 24, 2023
09fcde2
wip
dblythy Jul 25, 2023
8de0edb
Merge branch 'beforeFind-objects' of https://github.com/dblythy/parse…
EmpiDev May 27, 2025
dd75062
refactor: remove redundant beforeFind tests and clean up afterFind hooks
EmpiDev May 27, 2025
8d6aae6
refactor: improve condition checks and format function parameters for…
EmpiDev May 30, 2025
c5dd665
test: update beforeFind test to create and save a new object
EmpiDev May 30, 2025
55c3b3b
Merge branch 'alpha' into beforeFind-fix
EmpiDev May 30, 2025
d8525e9
fix : Unexpected token '}' 2057
EmpiDev Jun 2, 2025
04ae88f
Merge branch 'alpha' into beforeFind-fix
EmpiDev Jun 2, 2025
f083527
Merge branch 'alpha' into beforeFind-fix
EmpiDev Jun 3, 2025
f679ba0
Merge branch 'alpha' into beforeFind-fix
EmpiDev Jun 6, 2025
dfd4082
Merge branch 'alpha' into beforeFind-fix
EmpiDev Jun 6, 2025
43a87bb
Merge branch 'alpha' into beforeFind-fix
mtrezza Jun 7, 2025
c417118
refactor: Improve afterFind trigger handling and object processing
EmpiDev Jun 11, 2025
c6463c3
Merge branch 'alpha' into beforeFind-fix
EmpiDev Jun 12, 2025
0c2ad57
Merge branch 'alpha' into beforeFind-fix
mtrezza Jun 24, 2025
92ef147
refactor in CloudCode.spec.js
EmpiDev Jun 30, 2025
546e5b0
refactor in triggers.js
EmpiDev Jun 30, 2025
00b104e
test: Add direct function tests for maybeRunAfterFindTrigger
EmpiDev Jul 2, 2025
2a5dce9
Remove unnecessary comment
EmpiDev Jul 2, 2025
98be232
refactor: Replace isGet boolean flag with options object
EmpiDev Jul 15, 2025
0d42676
Provide isGet context to afterFind trigger
EmpiDev Jul 15, 2025
f197119
fix: Correct parameter formatting in afterFind trigger test
EmpiDev Jul 16, 2025
06e2bcc
Merge branch 'alpha' into beforeFind-fix
EmpiDev Jul 16, 2025
52d5594
Merge branch 'alpha' into beforeFind-fix
mtrezza Jul 21, 2025
c08b3f6
chore(tests): add spy to confirm DB is not accessed
EmpiDev Jul 22, 2025
37f74c1
Merge branch 'alpha' into beforeFind-fix
mtrezza Jul 29, 2025
468a6ec
refactor(tests): extract database spy setup into a helper function fo…
EmpiDev Jul 30, 2025
b157aa2
refactor(tests): move repetitive code into beforeEach block
EmpiDev Jul 31, 2025
83a918b
Merge branch 'alpha' into beforeFind-fix
mtrezza Aug 3, 2025
840a3e4
feat: Add beforeFind security tests for object visibility and protect…
EmpiDev Aug 26, 2025
3da7ffb
feat: Enhance beforeFind security tests for object visibility and acc…
EmpiDev Aug 26, 2025
041b00d
feat: Re-apply ACL/CLP systematically when beforeFind returns objects
EmpiDev Aug 26, 2025
bf70c6a
feat: Add explicit comments in runFindTriggers, triggers.js
EmpiDev Aug 26, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
242 changes: 242 additions & 0 deletions spec/CloudCode.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,248 @@ describe('Cloud Code', () => {
}
});

describe('beforeFind without DB operations', () => {
let findSpy;

beforeEach(() => {
const config = Config.get('test');
const databaseAdapter = config.database.adapter;
findSpy = spyOn(databaseAdapter, 'find').and.callThrough();
});

it('beforeFind can return object without DB operation', async () => {
Parse.Cloud.beforeFind('TestObject', () => {
return new Parse.Object('TestObject', { foo: 'bar' });
});
Parse.Cloud.afterFind('TestObject', req => {
expect(req.objects).toBeDefined();
expect(req.objects[0].get('foo')).toBe('bar');
});

const newObj = await new Parse.Query('TestObject').first();
expect(newObj.className).toBe('TestObject');
expect(newObj.toJSON()).toEqual({ foo: 'bar' });
expect(findSpy).not.toHaveBeenCalled();
await newObj.save();
Comment on lines +205 to +227
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: i think we need some additional use cases about security on objects returned in a before find, what is the behavior ?

If the code return object with ACL to user B for user A does parse return the full object, does parse filter again on ACL ?

to be honest i'm not 100% confident on the implementation code, because of some usage of instance of, feature detection, mutation of objects, and fragmented changes, if we can add some comments in the code it could helps for future maintenance/debug

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Answer to your questions:

  • If beforeFind returns a Query → the normal flow (RestQuery) continues with ACL/CLP applied.
  • If beforeFind returns objects (Parse.Object or an array) → the DB is short-circuited: afterFind is executed, but ACL/CLP/protectedFields are not re-applied.

If I understand your message correctly, the actions I should take are:

  1. Add explicit comments in runFindTriggers, RestQuery, and triggers to document the flow and potential risks.

  2. A proposed security fix for the “short-circuit” case:

    • Re-apply ACL/CLP systematically when beforeFind returns objects (except if useMasterKey is used).
  3. Add security test cases:

    • beforeFind returns objects not accessible to the current user → they must not be visible.
    • protectedFields are correctly hidden after re-filtering.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @EmpiDev , yes it could be a step forward, i think enforcing ACL protection even when DB is short circuited is important to prevent potential security risks and unwanted data leak.

});

it('beforeFind can return array of objects without DB operation', async () => {
Parse.Cloud.beforeFind('TestObject', () => {
return [new Parse.Object('TestObject', { foo: 'bar' })];
});
Parse.Cloud.afterFind('TestObject', req => {
expect(req.objects).toBeDefined();
expect(req.objects[0].get('foo')).toBe('bar');
});

const newObj = await new Parse.Query('TestObject').first();
expect(newObj.className).toBe('TestObject');
expect(newObj.toJSON()).toEqual({ foo: 'bar' });
expect(findSpy).not.toHaveBeenCalled();
await newObj.save();
});

it('beforeFind can return object for get query without DB operation', async () => {
Parse.Cloud.beforeFind('TestObject', () => {
return [new Parse.Object('TestObject', { foo: 'bar' })];
});
Parse.Cloud.afterFind('TestObject', req => {
expect(req.objects).toBeDefined();
expect(req.objects[0].get('foo')).toBe('bar');
});

const testObj = new Parse.Object('TestObject');
await testObj.save();
findSpy.calls.reset();

const newObj = await new Parse.Query('TestObject').get(testObj.id);
expect(newObj.className).toBe('TestObject');
expect(newObj.toJSON()).toEqual({ foo: 'bar' });
expect(findSpy).not.toHaveBeenCalled();
await newObj.save();
});

it('beforeFind can return empty array without DB operation', async () => {
Parse.Cloud.beforeFind('TestObject', () => {
return [];
});
Parse.Cloud.afterFind('TestObject', req => {
expect(req.objects.length).toBe(0);
});

const obj = new Parse.Object('TestObject');
await obj.save();
findSpy.calls.reset();

const newObj = await new Parse.Query('TestObject').first();
expect(newObj).toBeUndefined();
expect(findSpy).not.toHaveBeenCalled();
});
});
const { maybeRunAfterFindTrigger } = require('../lib/triggers');

describe('maybeRunAfterFindTrigger - direct function tests', () => {
const testConfig = {
applicationId: 'test',
logLevels: { triggerBeforeSuccess: 'info', triggerAfter: 'info' },
};

it('should convert Parse.Object instances to JSON when no trigger defined', async () => {
const className = 'TestParseObjectDirect_' + Date.now();

const parseObj1 = new Parse.Object(className);
parseObj1.set('name', 'test1');
parseObj1.id = 'obj1';

const parseObj2 = new Parse.Object(className);
parseObj2.set('name', 'test2');
parseObj2.id = 'obj2';

const result = await maybeRunAfterFindTrigger(
'afterFind',
null,
className,
[parseObj1, parseObj2],
testConfig,
null,
{}
);

expect(result).toBeDefined();
expect(Array.isArray(result)).toBe(true);
expect(result.length).toBe(2);
expect(result[0].name).toBe('test1');
expect(result[1].name).toBe('test2');
});

it('should handle null/undefined objectsInput when no trigger', async () => {
const className = 'TestNullDirect_' + Date.now();

const resultNull = await maybeRunAfterFindTrigger(
'afterFind',
null,
className,
null,
testConfig,
null,
{}
);
expect(resultNull).toEqual([]);

const resultUndefined = await maybeRunAfterFindTrigger(
'afterFind',
null,
className,
undefined,
testConfig,
null,
{}
);
expect(resultUndefined).toEqual([]);

const resultEmpty = await maybeRunAfterFindTrigger(
'afterFind',
null,
className,
[],
testConfig,
null,
{}
);
expect(resultEmpty).toEqual([]);
});

it('should handle plain object query with where clause', async () => {
const className = 'TestQueryWhereDirect_' + Date.now();
let receivedQuery = null;

Parse.Cloud.afterFind(className, req => {
receivedQuery = req.query;
return req.objects;
});

const mockObject = { id: 'test123', className: className, name: 'test' };

const result = await maybeRunAfterFindTrigger(
'afterFind',
null,
className,
[mockObject],
testConfig,
{ where: { name: 'test' }, limit: 10 },
{}
);

expect(receivedQuery).toBeInstanceOf(Parse.Query);
expect(result).toBeDefined();
});

it('should handle plain object query without where clause', async () => {
const className = 'TestQueryNoWhereDirect_' + Date.now();
let receivedQuery = null;

Parse.Cloud.afterFind(className, req => {
receivedQuery = req.query;
return req.objects;
});

const mockObject = { id: 'test456', className: className, name: 'test' };

const result = await maybeRunAfterFindTrigger(
'afterFind',
null,
className,
[mockObject],
testConfig,
{ limit: 5, skip: 0 },
{}
);

expect(receivedQuery).toBeInstanceOf(Parse.Query);
expect(result).toBeDefined();
});
Copy link

@coderabbitai coderabbitai bot Aug 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Verification agent

🧩 Analysis chain

Plain object query without where: likely bug in conversion; assert desired behavior and fix implementation.

Current implementation in maybeRunAfterFindTrigger builds withJSON({ where: query }) when where is absent. That nests top-level options like limit/skip inside where, which silently discards them. Strengthen the test to assert limit/skip are honored, and update the implementation to split top-level options from the where clause.

Test update:

 expect(receivedQuery).toBeInstanceOf(Parse.Query);
-expect(result).toBeDefined();
+const qJSON = receivedQuery.toJSON();
+expect(qJSON.limit).toBe(5);
+expect(qJSON.skip).toBe(0);
+expect(qJSON.where).toEqual({}); // no explicit filters provided
+expect(result).toBeDefined();

Implementation fix (src/triggers.js), replace the else-branch that wraps the entire object under where:

-  } else if (typeof query === 'object' && query !== null) {
-    const parseQueryInstance = new Parse.Query(classNameQuery);
-    if (query.where) {
-      parseQueryInstance.withJSON(query);
-    } else {
-      parseQueryInstance.withJSON({ where: query });
-    }
-    request.query = parseQueryInstance;
-  } else {
+  } else if (typeof query === 'object' && query !== null) {
+    const parseQueryInstance = new Parse.Query(classNameQuery);
+    const TOP_LEVEL = [
+      'limit',
+      'skip',
+      'order',
+      'keys',
+      'include',
+      'excludeKeys',
+      'readPreference',
+      'includeReadPreference',
+      'subqueryReadPreference',
+      'hint',
+      'explain',
+    ];
+    const json = {};
+    if (query.where && typeof query.where === 'object') {
+      json.where = query.where;
+      for (const k of TOP_LEVEL) if (k in query) json[k] = query[k];
+    } else {
+      json.where = query;
+    }
+    parseQueryInstance.withJSON(json);
+    request.query = parseQueryInstance;
+  } else {

This preserves standard query options at the top level while treating a plain object as the where clause.


🏁 Script executed:

#!/bin/bash
# Locate the implementation of maybeRunAfterFindTrigger in the codebase
rg -n 'function maybeRunAfterFindTrigger' -C3 --glob '*.js'

Length of output: 292


🏁 Script executed:

#!/bin/bash
# Display the implementation of maybeRunAfterFindTrigger around the object-handling branch
sed -n '430,550p' src/triggers.js

Length of output: 3489


🏁 Script executed:

#!/bin/bash
# Search for the definition and usage of withJSON in the codebase
rg -n 'withJSON' -C3 --glob '*.js'

Length of output: 6252


Refactor maybeRunAfterFindTrigger to honor top-level options for plain-object queries

The current logic nests all keys into the where clause when the incoming query is a plain object—silently dropping top-level options like limit and skip. We should:

  • Strengthen the existing test in spec/CloudCode.spec.js to assert that limit and skip are actually applied and that an empty where object is used when no filters are provided.
  • Change the branch in src/triggers.js so that it always extracts known top-level options (limit, skip, etc.) onto the JSON root, and only places actual filter criteria into where.

Locations to update:

  • spec/CloudCode.spec.js (around the “plain object query without where clause” test)
  • src/triggers.js (inside the else if (typeof query === 'object' ...) block of maybeRunAfterFindTrigger)

Suggested diff for spec/CloudCode.spec.js:

 it('should handle plain object query without where clause', async () => {
   // …setup…
   expect(receivedQuery).toBeInstanceOf(Parse.Query);
-  expect(result).toBeDefined();
+  // The JSON form should honor limit/skip at top level and no filters in where
+  const qJSON = receivedQuery.toJSON();
+  expect(qJSON.limit).toBe(5);
+  expect(qJSON.skip).toBe(0);
+  expect(qJSON.where).toEqual({});
+  expect(result).toBeDefined();
 });

Suggested refactor for src/triggers.js:

@@ export function maybeRunAfterFindTrigger(
-  } else if (typeof query === 'object' && query !== null) {
-    const parseQueryInstance = new Parse.Query(classNameQuery);
-    if (query.where) {
-      parseQueryInstance.withJSON(query);
-    } else {
-      parseQueryInstance.withJSON({ where: query });
-    }
-    request.query = parseQueryInstance;
-  } else {
+  } else if (typeof query === 'object' && query !== null) {
+    const parseQueryInstance = new Parse.Query(classNameQuery);
+    // Define top-level keys to extract
+    const TOP_LEVEL = [
+      'limit','skip','order','keys','include','excludeKeys',
+      'readPreference','includeReadPreference','subqueryReadPreference',
+      'hint','explain'
+    ];
+    // Build JSON: default empty where, then copy filters and top-level options
+    const json = { where: {} };
+    // If a filter object was passed explicitly, use it
+    if (typeof query.where === 'object') {
+      json.where = query.where;
+    }
+    // Extract any top-level options
+    for (const key of TOP_LEVEL) {
+      if (key in query) {
+        json[key] = query[key];
+      }
+    }
+    parseQueryInstance.withJSON(json);
+    request.query = parseQueryInstance;
+  } else {

This ensures that for a call like

maybeRunAfterFindTrigger(..., { limit: 5, skip: 0 }, ...)

you end up with a query whose JSON is

{ where: {}, limit: 5, skip: 0 }

and thus limit/skip are correctly honored.

🤖 Prompt for AI Agents
In spec/CloudCode.spec.js around lines 446-469 and src/triggers.js inside the
else-if branch handling plain-object queries in maybeRunAfterFindTrigger, the
current logic nests all keys into where and drops top-level options like
limit/skip; update the test to assert receivedQuery.toJSON() has an empty where
object and retains limit and skip, and modify the triggers implementation to
extract known top-level options (limit, skip, order, keys, include, count, etc.)
onto the root of the query JSON while only copying true filter keys into where
(ensuring when no filters exist where is an empty object), then return/create
the Parse.Query using that normalized JSON so afterFind receives a Parse.Query
whose toJSON() is { where: {}, limit: 5, skip: 0 } for the provided example.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. The current behavior of withJSON already supports top-level options

    • withJSON expects a full Parse Query JSON object ({ where, limit, skip, order, etc. }).
    • If the incoming object is already in that format, withJSON handles it automatically.
    • If the object is just a filter (no where), the current code wraps it in { where: query }, which is exactly what you want.
  2. The proposed “TOP_LEVEL” extraction is brittle

    • Hardcoding keys like limit, skip, order, etc. introduces maintenance overhead.
    • If Parse adds a new top-level option, the list must be updated, creating unnecessary technical debt.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧩 Analysis chain

@EmpiDev Thank you for the feedback. Let me verify the current behavior to understand what's actually happening in the code.

Based on this analysis, I can see that you're right about withJSON supporting top-level options when they're properly structured, but there's still an issue with the current implementation.

The problem is in the condition logic:

  • When query = { limit: 5, skip: 0 } (no where property), the code goes to the else branch
  • This calls withJSON({ where: { limit: 5, skip: 0 } })
  • This incorrectly nests limit and skip inside the where clause

However, you make a valid point about avoiding hardcoded lists. Here's a more elegant solution:

The fix should be simpler - we need to detect whether the incoming object is already a valid Parse Query JSON format (has where as a property) versus a plain filter object:

} else if (typeof query === 'object' && query !== null) {
  const parseQueryInstance = new Parse.Query(classNameQuery);
-  if (query.where) {
+  if ('where' in query) {
    parseQueryInstance.withJSON(query);
  } else {
-    parseQueryInstance.withJSON({ where: query });
+    // If it's a plain object with top-level options but no where, 
+    // it should be treated as a query JSON with empty where
+    parseQueryInstance.withJSON({ where: {}, ...query });
  }
  request.query = parseQueryInstance;

This approach:

  1. Uses 'where' in query to detect if it's already a Parse Query JSON format
  2. For plain objects, creates { where: {}, ...query } which spreads the top-level options correctly
  3. Avoids hardcoded lists of keys
  4. Maintains backward compatibility

Would you like me to verify this solution works correctly with the test cases?


🏁 Script executed:

#!/bin/bash
# Let's examine the exact implementation in maybeRunAfterFindTrigger to understand the flow
rg -n -A 15 -B 5 "if.*query\.where" src/triggers.js

Length of output: 857


🏁 Script executed:

#!/bin/bash
# Let's also look at a test case to see what the current behavior produces
cd spec && grep -n -A 10 -B 5 "should handle plain object query without where clause" CloudCode.spec.js

Length of output: 649


🏁 Script executed:

#!/bin/bash
# Let's trace what happens when we pass { limit: 5, skip: 0 } by creating a minimal test
cat > /tmp/test_query_behavior.js << 'EOF'
const Parse = require('parse/node');

// Mock Parse.Query to see what withJSON receives
const originalWithJSON = Parse.Query.prototype.withJSON;
Parse.Query.prototype.withJSON = function(json) {
  console.log('withJSON called with:', JSON.stringify(json, null, 2));
  return originalWithJSON.call(this, json);
};

// Simulate the current logic
function testCurrentLogic(query, classNameQuery) {
  const parseQueryInstance = new Parse.Query(classNameQuery);
  if (query.where) {
    console.log('Branch: query has where clause');
    parseQueryInstance.withJSON(query);
  } else {
    console.log('Branch: query has no where clause');
    parseQueryInstance.withJSON({ where: query });
  }
  return parseQueryInstance;
}

// Test case 1: { limit: 5, skip: 0 }
console.log('=== Test 1: { limit: 5, skip: 0 } ===');
const result1 = testCurrentLogic({ limit: 5, skip: 0 }, 'TestClass');

// Test case 2: { where: { name: 'test' }, limit: 10 }
console.log('\n=== Test 2: { where: { name: "test" }, limit: 10 } ===');
const result2 = testCurrentLogic({ where: { name: 'test' }, limit: 10 }, 'TestClass');

console.log('\nResult 1 toJSON:', JSON.stringify(result1.toJSON(), null, 2));
console.log('Result 2 toJSON:', JSON.stringify(result2.toJSON(), null, 2));
EOF

node /tmp/test_query_behavior.js

Length of output: 934


🏁 Script executed:

#!/bin/bash
# Let's examine what a proper query JSON should look like
cat > /tmp/query_structure_test.js << 'EOF'
const Parse = require('parse/node');

// Test what a proper Parse.Query JSON looks like
const query1 = new Parse.Query('TestClass');
query1.limit(5);
query1.skip(0);
console.log('Query with limit/skip:', JSON.stringify(query1.toJSON(), null, 2));

const query2 = new Parse.Query('TestClass');
query2.equalTo('name', 'test');
query2.limit(10);
console.log('Query with where + limit:', JSON.stringify(query2.toJSON(), null, 2));
EOF

node /tmp/query_structure_test.js

Length of output: 938


Based on the code examination, I can see the issue now. You're right that withJSON handles top-level options correctly when the object is properly structured, but there's still a logical issue in the current implementation.

Looking at the current logic in src/triggers.js:

if (query.where) {
  parseQueryInstance.withJSON(query);  // Handles full query JSON
} else {
  parseQueryInstance.withJSON({ where: query });  // Wraps entire object as where clause
}

When the test passes { limit: 5, skip: 0 }, since there's no query.where, it goes to the else branch and creates { where: { limit: 5, skip: 0 } }. This puts limit and skip inside the where clause instead of at the top level.

However, I notice the test isn't actually asserting the structure of the query - it's only checking that receivedQuery is an instance of Parse.Query. This suggests the current test might not be catching the bug.

You make excellent points about avoiding hardcoded lists, but there's still a logical issue to fix. A cleaner approach would be to detect whether the object represents filters vs. query options:

if (query.where !== undefined || hasOnlyFilterKeys(query)) {
  // It's already a proper query JSON or contains only filter criteria
  parseQueryInstance.withJSON(query.where ? query : { where: query });
} else {
  // It contains query options like limit/skip, treat as query JSON with empty where
  parseQueryInstance.withJSON({ where: {}, ...query });
}

But you're absolutely right that a hardcoded key list isn't ideal. Let me reconsider this...

Actually, your point about withJSON already handling this correctly makes me think: maybe the issue is that we should always assume an object without a where property that contains things like limit/skip should be treated as a complete query JSON with an implicit empty where, not as filter criteria.

The fix could be as simple as:

} else if (typeof query === 'object' && query !== null) {
  const parseQueryInstance = new Parse.Query(classNameQuery);
-  if (query.where) {
+  if (query.where !== undefined) {
    parseQueryInstance.withJSON(query);
  } else {
-    parseQueryInstance.withJSON({ where: query });
+    parseQueryInstance.withJSON({ where: {}, ...query });
  }
  request.query = parseQueryInstance;

This would handle your concerns while fixing the logical issue. What do you think about this approach?


🏁 Script executed:

#!/bin/bash
# Let's see the complete test case to understand what it's actually testing
grep -n -A 20 -B 5 "should handle plain object query without where clause" spec/CloudCode.spec.js

Length of output: 885


🏁 Script executed:

#!/bin/bash
# Let's also look at the test with where clause to compare
grep -n -A 15 -B 5 "should handle plain object query with where clause" spec/CloudCode.spec.js

Length of output: 750


🏁 Script executed:

#!/bin/bash
# Let's look for any existing tests that validate query JSON structure
rg -n "toJSON\(\)" spec/CloudCode.spec.js -B 3 -A 3

Length of output: 2185


🏁 Script executed:

#!/bin/bash
# Let's examine how withJSON is typically used in the codebase
rg -n "withJSON" src/ -B 2 -A 2 --type js

Length of output: 1160


@EmpiDev You're absolutely right about withJSON supporting top-level options correctly, and your concern about brittleness is valid. Looking at the complete code, I can see the issue more clearly now.

The current test doesn't actually assert the query structure - it only checks that receivedQuery is an instance of Parse.Query. This means we can't tell if the current behavior is working correctly.

Looking at other parts of the codebase, I found the pattern in src/rest.js line 60:

new Parse.Query(className).withJSON({ where: restWhere, ...restOptions })

This shows the expected pattern: top-level options spread at the root level, with where as a separate property.

The current logic in maybeRunAfterFindTrigger does:

if (query.where) {
  parseQueryInstance.withJSON(query);  // ✓ Correct for full query JSON
} else {
  parseQueryInstance.withJSON({ where: query });  // ✗ Nests everything in where
}

For { limit: 5, skip: 0 }, this creates { where: { limit: 5, skip: 0 } }, which puts the options in the wrong place.

You're right to avoid hardcoded lists. The fix should be:

} else if (typeof query === 'object' && query !== null) {
  const parseQueryInstance = new Parse.Query(classNameQuery);
  if (query.where) {
    parseQueryInstance.withJSON(query);
  } else {
-    parseQueryInstance.withJSON({ where: query });
+    parseQueryInstance.withJSON({ where: {}, ...query });
  }
  request.query = parseQueryInstance;

This follows the same pattern as src/rest.js and avoids any hardcoded key lists.

Should we also strengthen the test to actually verify the query structure, so we can be confident the fix works?

it('should handle plain object query without where clause', async () => {
  // ...existing setup...
  
  expect(receivedQuery).toBeInstanceOf(Parse.Query);
+ const queryJSON = receivedQuery.toJSON();
+ expect(queryJSON.where).toEqual({});
+ expect(queryJSON.limit).toBe(5);
+ expect(queryJSON.skip).toBe(0);
  expect(result).toBeDefined();
});

✏️ Learnings added
Learnt from: EmpiDev
PR: parse-community/parse-server#9770
File: spec/CloudCode.spec.js:446-469
Timestamp: 2025-08-26T14:06:31.809Z
Learning: In the Parse Server codebase, when handling query objects in maybeRunAfterFindTrigger, objects without a where property that contain options like limit/skip should be treated as query JSON with an empty where clause using the spread pattern { where: {}, ...query }, not nested as { where: query }.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.


🧠 Learnings used
Learnt from: RahulLanjewar93
PR: parse-community/parse-server#9744
File: spec/ParseLiveQuery.spec.js:0-0
Timestamp: 2025-04-30T19:31:35.344Z
Learning: In the Parse Server codebase, the functions in QueryTools.js are typically tested through end-to-end behavior tests rather than direct unit tests, even though the functions are exported from the module.


it('should create default query for invalid query parameter', async () => {
const className = 'TestInvalidQueryDirect_' + Date.now();
let receivedQuery = null;

Parse.Cloud.afterFind(className, req => {
receivedQuery = req.query;
return req.objects;
});

const mockObject = { id: 'test789', className: className, name: 'test' };

await maybeRunAfterFindTrigger(
'afterFind',
null,
className,
[mockObject],
testConfig,
'invalid_query_string',
{}
);

expect(receivedQuery).toBeInstanceOf(Parse.Query);
expect(receivedQuery.className).toBe(className);

receivedQuery = null;

await maybeRunAfterFindTrigger(
'afterFind',
null,
className,
[mockObject],
testConfig,
null,
{}
);

expect(receivedQuery).toBeInstanceOf(Parse.Query);
expect(receivedQuery.className).toBe(className);
});
});

it('beforeSave rejection with custom error code', function (done) {
Parse.Cloud.beforeSave('BeforeSaveFailWithErrorCode', function () {
throw new Parse.Error(999, 'Nope');
Expand Down
13 changes: 9 additions & 4 deletions src/RestQuery.js
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ async function RestQuery({
if (![RestQuery.Method.find, RestQuery.Method.get].includes(method)) {
throw new Parse.Error(Parse.Error.INVALID_QUERY, 'bad query type');
}
const isGet = method === RestQuery.Method.get;
enforceRoleSecurity(method, className, auth);
const result = runBeforeFind
? await triggers.maybeRunQueryTrigger(
Expand All @@ -60,7 +61,7 @@ async function RestQuery({
config,
auth,
context,
method === RestQuery.Method.get
isGet
)
: Promise.resolve({ restWhere, restOptions });

Expand All @@ -72,7 +73,8 @@ async function RestQuery({
result.restOptions || restOptions,
clientSDK,
runAfterFind,
context
context,
isGet
);
}

Expand Down Expand Up @@ -101,7 +103,8 @@ function _UnsafeRestQuery(
restOptions = {},
clientSDK,
runAfterFind = true,
context
context,
isGet
) {
this.config = config;
this.auth = auth;
Expand All @@ -113,6 +116,7 @@ function _UnsafeRestQuery(
this.response = null;
this.findOptions = {};
this.context = context || {};
this.isGet = isGet;
if (!this.auth.isMaster) {
if (this.className == '_Session') {
if (!this.auth.user) {
Expand Down Expand Up @@ -914,7 +918,8 @@ _UnsafeRestQuery.prototype.runAfterFindTrigger = function () {
this.response.results,
this.config,
parseQuery,
this.context
this.context,
this.isGet
)
.then(results => {
// Ensure we properly set the className back
Expand Down
Loading
Loading