Skip to content

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

Open
BobSong-dev wants to merge 12 commits into
apache:masterfrom
BobSong-dev:fix/6806-plugin-list-indexes
Open

BobSong-dev wants to merge 12 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 Agent Gateway production code or branch history is included.

Verification

  • Local: ./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.
  • Local: Admin Apache RAT passed (zero unapproved/unknown licenses); git diff --check passed. Current master is included without rewriting published history.
  • GitHub CI: latest head b580baad2 has 34 successful checks and three skipped checks. MySQL storage e2e failed in the POST /anything scenario after the external HTTPBin upstream returned HTML 502 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.
  • Not run locally: full-project/Docker tests or native upgrade migrations on database engines other than H2. The H2 check executes the fresh-install schema, not the other dialects' upgrade scripts.

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

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] — plugin list/snapshot: plugin_jar BLOB in list + unindexed selector.plugin_id join + correlated subqueries

3 participants