use Rc<RefCell<Role>> to store roles #9
Conversation
to make shared mutability
Codecov Report
@@ Coverage Diff @@
## master #9 +/- ##
===========================================
+ Coverage 61.63% 76.45% +14.82%
===========================================
Files 8 9 +1
Lines 735 1015 +280
===========================================
+ Hits 453 776 +323
+ Misses 282 239 -43
Continue to review full report at Codecov.
|
add more tests
|
@hsluoyz please take a look, I think most functions in |
|
I'm using a https://github.com/casbin/casbin/blob/master/rbac/default-role-manager/role_manager.go#L91 Is that a bug in casbin, because when passing 2 string as domain to I'm suggesting using |
|
Why not use the |
|
I have forgotten why this commit is necessary: casbin/casbin@9a33e2d Can anybody remind me why? |
|
I have separated the DefaultRoleManager to a different file. But I think module visibility is another thing that we can improve in the later commits. |
| .clone() | ||
| } | ||
|
|
||
| fn has_role(&self, name: &str) -> bool { |
There was a problem hiding this comment.
This method implementation can be replaced with a call to contains_key on the underlying HashMap.
There was a problem hiding this comment.
Thanks, contains_key is better here, I will fix this later.
|
|
||
| fn add_link(&mut self, name1: &str, name2: &str, domain: Vec<&str>) { | ||
| let mut name1 = name1.to_owned(); | ||
| let mut name2 = name2.to_owned(); |
There was a problem hiding this comment.
It appears unnecessary to create owned references to these strings.
There was a problem hiding this comment.
I'm using to_owned here to convert &str to String, because format! need String as parameters. Do you suggest using concat! here?
| } | ||
|
|
||
| fn has_direct_role(&self, name: &str) -> bool { | ||
| for role in self.roles.iter() { |
| @@ -0,0 +1,16 @@ | |||
| pub trait RoleManager { | |||
| fn clone_box(&self) -> Box<dyn RoleManager>; | |||
There was a problem hiding this comment.
Rather than require this method, you can add a constraint on the self type of this trait to require implementors are Clone.
There was a problem hiding this comment.
I learned this technical from https://users.rust-lang.org/t/how-to-clone-a-boxed-closure/31035 I don't know how to add clone constraint to self, can you be more specific?
There was a problem hiding this comment.
If you use dyn Rolemanager any where else you can not change this because your trait becomes object unsafe and you then can not refer to it as dyn Rolemanager.
If you don't need that ability it would look like this
// this means that every type that impl's RoleManager must be Clone
pub trait RoleManager: Clone {
// No more clone_box
fn clear(&mut self);
fn print_roles(&self);
}
#[derive(Clone)]
struct Try(Option<fn()>);
impl RoleManager for Try {
fn clear(&mut self) {}
fn print_roles(&self) {}
}There was a problem hiding this comment.
I tried to use RoleManager: Clone to replace boxed trait with clone, but gave up in the end, because this approach introduces so many trait constraints. I have a tracking task about it at #5 Can you help make a pull request on this? @DevinR528
There was a problem hiding this comment.
You do use RoleManager as a trait object constraints that require knowing the Size of the trait object is what makes it unsafe. I should have looked around a bit more 🤦♂️.
pub fn generate_g_function(rm: Box<dyn RoleManager>) -> Box<dyn MatchFnClone> So it is probably not worth changing just to remove one method that is perfectly fine. Although if you wanted to this an Rc may work I can try if you want or using dtonlay's crate or even easier objekt but it probably isn't worth it I don't think clone_box is all that much more expensive.
Fix: #7