Repository navigation
Fixes #6806: Skip the plugin jar blob in list queries and index snapshot joins - #7360
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 |
…t-indexes # Conflicts: # db/upgrade/2.7.1-upgrade-2.7.2-mysql.sql # db/upgrade/2.7.1-upgrade-2.7.2-ob.sql # db/upgrade/2.7.1-upgrade-2.7.2-og.sql # db/upgrade/2.7.1-upgrade-2.7.2-oracle.sql # db/upgrade/2.7.1-upgrade-2.7.2-pg.sql
Aias00
left a comment
There was a problem hiding this comment.
The updated head is directionally sound and CI is green. Because this spans list projections, indexes, and multiple database dialects, please ensure the list contract and all supported database upgrade paths remain covered before merge.
Thanks,I fix it |
Aias00
left a comment
There was a problem hiding this comment.
The updated head addresses the list projection and adds multi-dialect upgrade/index contract coverage. This remains a broad database compatibility change across schemas, upgrades, mappers, and service contracts, so please keep the supported-database SQL matrix as the final merge gate.
Aias00
left a comment
There was a problem hiding this comment.
Reviewed current head 96272b357791.
The current head addresses the earlier list-response blocker by returning PluginListVO without jar/file fields for paged/search results while preserving plugin_jar on detail/export paths. The added SQL matrix tooling and contract tests cover the broader index/upgrade surface, with local Python unit tests passing.
Validation: exact-head targeted Maven reactor tests passed on Java 17.
./mvnw -pl shenyu-admin -am test -Dtest=PluginControllerTest,AdminQueryIndexTest,PluginIndexUpgradeContractTest,PluginMapperTest,PluginServiceTest -Dsurefire.failIfNoSpecifiedTests=false -Drat.skip=true -Djacoco.skip=true -DskipRemoteResources=true -Dmaven.javadoc.skip=true
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.Upgrade coverage follow-up
PluginIndexUpgradeContractTestreads the real MySQL, OceanBase, PostgreSQL, openGauss and Oracle 2.7.1-to-2.7.2 scripts and guards this PR's exact index/table/column declarations, expected per-dialect sets, duplicate declarations, and PostgreSQL/openGaussIF NOT EXISTSclauses. This is automated script-contract coverage, not native database execution or validation of the entire migration.AdminQueryIndexTestnow also checksidx_selector_plugin_id(plugin_id)andidx_permission_resource_id(resource_id)using real H2 JDBC index metadata. Existing list tests continue verifying omitted list payload fields and byte-identical detail/export JAR content.Native SQL merge gate
ffa4ce140adds a separatenative-sql-matrixworkflow and stable aggregatesql-matrixcheck. Applicable SQL/Admin changes require list contracts and all five native engine jobs; failure/cancellation/skipping is not success. Unrelated changes explicitly report not-applicable rather than leaving a missing check.218c5634ebffb1f0ce7e8ea921b85cd5a633cde7, then the full current upgrade script. A separate new container executes the full current fresh schema. No SQL rewriting, tolerated-error list or H2 fallback is used.96272b357adds OceanBase disposable-DDL readiness probing (before real schema execution) and recognizes openGauss catalog TABLESPACE suffixes. Both have regression coverage; schema/upgrade errors are still fatal. Native index catalogs are saved with the artifacts.sql-matrixas required in repository settings; adding the workflow does not change branch protection.Verification
./mvnw.cmd -pl shenyu-admin -am test -Dtest=PluginMapperTest,PluginServiceTest,PluginControllerTest,AdminQueryIndexTest,PluginIndexUpgradeContractTest -Dsurefire.failIfNoSpecifiedTests=false -Dmaven.javadoc.skip=true: BUILD SUCCESS; 72 tests, zero failures/errors/skips; Checkstyle passed.git diff --checkpassed. Current master is included without rewriting published history.7925422b1in1dc7cb7c0; resolve five upgrade-script conflicts by retaining both this PR's indexes and master's Agent Gateway seeds. Five-dialect migration consistency checks and their eight Python regression tests passed. Fresh-schema index names and this PR's upgrade indexes have no duplicates.96272b357: 48 successful checks, three workflow-skipped checks, no failures/cancellations/pending checks. All five native jobs,list-contract,runner-testsand the aggregatesql-matrixpassed. The five native job logs each confirm BOTH upgrade and fresh flows succeeded: full SQL execution, mapper projections, sentinel metadata/JAR preservation, covered old-row-ID retention, and native index verification. Native matrix run. GitHub reports MERGEABLE.sql-matrixcheck as required; no repository settings were changed.