Skip to content

[ISSUE #6614] Fix namespace plugin enabled result mapping - #7188

Merged
Aias00 merged 2 commits into
apache:masterfrom
dengliming:fix-6614-namespace-plugin-result-map
Oct 27, 2026
Merged

Aias00 merged 2 commits into
apache:masterfrom
dengliming:fix-6614-namespace-plugin-result-map

Conversation

@dengliming

Copy link
Copy Markdown
Member

Correct the NamespacePluginRelDO base result map so the enabled database column populates the enabled property instead of overwriting sort.

Adds an H2 mapper regression test that verifies a disabled relation with a non-zero sort value is read back correctly through listByNamespaceId.

Tested with:
./mvnw -q -pl shenyu-admin -am -DskipTests=false -Dcheckstyle.skip=false -Dtest=NamespacePluginRelMapperTest -DfailIfNoTests=false test

Fixes #6614

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

Nice catch — this result-map entry was wrong in three separate ways at once:

  • column="enable" — the actual column is enabled (confirmed: Base_Column_List selects enabled, and every query in this file aliases npr.enabled AS enabled). With column="enable" the mapping silently never matched, so getEnabled() stayed at its default.
  • property="sort" — the value was being written into the sort field, which is why sort came back wrong too. This single line was corrupting two properties.
  • jdbcType="BOOLEAN" vs the TINYINT/integer column used throughout the rest of the mapper.

The corrected mapping (column="enabled" jdbcType="TINYINT" property="enabled") is consistent with the column list, the insert statements, and the Boolean enabled property on NamespacePluginRelDO.

The new mapper test is the right kind of regression test: it inserts a row with sort=37, enabled=false and asserts both survive the round-trip, so a reversion of either half of this line would fail.

Non-blocking inconsistency left behind: insertSelective declares #{enabled, jdbcType=INTEGER} (line ~476) while batchSave uses jdbcType=BOOLEAN and the update statements use TINYINT. That mismatch is pre-existing and harmless for MySQL, but consolidating on TINYINT would avoid surprises on stricter databases.

@Aias00

Aias00 commented Oct 23, 2026

Copy link
Copy Markdown
Contributor

Ping on state: this PR is approved on the code, but the required checks are red and it cannot be merged yet.

The failing jobs are e2e / e2e-case / list-contract / native-sql, which are currently failing across the repository and are tracked in #7970. The build job in this run does not fail on compilation: it stops at the gate step that consumes the e2e result ([[ "failure" == "success" ]] || exit -1), so there is nothing in this PR to fix for that.

No code change is expected from you. Once the shared e2e problem is resolved, please rerun the failed jobs (or push a trivial rebase) so the required checks go green, and this can be merged.

@Aias00

Aias00 commented Oct 24, 2026

Copy link
Copy Markdown
Contributor

@dengliming This PR is approved, but 7 checks are red: e2e, e2e-case (shenyu-e2e-case-grpc, e2e-grpc-sync-compose), e2e-case (shenyu-e2e-case-logging-rocketmq, e2e-logging-rocketmq-compose), e2e-case (shenyu-e2e-case-apache-dubbo, e2e-apache-dubbo-sync-compose) (+3 more).

I inspected these lanes today and they fail for infrastructure reasons rather than on the diff — the integrated-test jobs die while starting their containers (Elasticsearch /_cat/health returning 000 until the job is cancelled), and the http lane also hits the known CachePluginTest.testMemoryModle / testRedisModle flake. The same jobs are red on master and on unrelated PRs right now.

Please rebase onto master and push (or ask for a re-run) so the checks execute again. If a case is still failing on a fresh run, paste the test name and I'll look properly.

@Aias00

Aias00 commented Oct 25, 2026

Copy link
Copy Markdown
Contributor

@dengliming — a correction to the note I left here yesterday. The failures on this branch all predate the fix, so nothing in your change is implicated.

master went green at a8080ae: the SubscribeTest fixture isolation (#8092), the MQTT session scoping (#8093) and the pr_build dependency fix (#8075) all landed, and the latest ci.yml, integrated-test.yml and CodeQL runs on master are all success.

That means the red checks recorded here are stale. A push — an empty commit or a rebase, whichever is easier — re-runs them against the current baseline. I would not spend time reading the old logs.

For the e2e jobs specifically: /retest on a run whose shenyu-images artifact has expired now falls back to a full rerun and rebuilds it (#8094 landed), so that path works again too.

@Aias00

Aias00 commented Oct 26, 2026

Copy link
Copy Markdown
Contributor

Hi @dengliming, this PR is already approved, but it currently cannot be merged. Here is the blocker and what is needed:

Blocker
This PR is approved but blocked. The e2e / apache-dubbo case fails from a stale base.

What we need
Please rebase onto current master and re-run CI.

Once the branch is up to date and CI is green, we will squash-merge it. Thanks!

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

Additional local validation of the already-approved commit 3b6888f: NamespacePluginRelMapperTest passed all 6 tests in an isolated worktree, with zero failures/errors/skips. The H2-backed integration test confirms that enabled=false is returned correctly and sort remains 37. No new blocking findings; my existing approval still applies.

Ran the Maven wrapper with JDK 21 and project compiler settings; Checkstyle passed, RAT/JaCoCo skipped for this targeted run. Other database engines were not exercised.

@Aias00
Aias00 merged commit 2b9bbb1 into apache:master Oct 27, 2026
55 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] namespace_plugin_rel resultMap maps wrong column to wrong property (enabled never populated)

2 participants