fix: honor serialize false in JSONCompiled writers by hutiefang76 · Pull Request #7661 · alibaba/fastjson2

hutiefang76 · 2026-06-21T02:02:29Z

Summary

split JSONCompiled analysis into reader and writer attribute lists
exclude fields annotated with @JSONField(serialize = false) from generated writers
keep deserialize-only handling independent from serialization filtering

Root Cause

The annotation processor built both generated readers and writers from StructInfo#getReaderAttributes(). Field annotations were analyzed with deserialize semantics only, so serialize = false did not remove the field from the generated writer.

Tests

./mvnw -Penable-codegen -pl codegen -am -Dtest=Issue7660Test -Dsurefire.failIfNoSpecifiedTests=false test
./mvnw -Penable-codegen -pl codegen,codegen-test -am -Dsurefire.failIfNoSpecifiedTests=false test
./mvnw -Penable-codegen -pl codegen,codegen-test -am validate

Fixes #7660

CLAassistant · 2026-06-21T02:02:35Z

All committers have signed the CLA.

hutiefang76 · 2026-06-21T02:15:52Z

Updated the commit author to the GitHub-linked noreply identity so the CLA assistant can associate the commit with my account.

wenshao · 2026-06-22T23:06:22Z

+        Process process = new ProcessBuilder(command)
+                .redirectErrorStream(true)
+                .start();
+        byte[] output = process.getInputStream().readAllBytes();


[Critical] InputStream.readAllBytes() is a JDK 9+ API. The codegen module compiles with -source 8 -target 8 (both jdk1.8 and jdk9+ profiles in codegen/pom.xml). On JDK 8 (which is in the CI matrix), this will fail to compile with "cannot find symbol".

Suggested change

byte[] output = process.getInputStream().readAllBytes();

ByteArrayOutputStream baos = new ByteArrayOutputStream();

byte[] buf = new byte[4096];

int n;

while ((n = process.getInputStream().read(buf)) != -1) {

baos.write(buf, 0, n);

}

return new ProcessResult(process.waitFor(), baos.toString(StandardCharsets.UTF_8.name()));

— qwen3.7-max via Qwen Code /review

wenshao · 2026-06-22T23:06:22Z

+                    FieldInfo writerFieldInfo = new FieldInfo();

                    String name = field.getSimpleName().toString();
                    JSONField[] annotations = field.getAnnotationsByType(JSONField.class);


[Suggestion] Follow-up: the method-processing loop (lines 149-189) also never reads @JSONField from getter/setter methods. If a user places @JSONField(serialize = false) on a getter (common JavaBeans pattern for private fields), the generated writer will still serialize the field — the same class of bug, on a different annotation target.

Consider adding method.getAnnotationsByType(JSONField.class) processing in the method loop to mirror the field-level fix:

JSONField[] methodAnnotations = method.getAnnotationsByType(JSONField.class); for (JSONField annotation : methodAnnotations) { FieldInfo readerFI = new FieldInfo(); FieldInfo writerFI = new FieldInfo(); CodeGenUtils.getFieldInfo(readerFI, annotation, false); CodeGenUtils.getFieldInfo(writerFI, annotation, true); if (readerFI.ignore) attr.reader = false; if (writerFI.ignore) attr.writer = false; }

— qwen3.7-max via Qwen Code /review

wenshao · 2026-06-22T23:06:22Z

