fix(core): honor fastjson2.properties when -D system property is absent - #7763
fix(core): honor fastjson2.properties when -D system property is absent#7763fudianchn wants to merge 1 commit into
Conversation
wenshao
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
中文说明
已审查——无阻断问题。 建议见行内评论。
— qwen3.8-max via Qwen Code /review (v0.21.5)
| String name = "fastjson2.useJacksonAnnotation"; | ||
| String previous = System.clearProperty(name); |
There was a problem hiding this comment.
[Suggestion] R1-1: The regression tests only cover "system property absent → file value honored". The other half of the resolution contract — a non-empty -D system property wins over fastjson2.properties, and an empty/blank system property still falls back to the file — is pinned by no test anywhere in the core test tree, even though this PR restructured exactly those guard branches and the description states "The -D system property still wins when set". — Concrete cost: if a future change inverts the lookup order (file read unconditional, or before System.getProperty), a packaged fastjson2.properties value would silently override an explicit -D value (e.g. -Dfastjson2.writer.maxLevel=4096 with fastjson2.writer.maxLevel=512 in the file would silently adopt 512) while every assertion here stays green. Probe-verified: the current code honors -D precedence, and an inverted-order variant flips it while all Issue7757-direction assertions remain green. One case per helper inside the existing try blocks is enough:
// A non-empty -D system property wins over the properties file.
System.setProperty(name, "true");
assertTrue(getPropertyBool(properties, name, false));
// An empty/blank -D system property still falls back to the file.
System.setProperty(name, " ");
assertFalse(getPropertyBool(properties, name, true));(Note the finally must then restore unconditionally — System.clearProperty(name) when previous == null — because these cases set the property.)
中文说明
回归测试目前只覆盖了「系统属性缺失 → 配置文件值生效」这一半解析契约;另一半——非空 -D 系统属性优先于 fastjson2.properties、空串/空白系统属性仍回退到文件——在整个 core 测试树中没有任何测试钉住,而本 PR 恰恰重构了编码该优先级的守卫分支,且 PR 描述声称「-D 系统属性设置时仍然优先」。具体代价:若未来改动颠倒查找顺序(无条件先读文件,或先于 System.getProperty 读取),打包的 fastjson2.properties 值会静默覆盖显式 -D 值(如 -Dfastjson2.writer.maxLevel=4096 与文件中 maxLevel=512 并存时会静默采用 512),而此处所有断言仍为绿色。已用探针验证:当前代码遵守 -D 优先级;颠倒顺序的变体会翻转结果,而 Issue7757 方向的断言全部保持绿色。每个 helper 在现有 try 块内各补一个用例即可(如上代码)。注意:补充用例后 finally 需无条件恢复——previous == null 时执行 System.clearProperty(name),因为这些用例会设置系统属性。
— qwen3.8-max via Qwen Code /review (v0.21.5)
There was a problem hiding this comment.
Done. Issue7757 now pins the other half too: a non-empty -D value wins over fastjson2.properties, and an empty/blank -D value still falls back to the file (both helpers). The finally restores unconditionally (System.clearProperty(name) when previous == null), since these cases set the property.
中文
已补。Issue7757 现在也钉住另一半契约:非空 -D 优先于 fastjson2.properties、空/空白 -D 回退文件(两个 helper 均覆盖)。finally 改为无条件恢复(previous == null 时 System.clearProperty)。
| if (property == null || property.isEmpty()) { | ||
| property = properties.getProperty(name); | ||
| if (property != null) { | ||
| property = property.trim(); | ||
| } | ||
| } | ||
| if (property != null) { |
There was a problem hiding this comment.
[Suggestion] R1-2: After this PR, the system-property-first / trim / null-or-empty-fallback-to-fastjson2.properties resolution sequence exists verbatim in three places in this file — the fastjson2.creator block (~L167-180), getPropertyBool (~L246-255), and getPropertyInt (~L274-283) — with no shared helper. — Concrete cost: this PR's own bug is the scenario — the creator block carried the correct null-or-empty file fallback while the two typed helpers diverged into an isEmpty-only variant, and the inconsistency went unnoticed until a user report. With three copies, the next change to resolution semantics must sync all three sites inside one 878-line file; missing one reintroduces the same silent-configuration-ignored class of bug. Suggested extraction (all three sites are cold class-init paths — no performance impact):
private static String resolveProperty(Properties properties, String name) {
String property = System.getProperty(name);
if (property != null) {
property = property.trim();
}
if (property == null || property.isEmpty()) {
property = properties.getProperty(name);
if (property != null) {
property = property.trim();
}
}
return property;
}中文说明
本 PR 落地后,「先系统属性 / trim / null 或空串时回退 fastjson2.properties」的解析序列在本文件中逐字出现了三处——fastjson2.creator 块(约 L167-180)、getPropertyBool(约 L246-255)、getPropertyInt(约 L274-283)——且没有共享 helper。具体代价:本 PR 修复的正是这类漂移——creator 块早已是正确的 null-or-empty 回退形态,而两个 typed helper 漂移成了仅空串回退,直到用户报告才被发现。三处复制意味着下一次解析语义变更必须在这个 878 行的文件里同步三处,遗漏任何一处都会复现同一类「配置被静默忽略」的 bug。建议提取共享 resolver(如上代码),三处均为类初始化冷路径,无性能影响。
— qwen3.8-max via Qwen Code /review (v0.21.5)
There was a problem hiding this comment.
Done. Extracted resolveProperty(properties, name) and routed the fastjson2.creator initializer, getPropertyBool and getPropertyInt through it, so the resolution order lives in one place and can't drift again.
中文
已提取 resolveProperty(properties, name),fastjson2.creator 初始化块、getPropertyBool、getPropertyInt 三处统一走它,解析顺序只此一处,不会再漂移(本 PR 修的正是这种漂移)。
| if (property == null || property.isEmpty()) { | ||
| property = properties.getProperty(name); | ||
| if (property != null) { | ||
| property = property.trim(); | ||
| } | ||
| } | ||
| try { |
There was a problem hiding this comment.
[Suggestion] R1-3: This PR added an explicit if (property != null) guard before parsing in getPropertyBool, but its twin getPropertyInt — normalized in the same commit — lacks the guard and keeps passing a possibly-null property into Integer.parseInt inside the try/catch: asymmetric shapes between the two siblings this PR just aligned. — Concrete cost: behavior is correct today (null → NumberFormatException → caught → default), but that is the common case for fastjson2.writer.maxLevel (core ships no fastjson2.properties, so a default deployment takes this path on every init), so class init uses exception-as-control-flow for its ordinary input (measured ~305ns vs ~20ns per call — one-time, negligible in absolute terms), and the asymmetry misinforms the next maintainer about which shape is intended:
if (property != null) {
try {
propertyValue = Integer.parseInt(property);
} catch (NumberFormatException ignored) {
// ignore
}
}中文说明
本 PR 在 getPropertyBool 的解析前新增了显式 if (property != null) 守卫,但同一提交中规范化的孪生方法 getPropertyInt 没有该守卫,仍把可能为 null 的 property 传入 try/catch 内的 Integer.parseInt——两个刚被对齐的兄弟方法形态不对称。具体代价:当前行为正确(null → NumberFormatException → 被捕获 → 返回默认值),但这正是 fastjson2.writer.maxLevel 的常见情形(core 不自带 fastjson2.properties,默认部署每次初始化都走该路径),即类初始化在常规输入上用异常做控制流(实测每次调用约 305ns 对 20ns——一次性执行,绝对开销可忽略),且不对称的写法会误导后续维护者对预期形态的判断。建议改为如上写法。
— qwen3.8-max via Qwen Code /review (v0.21.5)
There was a problem hiding this comment.
Done. getPropertyInt now mirrors getPropertyBool with an explicit if (property != null) guard around Integer.parseInt, dropping the NumberFormatException-as-control-flow for the common config-absent case.
中文
已对齐。getPropertyInt 与 getPropertyBool 一样在 Integer.parseInt 外加了 if (property != null) 守卫,常规配置缺失不再用 NumberFormatException 做控制流。
wenshao
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
中文说明
已审查——无阻断问题。 建议见行内评论。
— glm-5.2 via Qwen Code /review (v0.21.10)
| * {@link #getPropertyBool} and {@link #getPropertyInt} so the resolution order | ||
| * cannot drift between them. | ||
| */ | ||
| private static String resolveProperty(Properties properties, String name) { |
There was a problem hiding this comment.
[Suggestion] R2-1: The fastjson2.features block (~10 lines above, lines 177-180) uses the same system-property→properties-file resolution pattern but was not routed through resolveProperty. It checks == null only (no isEmpty() check) and does not trim — so a blank/empty -Dfastjson2.features (e.g. from an unbound shell variable) silently discards any fastjson2.features entry in fastjson2.properties, while the same blank value for fastjson2.creator / useJacksonAnnotation / writer.maxLevel correctly falls back via resolveProperty. Concrete cost: the very inter-key drift this helper exists to prevent, 10 lines from a call to it.
| private static String resolveProperty(Properties properties, String name) { | |
| // Consider routing the features lookup through resolveProperty too: | |
| // String features = resolveProperty(properties, "fastjson2.features"); | |
| // (add per-element trim after split(",")) |
中文说明
[建议] R2-1:fastjson2.features 配置块(上方约10行处,177-180行)使用相同的系统参数→配置文件解析模式,但未通过 resolveProperty 统一处理。它仅检查 == null(无 isEmpty() 判断)且不做 trim——因此空白/空的 -Dfastjson2.features(例如来自未设置的 shell 变量)会静默丢弃 fastjson2.properties 中的 fastjson2.features 配置,而同样的空白值对于 fastjson2.creator / useJacksonAnnotation / writer.maxLevel 则能通过 resolveProperty 正确回退。这正是该 helper 要防止的配置键间漂移,且就在调用它的10行之外。
— glm-5.2 via Qwen Code /review (v0.21.10)
There was a problem hiding this comment.
Done. fastjson2.features now resolves through resolveProperty too, so a blank/empty -Dfastjson2.features (e.g. an unbound shell var) falls back to fastjson2.properties instead of silently discarding it, matching the other keys. Each comma-separated element is trimmed before the switch, so disableJSONB, disableAutoType (space after comma) now also matches. A non-empty -Dfastjson2.features still wins.
The feature-set parse itself runs once in the JSONFactory static initializer, so it isn't independently unit-testable without a refactor; the resolution it now shares is covered by Issue7757, and the per-element trim only widens the match (exact matches unchanged).
607d0ac to
9c91adf
Compare
wenshao
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
Not explored to full depth (tool budget reached): "PR 概述:本 PR 修复 fastjson2 的 JSONFactory——当对应 -D 系统参数未设置时,让…": none — all planned checks completed within budget..
中文说明
已审查。 建议见行内评论。
未探索到全部深度(达到工具调用预算):"PR 概述:本 PR 修复 fastjson2 的 JSONFactory——当对应 -D 系统参数未设置时,让…":none — all planned checks completed within budget.。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| for (String feature : features.split(",")) { | ||
| switch (feature) { | ||
| switch (feature.trim()) { |
There was a problem hiding this comment.
[Suggestion] The fastjson2.features behavior changes this diff introduces — per-token feature.trim() and the blank/empty--D fallback to the properties file — have no test coverage at all. Issue7757 drives only getPropertyBool/getPropertyInt via reflection, never the features/creator resolution path.
Failure scenario: if a future refactor drops feature.trim() or the blank fallback, a user who writes fastjson2.features=disableSmartMatch, disableAutoType (space after the comma) in fastjson2.properties would silently stop getting disableAutoType applied — a security-hardening flag that looks configured but is off, with no failing test to signal it.
Consider extracting the feature parse into a small package-private helper and unit-testing the comma-space case, or adding a reflection/classloader-based case asserting a spaced list like "disableSmartMatch, disableAutoType" enables both flags.
中文说明
[建议] 本 diff 引入的 fastjson2.features 行为变化——逐 token 的 feature.trim() 以及空白/空 -D 回退到 properties 文件——完全没有测试覆盖。Issue7757 只通过反射驱动了 getPropertyBool/getPropertyInt,从未覆盖 features/creator 的解析路径。
失败场景:若未来重构移除了 feature.trim() 或空白回退,当用户在 fastjson2.properties 中写入 fastjson2.features=disableSmartMatch, disableAutoType(逗号后带空格)时,disableAutoType 会被静默地停止生效——这个安全加固开关看似配置了,实际已失效,且没有任何测试失败来提示。
建议将 features 解析提取为一个小的包私有辅助方法并为其编写单元测试(覆盖逗号后带空格的场景),或新增一个基于反射/类加载器的用例,断言形如 "disableSmartMatch, disableAutoType"(带空格)的列表能同时启用两个开关。
— qwen3.8-max via Qwen Code /review (v0.21.11)
There was a problem hiding this comment.
Extracted the feature parse into a package-private featureEnabled(String features, String feature) next to resolveProperty; the initializer is now five one-liner assignments and the switch is gone.
Direct tests in Issue7757:
featureListTokensAreTrimmed:"disableSmartMatch, disableAutoType"enables both flags; null, empty, all-blank and unknown tokens enable nothing.featuresResolvedFromPropertiesFile: drivesresolvePropertywith a file value"disableSmartMatch, disableAutoType "(trailing space kept byProperties.load) and a blank-Dfalling back to it.
Mutation-verified: dropping the per-token trim fails both feature tests; dropping the file-branch trim fails the padded file-value cases in intFromPropertiesFile/booleanFromPropertiesFile.
Chose extraction over a classloader-based case: the five flags are static final, so the classloader route would need an isolated loader plus a classpath-injected fastjson2.properties and still would not observe the trim logic directly.
While consolidating I noticed ObjectReaderProvider resolves fastjson.parser.safeMode / fastjson2.parser.safeMode with the same hand-rolled pattern, where a blank -D value does not fall back to the file (no trim before the isEmpty check). Out of scope for this PR; I can prepare a follow-up routing it through the same helpers if that direction is of interest.
| properties.setProperty(name, "512"); | ||
| assertEquals(512, getPropertyInt(properties, name, 2048)); |
There was a problem hiding this comment.
[Suggestion] The file-branch trim in resolveProperty is never observed by the new tests — every test value is pre-trimmed. Confirmed with a mutation probe: a mutant deleting the file-branch property = property.trim() survives the entire suite (all 10 assertions still pass); the discriminating input file="512 " flips the result 512 → default 2048.
Failure scenario: Properties.load keeps trailing whitespace, so a real fastjson2.properties entry fastjson2.writer.maxLevel=512 (trailing space) resolves to "512 ". With the trim parseInt succeeds; if the file-branch trim is ever removed, the parse throws and the value silently degrades to the default 2048 — the current tests cannot tell the two implementations apart.
Consider adding a padded file-value case per helper:
| properties.setProperty(name, "512"); | |
| assertEquals(512, getPropertyInt(properties, name, 2048)); | |
| properties.setProperty(name, "512 "); | |
| assertEquals(512, getPropertyInt(properties, name, 2048)); |
中文说明
[建议] resolveProperty 文件分支中的 trim 从未被新测试观测到——所有测试值都是预先 trim 过的。经变异探针确认:删除文件分支 property = property.trim() 的变异体能存活于整个测试套件(全部 10 个断言仍通过);能区分二者的输入 file="512 " 会把结果从 512 翻转为默认值 2048。
失败场景:Properties.load 会保留行尾空白,因此真实的 fastjson2.properties 条目 fastjson2.writer.maxLevel=512 (尾随空格)会被解析为 "512 "。有 trim 时 parseInt 成功;一旦文件分支的 trim 被移除,解析会抛异常、值被静默回退为默认值 2048——当前测试无法区分这两种实现。
建议为每个 helper 增加一个带空白填充的文件值用例:
— qwen3.8-max via Qwen Code /review (v0.21.11)
There was a problem hiding this comment.
Added the padded file-value cases: intFromPropertiesFile asserts "512 " -> 512 and booleanFromPropertiesFile asserts " false " -> false. The system-property-wins cases now use " 4096 " / " true " so the system-branch trim before parsing is observed as well.
Mutation-verified: deleting the file-branch trim in resolveProperty now fails both tests ("512 " -> 2048, " false " -> default).
9c91adf to
0b84e1c
Compare
wenshao
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
中文说明
已审查——无阻断问题。 建议见行内评论。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| System.setProperty(name, " "); | ||
| assertEquals(512, getPropertyInt(properties, name, 2048)); |
There was a problem hiding this comment.
[Suggestion] R4-1: The rewritten getPropertyInt's error branch (malformed value → catch → keep default) has no test — a mutant deleting the try/catch survives the entire suite. I enumerated every value that reaches Integer.parseInt under all tests in the repo: all parseable, so removing the catch changes no test outcome today. — Failure scenario: a future edit drops or rethrows the catch → a typo'd config (-Dfastjson2.writer.maxLevel=2O48, or fastjson2.writer.maxLevel=2048L in the properties file) lets NumberFormatException escape the static initializer → ExceptionInInitializerError, and every fastjson2 entry point becomes unusable instead of falling back to 2048 as intended.
| System.setProperty(name, " "); | |
| assertEquals(512, getPropertyInt(properties, name, 2048)); | |
| System.setProperty(name, " "); | |
| assertEquals(512, getPropertyInt(properties, name, 2048)); | |
| // A malformed value falls back to the default (pins the catch branch). | |
| properties.setProperty(name, "not-a-number"); | |
| assertEquals(2048, getPropertyInt(properties, name, 2048)); |
中文说明
重写后的 getPropertyInt 的错误分支(非法值 → catch → 保留默认值)没有任何测试覆盖——删除该 try/catch 的变异体在整个测试套件中存活。枚举了全仓测试中所有会进入 Integer.parseInt 的取值:均可解析,因此目前删除该 catch 不会改变任何测试结果。— 失败场景:未来某次修改删除或重抛该 catch → 配置笔误(-Dfastjson2.writer.maxLevel=2O48,或 properties 文件中 fastjson2.writer.maxLevel=2048L)会使 NumberFormatException 逸出静态初始化器 → ExceptionInInitializerError,导致 fastjson2 所有入口不可用,而不是按预期回退到 2048。
— qwen3.8-max via Qwen Code /review (v0.21.11)
There was a problem hiding this comment.
Done in bd439b2. intFromPropertiesFile now sets a malformed file value (not-a-number) and asserts the default 2048, pinning the catch branch. Mutation check: removing the try/catch makes the test fail with the NumberFormatException propagating through the reflective call. booleanFromPropertiesFile gained the matching case for getPropertyBool: an unrecognized file value keeps the default in both default directions, and the mutant flipping that branch fails the assertions.
| } catch (NumberFormatException ignored) { | ||
| // ignore | ||
| } |
There was a problem hiding this comment.
[Suggestion] R4-2: Malformed config values are discarded with zero diagnostics — and this PR promotes fastjson2.properties to a first-class config source feeding exactly this path. — Failure scenario: fastjson2.writer.maxLevel=2O48 (letter-O typo) or 99999999999 (overflow) → NumberFormatException swallowed, default 2048 silently used; fastjson2.useJacksonAnnotation=FALSE/False/no matches no exact lowercase token and is treated as absent (default stays). Nothing distinguishes "file not visible to the TCCL" from "key misspelled" from "value rejected"; the effective values are queryable, but the source and raw configured string are not retained anywhere.
| } catch (NumberFormatException ignored) { | |
| // ignore | |
| } | |
| } catch (NumberFormatException ignored) { | |
| System.err.println("fastjson2: invalid value for " + name + ": " + property + ", using default " + defaultValue); | |
| } |
(An equivalent warning belongs in getPropertyBool when the value is neither "true" nor "false"; if silent fallback is deliberate, a one-line comment saying so saves the next oncall the archaeology.)
中文说明
非法配置值被静默丢弃且没有任何诊断——而本 PR 将 fastjson2.properties 提升为直达该路径的一等配置来源。— 失败场景:fastjson2.writer.maxLevel=2O48(字母 O 笔误)或 99999999999(溢出)→ NumberFormatException 被吞掉,静默使用默认值 2048;fastjson2.useJacksonAnnotation=FALSE/False/no 不匹配任何精确小写 token,被视为未配置(保留默认值)。无法区分"文件对 TCCL 不可见""键名拼错""值被拒绝"三种情况;生效值虽可查询,但来源与原始配置串没有任何留存。getPropertyBool 中值既非 "true" 也非 "false" 时也应给出等价警告;若静默回退是有意设计,加一行注释说明即可为下一个值班人省去考古成本。
— qwen3.8-max via Qwen Code /review (v0.21.11)
There was a problem hiding this comment.
Done in bd439b2. getPropertyInt's catch prints fastjson2: invalid value for <name>: <value>, using default <default> (your message verbatim). getPropertyBool is restructured into true / false / else with the same warning on unrecognized tokens, keeping the default; outcomes for true/false are unchanged with either default direction. Fresh-JVM probe with a classpath fastjson2.properties: fastjson2.writer.maxLevel=2O48 -> one stderr line + 2048; fastjson2.useJacksonAnnotation=no -> one stderr line + default true kept.
| String name = "fastjson2.useJacksonAnnotation"; | ||
| String previous = System.clearProperty(name); |
There was a problem hiding this comment.
[Suggestion] R4-3: The tests clear real production -D keys before the first reflective invocation — and Method.invoke on a static method initializes the declaring class. If JSONFactory is not yet initialized when the test runs, <clinit> fires inside the clear-window and freezes the flags with the user's -D overrides discarded; the finally restore cannot un-freeze static state. — Failure scenario: a JVM launched with -Dfastjson2.useJacksonAnnotation=false and a filtered run (e.g. -Dtest=Issue7757,*Jackson*) where this class initializes JSONFactory first → the override is silently lost JVM-wide. Firing also depends on JUnit method order, and when it fires it discards overrides for all keys the clinit reads. Default CI never fires it (flag unset); it detonates only when someone debugs with exactly the flags this library documents. — Witness: three-arm probe against the PR's compiled classes — ARM 1 (cold JVM with the flag, test sequence): sysprop restored = false yet FROZEN useJacksonAnnotation = true; ARM 2 (clinit forced before the clear): flips to FROZEN useJacksonAnnotation = false; ARM 3 (no flag): does not fire.
The fix spans all three clearing tests — use test-local property names (the helpers take the name as a parameter, so identical code paths are exercised):
String name = "fastjson2.test.issue7757.bool"; // in booleanFromPropertiesFile
String name = "fastjson2.test.issue7757.int"; // in intFromPropertiesFile
String name = "fastjson2.test.issue7757.features"; // in featuresResolvedFromPropertiesFileor force initialization before any mutation, e.g. an @BeforeAll calling JSONFactory.getDefaultMaxLevel().
中文说明
测试在首次反射调用前清除了真实的生产 -D 键——而对静态方法的 Method.invoke 会触发声明类的初始化。若测试运行时 JSONFactory 尚未初始化,<clinit> 会在清除窗口内执行,冻结各标志位时丢弃用户的 -D 覆盖值;finally 恢复系统属性无法解冻静态状态。— 失败场景:JVM 以 -Dfastjson2.useJacksonAnnotation=false 启动,且过滤运行(如 -Dtest=Issue7757,*Jackson*)使本类最先初始化 JSONFactory → 覆盖值在整个 JVM 内被静默丢弃。是否触发还取决于 JUnit 方法顺序;一旦触发会丢弃 clinit 读取的所有键的覆盖值。默认 CI 从不触发(参数未设置);只有在用本库文档记载的参数调试时才会引爆。— 证据:针对本 PR 编译产物的三臂探针——ARM 1(带该参数的冷 JVM 执行测试序列):sysprop restored = false 但 FROZEN useJacksonAnnotation = true;ARM 2(清除前先强制初始化):翻转为 FROZEN useJacksonAnnotation = false;ARM 3(无参数):不触发。修复涉及三个清除系统属性的测试——使用测试专用属性名(helper 以名称为参数,执行的代码路径完全相同),或在任何修改前强制初始化(如 @BeforeAll 中调用 JSONFactory.getDefaultMaxLevel())。
— qwen3.8-max via Qwen Code /review (v0.21.11)
There was a problem hiding this comment.
Done in bd439b2, via the first option: the three clearing tests now use fastjson2.test.issue7757.bool / fastjson2.test.issue7757.int / fastjson2.test.issue7757.features, and a comment at the top of the class records why the names are test-local. No production key is cleared anymore, so class-init ordering can no longer discard a real -D override. Reproduced your ARM 1 against the compiled classes (cold JVM, -Dfastjson2.useJacksonAnnotation=false, old clear -> invoke -> restore sequence on the production key): sysprop restored = false yet FROZEN useJacksonAnnotation = true; the same probe with a test-local key keeps false. Preferred over @BeforeAll forcing initialization: it removes the hazard structurally instead of sequencing around it while still mutating real keys.
| @@ -270,21 +258,14 @@ private static boolean getPropertyBool(Properties properties, String name, boole | |||
| private static int getPropertyInt(Properties properties, String name, int defaultValue) { | |||
There was a problem hiding this comment.
[Suggestion] R4-4: getPropertyInt performs no range validation: a zero/negative fastjson2.writer.maxLevel is silently accepted — while the class's own setDefaultMaxLevel rejects <= 0 with IllegalArgumentException. This PR newly makes that path reachable from a bare fastjson2.properties (which can arrive via a third-party jar) with no -D set at all. — Failure scenario: fastjson2.writer.maxLevel=0 ("0 = unlimited" is a common config convention; parseInt("0") succeeds, so no parse-failure path fires) → defaultMaxLevel = 0 freezes at clinit, every JSONWriter.Context copies it, and the first startObject()/startArray() throws JSONException("level too large : 1") — every object/array serialization throws JVM-wide, with no diagnostic naming the offending property. The same unvalidated path existed pre-PR via -D only; the diff is what makes a file entry reach it. — Witness: four-arm A/B probe (classpath file fastjson2.writer.maxLevel=0, no -D): BASE (pre-PR) arm defaultMaxLevel=2048, serializes {"a":1} OK; PR arm defaultMaxLevel=0, serialization threw JSONException: level too large : 1; CONTROL (no file) OK; FIX arm (PR + the clamp below) defaultMaxLevel=2048, OK.
The call site (line 184) is unchanged context, so the clamp belongs where the value is consumed:
int maxLevel = getPropertyInt(properties, "fastjson2.writer.maxLevel", 2048);
defaultMaxLevel = maxLevel > 0 ? maxLevel : 2048;(or range-check inside getPropertyInt and fall back to defaultValue, complementing R4-2's diagnostic.)
中文说明
getPropertyInt 不做取值范围校验:零或负的 fastjson2.writer.maxLevel 会被静默接受——而类自身的 setDefaultMaxLevel 对 <= 0 会抛 IllegalArgumentException。本 PR 使该路径在没有 -D 时仅凭一个裸的 fastjson2.properties(可能来自第三方 jar)即可到达。— 失败场景:fastjson2.writer.maxLevel=0("0 = 无限制"是常见配置约定;parseInt("0") 成功,解析失败路径不会触发)→ defaultMaxLevel = 0 在 clinit 冻结,每个 JSONWriter.Context 都会复制它,首次 startObject()/startArray() 即抛 JSONException("level too large : 1") —— 整个 JVM 内所有对象/数组序列化全部抛出,且诊断信息不会指出问题配置项。PR 前同样缺少校验的路径仅可经 -D 到达;是本 diff 让文件配置能够触达。— 证据:四臂 A/B 探针(classpath 文件 fastjson2.writer.maxLevel=0、无 -D):BASE(PR 前)臂 defaultMaxLevel=2048,序列化 {"a":1} 正常;PR 臂 defaultMaxLevel=0,序列化抛 JSONException: level too large : 1;CONTROL(无文件)正常;FIX 臂(PR + 下方钳制)defaultMaxLevel=2048,正常。调用点(184 行)是未变更上下文,因此钳制应放在取值消费处(或直接在 getPropertyInt 内做范围校验并回退 defaultValue,与 R4-2 的诊断互补)。
— qwen3.8-max via Qwen Code /review (v0.21.11)
There was a problem hiding this comment.
Done in bd439b2, with the clamp at the consumption site plus the R4-2-style diagnostic: maxLevel <= 0 -> one stderr line and fallback to 2048. Kept at the call site rather than inside getPropertyInt because positivity is a maxLevel-specific constraint (setDefaultMaxLevel rejects <= 0 the same way) while getPropertyInt stays generic; it also covers the pre-existing -D channel (-Dfastjson2.writer.maxLevel=0 froze 0 the same way). Fresh-JVM probes with a classpath fastjson2.properties (fastjson2.writer.maxLevel=0): pre-fix getDefaultMaxLevel()=0 and toJSONString({"a":1}) throws JSONException: level too large : 1; post-fix 2048 and serialization OK; deleting the clamp reverts to the throwing behavior. If you prefer the bare two-line clamp without the diagnostic, that is a two-line change.
0b84e1c to
bd439b2
Compare
wenshao
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
中文说明
已审查——无阻断问题。 建议见行内评论。
— qwen3.8-max via Qwen Code /review (v0.21.11)
| int maxLevel = getPropertyInt(properties, "fastjson2.writer.maxLevel", 2048); | ||
| if (maxLevel <= 0) { |
There was a problem hiding this comment.
[Suggestion] The newly added maxLevel <= 0 guard branch has zero test coverage — none of the four new tests touches it, and getPropertyInt is never fed a negative value (which parses successfully and is exactly the input the guard exists to catch). The guard is the only protection against a config like -Dfastjson2.writer.maxLevel=0 (or -1, or a fastjson2.properties entry of 0) freezing defaultMaxLevel at a non-positive value, which makes every serialization throw JSONException: level too large at the first startObject. Because the branch is untested, the one-line mutation <= → < (or deleting the guard entirely) ships green, and no test has pinned the fallback to 2048. Static init runs once per JVM, so the branch is unreachable from same-JVM reflection tests like the existing ones; this PR already solved this exact problem for featureEnabled by making it a package-private static method. Verified by probe: getPropertyInt(file="-1", default=2048) -> -1 and getPropertyInt(file="0", default=2048) -> 0 with no warning — the guard in the static initializer is the only gate between a non-positive parsed value and defaultMaxLevel.
Suggested fix — mirror the featureEnabled pattern the PR already uses:
static int validMaxLevel(int parsed) {
return parsed <= 0 ? 2048 : parsed;
}then test it directly in Issue7757 (0, -1 → 2048; 1, 512 → unchanged).
中文说明
新增的 maxLevel <= 0 守卫分支没有任何测试覆盖——四个新测试没有一个触及它,getPropertyInt 也从未被喂过负值(负值能成功解析,恰恰是该守卫要拦截的输入)。该守卫是防止 -Dfastjson2.writer.maxLevel=0(或 -1、或 fastjson2.properties 中的 0)把 defaultMaxLevel 冻结为非正值的唯一防线——非正值会让每次序列化都在第一个 startObject 处抛 JSONException: level too large。由于该分支无测试,单行变异 <= → <(或整个删掉守卫)会绿灯通过,也没有测试钉住回退到 2048 的行为。静态初始化每个 JVM 只跑一次,现有的同 JVM 反射测试够不到这个分支;本 PR 已经为 featureEnabled 用「包私有静态方法」解决过完全相同的问题。探针验证:getPropertyInt(file="-1", default=2048) -> -1、getPropertyInt(file="0", default=2048) -> 0 且无警告——静态初始化器中的守卫是非正解析值与 defaultMaxLevel 之间的唯一闸门。
建议仿照本 PR 已使用的 featureEnabled 模式:提取为包私有静态方法(如上),在 Issue7757 中直接测试(0、-1 → 2048;1、512 → 不变)。
— qwen3.8-max via Qwen Code /review (v0.21.11)
There was a problem hiding this comment.
Extracted as validMaxLevel with the diagnostic moved inside it; the initializer is a single line now. New maxLevelIsValidated drives it directly: 0 and -1 -> 2048, 1 and 512 unchanged. The <= -> < mutant flips it red (expected: <2048> but was: <0>), verified locally.
| assertTrue(featureEnabled(" disableSmartMatch , unknown ", "disableSmartMatch")); | ||
| assertFalse(featureEnabled(" disableSmartMatch , unknown ", "disableAutoType")); |
There was a problem hiding this comment.
[Suggestion] featureEnabled's exact-match semantics are never pinned: no test input contains a token that shares a prefix or substring with a real feature name, so the mutation part.trim().equals(feature) → part.trim().contains(feature) (or startsWith) survives the entire test file. Compiled both arms and compared every existing input: all six print identical results under equals and the contains mutant, so a future refactor to looser matching passes green. The discriminating input is concrete: featureEnabled("disableAutoTypeExtra", "disableAutoType") returns false under equals but true under contains — measured on compiled code. These tokens gate disableAutoType and disableSmartMatch — flags where accidental enablement by a similarly-named token changes security-relevant parsing behaviour — so exactness is the property worth pinning.
| assertTrue(featureEnabled(" disableSmartMatch , unknown ", "disableSmartMatch")); | |
| assertFalse(featureEnabled(" disableSmartMatch , unknown ", "disableAutoType")); | |
| assertTrue(featureEnabled(" disableSmartMatch , unknown ", "disableSmartMatch")); | |
| assertFalse(featureEnabled(" disableSmartMatch , unknown ", "disableAutoType")); | |
| assertFalse(featureEnabled("disableAutoTypeExtra", "disableAutoType")); | |
| assertFalse(featureEnabled("disableSmartMatch2", "disableSmartMatch")); |
中文说明
featureEnabled 的精确匹配语义从未被测试钉住:没有任何测试输入包含与真实 feature 名共享前缀/子串的 token,因此变异 part.trim().equals(feature) → part.trim().contains(feature)(或 startsWith)能在整个测试套件下存活。编译并对比了两个版本:现有全部六个输入在 equals 与 contains 变异体下结果完全相同,未来若重构为更宽松的匹配会绿灯通过。判别性输入是具体的:featureEnabled("disableAutoTypeExtra", "disableAutoType") 在 equals 下返回 false,在 contains 下返回 true——已在编译后的代码上实测。这些 token 控制 disableAutoType 和 disableSmartMatch——同名 token 意外开启会改变安全相关的解析行为——因此精确匹配正是值得钉住的性质。
— qwen3.8-max via Qwen Code /review (v0.21.11)
There was a problem hiding this comment.
Added to featureListTokensAreTrimmed: disableAutoTypeExtra and disableSmartMatch2 enable nothing, and disableAutoTypeExtra, disableAutoType enables via the exact token only. The equals -> contains mutant flips these red, verified locally.
| property = properties.getProperty(name); | ||
| if (property != null) { | ||
| property = property.trim(); | ||
| } | ||
| if (defaultValue) { | ||
| if ("false".equals(property)) { | ||
| propertyValue = false; | ||
| } | ||
| } | ||
| return property; | ||
| } |
There was a problem hiding this comment.
[Suggestion] resolveProperty treats empty values asymmetrically and contradicts its own javadoc: an empty/blank -D value is treated as absent (falls through to the file), but an empty/blank fastjson2.properties entry (key=) is returned as a non-null "" — where the javadoc says "otherwise null". The leaf helpers then print a misleading "invalid value" warning for a merely-empty value while correctly keeping the default. Failure scenario: a base image or template ships fastjson2.properties with a blanked-out entry (e.g. fastjson2.useJacksonAnnotation=, a common way to "unset" a key); on every JVM startup getPropertyBool/getPropertyInt print fastjson2: invalid value for fastjson2.useJacksonAnnotation: , using default true to stderr — the diagnostic claims an invalid value where the entry is merely empty, sending operators chasing a nonexistent misconfiguration. It is also a trap for the next caller of the helper the javadoc advertises as the shared resolution entry point. Probe: resolveProperty(file="") -> "" and resolveProperty(file=" ") -> "" while resolveProperty(-D="", no file) -> null; the PR arm printed the warning above, the fixed arm below printed no warning and returned the same defaults. All three switch (JSONFactory.CREATOR) sites group case "asm": with default:, so coercing "" to null changes no observable behaviour there.
| property = properties.getProperty(name); | |
| if (property != null) { | |
| property = property.trim(); | |
| } | |
| if (defaultValue) { | |
| if ("false".equals(property)) { | |
| propertyValue = false; | |
| } | |
| } | |
| return property; | |
| } | |
| property = properties.getProperty(name); | |
| if (property != null) { | |
| property = property.trim(); | |
| } | |
| } | |
| return property == null || property.isEmpty() ? null : property; | |
| } |
(Alternative, more conservative fix: keep resolveProperty as is and skip the warning in the two leaf helpers when property.isEmpty().)
中文说明
resolveProperty 对空值的处理不对称,且与自身 javadoc 矛盾:空/空白 -D 值被视为「未设置」(回退读文件),但 fastjson2.properties 中的空值条目(key=)会作为非 null 的 "" 返回——而 javadoc 写的是「否则返回 null」。随后两个叶子 helper 会对这个「仅仅是空」的值打印误导性的 "invalid value" 警告(同时正确地保留默认值)。失败场景:基础镜像或模板里的 fastjson2.properties 带有被清空的条目(如 fastjson2.useJacksonAnnotation=,这是「取消设置」某键的常见写法);每个 JVM 启动时 getPropertyBool/getPropertyInt 都会向 stderr 打印 fastjson2: invalid value for fastjson2.useJacksonAnnotation: , using default true——条目只是空,诊断却声称值非法,让运维去排查一个不存在的错误配置。这对下一个调用者的也是个陷阱——javadoc 可是把这个 helper 宣传为共享解析入口的。探针:resolveProperty(file="") -> ""、resolveProperty(file=" ") -> "",而 resolveProperty(-D="", no file) -> null;当前代码打印上述警告,下方修复后不打印警告且返回相同默认值。三处 switch (JSONFactory.CREATOR) 均将 case "asm": 与 default: 归并,因此把 "" 归一为 null 在那里不改变任何可观察行为。
(更保守的替代修法:保持 resolveProperty 不变,在两个叶子 helper 中当 property.isEmpty() 时跳过警告。)
— qwen3.8-max via Qwen Code /review (v0.21.11)
There was a problem hiding this comment.
Adopted the primary fix: resolveProperty returns null for empty/blank values from either source, and the javadoc states the empty-counts-as-unset rule, so key= behaves like an absent entry and prints no invalid value line. Verified the coercion changes no observable CREATOR behaviour: JSONFactory:345 has no explicit case "asm" so "" falls to default, and ObjectReaderProvider/ObjectWriterProvider group case "asm": with default:. New emptyValuesCountAsUnset pins resolveProperty(file "" / " ") -> null and blank -D with no file -> null; removing the normalization flips it red (expected: but was: <>), verified locally.
bd439b2 to
4a6441b
Compare
wenshao
left a comment
There was a problem hiding this comment.
Deferred under the convergence posture (round 6, not a blocker) — recorded, not requested in this round:
core/src/main/java/com/alibaba/fastjson2/JSONFactory.java:210 — [probe] resolveProperty is private while ObjectReaderProvider hand-rolls the same resolution 5×, and the two already divergecore/src/main/java/com/alibaba/fastjson2/JSONFactory.java:233 — [probe] unknown fastjson2.features tokens silently ignored — the one config parser without a diagnosticcore/src/main/java/com/alibaba/fastjson2/JSONFactory.java:172 — [review] static initializer's production-key wiring has no test witnesscore/src/main/java/com/alibaba/fastjson2/JSONFactory.java:249 — [probe] default 2048 hardcoded three times for fastjson2.writer.maxLevel
中文说明
收敛姿态下延后(第 6 轮,非阻断)——已记录,本轮不要求修改:共 4 条(原文未翻译,列表见上方英文部分)。
— qwen3.8-max via Qwen Code /review (v0.22.0)
getPropertyBool and getPropertyInt only fell back to fastjson2.properties when the matching -D system property was set to an empty string. When it was absent (null), the common case, the outer `if (property != null)` guard skipped the file lookup entirely, so file-only config such as fastjson2.useJacksonAnnotation=false (e.g. to scope one WAR in a shared-JVM deployment) was silently ignored. Extract resolveProperty(properties, name) as the single system-property-first / trim / null-or-empty-fallback-to-file resolver and route the fastjson2.creator initializer, getPropertyBool, getPropertyInt and the fastjson2.features block through it, so the resolution order lives in one place and cannot drift between keys (the bug fixed here was exactly such drift). Give getPropertyInt the same explicit null guard getPropertyBool already has, and trim each comma-separated element in the features parse. Config values that cannot be used are no longer dropped silently: a malformed int or a bool other than true/false prints a one-line System.err diagnostic and keeps the default, and fastjson2.writer.maxLevel <= 0 (setDefaultMaxLevel rejects it as well) falls back to 2048 instead of freezing an unusable level at class initialization. Issue7757 covers the resolution contract: file value honored when -D is absent, default kept when both are absent, a non-empty -D value wins, an empty/blank -D value still falls back to the file, and the malformed / unrecognized fallback branches keep the default. The tests use test-local property names so no production -D key is cleared while JSONFactory may still be uninitialized. Fixes alibaba#7757 Signed-off-by: 付典 <fudianchn@gmail.com>
4a6441b to
12392aa
Compare
|
The JDK 8 macos failure behind the downgrade was environmental, not from this change: the job failed in fastjson2-kotlin resolving the dokka-maven-plugin dependency (maven-repository-metadata:2.0.6 descriptor read failure, PluginResolutionException) after core tests had passed. The same JDK 8 job is green on ubuntu and windows, and other fork PRs' CI ran green on 09-05. Branch rebased onto main @ 879183e (head Local full gate on JDK 17 (moditect now requires 11+): The 4 deferred items are noted; if any should be addressed before merge, point me at it and I'll take it. 中文说明降级所引用的 JDK 8 macos 失败属环境问题,非本改动所致:该 job 是在 core 测试全部通过后,于 fastjson2-kotlin 模块解析 dokka-maven-plugin 依赖失败(maven-repository-metadata:2.0.6 descriptor 读取失败,PluginResolutionException)。同一 JDK 8 job 在 ubuntu/windows 均绿,09-05 其他 fork PR 的 CI 也正常。 分支已 rebase 到 main @ 879183e(head 本地 JDK 17 全量门禁: 4 条 deferred 项已知悉;若合并前需处理其中某条,请指出,我来落实。 |
AI 披露:本改动由 AI 编码代理辅助完成,我已逐行审改。
What
JSONFactory.getPropertyBool/getPropertyIntnow honor a value declared infastjson2.propertieseven when the matching-Dsystem property is not set. Previously such a value was silently ignored.Why
A user putting
fastjson2.useJacksonAnnotation=falseinsrc/main/resources/fastjson2.properties(no-Dflag, e.g. to scope the setting to one WAR in a shared-JVM deployment) found it had no effect — the library kept the defaulttrue. Closes #7757.Root cause
Both helpers read the system property first and only fell back to the
fastjson2.propertiesPropertieswhen it was set to an empty string:When the system property is absent (
null) — the common case — the outerif (property != null)skips the block entirely, so the properties file is never consulted and the default is returned.The
fastjson2.creatorresolution block right above (line 173) already implements the intended behavior — fall back when the system property isnullor empty:How
Align
getPropertyBoolandgetPropertyIntwith that existing pattern. This also fixes the same latent bug for the other configs that share these helpers:fastjson2.useGsonAnnotation,fastjson2.writer.alphabetic,fastjson2.writer.skipTransient, andfastjson2.writer.maxLevel. The-Dsystem property still wins when set (non-empty), preserving existing behavior.Config values that cannot be used are reported instead of silently dropped: a malformed int or a bool other than
true/falseprints a one-lineSystem.errdiagnostic and keeps the default, andfastjson2.writer.maxLevel <= 0(parseable, butsetDefaultMaxLevelrejects it as well) falls back to2048instead of freezing an unusable level at class initialization.Testing
Issue7757(regression): drives the helpers directly (they otherwise run only in the static initializer) and asserts the file value wins when the-Dproperty is absent, the-Dvalue wins when set, blank values fall back to the file, padded values are trimmed, malformed int / unrecognized bool values keep the default, andfastjson2.featurestokens are trimmed.expected: <false> but was: <true>,expected: <512> but was: <2048>) and passes after the fix; the new fallback cases fail under mutation (catch removed →NumberFormatExceptionescapes; unrecognized-bool branch flipped).fastjson2.properties(fastjson2.writer.maxLevel=0): pre-fixgetDefaultMaxLevel()=0andtoJSONStringthrowsJSONException: level too large : 1; post-fix2048and serialization OK with the diagnostic on stderr.中文说明
问题
JSONFactory.getPropertyBool/getPropertyInt在未设置对应-D系统参数时,不再读取fastjson2.properties中的配置,导致其中的设置被静默忽略。背景
在
src/main/resources/fastjson2.properties中设置fastjson2.useJacksonAnnotation=false(不带-D,例如多应用共享 JVM 时只让某一个 WAR 生效)实际不生效,库仍使用默认值true。对应 #7757。根因
两个 helper 先读系统参数,仅当其值为空串时才回退读
fastjson2.properties。当系统参数不存在(null,最常见情形)时,外层if (property != null)直接跳过整块,从不读配置文件,返回默认值。其上方fastjson2.creator的解析块(173 行)已是正确写法(null或空串都回退)。修复
让两个 helper 与该既有写法对齐。同时修好了共用这两个 helper 的其它配置(
useGsonAnnotation、writer.alphabetic、writer.skipTransient、writer.maxLevel)的相同隐患。系统参数非空时仍优先生效,行为不变。无法使用的配置值不再被静默丢弃:malformed 整数与
true/false之外的布尔值会打印一行System.err诊断并保留默认值;fastjson2.writer.maxLevel <= 0(可解析,但setDefaultMaxLevel同样拒绝)回退2048,不再把不可用值冻结进静态初始化器。测试
Issue7757(regression):直接驱动各 helper(它们否则只在静态块里跑一次),断言系统参数缺失时配置文件值生效、非空-D优先、空白-D回退文件、带空白值被 trim、malformed 整数 / 无法识别布尔值保留默认、fastjson2.features逐 token trim。NumberFormatException逸出;翻转无法识别布尔分支 → 断言失败)。fastjson2.properties且fastjson2.writer.maxLevel=0):修复前getDefaultMaxLevel()=0、toJSONString抛JSONException: level too large : 1;修复后2048、序列化正常且 stderr 出现诊断行。Fixes #7757