parse-community / parse-server

Parse Server for Node.js / Express
https://parseplatform.org
Apache License 2.0
20.84k stars 4.77k forks source link

Support wildcard classname in cloud triggers #7361

Open uzaysan opened 3 years ago

uzaysan commented 3 years ago

New Feature / Enhancement Checklist

Current Limitation

Currently user have to define cloud triggers one by one. If we have a recurring logic inside of cloud trigger, defining triggers become too hard.

Feature / Enhancement Description

We should be able to define one trigger that applies to all class. Unless that class has its own triggers.

Example Use Case

Parse.Cloud.beforeSave("Test", (request) => {
  //this trigger will be executed for only Test class
});

Parse.Cloud.beforeSave("*", (request) => {
  //this trigger will be executed for every class except Test class
});

Alternatives / Workarounds

No known workaround or alternative.

3rd Party References

Its similar to setting SSL sertificates to multiple subdomains.

mtrezza commented 3 years ago

Thanks for suggesting.

Please use the issue template provided and describe the feature in more detail.

The PR already makes the assumptions that the "trigger will be called for every class unless class has its own trigger".

This and possibly other aspects should be discussed in the issue.

uzaysan commented 3 years ago

@mtrezza I have edited the issue.

mtrezza commented 3 years ago

I think the obvious alternative would be to define a function that contains the recurring logic.

Can you explain the assumption that these generic Cloud Function definitions override a class specific definition?

uzaysan commented 3 years ago

You mean something like this?

Parse.Cloud.beforeSave("Test", (request)  => {
  recurringLogic(request);
});

const recurringLogic = (request)  => {
  //Logic
};

Or even like this:

Parse.Cloud.beforeSave("Test",recurringLogic);

const recurringLogic = (request)  => {
  //Logic
};

But user is still have to define trigger for every class.

Can you explain the assumption that these generic Cloud Function definitions override a class specific definition?

For example all classes has a user pointer. And Developer may want to assign the user that created the parse object. Developer can do this with this code:

Parse.Cloud.beforeSave("Test",(request) =>{
  if(!request.original){
    //assign user to newly created object
    request.object.set("user",request.user);
    return request.object;
  }
});

This is a simple code but if developer has many classes, defining the triggers manually would take too long. If developer has 100 classes, he/she has to write same code 100 times.

But I admit that 99% of projects doesn't go beyond 15-25 classes. And generally classes has seperate logic. But this feature would make coding easier for those who need it. Its rare. But since someone opened a thread on community forum, that means someone need it.

mtrezza commented 3 years ago

But user is still have to define trigger for every class.

Yes, I wanted to mention it as an alternative, so we understand what the status quo is. The issue says that no alternatives exist.

this feature would make coding easier for those who need it.

I agree, it seems to be a useful addition to Parse Server.

If user set trigger for class "*", that trigger will be called for every class unless class has its own trigger.

I would like to understand how you came to the assumption that these generic Cloud Function definitions should override a class specific definition. If I understand you correctly this means:

Parse.Cloud.beforeSave("Test", () => {
});

Parse.Cloud.beforeSave("*", () => {
  // This would not be called for `Test` because it has its own definition
});
uzaysan commented 3 years ago

Yes, I wanted to mention it as an alternative, so we understand what the status quo is. The issue says that no alternatives exist.

User still have to define triggers manually so not an exactly alternative.

I would like to understand how you came to the assumption that these generic Cloud Function definitions should override a class specific definition. If I understand you correctly this means:

This is because developer may want to seperate some class logic from the others. Just because you used wildcard triggers doesn't mean you cant use class specific triggers. If developer has a common logic for 50 class but different logic for a few class, they can use generic triggers for that classes.

Like this:

Parse.Cloud.beforeSave("Exception", () => {
  //This class is created by developer in cloud code. So we don't need to assign user to it.
});

//or

Parse.Cloud.beforeSave("_User", () => {
  //This class is already user. So assigning user to itself  wouldn't make sense.
});

Parse.Cloud.beforeSave("*", () => {
  // But this trigger will be called for the rest of the classes.
});

We are making the job easy while not limiting the developers options. Edit:

