From 25614710d54f0dc42e0926a70cd98b4c34b9e45f Mon Sep 17 00:00:00 2001 From: Moritz Halbritter Date: Thu, 30 Nov 2023 14:29:19 +0100 Subject: [PATCH] Fail if superfluous properties are used in property metadata Closes gh-37597 --- .../metadata/JsonMarshaller.java | 95 +++++++--- .../metadata/JsonMarshallerTests.java | 163 +++++++++++++++++- 2 files changed, 233 insertions(+), 25 deletions(-) diff --git a/spring-boot-project/spring-boot-tools/spring-boot-configuration-processor/src/main/java/org/springframework/boot/configurationprocessor/metadata/JsonMarshaller.java b/spring-boot-project/spring-boot-tools/spring-boot-configuration-processor/src/main/java/org/springframework/boot/configurationprocessor/metadata/JsonMarshaller.java index 9f049bb72b23..f5e28187fb0e 100644 --- a/spring-boot-project/spring-boot-tools/spring-boot-configuration-processor/src/main/java/org/springframework/boot/configurationprocessor/metadata/JsonMarshaller.java +++ b/spring-boot-project/spring-boot-tools/spring-boot-configuration-processor/src/main/java/org/springframework/boot/configurationprocessor/metadata/JsonMarshaller.java @@ -18,30 +18,31 @@ import java.io.IOException; import java.io.InputStream; -import java.io.InputStreamReader; import java.io.OutputStream; import java.nio.charset.StandardCharsets; import java.util.ArrayList; +import java.util.Arrays; import java.util.HashMap; import java.util.Iterator; import java.util.List; import java.util.Map; +import java.util.Set; +import java.util.TreeSet; import org.springframework.boot.configurationprocessor.json.JSONArray; import org.springframework.boot.configurationprocessor.json.JSONObject; import org.springframework.boot.configurationprocessor.metadata.ItemMetadata.ItemType; /** - * Marshaller to write {@link ConfigurationMetadata} as JSON. + * Marshaller to read and write {@link ConfigurationMetadata} as JSON. * * @author Stephane Nicoll * @author Phillip Webb + * @author Moritz Halbritter * @since 1.2.0 */ public class JsonMarshaller { - private static final int BUFFER_SIZE = 4098; - public void write(ConfigurationMetadata metadata, OutputStream outputStream) throws IOException { try { JSONObject object = new JSONObject(); @@ -65,42 +66,53 @@ public void write(ConfigurationMetadata metadata, OutputStream outputStream) thr public ConfigurationMetadata read(InputStream inputStream) throws Exception { ConfigurationMetadata metadata = new ConfigurationMetadata(); JSONObject object = new JSONObject(toString(inputStream)); + JsonPath path = JsonPath.root(); + checkAllowedKeys(object, path, "groups", "properties", "hints"); JSONArray groups = object.optJSONArray("groups"); if (groups != null) { for (int i = 0; i < groups.length(); i++) { - metadata.add(toItemMetadata((JSONObject) groups.get(i), ItemType.GROUP)); + metadata + .add(toItemMetadata((JSONObject) groups.get(i), path.resolve("groups").index(i), ItemType.GROUP)); } } JSONArray properties = object.optJSONArray("properties"); if (properties != null) { for (int i = 0; i < properties.length(); i++) { - metadata.add(toItemMetadata((JSONObject) properties.get(i), ItemType.PROPERTY)); + metadata.add(toItemMetadata((JSONObject) properties.get(i), path.resolve("properties").index(i), + ItemType.PROPERTY)); } } JSONArray hints = object.optJSONArray("hints"); if (hints != null) { for (int i = 0; i < hints.length(); i++) { - metadata.add(toItemHint((JSONObject) hints.get(i))); + metadata.add(toItemHint((JSONObject) hints.get(i), path.resolve("hints").index(i))); } } return metadata; } - private ItemMetadata toItemMetadata(JSONObject object, ItemType itemType) throws Exception { + private ItemMetadata toItemMetadata(JSONObject object, JsonPath path, ItemType itemType) throws Exception { + switch (itemType) { + case GROUP -> checkAllowedKeys(object, path, "name", "type", "description", "sourceType", "sourceMethod"); + case PROPERTY -> checkAllowedKeys(object, path, "name", "type", "description", "sourceType", "defaultValue", + "deprecation", "deprecated"); + } String name = object.getString("name"); String type = object.optString("type", null); String description = object.optString("description", null); String sourceType = object.optString("sourceType", null); String sourceMethod = object.optString("sourceMethod", null); Object defaultValue = readItemValue(object.opt("defaultValue")); - ItemDeprecation deprecation = toItemDeprecation(object); + ItemDeprecation deprecation = toItemDeprecation(object, path); return new ItemMetadata(itemType, name, null, type, sourceType, sourceMethod, description, defaultValue, deprecation); } - private ItemDeprecation toItemDeprecation(JSONObject object) throws Exception { + private ItemDeprecation toItemDeprecation(JSONObject object, JsonPath path) throws Exception { if (object.has("deprecation")) { JSONObject deprecationJsonObject = object.getJSONObject("deprecation"); + checkAllowedKeys(deprecationJsonObject, path.resolve("deprecation"), "level", "reason", "replacement", + "since"); ItemDeprecation deprecation = new ItemDeprecation(); deprecation.setLevel(deprecationJsonObject.optString("level", null)); deprecation.setReason(deprecationJsonObject.optString("reason", null)); @@ -111,32 +123,35 @@ private ItemDeprecation toItemDeprecation(JSONObject object) throws Exception { return object.optBoolean("deprecated") ? new ItemDeprecation() : null; } - private ItemHint toItemHint(JSONObject object) throws Exception { + private ItemHint toItemHint(JSONObject object, JsonPath path) throws Exception { + checkAllowedKeys(object, path, "name", "values", "providers"); String name = object.getString("name"); List values = new ArrayList<>(); if (object.has("values")) { JSONArray valuesArray = object.getJSONArray("values"); for (int i = 0; i < valuesArray.length(); i++) { - values.add(toValueHint((JSONObject) valuesArray.get(i))); + values.add(toValueHint((JSONObject) valuesArray.get(i), path.resolve("values").index(i))); } } List providers = new ArrayList<>(); if (object.has("providers")) { JSONArray providersObject = object.getJSONArray("providers"); for (int i = 0; i < providersObject.length(); i++) { - providers.add(toValueProvider((JSONObject) providersObject.get(i))); + providers.add(toValueProvider((JSONObject) providersObject.get(i), path.resolve("providers").index(i))); } } return new ItemHint(name, values, providers); } - private ItemHint.ValueHint toValueHint(JSONObject object) throws Exception { + private ItemHint.ValueHint toValueHint(JSONObject object, JsonPath path) throws Exception { + checkAllowedKeys(object, path, "value", "description"); Object value = readItemValue(object.get("value")); String description = object.optString("description", null); return new ItemHint.ValueHint(value, description); } - private ItemHint.ValueProvider toValueProvider(JSONObject object) throws Exception { + private ItemHint.ValueProvider toValueProvider(JSONObject object, JsonPath path) throws Exception { + checkAllowedKeys(object, path, "name", "parameters"); String name = object.getString("name"); Map parameters = new HashMap<>(); if (object.has("parameters")) { @@ -162,14 +177,48 @@ private Object readItemValue(Object value) throws Exception { } private String toString(InputStream inputStream) throws IOException { - StringBuilder out = new StringBuilder(); - InputStreamReader reader = new InputStreamReader(inputStream, StandardCharsets.UTF_8); - char[] buffer = new char[BUFFER_SIZE]; - int bytesRead; - while ((bytesRead = reader.read(buffer)) != -1) { - out.append(buffer, 0, bytesRead); - } - return out.toString(); + return new String(inputStream.readAllBytes(), StandardCharsets.UTF_8); + } + + @SuppressWarnings("unchecked") + private void checkAllowedKeys(JSONObject object, JsonPath path, String... allowedKeys) { + Set availableKeys = new TreeSet<>(); + object.keys().forEachRemaining((key) -> availableKeys.add((String) key)); + Arrays.stream(allowedKeys).forEach(availableKeys::remove); + if (!availableKeys.isEmpty()) { + throw new IllegalStateException("Expected only keys %s, but found additional keys %s. Path: %s" + .formatted(new TreeSet<>(Arrays.asList(allowedKeys)), availableKeys, path)); + } + } + + private static final class JsonPath { + + private final String path; + + private JsonPath(String path) { + this.path = path; + } + + JsonPath resolve(String path) { + if (this.path.endsWith(".")) { + return new JsonPath(this.path + path); + } + return new JsonPath(this.path + "." + path); + } + + JsonPath index(int index) { + return resolve("[%d]".formatted(index)); + } + + @Override + public String toString() { + return this.path; + } + + static JsonPath root() { + return new JsonPath("."); + } + } } diff --git a/spring-boot-project/spring-boot-tools/spring-boot-configuration-processor/src/test/java/org/springframework/boot/configurationprocessor/metadata/JsonMarshallerTests.java b/spring-boot-project/spring-boot-tools/spring-boot-configuration-processor/src/test/java/org/springframework/boot/configurationprocessor/metadata/JsonMarshallerTests.java index f7054f721200..e458efd52163 100644 --- a/spring-boot-project/spring-boot-tools/spring-boot-configuration-processor/src/test/java/org/springframework/boot/configurationprocessor/metadata/JsonMarshallerTests.java +++ b/spring-boot-project/spring-boot-tools/spring-boot-configuration-processor/src/test/java/org/springframework/boot/configurationprocessor/metadata/JsonMarshallerTests.java @@ -20,12 +20,14 @@ import java.io.ByteArrayOutputStream; import java.io.IOException; import java.io.InputStream; +import java.nio.charset.StandardCharsets; import java.util.Arrays; import java.util.Collections; import org.junit.jupiter.api.Test; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatException; /** * Tests for {@link JsonMarshaller}. @@ -38,14 +40,15 @@ class JsonMarshallerTests { @Test void marshallAndUnmarshal() throws Exception { ConfigurationMetadata metadata = new ConfigurationMetadata(); - metadata.add(ItemMetadata.newProperty("a", "b", StringBuffer.class.getName(), InputStream.class.getName(), - "sourceMethod", "desc", "x", new ItemDeprecation("Deprecation comment", "b.c.d", "1.2.3"))); + metadata.add(ItemMetadata.newProperty("a", "b", StringBuffer.class.getName(), InputStream.class.getName(), null, + "desc", "x", new ItemDeprecation("Deprecation comment", "b.c.d", "1.2.3"))); metadata.add(ItemMetadata.newProperty("b.c.d", null, null, null, null, null, null, null)); metadata.add(ItemMetadata.newProperty("c", null, null, null, null, null, 123, null)); metadata.add(ItemMetadata.newProperty("d", null, null, null, null, null, true, null)); metadata.add(ItemMetadata.newProperty("e", null, null, null, null, null, new String[] { "y", "n" }, null)); metadata.add(ItemMetadata.newProperty("f", null, null, null, null, null, new Boolean[] { true, false }, null)); metadata.add(ItemMetadata.newGroup("d", null, null, null)); + metadata.add(ItemMetadata.newGroup("e", null, null, "sourceMethod")); metadata.add(ItemHint.newHint("a.b")); metadata.add(ItemHint.newHint("c", new ItemHint.ValueHint(123, "hey"), new ItemHint.ValueHint(456, null))); metadata.add(new ItemHint("d", null, @@ -66,6 +69,7 @@ void marshallAndUnmarshal() throws Exception { assertThat(read).has(Metadata.withProperty("e").withDefaultValue(new String[] { "y", "n" })); assertThat(read).has(Metadata.withProperty("f").withDefaultValue(new Object[] { true, false })); assertThat(read).has(Metadata.withGroup("d")); + assertThat(read).has(Metadata.withGroup("e").fromSourceMethod("sourceMethod")); assertThat(read).has(Metadata.withHint("a.b")); assertThat(read).has(Metadata.withHint("c").withValue(0, 123, "hey").withValue(1, 456, null)); assertThat(read).has(Metadata.withHint("d").withProvider("first", "target", "foo").withProvider("second")); @@ -170,4 +174,159 @@ void orderingForSamePropertyNamesWithNullSourceType() throws IOException { "\"java.lang.Boolean\"", "\"com.example.bravo.aaa\"", "\"java.lang.Integer\"", "\"com.example.Bar"); } + @Test + void shouldCheckRootFields() { + String json = """ + { + "groups": [], "properties": [], "hints": [], "dummy": [] + }"""; + assertThatException().isThrownBy(() -> read(json)) + .withMessage("Expected only keys [groups, hints, properties], but found additional keys [dummy]. Path: ."); + } + + @Test + void shouldCheckGroupFields() { + String json = """ + { + "groups": [ + { + "name": "g", + "type": "java.lang.String", + "description": "Some description", + "sourceType": "java.lang.String", + "sourceMethod": "some()", + "dummy": "dummy" + } + ], "properties": [], "hints": [] + }"""; + assertThatException().isThrownBy(() -> read(json)) + .withMessage( + "Expected only keys [description, name, sourceMethod, sourceType, type], but found additional keys [dummy]. Path: .groups.[0]"); + } + + @Test + void shouldCheckPropertyFields() { + String json = """ + { + "groups": [], "properties": [ + { + "name": "name", + "type": "java.lang.String", + "description": "Some description", + "sourceType": "java.lang.String", + "defaultValue": "value", + "deprecation": { + "level": "warning", + "reason": "some reason", + "replacement": "name-new", + "since": "v17" + }, + "deprecated": true, + "dummy": "dummy" + } + ], "hints": [] + }"""; + assertThatException().isThrownBy(() -> read(json)) + .withMessage( + "Expected only keys [defaultValue, deprecated, deprecation, description, name, sourceType, type], but found additional keys [dummy]. Path: .properties.[0]"); + } + + @Test + void shouldCheckPropertyDeprecationFields() { + String json = """ + { + "groups": [], "properties": [ + { + "name": "name", + "type": "java.lang.String", + "description": "Some description", + "sourceType": "java.lang.String", + "defaultValue": "value", + "deprecation": { + "level": "warning", + "reason": "some reason", + "replacement": "name-new", + "since": "v17", + "dummy": "dummy" + }, + "deprecated": true + } + ], "hints": [] + }"""; + assertThatException().isThrownBy(() -> read(json)) + .withMessage( + "Expected only keys [level, reason, replacement, since], but found additional keys [dummy]. Path: .properties.[0].deprecation"); + } + + @Test + void shouldCheckHintFields() { + String json = """ + { + "groups": [], "properties": [], "hints": [ + { + "name": "name", + "values": [], + "providers": [], + "dummy": "dummy" + } + ] + }"""; + assertThatException().isThrownBy(() -> read(json)) + .withMessage( + "Expected only keys [name, providers, values], but found additional keys [dummy]. Path: .hints.[0]"); + } + + @Test + void shouldCheckHintValueFields() { + String json = """ + { + "groups": [], "properties": [], "hints": [ + { + "name": "name", + "values": [ + { + "value": "value", + "description": "some description", + "dummy": "dummy" + } + ], + "providers": [] + } + ] + }"""; + assertThatException().isThrownBy(() -> read(json)) + .withMessage( + "Expected only keys [description, value], but found additional keys [dummy]. Path: .hints.[0].values.[0]"); + } + + @Test + void shouldCheckHintProviderFields() { + String json = """ + { + "groups": [], "properties": [], "hints": [ + { + "name": "name", + "values": [], + "providers": [ + { + "name": "name", + "parameters": { + "target": "jakarta.servlet.http.HttpServlet" + }, + "dummy": "dummy" + } + ] + } + ] + }"""; + assertThatException().isThrownBy(() -> read(json)) + .withMessage( + "Expected only keys [name, parameters], but found additional keys [dummy]. Path: .hints.[0].providers.[0]"); + } + + private void read(String json) throws Exception { + JsonMarshaller marshaller = new JsonMarshaller(); + marshaller.read(new ByteArrayInputStream(json.getBytes(StandardCharsets.UTF_8))); + } + }