Add a Django example and setup.py #3
Conversation
|
It can be viewed in this repository before merging @gerbyzation |
|
@LiuG-lynx we can review your code here in this PR. This is why we have Pull Request :) I just want to make sure the time that you make it ready for review. |
|
@hsluoyz Please refer to the link text to set the username and password, and trigger the github action when a new release is built. Since I am not sure how to create a fake postgresql connection, I did not verify the unit test on the github action, only verified it locally. |
|
@gerbyzation @techoner plz review. |
|
@LiuG-lynx Can you squash your commits into one? |
add django-casbin with postgresql-watcher examples
There was a problem hiding this comment.
@LiuG-lynx thanks for the effort you're putting in. Unfortunately the tests fail for me when running locally (see my other comment for the error). I've had a look through the code and left some first reactions.
I think something that would be worth doing is improving the testing a bit further, would be great to have more of it's functionality tested and will help in getting this working. I added this to test if two enforcers would sync (needs proper teardown of connections/processes):
import time
import casbin
import casbin_sqlalchemy_adapter
class TestFunctionality(unittest.TestCase):
def setUp(self):
adapter = casbin_sqlalchemy_adapter.Adapter(
"postgres://postgres:postgres@localhost:5432/postgres"
)
enforcer = casbin.Enforcer("examples/rbac_model.conf", adapter)
enforcer.remove_policy("group1", "target3", "read")
def test_enforcers_sync(self):
adapter = casbin_sqlalchemy_adapter.Adapter(
"postgres://postgres:postgres@localhost:5432/postgres"
)
enforcer_1 = casbin.Enforcer("examples/rbac_model.conf", adapter)
watcher_1 = get_watcher()
watcher_1.set_update_callback(enforcer_1.load_policy)
enforcer_1.set_watcher(watcher_1)
adapter_2 = casbin_sqlalchemy_adapter.Adapter(
"postgres://postgres:postgres@localhost:5432/postgres"
)
enforcer_2 = casbin.Enforcer("examples/rbac_model.conf", adapter_2)
watcher_2 = get_watcher()
watcher_2.set_update_callback(enforcer_2.load_policy)
enforcer_2.set_watcher(watcher_2)
enforcer_1.add_policy("group1", "target3", "read")
assert enforcer_1.enforce("group1", "target3", "read") is True
# give it a sec to reconcile
time.sleep(1)
assert enforcer_2.enforce("group1", "target3", "read") is True
This test fails for me at the moment, it's bedtime here now so I can look into why it doesn't sync yet another time if needed.
I think we can make the examples clearer. At the moment it explains how to integrate with flask and django which I think add noise as integration (generally) happends with the enforcer and not with the web server. The django example uses CSV as model storage, sqlite for the Django ORM and this package and postgres to sync - not easy to follow what is going on. I'd suggest using a simple setup for the primary examples (casbin_sqlalchemy_adapter for example) and if needed provide documentation for integration with web framework/casbin integration packages in a less prominent place.
For spinning up a postgres database locally you can use docker:
docker run --name casbin-watcher-postgres -p 5432:5432 -e POSTGRES_PASSWORD=postgres -d postgres
I can help with setting up postgres with docker in Github Actions.
|
|
||
|
|
||
|
|
||
| class PostgresqlWatcher(object): |
There was a problem hiding this comment.
I think this is supposed to extend the ABC defined above?
| ```python | ||
| import casbin | ||
|
|
||
| def __init__(self, get_response): |
There was a problem hiding this comment.
Looks like this is missing the class definition
|
|
||
|
|
||
| def casbin_subscription( | ||
| process_conn:Any, |
There was a problem hiding this comment.
would be nice to have a type here.
| from psycopg2 import connect, extensions | ||
| from multiprocessing import Process, Pipe | ||
| import time | ||
| from select import select |
There was a problem hiding this comment.
Where does this come from, I don't see it listed in requirements?
| while conn.notifies: | ||
| notify = conn.notifies.pop(0) | ||
| print(f"Notify: {notify.payload}") | ||
|
|
There was a problem hiding this comment.
This doesn't seem to call anything when it's notified of a change?
| "Child casbin-watcher subscribe process has stopped, " | ||
| "attempting to recreate the process in 10 seconds..." | ||
| ) | ||
| self.subscribed_process, self.parent_conn = self.create_subscriber_process( |
There was a problem hiding this comment.
this is creating the subscriber process twice (first time on line 88)?
|
|
||
| class TestConfig(unittest.TestCase): | ||
| def test_pg_watcher_init(self): | ||
| assert isinstance(pg_watcher.parent_conn, connection.PipeConnection) |
There was a problem hiding this comment.
Based on the unittest output I think this should be connection.Connection
======================================================================
ERROR: test_pg_watcher_init (__main__.TestConfig)
----------------------------------------------------------------------
Traceback (most recent call last):
File "tests/test_postgresql_watcher.py", line 26, in test_pg_watcher_init
assert isinstance(pg_watcher.parent_conn, connection.PipeConnection)
AttributeError: module 'multiprocessing.connection' has no attribute 'PipeConnection'
|
@LiuG-lynx plz resolve the conflicts and respond to reviews.
|
|
@LiuG-lynx any progress on this? |
|
@Zxilly he already quited. So no reply would be expected. Can you borrow the useful code in this PR and make a new PR instead? |
I've added examples of the Django framework to this repository, check out the README to learn more.There is currently no packaged version.It can only be used as a local py file @gerbyzation
I am not sure how to package this Repository currently. I have added setup.py but it has not been tested. Where can I get help @hsluoyz