diff --git a/build-logic/build.gradle.kts b/build-logic/build.gradle.kts index ec9b67c813f..ecd8209912e 100644 --- a/build-logic/build.gradle.kts +++ b/build-logic/build.gradle.kts @@ -64,6 +64,11 @@ dependencies { // for inspecting modules implementation("org.terasology.gestalt:gestalt-module:8.0.1-SNAPSHOT") + // JSON parsing for build-time asset validation. + // Must match the engine's parser (settings.gradle.kts pins gson 2.8.6) so that validation + // accepts exactly what the engine's asset loaders accept - see ValidateJsonAssets. + implementation("com.google.code.gson:gson:2.8.6") + // plugins we configure implementation("com.github.spotbugs.snom:spotbugs-gradle-plugin:5.2.3") implementation("org.sonarsource.scanner.gradle:sonarqube-gradle-plugin:5.0.0.4638") diff --git a/build-logic/src/main/kotlin/org/terasology/gradology/ValidateJsonAssets.kt b/build-logic/src/main/kotlin/org/terasology/gradology/ValidateJsonAssets.kt new file mode 100644 index 00000000000..e577a49ec0b --- /dev/null +++ b/build-logic/src/main/kotlin/org/terasology/gradology/ValidateJsonAssets.kt @@ -0,0 +1,208 @@ +// Copyright 2024 The Terasology Foundation +// SPDX-License-Identifier: Apache-2.0 + +package org.terasology.gradology + +import com.google.gson.stream.JsonReader +import com.google.gson.stream.JsonToken +import org.gradle.api.DefaultTask +import org.gradle.api.GradleException +import org.gradle.api.file.ConfigurableFileCollection +import org.gradle.api.file.RegularFileProperty +import org.gradle.api.tasks.InputFiles +import org.gradle.api.tasks.OutputFile +import org.gradle.api.tasks.PathSensitive +import org.gradle.api.tasks.PathSensitivity +import org.gradle.api.tasks.SkipWhenEmpty +import org.gradle.api.tasks.TaskAction +import java.io.File + +/** + * The outcome of inspecting a single JSON asset. + * + * @property error why the file could not be parsed at all, or null if it parsed cleanly + * @property warnings problems that do not stop the engine loading the file, but are still defects + */ +data class AssetInspection(val error: String?, val warnings: List) + +/** + * Parses Terasology JSON assets the same way the engine does. + * + * Parsing deliberately mirrors the engine's own asset loaders rather than strict RFC 8259: + * `UIFormat` and `UISkinFormat` call `JsonReader.setLenient(true)` outright, and the block and + * prefab formats go through `Gson.fromJson`, which is lenient by default. Terasology's asset + * format therefore permits slash-star licence headers and double-slash inline notes, and a large + * share of shipped assets use them - `CoreAssets` alone has dozens. A strict parser here would + * reject content the engine loads happily. + * + * The contract that follows from this: **an error is something the engine genuinely cannot load; + * a warning is something it loads despite the file being defective.** Duplicate keys and trailing + * content are both warnings for that reason - Gson keeps the last duplicate, and the loaders read + * a single root value without checking what comes after it. Anything that fails the parse outright + * fails the build, because the engine would fail on it too. + * + * Kept free of Gradle types so it can be tested directly, without standing up a nested build. + */ +object JsonAssetInspector { + + fun inspect(file: File): AssetInspection { + val warnings = mutableListOf() + return try { + file.bufferedReader().use { source -> + val reader = JsonReader(source) + reader.isLenient = true + + if (reader.peek() == JsonToken.END_DOCUMENT) { + return AssetInspection("${file.path}: file is empty", warnings) + } + + walk(reader, file, "", warnings) + + // Content after the root value is a defect, but not a fatal one: the engine's + // loaders read a single value and never check what follows, so the file still + // loads. Peeking can itself throw when the trailing bytes are not the start of a + // value (a stray closing brace, say), so that has to be caught here rather than + // by the outer handler - otherwise it would be reported as a parse failure. + val hasTrailingContent = try { + reader.peek() != JsonToken.END_DOCUMENT + } catch (e: Exception) { + true + } + + if (hasTrailingContent) { + warnings.add( + "${file.path}: unexpected content after the root value" + + " - the engine reads the first value and silently ignores the rest" + ) + } + } + AssetInspection(null, warnings) + } catch (e: Exception) { + AssetInspection("${file.path}: ${e.message ?: e.javaClass.simpleName}", warnings) + } + } + + /** + * Walk the whole token stream. Reading every token is what proves the document parses; + * tracking the names seen per object is what surfaces duplicate keys. + * + * Duplicate keys are warnings rather than errors: Gson silently keeps the last occurrence, so + * a duplicate never breaks loading - but it does mean an earlier value is being discarded + * without anyone noticing, which is nearly always a mistake. + */ + private fun walk(reader: JsonReader, file: File, path: String, warnings: MutableList) { + when (reader.peek()) { + JsonToken.BEGIN_OBJECT -> { + reader.beginObject() + val seen = mutableSetOf() + while (reader.hasNext()) { + val name = reader.nextName() + val childPath = if (path.isEmpty()) name else "$path.$name" + if (!seen.add(name)) { + warnings.add( + "${file.path}: duplicate key \"$name\" at $childPath" + + " - the last occurrence wins, the earlier value is silently discarded" + ) + } + walk(reader, file, childPath, warnings) + } + reader.endObject() + } + + JsonToken.BEGIN_ARRAY -> { + reader.beginArray() + var index = 0 + while (reader.hasNext()) { + walk(reader, file, "$path[${index++}]", warnings) + } + reader.endArray() + } + + else -> reader.skipValue() + } + } +} + +/** + * Gradle task that validates JSON assets (prefabs, blocks, ui, etc.) at build time. + * + * Iterates over all configured JSON asset files and attempts to parse each one. + * If any file cannot be parsed, the build fails with a descriptive error message. + * See [JsonAssetInspector] for what counts as parseable, and why it is not strict JSON. + * + * Example usage in a build script: + * ```kotlin + * tasks.register("validateJsonAssets") { + * source(fileTree("assets") { include("**/*.prefab", "**/*.json") }) + * } + * ``` + */ +abstract class ValidateJsonAssets : DefaultTask() { + + init { + group = "Verification" + description = "Validates that all JSON assets (prefabs, blocks, ui, etc.) are well-formed." + } + + /** + * The set of JSON asset files to validate. + * Use [source] to add file trees. + */ + @get:InputFiles + @get:SkipWhenEmpty + @get:PathSensitive(PathSensitivity.RELATIVE) + val jsonAssets: ConfigurableFileCollection = project.files() + + /** + * Where the findings are written. + * + * This exists mainly so the task has a declared output. Without one Gradle has no up-to-date + * criterion and re-parses every asset on every build - which matters, because the + * `terasology-module` plugin wires this into `processResources` for every module. With it, + * an unchanged asset tree is skipped outright and the task can be served from the build cache. + */ + @get:OutputFile + val report: RegularFileProperty = project.objects.fileProperty() + .convention(project.layout.buildDirectory.file("reports/json-assets/validation.txt")) + + /** + * Add files or file trees of JSON assets to validate. + */ + fun source(vararg paths: Any) { + jsonAssets.from(*paths) + } + + @TaskAction + fun validate() { + val errors = mutableListOf() + val warnings = mutableListOf() + + for (file in jsonAssets) { + val inspection = JsonAssetInspector.inspect(file) + inspection.error?.let { errors.add(it) } + warnings.addAll(inspection.warnings) + } + + warnings.forEach { logger.warn(" ! $it") } + + val reportFile = report.get().asFile + reportFile.parentFile.mkdirs() + reportFile.writeText(buildString { + appendLine("checked: ${jsonAssets.count()}") + appendLine("errors: ${errors.size}") + appendLine("warnings: ${warnings.size}") + errors.forEach { appendLine("ERROR $it") } + warnings.forEach { appendLine("WARN $it") } + }) + + if (errors.isNotEmpty()) { + val message = buildString { + appendLine("Found ${errors.size} invalid JSON asset(s):") + errors.forEach { appendLine(" - $it") } + } + throw GradleException(message) + } + + logger.lifecycle("All JSON assets are valid.") + } +} diff --git a/build-logic/src/main/kotlin/terasology-module.gradle.kts b/build-logic/src/main/kotlin/terasology-module.gradle.kts index 33adefd9122..2b1d78311fe 100644 --- a/build-logic/src/main/kotlin/terasology-module.gradle.kts +++ b/build-logic/src/main/kotlin/terasology-module.gradle.kts @@ -6,6 +6,7 @@ import org.gradle.plugins.ide.eclipse.model.EclipseModel import org.gradle.plugins.ide.idea.model.IdeaModel import org.terasology.gradology.ModuleMetadataForGradle +import org.terasology.gradology.ValidateJsonAssets plugins { `java-library` @@ -138,9 +139,24 @@ tasks.register("syncModuleInfo") { into(mainSourceSet.output.classesDirs.first()) } +// Validate all JSON assets (prefabs, blocks, ui, etc.) at build time +tasks.register("validateJsonAssets") { + val assetsDir = project.file("assets") + if (assetsDir.exists()) { + listOf("prefabs", "blocks", "blockSounds", "ui", "shapes", "materials", "fonts", "behaviors").forEach { assetType -> + val dir = assetsDir.resolve(assetType) + if (dir.exists()) { + source(project.fileTree(dir) { include("**/*.json", "**/*.prefab", "**/*.block", "**/*.ui") }) + } + } + // Also catch any other .json files directly in assets + source(project.fileTree(assetsDir) { include("**/*.json") }) + } +} + tasks.named("processResources") { // Make sure the assets directory is included - dependsOn("syncAssets", "syncOverrides", "syncDeltas", "syncModuleInfo") + dependsOn("syncAssets", "syncOverrides", "syncDeltas", "syncModuleInfo", "validateJsonAssets") } tasks.named("compileJava") { diff --git a/build-logic/src/test/kotlin/org/terasology/gradology/ValidateJsonAssetsTest.kt b/build-logic/src/test/kotlin/org/terasology/gradology/ValidateJsonAssetsTest.kt new file mode 100644 index 00000000000..2f0a0928ed7 --- /dev/null +++ b/build-logic/src/test/kotlin/org/terasology/gradology/ValidateJsonAssetsTest.kt @@ -0,0 +1,248 @@ +// Copyright 2024 The Terasology Foundation +// SPDX-License-Identifier: Apache-2.0 + +package org.terasology.gradology + +import org.gradle.testfixtures.ProjectBuilder +import java.io.File +import java.nio.file.Files +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertNotNull +import kotlin.test.assertNull +import kotlin.test.assertTrue + +/** + * Tests exercise [JsonAssetInspector] directly rather than through Gradle TestKit. + * + * TestKit was tried first and does not work for this task: `withPluginClasspath()` makes the + * plugin available to the `plugins {}` DSL, but not to the build script's *compile* classpath, so + * a generated script that does `import org.terasology.gradology.ValidateJsonAssets` fails with + * "Unresolved reference". That failure is the worst kind - the build under test dies for a reason + * unrelated to the assertion, so the tests never exercised the validator at all. Calling the + * inspector directly is both honest and considerably faster. + */ +class ValidateJsonAssetsTest { + + /** + * Verifies that the task can be registered and configured on a Gradle project. + */ + @Test + fun `task can be registered on a project`() { + val project = ProjectBuilder.builder().build() + val task = project.tasks.register("validateJsonAssets", ValidateJsonAssets::class.java).get() + assertNotNull(task) + assertEquals("Verification", task.group) + } + + /** + * Verifies that valid JSON files pass validation without errors. + */ + @Test + fun `valid JSON files pass validation`() { + val file = writeAsset("player.prefab", """ + { + "persisted": true, + "Location": {}, + "Network": { "replicateMode": "ALWAYS" } + } + """.trimIndent()) + + val inspection = JsonAssetInspector.inspect(file) + + assertNull(inspection.error, "expected a clean parse, got: ${inspection.error}") + assertTrue(inspection.warnings.isEmpty()) + } + + /** + * Verifies that malformed JSON is reported, naming the offending file. + */ + @Test + fun `malformed JSON is reported as an error`() { + val file = writeAsset("broken.prefab", """ + { + "persisted": true, + "Location": { + """.trimIndent()) + + val inspection = JsonAssetInspector.inspect(file) + + assertNotNull(inspection.error) + assertTrue( + inspection.error!!.contains("broken.prefab"), + "Expected the error to name the file, got: ${inspection.error}" + ) + } + + /** + * The regression guard for this task's original defect. + * + * Terasology's asset format is lenient JSON, not RFC 8259. Shipped assets rely on that - a + * licence header or an inline note is the norm, not the exception. Validating with a strict + * parser (the task originally used org.json) fails the build on content the engine loads + * fine: every module sampled failed, `CoreAssets` alone with 56 files. + * + * The fixture is modelled directly on `CoreAssets/assets/blocks/soil/Snowball.block` and + * `CakeLie/assets/blocks/ChocolateBlock.block`. + */ + @Test + fun `assets using engine-style comments pass validation`() { + val file = writeAsset("Chocolate.block", """ + /* + * Copyright 2014 MovingBlocks + * + * Licensed under the Apache License, Version 2.0 (the "License"); + */ + { + // Graphics + "displayName": "Chocolate", + "basedOn": "CoreAssets:soil", + //no prefab I could find; WIP? + "inventory": { + "stackable": true + } + } + """.trimIndent()) + + val inspection = JsonAssetInspector.inspect(file) + + assertNull(inspection.error, "comments are valid in Terasology assets, got: ${inspection.error}") + } + + /** + * Trailing commas are likewise accepted by the engine's lenient parser. + * `moduleDetailsScreen.ui` has one. + */ + @Test + fun `trailing commas pass validation`() { + val file = writeAsset("trailing.ui", """ + { + "contents": [ + { "type": "UILabel" }, + ] + } + """.trimIndent()) + + assertNull(JsonAssetInspector.inspect(file).error) + } + + /** + * Duplicate keys are real defects but do not break loading - Gson keeps the last occurrence - + * so they are reported without failing the build. Found in the engine's own + * `moduleDetailsScreen.ui`, which declared `layoutInfo` twice on one widget. + */ + @Test + fun `duplicate keys warn without failing`() { + val file = writeAsset("duplicate.prefab", """ + { + "layoutInfo": { "position-top": { "target": "TOP" } }, + "other": 1, + "layoutInfo": { "position-top": { "target": "BOTTOM" } } + } + """.trimIndent()) + + val inspection = JsonAssetInspector.inspect(file) + + assertNull(inspection.error, "a duplicate key must not fail the build") + assertEquals(1, inspection.warnings.size, "expected exactly one warning: ${inspection.warnings}") + assertTrue(inspection.warnings.single().contains("duplicate key \"layoutInfo\"")) + } + + /** + * Duplicate keys are detected at any depth, not just at the root. + */ + @Test + fun `duplicate keys are detected in nested objects`() { + val file = writeAsset("nested.prefab", """ + { + "outer": { + "inner": [ + { "a": 1, "a": 2 } + ] + } + } + """.trimIndent()) + + val inspection = JsonAssetInspector.inspect(file) + + assertNull(inspection.error) + assertTrue( + inspection.warnings.single().contains("outer.inner[0].a"), + "Expected the warning to carry the JSON path, got: ${inspection.warnings}" + ) + } + + /** + * An empty asset file is a packaging mistake rather than valid content. + */ + @Test + fun `empty file is reported as an error`() { + val file = writeAsset("empty.prefab", "") + + assertNotNull(JsonAssetInspector.inspect(file).error) + } + + /** + * A stray closing brace after the root object warns rather than failing. + * + * The engine's loaders read one root value and never check what follows, so the file loads + * fine - failing the build would punish a defect the engine does not care about. Modelled on + * `Apiculture/assets/ui/extractor.ui`, found by the first full Omega sweep. + * + * Note this shape makes `JsonReader.peek()` itself throw, so it is not enough to compare + * against END_DOCUMENT; the regression here is the error/warning classification. + */ + @Test + fun `trailing content after the root value warns without failing`() { + val file = writeAsset("extractor.ui", """ + { + "type": "UIBox", + "contents": [] + } + } + """.trimIndent()) + + val inspection = JsonAssetInspector.inspect(file) + + assertNull(inspection.error, "the engine loads this, so it must not fail the build") + assertTrue( + inspection.warnings.single().contains("unexpected content after the root value"), + "expected a trailing-content warning, got: ${inspection.warnings}" + ) + } + + /** + * A missing separator between object members is a hard failure - no JSON parser accepts it, + * lenient or not, so the engine cannot load the file either. + * + * Modelled on `Cooking/assets/prefabs/CookingRecipes.prefab`, which is broken in the shipped + * module: its recipes silently do not load today. + */ + @Test + fun `missing comma between members is an error`() { + val file = writeAsset("CookingRecipes.prefab", """ + { + "ListRecipes": { + "recipes": { + "Cooking:Coconut": { "outputCount": 1 } + "Cooking:BoiledEgg": { "outputCount": 1 } + } + } + } + """.trimIndent()) + + assertNotNull( + JsonAssetInspector.inspect(file).error, + "a missing separator must fail - the engine cannot parse it either" + ) + } + + private fun writeAsset(name: String, content: String): File { + val dir = Files.createTempDirectory("terasology-test").toFile() + dir.deleteOnExit() + return dir.resolve(name).also { + it.writeText(content) + it.deleteOnExit() + } + } +} diff --git a/engine/build.gradle.kts b/engine/build.gradle.kts index 4f82a8e34df..4e1fab5beeb 100644 --- a/engine/build.gradle.kts +++ b/engine/build.gradle.kts @@ -5,6 +5,7 @@ import java.time.OffsetDateTime import java.time.ZoneOffset import java.time.format.DateTimeFormatter +import org.terasology.gradology.ValidateJsonAssets plugins { id("java-library") @@ -205,6 +206,15 @@ tasks.named("processResources") { from("$rootDir/docs") { include("Credits.md") } + dependsOn("validateJsonAssets") +} + +// Validate all engine JSON assets (prefabs, etc.) at build time +tasks.register("validateJsonAssets") { + val resourcesDir = project.file("src/main/resources") + if (resourcesDir.exists()) { + source(project.fileTree(resourcesDir) { include("**/*.prefab", "**/*.block", "**/*.ui", "**/*.json") }) + } } //TODO: Remove this when gestalt can handle ProtectionDomain without classes (Resources) diff --git a/engine/src/main/resources/org/terasology/engine/assets/ui/menu/moduleDetailsScreen.ui b/engine/src/main/resources/org/terasology/engine/assets/ui/menu/moduleDetailsScreen.ui index 0a84627d36d..53ac2b63661 100644 --- a/engine/src/main/resources/org/terasology/engine/assets/ui/menu/moduleDetailsScreen.ui +++ b/engine/src/main/resources/org/terasology/engine/assets/ui/menu/moduleDetailsScreen.ui @@ -59,11 +59,6 @@ "columns": 2, "column-widths": [0.35, 0.65], "horizontalSpacing": 4, - "layoutInfo": { - "position-top": { - "target": "TOP" - } - }, "contents": [ { "id": "firstColumn",