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 @@ 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 { private static final long serialVersionUID = -7040054009670771266L; - private Map> classes = new HashMap<>(); + private Map> 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 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 @@ 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 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 @@ 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); 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 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(); } } 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.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 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 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",