-
-
Notifications
You must be signed in to change notification settings - Fork 4.8k
feat: Allow short circuit of beforeFind #9770
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: alpha
Are you sure you want to change the base?
Changes from 40 commits
6f65ec5
41bf1b7
21c9289
ca74929
207c3c0
bc68816
d66fa48
34f7741
5825abf
6daa039
ec37789
09fcde2
8de0edb
dd75062
8d6aae6
c5dd665
55c3b3b
d8525e9
04ae88f
f083527
f679ba0
dfd4082
43a87bb
c417118
c6463c3
0c2ad57
92ef147
546e5b0
00b104e
2a5dce9
98be232
0d42676
f197119
06e2bcc
52d5594
c08b3f6
37f74c1
468a6ec
b157aa2
83a918b
840a3e4
3da7ffb
041b00d
bf70c6a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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(); | ||
}); | ||
|
||
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(); | ||
}); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Verification agent 🧩 Analysis chainPlain object query without where: likely bug in conversion; assert desired behavior and fix implementation. Current implementation in 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 - } 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 🏁 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 The current logic nests all keys into the
Locations to update:
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 🤖 Prompt for AI Agents
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 The problem is in the condition logic:
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 } 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:
Would you like me to verify this solution works correctly with the test cases? 🏁 Script executed:
Length of output: 857 🏁 Script executed:
Length of output: 649 🏁 Script executed:
Length of output: 934 🏁 Script executed:
Length of output: 938 Based on the code examination, I can see the issue now. You're right that Looking at the current logic in 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 However, I notice the test isn't actually asserting the structure of the query - it's only checking that 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 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:
Length of output: 885 🏁 Script executed:
Length of output: 750 🏁 Script executed:
Length of output: 2185 🏁 Script executed:
Length of output: 1160 @EmpiDev You're absolutely right about The current test doesn't actually assert the query structure - it only checks that Looking at other parts of the codebase, I found the pattern in new Parse.Query(className).withJSON({ where: restWhere, ...restOptions }) This shows the expected pattern: top-level options spread at the root level, with The current logic in if (query.where) {
parseQueryInstance.withJSON(query); // ✓ Correct for full query JSON
} else {
parseQueryInstance.withJSON({ where: query }); // ✗ Nests everything in where
} For 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 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
🧠 Learnings used
|
||
|
||
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'); | ||
|
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Answer to your questions:
beforeFind
returns aQuery
→ the normal flow (RestQuery
) continues with ACL/CLP applied.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:
Add explicit comments in
runFindTriggers
,RestQuery
, andtriggers
to document the flow and potential risks.A proposed security fix for the “short-circuit” case:
beforeFind
returns objects (except ifuseMasterKey
is used).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.There was a problem hiding this comment.
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.