I would like to understand how you came to the assumption that these generic Cloud Function definitions should override a class specific definition.

I think you meant class specific trigger override generic one. PR looks first class specific trigger. If there is no class specific trigger, then it uses wildcard trigger.

If I understand you correctly this means:

Parse.Cloud.beforeSave("Test", () => { });

Parse.Cloud.beforeSave("*", () => { // This would not be called for Test because it has its own definition });

Yep. That's correct.

mtrezza commented 3 years ago

User still have to define triggers manually so not an exactly alternative.

Alternative here means how it can be done currently, without this enhancement. So we know where we are coming from. Could you add this to the issue description?

We are making the job easy while not limiting the developers options.

We are actually doing the opposite, because we implement a fixed rule in case of multiple definitions for the same class. You can make your feature more useful to more developers if you give some freedom to cover more use cases. The question is: if we have multiple definitions (specific and generic) for a class, how should they be handled?

The above is roughly in order of complexity, meaning a) will be less work in the PR than d), but inversely, d) will be more useful to developers than a).

When choosing a concept we have to look at forward compatibility, meaning if we go for a) or b) now, we may have more difficulty going to c) or d) later, because it may be a breaking change.

uzaysan commented 3 years ago

Alternative here means how it can be done currently

Its still can't be done. Because we still have to write all class name manually which is what we are trying to escape.

The question is: if we have multiple definitions (specific and generic) for a class, how should they be handled?

a) Execute the specific definition but ignore the generic definition ← currently suggested b) Execute the generic definition but ignore the specific definition c) Execute both definitions in some fixed order d) Execute both definitions in a developer customizable order

I think option A would be best. Option B is in least favorable in my opinion. Because It locks developers hand. Its either black or white. No between. And about option C and D, I would choose C. I would run class specific trigger first then execute generic trigger. But running two triggers for same class doesn't make sense to me. I mean I see your point but why use generic trigger if you already defined class specific trigger?

if we go for a) or b) now, we may have more difficulty going to c) or d) later, because it may be a breaking change.

Why would we wanna switch to the c or d in future? Can you give me an example

Edit: I miss read. You can Ignore the below. How exactly? Solution A wont be a problem for old versions. Since old versions has only class specific triggers, PR will look class specific trigger first, If user defined it, It will execute. If its undefined, then it look for generic trigger. And since old versions doesn't have generic trigger parse will do what it do when you save an object and class doesn't have any trigger.

For the future versions, I really can't see how is this gonna be problem. Can you clarify this?

mtrezza commented 3 years ago

Its still can't be done. Because we still have to write all class name manually which is what we are trying to escape.

You could just iterate through the schema and get all class names that match a regex, that's the same.

But running two triggers for same class doesn't make sense to me.

Your user example, if you have additional per-class custom code.

I think option A would be best. Option B is in least favorable in my opinion.

Option a) and b) are equally restrictive. You may prefer a) because it works for your case, other may prefer b) for their case. The same with the order in c). The only practical option is d) in my opinion. Any idea how that can be made configurable?

uzaysan commented 3 years ago

You could just iterate through the schema and get all class names that match a regex, that's the same.

Yes. You are right.

Option a) and b) are equally restrictive.

I disagree. You can always put generic logic to class specific trigger. But you can't put class specific logic to generic trigger. If you do, it's not generic logic anymore.

The only practical option is d) in my opinion.

I would choose A. My logic is start from the small cluster and move to bigger ones. But If we really need to run both triggers I would let developer decide that.

Any idea how that can be made configurable

We already have validators. We can use that.

Parse.Cloud.beforeSave("Test", (request) => {
// do any additional beforeSave logic here
},{
    executionOrder: Parse.runThisOnly || Parse.before || Parse.after 
    // if its before execute  class specific trigger before generic one and visa versa and if user choose runThisOnly, we dont run generic trigger.
  }
});

We can use that. But its unnecessarily complex. Current solution works for old versions. And will work for newer versions. We can put documentation that" class specific trigger will override generic trigger " or something like that.

mtrezza commented 3 years ago

You can always put generic logic to class specific trigger. But you can't put class specific logic to generic trigger. If you do, it's not generic logic anymore.

