Disallow trailing tokens in JSON files #1204
Conversation
Motivation:
We observed an unexpected deserialization outcome with malformed JSON.
Jackson does not fail on that data but silently drops part of the data.
For example, we expected Jackson to fail to parse the data.
```json5
{
"a" : {},
"b" : {},
"c" : {
},
"c1" : true
} // a malformed JSON
"d" : {},
"e": {}
}
```
But Jackson just drops `d` and `e` fields and parses the JSON into:
```
{
"a":{},
"b":{},
"c":{},
"c1":true
}
```
The maintainer of Jackson said it is an expected behavior but not a bug.
FasterXML/jackson-core#808 (comment)
> But this is intended behavior, not a bug: Jackson only reads as much
> content as it needs for a valid JSON Value. It does not try to consume
> everything there might be available, given that the white-space-separated
> content streams are commonly used.
He suggested enabling `DeserializationFeature.FAIL_ON_TRAILING_TOKENS`
to fail on the malformed data added after the end of the top-level JSON.
Modifications:
- Enable `DeserializationFeature.FAIL_ON_TRAILING_TOKENS` to the default
Jackson `ObjectMapper` in Central Dogma
- Breaking) Central Dogma no longer allow tailing commas in JSON
Result:
Central Dogma now correctly rejects malformed JSON when parsing JSON
data.
WalkthroughThe PR enables Jackson's FAIL_ON_TRAILING_TOKENS deserialization feature on both compact and pretty mappers to enforce stricter JSON parsing. Supporting test cases verify the new behavior for invalid JSON with trailing content, and existing test JSON patches are corrected to contain valid syntax. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)server/src/test/java/com/linecorp/centraldogma/server/metadata/MetadataApiServiceTest.java (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
🔇 Additional comments (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
common/src/main/java/com/linecorp/centraldogma/internal/Jackson.java(2 hunks)server/src/test/java/com/linecorp/centraldogma/server/InvalidJsonParsingTest.java(1 hunks)server/src/test/resources/invalid-content-after-trailing-comma.json(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
server/src/test/java/com/linecorp/centraldogma/server/InvalidJsonParsingTest.java (1)
common/src/main/java/com/linecorp/centraldogma/internal/Jackson.java (1)
Jackson(69-393)
🪛 Biome (2.1.2)
server/src/test/resources/invalid-content-after-trailing-comma.json
[error] 10-10: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 10-10: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 10-11: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 11-11: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 12-12: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 12-12: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 12-13: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 13-14: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: flaky-tests
- GitHub Check: build-ubuntu-latest-jdk-17-min-java-11
- GitHub Check: build-macos-latest-jdk-21
- GitHub Check: docker-build
- GitHub Check: build-windows-latest-jdk-21
- GitHub Check: build-ubuntu-latest-jdk-21-snapshot
- GitHub Check: build-ubuntu-latest-jdk-17-coverage
- GitHub Check: build-ubuntu-latest-jdk-11
- GitHub Check: lint
🔇 Additional comments (4)
common/src/main/java/com/linecorp/centraldogma/internal/Jackson.java (2)
42-42: LGTM!The import is correctly added to support the new deserialization feature.
81-82: LGTM!Enabling
FAIL_ON_TRAILING_TOKENSon both mappers ensures consistent validation of JSON content. This addresses the issue where Jackson could silently drop fields when malformed JSON contains trailing content.server/src/test/java/com/linecorp/centraldogma/server/InvalidJsonParsingTest.java (1)
38-40: LGTM!The assertion correctly verifies that malformed JSON with trailing content triggers an
IOErrorwithMismatchedInputExceptionas the cause, which aligns with the enabledFAIL_ON_TRAILING_TOKENSfeature.server/src/test/resources/invalid-content-after-trailing-comma.json (1)
1-14: LGTM!This test resource is intentionally invalid JSON to verify that the
FAIL_ON_TRAILING_TOKENSfeature correctly rejects malformed content. The missing comma after line 9's closing brace creates the exact scenario the feature is designed to catch.
server/src/test/java/com/linecorp/centraldogma/server/InvalidJsonParsingTest.java
Outdated
Show resolved
Hide resolved
server/src/test/java/com/linecorp/centraldogma/server/InvalidJsonParsingTest.java
Outdated
Show resolved
Hide resolved
ikhoon
changed the title
build.gradle
Outdated
| // Target Java 8 except for spring-boot3 | ||
| if (!project.name.startsWith("java-spring-boot3")) { | ||
| tasks.withType(JavaCompile) { | ||
| if (JavaVersion.current() >= JavaVersion.VERSION_1_9) { | ||
| options.release = 8 | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Removed this hard-coded logic. I believe options.release is properly set through java.gradle according to javaTargetCompatibility and javaN flags.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
server/src/test/java/com/linecorp/centraldogma/server/InvalidJsonParsingTest.java (1)
35-37: Consider adding null-check for better test diagnostics.If the resource is not found,
getResourceAsStreamreturnsnull, and callingreadAllBytes()onnullproduces an NPE. While acceptable for a unit test, an explicit null-check or assertion would provide a clearer error message during test failures.For example:
try (InputStream is = InvalidJsonParsingTest.class.getResourceAsStream( "/invalid-content-after-trailing-comma.json")) { + assertThat(is).isNotNull(); final byte[] bytes = is.readAllBytes();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/src/test/java/com/linecorp/centraldogma/server/InvalidJsonParsingTest.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
server/src/test/java/com/linecorp/centraldogma/server/InvalidJsonParsingTest.java (1)
common/src/main/java/com/linecorp/centraldogma/internal/Jackson.java (1)
Jackson(69-393)
🔇 Additional comments (1)
server/src/test/java/com/linecorp/centraldogma/server/InvalidJsonParsingTest.java (1)
34-36: Past issues resolved correctly.The typo in the method name has been fixed ("Trailing" instead of "Tailing"), and the resource leak has been properly addressed using try-with-resources.
| void shouldFailOnInvalidJsonAfterTrailingComma() throws IOException { | ||
| try (InputStream is = InvalidJsonParsingTest.class.getResourceAsStream( | ||
| "/invalid-content-after-trailing-comma.json")) { | ||
| final byte[] bytes = is.readAllBytes(); | ||
| assertThatThrownBy(() -> Jackson.readTree(bytes)) | ||
| .isInstanceOf(IOError.class) | ||
| .hasCauseInstanceOf(MismatchedInputException.class); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify test resource aligns with FAIL_ON_TRAILING_TOKENS feature.
The test name and resource name suggest testing "invalid JSON after trailing comma," but FAIL_ON_TRAILING_TOKENS specifically addresses cases where valid top-level JSON is followed by additional tokens that Jackson would otherwise silently ignore.
If the test resource contains already-malformed JSON (e.g., syntax errors like {"a":1,} with a trailing comma inside the object), it would fail parsing even without this feature enabled, which would not properly validate the PR's objective.
Please confirm the test resource contains a valid JSON value followed by additional content (e.g., {"a":1}{"b":2} or {"a":1}\n{"b":2}) to properly test the scenario described in the PR motivation.
🏁 Script executed:
#!/bin/bash
# Display the test resource content to verify it matches the intended scenario
cat server/src/test/resources/invalid-content-after-trailing-comma.jsonLength of output: 98
Correct test resource to properly validate FAIL_ON_TRAILING_TOKENS feature.
The test resource contains malformed JSON with structural syntax errors (mismatched braces, missing commas, orphaned keys), which fails parsing regardless of whether FAIL_ON_TRAILING_TOKENS is enabled. This does not validate the feature's actual purpose: rejecting valid JSON values followed by additional tokens that would otherwise be silently ignored.
Replace the resource file content with valid JSON followed by trailing content. Example: {"a":1}{"b":2} or {"a":1}\nextra, which demonstrates the exact scenario the feature addresses.
🤖 Prompt for AI Agents
In
server/src/test/java/com/linecorp/centraldogma/server/InvalidJsonParsingTest.java
around lines 34 to 41, the test currently uses a resource containing
structurally invalid JSON which causes parsing to fail irrespective of
FAIL_ON_TRAILING_TOKENS; replace the test resource file
(/invalid-content-after-trailing-comma.json) with content that is syntactically
valid JSON followed by extra tokens (for example: {"a":1}{"b":2} or
{"a":1}\nextra) so the test exercises rejection of trailing tokens specifically,
then keep the assertion expecting an IOError caused by MismatchedInputException.
|
I found that Although it could be worked around for that case, I wasn't sure if enabling If there is consensus that applying a workaround and changing the internal deserializers is preferable, I'm happy to follow the direction. Otherwise, I will close this PR. |
3 participants
Motivation:
We observed an unexpected deserialization outcome with malformed JSON. Jackson does not fail on that data but silently drops part of the data.
For example, we expected Jackson to fail to parse the data.
But Jackson just drops
dandefields and parses the JSON into:{ "a":{}, "b":{}, "c":{}, "c1":true }The maintainer of Jackson said it is an expected behavior but not a bug. FasterXML/jackson-core#808 (comment)
He suggested enabling
DeserializationFeature.FAIL_ON_TRAILING_TOKENSto fail on the malformed data added after the end of the top-level JSON.Modifications:
DeserializationFeature.FAIL_ON_TRAILING_TOKENSto the default JacksonObjectMapperin Central DogmaResult:
Central Dogma now correctly rejects malformed JSON when parsing JSON data.