This is an automated email from the ASF dual-hosted git repository.
sergehuber pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/unomi.git
The following commit(s) were added to refs/heads/master by this push:
new 85fa6f215 UNOMI-952: Return HTTP 400 for malformed Item JSON in REST
requests (#777)
85fa6f215 is described below
commit 85fa6f215707d1be6f189321bfdf095710226f7e
Author: Serge Huber <[email protected]>
AuthorDate: Sun Jun 21 21:49:18 2026 +0200
UNOMI-952: Return HTTP 400 for malformed Item JSON in REST requests (#777)
---
.../unomi/persistence/spi/ItemDeserializer.java | 45 +++++++-
.../persistence/spi/ItemDeserializerTest.java | 124 +++++++++++++++++++++
.../exception/AbstractRestExceptionMapper.java | 2 +
.../apache/unomi/rest/exception/LogSanitizer.java | 4 +
.../rest/exception/RuntimeExceptionMapper.java | 7 +-
.../rest/exception/RestExceptionMapperTest.java | 18 ++-
.../impl/scheduler/SchedulerServiceImplTest.java | 7 +-
7 files changed, 190 insertions(+), 17 deletions(-)
diff --git
a/persistence-spi/src/main/java/org/apache/unomi/persistence/spi/ItemDeserializer.java
b/persistence-spi/src/main/java/org/apache/unomi/persistence/spi/ItemDeserializer.java
index 32cc13515..4b8265e3e 100644
---
a/persistence-spi/src/main/java/org/apache/unomi/persistence/spi/ItemDeserializer.java
+++
b/persistence-spi/src/main/java/org/apache/unomi/persistence/spi/ItemDeserializer.java
@@ -20,19 +20,21 @@ package org.apache.unomi.persistence.spi;
import com.fasterxml.jackson.core.JsonParser;
import com.fasterxml.jackson.core.ObjectCodec;
import com.fasterxml.jackson.databind.DeserializationContext;
+import com.fasterxml.jackson.databind.JsonMappingException;
+import com.fasterxml.jackson.databind.JsonNode;
import com.fasterxml.jackson.databind.deser.std.StdDeserializer;
import com.fasterxml.jackson.databind.node.ObjectNode;
import org.apache.unomi.api.CustomItem;
import org.apache.unomi.api.Item;
import java.io.IOException;
-import java.util.HashMap;
import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
public class ItemDeserializer extends StdDeserializer<Item> {
private static final long serialVersionUID = -7040054009670771266L;
- private Map<String,Class<? extends Item>> classes = new HashMap<>();
+ private Map<String, Class<? extends Item>> classes = new
ConcurrentHashMap<>();
public ItemDeserializer() {
super(Item.class);
@@ -46,22 +48,55 @@ public class ItemDeserializer extends StdDeserializer<Item>
{
classes.remove(type);
}
+ @Override
+ public Item getNullValue(DeserializationContext ctxt) throws
JsonMappingException {
+ throw JsonMappingException.from(ctxt, "Item cannot be null");
+ }
+
@Override
public Item deserialize(JsonParser jp, DeserializationContext ctxt) throws
IOException {
ObjectCodec codec = jp.getCodec();
- ObjectNode treeNode = codec.readTree(jp);
- String type = treeNode.get("itemType").textValue();
+ JsonNode jsonNode = codec.readTree(jp);
+ if (!jsonNode.isObject()) {
+ throw JsonMappingException.from(jp, "Expected a JSON object to
deserialize an Item but got "
+ + describeJsonNode(jsonNode));
+ }
+ ObjectNode treeNode = (ObjectNode) jsonNode;
+ JsonNode itemTypeNode = treeNode.get("itemType");
+ if (itemTypeNode == null || !itemTypeNode.isTextual()) {
+ throw JsonMappingException.from(jp, "Item JSON object must contain
a textual itemType property");
+ }
+ String type = itemTypeNode.textValue();
+ JsonNode itemIdNode = treeNode.get("itemId");
+ if (itemIdNode == null || !itemIdNode.isTextual()) {
+ throw JsonMappingException.from(jp, "Item JSON object must contain
a textual itemId property");
+ }
Class<? extends Item> objectClass = classes.get(type);
if (objectClass == null) {
objectClass = CustomItem.class;
} else {
+ // Registered Item subclasses don't declare itemType as a Jackson
field; remove it
+ // so treeToValue doesn't fail with an unknown-property error.
treeNode.remove("itemType");
}
Item item = codec.treeToValue(treeNode, objectClass);
- item.setItemId(treeNode.get("itemId").asText());
+ if (item == null) {
+ throw JsonMappingException.from(jp, "Deserializing itemType '" +
type + "' produced a null Item");
+ }
+ item.setItemId(itemIdNode.textValue());
if (item instanceof CustomItem) {
((CustomItem) item).setCustomItemType(type);
}
return item;
}
+
+ private static String describeJsonNode(JsonNode jsonNode) {
+ if (jsonNode.isTextual()) {
+ return "a string";
+ }
+ if (jsonNode.isArray()) {
+ return "an array";
+ }
+ return "a " + jsonNode.getNodeType().name().toLowerCase();
+ }
}
diff --git
a/persistence-spi/src/test/java/org/apache/unomi/persistence/spi/ItemDeserializerTest.java
b/persistence-spi/src/test/java/org/apache/unomi/persistence/spi/ItemDeserializerTest.java
new file mode 100644
index 000000000..50a34a24e
--- /dev/null
+++
b/persistence-spi/src/test/java/org/apache/unomi/persistence/spi/ItemDeserializerTest.java
@@ -0,0 +1,124 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.unomi.persistence.spi;
+
+import com.fasterxml.jackson.databind.JsonMappingException;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.fasterxml.jackson.databind.module.SimpleModule;
+import org.apache.unomi.api.CustomItem;
+import org.apache.unomi.api.Item;
+import org.apache.unomi.api.Profile;
+import org.junit.Before;
+import org.junit.Test;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertTrue;
+
+public class ItemDeserializerTest {
+
+ private ObjectMapper objectMapper;
+
+ @Before
+ public void setUp() {
+ objectMapper = new ObjectMapper();
+ SimpleModule module = new SimpleModule();
+ module.addDeserializer(Item.class, new ItemDeserializer());
+ objectMapper.registerModule(module);
+ }
+
+ @Test
+ public void deserialize_validCustomItem() throws Exception {
+ Item item =
objectMapper.readValue("{\"itemType\":\"page\",\"itemId\":\"home\"}",
Item.class);
+ assertNotNull(item);
+ assertEquals("home", item.getItemId());
+ assertTrue(item instanceof CustomItem);
+ assertEquals("page", ((CustomItem) item).getCustomItemType());
+ }
+
+ @Test(expected = JsonMappingException.class)
+ public void deserialize_nullItem_throwsJsonMappingException() throws
Exception {
+ objectMapper.readValue("null", Item.class);
+ }
+
+ @Test(expected = JsonMappingException.class)
+ public void deserialize_stringValue_throwsJsonMappingException() throws
Exception {
+ objectMapper.readValue("\"not-an-object\"", Item.class);
+ }
+
+ @Test(expected = JsonMappingException.class)
+ public void deserialize_arrayValue_throwsJsonMappingException() throws
Exception {
+
objectMapper.readValue("[{\"itemType\":\"page\",\"itemId\":\"home\"}]",
Item.class);
+ }
+
+ @Test(expected = JsonMappingException.class)
+ public void deserialize_missingItemId_throwsJsonMappingException() throws
Exception {
+ objectMapper.readValue("{\"itemType\":\"page\"}", Item.class);
+ }
+
+ @Test(expected = JsonMappingException.class)
+ public void deserialize_missingItemType_throwsJsonMappingException()
throws Exception {
+ objectMapper.readValue("{\"itemId\":\"home\"}", Item.class);
+ }
+
+ @Test(expected = JsonMappingException.class)
+ public void deserialize_booleanValue_throwsJsonMappingException() throws
Exception {
+ objectMapper.readValue("true", Item.class);
+ }
+
+ @Test(expected = JsonMappingException.class)
+ public void deserialize_numericItemType_throwsJsonMappingException()
throws Exception {
+ objectMapper.readValue("{\"itemType\":42,\"itemId\":\"home\"}",
Item.class);
+ }
+
+ @Test(expected = JsonMappingException.class)
+ public void deserialize_numericItemId_throwsJsonMappingException() throws
Exception {
+ objectMapper.readValue("{\"itemType\":\"page\",\"itemId\":99}",
Item.class);
+ }
+
+ @Test
+ public void deserialize_registeredMapping_usesRegisteredClass() throws
Exception {
+ ItemDeserializer deserializer = new ItemDeserializer();
+ deserializer.registerMapping(Profile.ITEM_TYPE, Profile.class);
+ SimpleModule module = new SimpleModule();
+ module.addDeserializer(Item.class, deserializer);
+ ObjectMapper profileMapper = new ObjectMapper();
+ profileMapper.registerModule(module);
+
+ Item item =
profileMapper.readValue("{\"itemType\":\"profile\",\"itemId\":\"p1\"}",
Item.class);
+ assertNotNull(item);
+ assertTrue(item instanceof Profile);
+ assertEquals("p1", item.getItemId());
+ }
+
+ @Test
+ public void deserialize_unregisterMapping_fallsBackToCustomItem() throws
Exception {
+ ItemDeserializer deserializer = new ItemDeserializer();
+ deserializer.registerMapping(Profile.ITEM_TYPE, Profile.class);
+ deserializer.unregisterMapping(Profile.ITEM_TYPE);
+ SimpleModule module = new SimpleModule();
+ module.addDeserializer(Item.class, deserializer);
+ ObjectMapper mapper = new ObjectMapper();
+ mapper.registerModule(module);
+
+ Item item =
mapper.readValue("{\"itemType\":\"profile\",\"itemId\":\"p1\"}", Item.class);
+ assertNotNull(item);
+ assertTrue(item instanceof CustomItem);
+ assertEquals("profile", ((CustomItem) item).getCustomItemType());
+ assertEquals("p1", item.getItemId());
+ }
+}
diff --git
a/rest/src/main/java/org/apache/unomi/rest/exception/AbstractRestExceptionMapper.java
b/rest/src/main/java/org/apache/unomi/rest/exception/AbstractRestExceptionMapper.java
index e5fe695b7..5f2f55045 100644
---
a/rest/src/main/java/org/apache/unomi/rest/exception/AbstractRestExceptionMapper.java
+++
b/rest/src/main/java/org/apache/unomi/rest/exception/AbstractRestExceptionMapper.java
@@ -74,6 +74,8 @@ public abstract class AbstractRestExceptionMapper {
return null;
}
Throwable current = throwable;
+ // Standard Java Throwable.initCause() prevents circular cause chains,
so the visited-set
+ // cycle guard below is defensive and not reachable in practice.
java.util.Set<Throwable> visited =
java.util.Collections.newSetFromMap(new java.util.IdentityHashMap<>());
while (current.getCause() != null && current.getCause() != current &&
visited.add(current)) {
current = current.getCause();
diff --git
a/rest/src/main/java/org/apache/unomi/rest/exception/LogSanitizer.java
b/rest/src/main/java/org/apache/unomi/rest/exception/LogSanitizer.java
index 4b40d7516..e9e2f4f39 100644
--- a/rest/src/main/java/org/apache/unomi/rest/exception/LogSanitizer.java
+++ b/rest/src/main/java/org/apache/unomi/rest/exception/LogSanitizer.java
@@ -32,6 +32,7 @@ final class LogSanitizer {
private static final int MAX_URL_LENGTH = 500;
private static final int MAX_QUERY_STRING_LENGTH = 200;
+ private static final int MAX_MESSAGE_LENGTH = 500;
private static final int MAX_CLASS_NAME_LENGTH = 100;
private static final int MAX_METHOD_LENGTH = 10;
private static final int MAX_QUERY_PARAMS = 10;
@@ -52,6 +53,9 @@ final class LogSanitizer {
if (input == null) {
return "";
}
+ if (input.length() > MAX_MESSAGE_LENGTH) {
+ input = input.substring(0, MAX_MESSAGE_LENGTH) + "...[truncated]";
+ }
StringBuilder sanitized = new StringBuilder(input.length());
for (int i = 0; i < input.length(); i++) {
char c = input.charAt(i);
diff --git
a/rest/src/main/java/org/apache/unomi/rest/exception/RuntimeExceptionMapper.java
b/rest/src/main/java/org/apache/unomi/rest/exception/RuntimeExceptionMapper.java
index 1b50558a3..a87cfbcc8 100644
---
a/rest/src/main/java/org/apache/unomi/rest/exception/RuntimeExceptionMapper.java
+++
b/rest/src/main/java/org/apache/unomi/rest/exception/RuntimeExceptionMapper.java
@@ -39,8 +39,9 @@ public class RuntimeExceptionMapper extends
AbstractRestExceptionMapper implemen
? rootCause.getMessage()
: (exception.getMessage() != null ? exception.getMessage() :
""));
- // For client errors (like deserialization), log at WARN level. For
true server errors, log at ERROR level.
- if (isJsonDeserializationError(rootCause)) {
+ // Client errors (deserialization) are logged at WARN; genuine server
faults at ERROR.
+ boolean isDeserializationError = isJsonDeserializationError(rootCause);
+ if (isDeserializationError) {
LOGGER.warn(
"Bad request on {} - Root cause: {} - {} (Set
RuntimeExceptionMapper to debug to get the full stacktrace)",
requestContext, rootCauseClassName, rootCauseMessage);
@@ -51,6 +52,6 @@ public class RuntimeExceptionMapper extends
AbstractRestExceptionMapper implemen
}
LOGGER.debug("Full exception details for request: {}", requestContext,
exception);
- return internalServerErrorResponse();
+ return isDeserializationError ? badRequestResponse() :
internalServerErrorResponse();
}
}
diff --git
a/rest/src/test/java/org/apache/unomi/rest/exception/RestExceptionMapperTest.java
b/rest/src/test/java/org/apache/unomi/rest/exception/RestExceptionMapperTest.java
index 8594a89c7..4ca8ce885 100644
---
a/rest/src/test/java/org/apache/unomi/rest/exception/RestExceptionMapperTest.java
+++
b/rest/src/test/java/org/apache/unomi/rest/exception/RestExceptionMapperTest.java
@@ -28,9 +28,9 @@ import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertTrue;
/**
- * Unit tests for the REST exception mappers (UNOMI-928). These verify the
status code and JSON body
- * contract directly, without a running container, so the mapper logic is
validated deterministically
- * regardless of how a given endpoint deserializes its request body.
+ * Unit tests for the REST exception mappers (UNOMI-928, UNOMI-952). These
verify the status code
+ * and JSON body contract directly, without a running container, so the mapper
logic is validated
+ * deterministically regardless of how a given endpoint deserializes its
request body.
*/
class RestExceptionMapperTest {
@@ -48,12 +48,18 @@ class RestExceptionMapperTest {
}
@Test
- void runtimeException_withJsonCause_stillMapsToInternalServerError() {
- // RuntimeExceptionMapper only downgrades the log level for JSON
causes; the response stays 500.
+ void runtimeException_withJsonMappingCause_mapsToBadRequest() {
RuntimeException exception = new RuntimeException(
JsonMappingException.from((com.fasterxml.jackson.core.JsonParser) null, "cannot
deserialize"));
Response response = new RuntimeExceptionMapper().toResponse(exception);
- assertErrorResponse(response, 500, "internalServerError");
+ assertErrorResponse(response, 400, "badRequest");
+ }
+
+ @Test
+ void runtimeException_withJsonParseCause_mapsToBadRequest() {
+ RuntimeException exception = new RuntimeException(new
JsonParseException(null, "malformed"));
+ Response response = new RuntimeExceptionMapper().toResponse(exception);
+ assertErrorResponse(response, 400, "badRequest");
}
@Test
diff --git
a/services/src/test/java/org/apache/unomi/services/impl/scheduler/SchedulerServiceImplTest.java
b/services/src/test/java/org/apache/unomi/services/impl/scheduler/SchedulerServiceImplTest.java
index 778c14de8..3a1b04b92 100644
---
a/services/src/test/java/org/apache/unomi/services/impl/scheduler/SchedulerServiceImplTest.java
+++
b/services/src/test/java/org/apache/unomi/services/impl/scheduler/SchedulerServiceImplTest.java
@@ -2435,14 +2435,15 @@ public class SchedulerServiceImplTest {
assertTrue(
firstExecutionLatch.await(TEST_TIMEOUT, TEST_TIME_UNIT),
"Task should execute with dedicated executor");
+
+ // Stop the scheduler before asserting: the task runs at 100ms
fixed-rate, so on a loaded
+ // CI runner the second tick can fire between the latch release and
the assertEquals.
+ schedulerService.preDestroy();
assertEquals(1, executionCount.get(), "Task should execute once");
// Force refresh to ensure the task is properly saved
persistenceService.refreshIndex(ScheduledTask.class);
- // Now shut down and restart the scheduler
- schedulerService.preDestroy();
-
// Create a new scheduler service
SchedulerServiceImpl newSchedulerService = (SchedulerServiceImpl)
TestHelper.createSchedulerService(
"dedicated-executor-scheduler-node",
