Refactor FieldReader boxed types to eliminate duplication#3983
Refactor FieldReader boxed types to eliminate duplication#3983
Conversation
|
|
…, Bool) to use base class Co-authored-by: wenshao <1166785+wenshao@users.noreply.github.com>
Co-authored-by: wenshao <1166785+wenshao@users.noreply.github.com>
…nt64 Co-authored-by: wenshao <1166785+wenshao@users.noreply.github.com>
Co-authored-by: wenshao <1166785+wenshao@users.noreply.github.com>
|
@MonkeyCode-AI review 一下 |
|
MonkeyCode-AI 正在分析任务... |
MonkeyCode-AI
left a comment
There was a problem hiding this comment.
我是 MonkeyCode AI 编程助手,你可以在 GitHub 仓库的 PR 中 at @MonkeyCode-AI 来呼唤我。
任务执行细节请参考: https://monkeycode-ai.com/tasks/public?id=588fc27e-c930-4e24-939f-f3ea9e4bf626
代码审查结果
通过引入 FieldReaderBoxedType 统一了 boxed 基础类型的 NullOnError 与 schema 校验逻辑,整体重构方向正确,但新增基类的泛型/类型安全与部分子类的 readFieldValue(JSONReader) 语义存在潜在行为变化,建议在合入前做小幅修正。
✨ 代码亮点
- 将 NullOnError try-catch 模式与 schema 校验集中到抽象基类,明显降低重复代码与后续维护成本
- 对 Double 的 defaultValue 早返回与 Int64/Bool 的特殊校验语义做了保留处理,避免常见的重构回归
| 🚨 Critical | 💡 Suggestion | |
|---|---|---|
| 0 | 2 | 0 |
| protected Byte readValue(JSONReader jsonReader) { | ||
| Integer intValue = jsonReader.readInt32(); | ||
| return intValue == null ? null : intValue.byteValue(); | ||
| } |
There was a problem hiding this comment.
Warning
在重构前,FieldReaderInt8/FieldReaderInt16 覆盖了 public Object readFieldValue(JSONReader jsonReader) 并直接 return jsonReader.readInt32(),即“读取但不落字段”时返回的是 Integer。重构后它们继承 FieldReaderBoxedType.readFieldValue(JSONReader) 并返回 readValue(jsonReader),而 readValue 内部将 int32 转换成 Byte/Short,因此该方法返回类型从 Integer 变成了 Byte/Short。
这属于可观察的行为变化:如果框架/上层逻辑在某些路径依赖该 Object 返回值的具体运行时类型(例如统一当作 Integer 处理、或做 instanceof Integer 分支),可能会产生兼容性问题。虽然 PR 描述没有提到这一点,但从 diff 可以确定语义确实发生了变化。
建议: 为 Int8/Int16 恢复原先 readFieldValue(JSONReader) 的返回值语义(返回 Integer),同时保留 readFieldValue(JSONReader,T) 写入字段时的 Byte/Short 转换。也可以在基类中拆分“赋值类型”和“readFieldValue(JsonReader)返回类型”,但最小改动是在子类里单独 override readFieldValue(JSONReader)。
| protected Byte readValue(JSONReader jsonReader) { | |
| Integer intValue = jsonReader.readInt32(); | |
| return intValue == null ? null : intValue.byteValue(); | |
| } | |
| @Override | |
| public Object readFieldValue(JSONReader jsonReader) { | |
| return jsonReader.readInt32(); | |
| } | |
| @Override | |
| protected Byte readValue(JSONReader jsonReader) { | |
| Integer intValue = jsonReader.readInt32(); | |
| return intValue == null ? null : intValue.byteValue(); | |
| } |
| protected Short readValue(JSONReader jsonReader) { | ||
| Integer intValue = jsonReader.readInt32(); | ||
| return intValue == null ? null : intValue.shortValue(); | ||
| } |
There was a problem hiding this comment.
Warning
同 Int8:重构前 FieldReaderInt16.readFieldValue(JSONReader) 返回 jsonReader.readInt32()(Integer),重构后继承基类返回 readValue(jsonReader)(Short)。这可能影响依赖返回类型为 Integer 的调用路径。
建议: 在 FieldReaderInt16 中覆盖 readFieldValue(JSONReader) 返回 Integer,保持兼容;readFieldValue(JSONReader,T) 仍通过 readValue 写入 Short。
| protected Short readValue(JSONReader jsonReader) { | |
| Integer intValue = jsonReader.readInt32(); | |
| return intValue == null ? null : intValue.shortValue(); | |
| } | |
| @Override | |
| public Object readFieldValue(JSONReader jsonReader) { | |
| return jsonReader.readInt32(); | |
| } | |
| @Override | |
| protected Short readValue(JSONReader jsonReader) { | |
| Integer intValue = jsonReader.readInt32(); | |
| return intValue == null ? null : intValue.shortValue(); | |
| } |
Eight FieldReader classes (Int32, Int64, Int8, Int16, Float, Double, Bool, BigDecimal) contained identical error handling and schema validation logic. This refactors them to share a common base class.
Changes
FieldReaderBoxedType<T, V>consolidates try-catch-NullOnError pattern and schema validationreadValue()for type-specific readingFieldReaderDouble: Maintains defaultValue early-return behaviorFieldReaderInt64: Preserves TypeUtils.toLong conversion for validation while setting original valueFieldReaderBool: Validates converted Boolean value, not original inputBefore/After
Impact
Warning
Firewall rules blocked me from connecting to one or more addresses (expand for details)
I tried to connect to the following addresses, but was blocked by firewall rules:
a.b.com/usr/lib/jvm/temurin-17-jdk-amd64/bin/java /usr/lib/jvm/temurin-17-jdk-amd64/bin/java -javaagent:/home/REDACTED/.m2/repository/org/jacoco/org.jacoco.agent/0.8.13/org.jacoco.agent-0.8.13-runtime.jar=destfile=/home/REDACTED/work/fastjson2/fastjson2/core/target/jacoco.exec,excludes=com/alibaba/fastjson2/benchmark:com/alibaba/example/springtest:co(dns block)central.sonatype.com/usr/lib/jvm/temurin-17-jdk-amd64/bin/java /usr/lib/jvm/temurin-17-jdk-amd64/bin/java --enable-native-access=ALL-UNNAMED -classpath /usr/share/apache-maven-3.9.12/boot/plexus-classworlds-2.9.0.jar -Dclassworlds.conf=/usr/share/apache-maven-3.9.12/bin/m2.conf -Dmaven.home=/usr/share/apache-maven-3.9.12 -Dlibrary.jansi.path=/usr/share/apache-maven-3.9.12/lib/jansi-native -Dmaven.multiModuleProjectDirectory=/home/REDACTED/work/fastjson2/fastjson2 org.codehaus.plexus.classworlds.launcher.Launcher clean compile -pl core -am -DskipTests(dns block)maven.aliyun.com/usr/lib/jvm/temurin-17-jdk-amd64/bin/java /usr/lib/jvm/temurin-17-jdk-amd64/bin/java --enable-native-access=ALL-UNNAMED -classpath /usr/share/apache-maven-3.9.12/boot/plexus-classworlds-2.9.0.jar -Dclassworlds.conf=/usr/share/apache-maven-3.9.12/bin/m2.conf -Dmaven.home=/usr/share/apache-maven-3.9.12 -Dlibrary.jansi.path=/usr/share/apache-maven-3.9.12/lib/jansi-native -Dmaven.multiModuleProjectDirectory=/home/REDACTED/work/fastjson2/fastjson2 org.codehaus.plexus.classworlds.launcher.Launcher clean compile -pl core -am -DskipTests(dns block)If you need me to access, download, or install something from one of these locations, you can either:
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.