+
+public class Issue7660Test {
+    @TempDir
+    public Path tempDir;


[Suggestion] Consider adding a test for @JSONField(deserialize = false) as well. The reader-side code path was modified (attr.reader = !readerFieldInfo.ignore, supportSet() gating on reader), but only the writer path is tested. A deserialization test would protect against regressions in the new reader-side filtering logic.

— qwen3.7-max via Qwen Code /review

wenshao · 2026-06-23T01:08:10Z


-                    info.getAttributeByField(name, field);
+                    AttributeInfo attr = info.getAttributeByField(name, field);
+                    attr.reader = !readerFieldInfo.ignore;


[Critical] Field-level unconditional overwrite of attr.reader/attr.writer

The field processing loop unconditionally assigns attr.reader = !readerFieldInfo.ignore and attr.writer = !writerFieldInfo.ignore. In inheritance scenarios where getTypeHierarchy() visits child classes before parent classes, a child's method-level annotation (e.g., @JSONField(serialize = false) on a getter) can set attr.writer = false, but then the parent's unannotated field overwrites it back to true, silently losing the child's disable decision.

The method loop (lines 206-211) correctly uses a one-way disable pattern (if (ignore) attr.reader = false). The field loop should do the same.

Suggested change

attr.reader = !readerFieldInfo.ignore;

AttributeInfo attr = info.getAttributeByField(name, field);

if (readerFieldInfo.ignore) {

attr.reader = false;

}

if (writerFieldInfo.ignore) {

attr.writer = false;

}

— qwen3.7-max via Qwen Code /review

wenshao · 2026-06-23T01:08:10Z

+                    "    public static class Person {",
+                    "        @JSONField(deserialize = false)",
+                    "        public String internalCode;",
+                    "        public String name;",


[Suggestion] Consider adding a round-trip test that verifies both directions work correctly together on the same @JSONCompiled bean. For example, a bean with @JSONField(serialize = false) on a field should exclude it from toJSONString output while still populating it from parseObject. This catches asymmetric bugs where the reader/writer attribute list split inadvertently affects the wrong direction.

— qwen3.7-max via Qwen Code /review

hutiefang76 · 2026-06-23T01:16:23Z

Addressed the latest review feedback in d45d5fa.

What changed:

changed field-level JSONField handling to one-way disable reader/writer flags instead of unconditionally resetting them back to true
added a regression test for the inheritance case where a child accessor has @JSONField(serialize = false) and a parent field would previously re-enable the writer
added a round-trip test confirming field-level serialize=false still allows deserialization while omitting the field from serialization

Verification:

./mvnw -Penable-codegen -pl codegen -Dtest=Issue7660Test test -DskipITs -Dcheckstyle.skip -Drat.skip=true
git diff --check

wenshao · 2026-06-23T02:03:37Z

+                        name = writerFieldInfo.fieldName;
+                    }
+
                    AttributeInfo attr = info.getAttributeByMethod(name, type, getter, setter);


[Suggestion] Dead code: the else if (writerFieldInfo.fieldName != null) branch is unreachable. CodeGenUtils.getFieldInfo sets fieldName from JSONField.name() regardless of the serialize parameter, so readerFieldInfo.fieldName and writerFieldInfo.fieldName are always identical. When readerFieldInfo.fieldName is null, writerFieldInfo.fieldName is guaranteed null too. The field loop (line 140) correctly omits this redundant check, making the two code paths inconsistent.

Suggested change

AttributeInfo attr = info.getAttributeByMethod(name, type, getter, setter);

if (readerFieldInfo.fieldName != null) {

name = readerFieldInfo.fieldName;

}

— qwen3.7-max via Qwen Code /review

hutiefang76 · 2026-06-23T02:50:06Z

Updated in 1c4c90a67.

This follow-up removes the redundant method-level writerFieldInfo.fieldName branch in Analysis. CodeGenUtils.getFieldInfo(...) derives the same field name for reader and writer metadata, so the writer-side fallback was unreachable.

Validation:

git diff --check
./mvnw -Penable-codegen -pl codegen -Dtest=Issue7660Test test -DskipITs -Dcheckstyle.skip -Drat.skip=true
./mvnw -Penable-codegen -pl codegen test -DskipITs -Dcheckstyle.skip -Drat.skip=true

The full codegen module test run passed 10 tests with 0 failures/errors.

wenshao · 2026-06-27T02:13:23Z

                    for (JSONField annotation : annotations) {
-                        CodeGenUtils.getFieldInfo(fieldInfo, annotation, false);
+                        CodeGenUtils.getFieldInfo(readerFieldInfo, annotation, false);
+                        CodeGenUtils.getFieldInfo(writerFieldInfo, annotation, true);


[Critical] serialize=false is bypassed when deserializeFeatures includes FieldBased

The new call getFieldInfo(writerFieldInfo, annotation, true) reuses CodeGenUtils.getFieldInfo, which contains reader-specific reset logic (lines 542–547) that sets fieldInfo.ignore = false when JSONReader.Feature.FieldBased is present — regardless of the serialize parameter. This silently overrides the ignore=true that serialize=false just set.

Verified empirically: @JSONField(serialize = false, deserializeFeatures = {JSONReader.Feature.FieldBased}) leaks the field into JSON output. This is a realistic combination — FieldBased for private-field deserialization + serialize=false to prevent leakage.

Suggested change

CodeGenUtils.getFieldInfo(writerFieldInfo, annotation, true);

Fix should be in CodeGenUtils.getFieldInfo — gate the FieldBased override on !serialize:

if (!serialize && fieldInfo.ignore && feature == JSONReader.Feature.FieldBased) { fieldInfo.ignore = false; }

— glm-5.2 via Qwen Code /review

wenshao · 2026-06-27T02:13:23Z

+                    }
+
                    AttributeInfo attr = info.getAttributeByMethod(name, type, getter, setter);
