[ISSUE #6622] Make plugin resource initialization transactional - #7192
dengliming wants to merge 1 commit into
Conversation
Aias00
left a comment
There was a problem hiding this comment.
Correct change. onPluginCreated performs several writes (resource row, permission/resource bindings) and previously had no transaction, so a failure partway through left an orphaned resource behind — and because the Assert failure path is precisely where you'd want a rollback, the missing @Transactional was easy to hit.
rollbackFor = Exception.class is the right choice here rather than the Spring default: Assert.isNull(...) throws IllegalArgumentException (a RuntimeException, so it would roll back anyway), but any checked exception surfacing from the mapper would otherwise commit a half-built resource.
Approving with one observation on the test: testPluginCreationListenerIsTransactional asserts the annotation exists via reflection. That pins the declarative metadata but not the behaviour — it will not catch, for example, a self-invocation problem or a proxy-less call path where the annotation is present but never applied. An integration-style test (force a mapper failure and assert the resource insert is rolled back) would be a much stronger guarantee. Not a blocker, just noting that the current test is a guard against accidental deletion rather than a functional check.
Run the plugin-created resource initialization listener in one transaction. If any resource or downstream permission write fails, all writes from the listener now roll back together, matching the existing plugin-deletion listener behavior.
Adds a regression check for the listener transaction configuration and rollback policy.
Tested with:
./mvnw -q -pl shenyu-admin -am -DskipTests=false -Dcheckstyle.skip=false -Dtest=ResourceServiceTest -DfailIfNoTests=false testFixes #6622