build(gradle): add build-time validation for prefabs and JSON assets #4986 - #5326
build(gradle): add build-time validation for prefabs and JSON assets #4986#5326mmruii wants to merge 5 commits into
Conversation
|
Hi there, welcome, and thank you 👍 It looks like you already triggered a good looking test in the Jenkins build :-) That's encouraging! Although it does need a quick fix in that actual file - great example though 😁 And I see tests, also good! I'll try to find time to test and review a bit later, but one thing that stuck out is |
Registers a ValidateJsonAssets Gradle task that parses all JSON-based asset files (*.prefab, *.block, *.ui, *.json) under the assets directory at build time, failing the build with a descriptive message if any file contains malformed JSON. - Add ValidateJsonAssets task class to build-logic - Add org.json:json dependency to build-logic for JSON parsing - Hook validateJsonAssets into processResources for modules - Hook validateJsonAssets into processResources for engine Closes MovingBlocks#4986
Tests cover: - Task registration on a Gradle project - Valid JSON/prefab files pass without errors - Malformed JSON files fail the build with a descriptive message mentioning the offending file - Projects with no assets directory succeed gracefully Also refactor ValidateJsonAssets to use ConfigurableFileCollection with @SkipWhenEmpty for proper Gradle up-to-date checking, and replace deprecated createTempDir() with Files.createTempDirectory().
41638bf to
e1276e8
Compare
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a ChangesJSON asset validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Mergeability Score: 🟡 Moderate · up to The PR adds build-time validation for JSON assets, but its current error handling may hide file-read failures and allow invalid or incompletely checked assets to pass the build. Merge should wait for the exception handling to be narrowed to the intended malformed-JSON case. Sequence Diagram(s)sequenceDiagram
participant processResources
participant ValidateJsonAssets
participant JsonAssetInspector
participant Gson
processResources->>ValidateJsonAssets: depend on validation
ValidateJsonAssets->>JsonAssetInspector: inspect each asset
JsonAssetInspector->>Gson: parse lenient JSON
Gson-->>JsonAssetInspector: parsed tokens or parse error
JsonAssetInspector-->>ValidateJsonAssets: errors and warnings
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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
🧹 Nitpick comments (1)
build-logic/build.gradle.kts (1)
74-74: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueScope
kotlin.testto the Gradle test classpath.
build-logiconly useskotlin.testfromValidateJsonAssetsTest.kt, not in production build logic. UsetestImplementation(kotlin("test"))so consumers do not receive Gradle plugin test dependencies fromapi.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@build-logic/build.gradle.kts` at line 74, Change the kotlin("test") dependency declaration in the build-logic dependencies block from api to testImplementation. Keep it scoped to the test classpath so production build logic consumers do not receive it.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@build-logic/src/main/kotlin/org/terasology/gradology/ValidateJsonAssets.kt`:
- Around line 70-76: Update validateFile so it uses a parser configured for
strict JSON syntax instead of JSONObject(file.readText()), rejecting trailing
commas, \u0027-quoted strings, and unquoted fields or values. Preserve the
existing valid/null and JSONException-to-error-message behavior, and add TestKit
coverage for each malformed-input case.
In
`@build-logic/src/test/kotlin/org/terasology/gradology/ValidateJsonAssetsTest.kt`:
- Around line 85-95: Update the `project with no assets directory passes
validation` test to expect `TaskOutcome.NO_SOURCE` for
`result.task(":validateJsonAssets")?.outcome`, reflecting the task’s
`@SkipWhenEmpty` behavior when no asset files exist.
---
Nitpick comments:
In `@build-logic/build.gradle.kts`:
- Line 74: Change the kotlin("test") dependency declaration in the build-logic
dependencies block from api to testImplementation. Keep it scoped to the test
classpath so production build logic consumers do not receive it.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 058cf0dc-ac69-4a27-8572-903e2b999119
📒 Files selected for processing (5)
build-logic/build.gradle.ktsbuild-logic/src/main/kotlin/org/terasology/gradology/ValidateJsonAssets.ktbuild-logic/src/main/kotlin/terasology-module.gradle.ktsbuild-logic/src/test/kotlin/org/terasology/gradology/ValidateJsonAssetsTest.ktengine/build.gradle.kts
| private fun validateFile(file: File): String? { | ||
| return try { | ||
| JSONObject(file.readText()) | ||
| null // valid | ||
| } catch (e: JSONException) { | ||
| "${file.path}: ${e.message}" | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
tmp_dir="$(mktemp -d)"
trap 'rm -rf "$tmp_dir"' EXIT
curl -fsSL \
-o "$tmp_dir/json-sources.jar" \
"https://repo1.maven.org/maven2/org/json/json/20240303/json-20240303-sources.jar"
unzip -p "$tmp_dir/json-sources.jar" org/json/JSONObject.java |
sed -n '/The constructors are more forgiving/,/public JSONObject/p'Repository: MovingBlocks/Terasology
Length of output: 3456
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the Gradle validation implementation and relevant test/task wiring.
fd -a 'ValidateJsonAssets|JsonAssets|Json' . | sed 's#^\./##' | head -100
printf '\n--- outline ---\n'
ast-grep outline build-logic/src/main/kotlin/org/terasology/gradology/ValidateJsonAssets.kt || true
printf '\n--- relevant source ---\n'
sed -n '1,180p' build-logic/src/main/kotlin/org/terasology/gradology/ValidateJsonAssets.kt
printf '\n--- tests/usages ---\n'
rg -n "ValidateJsonAssets|jsonAssets|validateJsonAssets|JSONException|JSONParser|Jackson|JsonParser|JsonFactory|JsonProcessingException" build-logic .github gradle || trueRepository: MovingBlocks/Terasology
Length of output: 6268
🌐 Web query:
Jackson JsonParser.Feature.ALLOW_UNQUOTED_FIELD_NAMES trailing comma single quote documentation
💡 Result:
In Jackson, features that allow non-standard JSON formats—such as unquoted field names, trailing commas, and single quotes—are managed by the JsonReadFeature enumeration [1][2][3]. While older versions of Jackson used JsonParser.Feature for these settings, they have been superseded by JsonReadFeature starting in version 2.10 to provide better granularity [4][5][6]. Below are the specific features for the requirements you mentioned: 1. Unquoted Field Names: Use JsonReadFeature.ALLOW_UNQUOTED_FIELD_NAMES [1]. This allows field names that are not enclosed in double quotes, which is non-standard but often used in JavaScript [4][1]. 2. Trailing Commas: Use JsonReadFeature.ALLOW_TRAILING_COMMA [2]. This allows a single trailing comma following the final value in an array or the final member in an object, and these commas are ignored during parsing [2][6]. 3. Single Quotes: Use JsonReadFeature.ALLOW_SINGLE_QUOTES [7][3]. This permits the use of single quotes (apostrophes) for both field names and string values, in addition to the standard double quotes [7][3]. Note: Because these features deviate from the strict JSON specification, they are disabled by default [1][2][3]. If you are using legacy code that still refers to JsonParser.Feature, it is recommended to migrate to JsonReadFeature [4][6].
Citations:
- 1: https://www.javadoc.io/static/com.fasterxml.jackson.core/jackson-core/2.19.4/com/fasterxml/jackson/core/json/JsonReadFeature.html
- 2: https://www.javadoc.io/static/com.fasterxml.jackson.core/jackson-core/2.21.2/com/fasterxml/jackson/core/json/JsonReadFeature.html
- 3: https://javadoc.io/static/com.fasterxml.jackson.core/jackson-core/2.17.3/com/fasterxml/jackson/core/json/JsonReadFeature.html
- 4: https://fasterxml.github.io/jackson-core/javadoc/2.13/com/fasterxml/jackson/core/JsonParser.Feature.html
- 5: https://fasterxml.github.io/jackson-core/javadoc/2.10/com/fasterxml/jackson/core/JsonParser.Feature.html?is-external=true
- 6: https://www.javadoc.io/static/com.fasterxml.jackson.core/jackson-core/2.20.0/com/fasterxml/jackson/core/JsonParser.Feature.html
- 7: http://fasterxml.github.io/jackson-core/javadoc/2.11/com/fasterxml/jackson/core/json/JsonReadFeature.html
Use a strict JSON parser.
JSONObject(file.readText()) accepts trailing commas, single-quoted strings, and unquoted strings, so malformed JSON assets can pass this task. Use a parser configured for strict JSON syntax instead. Add TestKit cases for trailing commas, \u0027-quoted strings, and unquoted fields/values.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@build-logic/src/main/kotlin/org/terasology/gradology/ValidateJsonAssets.kt`
around lines 70 - 76, Update validateFile so it uses a parser configured for
strict JSON syntax instead of JSONObject(file.readText()), rejecting trailing
commas, \u0027-quoted strings, and unquoted fields or values. Preserve the
existing valid/null and JSONException-to-error-message behavior, and add TestKit
coverage for each malformed-input case.
| fun `project with no assets directory passes validation`() { | ||
| val projectDir = createTempProjectDir() | ||
| // No assets dir created intentionally | ||
|
|
||
| val result = GradleRunner.create() | ||
| .withProjectDir(projectDir) | ||
| .withArguments("validateJsonAssets") | ||
| .withPluginClasspath() | ||
| .build() | ||
|
|
||
| assertEquals(TaskOutcome.SUCCESS, result.task(":validateJsonAssets")?.outcome) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 3 \
'`@get`:SkipWhenEmpty|TaskOutcome\.(SUCCESS|NO_SOURCE)' \
build-logic/src/main/kotlin/org/terasology/gradology/ValidateJsonAssets.kt \
build-logic/src/test/kotlin/org/terasology/gradology/ValidateJsonAssetsTest.ktRepository: MovingBlocks/Terasology
Length of output: 2382
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Gradle plugin/task declarations:"
rg -n -C 4 'class ValidateJsonAssets|`@SkipWhenEmpty`|TaskOutcome|task\(' build-logic/src/main build-logic/src/test
echo
echo "Relevant file outlines:"
ast-grep outline build-logic/src/main/kotlin/org/terasology/gradology/ValidateJsonAssets.kt --view expanded || true
ast-grep outline build-logic/src/test/kotlin/org/terasology/gradology/ValidateJsonAssetsTest.kt --view expanded || true
echo
echo "Read relevant sources:"
sed -n '1,140p' build-logic/src/main/kotlin/org/terasology/gradology/ValidateJsonAssets.kt
sed -n '1,130p' build-logic/src/test/kotlin/org/terasology/gradology/ValidateJsonAssetsTest.ktRepository: MovingBlocks/Terasology
Length of output: 13098
🌐 Web query:
Gradle documentation @SkipWhenEmpty task Outcome no-inputs no-source
💡 Result:
In Gradle, the @SkipWhenEmpty annotation (or the equivalent skipWhenEmpty method on a task input property) is used to indicate that a task should be skipped if the annotated file collection or directory is empty [1][2][3][4]. When all task input properties declared with @SkipWhenEmpty are empty, the task is skipped and labeled with the NO-SOURCE outcome [1][3][5]. Key details regarding this behavior include: 1. Outcome: The NO-SOURCE label indicates that the task had inputs and outputs defined, but because the sources (the @SkipWhenEmpty inputs) were empty, the task did not need to execute its actions [6][7]. 2. Requirements: If multiple input properties are marked with @SkipWhenEmpty, all of them must be empty for the task to be skipped [4]. 3. Incremental Builds: The @SkipWhenEmpty annotation implicitly marks a property as @Incremental [1][2][8]. This allows Gradle to track changes to these inputs for incremental builds [1][3]. 4. Configuration: The annotation should be applied to the getter method of the property in Java or the property itself in Groovy [1][2]. Alternatively, the skipWhenEmpty method can be called on the TaskInputFilePropertyBuilder when configuring task inputs programmatically [3][4]. 5. Related Annotations: It is often recommended to use the @IgnoreEmptyDirectories annotation in conjunction with @SkipWhenEmpty if the task should only process files and not treat empty directories as sources [1][2]. You can check if a task was skipped due to having no sources by inspecting the task state, such as via the getNoSource method in the Gradle API [9].
Citations:
- 1: https://docs.gradle.org/current/javadoc/org/gradle/api/tasks/SkipWhenEmpty.html
- 2: https://docs.gradle.org/current/kotlin-dsl/gradle/org.gradle.api.tasks/-skip-when-empty/index.html
- 3: https://docs.gradle.org/9.5.0/userguide/incremental_build.html
- 4: https://docs.gradle.org/current/kotlin-dsl/gradle/org.gradle.api.tasks/-task-input-file-property-builder/skip-when-empty.html
- 5: https://docs.gradle.org/9.3.0/javadoc/org/gradle/testkit/runner/TaskOutcome.html
- 6: https://docs.gradle.org/current/userguide/more_about_tasks.html
- 7: https://docs.gradle.org/9.5.0/userguide/more_about_tasks.html
- 8:
@Incrementaland@SkipWhenEmptyshould be combinable or maybe even independent gradle/gradle#17593 - 9: https://docs.gradle.org/current/kotlin-dsl/gradle/org.gradle.api.tasks/-task-state/get-no-source.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Check Gradle dependency versions/plugins:"
rg -n 'gradle|org.gradle|org.terasology|ValidateJsonAssets|gradle-plugin|pluginId|id ' build-logic settings.gradle.kts settings.gradle.kts.gradle.plugin build.gradle.kts --glob '!build/**'Repository: MovingBlocks/Terasology
Length of output: 18715
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "build-logic Gradle configuration:"
cat -n build-logic/build.gradle.kts | sed -n '1,120p'
echo
echo "Root Gradle version markers:"
if [ -f gradle/wrapper/gradle-wrapper.properties ]; then cat -n gradle/wrapper/gradle-wrapper.properties; fi
if [ -f gradlew ]; then echo "gradlew exists"; fiRepository: MovingBlocks/Terasology
Length of output: 3987
Assert the no-source task outcome.
@SkipWhenEmpty skips validateJsonAssets with TaskOutcome.NO_SOURCE when no asset files exist. The no-assets test should assert that outcome instead of SUCCESS.
Proposed fix
- assertEquals(TaskOutcome.SUCCESS, result.task(":validateJsonAssets")?.outcome)
+ assertEquals(TaskOutcome.NO_SOURCE, result.task(":validateJsonAssets")?.outcome)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fun `project with no assets directory passes validation`() { | |
| val projectDir = createTempProjectDir() | |
| // No assets dir created intentionally | |
| val result = GradleRunner.create() | |
| .withProjectDir(projectDir) | |
| .withArguments("validateJsonAssets") | |
| .withPluginClasspath() | |
| .build() | |
| assertEquals(TaskOutcome.SUCCESS, result.task(":validateJsonAssets")?.outcome) | |
| fun `project with no assets directory passes validation`() { | |
| val projectDir = createTempProjectDir() | |
| // No assets dir created intentionally | |
| val result = GradleRunner.create() | |
| .withProjectDir(projectDir) | |
| .withArguments("validateJsonAssets") | |
| .withPluginClasspath() | |
| .build() | |
| assertEquals(TaskOutcome.NO_SOURCE, result.task(":validateJsonAssets")?.outcome) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@build-logic/src/test/kotlin/org/terasology/gradology/ValidateJsonAssetsTest.kt`
around lines 85 - 95, Update the `project with no assets directory passes
validation` test to expect `TaskOutcome.NO_SOURCE` for
`result.task(":validateJsonAssets")?.outcome`, reflecting the task’s
`@SkipWhenEmpty` behavior when no asset files exist.
…t JSON
org.json rejects the comments Terasology assets use pervasively, so every module sampled failed - CoreAssets alone with 56 files - and the task gated processResources, meaning nothing could build. Validation now uses gson 2.8.6, the version settings.gradle.kts already pins for the engine, where UIFormat and UISkinFormat set leniency outright.
The task also gained a declared output: without one Gradle has no up-to-date criterion, so it re-parsed every asset on every build of every module.
Tests now call the inspector directly. The TestKit harness never resolved the plugin under test ("Unresolved reference"), so 3 of its 4 tests failed at base and the validator was never exercised - which is how the parser mismatch survived review.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The ColumnLayout declared layoutInfo twice; Gson keeps the last, so the earlier block had been silently discarded for an unknown length of time. Removing it is behaviour-preserving. Found by the validator this branch adds - its first run against real assets, which is the point of the change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
build-logic/src/test/kotlin/org/terasology/gradology/ValidateJsonAssetsTest.kt (1)
31-35: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExecute the task action in direct unit tests.
These tests cover
JsonAssetInspectorand task registration. They do not executeValidateJsonAssets.validate(). Add directProjectBuildertests that configuresource()andreport, then verify report output for valid input andGradleExceptionfor invalid input.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@build-logic/src/test/kotlin/org/terasology/gradology/ValidateJsonAssetsTest.kt` around lines 31 - 35, Add direct ProjectBuilder tests for ValidateJsonAssets.validate(), configuring source() and report for both valid and invalid JSON asset inputs. Assert that valid input produces the expected report output, while invalid input causes GradleException; retain the existing task-registration assertions.build-logic/src/main/kotlin/org/terasology/gradology/ValidateJsonAssets.kt (1)
139-149: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winMake the build-cache claim true or remove it.
@OutputFileenables up-to-date checks, but this task is not cacheable because it has neither@CacheableTasknoroutputs.cacheIf. If build-cache reuse is intended, add one of these mechanisms. Also make diagnostic paths relocation-safe becauseJsonAssetInspectorwrites absoluteFile.pathvalues whilejsonAssetsusesPathSensitivity.RELATIVE.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@build-logic/src/main/kotlin/org/terasology/gradology/ValidateJsonAssets.kt` around lines 139 - 149, The ValidateJsonAssets task’s build-cache claim is incomplete and its diagnostics are not relocation-safe. Update the ValidateJsonAssets task to enable build-cache reuse via the appropriate cacheability mechanism, and adjust JsonAssetInspector diagnostic path generation so reported paths are relative and consistent with the RELATIVE-sensitive jsonAssets input rather than using absolute File.path values.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@build-logic/src/main/kotlin/org/terasology/gradology/ValidateJsonAssets.kt`:
- Around line 139-149: The ValidateJsonAssets task’s build-cache claim is
incomplete and its diagnostics are not relocation-safe. Update the
ValidateJsonAssets task to enable build-cache reuse via the appropriate
cacheability mechanism, and adjust JsonAssetInspector diagnostic path generation
so reported paths are relative and consistent with the RELATIVE-sensitive
jsonAssets input rather than using absolute File.path values.
In
`@build-logic/src/test/kotlin/org/terasology/gradology/ValidateJsonAssetsTest.kt`:
- Around line 31-35: Add direct ProjectBuilder tests for
ValidateJsonAssets.validate(), configuring source() and report for both valid
and invalid JSON asset inputs. Assert that valid input produces the expected
report output, while invalid input causes GradleException; retain the existing
task-registration assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f444232c-f2b8-4173-9e00-c70f81d66c19
📒 Files selected for processing (4)
build-logic/build.gradle.ktsbuild-logic/src/main/kotlin/org/terasology/gradology/ValidateJsonAssets.ktbuild-logic/src/test/kotlin/org/terasology/gradology/ValidateJsonAssetsTest.ktengine/src/main/resources/org/terasology/engine/assets/ui/menu/moduleDetailsScreen.ui
💤 Files with no reviewable changes (1)
- engine/src/main/resources/org/terasology/engine/assets/ui/menu/moduleDetailsScreen.ui
|
Hi again @mmruii - sorry for the long delay, had to invent some tooling real quick to be able to better put little found bits of time to use so dealing with these PRs gets easier! :-) Thank you again for getting this started! I tested it out alongside my handy dandy new agent friend and found it viable but just a little off, as it validated overly strict JSON when we use GSON with a bit more leniency in places (like accepting comments). I was able to get that swapped over and tested then send another couple commits to your branch so this PR will open and re-validate. Should be able to do final testing on this then merge it soon! |
Cervator
left a comment
Cervator
left a comment
There was a problem hiding this comment.
Did a thorough test pass with added changes (JSON -> GSON) in a full Omega workspace. Looking good, letting it rebuild in Jenkins then one final sweep locally and we should be able to merge!
A full sweep over all 144 Omega modules turned up two distinct classes wearing one error. Cooking's CookingRecipes.prefab is missing a comma between members, which no parser accepts - its recipes silently do not load today. Apiculture's extractor.ui and injector.ui carry a stray closing brace, which the engine ignores outright, because the loaders read one root value and never check what follows. Trailing content is therefore now a warning, leaving the contract as: errors are what the engine cannot load, warnings are what it loads despite the file being defective. Duplicate keys already sat on the warning side for the same reason. Peeking past the root value can itself throw on a stray brace, so that check needs its own catch - without it the classification silently reverts to a parse failure. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Poked around a bit more in module land and found a few issues detectable with this 👍 Some were more severe than others so now there are both warnings and errors. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
build-logic/src/test/kotlin/org/terasology/gradology/ValidateJsonAssetsTest.kt (1)
205-212: 🗄️ Data Integrity & Integration | 🔵 Trivial | 🏗️ Heavy liftAdd one task-execution test.
This test calls
JsonAssetInspector.inspectdirectly. It does not executeValidateJsonAssets.validate(). A regression injsonAssetswiring, report creation, warning aggregation, or failure propagation would leave the suite green. Add a task-level test that configures a fixture, runs the task action, and checks the report and success/failure behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@build-logic/src/test/kotlin/org/terasology/gradology/ValidateJsonAssetsTest.kt` around lines 205 - 212, Add a task-execution test alongside the existing JsonAssetInspector test that configures a fixture through the jsonAssets task, invokes ValidateJsonAssets.validate(), and verifies the generated report plus the expected success or failure result. Exercise the task wiring, warning aggregation, and failure propagation rather than calling JsonAssetInspector.inspect directly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@build-logic/src/main/kotlin/org/terasology/gradology/ValidateJsonAssets.kt`:
- Around line 66-70: In the hasTrailingContent calculation, narrow the catch
around reader.peek() to MalformedJsonException only. Preserve the true result
for malformed JSON, while allowing IOException and unexpected runtime failures
to propagate to the existing outer handler instead of being treated as valid
validation results.
---
Nitpick comments:
In
`@build-logic/src/test/kotlin/org/terasology/gradology/ValidateJsonAssetsTest.kt`:
- Around line 205-212: Add a task-execution test alongside the existing
JsonAssetInspector test that configures a fixture through the jsonAssets task,
invokes ValidateJsonAssets.validate(), and verifies the generated report plus
the expected success or failure result. Exercise the task wiring, warning
aggregation, and failure propagation rather than calling
JsonAssetInspector.inspect directly.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 93e0f2a6-fd98-459b-8fc7-007695e3805a
📒 Files selected for processing (2)
build-logic/src/main/kotlin/org/terasology/gradology/ValidateJsonAssets.ktbuild-logic/src/test/kotlin/org/terasology/gradology/ValidateJsonAssetsTest.kt
| val hasTrailingContent = try { | ||
| reader.peek() != JsonToken.END_DOCUMENT | ||
| } catch (e: Exception) { | ||
| true | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- target file ---'
sed -n '1,130p' build-logic/src/main/kotlin/org/terasology/gradology/ValidateJsonAssets.kt
printf '%s\n' '--- Gson declarations and dependency versions ---'
rg -n -i 'gson|MalformedJsonException|JsonReader|JsonToken' --glob '!**/build/**' --glob '!**/node_modules/**' .
printf '%s\n' '--- relevant Gradle files ---'
fd -t f . | rg '(^|/)(build.gradle|build.gradle.kts|settings.gradle|settings.gradle.kts|gradle.properties|libs.versions.toml)$' | xargs -r rg -n -i 'gson|com.google.code.gson'Repository: MovingBlocks/Terasology
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- focused tests ---'
sed -n '160,220p' build-logic/src/test/kotlin/org/terasology/gradology/ValidateJsonAssetsTest.kt
printf '%s\n' '--- local Gson 2.8.6 artifacts ---'
find "${HOME}/.gradle" -type f \( -name 'gson-2.8.6.jar' -o -name 'gson-2.8.6-sources.jar' \) -print 2>/dev/null | head -20
printf '%s\n' '--- Gson 2.8.6 JsonReader source references ---'
find . -type f \( -name 'JsonReader.java' -o -name 'MalformedJsonException.java' \) -printRepository: MovingBlocks/Terasology
Length of output: 2468
🌐 Web query:
Gson 2.8.6 JsonReader.peek doPeek MalformedJsonException stray closing brace source
💡 Result:
A MalformedJsonException with a message regarding a "stray closing brace" (or "unexpected" closing brace) in Gson generally indicates that the input JSON string is syntactically invalid according to the strict JSON specification [1][2]. When JsonReader.peek or doPeek throws this exception, it means the parser encountered a closing brace '}' where it did not expect one based on the current state of the JSON structure [3][1]. Common causes for this error include: 1. Malformed JSON structure: The most frequent cause is simply invalid JSON, such as a trailing comma, an extra closing brace, or mismatched braces that do not align with the object hierarchy [1][2]. 2. Trailing data: If the input contains valid JSON followed by extra characters (even non-visible characters like certain null terminators or trailing whitespace that does not strictly adhere to the parser's definition of whitespace), Gson will throw this exception because it expects the end of the document (EOF) [4][5]. 3. Server-side issues: Often, the source of the data (such as an API response) may be returning unexpected content, such as an HTML error page, a partial JSON string, or serialized data that is not proper JSON, despite the application expecting a valid JSON object [6][1]. To troubleshoot and resolve this: - Inspect the raw input: Log or print the exact string being passed to the Gson deserializer [1][2]. Do not rely on debuggers that might obscure invisible characters. - Check for trailing data: If your input has extra data after the valid JSON object, consider using a JsonReader and setting it to lenient mode via reader.setLenient(true) to allow the parser to be more permissive, though fixing the data source is the preferred solution [1][4]. - Verify formatting: Ensure that the JSON is properly encoded and does not contain illegal trailing commas or unescaped characters [1][2]. If you are encountering this in a production environment, the issue is almost certainly with the format of the incoming data stream, and "lenient" mode may serve as a temporary workaround rather than a root-cause fix [1][4].
Citations:
- 1: http://google.github.io/gson/Troubleshooting.html
- 2: https://github.com/google/gson/blob/gson-parent-2.13.2/Troubleshooting.md
- 3: https://github.com/google/gson/blob/master/gson/src/main/java/com/google/gson/stream/JsonReader.java
- 4: https://stackoverflow.com/questions/11484353/gson-throws-malformedjsonexception
- 5: Improve Gson and JsonParser trailing data handling google/gson#2123
- 6: https://stackoverflow.com/questions/73429466/malformedjsonexception-when-reading-response-using-retrofit2
🏁 Script executed:
#!/bin/bash
set -e
base='https://raw.githubusercontent.com/google/gson/gson-parent-2.8.6/gson/src/main/java/com/google/gson/stream'
tmp="$(mktemp -d)"
trap 'rm -rf "$tmp"' EXIT
curl -fsSL "$base/JsonReader.java" -o "$tmp/JsonReader.java"
curl -fsSL "$base/MalformedJsonException.java" -o "$tmp/MalformedJsonException.java"
python3 - "$tmp/JsonReader.java" "$tmp/MalformedJsonException.java" <<'PY'
from pathlib import Path
import re
import sys
reader = Path(sys.argv[1]).read_text()
malformed = Path(sys.argv[2]).read_text()
peek = re.search(r'public\s+JsonToken\s+peek\s*\(\s*\)\s*throws\s+([^{]+)\{', reader)
print("peek declaration:", peek.group(0).strip() if peek else "not found")
print("peek calls doPeek:", bool(re.search(r'JsonToken\s+peek\s*\([^)]*\).*?return\s+peeked\s*!=\s+null\s*\?\s+peeked\s*:\s*\(peeked\s*=\s*doPeek\(\)\)', reader, re.S)))
print("MalformedJsonException extends:", re.search(
r'class\s+MalformedJsonException\s+extends\s+([^{\s]+)', malformed
).group(1))
print("doPeek throws MalformedJsonException:", "MalformedJsonException" in reader)
print("doPeek throws IOException:", bool(re.search(r'JsonToken\s+doPeek\s*\(\s*\)\s*throws\s+IOException', reader)))
PYRepository: MovingBlocks/Terasology
Length of output: 369
Catch only MalformedJsonException around reader.peek().
JsonReader.peek() declares IOException, and MalformedJsonException extends IOException. Catching Exception also hides I/O and unexpected runtime failures, so validation can accept an asset that was not fully read. Let those failures reach the outer handler.
🧰 Tools
🪛 detekt (1.23.8)
[warning] 68-68: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@build-logic/src/main/kotlin/org/terasology/gradology/ValidateJsonAssets.kt`
around lines 66 - 70, In the hasTrailingContent calculation, narrow the catch
around reader.peek() to MalformedJsonException only. Preserve the true result
for malformed JSON, while allowing IOException and unexpected runtime failures
to propagate to the existing outer handler instead of being treated as valid
validation results.
Source: Linters/SAST tools
2 participants
This PR implements build-time validation for all JSON-based assets (such as .prefab, .block, .ui, and .json files) in both engine and modules.
It introduces a custom Gradle task that parses each asset and fails the build with a descriptive error if any file contains malformed JSON.
Modified files:
ValidateJsonAssets.kt:
New Gradle task that validates JSON assets at build time.
build.gradle.kts:
Adds the org.json dependency and test dependencies for the new task.
terasology-module.gradle.kts:
Registers the validation task for all modules and hooks it into the build process.
build.gradle.kts:
Registers the validation task for engine assets and hooks it into the build process.
build-logic/src/test/kotlin/org/terasology/gradology/ValidateJsonAssetsTest.kt:
Unit tests for the validation task, covering valid, invalid, and missing asset scenarios.
How to test:
Run a build (./gradlew build) with valid and invalid JSON assets to confirm that the build fails on malformed files and succeeds otherwise.
Run the included unit tests for the validation task.
Outstanding before merging:
Review if additional asset types should be included.
Documentation update if needed.
Closes #4986