+                    if (annotations.length != 0) {


[Suggestion] Redundant if (annotations.length != 0) guard

The method-loop block wraps the reader/writer ignore mapping in this guard, but the structurally identical field-loop block (lines 145–150) does not. The guard is provably dead: when annotations is empty, the for loop never executes, so both ignore flags remain false and the inner if branches never fire regardless.

Suggested change

if (annotations.length != 0) {

AttributeInfo attr = info.getAttributeByMethod(name, type, getter, setter);

if (readerFieldInfo.ignore) {

attr.reader = false;

}

if (writerFieldInfo.ignore) {

attr.writer = false;

}

— glm-5.2 via Qwen Code /review

wenshao · 2026-06-27T02:13:23Z

                        continue;
                    }
+
+                    FieldInfo readerFieldInfo = new FieldInfo();


[Suggestion] Duplicated annotation-processing logic

The reader/writer FieldInfo construction, annotation iteration, name override, and ignore-to-flag mapping is duplicated verbatim between the field loop (lines 130–150) and this method loop (lines 193–211). The copies have already drifted (the redundant guard above exists in only one). Consider extracting a helper to keep them in sync.

— glm-5.2 via Qwen Code /review

wenshao · 2026-06-27T02:13:23Z

+    }
+
+    @Test
+    public void jsonCompiledHonorsDeserializeFalse() throws Exception {


[Suggestion] Missing test coverage for combined flags

Two gaps in the otherwise thorough test suite:

No test exercises @JSONField(serialize = false, deserialize = false) simultaneously — the only case where both attr.reader and attr.writer flip to false, dropping the attribute from both lists.

The deserialize = false test (this method) only checks that deserialization is blocked. It never verifies the field still serializes correctly (the mirror of the serialize=false round-trip test at line 228).

— glm-5.2 via Qwen Code /review

hutiefang76 · 2026-07-01T16:06:04Z

Follow-up status on the latest head a75638839:\n\n- addressed the FieldBased + serialize=false issue by keeping reader-side FieldBased handling from re-enabling writer-side disabled fields\n- added coverage for combined serialize=false / deserialize=false cases and the related JSONCompiled writer/reader paths\n- fixed the JDK 8 test helper compatibility issue where java.home may point at a jre directory on Windows\n\nThe latest GitHub CI run is now green across the JDK 8/11/17/21/25 matrix, reflect jobs, coverage, and license/cla. When you have time, could you please re-review the latest head?

wenshao · 2026-07-09T08:50:48Z

+                    }
+
                    AttributeInfo attr = info.getAttributeByMethod(name, type, getter, setter);
