Skip to content

Fixes #6806: Skip the plugin jar blob in list queries and index snapshot joins - #7360

Merged
Aias00 merged 16 commits into
apache:masterfrom
BobSong-dev:fix/6806-plugin-list-indexes
Oct 8, 2026
Merged

Aias00 merged 16 commits into
apache:masterfrom
BobSong-dev:fix/6806-plugin-list-indexes

Conversation

@BobSong-dev

@BobSong-dev BobSong-dev commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

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

  • Implement review option (a): introduce a separate PluginListVO with no JAR/file field. GET /plugin-template and POST /plugin-template/list/search and /plugin-template/list/search/adaptor return this metadata-only list contract. The paged response intentionally omits file instead of reporting a misleading empty string. Clients needing JAR content must use the detail/export paths.
  • Both paged mapper queries select the shared non-BLOB column fragment. Detail and export retain PluginVO and the full column fragment, including plugin_jar.
  • Verify the response schema at controller and real H2 mapper/service levels, using a nonempty binary JAR. Lists omit the JAR field; detail/export preserve identical bytes.
  • Add missing snapshot/permission indexes without duplicating indexes already supplied by master. Preserve the upstream metadata namespace/path index. Use CREATE INDEX IF NOT EXISTS for 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.
  • Preserve the independently authorized test-only RocketMQ readiness fix: await exact created selector/rule IDs before the single request, without relaxing log assertions or consumption timeouts. No unrelated feature-branch cherry-picks are included; changes already merged into master are preserved.

Upgrade coverage follow-up

  • PluginIndexUpgradeContractTest reads 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/openGauss IF NOT EXISTS clauses. This is automated script-contract coverage, not native database execution or validation of the entire migration.
  • AdminQueryIndexTest now also checks idx_selector_plugin_id(plugin_id) and idx_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.
  • No production SQL or API behavior changed in this follow-up.

Native SQL merge gate

  • Follow-up ffa4ce140 adds a separate native-sql-matrix workflow and stable aggregate sql-matrix check. 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.
  • Each digest-pinned engine executes the full released 2.7.1 schema at commit 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.
  • Check native mapper projections, all sentinel metadata and nonempty JAR bytes, retention of old row IDs in the covered core tables, and native leading-column index metadata. This is not a full-table content fingerprint or whole-migration idempotency test, and selected engine versions do not certify every vendor version.
  • Follow-up 96272b357 adds 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.
  • Matrix jobs do not cancel siblings on failure; artifacts retain results/durations/startup logs. Maintainers must validate runtime/resource use and configure sql-matrix as required in repository settings; adding the workflow does not change branch protection.
  • Local harness verification: 20 offline regression tests, actionlint, YAML/shell checks and Admin RAT passed. These do not constitute native database execution. Local Docker daemon remains unavailable.

Verification

  • Local: ./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.
  • Local: Admin Apache RAT passed (zero unapproved/unknown licenses); git diff --check passed. Current master is included without rewriting published history.
  • Merge current master 7925422b1 in 1dc7cb7c0; 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.
  • Local verification rerun after the merge: 72 Admin tests, Checkstyle and Admin RAT passed on the coverage follow-up (1016 approved licenses, zero unapproved/unknown). The JaCoCo report warned about old execution data from other branch builds; this did not fail the tests/build, and no fresh coverage claim is made.
  • GitHub CI verified on 96272b357: 48 successful checks, three workflow-skipped checks, no failures/cancellations/pending checks. All five native jobs, list-contract, runner-tests and the aggregate sql-matrix passed. 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.
  • Not run locally: full-project/Docker e2e or native migrations (local Docker daemon is unavailable). Native upgrade/fresh execution was performed successfully in GitHub CI for the selected MySQL 8.0, OceanBase 4.3.5 LTS MySQL mode, PostgreSQL 15, openGauss 5.0.1 and Oracle Free 23.26.3 images. No all-version or full-production-content certification is claimed. Maintainers still need to configure the successful sql-matrix check as required; no repository settings were changed.

@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.

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 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.

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 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.

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.

@Aias00

Aias00 commented Oct 1, 2026

Copy link
Copy Markdown
Contributor

Re-checked after the new commits. The SQL fragment restructure is cleaner (Base_Column_List now composes List_Column_List + plugin_jar), but the point I raised is still open: selectByQuery uses List_Column_List, which does not include plugin_jar.

The chain is unchanged — PluginServiceImpl#listByPage loads via pluginMapper.selectByQuery(...) and maps with PluginVO::buildPluginVO, which sets pluginJar from Optional.ofNullable(pluginDO.getPluginJar()).map(Base64::encodeToString).orElse(""). So the paginated plugin list will still report jar: "" for every row.

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 plugin_jar in selectByQuery, or (c) reply here confirming the admin UI never reads jar from the list response, and I'll take that as the answer. I'm keeping this at request-changes until one of those happens.

@BobSong-dev

Copy link
Copy Markdown
Contributor Author

Re-checked after the new commits. The SQL fragment restructure is cleaner (Base_Column_List now composes List_Column_List + plugin_jar), but the point I raised is still open: selectByQuery uses List_Column_List, which does not include plugin_jar.

The chain is unchanged — PluginServiceImpl#listByPage loads via pluginMapper.selectByQuery(...) and maps with PluginVO::buildPluginVO, which sets pluginJar from Optional.ofNullable(pluginDO.getPluginJar()).map(Base64::encodeToString).orElse(""). So the paginated plugin list will still report jar: "" for every row.

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 plugin_jar in selectByQuery, or (c) reply here confirming the admin UI never reads jar from the list response, and I'll take that as the answer. I'm keeping this at request-changes until one of those happens.

sorry,I missed it.The problem has been resolved.Thanks for your review.

Aias00
Aias00 previously approved these changes Oct 1, 2026

@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.

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.

@sunnysabor

Copy link
Copy Markdown
Contributor

I checked the current conflict against upstream/master (09c6a528) and PR head b580baad2 with git merge-tree. The only conflicts are the five 2.7.1-to-2.7.2 upgrade files for MySQL, OceanBase, openGauss, Oracle, and PostgreSQL; the five corresponding initialization schemas and the Java/XML/test files auto-merge.

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 IF NOT EXISTS form. No production or test conflict was reported by the merge-tree check.

…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 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.

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.

@BobSong-dev

Copy link
Copy Markdown
Contributor Author

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 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.

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 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.

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

@Aias00
Aias00 merged commit bf2f583 into apache:master Oct 8, 2026
51 checks passed
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] — plugin list/snapshot: plugin_jar BLOB in list + unindexed selector.plugin_id join + correlated subqueries

3 participants