Skip to content

[feature] Add policy filtering#78

Merged
hsluoyz merged 2 commits intoapache:masterfrom
faceless-saint:master
Mar 16, 2018
Merged

[feature] Add policy filtering#78
hsluoyz merged 2 commits intoapache:masterfrom
faceless-saint:master

Conversation

@faceless-saint
Copy link
Contributor

@faceless-saint faceless-saint commented Mar 14, 2018

A new interface, FilteredAdapter, extends the Adapter interface
with the ability to load a filtered subset of the back-end policy.
This allows Casbin to more effectively scale when enforcing a very
large number of policies, such as a busy multi-tenant system. There
is also a second built-in file adapter supporting this feature.

To prevent accidental data loss, the SavePolicy method is disabled
when a filtered policy is loaded. For maximum compatibility, whether
a given Adapter implements the new feature is checked at runtime.

@coveralls
Copy link

coveralls commented Mar 14, 2018

Coverage Status

Coverage increased (+0.03%) to 84.073% when pulling 0f4a37f on faceless-saint:master into f418a84 on casbin:master.

@hsluoyz
Copy link
Member

hsluoyz commented Mar 15, 2018

  1. For the 2nd commit, please add another adapter for this filtered file adapter here: persist/filtered-file-adapter/adapter.go. I want the default file adapter to be simple and friendly for new users. I know this filtered file adapter is compatible with the original one. But our users use file adapter as an example to implement their own adapter. The filtering code will complicate their understanding. You can implement a filtered adapter, then make it a subclass of the original one to remove the duplicated code, just add your own functions and structs.

  2. The 2nd commit has some code style re-formatting. If possible, please submit the code style change in another commit. The current commit makes me difficult to see where the real change is.

  3. Please fix the codeclimate and coverage/coveralls downgrade.

@faceless-saint faceless-saint force-pushed the master branch 5 times, most recently from 378a457 to ef1881b Compare March 15, 2018 16:15
for {
line, err := buf.ReadString('\n')
line = strings.TrimSpace(line)
scanner := bufio.NewScanner(f)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I took the opportunity to integrate my streamlined handler into the base file adapter, as it will be simpler for others to use as a reference IMO.

@faceless-saint faceless-saint force-pushed the master branch 4 times, most recently from ff40def to 4232862 Compare March 15, 2018 16:42
A new interface, `FilteredAdapter`, extends the `Adapter` interface
with the ability to load a filtered subset of the backend policy.
This allows Casbin to more effectively scale when enforcing a very
large number of policies, such as a busy multi-tenant system. There
is also a second built-in file adapter supporting this feature.

To prevent accidental data loss, the `SavePolicy` method is disabled
when a filtered policy is loaded. For maximum compatibility, whether
a given `Adapter` implements the new feature is checked at runtime.
@faceless-saint
Copy link
Contributor Author

faceless-saint commented Mar 15, 2018

@hsluoyz

  1. DONE - the fileadapter package has a new struct, FilteredAdapter, which implements the new interface. This is implemented cleanly in a separate file, and should provide a useful reference for new filtered adapter implementations in the same way that the existing file adapter is.

  2. DONE - all style edits from go fmt are now contained in a separate commit without functional changes.

  3. DONE - refactored new code to satisfy codeclimate constraints and added additional tests to satisfy coverage/coveralls. The refactored line scanning, using a bufio.Scanner instead of a bufio.Reader, was also backported to the appropriate fileadapter.Adapter method to further reduce cognitive complexity.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants