feat: Allow short circuit of beforeFind

[dblythy]

Pull Request

• Report security issues confidentially.
• Any contribution is under this license.
• Link this pull request to an issue.

Issue

Closes: #8693

Approach

Adds ability to return objects (or an empty array) from a beforeFind trigger.

Tasks

• Add tests

[parse-github-assistant]

I will reformat the title to use the proper commit message syntax.

[parse-github-assistant]

Thanks for opening this pull request!

[codecov]

Codecov Report

Patch coverage: 83.33% and project coverage change: +0.02% 🎉

Comparison is base (2b3d4e5) 94.33% compared to head (6b2de6c) 94.36%.
Report is 1 commits behind head on alpha.

❗ Current head 6b2de6c differs from pull request most recent head d66fa48. Consider uploading reports for the commit d66fa48 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##            alpha    #8694      +/-   ##
==========================================
+ Coverage   94.33%   94.36%   +0.02%     
==========================================
  Files         185      185              
  Lines       14766    14773       +7     
==========================================
+ Hits        13930    13941      +11     
+ Misses        836      832       -4     

Files Changed	Coverage Δ
src/rest.js	96.80% <66.66%> (-2.06%)	⬇️
src/triggers.js	95.42% <100.00%> (+0.06%)	⬆️

... and 3 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[mtrezza]

Great idea, I just wonder whether client SDKs is equipped for that.

I assume the results of a query until this PR are always saved objects with objectId, createdAt and updatedAt being set. Could there be unexpected behavior if a query returns "made up" objects that don't exist in the DB and are without those properties?

We could still merge the PR, as the feature would be usable via the REST API, but the docs would need a caveat note.

Also can Parse Server internally handle that in all situations? For example, in combination with explain option, etc.

[mtrezza]

This looks good; just one thing you surely have thought about already:

Should afterFind really not run because beforeFind substitutes the DB result? In theory one doesn't have to exclude the other, i.e. returning a result in beforeFind could skip the DB operation but still run the afterFind logic on the returned object. I'm thinking in cases were the afterFind block manipulates objects in a certain way, and one would want all objects to run through that. One could still differentiate between beforeFind-generated results and DB results by passing a flag in the context container.

[dblythy]

It could cause issues for a strongly typed language such as Swift, if an object is returned that has not been saved (objectId might be required). Not 100% sure but i'm pretty sure triggers are skipped with explain

[mtrezza]

Is that really a "strongly typed" question? Or do you mean that (any) Parse client SDK may expect a returned object to have a objectId - regardless of whether it's a strongly typed language or not.

[dblythy]

The JS SDK can handle "unsaved" objects, i'm not sure whether other SDKs can just yet.

Regarding the afterFind issue - imo I think it makes sense to skip the afterFind trigger - if there is prevailing logic, it can be called with ES imports

[mtrezza]

Then what do you mean by

It could cause issues for a strongly typed language (...), if an object is returned that has not been saved

Doesn't the same argument apply when returning an object from beforeSave? The object can be the same, whether it's returned from beforeFind or afterFind, in both cases the client needs to be able to handle it?

[dblythy]

Doesn't the same argument apply when returning an object from beforeSave? The object can be the same, whether it's returned from beforeFind or afterFind, in both cases the client needs to be able to handle it?

Yes you are correct. So I think this should be safe to merge for other SDKs.

[mtrezza]

I think so, and I think the docs should explain that, because the developer is interfering in something that Parse Server would do internally, i.e. compose the query results. That's even more difficult than modifying the results in afterFind, because there the developer has as least a "template" of results and can infer what results should look like and that whatever they modify from the original results may cause issues.

I think there are 2 open points:

• How does a developer know the requirements for composing query results? E.g. does an object in a query result need an objectId, createdAt, updatedAt, etc. SDKs may be able to handle objects without these fields, but maybe not as part of query results, because as part of a query result they usually come from the DB where these fields are always set for stored objects.
• Should the afterFind trigger be invoked with the custom results returned in beforeFind? It seems to me that there is no reason that it shouldn't. In fact, it may be easier to maintain an existing data pipeline if any existing logic in afterFind applies to these custom results as well. If a developer wants to skip the afterFind logic they can use the context container to set a flag which can be read in the afterFind trigger.

[dblythy]

How does a developer know the requirements for composing query results

It's only possible to return Parse Objects, and objectId, createdAt, updatedAt etc are set by default, so it should be fine.

If an object with a different class name is returned, which afterFind trigger should run? The original class name, or the latter?

