Skip to content
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

Add new CheckRulesAsync overloads #3756

Closed
StefanOssendorf opened this issue Mar 27, 2024 · 5 comments · Fixed by #3792
Closed

Add new CheckRulesAsync overloads #3756

StefanOssendorf opened this issue Mar 27, 2024 · 5 comments · Fixed by #3792

Comments

@StefanOssendorf
Copy link
Contributor

StefanOssendorf commented Mar 27, 2024

Currently BusinessRules has two async check methods

  • CheckRulesAsync()
  • CheckRulesAsync(int timeout)

I'd like to request/propose 3 new and an idea:

New overloads

  • CheckRulesAsync(TimeSpan timeout)
  • CheckRulesAsync(Csla.Core.IPropertyInfo property)
  • CheckRulesAsync(Csla.Core.IPropertyInfo property, TimeSpan timeout)

The first one for using the right type for the right job. For an int you always have to look up in which unit it's wanted.
The second and third to get the same options as for the sync ones.

Idea

What about supporting cancellation tokens for the async execution? I see the problem that that would give the impression that the rule execution can be cancelled. Maybe that's a nice new feature after #3736 ?

@rockfordlhotka
Copy link
Member

I like the new overloads.

The challenge I see with cancelation tokens is that they are viral - they'd need to flow down through all the async APIs associated with rules - which also means the async data portal methods.

Also, there isn't an API to cancel running rules, so how would that actually work?

@StefanOssendorf
Copy link
Contributor Author

Yes. Adding CancellationToken support is another kind of pandoras box. It was just an idea to create mor work 😂.

There are a lot more implications to consider:
e.g. a token in FetchAsync which would mean the HttpClient gets cancellable. Which in turns mean the asp.net core part has to obsovere the Request.Aborted (IIRC) token which must flow throughout the whole framework.
Furthermore how to ignore that token for specific calls where I know the call ended but I want to finish my stuff regardles of client connection and so on.
I will think about that along the way. But I have other stuff I want to do first :D

@StefanOssendorf
Copy link
Contributor Author

I started working on this. What about an additional overload:
CheckRulesAsync(IEnumerable<IPropertyInfo> properties, TimeSpan timeout) to give the ability to check multiple properties at once?

@rockfordlhotka
Copy link
Member

Intuitively this makes sense. My only concern is YAGNI - is there an actual scenario where someone would use this, or is it just code we have to maintain that nobody will use?

@StefanOssendorf
Copy link
Contributor Author

We have a couple of places where we invoke rules for up to four properties explicitly because we can't add them as Input properties. We end in an endless rule execution loop when we add them to the rule.

Furthermore I'll create a proof of concept (rough outline here) for the bo.SetAndWaitForRules(Action<BO> operation); method on business objects.
I imagine something like

await myBo.SetAndWaitForRules(bo => {
  bo.Prop1 = xyz;
  bo.Prop2 = xyz;
  bo.Prop3 = xyz;
});

And the method tracks which properties were set and invoke only the rules affected by those properties.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants