fix(adapter): correct removePolicies semantics and filtering; align MemoryAdapter load/save with model

zamderax · zamderax · commit 8e7a510d1588 · 2025-10-16T00:05:58.000+02:00
- Make removePolicies atomic: return true only if all rules exist; otherwise do not remove any
- Fix inverted existence check and remove early return
- Mirror semantics in DefaultModel.removePolicies
- MemoryAdapter.savePolicy: write explicit section ('p'/'g') instead of iterating ptype characters
- MemoryAdapter.loadPolicy/loadFilteredPolicy: drop [sec, ptype] headers; append only rule fields
- Fix loadFilteredPolicy indexing to match dropped headers
- Tests: re-enable async removePolicies test; update expectations to no-ptype rules
diff --git a/Sources/Casbin/Adapter/MemoryAdapter.swift b/Sources/Casbin/Adapter/MemoryAdapter.swift
@@ -31,7 +31,8 @@ extension MemoryAdapter: Adapter {
         for line in policy {
             let sec = line[0]
             let ptype = line[1]
-            let rule = Array(line[1...])
+            // Model policies should NOT include ptype; drop sec and ptype headers
+            let rule = Array(line.dropFirst(2))
             if let ast = m.getModel()[sec]?[ptype] {
                 ast.policy.append(rule)
             }
@@ -44,7 +45,7 @@ extension MemoryAdapter: Adapter {
         for line in policy {
             let sec = line[0]
             let ptype = line[1]
-            let rule = Array(line[1...])
+            let rule = Array(line.dropFirst(2))
             if let ast = m.getModel()[sec]?[ptype] {
                 ast.policy.append(rule)
             }
@@ -55,18 +56,18 @@ extension MemoryAdapter: Adapter {
         for line in policy {
             let sec = line[0]
             let ptype = line[1]
-            let rule = Array(line[1...])
+            let rule = Array(line.dropFirst(2))
             var isFiltered = false
             if sec == "p" {
                 for (i,r) in f.p.enumerated() {
-                    if !r.isEmpty && r != rule[i+1] {
+                    if !r.isEmpty && r != rule[i] {
                         isFiltered = true
                     }
                 }
             }
             if sec == "g" {
                 for (i,r) in f.g.enumerated() {
-                    if !r.isEmpty && r != rule[i+1] {
+                    if !r.isEmpty && r != rule[i] {
                         isFiltered = true
                     }
                 }
@@ -87,18 +88,18 @@ extension MemoryAdapter: Adapter {
         for line in policy {
             let sec = line[0]
             let ptype = line[1]
-            let rule = Array(line[1...])
+            let rule = Array(line.dropFirst(2))
             var isFiltered = false
             if sec == "p" {
                 for (i,r) in f.p.enumerated() {
-                    if !r.isEmpty && r != rule[i+1] {
+                    if !r.isEmpty && r != rule[i] {
                         isFiltered = true
                     }
                 }
             }
             if sec == "g" {
                 for (i,r) in f.g.enumerated() {
-                    if !r.isEmpty && r != rule[i+1] {
+                    if !r.isEmpty && r != rule[i] {
                         isFiltered = true
                     }
                 }
@@ -117,25 +118,21 @@ extension MemoryAdapter: Adapter {
         self.policy = []
         if let astMap = m.getModel()["p"] {
             for (ptype,ast) in astMap {
-                ptype.forEach { sec in
-                    for policy in ast.policy {
-                        var rule = policy
-                        rule.insert(ptype, at: 0)
-                        rule.insert(String(sec), at: 0)
-                        self.policy.insert(rule)
-                    }
+                for policy in ast.policy {
+                    var rule = policy
+                    rule.insert(ptype, at: 0)
+                    rule.insert("p", at: 0)
+                    self.policy.insert(rule)
                 }
             }
         }
         if let astMap = m.getModel()["g"] {
             for (ptype,ast) in astMap {
-                ptype.forEach { sec in
-                    for policy in ast.policy {
-                        var rule = policy
-                        rule.insert(ptype, at: 0)
-                        rule.insert(String(sec), at: 0)
-                        self.policy.insert(rule)
-                    }
+                for policy in ast.policy {
+                    var rule = policy
+                    rule.insert(ptype, at: 0)
+                    rule.insert("g", at: 0)
+                    self.policy.insert(rule)
                 }
             }
         }
@@ -147,25 +144,21 @@ extension MemoryAdapter: Adapter {
         self.policy = []
         if let astMap = m.getModel()["p"] {
             for (ptype,ast) in astMap {
-                ptype.forEach { sec in
-                    for policy in ast.policy {
-                        var rule = policy
-                        rule.insert(ptype, at: 0)
-                        rule.insert(String(sec), at: 0)
-                        self.policy.insert(rule)
-                    }
+                for policy in ast.policy {
+                    var rule = policy
+                    rule.insert(ptype, at: 0)
+                    rule.insert("p", at: 0)
+                    self.policy.insert(rule)
                 }
             }
         }
         if let astMap = m.getModel()["g"] {
             for (ptype,ast) in astMap {
-                ptype.forEach { sec in
-                    for policy in ast.policy {
-                        var rule = policy
-                        rule.insert(ptype, at: 0)
-                        rule.insert(String(sec), at: 0)
-                        self.policy.insert(rule)
-                    }
+                for policy in ast.policy {
+                    var rule = policy
+                    rule.insert(ptype, at: 0)
+                    rule.insert("g", at: 0)
+                    self.policy.insert(rule)
                 }
             }
         }
@@ -258,14 +251,17 @@ extension MemoryAdapter: Adapter {
             rule.insert(sec, at: 0)
             return rule
         }
+        // Atomic semantics: if any rule doesn't exist, do not remove any and return false.
         for rule in rules {
-            if policy.contains(rule) {
+            if !policy.contains(rule) {
                 allRemoved = false
-                return eventloop.makeSucceededFuture(allRemoved)
+                break
             }
         }
-        for rule in rules {
-            self.policy.remove(rule)
+        if allRemoved {
+            for rule in rules {
+                _ = self.policy.remove(rule)
+            }
         }
         return eventloop.makeSucceededFuture(allRemoved)
     }
@@ -279,14 +275,17 @@ extension MemoryAdapter: Adapter {
             rule.insert(sec, at: 0)
             return rule
         }
+        // Atomic semantics
         for rule in rules {
-            if policy.contains(rule) {
+            if !policy.contains(rule) {
                 allRemoved = false
-                return allRemoved
+                break
             }
         }
-        for rule in rules {
-            self.policy.remove(rule)
+        if allRemoved {
+            for rule in rules {
+                _ = self.policy.remove(rule)
+            }
         }
         return allRemoved
     }
diff --git a/Sources/Casbin/Model/DefaultModel.swift b/Sources/Casbin/Model/DefaultModel.swift
@@ -223,14 +223,17 @@ extension DefaultModel:Model {
     public func removePolicies(sec:String,ptype:String,rules:[[String]]) -> Bool {
         var allRemoved = true
         if let ast = model[sec]?[ptype] {
+            // Atomic semantics: if any rule is missing, do nothing and return false
             for rule in rules {
-                if ast.policy.contains(rule) {
+                if !ast.policy.contains(rule) {
                     allRemoved = false
-                    return allRemoved
+                    break
                 }
             }
-            for rule in rules {
-                ast.policy.removeAll { $0 == rule }
+            if allRemoved {
+                for rule in rules {
+                    ast.policy.removeAll { $0 == rule }
+                }
             }
         }
         return allRemoved
diff --git a/Tests/CasbinTests/MemoryAdapterAsyncTests.swift b/Tests/CasbinTests/MemoryAdapterAsyncTests.swift
@@ -36,8 +36,8 @@ struct MemoryAdapterAsyncTests {
             Issue.record("Policy not found in model")
             return
         }
-        // Note: policy array includes ptype as first element
-        #expect(ast.policy.contains(["p", "alice", "data1", "read"]))
+        // Model policies do NOT include ptype; only the rule fields
+        #expect(ast.policy.contains(["alice", "data1", "read"]))
     }
 
     @Test("async addPolicies and savePolicy")
@@ -97,7 +97,7 @@ struct MemoryAdapterAsyncTests {
         #expect(ast.policy.isEmpty)
     }
 
-    @Test("async removePolicies", .disabled("Pre-existing bug in removePolicies logic - checks if policy.contains() instead of !policy.contains()"))
+    @Test("async removePolicies")
     func testAsyncRemovePolicies() async throws {
         let adapter = makeAdapter()
         let model = makeModel()
@@ -113,9 +113,9 @@ struct MemoryAdapterAsyncTests {
         // Note: removePolicies has a bug - it checks if policies exist and returns false
         // This is pre-existing behavior in the original implementation
         let removed = try await adapter.removePolicies(sec: "p", ptype: "p", rules: rules)
+        #expect(removed == true)
 
-        // Due to the bug, removed will be false even though policies were removed
-        // Let's just verify the policies are actually gone
+        // Verify the policies are actually gone
         try await adapter.loadPolicy(m: model)
         guard let ast = model.getModel()["p"]?["p"] else {
             // No policies - this is expected
@@ -172,8 +172,7 @@ struct MemoryAdapterAsyncTests {
             return
         }
         #expect(ast.policy.count == 1)
-        // Note: policy array includes ptype as first element
-        #expect(ast.policy.contains(["p", "bob", "data1", "read"]))
+        #expect(ast.policy.contains(["bob", "data1", "read"]))
     }
 
     @Test("async loadFilteredPolicy")
diff --git a/scripts/run-tests.sh b/scripts/run-tests.sh

Original file line number	Diff line number	Diff line change
`@@ -31,7 +31,8 @@ extension MemoryAdapter: Adapter {`
`31`	`31`	`for line in policy {`
`32`	`32`	`let sec = line[0]`
`33`	`33`	`let ptype = line[1]`
`34`		`- let rule = Array(line[1...])`
	`34`	`+ // Model policies should NOT include ptype; drop sec and ptype headers`
	`35`	`+ let rule = Array(line.dropFirst(2))`
`35`	`36`	`if let ast = m.getModel()[sec]?[ptype] {`
`36`	`37`	`ast.policy.append(rule)`
`37`	`38`	`}`
`@@ -44,7 +45,7 @@ extension MemoryAdapter: Adapter {`
`44`	`45`	`for line in policy {`
`45`	`46`	`let sec = line[0]`
`46`	`47`	`let ptype = line[1]`
`47`		`- let rule = Array(line[1...])`
	`48`	`+ let rule = Array(line.dropFirst(2))`
`48`	`49`	`if let ast = m.getModel()[sec]?[ptype] {`
`49`	`50`	`ast.policy.append(rule)`
`50`	`51`	`}`
`@@ -55,18 +56,18 @@ extension MemoryAdapter: Adapter {`
`55`	`56`	`for line in policy {`
`56`	`57`	`let sec = line[0]`
`57`	`58`	`let ptype = line[1]`
`58`		`- let rule = Array(line[1...])`
	`59`	`+ let rule = Array(line.dropFirst(2))`
`59`	`60`	`var isFiltered = false`
`60`	`61`	`if sec == "p" {`
`61`	`62`	`for (i,r) in f.p.enumerated() {`
`62`		`- if !r.isEmpty && r != rule[i+1] {`
	`63`	`+ if !r.isEmpty && r != rule[i] {`
`63`	`64`	`isFiltered = true`
`64`	`65`	`}`
`65`	`66`	`}`
`66`	`67`	`}`
`67`	`68`	`if sec == "g" {`
`68`	`69`	`for (i,r) in f.g.enumerated() {`
`69`		`- if !r.isEmpty && r != rule[i+1] {`
	`70`	`+ if !r.isEmpty && r != rule[i] {`
`70`	`71`	`isFiltered = true`
`71`	`72`	`}`
`72`	`73`	`}`
`@@ -87,18 +88,18 @@ extension MemoryAdapter: Adapter {`
`87`	`88`	`for line in policy {`
`88`	`89`	`let sec = line[0]`
`89`	`90`	`let ptype = line[1]`
`90`		`- let rule = Array(line[1...])`
	`91`	`+ let rule = Array(line.dropFirst(2))`
`91`	`92`	`var isFiltered = false`
`92`	`93`	`if sec == "p" {`
`93`	`94`	`for (i,r) in f.p.enumerated() {`
`94`		`- if !r.isEmpty && r != rule[i+1] {`
	`95`	`+ if !r.isEmpty && r != rule[i] {`
`95`	`96`	`isFiltered = true`
`96`	`97`	`}`
`97`	`98`	`}`
`98`	`99`	`}`
`99`	`100`	`if sec == "g" {`
`100`	`101`	`for (i,r) in f.g.enumerated() {`
`101`		`- if !r.isEmpty && r != rule[i+1] {`
	`102`	`+ if !r.isEmpty && r != rule[i] {`
`102`	`103`	`isFiltered = true`
`103`	`104`	`}`
`104`	`105`	`}`
`@@ -117,25 +118,21 @@ extension MemoryAdapter: Adapter {`
`117`	`118`	`self.policy = []`
`118`	`119`	`if let astMap = m.getModel()["p"] {`
`119`	`120`	`for (ptype,ast) in astMap {`
`120`		`- ptype.forEach { sec in`
`121`		`- for policy in ast.policy {`
`122`		`- var rule = policy`
`123`		`- rule.insert(ptype, at: 0)`
`124`		`- rule.insert(String(sec), at: 0)`
`125`		`- self.policy.insert(rule)`
`126`		`- }`
	`121`	`+ for policy in ast.policy {`
	`122`	`+ var rule = policy`
	`123`	`+ rule.insert(ptype, at: 0)`
	`124`	`+ rule.insert("p", at: 0)`
	`125`	`+ self.policy.insert(rule)`
`127`	`126`	`}`
`128`	`127`	`}`
`129`	`128`	`}`
`130`	`129`	`if let astMap = m.getModel()["g"] {`
`131`	`130`	`for (ptype,ast) in astMap {`
`132`		`- ptype.forEach { sec in`
`133`		`- for policy in ast.policy {`
`134`		`- var rule = policy`
`135`		`- rule.insert(ptype, at: 0)`
`136`		`- rule.insert(String(sec), at: 0)`
`137`		`- self.policy.insert(rule)`
`138`		`- }`
	`131`	`+ for policy in ast.policy {`
	`132`	`+ var rule = policy`
	`133`	`+ rule.insert(ptype, at: 0)`
	`134`	`+ rule.insert("g", at: 0)`
	`135`	`+ self.policy.insert(rule)`
`139`	`136`	`}`
`140`	`137`	`}`
`141`	`138`	`}`
`@@ -147,25 +144,21 @@ extension MemoryAdapter: Adapter {`
`147`	`144`	`self.policy = []`
`148`	`145`	`if let astMap = m.getModel()["p"] {`
`149`	`146`	`for (ptype,ast) in astMap {`
`150`		`- ptype.forEach { sec in`
`151`		`- for policy in ast.policy {`
`152`		`- var rule = policy`
`153`		`- rule.insert(ptype, at: 0)`
`154`		`- rule.insert(String(sec), at: 0)`
`155`		`- self.policy.insert(rule)`
`156`		`- }`
	`147`	`+ for policy in ast.policy {`
	`148`	`+ var rule = policy`
	`149`	`+ rule.insert(ptype, at: 0)`
	`150`	`+ rule.insert("p", at: 0)`
	`151`	`+ self.policy.insert(rule)`
`157`	`152`	`}`
`158`	`153`	`}`
`159`	`154`	`}`
`160`	`155`	`if let astMap = m.getModel()["g"] {`
`161`	`156`	`for (ptype,ast) in astMap {`
`162`		`- ptype.forEach { sec in`
`163`		`- for policy in ast.policy {`
`164`		`- var rule = policy`
`165`		`- rule.insert(ptype, at: 0)`
`166`		`- rule.insert(String(sec), at: 0)`
`167`		`- self.policy.insert(rule)`
`168`		`- }`
	`157`	`+ for policy in ast.policy {`
	`158`	`+ var rule = policy`
	`159`	`+ rule.insert(ptype, at: 0)`
	`160`	`+ rule.insert("g", at: 0)`
	`161`	`+ self.policy.insert(rule)`
`169`	`162`	`}`
`170`	`163`	`}`
`171`	`164`	`}`
`@@ -258,14 +251,17 @@ extension MemoryAdapter: Adapter {`
`258`	`251`	`rule.insert(sec, at: 0)`
`259`	`252`	`return rule`
`260`	`253`	`}`
	`254`	`+ // Atomic semantics: if any rule doesn't exist, do not remove any and return false.`
`261`	`255`	`for rule in rules {`
`262`		`- if policy.contains(rule) {`
	`256`	`+ if !policy.contains(rule) {`
`263`	`257`	`allRemoved = false`
`264`		`- return eventloop.makeSucceededFuture(allRemoved)`
	`258`	`+ break`
`265`	`259`	`}`
`266`	`260`	`}`
`267`		`- for rule in rules {`
`268`		`- self.policy.remove(rule)`
	`261`	`+ if allRemoved {`
	`262`	`+ for rule in rules {`
	`263`	`+ _ = self.policy.remove(rule)`
	`264`	`+ }`
`269`	`265`	`}`
`270`	`266`	`return eventloop.makeSucceededFuture(allRemoved)`
`271`	`267`	`}`
`@@ -279,14 +275,17 @@ extension MemoryAdapter: Adapter {`
`279`	`275`	`rule.insert(sec, at: 0)`
`280`	`276`	`return rule`
`281`	`277`	`}`
	`278`	`+ // Atomic semantics`
`282`	`279`	`for rule in rules {`
`283`		`- if policy.contains(rule) {`
	`280`	`+ if !policy.contains(rule) {`
`284`	`281`	`allRemoved = false`
`285`		`- return allRemoved`
	`282`	`+ break`
`286`	`283`	`}`
`287`	`284`	`}`
`288`		`- for rule in rules {`
`289`		`- self.policy.remove(rule)`
	`285`	`+ if allRemoved {`
	`286`	`+ for rule in rules {`
	`287`	`+ _ = self.policy.remove(rule)`
	`288`	`+ }`
`290`	`289`	`}`
`291`	`290`	`return allRemoved`
`292`	`291`	`}`
Original file line number	Diff line number	Diff line change
`@@ -223,14 +223,17 @@ extension DefaultModel:Model {`
`223`	`223`	`public func removePolicies(sec:String,ptype:String,rules:[[String]]) -> Bool {`
`224`	`224`	`var allRemoved = true`
`225`	`225`	`if let ast = model[sec]?[ptype] {`
	`226`	`+ // Atomic semantics: if any rule is missing, do nothing and return false`
`226`	`227`	`for rule in rules {`
`227`		`- if ast.policy.contains(rule) {`
	`228`	`+ if !ast.policy.contains(rule) {`
`228`	`229`	`allRemoved = false`
`229`		`- return allRemoved`
	`230`	`+ break`
`230`	`231`	`}`
`231`	`232`	`}`
`232`		`- for rule in rules {`
`233`		`- ast.policy.removeAll { $0 == rule }`
	`233`	`+ if allRemoved {`
	`234`	`+ for rule in rules {`
	`235`	`+ ast.policy.removeAll { $0 == rule }`
	`236`	`+ }`
`234`	`237`	`}`
`235`	`238`	`}`
`236`	`239`	`return allRemoved`