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
refactor: Quick return of merging effects #140
Conversation
@sagilio plz resolve conflicts: |
Signed-off-by: Sagilio <Sagilio@outlook.com>
@xcaptain plz review. |
@sagilio can we release more frequently? the latest release is 26 days ago: https://github.com/casbin/Casbin.NET/releases/tag/v1.5.0 |
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.
Though I don't understand now, It should be right, we can merge it first and learn the new algorithm gradually.
@@ -631,12 +535,16 @@ private bool Enforce(IReadOnlyList<object> requestValues, ICollection<IEnumerabl | |||
IReadOnlyList<string> policyValues = Enumerable.Repeat(string.Empty, policyTokenCount).ToArray(); | |||
ExpressionHandler.SetPolicyParameters(policyValues); | |||
var nowEffect = GetEffect(ExpressionHandler.Invoke(expressionString, requestValues)); | |||
finalResult = Effector.MergeEffects(effect, new[] { nowEffect }, null, out hitPolicyIndex); |
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.
Actually, I do not quite understand this pr, the old behavior is put all policy evaluate result into an effects array and then decide the final result in MergeEffects
, you skiped this MergeEffects
how can you have a whole view of all the policy effects.
if (explain && hitPolicyIndex is not -1) | ||
{ | ||
explains.Add(policyList[hitPolicyIndex]); | ||
if (effector.TryChain(nowEffect)) |
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.
use TryChain
to replace MergeEffects
? I do not quite understand this method, but it should be right because all the tests passed.
Ok, Several features rest in the v2.x plan will be implemented in the 1.x version first. |
Is this a breaking change? If not, please put it into origin:master instead of origin:develop. Putting everything into origin:develop like this will cause a big disaster when people start to migrate to it, as they will find so many things have changed, and a cetain number of them may be bugs. |
@hsluoyz |
🎉 This PR is included in version 2.0.0-preview.1 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
🎉 This PR is included in version 2.0.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
🎉 This PR is included in version 2.0.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Fixes: #139