Not sure I follow. My point is that you want the following rule:

But someone else may want this rule:

It may be a less common use case, but there may be scenarios in which this applies. For example if someone has a deployment in which Parse Functions load conditionally.

I think we need such a decision path logic anyway, because you can have multiple generic triggers that overlap.

Parse.Cloud.beforeSave("Test", () => {
});

Parse.Cloud.beforeSave("T.*", () => {
});

Parse.Cloud.beforeSave(".*", () => {
});

Which one should be executed?

In my opinion all, the question is just in which order. The developer just has to understand that the class name allows regex, that would be the only change in the docs.

uzaysan commented 3 years ago

Not sure I follow. A specific trigger can contain the same code as generic triggers. Can you give an example?

Parse.Cloud.beforeSave("*", (request) => {
  if(!request.original){
    //assign user to newly created object
    request.object.set("user",request.user);
    return request.object;
  }
});

Parse.Cloud.beforeSave("DirectMessage", (request) => {
  if(!request.original){
    //assign user to newly created object
    request.object.set("user",request.user);

    const otherUser = request.object.get("receiver");
    const checkBlock = new Parse.Query("Block");
    checkBlock.equalTo("owner",otherUser);
    checkBlock.equalTo("blocked",request.user);
    checkBlock.limit(1); 
    const result = await checkBlock.count();
    if(count > 0){
      throw "You cant send message to this user";
    }
    return request.object;
  }
});

if a generic trigger exists, do not run the specific trigger

If you don't want the specific trigger to run, why would you even define it in first place.

It may be a less common use case, but there may be scenarios in which this applies. For example if someone has a deployment in which Parse Functions load conditionally.

I didn't understand this.

In this code,

Parse.Cloud.beforeSave("Test", () => {
});

This only executed for class Test

Parse.Cloud.beforeSave("T.*", () => {
});

Parse.Cloud.beforeSave(".*", () => {
});

Which one should be executed?

They dont overlap. Do they? They will all call same generic function. Now I see. But you can do same with the current system.

Parse.Cloud.beforeSave("Test", () => {
});

Parse.Cloud.beforeSave("Random.Test", () => {
});

Right now which one parse executes?

mtrezza commented 3 years ago
Parse.Cloud.beforeSave("Test", () => {
});

Parse.Cloud.beforeSave("Test", () => {
});

Currently, if you have multiple CFs, I assume the official behavior is undefined (it's a developer mistake), the unofficial behavior is that the second CF definition overrides the first. There has been a PR that warns if there are multiple CFs for the same class.

This gets more complex with regex CF, because we need to define now what should happen. In my opinion all CFs that match the class name regex should be executed. That gives the developer most versatility. It would be a straight forward PR and easy to comprehend: execute all triggers that regex match the class name.

In addition, the context object is accessible from all triggers, so the developer can leverage that for more complex scenarios.

If you want a specific CF to "override" the generic CF, you would simply change the regex of the generic CF to exclude a specific class name, so that only the specific CF is executed. That would be the most intuitive, straight forward behavior. Any compulsory rule (like ignore generic if there is a specific) is arbitrary and increases complexity and restrictions down the road.

uzaysan commented 3 years ago

Should we create a poll on community forum? What do you say? We would see what people want. İf they want to run both triggers we run both triggers. İf they want one to override the other, we go that way. @mtrezza

mtrezza commented 3 years ago

Not sure why you suggest polling? This is not a question of apple or carrot, but rather of apple or grocery store (grocery store having apples, carrots and much more).

I want to make sure you understand the regex approach:

The approach to implement a regex syntax and execute all matching functions covers many use cases, including your specific use case. You can set it up to not execute a specific CF.

The approach you are suggesting may cover your specific use case, but it is restrictive and not versatile. That means it will be usable in less use cases. It is also problematic on other fronts:

For example, what you can do with regex:

Let me know where you see an issue with the regex approach, or why you think a restrictive approach is better.

uzaysan commented 3 years ago

I think our issue here is not how we implement wildcard triggers but what will we do if multiple triggers are set for same class. I support one trigger approach. If user defined multiple triggers we must override some of them like current system.

Let me know where you see an issue with the regex approach

I liked the regex approach. But problem relays on what if one class has multiple triggers. Currently if multiple trigger is set to one class last one overrides other. I think we should keep this functionality. And since we cant do this on regex approach, we should run smallest cluster.

For example if user defined 2 triggers and we have UserProfile class.

1- beforeSave("U" - trigger for all classes that name starts with letter U. 2- beforeSave("User" - trigger for all classes that name starts with User.

I think we should run only 2nd trigger. Because ıt invokes for less classes.

And If user add 3rd trigger like beforeSave("UserProfiles" we should bypass 1st and 2nd. And we should run 3rd one.

Not sure why you suggest polling

To see what people really want.

but rather of apple or grocery store (grocery store having apples, carrots and much more).

But if market demand is for only apples, you lose money on others. And If we bring a feature that nobody wants, time and effort will be wasted.

mtrezza commented 3 years ago

It seems you are misunderstanding the regex approach and ignoring the implications of your approach.

1- beforeSave("U" - trigger for all classes that name starts with letter U. 2- beforeSave("User" - trigger for all classes that name starts with User. I think we should run only 2nd trigger. Because ıt invokes for less classes. And If user add 3rd trigger like beforeSave("UserProfiles" we should bypass 1st and 2nd. And we should run 3rd one.

You keep saying "we should", but what is your argument for that?

Maybe it's easier if we talk about a specific example. Can you give one example that you want to do with your approach but cannot be done with the regex approach?

uzaysan commented 3 years ago

It seems you are misunderstanding the regex approach and the implications of your approach.

One question. Is it possible to run multiple triggers for same class with regex? Multiple beforeSave trigger for example?

mtrezza commented 3 years ago

Can you give an example, I want to make sure we talk about the same.

uzaysan commented 3 years ago

Can you give an example, I want to make sure we talk about the same.

My way is running only one trigger for classes. Multiple triggers can be confusing and developer can lose the track easily. And even we run multiple trigger, how will we determine the order? And ıf I understand regex way, we cant really know how many triggers will user define.

My example is this:

For example if user defined 2 triggers and we have UserProfile class.

1- beforeSave("U" - trigger for all classes that name starts with letter U. 2- beforeSave("User" - trigger for all classes that name starts with User.

I think we should run only 2nd trigger. Because ıt invokes for less classes.

And If user add 3rd trigger like beforeSave("UserProfiles" we should bypass 1st and 2nd. And we should run 3rd one.

We should run only one trigger and that trigger must represent smallest cluster.

Now If you can give an example that would be great. Maybe we have a communication problem.

mtrezza commented 3 years ago

Yes, in your example, all 3 triggers will run.

We should run only one trigger and that trigger must represent smallest cluster.

You can define that with regex and run only 1 trigger.

What exactly is your concern if the behavior you want (and much more) can be achieved with regex?

If you prefer we can take this off to Slack for a quick chat. I really think this is just a miscommunication.

uzaysan commented 3 years ago

Can you give an example that works with your approach but not with regex?

No I can't because like you said I can define class specific trigger by directly entering class name. But If I already define class specific trigger why would I want to seperate my code? PR's aim was getting rid of the defining triggers seperatly.

If you prefer we can take this off to Slack for a quick chat. I really think this is just a miscommunication.

Sure I've downloaded it. How can I reach you?

mtrezza commented 3 years ago

PR's aim was getting rid of the defining triggers seperatly.

Your PR may do that, but it is tailored too much to your use case. Parse Server philosophy is choosing universality over restriction to make it applicable for as many use cases as possible. Also we have to think beyond this PR, regex is the logical next step after your PR, and we will run into compatibility issues if we go with that PR now.

How can I reach you?

You can join the parse community slack group. I'll be reachable there in about two hours.

uzaysan commented 3 years ago

Your PR may do that, but it is tailored too much to your use case. Parse Server philosophy is choosing universality over restriction to make it applicable for as many use cases as possible. Also we have to think beyond this PR, regex is the logical next step after your PR, and we will run into compatibility issues if we go with that PR now.

Ok. That's understandable. Do you have any idea how can we do regex implementation?

This is current code in parse repo:

const trigger = get(Category.Triggers,${triggerType}.${className}, applicationId);

and this is the get unctions:

function get(category, name, applicationId) {
  const lastComponent = name.split('.').splice(-1);
  const store = getStore(category, name, applicationId);
  return store[lastComponent];
}

It seems like if we wanna support regex, we need to retrieve every trigger before countinue. And check them manually. How can we solve this? Or does it need to be solved? Doing a few extra steps doesnt exaust CPU.

You can join the parse community slack group. I'll be reachable there in about two hours.

Two hours from now is midnight here so I may not reply but I can reply tomorrow. My username is @uzaysan

mtrezza commented 3 years ago

Basically there are two regex approaches:

We can go with a) for simplicity and it can be optimized in the future if someone wants to do that.

The question you asked is still valid: in which order should the CFs run if multiple CFs regex match a class? Do you have any suggestion?

uzaysan commented 3 years ago

in which order should the CFs run if multiple CFs regex match a class?

Order actually doesn't matter anymore because parse will execute them all. If we were eliminate some of the triggers(like my suggestion) order was important. But since we will run them all, we can run them all without a specific order.

Basically there are two regex approaches:

a) On-the fly evaluation in maybeRunTigger and test every CF regex definition whether it matches the current class name b) Create a mapping table on parse server start that defines for each class which triggers it should run. A table look-up may use less resources than n regex evaluations, but this introduces more complexity on schema change.

Option A looks better than B.

mtrezza commented 3 years ago

Order actually doesn't matter anymore because parse will execute them all.

Order can matter depending on what the trigger does. We may well say that the execution order is undefined for now. That simplifies the PR, leaving the door open for future change without breaking anything.

uzaysan commented 3 years ago

Currently we have this triggers:

export const Types = {
  beforeLogin: 'beforeLogin',
  afterLogin: 'afterLogin',
  afterLogout: 'afterLogout',
  beforeSave: 'beforeSave',
  afterSave: 'afterSave',
  beforeDelete: 'beforeDelete',
  afterDelete: 'afterDelete',
  beforeFind: 'beforeFind',
  afterFind: 'afterFind',
  beforeSaveFile: 'beforeSaveFile',
  afterSaveFile: 'afterSaveFile',
  beforeDeleteFile: 'beforeDeleteFile',
  afterDeleteFile: 'afterDeleteFile',
  beforeConnect: 'beforeConnect',
  beforeSubscribe: 'beforeSubscribe',
  afterEvent: 'afterEvent',
};

Since this triggers will only run for one class or for one specific thing, we should leave them.

  beforeLogin: 'beforeLogin',
  afterLogin: 'afterLogin',
  afterLogout: 'afterLogout',
  beforeSaveFile: 'beforeSaveFile',
  afterSaveFile: 'afterSaveFile',
  beforeDeleteFile: 'beforeDeleteFile',
  afterDeleteFile: 'afterDeleteFile',
  beforeConnect: 'beforeConnect',
  beforeSubscribe: 'beforeSubscribe',
  afterEvent: 'afterEvent',

Regex will only apply to these triggers

  beforeSave: 'beforeSave',
  afterSave: 'afterSave',
  beforeDelete: 'beforeDelete',
  afterDelete: 'afterDelete',
  beforeFind: 'beforeFind',
  afterFind: 'afterFind',

Is this sounds good?

Edit: Looks like all triggers (except file trigger) use same maybeRunTrigger function for trigger. So This may not matter.

mtrezza commented 3 years ago

beforeSave: 'beforeSave', afterSave: 'afterSave', beforeDelete: 'beforeDelete', afterDelete: 'afterDelete', beforeFind: 'beforeFind', afterFind: 'afterFind',

Yes, I think these make sense, they are the only class related triggers it seems.

mtrezza commented 3 years ago

So the PR needs to add is a regex test condition and iterate over all definition names.

We may need to think of how to store the class definitions, currently definitions are only chars and numbers I think, but a regex definition could be a string or a RegExp instance.

uzaysan commented 3 years ago

I was thinking something like this. Syntax will be "Cla*". and this trigger will run for all classes that starts with Cla.

My example code:

export function getTrigger(className, triggerType, applicationId) {
  if (!applicationId) {
    throw 'Missing ApplicationID';
  }
  let tmpClassName = className;
  const classSpecificTrigger = get(Category.Triggers, `${triggerType}.${className}`, applicationId);
  const triggers = [classSpecificTrigger];

  while (tmpClassName.length > 0){
    const trigger = get(Category.Triggers, `${triggerType}.${tmpClassName}*`, applicationId);
    triggers.push(trigger);
    tmpClassName = tmpClassName.substring(0,tmpClassName.length -1);
  }

  const triggerForAll = get(Category.Triggers, `${triggerType}.*`, applicationId);
  triggers.push(triggerForAll);
  return triggers;
}

This function will look for this triggers:

beforeSave.ClassName
beforeSave.ClassName*
beforeSave.ClassNam*
beforeSave.ClassNa*
beforeSave.ClassN*
beforeSave.Class*
beforeSave.Clas*
beforeSave.Cla*
beforeSave.Cl*
beforeSave.C*
beforeSave.*

And in maybeRunTrigger istead of running trigger for one time, we run trigger in a loop.

İnstead of this :

var trigger = getTrigger(parseObject.className, triggerType, config.applicationId);//trigger is a single trigger here
const promise = trigger(request);

We do this:

var triggers = getTrigger(parseObject.className, triggerType, config.applicationId); // triggers is an array.
for(const trigger of triggers){
  const promise = trigger(request);
}

This function assume that syntax will be my suggestion.

mtrezza commented 3 years ago

This is not regex syntax. Can you come up with a regex approach?

uzaysan commented 3 years ago

Can you give me an example? And also whats wrong with this approach? It does what we want. Doesn't it?

mtrezza commented 3 years ago

You would have to get familiar with what regex is and how it works to design a solution for this issue.

whats wrong with this approach? It does what we want. Doesn't it?

No it doesn't. How would you run a trigger for all class names that start with U and end with r or s?

uzaysan commented 3 years ago

You would have to get familiar with what regex is and how it works to design a solution for this issue.

I know what regex is. But parse gets trigger with return store[lastComponent]; . And last component is class name in this case. Do you have any idea how can we implement regex here?

No it doesn't.

Can you give an example that regex can do but this approach cant?

mtrezza commented 3 years ago

Can you give an example that regex can do but this approach cant?

How would you run a trigger for all class names that start with U and end with r or s?

uzaysan commented 3 years ago

Can you give an example that regex can do but this approach cant?

How would you run a trigger for all class names that start with U and end with r or s?

Do you have any idea how can we implement regex to this: return store[lastComponent]; ? If we go with your way storing triggers must change.

mtrezza commented 3 years ago

It seems we are going in circles so let's settle some points:

As I suggested previously, the next step would be to look at how triggers are stored and what needs to be changed.

uzaysan commented 3 years ago

So you are saying that

Parse.Cloud.beforeSave("/@[a-z0-9_\.]+/gi", function); User can do this. Regex here is just an example.

And internally we can retrieve all beforeSave triggers. And chech if regex matches like this:

let triggerArray = [];
Object.keys(triggerStore).forEach(function(key) {
    if(className === key) {
        triggerArray.push(triggerStore[key]);
    }
    const regex = new RegExp(key);
    if(className.match(regex) {
        triggerArray.push(triggerStore[key]);
    }
});

Then we run all triggers inside of the array. Are you talking about something like this?

mtrezza commented 3 years ago

Yep! That's what I was thinking of, although I don't fully understand the code above.

I just assume that we need to account for some cases.

For example, Test if interpreted as regex matches anything that includes the word, like AnotherTestClass. That would break current behavior. So either we

I think b) makes more sense.

To clarify, there are:

Currently, we have only (a) and we are now adding (b), which is not the same as (a). We need to consider the difference between (a) and (b), because (a) is actually /^Test$/.

uzaysan commented 3 years ago

although I don't fully understand the code above.

If we want to support regex, that means we use regex string as object key. So our object will look like this

  beforeSaveTriggers: {
    Test: triggerFunction,
    /^Test$/: triggerFunction1,
    SomeOtherClass: triggerFunction2,
    ...
  }

So first we have to extract regex strings from object. And Object.keys do that. İt extracts key. Then my code check if key is equal to Class name. İf it is equal, that means our key is not regex. Its regular string since we cant create class with regex string.

if classname is now equal, we can assume its regex altough it may not. But since it will not match other strings we can ignore that.

And our final code will look like this

export function getTrigger(className, triggerType, applicationId) {
  if (!applicationId) {
    throw 'Missing ApplicationID';
  }
  if(triggerType === Types.beforeSave ||
    triggerType === Types.afterSave ||
    triggerType === Types.beforeDelete ||
    triggerType === Types.afterDelete ||
    triggerType === Types.beforeFind ||
    triggerType === Types.afterFind ) {
      //We will get trigger store
    const triggerList = [];
    const store = getStore(Category.Triggers, `${triggerType}.${className}`, applicationId);
    Object.keys(store).forEach(key => {

      if(className === key) {
        triggerList.push(triggerStore[key]);
      } else {
        const regex = new RegExp(key);
        const regexResult = className.match(regex);
        if(regexResult !== undefined && regexResult !== null) {
          triggerList.push(triggerStore[key]);
        }
      }
    });
    return triggerList;
  }
  return [get(Category.Triggers, `${triggerType}.${className}`, applicationId)];
}

We will return array of trigger for all triggers:

And in maybeRunTrigger function we will run array of triggers.

for(const trigger of triggers) {
    const promise = trigger(request);
}
mtrezza commented 3 years ago

Conceptually yes, but a regex cannot be used as JS object key, so the code above is unlikely to work as it is.

Do you want to start a PR and see how to get it working?

uzaysan commented 3 years ago

Do you want to start a PR and see how to get it working?

Sure. Let me see what I can do.

tremendus commented 3 years ago

Somehow I missed this thread going on but I think it was triggered by my request on the community forum. My summary of your discussion and points related to it:

So in this case: Parse.Cloud.beforeSave('*', commonFunctions)

and Parse.Cloud.beforeSave(ParseUser, async (request) { await checkUserNotExists(request) //- more functions .... })

Regarding hooks-vs-behaviors-and-traits - the idea of specifying the behavior of class type based on the hooks called this way may be considered counter-intuitive by those used to the ORM world and data modelling.

IMHO, it would have been much better - or would become much better - if the behaviors/traits were class-based:

I realize that's a breaking change, but it would have been/would be a better design pattern IMHO.

mtrezza commented 3 years ago

@tremendus thanks for your comment.

I want to iterate 3 points here that we have already discussed at length in this thread:

  1. Regex can do anything and more than what * can do; we need to look beyond satisfying a particular use case and aim for versatility
  2. * is a custom syntax; there is no benefit but many downsides when reinventing the wheel with a custom syntax; regex on the other hand is defined and easily extendable.
  3. So far there has not been one valid argument in favor of introducing the custom definition of * with a use case example that can be achieved with * but not with regex. On the other hand this thread shows the many downsides of *. So argumentatively, everything speaks for regex.

Maybe you can give a more detailed example use case, so we can look into that.

If what you are suggesting is something like a Parse.Cloud.beforeSaveDefault(()=>{}) function, that executes if no class specific trigger is set, that seems like a tailored solution that goes against Parse Server's aim for versatility.

To your specific points:

class-specific callbacks (eg. on create user,

If you mean "on instantiating" the user, this can already be achieved with subclassing. If you mean "on saving" the user that would be a collection based hook, i.e. a beforeSave User, as it already exists.

may be considered counter-intuitive by those used to the ORM world and data modelling. ... it would have been much better - or would become much better - if the behaviors/traits were class-based

Could you elaborate on that? In Parse Server, a "class" is essentially a fusion of a programmatic "object class" and a DB "storage collection". If you suggest that it should be possible to do Parse.User.beforeSave(...), that is just a different way of writing Parse.Cloud.beforeSave("User",...). I assume the historic reason why this is a method of Parse.Cloud is because this object is only available in Cloud Code, which is where triggers are defined, while Parse.User is available client side as well, so this makes sense for Parse Server. In a data model there is no client - server distribution aspect, so it would be expected that data modeling aspects differ from aspects of a client-server system.

sadortun commented 3 years ago

Sorry for getting late in this thread ...

I would suggest to avoid as much as possible using strings definitions for classes names. Simply for a type-safety perspective, using real classes types is safer, expecially in larger applications. If a class get refactored, you get a direct error instead of simply having your callback not called.

I think this is especially important since if you use a Regex, and for some reason it doesnt get triggered, this could have severe security implications for the server (aka, Auth no longer being enforced )


I would recommend the following:

class SensitiveObject  extends Parse.Object  {} 
class SomeOtherObject extends SensitiveObject  {} 

Parse.Cloud.beforeSave(Parse.Object,   () => console.log("Object saved") )

Parse.Cloud.beforeSave(SensitiveObject,   () => throw "Yikes, Not authorized ...." )

On the server side, it's now very easy and simple to locate triggers.

triggers.filter( myTrigger =>  myTrigger.className instanceof  className ) 
mtrezza commented 3 years ago

I think this is especially important since if you use a Regex, and for some reason it doesnt get triggered, this could have severe security implications for the server (aka, Auth no longer being enforced )

I think this is a different topic. That is a developer responsibility. Regex has no type safety, it works on a lower level if you will. We cannot force a developer to write tests for their app, nor can we force a developer to only centrally subclass custom Parse Objects instead of writing multiple lines of Parse.Object.extend('CustomClassName'). All best practices you mention make sense, and they are currently possible with Parse Server.

sadortun commented 3 years ago

As discussed above, this will allow to have multiple triggers for one class, which is great!

Using this method, It would also be fairly easy to guarantee the execution order. Once you filtered the triggers to get only relevant ones. You can in a second step sort the triggers by hineritance order ( aka, the most top object level first )

In my example above, this would result in

SomeOtherObject instanceof SensitiveObject     && SomeOtherObject  instanceof Parse.Object
SensitiveObject  instanceof Parse.Object

Its now easy to deduce we need to execute them in the following order:

  1. Parse.Object
  2. SensitiveObject
  3. SomeOtherObject

This would solve so many issues :) And simplify cloud functions a lot :)

mtrezza commented 3 years ago

You can in a second step sort the triggers by hineritance order ( aka, the most top object level first )

Interesting idea. The execution order seems arbitrary. If we go with inheritance principles, a trigger would be inherited by child and we would offer something like a super() to execute the inherited trigger and one can override the trigger without calling super().

That means one could define a trigger for Parse.Object which would be the default trigger, which I think @tremendus mentioned. A child class can then simply override the trigger in a OOO-similar syntax. That could make a regex or wildcard approach unnecessary. A difference being that OOO requires a tree inheritance, but regex allows an across-inheritance triggers, which means greater flexibility.

I think this goes beyond this issue of "wildcard classname in cloud triggers" and requires discussion in a separate issue, if we want to pursue that alley.

sadortun commented 3 years ago

I think this goes beyond this issue of "wildcard classname in cloud triggers" and requires discussion in a separate issue, if we want to pursue that alley.

Yes probably. 😄

I think this is a different topic. That is a developer responsibility. Regex has no type safety, it works on a lower level if you will.

I completely agree with you on this, but you need to keep in mind that the more options we give the developer, the more chances someone have to make mistakes. Using straightforwoard exact classes names have a very low risk of making mistakes. using wildcards increase the risk of going wrong, and regex is especially "risky" to use. The chance you make a change in a regex at some point and you break everything is quite high ;) !


to execute the inherited trigger and one can override the trigger without calling super()

I think that if inheritance is used, each trigger should be independent and un-releated. Ill give you a quick example:

You add beforeSave(SomeRandomObject , ... ) and a few months later you decide to add a security trigger to beforeSave(Parse.Object, ...) This new Parse.Object trigger should apply to EVERYTGHING and not have to modify existing triggers.

If for some reason you do not want to have the Parse.Object trigger to be executed, it's probably the developper responsability to re-factor your code so that parent trigger is not in the line of inheritance.

Another option could be to have DOM-like helper added to the request like request.event.stopPropagation();