Copilot commented on code in PR #5:
URL:
https://github.com/apache/casbin-python-django-orm-adapter/pull/5#discussion_r2936744475
##########
casbin_adapter/enforcer.py:
##########
@@ -43,9 +43,9 @@ def _load(self):
self.set_role_manager(role_manager)
def __getattribute__(self, name):
- safe_methods = ["__init__", "_load", "_initialized"]
+ safe_methods = ["__init__", "_load", "_initialized", "__class__"]
Review Comment:
Accessing `enforcer.db_alias` while `_initialized` is false will currently
trigger `initialize_enforcer(...)` (DB access) and may raise the "registry is
ready" exception, even though callers might only be trying to read
configuration. Add `"db_alias"` (and potentially other harmless introspection
attrs) to `safe_methods` so reading the alias does not force initialization.
##########
casbin_adapter/enforcer.py:
##########
@@ -43,9 +43,9 @@ def _load(self):
self.set_role_manager(role_manager)
def __getattribute__(self, name):
- safe_methods = ["__init__", "_load", "_initialized"]
+ safe_methods = ["__init__", "_load", "_initialized", "__class__"]
if not super().__getattribute__("_initialized") and name not in
safe_methods:
- initialize_enforcer(self.db_alias)
+ initialize_enforcer(super().__getattribute__("db_alias"))
if not super().__getattribute__("_initialized"):
Review Comment:
There are no tests covering the updated lazy-init / recursion-avoidance
behavior in `ProxyEnforcer` (e.g., `__class__` access not triggering
initialization, and no recursion when initializing). Add targeted tests to
prevent regressions, since this logic is sensitive to Django
startup/test-discovery behavior.
##########
casbin_adapter/enforcer.py:
##########
@@ -12,7 +12,7 @@
class ProxyEnforcer(Enforcer):
_initialized = False
- db_alias = "default"
+ db_alias = getattr(settings, "CASBIN_DB_ALIAS", "default")
Review Comment:
`db_alias` is being read from `django.conf.settings` at
import/class-definition time. Accessing `settings` before Django is configured
(e.g., importing this module outside a fully-initialized Django context or
before `django.setup()`) can raise `ImproperlyConfigured`, and it also
undermines the goal of deferring initialization. Consider keeping the class
attribute as a plain default (e.g., "default") and reading `CASBIN_DB_ALIAS`
lazily when needed (inside `_load()` / `__getattribute__()` /
`initialize_enforcer`).
##########
casbin_adapter/apps.py:
##########
@@ -4,9 +4,3 @@
class CasbinAdapterConfig(AppConfig):
name = "casbin_adapter"
Review Comment:
After removing `ready()`, `from django.conf import settings` is now unused
in this module. Please remove the unused import to avoid lint/quality issues.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