Parse.Cloud.beforeFind('beforeFind', () => {
  return new Parse.Object('TestObject', { foo: 'bar' });
});
Parse.Cloud.afterFind('beforeFind', (req) => {
 // should this still fire? req.objects[0].className will equal testObject
});
Parse.Cloud.afterFind('TestObject', (req) => {
 // should this?
});

[mtrezza]

It's only possible to return Parse Objects, and objectId, createdAt, updatedAt etc are set by default, so it should be fine.

They are set even if a Parse Object is returned when these fields a not set? For example if beforeFind returns:

const MyClass = Parse.Object.extend("MyClass");
const o = new MyClass();
o.a = 1;
return [o];

If an object with a different class name is returned, which afterFind trigger should run? The original class name, or the latter?

So a query on class _Users could return Parse Objects of class _Installation? Looks like trouble ahead... I'm not sure we should allow this. I think we need to at least document some requirements, such as:

• The return value must be of type Array<ParseObject> when using ParseQuery.find; the array can be empty in case of no results; the return value cannot be undefined.
• The return value must be of type ParseObject when using ParseQuery.get; the return value can be undefined in case of no result [is that so]?
• The returned ParseObject's class must be the same class as the invoking ParseQuery's class.

Maybe we should even check in code whether the return value complies with these requirements and throw an error otherwise. This would help developers to ensure they are composing compliant return values.

[dblythy]

It’s probably worth mentioning that the same concerns regarding fields, matching className, etc, apply to the current implementation of the afterFind trigger - there aren’t any restrictions what can be returned there (in fact, json that does not conform to a parse object can be returned)

[mtrezza]

Yes, I think we discovered a lack of documentation / restriction. We don't know whether Parse Server can handle this change of return values in all scenarios, and for the SDKs we currently don't know how they behave, nor do we have the test set-up to test this out in the CI. Your question already indicates that we are going down a slippery slope.

If we allow (per docs or code) any return type or even return a different class than was queried, then we cannot guarantee that Parse Server or the Parse client SDKs are able to handle this (not just for strongly typed languages).

I think the ParseQuery mechanisms need a framework with restrictions so that the server stays compatible with the SDKs, see list above, and the whole query pipeline (triggers, etc) works. If someone wants to return whatever they like they can use Cloud Functions.

[matheusfrozzi]

This makes me think about something I'm thinking a lot, It's not better to have a version of parse server only as API? Ignoring that clients' SDKs exist?

For example, I don't use the client SDKs, I have my own handles when I want to have a different return I send a type param (ie: 'search', profileList') and send It to the afterFind as a context to manage to have a different result.

It would give more freedom to work as a backend and make customizations, maybe a key on the config file.

For example, now I want to customize the JSON returning on an aggregation query, but it doesn't work well with Parse Server.
And for the use case of this issue would not be a problem, as you don't have to care about client's SDKs.

[mtrezza]

Ignoring that clients' SDKs exist?

What concept do you have in mind?

For example, now I want to customize the JSON returning on an aggregation query, but it doesn't work well with Parse Server.

With a Cloud Function you are free to return anything.

[matheusfrozzi]

Ignoring that clients' SDKs exist?

What concept do you have in mind?

Nothing special, just a focused backend version. hehehe

For example, now I want to customize the JSON returning on an aggregation query, but it doesn't work well with Parse Server.

With a Cloud Function you are free to return anything.

Yes for sure, but I don't want to have a lot of endpoints as POST that are supposed to be GET. But as I'm saying, I'm customizing on before/after find and not using the client's and it works. Just have to be organized.

[mtrezza]

What is a "focused backend version"? Not sure I understand what change you are suggesting.

I don't want to have a lot of endpoints as POST that are supposed to be GET.

I'm sure Cloud Functions could easily support GET requests (if they don't already do this). However this seems to be off topic; if you would like to open a feature request for that please search for existing issues and if necessary open a new issue.

[matheusfrozzi]

What is a "focused backend version"? Not sure I understand what change you are suggesting.

I don't want to have a lot of endpoints as POST that are supposed to be GET.

I'm sure Cloud Functions could easily support GET requests (if they don't already do this). However this seems to be off topic; if you would like to open a feature request for that please search for existing issues and if necessary open a new issue.

Sorry, I'm suggesting nothing, just this PR made me think about how the client's SDKs can make it harder to do some stuff, but as you said, it's possible to use cloud functions.

[mtrezza]

Got it, feel free to open a feature request for Cloud Functions for GET requests in a separate issue, if it's not already supported.

[mtrezza]

@dblythy Do you want to try to get this over the finish line? I believe this is almost done.