UNOMI-952: Return HTTP 400 for malformed Item JSON in REST requests

persistence-spi/src/main/java/org/apache/unomi/persistence/spi/ItemDeserializer.java

@@ -20,19 +20,21 @@
 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 void unregisterMapping(String type) {
         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();
+    }
 }

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());
+    }
+}

rest/src/main/java/org/apache/unomi/rest/exception/AbstractRestExceptionMapper.java

@@ -74,6 +74,8 @@ protected Throwable getRootCause(Throwable throwable) {
             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();

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 @@ static String forLogging(String input) {
         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);

rest/src/main/java/org/apache/unomi/rest/exception/RuntimeExceptionMapper.java

@@ -39,8 +39,9 @@ public Response toResponse(RuntimeException exception) {
                 ? 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 Response toResponse(RuntimeException exception) {
         }
         LOGGER.debug("Full exception details for request: {}", requestContext, exception);
 
-        return internalServerErrorResponse();
+        return isDeserializationError ? badRequestResponse() : internalServerErrorResponse();
     }
 }

rest/src/test/java/org/apache/unomi/rest/exception/RestExceptionMapperTest.java

@@ -28,9 +28,9 @@
 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 @@ void runtimeException_mapsToInternalServerError() {
     }
 
     @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

services/src/test/java/org/apache/unomi/services/impl/scheduler/SchedulerServiceImplTest.java

@@ -2435,14 +2435,15 @@ public void execute(ScheduledTask task, TaskStatusCallback callback) {
         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",