Repository navigation
Conversation
Aias00
left a comment
There was a problem hiding this comment.
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")inSelectorDTO/RuleDTO,@SerializedName(value = "logged", alternate = "loged")inRateLimiterHandle,- the five
ALTER TABLE ... RENAME COLUMNstatements.
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.
@JsonAliasis deserialization-only, so old REST requests postinglogedkeep working while responses always emitlogged— exactly as documented. Worth noting the alias is load-bearing, not cosmetic:loggedcarries@NotNull, so without it old clients would get a 400 rather than a silently-null field.- For
RateLimiterHandleI confirmed the only parse site isRateLimiterPluginDataHandler.java:88(GsonUtils.getInstance().fromJson(s, RateLimiterHandle.class)), so the Gsonalternateis 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, leavingRuleData.logged == null.AbstractShenyuPlugin.printLog(RuleData, String)then doesif (ruleData.getLogged())(AbstractShenyuPlugin.java:352) — unboxing a nullBoolean. That call sits on the request path atAbstractShenyuPlugin.java:116and: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
logedstays null and its ownprintLogNPEs. 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")toRuleData.logged(one line, mirrors what you already did forRateLimiterHandle, and Gson is the only deserializer on this path), and - make
printLognull-safe (Boolean.TRUE.equals(...)) as defence in depth —LocalPluginController.buildDefaultRuleDataalready 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
e2ejob that failed in 3s (job116840545041) is the fail-fast aggregator — its log is just[[ "failure" == "success" ]] || exit -1. - The real failure is
e2e-storage (mysql)(job116836501899):DividePluginTest.testDivide:105->lambda$testDivide$1:105 endpoint '/anything/xx/yy' not exists, casesingle-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.javais empty). - Master hit the identical assertion three hours earlier on a different engine: run
38836735684(head297ac6f8), job116769167809(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-dashboardsource 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.
…inting null-safe (apache#7589)
|
Hi @Aias00, thank you for the thorough review — the
Minor: the matching one-word change in the dashboard source is ready as apache/shenyu-dashboard#723 (3 files, 18 occurrences: Verification: |
Aias00
left a comment
There was a problem hiding this comment.
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
RuleDatasync-contract compatibility —RuleData.java:41-42now carries@SerializedName(value = "logged", alternate = "loged"), so Gson keeps deserializing the old admin payload during a rolling upgrade;AbstractShenyuPlugin.printLog(RuleData)(line 352) now usesBoolean.TRUE.equals(ruleData.getLogged()), removing the NPE-on-the-hot-path risk. The release note also states the required upgrade ordering.- Migration execution is now covered —
native-sql-matrix.pyapplies 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 new2.7.2-upgrade-2.7.3-*.sqlscripts (mysql/ob/og/pg/oracle) carry theRENAME COLUMNstatements. db/upgrade/upgrade-guide.mdnow lists the new migration in the "To Shenyu Users" section.
Verification
- Head source confirmed for the annotation and the null-safe
printLogguard. - CI:
pr_build,Analyze (java),check-license-header,rat_check,scope,sql-matrixpass. Thee2e/*-sync-compose/cluster-jdbcreds are the known httpbin.org / RocketMQ infra flakes, untouched by a rename.
Conclusion
APPROVE.
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.
Description
Fixes #7589.
Renames the misspelled
logedcolumn of theselectorandruletables tologged, and finishes the corresponding field renames across the codebase. The rename was already half-landed (SelectorDatain shenyu-common and the e2e models usedloggedwhileRuleDataand the admin DTOs usedloged), so this makes the naming consistent everywhere in one pass.Changes
loged→loggedin the mysql/pg/oracle init schemas, the h2 schema, and the mysql seed INSERT column list.db/upgrade/2.7.2-upgrade-2.7.3-{mysql,pg,oracle,og,ob}.sqlissuingALTER TABLE ... RENAME COLUMN loged TO loggedfor both tables, following the existing 2.7.x upgrade script conventions. Existing deployments must run the script for their dialect before upgrading.selector-sqlmap.xml/rule-sqlmap.xmlcolumn references updated (12 + 11 occurrences).SelectorDTO,RuleDTO,SelectorDO,RuleDO,SelectorVO,RuleVO, events,SelectorUtil, register services, gateway-sideRuleData,AbstractShenyuPlugin,LocalPluginController, the k8s parsers and the e2e models now uselogged/getLogged/setLoggedconsistently. Tests updated accordingly (no assertion changes beyond the rename).SelectorDTO/RuleDTOcarry@JsonAlias("loged"), so REST requests posting the old field name keep working; responses now always serializelogged.RateLimiterHandleuses@SerializedName(value = "logged", alternate = "loged"), so rate-limiter rule handle JSON stored with the old key by existing deployments keeps parsing; new writes uselogged(Gson writes the primary name only, so re-saved handles converge tologged)."loged"form-field occurrences in the checked-inindex.f5d66cf1.jsbundle. Note for maintainers: the upstreamshenyu-dashboardsource needs the matching one-word change in a follow-up (I can link a PR there if wanted).Unreleased → Behavior Changeswith the upgrade-script requirement and the REST response field change.Verbatim check
git grep -in logedreturns 0 hits after this change.Checklist
./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