From 46c953492b1a31184d0e5b36be9ba88fba9a7b38 Mon Sep 17 00:00:00 2001 From: "AAVN\\pvquan" Date: Mon, 16 Dec 2024 16:30:00 +0700 Subject: [PATCH] handle feedback --- marketplace-build/dev/docker-compose.yml | 1 + marketplace-build/docker-compose.yml | 1 + marketplace-build/release/docker-compose.yml | 1 + .../market/constants/LoggingConstants.java | 22 ++++++++++++++++ .../market/logging/LoggableAspect.java | 22 ++++++++-------- .../com/axonivy/market/util/LoggingUtils.java | 18 +++++++------ .../market/logging/LoggableAspectTest.java | 25 ++++++++++++------- .../axonivy/market/util/FileUtilsTest.java | 11 +++----- .../axonivy/market/util/LoggingUtilsTest.java | 7 ++---- 9 files changed, 67 insertions(+), 41 deletions(-) create mode 100644 marketplace-service/src/main/java/com/axonivy/market/constants/LoggingConstants.java diff --git a/marketplace-build/dev/docker-compose.yml b/marketplace-build/dev/docker-compose.yml index 260fd1f6..760b3564 100644 --- a/marketplace-build/dev/docker-compose.yml +++ b/marketplace-build/dev/docker-compose.yml @@ -36,6 +36,7 @@ services: - MARKET_JWT_SECRET_KEY=${MARKET_JWT_SECRET_KEY} - MARKET_CORS_ALLOWED_ORIGIN=${MARKET_CORS_ALLOWED_ORIGIN} - MARKET_MONGO_LOG_LEVEL=${MARKET_MONGO_LOG_LEVEL} + - MARKET_LOG_PATH=${MARKET_LOG_PATH} build: context: ../../marketplace-service dockerfile: Dockerfile diff --git a/marketplace-build/docker-compose.yml b/marketplace-build/docker-compose.yml index 74011c3a..493a5a50 100644 --- a/marketplace-build/docker-compose.yml +++ b/marketplace-build/docker-compose.yml @@ -36,6 +36,7 @@ services: - MARKET_JWT_SECRET_KEY=${MARKET_JWT_SECRET_KEY} - MARKET_CORS_ALLOWED_ORIGIN=${MARKET_CORS_ALLOWED_ORIGIN} - MARKET_MONGO_LOG_LEVEL=${MARKET_MONGO_LOG_LEVEL} + - MARKET_LOG_PATH=${MARKET_LOG_PATH} build: context: ../marketplace-service dockerfile: Dockerfile diff --git a/marketplace-build/release/docker-compose.yml b/marketplace-build/release/docker-compose.yml index f8fc73f2..fc56c62a 100644 --- a/marketplace-build/release/docker-compose.yml +++ b/marketplace-build/release/docker-compose.yml @@ -34,6 +34,7 @@ services: - MARKET_JWT_SECRET_KEY=${MARKET_JWT_SECRET_KEY} - MARKET_CORS_ALLOWED_ORIGIN=${MARKET_CORS_ALLOWED_ORIGIN} - MARKET_MONGO_LOG_LEVEL=${MARKET_MONGO_LOG_LEVEL} + - MARKET_LOG_PATH=${MARKET_LOG_PATH} networks: - marketplace-network diff --git a/marketplace-service/src/main/java/com/axonivy/market/constants/LoggingConstants.java b/marketplace-service/src/main/java/com/axonivy/market/constants/LoggingConstants.java new file mode 100644 index 00000000..2ce72158 --- /dev/null +++ b/marketplace-service/src/main/java/com/axonivy/market/constants/LoggingConstants.java @@ -0,0 +1,22 @@ +package com.axonivy.market.constants; + +import lombok.AccessLevel; +import lombok.NoArgsConstructor; + +@NoArgsConstructor(access = AccessLevel.PRIVATE) +public class LoggingConstants { + + public static final String ENTRY_FORMAT = " <%s>%s%n"; + public static final String ENTRY_START = " \n"; + public static final String ENTRY_END = " \n"; + public static final String DATE_FORMAT = "yyyy-MM-dd"; + public static final String TIMESTAMP_FORMAT = "yyyy-MM-dd HH:mm:ss"; + public static final String LOG_START = "\n"; + public static final String LOG_END = ""; + public static final String METHOD = "method"; + public static final String ARGUMENTS = "arguments"; + public static final String TIMESTAMP = "timestamp"; + public static final String NO_ARGUMENTS = "No arguments"; + public static final String MARKET_WEBSITE = "marketplace-website"; + +} diff --git a/marketplace-service/src/main/java/com/axonivy/market/logging/LoggableAspect.java b/marketplace-service/src/main/java/com/axonivy/market/logging/LoggableAspect.java index f04fbb8e..059a1998 100644 --- a/marketplace-service/src/main/java/com/axonivy/market/logging/LoggableAspect.java +++ b/marketplace-service/src/main/java/com/axonivy/market/logging/LoggableAspect.java @@ -1,6 +1,7 @@ package com.axonivy.market.logging; import com.axonivy.market.constants.CommonConstants; +import com.axonivy.market.constants.LoggingConstants; import com.axonivy.market.exceptions.model.MissingHeaderException; import jakarta.servlet.http.HttpServletRequest; import lombok.extern.log4j.Log4j2; @@ -18,7 +19,6 @@ import java.nio.file.Files; import java.nio.file.Path; import java.util.Map; -import java.util.Objects; import static com.axonivy.market.util.FileUtils.createFile; import static com.axonivy.market.util.FileUtils.writeToFile; @@ -32,8 +32,6 @@ public class LoggableAspect { @Value("${loggable.log-path}") public String logFilePath; - private static final String REQUESTED_BY = "marketplace-website"; - @Before("@annotation(com.axonivy.market.logging.Loggable)") public void logMethodCall(JoinPoint joinPoint) throws MissingHeaderException { MethodSignature signature = (MethodSignature) joinPoint.getSignature(); @@ -45,7 +43,7 @@ public void logMethodCall(JoinPoint joinPoint) throws MissingHeaderException { saveLogToDailyFile(headersMap); // block execution if request isn't from Market or Ivy Designer - if (!Objects.equals(headersMap.get(CommonConstants.REQUESTED_BY), REQUESTED_BY)) { + if (!LoggingConstants.MARKET_WEBSITE.equals(headersMap.get(CommonConstants.REQUESTED_BY))) { throw new MissingHeaderException(); } } @@ -54,11 +52,11 @@ public void logMethodCall(JoinPoint joinPoint) throws MissingHeaderException { private Map extractHeaders(HttpServletRequest request, MethodSignature signature, JoinPoint joinPoint) { return Map.of( - "method", escapeXml(String.valueOf(signature.getMethod())), - "timestamp", escapeXml(getCurrentTimestamp()), - "user-agent", escapeXml(request.getHeader(CommonConstants.USER_AGENT)), - "arguments", escapeXml(getArgumentsString(signature.getParameterNames(), joinPoint.getArgs())), - "X-Requested-By", escapeXml(request.getHeader(CommonConstants.REQUESTED_BY)) + LoggingConstants.METHOD, escapeXml(String.valueOf(signature.getMethod())), + LoggingConstants.TIMESTAMP, escapeXml(getCurrentTimestamp()), + CommonConstants.USER_AGENT, escapeXml(request.getHeader(CommonConstants.USER_AGENT)), + LoggingConstants.ARGUMENTS, escapeXml(getArgumentsString(signature.getParameterNames(), joinPoint.getArgs())), + CommonConstants.REQUESTED_BY, escapeXml(request.getHeader(CommonConstants.REQUESTED_BY)) ); } @@ -72,14 +70,14 @@ private synchronized void saveLogToDailyFile(Map headersMap) { content.append(new String(Files.readAllBytes(logFile.toPath()))); } if (content.isEmpty()) { - content.append("\n"); + content.append(LoggingConstants.LOG_START); } - int lastLogIndex = content.lastIndexOf(""); + int lastLogIndex = content.lastIndexOf(LoggingConstants.LOG_END); if (lastLogIndex != -1) { content.delete(lastLogIndex, content.length()); } content.append(buildLogEntry(headersMap)); - content.append("\n"); + content.append(LoggingConstants.LOG_END); writeToFile(logFile, content.toString()); } catch (IOException e) { diff --git a/marketplace-service/src/main/java/com/axonivy/market/util/LoggingUtils.java b/marketplace-service/src/main/java/com/axonivy/market/util/LoggingUtils.java index f4bf370d..e69929bc 100644 --- a/marketplace-service/src/main/java/com/axonivy/market/util/LoggingUtils.java +++ b/marketplace-service/src/main/java/com/axonivy/market/util/LoggingUtils.java @@ -1,7 +1,9 @@ package com.axonivy.market.util; +import com.axonivy.market.constants.LoggingConstants; import lombok.AccessLevel; import lombok.NoArgsConstructor; +import org.apache.commons.lang3.StringUtils; import java.text.SimpleDateFormat; import java.util.Map; @@ -13,16 +15,16 @@ public class LoggingUtils { public static String getCurrentDate() { - return new SimpleDateFormat("yyyy-MM-dd").format(System.currentTimeMillis()); + return new SimpleDateFormat(LoggingConstants.DATE_FORMAT).format(System.currentTimeMillis()); } public static String getCurrentTimestamp() { - return new SimpleDateFormat("yyyy-MM-dd HH:mm:ss").format(System.currentTimeMillis()); + return new SimpleDateFormat(LoggingConstants.TIMESTAMP_FORMAT).format(System.currentTimeMillis()); } public static String escapeXml(String value) { - if (value == null) { - return ""; + if (StringUtils.isEmpty(value)) { + return StringUtils.EMPTY; } return value.replace("&", "&") .replace("<", "<") @@ -33,7 +35,7 @@ public static String escapeXml(String value) { public static String getArgumentsString(String[] paramNames, Object[] args) { if (paramNames == null || paramNames.length == 0 || args == null || args.length == 0) { - return "No arguments"; + return LoggingConstants.NO_ARGUMENTS; } return IntStream.range(0, paramNames.length) .mapToObj(i -> paramNames[i] + ": " + args[i]) @@ -43,9 +45,9 @@ public static String getArgumentsString(String[] paramNames, Object[] args) { public static String buildLogEntry(Map headersMap) { StringBuilder logEntry = new StringBuilder(); Map map = new TreeMap<>(headersMap); - logEntry.append(" \n"); - map.forEach((key, value) -> logEntry.append(String.format(" <%s>%s%n", key, value, key))); - logEntry.append(" \n"); + logEntry.append(LoggingConstants.ENTRY_START); + map.forEach((key, value) -> logEntry.append(String.format(LoggingConstants.ENTRY_FORMAT, key, value, key))); + logEntry.append(LoggingConstants.ENTRY_END); return logEntry.toString(); } diff --git a/marketplace-service/src/test/java/com/axonivy/market/logging/LoggableAspectTest.java b/marketplace-service/src/test/java/com/axonivy/market/logging/LoggableAspectTest.java index 583e0810..c88c490c 100644 --- a/marketplace-service/src/test/java/com/axonivy/market/logging/LoggableAspectTest.java +++ b/marketplace-service/src/test/java/com/axonivy/market/logging/LoggableAspectTest.java @@ -1,11 +1,13 @@ package com.axonivy.market.logging; import com.axonivy.market.constants.CommonConstants; +import com.axonivy.market.constants.LoggingConstants; import com.axonivy.market.exceptions.model.MissingHeaderException; import com.axonivy.market.util.LoggingUtils; import jakarta.servlet.http.HttpServletRequest; import org.aspectj.lang.JoinPoint; import org.aspectj.lang.reflect.MethodSignature; +import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.mockito.Mock; @@ -33,21 +35,26 @@ public class LoggableAspectTest { void setUp() throws IOException { MockitoAnnotations.openMocks(this); loggableAspect = new LoggableAspect(); - loggableAspect.logFilePath = Files.createTempDirectory("logs").toString(); // Use temp directory for tests + loggableAspect.logFilePath = Files.createTempDirectory("logs").toString(); + } + + @AfterEach + void tearDown() { + RequestContextHolder.resetRequestAttributes(); } @Test void testLogFileCreation() throws Exception { - mockRequestAttributes("marketplace-website", "test-agent"); + mockRequestAttributes(LoggingConstants.MARKET_WEBSITE, "test-agent"); MethodSignature signature = mockMethodSignature(); loggableAspect.logMethodCall(mockJoinPoint(signature)); - Path logFilePath = Path.of(loggableAspect.logFilePath, "log-" + LoggingUtils.getCurrentDate() + ".xml"); assertTrue(Files.exists(logFilePath), "Log file should be created"); String content = Files.readString(logFilePath); - assertTrue(content.contains(""), "Log file should contain log entries"); + assertTrue(content.contains(LoggingConstants.LOG_START), "Log file should contain log"); + assertTrue(content.contains(LoggingConstants.ENTRY_START), "Log file should contain log entry"); } @Test @@ -55,15 +62,15 @@ void testMissingHeaderException() { mockRequestAttributes("invalid-source", "mock-agent"); MethodSignature signature = mockMethodSignature(); - assertThrows(MissingHeaderException.class, () -> { - loggableAspect.logMethodCall(mockJoinPoint(signature)); - }); + assertThrows(MissingHeaderException.class, () -> + loggableAspect.logMethodCall(mockJoinPoint(signature)) + ); } private JoinPoint mockJoinPoint(MethodSignature signature) { JoinPoint joinPoint = mock(JoinPoint.class); when(joinPoint.getSignature()).thenReturn(signature); - when(joinPoint.getArgs()).thenReturn(new Object[]{"arg1", "arg2"}); // Example arguments + when(joinPoint.getArgs()).thenReturn(new Object[]{"arg1", "arg2"}); return joinPoint; } @@ -75,7 +82,7 @@ private void mockRequestAttributes(String requestedBy, String userAgent) { private MethodSignature mockMethodSignature() { MethodSignature signature = mock(MethodSignature.class); - when(signature.getMethod()).thenReturn(this.getClass().getMethods()[0]); // Use any available method + when(signature.getMethod()).thenReturn(this.getClass().getMethods()[0]); return signature; } diff --git a/marketplace-service/src/test/java/com/axonivy/market/util/FileUtilsTest.java b/marketplace-service/src/test/java/com/axonivy/market/util/FileUtilsTest.java index b7db9681..196152d8 100644 --- a/marketplace-service/src/test/java/com/axonivy/market/util/FileUtilsTest.java +++ b/marketplace-service/src/test/java/com/axonivy/market/util/FileUtilsTest.java @@ -20,21 +20,20 @@ void testCreateFile() throws IOException { File createdFile = FileUtils.createFile(FILE_PATH); assertTrue(createdFile.exists(), "File should exist"); assertTrue(createdFile.isFile(), "Should be a file"); - createdFile.delete(); } @Test - void testFailedToCreateDirectory() throws IOException { + void testFailedToCreateDirectory() { File createdFile = new File("testDirAsFile"); try { if (!createdFile.exists()) { assertTrue(createdFile.createNewFile(), "Setup failed: could not create file"); } - IOException exception = assertThrows(IOException.class, () -> { - FileUtils.createFile("testDirAsFile/subDir/testFile.txt"); - }); + IOException exception = assertThrows(IOException.class, () -> + FileUtils.createFile("testDirAsFile/subDir/testFile.txt") + ); assertTrue(exception.getMessage().contains("Failed to create directory"), "Exception message does not contain expected text"); } catch (IOException e) { @@ -49,10 +48,8 @@ void testWriteFile() throws IOException { File createdFile = FileUtils.createFile(FILE_PATH); String content = "Hello, world!"; FileUtils.writeToFile(createdFile, content); - String fileContent = Files.readString(createdFile.toPath()); assertEquals(content, fileContent, "File content should match the written content"); - createdFile.delete(); } diff --git a/marketplace-service/src/test/java/com/axonivy/market/util/LoggingUtilsTest.java b/marketplace-service/src/test/java/com/axonivy/market/util/LoggingUtilsTest.java index 00d78874..673630ed 100644 --- a/marketplace-service/src/test/java/com/axonivy/market/util/LoggingUtilsTest.java +++ b/marketplace-service/src/test/java/com/axonivy/market/util/LoggingUtilsTest.java @@ -1,5 +1,6 @@ package com.axonivy.market.util; +import com.axonivy.market.constants.LoggingConstants; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; @@ -37,9 +38,8 @@ void testGetArgumentsString() { @Test void testGetArgumentsStringOnNullValue() { - String expectedValue = "No arguments"; String result = LoggingUtils.getArgumentsString(null, null); - Assertions.assertEquals(expectedValue, result); + Assertions.assertEquals(LoggingConstants.NO_ARGUMENTS, result); } @Test @@ -56,7 +56,6 @@ void testBuildLogEntry() { """.indent(2); var result = LoggingUtils.buildLogEntry(given); - Assertions.assertEquals(expected, result); } @@ -64,7 +63,6 @@ void testBuildLogEntry() { void testGetCurrentDate() { String expectedDate = LocalDate.now().toString(); String actualDate = LoggingUtils.getCurrentDate(); - Assertions.assertEquals(expectedDate, actualDate, "The returned date does not match the current date"); } @@ -73,7 +71,6 @@ void testGetCurrentTimestamp() { String expectedTimestamp = LocalDateTime.now() .format(DateTimeFormatter.ofPattern("yyyy-MM-dd HH:mm:ss")); String actualTimestamp = LoggingUtils.getCurrentTimestamp(); - Assertions.assertEquals(expectedTimestamp.substring(0, 19), actualTimestamp.substring(0, 19), "The returned timestamp does not match the expected format or value"); }