Skip to content

refactor(admin): rename misspelled loged column/field to logged in selector and rule (#7589) - #7677

Open
konlue wants to merge 2 commits into
apache:masterfrom
konlue:cleanup/rename-loged-to-logged
Open

konlue wants to merge 2 commits into
apache:masterfrom
konlue:cleanup/rename-loged-to-logged

Conversation

@konlue

@konlue konlue commented Oct 16, 2026

Copy link
Copy Markdown
Contributor

Description

Fixes #7589.

Renames the misspelled loged column of the selector and rule tables to logged, and finishes the corresponding field renames across the codebase. The rename was already half-landed (SelectorData in shenyu-common and the e2e models used logged while RuleData and the admin DTOs used loged), so this makes the naming consistent everywhere in one pass.

Changes

  • Schemas: loged → logged in the mysql/pg/oracle init schemas, the h2 schema, and the mysql seed INSERT column list.
  • Upgrade scripts: new db/upgrade/2.7.2-upgrade-2.7.3-{mysql,pg,oracle,og,ob}.sql issuing ALTER TABLE ... RENAME COLUMN loged TO logged for both tables, following the existing 2.7.x upgrade script conventions. Existing deployments must run the script for their dialect before upgrading.
  • Mappers: selector-sqlmap.xml / rule-sqlmap.xml column references updated (12 + 11 occurrences).
  • Java: SelectorDTO, RuleDTO, SelectorDO, RuleDO, SelectorVO, RuleVO, events, SelectorUtil, register services, gateway-side RuleData, AbstractShenyuPlugin, LocalPluginController, the k8s parsers and the e2e models now use logged/getLogged/setLogged consistently. Tests updated accordingly (no assertion changes beyond the rename).
  • Compatibility (one release):
    • SelectorDTO/RuleDTO carry @JsonAlias("loged"), so REST requests posting the old field name keep working; responses now always serialize logged.
    • RateLimiterHandle uses @SerializedName(value = "logged", alternate = "loged"), so rate-limiter rule handle JSON stored with the old key by existing deployments keeps parsing; new writes use logged (Gson writes the primary name only, so re-saved handles converge to logged).
  • Admin console bundle: updated the 3 "loged" form-field occurrences in the checked-in index.f5d66cf1.js bundle. Note for maintainers: the upstream shenyu-dashboard source needs the matching one-word change in a follow-up (I can link a PR there if wanted).
  • Release notes: recorded under Unreleased → Behavior Changes with the upgrade-script requirement and the REST response field change.

Verbatim check

git grep -in loged returns 0 hits after this change.

Checklist

  • I have read the CONTRIBUTING doc.
  • I have updated the documentation accordingly (release notes + upgrade scripts).
  • ./mvnw -B install -DskipTests -Dmaven.javadoc.skip=true -pl shenyu-admin,shenyu-kubernetes-controller -am → BUILD SUCCESS
  • ./mvnw -B test -pl shenyu-admin -Dtest='*Selector*,*Rule*,*Register*,*RateLimiter*' → 407 tests, 0 failures; ./mvnw -B test -pl shenyu-common,shenyu-plugin/shenyu-plugin-base,shenyu-web → all green; shenyu-plugin-ratelimiter -am → 37/37; shenyu-plugin-modify-response,shenyu-plugin-wasm-base -am → all green

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

Request changes — three items, all mechanical. The rename itself is complete and clean (details in "What I verified"); my asks are about the two compatibility edges the PR does not yet cover and one doc file.

What I verified

The rename is complete. I grepped the full tree at cf0d8330 case-insensitively for loged. The only remaining hits are the intentional ones:

  • RELEASE-NOTES.md (prose describing the rename),
  • @JsonAlias("loged") in SelectorDTO / RuleDTO,
  • @SerializedName(value = "logged", alternate = "loged") in RateLimiterHandle,
  • the five ALTER TABLE ... RENAME COLUMN statements.

Everything else moved: the four init schemas plus the MySQL seed INSERT column list, the h2 schema, both sqlmaps (result column, plain select lists, INSERT column+values, the dynamic <if> update columns and the UPDATE ... SET clause), SelectorDO/RuleDO/SelectorDTO/RuleDTO/SelectorVO/RuleVO, the two changed events, SelectorUtil, both register services, gateway-side RuleData, AbstractShenyuPlugin, LocalPluginController, all seven k8s parsers, the e2e models, the tests, and the minified dashboard bundle (one line, all three occurrences).

Both compatibility mechanisms are correctly wired.

  • @JsonAlias is deserialization-only, so old REST requests posting loged keep working while responses always emit logged — exactly as documented. Worth noting the alias is load-bearing, not cosmetic: logged carries @NotNull, so without it old clients would get a 400 rather than a silently-null field.
  • For RateLimiterHandle I confirmed the only parse site is RateLimiterPluginDataHandler.java:88 (GsonUtils.getInstance().fromJson(s, RateLimiterHandle.class)), so the Gson alternate is effective, and Gson serializes the primary name only, matching your convergence claim.

Blocking

1. RuleData.logged is a wire-format rename with no compatibility, and the failure mode is an NPE on the hot path

RuleData is the admin→bootstrap sync contract. On master it is private Boolean loged; (RuleData.java:39); this PR renames it to logged. Both sync paths deserialize with Gson, which silently drops unknown keys:

  • HTTP long-poll: RuleDataRefresh.java:55-56 → GsonUtils.getGson().fromJson(data, new TypeToken<ConfigData<RuleData>>() {}.getType())
  • WebSocket: ShenyuWebsocketClient.java:442 → GsonUtils.getInstance().fromJson(result, WebsocketData.class)

So during a rolling upgrade (admin and bootstrap are separate processes, usually not restarted together):

  • new bootstrap + old admin: the old admin emits "loged": true; the new bootstrap drops it, leaving RuleData.logged == null. AbstractShenyuPlugin.printLog(RuleData, String) then does if (ruleData.getLogged()) (AbstractShenyuPlugin.java:352) — unboxing a null Boolean. That call sits on the request path at AbstractShenyuPlugin.java:116 and :132, i.e. every matched rule throws NPE, which would take the gateway down for all traffic.
  • new admin + old bootstrap: symmetric — the old bootstrap's loged stays null and its own printLog NPEs. This direction cannot be fixed in this repo, it has to be handled by upgrade ordering.

This is inconsistent with the rest of the PR: you protected the nested rate-limiter handle and the REST DTOs, but not the outer field that carries them. Two cheap options, either is fine:

  • add @SerializedName(value = "logged", alternate = "loged") to RuleData.logged (one line, mirrors what you already did for RateLimiterHandle, and Gson is the only deserializer on this path), and
  • make printLog null-safe (Boolean.TRUE.equals(...)) as defence in depth — LocalPluginController.buildDefaultRuleData already guards this field, so the guard is clearly considered necessary elsewhere.

For context on why I still think this is worth doing despite the precedent: SelectorData.logged was renamed the same way back in the 2.5.0 cycle (it is already logged at tag v2.5.0, and no historical db/upgrade script ever mentioned loged), so the project has shipped this pattern before. But that rename predates the compat discipline this PR applies elsewhere. At minimum, please state the required upgrade order explicitly in the release note ("upgrade all bootstrap instances before, or together with, admin"), since the new-admin/old-bootstrap direction is unfixable in code.

2. The five new upgrade scripts are not executed by any test

.github/scripts/native-sql-matrix.py:469 hardcodes the migration file as db/upgrade/2.7.1-upgrade-2.7.2-{dialect}.sql and pins BASELINE to v2.7.1 (:27). The native-sql (mysql / ob / pg / og / oracle) checks that passed therefore ran the old upgrade script plus the fresh-install schema — the new ALTER TABLE ... RENAME COLUMN statements were never executed on a real engine. The fresh-install flow does cover the renamed schema columns, so the schema half is verified; the migration half is not.

These are one-shot migrations that operators must run before upgrading, and the harness aborts on any SQL error, so extending it is a genuine gate rather than paperwork. Please either extend the matrix to also execute the newest upgrade script (baseline bumped to the PR head, both scripts in sequence), or verify each of the five manually on a real engine and paste the results into the PR.

3. db/upgrade/upgrade-guide.md still points at the previous migration

The guide's "To Shenyu Users" section (upgrade-guide.md:9-27) still tells operators to run 2.7.1-upgrade-2.7.2-<dialect>.sql and describes the 2.7.2 data-permission change. Because this rename makes running an upgrade script mandatory for existing deployments, the new scripts need to be listed there in the same bullet format — otherwise an operator following the guide will skip the migration and the new admin will fail against the old schema.

CI status

The red is a pre-existing flake, not a regression:

  • The e2e job that failed in 3s (job 116840545041) is the fail-fast aggregator — its log is just [[ "failure" == "success" ]] || exit -1.
  • The real failure is e2e-storage (mysql) (job 116836501899): DividePluginTest.testDivide:105->lambda$testDivide$1:105 endpoint '/anything/xx/yy' not exists, case single-divide uri starts_with. That test file and its case provider are untouched by this PR (git diff origin/master cf0d8330 -- .../testcase/divide/DividePluginTest.java .../DividePluginCases.java is empty).
  • Master hit the identical assertion three hours earlier on a different engine: run 38836735684 (head 297ac6f8), job 116769167809 (postgres) — same message, same endpoint. The three most recent master runs are green.

So no code change is needed for CI, but the run does need to be re-triggered (the fail-fast cancellation also took out the h2/pg/og storage siblings) before merge.

Minor

  • The dashboard bundle edit is fine, but as you note the matching one-word change in the shenyu-dashboard source should land (and be linked) before this bundle gets rebuilt, otherwise the next dashboard release will silently revert the fix.

Once the three blocking items are addressed and a clean e2e run lands, this is good to merge — no other code concerns from me.

@konlue

konlue commented Oct 17, 2026

Copy link
Copy Markdown
Contributor Author

Hi @Aias00, thank you for the thorough review — the RuleData wire-format catch in particular was exactly the edge I missed. All three items are addressed:

  1. RuleData sync-contract compatibility

    • RuleData.logged now carries @SerializedName(value = "logged", alternate = "loged") — the same mechanism as RateLimiterHandle, and Gson is the deserializer on both sync paths.
    • AbstractShenyuPlugin.printLog now uses Boolean.TRUE.equals(ruleData.getLogged()) as defence in depth.
    • The release note states the required upgrade order explicitly: “upgrade all bootstrap instances before, or together with, admin”, since the new-admin/old-bootstrap direction cannot be tolerated in code.
    • Red-green verified both ways:
      • testDeserializationAcceptsLegacyLogedKey fails without the alternate when parsing legacy {"loged": true} sync JSON.
      • printLogToleratesNullLoggedFlag throws NPE without the guard when invoking printLog with a null flag.
  2. Upgrade scripts gated on real engines

    • native-sql-matrix.py now executes both scripts in sequence on top of the pinned v2.7.1 baseline:
      MIGRATIONS_FROM_BASELINE = ("2.7.1-upgrade-2.7.2", "2.7.2-upgrade-2.7.3"), keeping abort-on-SQL-error.
    • The native-sql jobs on this PR will therefore run the ALTER TABLE ... RENAME COLUMN statements on all five engines.
    • The migration files are read from the PR tree, so future additions only need one list entry.
    • The matrix’s post-migration assertions only read plugin columns and table ids, so the renamed selector/rule columns do not disturb them.
  3. Upgrade guide

    • db/upgrade/upgrade-guide.md gained a “loged column renamed to logged in 2.7.3” section listing all five scripts as mandatory.
    • It includes the upgrade-order note and the stop-admin/run/restart sequence mirroring the 2.7.2 section.

Minor: the matching one-word change in the dashboard source is ready as apache/shenyu-dashboard#723 (3 files, 18 occurrences: Selector.js, Rule.js, ToolsModal.js).

Verification: shenyu-common + shenyu-plugin-base reactor tests 250/250 green with checkstyle. I agree the previous e2e red was the pre-existing DividePluginTest flake; I’ll watch the new run and re-trigger if it recurs.

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

Summary

Re-review at current head 6ffa0d8107. The loged → logged rename is complete and all three items from my previous review are now addressed.

Resolved

  1. RuleData sync-contract compatibility — RuleData.java:41-42 now carries @SerializedName(value = "logged", alternate = "loged"), so Gson keeps deserializing the old admin payload during a rolling upgrade; AbstractShenyuPlugin.printLog(RuleData) (line 352) now uses Boolean.TRUE.equals(ruleData.getLogged()), removing the NPE-on-the-hot-path risk. The release note also states the required upgrade ordering.
  2. Migration execution is now covered — native-sql-matrix.py applies the upgrade chain from the v2.7.1 baseline (MIGRATIONS_FROM_BASELINE = ("2.7.1-upgrade-2.7.2", "2.7.2-upgrade-2.7.3")), and the new 2.7.2-upgrade-2.7.3-*.sql scripts (mysql/ob/og/pg/oracle) carry the RENAME COLUMN statements.
  3. db/upgrade/upgrade-guide.md now lists the new migration in the "To Shenyu Users" section.

Verification

  • Head source confirmed for the annotation and the null-safe printLog guard.
  • CI: pr_build, Analyze (java), check-license-header, rat_check, scope, sql-matrix pass. The e2e / *-sync-compose / cluster-jdbc reds are the known httpbin.org / RocketMQ infra flakes, untouched by a rename.

Conclusion

APPROVE.

yangjj-iso pushed a commit to yangjj-iso/shenyu that referenced this pull request Oct 18, 2026
Integrate apache#7677 and preserve current master changes. Use the renamed builder in the file parser, assert logging defaults, and adapt the new base-plugin tests. The duplicate Apollo selector test fix was dropped when rebasing onto master, which already contains apache#7680.

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.

[Cleanup] Rename the misspelled loged column/field to logged in selector and rule

2 participants