-
-
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 13 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
6ec28a8
c49f8aa
0d68f26
f5acfa0
ec9033d
97e143a
fdd9270
08865e3
069551f
caa5037
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 |
---|---|---|
|
@@ -23,11 +23,52 @@ function checkTriggers(className, config, types) { | |
function checkLiveQuery(className, config) { | ||
return config.liveQueryController && config.liveQueryController.hasLiveQuery(className); | ||
} | ||
async function runFindTriggers( | ||
config, | ||
auth, | ||
className, | ||
restWhere, | ||
restOptions, | ||
clientSDK, | ||
context, | ||
isGet | ||
) { | ||
EmpiDev marked this conversation as resolved.
Show resolved
Hide resolved
|
||
const result = await triggers.maybeRunQueryTrigger( | ||
triggers.Types.beforeFind, | ||
className, | ||
restWhere, | ||
restOptions, | ||
config, | ||
auth, | ||
context, | ||
isGet | ||
); | ||
|
||
// Returns a promise for an object with optional keys 'results' and 'count'. | ||
const find = async (config, auth, className, restWhere, restOptions, clientSDK, context) => { | ||
restWhere = result.restWhere || restWhere; | ||
restOptions = result.restOptions || restOptions; | ||
|
||
if (result?.objects) { | ||
const objects = result.objects; | ||
|
||
// Déclencher le trigger afterFind si des objets sont retournés | ||
await triggers.maybeRunAfterFindTrigger( | ||
triggers.Types.afterFind, | ||
auth, | ||
className, | ||
objects, | ||
config, | ||
restWhere, | ||
context | ||
); | ||
|
||
return { | ||
results: objects.map(row => row._toFullJSON()), | ||
}; | ||
} | ||
|
||
// Conserver la distinction entre get et find | ||
const query = await RestQuery({ | ||
method: RestQuery.Method.find, | ||
method: isGet ? RestQuery.Method.get : RestQuery.Method.find, | ||
config, | ||
auth, | ||
className, | ||
|
@@ -36,23 +77,38 @@ const find = async (config, auth, className, restWhere, restOptions, clientSDK, | |
clientSDK, | ||
context, | ||
}); | ||
|
||
return query.execute(); | ||
} | ||
|
||
// Returns a promise for an object with optional keys 'results' and 'count'. | ||
const find = async (config, auth, className, restWhere, restOptions, clientSDK, context) => { | ||
enforceRoleSecurity('find', className, auth); | ||
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. question: how the enforce role security was implemented before, it's not clear 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. @Moumouls Can this be resolved? 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. Okay i investigated the code deeper it seems that in the common cases "enforceRoleSecurity" will run twice (here and in RestQuery instance), enforceRoleSecurity ONLY prevent some queries on core classes, it's a small sync function with 2 "if" so i guess it's okay to run it twice. What do you think @mtrezza ? @EmpiDev you added this function here because some existing tests fails without it ? 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. It was added by @dblythy . I've verified that all tests pass without these calls in rest.js (lines 126 and 141), since the check is already performed in RestQuery() which is always called downstream. I think we have two reasonable options:
I'm fine with either approach. What's your preference @mtrezza? |
||
return runFindTriggers( | ||
config, | ||
auth, | ||
className, | ||
restWhere, | ||
restOptions, | ||
clientSDK, | ||
context, | ||
false | ||
); | ||
}; | ||
|
||
// get is just like find but only queries an objectId. | ||
const get = async (config, auth, className, objectId, restOptions, clientSDK, context) => { | ||
var restWhere = { objectId }; | ||
const query = await RestQuery({ | ||
method: RestQuery.Method.get, | ||
enforceRoleSecurity('get', className, auth); | ||
return runFindTriggers( | ||
config, | ||
auth, | ||
className, | ||
restWhere, | ||
{ objectId }, | ||
restOptions, | ||
clientSDK, | ||
context, | ||
}); | ||
return query.execute(); | ||
true | ||
); | ||
}; | ||
|
||
// Returns a promise that doesn't resolve to any useful value. | ||
|
Uh oh!
There was an error while loading. Please reload this page.