bitflicker64 commented on code in PR #1762: URL: https://github.com/apache/casbin/pull/1762#discussion_r3983721246
########## transaction_shared_test.go: ########## @@ -0,0 +1,236 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you 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. + +package casbin + +// Tests for the shared-transaction pattern described in +// https://github.com/apache/casbin/issues/1702. +// +// The core concern: when an application runs business-table writes and policy +// changes inside the same database transaction, both must be atomic. If the +// business step fails after the policy write, the policy write must roll back +// too — not get silently persisted. +// +// The tests here use a mock adapter that models exactly that contract: writes +// are staged in an "inflight" buffer and only moved to the "persisted" store +// on Commit. Rollback discards the buffer. Business writes go through the +// same adapter so we can assert on both tables simultaneously. + +import ( + "context" + "errors" + "strings" + "testing" + + "github.com/casbin/casbin/v3/model" + "github.com/casbin/casbin/v3/persist" +) + +// atomicMockAdapter is a TransactionalAdapter that keeps a clear boundary +// between in-flight and committed writes. It stands in for something like +// gorm-adapter where the database enforces that boundary for real. +type atomicMockAdapter struct { + persisted map[string]bool + businessRows map[string]bool +} Review Comment: A fault hook, so the mock can reproduce the case that actually matters. The suggestions on `AddPolicy` below and on the last line of the file both depend on this one, so take all three or none. ```suggestion type atomicMockAdapter struct { persisted map[string]bool businessRows map[string]bool failOnRule string } ``` ########## transaction_shared_test.go: ########## @@ -0,0 +1,236 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you 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. + +package casbin + +// Tests for the shared-transaction pattern described in +// https://github.com/apache/casbin/issues/1702. +// +// The core concern: when an application runs business-table writes and policy +// changes inside the same database transaction, both must be atomic. If the +// business step fails after the policy write, the policy write must roll back +// too — not get silently persisted. +// +// The tests here use a mock adapter that models exactly that contract: writes +// are staged in an "inflight" buffer and only moved to the "persisted" store +// on Commit. Rollback discards the buffer. Business writes go through the +// same adapter so we can assert on both tables simultaneously. + +import ( + "context" + "errors" + "strings" + "testing" + + "github.com/casbin/casbin/v3/model" + "github.com/casbin/casbin/v3/persist" +) + +// atomicMockAdapter is a TransactionalAdapter that keeps a clear boundary +// between in-flight and committed writes. It stands in for something like +// gorm-adapter where the database enforces that boundary for real. +type atomicMockAdapter struct { + persisted map[string]bool + businessRows map[string]bool +} + +type atomicMockTxState struct { + parent *atomicMockAdapter + inflightPolicies []atomicTxOp + inflightBusiness []atomicTxOp + done bool +} + +type atomicTxOp struct { + add bool + key string +} + +func newAtomicMockAdapter() *atomicMockAdapter { + return &atomicMockAdapter{ + persisted: make(map[string]bool), + businessRows: make(map[string]bool), + } +} + +func (a *atomicMockAdapter) LoadPolicy(model.Model) error { return nil } +func (a *atomicMockAdapter) SavePolicy(model.Model) error { return nil } +func (a *atomicMockAdapter) AddPolicy(_ string, _ string, _ []string) error { + return nil +} +func (a *atomicMockAdapter) RemovePolicy(_ string, _ string, _ []string) error { + return nil +} +func (a *atomicMockAdapter) RemoveFilteredPolicy(_ string, _ string, _ int, _ ...string) error { + return nil +} +func (a *atomicMockAdapter) BeginTransaction(_ context.Context) (persist.TransactionContext, error) { + return &atomicMockTxState{parent: a}, nil +} + +func (tx *atomicMockTxState) insertBusinessRow(key string) { + tx.inflightBusiness = append(tx.inflightBusiness, atomicTxOp{add: true, key: key}) +} + +func (tx *atomicMockTxState) Commit() error { + if tx.done { + return errors.New("transaction already finished") + } + tx.done = true + for _, op := range tx.inflightPolicies { + if op.add { + tx.parent.persisted[op.key] = true + } else { + delete(tx.parent.persisted, op.key) + } + } + for _, op := range tx.inflightBusiness { + if op.add { + tx.parent.businessRows[op.key] = true + } else { + delete(tx.parent.businessRows, op.key) + } + } + return nil +} + +func (tx *atomicMockTxState) Rollback() error { + if tx.done { + return errors.New("transaction already finished") + } + tx.done = true + return nil +} + +func (tx *atomicMockTxState) GetAdapter() persist.Adapter { + return &atomicMockTxAdapter{tx: tx} +} + +// atomicMockTxAdapter routes policy writes through the open transaction so +// they share fate with any business writes staged via insertBusinessRow. +type atomicMockTxAdapter struct { + tx *atomicMockTxState +} + +func (a *atomicMockTxAdapter) LoadPolicy(model.Model) error { return nil } +func (a *atomicMockTxAdapter) SavePolicy(model.Model) error { return nil } +func (a *atomicMockTxAdapter) AddPolicy(_ string, _ string, rule []string) error { + a.tx.inflightPolicies = append(a.tx.inflightPolicies, atomicTxOp{add: true, key: atomicRuleKey(rule)}) + return nil +} Review Comment: Honour the hook here, so a policy write can fail during the commit flush the way a real database rejects an insert. Needs the `failOnRule` field from the suggestion above, otherwise this does not compile on its own. ```suggestion func (a *atomicMockTxAdapter) AddPolicy(_ string, _ string, rule []string) error { k := atomicRuleKey(rule) if k == a.tx.parent.failOnRule { return errors.New("db write failed: " + k) } a.tx.inflightPolicies = append(a.tx.inflightPolicies, atomicTxOp{add: true, key: k}) return nil } ``` ########## transaction_shared_test.go: ########## @@ -0,0 +1,236 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you 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. + +package casbin + +// Tests for the shared-transaction pattern described in +// https://github.com/apache/casbin/issues/1702. +// +// The core concern: when an application runs business-table writes and policy +// changes inside the same database transaction, both must be atomic. If the +// business step fails after the policy write, the policy write must roll back +// too — not get silently persisted. Review Comment: Nit while you are here: this is the only em dash in any `.go` file in the tree, the rest is plain ASCII. ```suggestion // too, not get silently persisted. ``` ########## transaction_shared_test.go: ########## @@ -0,0 +1,236 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you 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. + +package casbin + +// Tests for the shared-transaction pattern described in +// https://github.com/apache/casbin/issues/1702. +// +// The core concern: when an application runs business-table writes and policy +// changes inside the same database transaction, both must be atomic. If the +// business step fails after the policy write, the policy write must roll back +// too — not get silently persisted. +// +// The tests here use a mock adapter that models exactly that contract: writes +// are staged in an "inflight" buffer and only moved to the "persisted" store +// on Commit. Rollback discards the buffer. Business writes go through the +// same adapter so we can assert on both tables simultaneously. + +import ( + "context" + "errors" + "strings" + "testing" + + "github.com/casbin/casbin/v3/model" + "github.com/casbin/casbin/v3/persist" +) + +// atomicMockAdapter is a TransactionalAdapter that keeps a clear boundary +// between in-flight and committed writes. It stands in for something like +// gorm-adapter where the database enforces that boundary for real. +type atomicMockAdapter struct { + persisted map[string]bool + businessRows map[string]bool +} + +type atomicMockTxState struct { + parent *atomicMockAdapter + inflightPolicies []atomicTxOp + inflightBusiness []atomicTxOp + done bool +} + +type atomicTxOp struct { + add bool + key string +} + +func newAtomicMockAdapter() *atomicMockAdapter { + return &atomicMockAdapter{ + persisted: make(map[string]bool), + businessRows: make(map[string]bool), + } +} + +func (a *atomicMockAdapter) LoadPolicy(model.Model) error { return nil } +func (a *atomicMockAdapter) SavePolicy(model.Model) error { return nil } +func (a *atomicMockAdapter) AddPolicy(_ string, _ string, _ []string) error { + return nil +} +func (a *atomicMockAdapter) RemovePolicy(_ string, _ string, _ []string) error { + return nil +} +func (a *atomicMockAdapter) RemoveFilteredPolicy(_ string, _ string, _ int, _ ...string) error { + return nil +} +func (a *atomicMockAdapter) BeginTransaction(_ context.Context) (persist.TransactionContext, error) { + return &atomicMockTxState{parent: a}, nil +} + +func (tx *atomicMockTxState) insertBusinessRow(key string) { + tx.inflightBusiness = append(tx.inflightBusiness, atomicTxOp{add: true, key: key}) +} + +func (tx *atomicMockTxState) Commit() error { + if tx.done { + return errors.New("transaction already finished") + } + tx.done = true + for _, op := range tx.inflightPolicies { + if op.add { + tx.parent.persisted[op.key] = true + } else { + delete(tx.parent.persisted, op.key) + } + } + for _, op := range tx.inflightBusiness { + if op.add { + tx.parent.businessRows[op.key] = true + } else { + delete(tx.parent.businessRows, op.key) + } + } + return nil +} + +func (tx *atomicMockTxState) Rollback() error { + if tx.done { + return errors.New("transaction already finished") + } + tx.done = true + return nil +} + +func (tx *atomicMockTxState) GetAdapter() persist.Adapter { + return &atomicMockTxAdapter{tx: tx} +} + +// atomicMockTxAdapter routes policy writes through the open transaction so +// they share fate with any business writes staged via insertBusinessRow. +type atomicMockTxAdapter struct { + tx *atomicMockTxState +} + +func (a *atomicMockTxAdapter) LoadPolicy(model.Model) error { return nil } +func (a *atomicMockTxAdapter) SavePolicy(model.Model) error { return nil } +func (a *atomicMockTxAdapter) AddPolicy(_ string, _ string, rule []string) error { + a.tx.inflightPolicies = append(a.tx.inflightPolicies, atomicTxOp{add: true, key: atomicRuleKey(rule)}) + return nil +} +func (a *atomicMockTxAdapter) RemovePolicy(_ string, _ string, rule []string) error { + a.tx.inflightPolicies = append(a.tx.inflightPolicies, atomicTxOp{add: false, key: atomicRuleKey(rule)}) + return nil +} +func (a *atomicMockTxAdapter) RemoveFilteredPolicy(_ string, _ string, _ int, _ ...string) error { + return nil +} Review Comment: Minor, and the fix is a comment rather than code: this can never be reached through the commit flush. `Transaction.RemoveFilteredPolicy` resolves the filter against the buffered model in `bufferRemoveFiltered` and buffers a plain `OperationRemove`, so `applyRemoveOperationToDatabase` calls `RemovePolicies` or `RemovePolicy` and nothing in `transaction_commit.go` ever calls `adapter.RemoveFilteredPolicy`. It exists only to satisfy `persist.Adapter`. Worth one line saying so, so nobody later adds staging here and wonders why no test moves. Same applies to `RemovePolicy` above and the `add: false` branch of `Commit`: both are unreachable from any test in this file today. ########## transaction_shared_test.go: ########## @@ -0,0 +1,236 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you 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. + +package casbin + +// Tests for the shared-transaction pattern described in +// https://github.com/apache/casbin/issues/1702. +// +// The core concern: when an application runs business-table writes and policy +// changes inside the same database transaction, both must be atomic. If the +// business step fails after the policy write, the policy write must roll back +// too — not get silently persisted. +// +// The tests here use a mock adapter that models exactly that contract: writes +// are staged in an "inflight" buffer and only moved to the "persisted" store +// on Commit. Rollback discards the buffer. Business writes go through the +// same adapter so we can assert on both tables simultaneously. + +import ( + "context" + "errors" + "strings" + "testing" + + "github.com/casbin/casbin/v3/model" + "github.com/casbin/casbin/v3/persist" +) + +// atomicMockAdapter is a TransactionalAdapter that keeps a clear boundary +// between in-flight and committed writes. It stands in for something like +// gorm-adapter where the database enforces that boundary for real. +type atomicMockAdapter struct { + persisted map[string]bool + businessRows map[string]bool +} + +type atomicMockTxState struct { + parent *atomicMockAdapter + inflightPolicies []atomicTxOp + inflightBusiness []atomicTxOp + done bool +} + +type atomicTxOp struct { + add bool + key string +} + +func newAtomicMockAdapter() *atomicMockAdapter { + return &atomicMockAdapter{ + persisted: make(map[string]bool), + businessRows: make(map[string]bool), + } +} + +func (a *atomicMockAdapter) LoadPolicy(model.Model) error { return nil } +func (a *atomicMockAdapter) SavePolicy(model.Model) error { return nil } +func (a *atomicMockAdapter) AddPolicy(_ string, _ string, _ []string) error { + return nil +} +func (a *atomicMockAdapter) RemovePolicy(_ string, _ string, _ []string) error { + return nil +} +func (a *atomicMockAdapter) RemoveFilteredPolicy(_ string, _ string, _ int, _ ...string) error { + return nil +} +func (a *atomicMockAdapter) BeginTransaction(_ context.Context) (persist.TransactionContext, error) { + return &atomicMockTxState{parent: a}, nil +} + +func (tx *atomicMockTxState) insertBusinessRow(key string) { + tx.inflightBusiness = append(tx.inflightBusiness, atomicTxOp{add: true, key: key}) +} + +func (tx *atomicMockTxState) Commit() error { + if tx.done { + return errors.New("transaction already finished") + } + tx.done = true + for _, op := range tx.inflightPolicies { + if op.add { + tx.parent.persisted[op.key] = true + } else { + delete(tx.parent.persisted, op.key) + } + } + for _, op := range tx.inflightBusiness { + if op.add { + tx.parent.businessRows[op.key] = true + } else { + delete(tx.parent.businessRows, op.key) + } + } + return nil +} + +func (tx *atomicMockTxState) Rollback() error { + if tx.done { + return errors.New("transaction already finished") + } + tx.done = true + return nil +} + +func (tx *atomicMockTxState) GetAdapter() persist.Adapter { + return &atomicMockTxAdapter{tx: tx} +} + +// atomicMockTxAdapter routes policy writes through the open transaction so +// they share fate with any business writes staged via insertBusinessRow. +type atomicMockTxAdapter struct { + tx *atomicMockTxState +} + +func (a *atomicMockTxAdapter) LoadPolicy(model.Model) error { return nil } +func (a *atomicMockTxAdapter) SavePolicy(model.Model) error { return nil } +func (a *atomicMockTxAdapter) AddPolicy(_ string, _ string, rule []string) error { + a.tx.inflightPolicies = append(a.tx.inflightPolicies, atomicTxOp{add: true, key: atomicRuleKey(rule)}) + return nil +} +func (a *atomicMockTxAdapter) RemovePolicy(_ string, _ string, rule []string) error { + a.tx.inflightPolicies = append(a.tx.inflightPolicies, atomicTxOp{add: false, key: atomicRuleKey(rule)}) + return nil +} +func (a *atomicMockTxAdapter) RemoveFilteredPolicy(_ string, _ string, _ int, _ ...string) error { + return nil +} + +func atomicRuleKey(rule []string) string { + return strings.Join(rule, ",") +} + +// TestSharedTransactionRollback shows that when business logic fails after a +// policy change, neither the policy write nor the business write survives. +// +// This is the failure mode reported in issue #1702: without a shared +// transaction the policy row gets committed even though the rest of the +// operation rolls back. +func TestSharedTransactionRollback(t *testing.T) { + a := newAtomicMockAdapter() + e, err := NewTransactionalEnforcer("examples/rbac_model.conf", a) + if err != nil { + t.Fatalf("enforcer: %v", err) + } + + bizErr := errors.New("payment gateway timeout") + + err = e.WithTransaction(context.Background(), func(tx *Transaction) error { + if _, addErr := tx.AddGroupingPolicy("alice", "admin"); addErr != nil { + return addErr + } + + // Caller writes to their own table via the transaction-scoped adapter. + if sharedTx, ok := tx.txContext.(*atomicMockTxState); ok { + sharedTx.insertBusinessRow("order:42") + } + + return bizErr + }) + + if !errors.Is(err, bizErr) { + t.Fatalf("expected bizErr, got %v", err) + } + + // The in-memory model must not reflect the rolled-back policy. + if ok, _ := e.HasGroupingPolicy("alice", "admin"); ok { + t.Fatal("policy must be rolled back together with the transaction") + } + if a.persisted["alice,admin"] { + t.Fatal("policy row must not reach the persisted store after rollback") + } + if a.businessRows["order:42"] { + t.Fatal("business row must not reach the persisted store after rollback") + } +} + +// TestSharedTransactionCommit is the happy path: both the policy change and +// the business write land atomically when there is no error. +func TestSharedTransactionCommit(t *testing.T) { + a := newAtomicMockAdapter() + e, err := NewTransactionalEnforcer("examples/rbac_model.conf", a) + if err != nil { + t.Fatalf("enforcer: %v", err) + } + + err = e.WithTransaction(context.Background(), func(tx *Transaction) error { + if _, addErr := tx.AddGroupingPolicy("alice", "admin"); addErr != nil { + return addErr + } + if _, addErr := tx.AddPolicy("admin", "data1", "read"); addErr != nil { + return addErr + } + if sharedTx, ok := tx.txContext.(*atomicMockTxState); ok { + sharedTx.insertBusinessRow("order:99") + } + return nil + }) + if err != nil { + t.Fatalf("transaction: %v", err) + } + + if ok, _ := e.HasGroupingPolicy("alice", "admin"); !ok { + t.Fatal("grouping policy should be visible after commit") + } + if ok, _ := e.HasPolicy("admin", "data1", "read"); !ok { + t.Fatal("policy should be visible after commit") + } + if !a.persisted["alice,admin"] { + t.Fatal("grouping policy row should be in persisted store after commit") + } + if !a.persisted["admin,data1,read"] { + t.Fatal("policy row should be in persisted store after commit") + } + if !a.businessRows["order:99"] { + t.Fatal("business row should be in persisted store after commit") + } + + // Role links must have been rebuilt: alice inherits admin's permission. + if ok, enforceErr := e.Enforce("alice", "data1", "read"); enforceErr != nil || !ok { + t.Fatalf("alice should inherit admin's read, got %v (err %v)", ok, enforceErr) + } +} Review Comment: Here is the missing case. Nothing in the current pair reaches the state where rules have been flushed to the adapter and then have to be discarded, which is the whole point of sharing a transaction. A write that fails partway through the flush produces it. Depends on the `failOnRule` field and the `AddPolicy` hook suggested above; applied alone it will not compile. It does discriminate, which the other two do not. Keeping the fault hook and changing only the staging line to `a.tx.parent.persisted[k] = true` makes this fail on `a rule staged before the failure must not survive the rollback` while `TestSharedTransactionRollback` and `TestSharedTransactionCommit` stay green. As submitted it passes on this branch, gofmt and vet clean. ```suggestion } // TestSharedTransactionFlushFailure fails a policy write during the commit // flush, after an earlier rule and the business row have already been staged // on the transaction. Only a genuinely transactional adapter discards them. func TestSharedTransactionFlushFailure(t *testing.T) { a := newAtomicMockAdapter() a.failOnRule = "admin,data1,read" e, err := NewTransactionalEnforcer("examples/rbac_model.conf", a) if err != nil { t.Fatalf("enforcer: %v", err) } err = e.WithTransaction(context.Background(), func(tx *Transaction) error { if _, addErr := tx.AddGroupingPolicy("alice", "admin"); addErr != nil { return addErr } if _, addErr := tx.AddPolicy("admin", "data1", "read"); addErr != nil { return addErr } tx.GetAdapter().(*atomicMockTxAdapter).tx.insertBusinessRow("order:7") return nil }) if err == nil { t.Fatal("commit must surface the failed policy write") } if a.persisted["alice,admin"] { t.Fatal("a rule staged before the failure must not survive the rollback") } if a.businessRows["order:7"] { t.Fatal("the business row must not survive the rollback") } if ok, _ := e.HasGroupingPolicy("alice", "admin"); ok { t.Fatal("the model must not reflect a transaction that never committed") } } ``` A sibling worth considering later: `transaction_commit.go:84` returns a `txContext.Commit()` failure without calling `Rollback` and without touching the model. Nothing covers that path either. ########## transaction_shared_test.go: ########## @@ -0,0 +1,236 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you 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. + +package casbin + +// Tests for the shared-transaction pattern described in +// https://github.com/apache/casbin/issues/1702. +// +// The core concern: when an application runs business-table writes and policy +// changes inside the same database transaction, both must be atomic. If the +// business step fails after the policy write, the policy write must roll back +// too — not get silently persisted. +// +// The tests here use a mock adapter that models exactly that contract: writes +// are staged in an "inflight" buffer and only moved to the "persisted" store +// on Commit. Rollback discards the buffer. Business writes go through the +// same adapter so we can assert on both tables simultaneously. + +import ( + "context" + "errors" + "strings" + "testing" + + "github.com/casbin/casbin/v3/model" + "github.com/casbin/casbin/v3/persist" +) + +// atomicMockAdapter is a TransactionalAdapter that keeps a clear boundary +// between in-flight and committed writes. It stands in for something like +// gorm-adapter where the database enforces that boundary for real. +type atomicMockAdapter struct { + persisted map[string]bool + businessRows map[string]bool +} + +type atomicMockTxState struct { + parent *atomicMockAdapter + inflightPolicies []atomicTxOp + inflightBusiness []atomicTxOp + done bool +} + +type atomicTxOp struct { + add bool + key string +} + +func newAtomicMockAdapter() *atomicMockAdapter { + return &atomicMockAdapter{ + persisted: make(map[string]bool), + businessRows: make(map[string]bool), + } +} + +func (a *atomicMockAdapter) LoadPolicy(model.Model) error { return nil } +func (a *atomicMockAdapter) SavePolicy(model.Model) error { return nil } +func (a *atomicMockAdapter) AddPolicy(_ string, _ string, _ []string) error { + return nil +} +func (a *atomicMockAdapter) RemovePolicy(_ string, _ string, _ []string) error { + return nil +} +func (a *atomicMockAdapter) RemoveFilteredPolicy(_ string, _ string, _ int, _ ...string) error { + return nil +} +func (a *atomicMockAdapter) BeginTransaction(_ context.Context) (persist.TransactionContext, error) { + return &atomicMockTxState{parent: a}, nil +} + +func (tx *atomicMockTxState) insertBusinessRow(key string) { + tx.inflightBusiness = append(tx.inflightBusiness, atomicTxOp{add: true, key: key}) +} + +func (tx *atomicMockTxState) Commit() error { + if tx.done { + return errors.New("transaction already finished") + } + tx.done = true + for _, op := range tx.inflightPolicies { + if op.add { + tx.parent.persisted[op.key] = true + } else { + delete(tx.parent.persisted, op.key) + } + } + for _, op := range tx.inflightBusiness { + if op.add { + tx.parent.businessRows[op.key] = true + } else { + delete(tx.parent.businessRows, op.key) + } + } + return nil +} + +func (tx *atomicMockTxState) Rollback() error { + if tx.done { + return errors.New("transaction already finished") + } + tx.done = true + return nil +} + +func (tx *atomicMockTxState) GetAdapter() persist.Adapter { + return &atomicMockTxAdapter{tx: tx} +} + +// atomicMockTxAdapter routes policy writes through the open transaction so +// they share fate with any business writes staged via insertBusinessRow. +type atomicMockTxAdapter struct { + tx *atomicMockTxState +} + +func (a *atomicMockTxAdapter) LoadPolicy(model.Model) error { return nil } +func (a *atomicMockTxAdapter) SavePolicy(model.Model) error { return nil } +func (a *atomicMockTxAdapter) AddPolicy(_ string, _ string, rule []string) error { + a.tx.inflightPolicies = append(a.tx.inflightPolicies, atomicTxOp{add: true, key: atomicRuleKey(rule)}) + return nil +} +func (a *atomicMockTxAdapter) RemovePolicy(_ string, _ string, rule []string) error { + a.tx.inflightPolicies = append(a.tx.inflightPolicies, atomicTxOp{add: false, key: atomicRuleKey(rule)}) + return nil +} +func (a *atomicMockTxAdapter) RemoveFilteredPolicy(_ string, _ string, _ int, _ ...string) error { + return nil +} + +func atomicRuleKey(rule []string) string { + return strings.Join(rule, ",") +} + +// TestSharedTransactionRollback shows that when business logic fails after a +// policy change, neither the policy write nor the business write survives. +// +// This is the failure mode reported in issue #1702: without a shared +// transaction the policy row gets committed even though the rest of the +// operation rolls back. +func TestSharedTransactionRollback(t *testing.T) { + a := newAtomicMockAdapter() + e, err := NewTransactionalEnforcer("examples/rbac_model.conf", a) + if err != nil { + t.Fatalf("enforcer: %v", err) + } + + bizErr := errors.New("payment gateway timeout") + + err = e.WithTransaction(context.Background(), func(tx *Transaction) error { + if _, addErr := tx.AddGroupingPolicy("alice", "admin"); addErr != nil { + return addErr + } + + // Caller writes to their own table via the transaction-scoped adapter. + if sharedTx, ok := tx.txContext.(*atomicMockTxState); ok { + sharedTx.insertBusinessRow("order:42") + } + + return bizErr + }) + + if !errors.Is(err, bizErr) { + t.Fatalf("expected bizErr, got %v", err) + } + + // The in-memory model must not reflect the rolled-back policy. + if ok, _ := e.HasGroupingPolicy("alice", "admin"); ok { + t.Fatal("policy must be rolled back together with the transaction") + } + if a.persisted["alice,admin"] { + t.Fatal("policy row must not reach the persisted store after rollback") + } + if a.businessRows["order:42"] { + t.Fatal("business row must not reach the persisted store after rollback") + } Review Comment: Neither of these two assertions can fail as written. `tx.AddGroupingPolicy` only buffers into `tx.buffer`. The buffer is flushed to the transaction-scoped adapter in `Commit()` phase 1, and on this path `Commit()` never runs, so `atomicMockTxAdapter.AddPolicy` is never called and `a.persisted` is empty by construction. `a.businessRows` is empty for the matching reason, per mutation 2 in the review body. The assertion carrying weight is `HasGroupingPolicy` on line 180: a change to the buffering path would break that one, and it is worth keeping. The thing to adjust is the file doc comment on lines 20-31, which says a policy write that is not rolled back would "get silently persisted". These tests do not demonstrate that, so I would narrow the claim there and let the new test below cover persistence. ########## transaction_shared_test.go: ########## @@ -0,0 +1,236 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you 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. + +package casbin + +// Tests for the shared-transaction pattern described in +// https://github.com/apache/casbin/issues/1702. +// +// The core concern: when an application runs business-table writes and policy +// changes inside the same database transaction, both must be atomic. If the +// business step fails after the policy write, the policy write must roll back +// too — not get silently persisted. +// +// The tests here use a mock adapter that models exactly that contract: writes +// are staged in an "inflight" buffer and only moved to the "persisted" store +// on Commit. Rollback discards the buffer. Business writes go through the +// same adapter so we can assert on both tables simultaneously. + +import ( + "context" + "errors" + "strings" + "testing" + + "github.com/casbin/casbin/v3/model" + "github.com/casbin/casbin/v3/persist" +) + +// atomicMockAdapter is a TransactionalAdapter that keeps a clear boundary +// between in-flight and committed writes. It stands in for something like +// gorm-adapter where the database enforces that boundary for real. +type atomicMockAdapter struct { + persisted map[string]bool + businessRows map[string]bool +} + +type atomicMockTxState struct { + parent *atomicMockAdapter + inflightPolicies []atomicTxOp + inflightBusiness []atomicTxOp + done bool +} + +type atomicTxOp struct { + add bool + key string +} + +func newAtomicMockAdapter() *atomicMockAdapter { + return &atomicMockAdapter{ + persisted: make(map[string]bool), + businessRows: make(map[string]bool), + } +} + +func (a *atomicMockAdapter) LoadPolicy(model.Model) error { return nil } +func (a *atomicMockAdapter) SavePolicy(model.Model) error { return nil } +func (a *atomicMockAdapter) AddPolicy(_ string, _ string, _ []string) error { + return nil +} +func (a *atomicMockAdapter) RemovePolicy(_ string, _ string, _ []string) error { + return nil +} +func (a *atomicMockAdapter) RemoveFilteredPolicy(_ string, _ string, _ int, _ ...string) error { + return nil +} +func (a *atomicMockAdapter) BeginTransaction(_ context.Context) (persist.TransactionContext, error) { + return &atomicMockTxState{parent: a}, nil +} + +func (tx *atomicMockTxState) insertBusinessRow(key string) { + tx.inflightBusiness = append(tx.inflightBusiness, atomicTxOp{add: true, key: key}) +} + +func (tx *atomicMockTxState) Commit() error { + if tx.done { + return errors.New("transaction already finished") + } + tx.done = true + for _, op := range tx.inflightPolicies { + if op.add { + tx.parent.persisted[op.key] = true + } else { + delete(tx.parent.persisted, op.key) + } + } + for _, op := range tx.inflightBusiness { + if op.add { + tx.parent.businessRows[op.key] = true + } else { + delete(tx.parent.businessRows, op.key) + } + } + return nil +} + +func (tx *atomicMockTxState) Rollback() error { + if tx.done { + return errors.New("transaction already finished") + } + tx.done = true + return nil +} + +func (tx *atomicMockTxState) GetAdapter() persist.Adapter { + return &atomicMockTxAdapter{tx: tx} +} + +// atomicMockTxAdapter routes policy writes through the open transaction so +// they share fate with any business writes staged via insertBusinessRow. +type atomicMockTxAdapter struct { + tx *atomicMockTxState +} + +func (a *atomicMockTxAdapter) LoadPolicy(model.Model) error { return nil } +func (a *atomicMockTxAdapter) SavePolicy(model.Model) error { return nil } +func (a *atomicMockTxAdapter) AddPolicy(_ string, _ string, rule []string) error { + a.tx.inflightPolicies = append(a.tx.inflightPolicies, atomicTxOp{add: true, key: atomicRuleKey(rule)}) + return nil +} +func (a *atomicMockTxAdapter) RemovePolicy(_ string, _ string, rule []string) error { + a.tx.inflightPolicies = append(a.tx.inflightPolicies, atomicTxOp{add: false, key: atomicRuleKey(rule)}) + return nil +} +func (a *atomicMockTxAdapter) RemoveFilteredPolicy(_ string, _ string, _ int, _ ...string) error { + return nil +} + +func atomicRuleKey(rule []string) string { + return strings.Join(rule, ",") +} + +// TestSharedTransactionRollback shows that when business logic fails after a +// policy change, neither the policy write nor the business write survives. +// +// This is the failure mode reported in issue #1702: without a shared +// transaction the policy row gets committed even though the rest of the +// operation rolls back. +func TestSharedTransactionRollback(t *testing.T) { + a := newAtomicMockAdapter() + e, err := NewTransactionalEnforcer("examples/rbac_model.conf", a) + if err != nil { + t.Fatalf("enforcer: %v", err) + } + + bizErr := errors.New("payment gateway timeout") + + err = e.WithTransaction(context.Background(), func(tx *Transaction) error { + if _, addErr := tx.AddGroupingPolicy("alice", "admin"); addErr != nil { + return addErr + } + + // Caller writes to their own table via the transaction-scoped adapter. + if sharedTx, ok := tx.txContext.(*atomicMockTxState); ok { + sharedTx.insertBusinessRow("order:42") + } Review Comment: Two things here. The `ok` is swallowed, so if the assertion ever stopped matching, the test would quietly stop staging a business row and still pass. And `tx.txContext` is unexported: a caller following #1702 cannot reach it. The public entry point is `tx.GetAdapter()`, which is what `TestTransactionGetAdapter` already documents, so going through it makes the test a pattern someone can copy. ```suggestion // Caller writes to their own table via the transaction-scoped adapter. tx.GetAdapter().(*atomicMockTxAdapter).tx.insertBusinessRow("order:42") ``` Same change applies to lines 207-209 in `TestSharedTransactionCommit`. That one is not vacuous, since `a.businessRows["order:99"]` would catch a missed staging, but the two should read alike. -- 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]
