Fixes #6806: Skip the plugin jar blob in list queries and index snapshot joins - #7360
BobSong-dev wants to merge 12 commits into
Conversation
Aias00
left a comment
There was a problem hiding this comment.
The code direction is reasonable, but the current head is conflicted and has no checks, so this exact diff is not merge-ready. Please rebase/resolve the conflicts and rerun CI. Since selectByQuery intentionally stops loading plugin_jar and the public paged response will now expose file as an empty string, please also add a mapper/service-level contract test showing paged list omits the jar while detail/export paths still preserve it.
Aias00
left a comment
There was a problem hiding this comment.
The current head is still not merge-ready: GitHub reports mergeStateStatus=DIRTY and no checks are reported for this branch. Please rebase/resolve the conflicts and rerun CI. The earlier contract-test gap also remains: since selectByQuery intentionally omits plugin_jar, please add coverage proving the paged list omits the jar while detail/export paths still preserve it.
Aias00
left a comment
There was a problem hiding this comment.
Thanks for this — skipping the plugin_jar BLOB on list queries is a good win, and I verified your claim about the MySQL indexes: db/init/mysql/schema.sql on master already has idx_permission_object_resource(object_id, resource_id), idx_user_role_user_role(user_id, role_id) and idx_resource_parent(parent_id), so not re-adding them on MySQL is correct. Adding idx_permission_resource_id is also justified, because resource_id is not the leading column of the existing composite index.
Requesting changes on one point:
1. The list API will silently return an empty jar
PluginServiceImpl.listByPage loads via pluginMapper.selectByQuery(...) and maps each row with PluginVO::buildPluginVO, and PluginVO.buildPluginVO sets:
Optional.ofNullable(pluginDO.getPluginJar()).map(Base64::encodeToString).orElse("")
With plugin_jar no longer selected by selectByQuery, every plugin in the paginated list response will now carry jar: "". Please handle this explicitly — either use a list VO without the jar field, or confirm the admin UI does not consume jar from the list response, or keep the column here.
Non-blocking:
2. Upgrade scripts are not idempotent
db/upgrade/2.7.1-upgrade-2.7.2-*.sql uses bare ALTER TABLE ... ADD INDEX (mysql/ob) and CREATE INDEX (pg/og/oracle). Re-running the upgrade, or re-running it after a partial failure, fails on the already-created indexes. PostgreSQL and openGauss support CREATE INDEX IF NOT EXISTS — worth using it there at least.
3. Minor placement and maintenance
The H2 CREATE INDEX IF NOT EXISTS statements are inserted between INSERT data rows; they work, but belong with the other index DDL at the end of the file. Also List_Column_List duplicates the column list, so any future column addition has to be made in two places.
|
Re-checked after the new commits. The SQL fragment restructure is cleaner ( The chain is unchanged — I don't think this is something we can just let through: it silently changes the admin list API response. Please pick one — (a) give the list path a VO without a jar field, (b) keep |
sorry,I missed it.The problem has been resolved.Thanks for your review. |
Aias00
left a comment
There was a problem hiding this comment.
Re-reviewed — this now addresses the point I raised, and in the cleanest of the three options. The list path (PluginMapper#searchByCondition, PluginServiceImpl#listByPage, PluginController pagination) now returns PluginListVO, which has no jar field, so the paginated plugin list no longer pretends to carry a Base64 jar it never loaded. findById still returns PluginVO with the jar, so the detail view and export are unaffected, and selectByQuery can keep omitting the BLOB.
The controller and mapper tests were updated to match, and the index work across all five dialects plus the upgrade scripts is unchanged. The red e2e / e2e-storage jobs are the known infrastructure failures, not this change. Approving — thanks for seeing it through.
|
I checked the current conflict against The conflicting hunks are both adding indexes to the same migration sections: this PR's snapshot/permission indexes and #7404's operation-log indexes. When resolving, preserve both independent index sets and retain this PR's PostgreSQL/openGauss |
Fixes #6806
Background
Plugin list queries load a JAR BLOB for each row although listing plugin metadata does not need it. Snapshot and permission queries also need indexes with suitable leading columns.
Changes
PluginListVOwith no JAR/file field. GET/plugin-templateand POST/plugin-template/list/searchand/plugin-template/list/search/adaptorreturn this metadata-only list contract. The paged response intentionally omitsfileinstead of reporting a misleading empty string. Clients needing JAR content must use the detail/export paths.PluginVOand the full column fragment, includingplugin_jar.CREATE INDEX IF NOT EXISTSfor this PR's PostgreSQL/openGauss upgrade indexes, put H2 index DDL in the index section, and share list/detail column definitions. Other dialects retain their existing one-time upgrade DDL; the whole upgrade script is not claimed to be idempotent.Verification
./mvnw.cmd -pl shenyu-admin -am test -Dtest=PluginMapperTest,PluginServiceTest,PluginControllerTest -Dsurefire.failIfNoSpecifiedTests=false -Dmaven.javadoc.skip=true: BUILD SUCCESS; 33 tests, zero failures/errors/skips; Checkstyle passed.git diff --checkpassed. Current master is included without rewriting published history.b580baad2has 34 successful checks and three skipped checks. MySQL storage e2e failed in the POST/anythingscenario after the external HTTPBin upstream returned HTML502 Bad Gateway(Server: awselb/2.0); the aggregate e2e check also failed. PostgreSQL and OpenGauss storage checks passed on this retry. Storage testcase/Compose files are unchanged from current master. CI is not green; no assertions or implementation were changed to hide this failure.