This is an automated email from the ASF dual-hosted git repository. hsluoyz pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/casbin-lua-casbin.git
commit 4a8b6df631f5e696b6ea40c643455815e7909c8e Author: Yang Luo <[email protected]> AuthorDate: Thu Aug 6 18:15:17 2026 +0800 fix: propagate adapter errors instead of silently ignoring them --- src/main/CoreEnforcer.lua | 23 +++++- src/main/InternalEnforcer.lua | 131 ++++++++++++++++++----------------- src/persist/Adapter.lua | 5 ++ tests/persist/adapter_error_spec.lua | 104 +++++++++++++++++++++++++++ 4 files changed, 197 insertions(+), 66 deletions(-) diff --git a/src/main/CoreEnforcer.lua b/src/main/CoreEnforcer.lua index c8b5aa8..659a9b9 100644 --- a/src/main/CoreEnforcer.lua +++ b/src/main/CoreEnforcer.lua @@ -25,6 +25,23 @@ local Util = require("src.util.Util") local luaxp = require("modules.luaxp") +--[[ + * checkAdapterResult raises a Lua error when an adapter reports a failure + * through the "return nil/false, err" convention instead of raising it. + * Without this, storage errors (e.g. a database that is down) would be + * silently ignored and the enforcer would keep running on an empty policy. + * + * @param op the name of the adapter operation, used in the error message. + * @param ok the first value returned by the adapter. + * @param err the second value returned by the adapter. +]] +local function checkAdapterResult(op, ok, err) + -- Adapters that return nothing (like FileAdapter) are considered successful. + if ok == false or (ok == nil and err ~= nil) then + error(op .. " failed: " .. tostring(err or "unknown error")) + end +end + local CoreEnforcer = { enabled = false, autoSave = false, @@ -244,7 +261,7 @@ end ]] function CoreEnforcer:loadPolicy() self.model:clearPolicy() - self.adapter:loadPolicy(self.model) + checkAdapterResult("loadPolicy", self.adapter:loadPolicy(self.model)) self.model:sortPoliciesByPriority() self.model:printPolicy() @@ -262,7 +279,7 @@ end function CoreEnforcer:loadFilteredPolicy(filter) self.model:clearPolicy() - self.adapter:loadFilteredPolicy(self.model, filter) + checkAdapterResult("loadFilteredPolicy", self.adapter:loadFilteredPolicy(self.model, filter)) self:initBuildRoleLinks() self.model:printPolicy() @@ -289,7 +306,7 @@ function CoreEnforcer:savePolicy() error("cannot save a filtered policy") end - self.adapter:savePolicy(self.model) + checkAdapterResult("savePolicy", self.adapter:savePolicy(self.model)) if self.watcher then if self.watcher.updateForSavePolicy then diff --git a/src/main/InternalEnforcer.lua b/src/main/InternalEnforcer.lua index 00a0e1d..89968f2 100644 --- a/src/main/InternalEnforcer.lua +++ b/src/main/InternalEnforcer.lua @@ -14,6 +14,35 @@ local CoreEnforcer = require("src/main/CoreEnforcer") +--[[ + * tryCall calls fn, usually an adapter operation, and normalizes the way a + * failure is reported: an adapter may either raise a Lua error or return + * "false, err", and both must reach the caller instead of being dropped. + * + * @param fn the function to call. + * @return true on success, or false plus the error message on failure. + * Operations that the adapter does not implement count as success, + * because persistence of them is optional. +]] +local function tryCall(fn) + local status, ok, err = pcall(fn) + + if status == false then + local message = tostring(ok) + if string.sub(message, -15) == "not implemented" then + -- log, continue + return true + end + return false, message + end + + if ok == false or (ok == nil and err ~= nil) then + return false, tostring(err or "unknown error") + end + + return true +end + -- InternalEnforcer = CoreEnforcer + Internal API. local InternalEnforcer = {} setmetatable(InternalEnforcer, CoreEnforcer) @@ -38,11 +67,9 @@ function InternalEnforcer:addPolicy(sec, ptype, rule) if self:shouldPersist() then - local status, err = pcall(function () self.adapter:addPolicy(sec, ptype, rule) end) - if status == false and string.sub(err, -15) == "not implemented" then - -- log, continue - elseif status == false then - return false + local ok, err = tryCall(function () return self.adapter:addPolicy(sec, ptype, rule) end) + if not ok then + return false, err end end @@ -80,15 +107,13 @@ function InternalEnforcer:addPolicies(sec, ptype, rules) if self:shouldPersist() then - local status, err = pcall(function () + local ok, err = tryCall(function () if self.adapter.addPolicies then - self.adapter:addPolicies(sec, ptype, rules) + return self.adapter:addPolicies(sec, ptype, rules) end end) - if status == false and string.sub(err, -15) == "not implemented" then - -- log, continue - elseif status == false then - return false + if not ok then + return false, err end end @@ -130,11 +155,9 @@ function InternalEnforcer:removePolicy(sec, ptype, rule) if self:shouldPersist() then - local status, err = pcall(function () self.adapter:removePolicy(sec, ptype, rule) end) - if status == false and string.sub(err, -15) == "not implemented" then - -- log, continue - elseif status == false then - return false + local ok, err = tryCall(function () return self.adapter:removePolicy(sec, ptype, rule) end) + if not ok then + return false, err end end @@ -177,11 +200,9 @@ function InternalEnforcer:updatePolicy(sec, ptype, oldRule, newRule) if self:shouldPersist() then - local status, err = pcall(function () self.adapter:updatePolicy(sec, ptype, oldRule, newRule) end) - if status == false and string.sub(err, -15) == "not implemented" then - -- log, continue - elseif status == false then - return false + local ok, err = tryCall(function () return self.adapter:updatePolicy(sec, ptype, oldRule, newRule) end) + if not ok then + return false, err end end @@ -192,26 +213,22 @@ function InternalEnforcer:updatePolicy(sec, ptype, oldRule, newRule) end if sec == "g" then - local status, err = pcall(function () + local ok, err = tryCall(function () local oldRules = {} table.insert(oldRules, oldRule) self:buildIncrementalRoleLinks(self.model.PolicyOperations.POLICY_REMOVE, ptype, oldRules) end) - if status == false and string.sub(err, -15) == "not implemented" then - -- log, continue - elseif status == false then - return false + if not ok then + return false, err end - - status, err = pcall(function () + + ok, err = tryCall(function () local newRules = {} table.insert(newRules, newRule) self:buildIncrementalRoleLinks(self.model.PolicyOperations.POLICY_ADD, ptype, newRules) end) - if status == false and string.sub(err, -15) == "not implemented" then - -- log, continue - elseif status == false then - return false + if not ok then + return false, err end end @@ -234,11 +251,9 @@ function InternalEnforcer:updatePolicies(sec, ptype, oldRules, newRules) if self:shouldPersist() then - local status, err = pcall(function () self.adapter:updatePolicies(sec, ptype, oldRules, newRules) end) - if status == false and string.sub(err, -15) == "not implemented" then - -- log, continue - elseif status == false then - return false + local ok, err = tryCall(function () return self.adapter:updatePolicies(sec, ptype, oldRules, newRules) end) + if not ok then + return false, err end end @@ -249,22 +264,18 @@ function InternalEnforcer:updatePolicies(sec, ptype, oldRules, newRules) end if sec == "g" then - local status, err = pcall(function () + local ok, err = tryCall(function () self:buildIncrementalRoleLinks(self.model.PolicyOperations.POLICY_REMOVE, ptype, oldRules) end) - if status == false and string.sub(err, -15) == "not implemented" then - -- log, continue - elseif status == false then - return false + if not ok then + return false, err end - status, err = pcall(function () + ok, err = tryCall(function () self:buildIncrementalRoleLinks(self.model.PolicyOperations.POLICY_ADD, ptype, newRules) end) - if status == false and string.sub(err, -15) == "not implemented" then - -- log, continue - elseif status == false then - return false + if not ok then + return false, err end end @@ -294,15 +305,13 @@ function InternalEnforcer:removePolicies(sec, ptype, rules) if self:shouldPersist() then - local status, err = pcall(function () + local ok, err = tryCall(function () if self.adapter.removePolicies then - self.adapter:removePolicies(sec, ptype, rules) + return self.adapter:removePolicies(sec, ptype, rules) end end) - if status == false and string.sub(err, -15) == "not implemented" then - -- log, continue - elseif status == false then - return false + if not ok then + return false, err end end @@ -342,11 +351,9 @@ function InternalEnforcer:removeFilteredPolicy(sec, ptype, fieldIndex, fieldValu if self:shouldPersist() then - local status, err = pcall(function () self.adapter:removeFilteredPolicy(sec, ptype, fieldIndex, fieldValues) end) - if status == false and string.sub(err, -15) == "not implemented" then - -- log, continue - elseif status == false then - return false + local ok, err = tryCall(function () return self.adapter:removeFilteredPolicy(sec, ptype, fieldIndex, fieldValues) end) + if not ok then + return false, err end end @@ -379,11 +386,9 @@ function InternalEnforcer:updateFilteredPolicies(sec, ptype, newRules, fieldInde local oldRules = self.model:getFilteredPolicy(sec, ptype, fieldIndex, fieldValues) if self:shouldPersist() then - local status, err = pcall(function () self.adapter:updateFilteredPolicies(sec, ptype, newRules, fieldIndex, fieldValues) end) - if status == false and string.sub(err, -15) == "not implemented" then - -- log, continue - elseif status == false then - return false + local ok, err = tryCall(function () return self.adapter:updateFilteredPolicies(sec, ptype, newRules, fieldIndex, fieldValues) end) + if not ok then + return false, err end end diff --git a/src/persist/Adapter.lua b/src/persist/Adapter.lua index fc0e74d..ee44aff 100644 --- a/src/persist/Adapter.lua +++ b/src/persist/Adapter.lua @@ -51,6 +51,11 @@ end --[[ * loadPolicy loads all policy rules from the storage. * + * An adapter reports a failure either by raising an error with error() + * or by returning false plus an error message. Casbin turns the latter + * into an error() as well, so callers can always catch storage failures + * with pcall() instead of silently getting an empty policy. + * * @param model the model. ]] function Adapter:loadPolicy(model) diff --git a/tests/persist/adapter_error_spec.lua b/tests/persist/adapter_error_spec.lua new file mode 100644 index 0000000..5f6e717 --- /dev/null +++ b/tests/persist/adapter_error_spec.lua @@ -0,0 +1,104 @@ +--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 Adapter = require("src.persist.Adapter") +local Enforcer = require("src.main.Enforcer") + +local path = os.getenv("PWD") or io.popen("cd"):read() +local model_path = path .. "/examples/basic_model.conf" +local policy_path = path .. "/examples/basic_policy.csv" + +-- FailingAdapter reports failures the way most database adapters do: +-- by returning false plus an error message instead of raising an error. +local function FailingAdapter(failures) + local a = {} + setmetatable(a, Adapter) + + function a:loadPolicy(model) + if failures.loadPolicy then + return false, "connection to the database failed" + end + end + + function a:savePolicy(model) + if failures.savePolicy then + return false, "the database is read only" + end + end + + function a:addPolicy(sec, ptype, rule) + if failures.addPolicy then + return false, "duplicate key value violates unique constraint" + end + return true + end + + return a +end + +describe("adapter error tests", function () + + it("test loadPolicy error is catchable with pcall", function () + local ok, err = pcall(function () + Enforcer:new(model_path, FailingAdapter({loadPolicy = true})) + end) + + assert.is.False(ok) + assert.is.truthy(string.find(err, "connection to the database failed", 1, true)) + end) + + it("test loadPolicy error of an explicit loadPolicy() call", function () + local e = Enforcer:new(model_path, FailingAdapter({})) + e.adapter = FailingAdapter({loadPolicy = true}) + + local ok, err = pcall(function () e:loadPolicy() end) + + assert.is.False(ok) + assert.is.truthy(string.find(err, "connection to the database failed", 1, true)) + end) + + it("test savePolicy error is catchable with pcall", function () + local e = Enforcer:new(model_path, FailingAdapter({savePolicy = true})) + + local ok, err = pcall(function () e:savePolicy() end) + + assert.is.False(ok) + assert.is.truthy(string.find(err, "the database is read only", 1, true)) + end) + + it("test addPolicy returns the adapter error", function () + local e = Enforcer:new(model_path, FailingAdapter({addPolicy = true})) + + local ok, err = e:AddPolicy("alice", "data1", "read") + + assert.is.False(ok) + assert.is.truthy(string.find(err, "duplicate key value violates unique constraint", 1, true)) + -- the rule must not be added to the model when storage rejected it + assert.is.False(e:HasPolicy("alice", "data1", "read")) + end) + + it("test a successful adapter is not reported as an error", function () + local e = Enforcer:new(model_path, FailingAdapter({})) + + assert.is.True(e:AddPolicy("alice", "data1", "read")) + e:savePolicy() + end) + + -- adapters that return nothing at all (like FileAdapter) keep working + it("test file adapter still loads", function () + local e = Enforcer:new(model_path, policy_path) + + assert.is.True(e:enforce("alice", "data1", "read")) + end) +end) --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
