From 790a49d1662dce6d3f1a2f89b6888893ec987841 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 1 Jan 2026 13:31:04 +0000 Subject: [PATCH 1/6] Initial plan From 1d93707750390ff8850770939ce8a4a59d85fb6e Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 1 Jan 2026 13:36:47 +0000 Subject: [PATCH 2/6] Add documentation requiring adapters to raise errors on failure Co-authored-by: hsluoyz <3787410+hsluoyz@users.noreply.github.com> --- src/persist/Adapter.lua | 15 ++++ src/persist/FilteredAdapter.lua | 3 + tests/persist/error_handling_spec.lua | 112 ++++++++++++++++++++++++++ 3 files changed, 130 insertions(+) create mode 100644 tests/persist/error_handling_spec.lua diff --git a/src/persist/Adapter.lua b/src/persist/Adapter.lua index fc0e74d..08f9cb6 100644 --- a/src/persist/Adapter.lua +++ b/src/persist/Adapter.lua @@ -52,6 +52,9 @@ end * loadPolicy loads all policy rules from the storage. * * @param model the model. + * @raises error if the policy cannot be loaded (e.g., database connection failure). + * Implementations MUST raise an error using error() on failure so that + * callers can catch it with pcall(). ]] function Adapter:loadPolicy(model) @@ -61,6 +64,9 @@ end * savePolicy saves all policy rules to the storage. * * @param model the model. + * @raises error if the policy cannot be saved (e.g., database connection failure). + * Implementations MUST raise an error using error() on failure so that + * callers can catch it with pcall(). ]] function Adapter:savePolicy(model) @@ -73,6 +79,9 @@ end * @param sec the section, "p" or "g". * @param ptype the policy type, "p", "p2", .. or "g", "g2", .. * @param rule the rule, like (sub, obj, act). + * @raises error if the policy cannot be added (e.g., database connection failure). + * Implementations MUST raise an error using error() on failure so that + * callers can catch it with pcall(). ]] function Adapter:addPolicy(sec, ptype, rule) @@ -85,6 +94,9 @@ end * @param sec the section, "p" or "g". * @param ptype the policy type, "p", "p2", .. or "g", "g2", .. * @param rule the rule, like (sub, obj, act). + * @raises error if the policy cannot be removed (e.g., database connection failure). + * Implementations MUST raise an error using error() on failure so that + * callers can catch it with pcall(). ]] function Adapter:removePolicy(sec, ptype, rule) @@ -99,6 +111,9 @@ end * @param fieldIndex the policy rule's start index to be matched. * @param fieldValues the field values to be matched, value "" * means not to match this field. + * @raises error if the policy cannot be removed (e.g., database connection failure). + * Implementations MUST raise an error using error() on failure so that + * callers can catch it with pcall(). ]] function Adapter:removeFilteredPolicy(sec, ptype, fieldIndex, fieldValues) diff --git a/src/persist/FilteredAdapter.lua b/src/persist/FilteredAdapter.lua index b80874d..aef4216 100644 --- a/src/persist/FilteredAdapter.lua +++ b/src/persist/FilteredAdapter.lua @@ -24,6 +24,9 @@ setmetatable(FilteredAdapter, Adapter) * loadFilteredPolicy loads only policy rules that match the filter. * @param model the model. * @param filter the filter used to specify which type of policy should be loaded. + * @raises error if the policy cannot be loaded (e.g., database connection failure). + * Implementations MUST raise an error using error() on failure so that + * callers can catch it with pcall(). ]] function FilteredAdapter:loadFilteredPolicy(model, filter) diff --git a/tests/persist/error_handling_spec.lua b/tests/persist/error_handling_spec.lua new file mode 100644 index 0000000..85f5d5b --- /dev/null +++ b/tests/persist/error_handling_spec.lua @@ -0,0 +1,112 @@ +--Copyright 2021 The casbin Authors. All Rights Reserved. +-- +--Licensed under the Apache License, Version 2.0 (the "License"); +--you may not use this file except in compliance with the License. +--You may obtain a copy of the License at +-- +-- http://www.apache.org/licenses/LICENSE-2.0 +-- +--Unless required by applicable law or agreed to in writing, software +--distributed under the License is distributed on an "AS IS" BASIS, +--WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +--See the License for the specific language governing permissions and +--limitations under the License. + +local Enforcer = require("src.main.Enforcer") +local Adapter = require("src.persist.Adapter") +local path = os.getenv("PWD") or io.popen("cd"):read() + +describe("Error handling tests", function () + it("should catch errors from adapter during enforcer initialization", function () + -- Create a failing adapter that throws an error during loadPolicy + local FailingAdapter = {} + setmetatable(FailingAdapter, Adapter) + FailingAdapter.__index = FailingAdapter + + function FailingAdapter:new() + local o = {} + setmetatable(o, self) + self.__index = self + return o + end + + function FailingAdapter:loadPolicy(model) + error("Database connection failed: authentication failed") + end + + local model = path .. "/examples/rbac_model.conf" + local a = FailingAdapter:new() + + -- Test that the error can be caught with pcall + local ok, err = pcall(function() + local e = Enforcer:new(model, a) + end) + + assert.is.False(ok) + assert.is.truthy(string.find(err, "Database connection failed")) + end) + + it("should catch errors from adapter during explicit loadPolicy call", function () + -- Create an adapter that succeeds initially but fails on explicit loadPolicy + local DelayedFailingAdapter = {} + setmetatable(DelayedFailingAdapter, Adapter) + DelayedFailingAdapter.__index = DelayedFailingAdapter + + function DelayedFailingAdapter:new() + local o = {} + setmetatable(o, self) + self.__index = self + o.isFiltered = true -- Prevent automatic loadPolicy during init + return o + end + + function DelayedFailingAdapter:loadPolicy(model) + error("Database connection failed: wrong password") + end + + local model = path .. "/examples/rbac_model.conf" + local a = DelayedFailingAdapter:new() + + -- Create enforcer (won't load policy due to isFiltered=true) + local e = Enforcer:new(model, a) + + -- Now test that calling loadPolicy explicitly raises an error + local ok, err = pcall(function() + e:loadPolicy() + end) + + assert.is.False(ok) + assert.is.truthy(string.find(err, "Database connection failed")) + end) + + it("should catch errors from adapter during loadFilteredPolicy call", function () + -- Create an adapter that fails during loadFilteredPolicy + local FilteredFailingAdapter = {} + setmetatable(FilteredFailingAdapter, Adapter) + FilteredFailingAdapter.__index = FilteredFailingAdapter + + function FilteredFailingAdapter:new() + local o = {} + setmetatable(o, self) + self.__index = self + o.isFiltered = true + return o + end + + function FilteredFailingAdapter:loadFilteredPolicy(model, filter) + error("Database query failed: connection timeout") + end + + local model = path .. "/examples/rbac_model.conf" + local a = FilteredFailingAdapter:new() + local e = Enforcer:new(model, a) + + -- Test that error from loadFilteredPolicy can be caught + local ok, err = pcall(function() + e:loadFilteredPolicy({}) + end) + + assert.is.False(ok) + assert.is.truthy(string.find(err, "Database query failed")) + end) +end) From 0967a4d4d405cda577b54051412018d0619618fe Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 1 Jan 2026 13:37:44 +0000 Subject: [PATCH 3/6] Add README section on adapter error handling best practices Co-authored-by: hsluoyz <3787410+hsluoyz@users.noreply.github.com> --- README.md | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/README.md b/README.md index ef396da..77429dd 100644 --- a/README.md +++ b/README.md @@ -162,6 +162,29 @@ We also provide a [web-based UI](https://casbin.org/docs/admin-portal) for model https://casbin.org/docs/adapters +### Error Handling for Adapters + +When implementing or using custom adapters (e.g., database adapters for PostgreSQL, MySQL, etc.), it's important to ensure proper error handling: + +**For Adapter Developers:** +- Adapters MUST raise errors using `error()` when operations fail (e.g., database connection failures, authentication errors). +- Do NOT silently log errors without raising them, as this prevents users from catching errors with `pcall()`. +- See the `FileAdapter` implementation for reference - it uses `assert()` which properly raises errors. + +**For Adapter Users:** +- You can catch errors from adapter operations using `pcall()`: +```lua +local ok, err = pcall(function() + local adapter = MyAdapter:new(config) + local enforcer = Enforcer:new("model.conf", adapter) +end) + +if not ok then + print("Error loading policy: " .. tostring(err)) +end +``` +- If errors appear in logs but `pcall()` doesn't catch them, the adapter may not be raising errors properly. Contact the adapter maintainer to fix this. + ## Policy consistence between multiple nodes https://casbin.org/docs/watchers From 24b63adb78a4641d31bcae9fa6b5162c2915d18a Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 1 Jan 2026 13:39:20 +0000 Subject: [PATCH 4/6] Add troubleshooting section for pcall error handling issue Co-authored-by: hsluoyz <3787410+hsluoyz@users.noreply.github.com> --- README.md | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/README.md b/README.md index 77429dd..22f7767 100644 --- a/README.md +++ b/README.md @@ -217,6 +217,22 @@ Authz middlewares for web frameworks: https://casbin.org/docs/middlewares https://casbin.org/docs/adopters +## Troubleshooting + +### Errors not caught by pcall() + +**Problem:** When using an adapter (e.g., PostgreSQL adapter) with incorrect configuration (wrong password, etc.), errors appear in `error.log` but cannot be caught with `pcall()`. + +**Cause:** The adapter is logging errors (e.g., using `ngx.log()` in OpenResty) instead of raising them with `error()`. + +**Solution:** +1. Verify the adapter is implementing the Casbin adapter interface correctly - it should raise errors using `error()` when operations fail +2. If you're using a third-party adapter, report this issue to the adapter maintainer +3. As a workaround, check logs for errors and handle them at the application level +4. Consider switching to an adapter that properly raises errors + +For adapter developers: See the [Policy persistence](#policy-persistence) section for requirements on proper error handling. + ## How to Contribute Please read the [contributing guide](CONTRIBUTING.md). From ece07e0d19833b88059a8390c49eaee52b8088b4 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 1 Jan 2026 13:40:54 +0000 Subject: [PATCH 5/6] Improve error handling tests with assert.has_error pattern Co-authored-by: hsluoyz <3787410+hsluoyz@users.noreply.github.com> --- tests/persist/error_handling_spec.lua | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/tests/persist/error_handling_spec.lua b/tests/persist/error_handling_spec.lua index 85f5d5b..6eaaa89 100644 --- a/tests/persist/error_handling_spec.lua +++ b/tests/persist/error_handling_spec.lua @@ -37,13 +37,19 @@ describe("Error handling tests", function () local model = path .. "/examples/rbac_model.conf" local a = FailingAdapter:new() - -- Test that the error can be caught with pcall + -- Test that the error can be caught with pcall and has the expected message local ok, err = pcall(function() local e = Enforcer:new(model, a) end) assert.is.False(ok) assert.is.truthy(string.find(err, "Database connection failed")) + + -- Also verify using assert.has_error pattern + assert.has_error(function() + local a2 = FailingAdapter:new() + local e2 = Enforcer:new(model, a2) + end, "Database connection failed") end) it("should catch errors from adapter during explicit loadPolicy call", function () @@ -70,13 +76,18 @@ describe("Error handling tests", function () -- Create enforcer (won't load policy due to isFiltered=true) local e = Enforcer:new(model, a) - -- Now test that calling loadPolicy explicitly raises an error + -- Test that calling loadPolicy explicitly raises an error that can be caught local ok, err = pcall(function() e:loadPolicy() end) assert.is.False(ok) assert.is.truthy(string.find(err, "Database connection failed")) + + -- Also verify using assert.has_error pattern + assert.has_error(function() + e:loadPolicy() + end, "wrong password") end) it("should catch errors from adapter during loadFilteredPolicy call", function () @@ -108,5 +119,10 @@ describe("Error handling tests", function () assert.is.False(ok) assert.is.truthy(string.find(err, "Database query failed")) + + -- Also verify using assert.has_error pattern + assert.has_error(function() + e:loadFilteredPolicy({}) + end, "connection timeout") end) end) From 41aed4cc7008ab26ec3c2533d75f07ee161ce787 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 1 Jan 2026 13:42:20 +0000 Subject: [PATCH 6/6] Address code review feedback - improve comments and examples Co-authored-by: hsluoyz <3787410+hsluoyz@users.noreply.github.com> --- README.md | 11 ++++++++++- tests/persist/error_handling_spec.lua | 6 +++++- 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 22f7767..4fb28d6 100644 --- a/README.md +++ b/README.md @@ -169,7 +169,16 @@ When implementing or using custom adapters (e.g., database adapters for PostgreS **For Adapter Developers:** - Adapters MUST raise errors using `error()` when operations fail (e.g., database connection failures, authentication errors). - Do NOT silently log errors without raising them, as this prevents users from catching errors with `pcall()`. -- See the `FileAdapter` implementation for reference - it uses `assert()` which properly raises errors. +- Example of correct error handling: +```lua +function MyAdapter:loadPolicy(model) + local conn, err = connect_to_database(self.config) + if not conn then + error("Database connection failed: " .. tostring(err)) + end + -- ... continue with loading policy +end +``` **For Adapter Users:** - You can catch errors from adapter operations using `pcall()`: diff --git a/tests/persist/error_handling_spec.lua b/tests/persist/error_handling_spec.lua index 6eaaa89..63b291f 100644 --- a/tests/persist/error_handling_spec.lua +++ b/tests/persist/error_handling_spec.lua @@ -62,7 +62,9 @@ describe("Error handling tests", function () local o = {} setmetatable(o, self) self.__index = self - o.isFiltered = true -- Prevent automatic loadPolicy during init + -- Set isFiltered = true to prevent automatic loadPolicy during Enforcer initialization. + -- This allows us to test explicit loadPolicy() calls separately. + o.isFiltered = true return o end @@ -100,6 +102,8 @@ describe("Error handling tests", function () local o = {} setmetatable(o, self) self.__index = self + -- Set isFiltered = true to prevent automatic loadPolicy during Enforcer initialization. + -- This allows us to test loadFilteredPolicy() calls independently. o.isFiltered = true return o end