Skip to content

fix: matcher function lost#818

Merged
nodece merged 1 commit intoapache:masterfrom
kilosonc:js
Jun 19, 2021
Merged

fix: matcher function lost#818
nodece merged 1 commit intoapache:masterfrom
kilosonc:js

Conversation

@kilosonc
Copy link
Contributor

Signed-off-by: closetool c299999999@qq.com

Fix: #817

@kilosonc kilosonc marked this pull request as draft June 15, 2021 14:27
@kilosonc kilosonc marked this pull request as ready for review June 15, 2021 14:29
@nodece
Copy link
Member

nodece commented Jun 15, 2021

@closetool can you add a test for this change?

I also make a PR: #819 to fix the matcher function lost, and dirty data problems cause by LoadPolicy.

Your PR can be used to fix the model.ToText().

@kilosonc
Copy link
Contributor Author

@nodece Sure

@kilosonc
Copy link
Contributor Author

@nodece Added, plz review

@hsluoyz
Copy link
Member

hsluoyz commented Jun 16, 2021

@nodece @closetool #819 is merged, is this PR still useful?

@kilosonc
Copy link
Contributor Author

@hsluoyz Yeap, Model.ToText() still have a bug to fix.

"r": "sub, obj, act",
"p": "sub, obj, act",
"e": "some(where (p_eft == allow))",
"m": "r_sub == p_sub && r_obj == p_obj && is_rw(r_act, p_act)",
Copy link
Member

@nodece nodece Jun 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add test m = r_sub == p_sub && r_obj == p_obj && r_func(r_act, p_act) && testr_func(r_act, p_act).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@closetool don't remove the r_func() and testr_func(), which used to test rPattern := regexp.MustCompile(r_), and you should also test the regexp.MustCompile(p_).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@closetool Please update the test.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nodece There's still some problems, we cann't figure out if it's a policy token or a user defined function for pattern has p_ and r_, so testr_func will lead to error.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@closetool I found this problem, so I asked to use this master for test.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nodece fixed

Signed-off-by: closetool <c299999999@qq.com>
@nodece nodece merged commit 2c4ba4a into apache:master Jun 19, 2021
@github-actions
Copy link

🎉 This PR is included in version 2.31.5 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] casbin 2.30.3 an unexpected error occurred in the custom function

3 participants