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

[Bug] WatcherUpdatable interface needs more params #1098

Closed
Rainshaw opened this issue Sep 4, 2022 · 7 comments · Fixed by #1100
Closed

[Bug] WatcherUpdatable interface needs more params #1098

Rainshaw opened this issue Sep 4, 2022 · 7 comments · Fixed by #1100
Assignees

Comments

@Rainshaw
Copy link
Contributor

Rainshaw commented Sep 4, 2022

Want to prioritize this issue? Try:

issuehunt-to-marktext


Describe the bug
now the WathcerUpdatable defines is

// WatcherUpdatable is the strengthen for Casbin watchers.
type WatcherUpdatable interface {
	Watcher
	// UpdateForUpdatePolicy calls the update callback of other instances to synchronize their policy.
	// It is called after Enforcer.UpdatePolicy()
	UpdateForUpdatePolicy(oldRule, newRule []string) error
	// UpdateForUpdatePolicies calls the update callback of other instances to synchronize their policy.
	// It is called after Enforcer.UpdatePolicies()
	UpdateForUpdatePolicies(oldRules, newRules [][]string) error
}

but UpdatableAdapter needs more params: sec and ptype.

// UpdatableAdapter is the interface for Casbin adapters with add update policy function.
type UpdatableAdapter interface {
	Adapter
	// UpdatePolicy updates a policy rule from storage.
	// This is part of the Auto-Save feature.
	UpdatePolicy(sec string, ptype string, oldRule, newPolicy []string) error
	// UpdatePolicies updates some policy rules to storage, like db, redis.
	UpdatePolicies(sec string, ptype string, oldRules, newRules [][]string) error
	// UpdateFilteredPolicies deletes old rules and adds new rules.
	UpdateFilteredPolicies(sec string, ptype string, newPolicies [][]string, fieldIndex int, fieldValues ...string) ([][]string, error)
}

the management api also needs these params:

type IEnforcer interface {
        ...
	SelfUpdatePolicy(sec string, ptype string, oldRule, newRule []string) (bool, error)
	SelfUpdatePolicies(sec string, ptype string, oldRules, newRules [][]string) (bool, error)
}

Should we add the params? I searched around and did not find a watcher repo already implemented this interface, so we may just add the params. If yes, I'd like to submit a pr

@casbin-bot
Copy link
Member

@tangyang9464 @JalinWang

@hsluoyz
Copy link
Member

hsluoyz commented Sep 5, 2022

@JalinWang

/cc @tangyang9464

@JalinWang
Copy link
Member

uhh, I think it's definitely a bug and should be fixed. We need to specify whether p or p2 is updated.

We have no updatable watcher to fix. However, we need to fix it across four repositories.
https://github.com/search?p=2&q=org%3Acasbin+UpdateForUpdatePolicy&type=Code
Should we go? @tangyang9464

@hsluoyz
Copy link
Member

hsluoyz commented Sep 6, 2022

@JalinWang It should be UpdatableWatcher instead of WatcherUpdatable. We can just define a new UpdatableWatcher interface with new APIs. Remove the old interface.

BTW, here are typos, it should be rule instead of policy. Plz fix them

image

@hsluoyz
Copy link
Member

hsluoyz commented Sep 7, 2022

@RainshawGao merged: #1100

Plz also update the related repos as @JalinWang said: https://github.com/search?p=2&q=org%3Acasbin+UpdateForUpdatePolicy&type=Code

@github-actions
Copy link

github-actions bot commented Sep 7, 2022

🎉 This issue has been resolved in version 2.54.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@Rainshaw
Copy link
Contributor Author

Rainshaw commented Sep 7, 2022

@hsluoyz I did not write c++ and lua 😭

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.

4 participants