+                    if (readerFieldInfo.ignore) {


[Critical] Split-attribute bypass: @JSONField(name, serialize=false) can leak supposedly-hidden fields

When a field has @JSONField(name = "alias", serialize = false) and a getter exists for the same Java property, the field loop creates an AttributeInfo under "alias" with writer=false, but this method loop creates a separate AttributeInfo under the Java property name (e.g., "internalCode") with writer=true (default). getWriterAttributes() then includes the second entry, leaking the supposedly-hidden value into serialized output.

The same vulnerability exists for deserialize=false: a field with @JSONField(name = "alias", deserialize = false) plus a setter creates two entries, and the generated reader accepts the value under the Java property name.

Consider propagating the field-level disable to the method-resolved attribute when the names diverge, or unifying disable tracking by Java property name rather than JSON name.

— qwen3.7-max via Qwen Code /review

wenshao · 2026-07-09T08:50:48Z


    public boolean supportSet() {
-        return field != null || setMethod != null;
+        return reader && (field != null || setMethod != null);


[Critical] Record canonical constructor arity mismatch with @JSONField(deserialize=false)

For records, field != null (set by the field loop for record components), so the new reader guard changes supportSet() behavior. When @JSONField(deserialize = false) is on a record component, reader becomes false, the component is excluded from getReaderAttributes(), and genCreateRecordInstance / genReadRecordObject generate a constructor call with fewer arguments than the canonical constructor requires.

Example: record Person(@JSONField(deserialize=false) String secret, String name) would generate new Person((String) null) instead of new Person(null, null) — a compile error.

Fix: for records, the constructor call sites need ALL components regardless of the reader filter. Components with reader=false should contribute a default value but not be read from JSON.

— qwen3.7-max via Qwen Code /review

wenshao · 2026-07-09T08:51:05Z

Suggestions — commit `a756388`

File	Issue	Suggested fix
`AttributeInfo.java:44`	New `supportGet()` includes getter-only computed properties in generated writer (behavior change). Previously excluded since `supportSet()` required `field \|\| setMethod`.	Document this behavior change in the PR description. If backward compat is needed, gate `supportGet()` to also require a backing field.
`AttributeInfo.java:19-20`	`reader`/`writer` boolean names are terse; `supportSet()` gated by `reader`, `supportGet()` gated by `writer` — perspective mismatch.	Add Javadoc or rename to `deserializable`/`serializable`.
`Issue7660Test.java`	No test for `@JSONField(name, serialize=false)` combination (triggers split-attribute bypass). No record test with `@JSONField(deserialize=false)`.	Add tests covering both scenarios to prevent regressions.

— qwen3.7-max via Qwen Code /review

hutiefang76 · 2026-07-10T16:01:56Z

The latest review exposed two correctness gaps in the current design: an aliased field can be split from its accessor and bypass serialize=false, and filtering record components can break canonical-constructor arity. Both need a redesign of attribute identity and record construction rather than another narrow patch. I am closing this version instead of expanding an already broad change without enough confidence. I will only revisit it as a smaller change after separate reproductions and a narrower design.

hutiefang76 force-pushed the codex/fix-jsoncompiled-serialize-false branch from 97d0996 to 21df00e Compare June 21, 2026 02:15

wenshao requested changes Jun 22, 2026

View reviewed changes

wenshao requested changes Jun 23, 2026

View reviewed changes

wenshao reviewed Jun 23, 2026

View reviewed changes

wenshao requested changes Jun 27, 2026

View reviewed changes

hutiefang76 added 7 commits July 1, 2026 02:09

fix: honor serialize false in JSONCompiled writers

0cc63ee

fix: honor JSONField on JSONCompiled accessors

0a2d3b7

fix: preserve JSONCompiled annotation disables

92e9e28

fix: remove redundant JSONField method rename branch

81ad5e6

test: cover JSONCompiled setter deserialize false

68cedd3

test: locate javac outside JDK8 jre home

2b5b826

test: guard JSONCompiled javac module args on JDK8

4dac8fd

hutiefang76 force-pushed the codex/fix-jsoncompiled-serialize-false branch from 83f638d to 4dac8fd Compare June 30, 2026 18:19

hutiefang76 added 3 commits July 1, 2026 05:40

fix: keep JSONCompiled writer ignores with FieldBased

4e9ef3f

fix: preserve JSONCompiled record writer order

050a2a7

test: locate Windows javac executable in codegen tests

a756388

wenshao requested changes Jul 9, 2026

View reviewed changes

hutiefang76 closed this Jul 10, 2026

-        byte[] output = process.getInputStream().readAllBytes();
+        ByteArrayOutputStream baos = new ByteArrayOutputStream();
+        byte[] buf = new byte[4096];
+        int n;
+        while ((n = process.getInputStream().read(buf)) != -1) {
+            baos.write(buf, 0, n);
+        }
+        return new ProcessResult(process.waitFor(), baos.toString(StandardCharsets.UTF_8.name()));

-                    AttributeInfo attr = info.getAttributeByMethod(name, type, getter, setter);
+                    if (readerFieldInfo.fieldName != null) {
+                        name = readerFieldInfo.fieldName;
+                    }

Uh oh!

Conversation

hutiefang76 commented Jun 21, 2026

Summary

Root Cause

Tests

Uh oh!

CLAassistant commented Jun 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hutiefang76 commented Jun 21, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hutiefang76 commented Jun 23, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hutiefang76 commented Jun 23, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hutiefang76 commented Jul 1, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wenshao commented Jul 9, 2026

Suggestions — commit a756388

Uh oh!

hutiefang76 commented Jul 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

CLAassistant commented Jun 21, 2026 •

edited

Loading

Suggestions — commit `a756388`