Skip to content

[ISSUE #6622] Make plugin resource initialization transactional - #7192

Open
dengliming wants to merge 1 commit into
apache:masterfrom
dengliming:fix-6622-transactional-plugin-resource-init
Open

dengliming wants to merge 1 commit into
apache:masterfrom
dengliming:fix-6622-transactional-plugin-resource-init

Conversation

@dengliming

Copy link
Copy Markdown
Member

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 test

Fixes #6622

@Aias00 Aias00 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] ResourceServiceImpl.onPluginCreated performs multiple writes non-transactionally

2 participants