-
Notifications
You must be signed in to change notification settings - Fork 0
[Improve] add unit test #12
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
65c0522
dd22619
7133347
723e6ae
a2b866d
84a4bff
00dbcbf
c9ba2e4
9099d75
fdfbcaa
4c9c82f
1dfb83f
3def4ef
827d723
f79f15e
35b6249
4c0fbe0
074b56b
3e3bbf7
88bb8ab
fa8681d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -56,6 +56,7 @@ public ResponseEntity<Message<Page<AlertSilence>>> getAlertSilences( | |
@Parameter(description = "Sort mode: asc: ascending, desc: descending", example = "desc") @RequestParam(defaultValue = "desc") String order, | ||
@Parameter(description = "List current page", example = "0") @RequestParam(defaultValue = "0") int pageIndex, | ||
@Parameter(description = "Number of list pages", example = "8") @RequestParam(defaultValue = "8") int pageSize) { | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
Page<AlertSilence> alertSilencePage = alertSilenceService.getAlertSilences(ids, search, sort, order, pageIndex, pageSize); | ||
return ResponseEntity.ok(Message.success(alertSilencePage)); | ||
} | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -17,16 +17,9 @@ | |||||
|
||||||
package org.apache.hertzbeat.alert.controller; | ||||||
|
||||||
import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.jsonPath; | ||||||
import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; | ||||||
import java.util.ArrayList; | ||||||
import java.util.Collections; | ||||||
import java.util.HashMap; | ||||||
import java.util.LinkedList; | ||||||
import java.util.List; | ||||||
import java.util.Map; | ||||||
import java.util.stream.Collectors; | ||||||
import java.util.stream.Stream; | ||||||
import org.apache.catalina.Manager; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Verification agent 🧩 Analysis chainFix incorrect import and SpringBootTest configuration. The import Also applies to: 48-48 🏁 Script executed: #!/bin/bash
# Description: Find the correct Manager class that should be imported for this test
# Expected: Find the main application class or manager module class
# Search for Manager classes in the codebase
ast-grep --pattern 'class Manager {
$$$
}'
# Also search for @SpringBootApplication annotated classes
ast-grep --pattern '@SpringBootApplication
class $_ {
$$$
}' Length of output: 1875 Correct the Manager import and SpringBootTest configuration The test currently imports
🤖 Prompt for AI Agents
|
||||||
import org.apache.hertzbeat.alert.service.AlertDefineService; | ||||||
import org.apache.hertzbeat.common.constants.CommonConstants; | ||||||
import org.apache.hertzbeat.common.entity.alerter.AlertDefine; | ||||||
|
@@ -36,116 +29,60 @@ | |||||
import org.junit.jupiter.api.extension.ExtendWith; | ||||||
import org.mockito.InjectMocks; | ||||||
import org.mockito.Mock; | ||||||
import org.mockito.Mockito; | ||||||
import org.mockito.junit.jupiter.MockitoExtension; | ||||||
import org.springframework.data.domain.PageImpl; | ||||||
import org.springframework.data.domain.PageRequest; | ||||||
import org.springframework.data.domain.Sort; | ||||||
import org.springframework.boot.test.context.SpringBootTest; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The |
||||||
import org.springframework.http.MediaType; | ||||||
import org.springframework.test.web.servlet.MockMvc; | ||||||
import org.springframework.test.web.servlet.request.MockMvcRequestBuilders; | ||||||
import org.springframework.test.web.servlet.setup.MockMvcBuilders; | ||||||
|
||||||
import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.jsonPath; | ||||||
import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; | ||||||
import static org.springframework.test.web.servlet.setup.MockMvcBuilders.standaloneSetup; | ||||||
|
||||||
/** | ||||||
* Test case for {@link AlertDefinesController} | ||||||
* Test whether the data mocked at the mock is correct, and test whether the format of the returned data is correct | ||||||
*/ | ||||||
|
||||||
@ExtendWith(MockitoExtension.class) | ||||||
@SpringBootTest(classes = Manager.class) | ||||||
class AlertDefinesControllerTest { | ||||||
|
||||||
private MockMvc mockMvc; | ||||||
|
||||||
@InjectMocks | ||||||
private AlertDefinesController alertDefinesController; | ||||||
private MockMvc mockMvc; | ||||||
|
||||||
@Mock | ||||||
AlertDefineService alertDefineService; | ||||||
@InjectMocks | ||||||
private AlertDefinesController alertDefinesController; | ||||||
|
||||||
// Parameters to avoid default values interference, default values have been replaced | ||||||
List<Long> ids = Stream.of(6565463543L, 6565463544L).collect(Collectors.toList()); | ||||||
Byte priority = Byte.parseByte("1"); | ||||||
String sort = "gmtCreate"; | ||||||
String order = "asc"; | ||||||
Integer pageIndex = 1; | ||||||
Integer pageSize = 7; | ||||||
|
||||||
// Parameter collection | ||||||
Map<String, Object> content = new HashMap<>(); | ||||||
@Mock | ||||||
private AlertDefineService alertDefineService; | ||||||
|
||||||
// Object for mock | ||||||
PageRequest pageRequest; | ||||||
private AlertDefine alertDefine; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||||||
|
||||||
// Since the specification is used in dynamic proxy, it cannot be mocked | ||||||
// Missing debugging parameters are ids, priority | ||||||
// The missing part has been manually output for testing | ||||||
@BeforeEach | ||||||
void setUp() { | ||||||
|
||||||
@BeforeEach | ||||||
void setUp() { | ||||||
this.mockMvc = MockMvcBuilders.standaloneSetup(alertDefinesController).build(); | ||||||
content.put("ids", ids); | ||||||
content.put("priority", priority); | ||||||
content.put("sort", sort); | ||||||
content.put("order", order); | ||||||
content.put("pageIndex", pageIndex); | ||||||
content.put("pageSize", pageSize); | ||||||
Sort sortExp = Sort.by(new Sort.Order(Sort.Direction.fromString(content.get("order").toString()), content.get("sort").toString())); | ||||||
pageRequest = PageRequest.of((Integer) content.get("pageIndex"), (Integer) content.get("pageSize"), sortExp); | ||||||
} | ||||||
this.mockMvc = standaloneSetup(alertDefinesController).build(); | ||||||
|
||||||
// @Test | ||||||
// todo: fix this test | ||||||
void getAlertDefines() throws Exception { | ||||||
alertDefine = AlertDefine.builder() | ||||||
.id(9L) | ||||||
.app("linux") | ||||||
.metric("disk") | ||||||
.field("usage") | ||||||
.expr("x") | ||||||
.times(1) | ||||||
.tags(new LinkedList<>()) | ||||||
.build(); | ||||||
Comment on lines
+66
to
+74
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||||||
} | ||||||
|
||||||
// Test the correctness of the mock | ||||||
// Although objects cannot be mocked, stubs can be stored using class files | ||||||
// Mockito.when(alertDefineService.getAlertDefines(Mockito.any(Specification.class), Mockito.argThat(new ArgumentMatcher<PageRequest>() { | ||||||
// @Override | ||||||
// public boolean matches(PageRequest pageRequestMidden) { | ||||||
// // There are three methods in the source code that need to be compared, namely getPageNumber(), getPageSize(), getSort() | ||||||
// if(pageRequestMidden.getPageSize() == pageRequest.getPageSize() && | ||||||
// pageRequestMidden.getPageNumber() == pageRequest.getPageNumber() && | ||||||
// pageRequestMidden.getSort().equals(pageRequest.getSort())) { | ||||||
// return true; | ||||||
// } | ||||||
// return false; | ||||||
// } | ||||||
// }))).thenReturn(new PageImpl<AlertDefine>(new ArrayList<AlertDefine>())); | ||||||
AlertDefine define = AlertDefine.builder().id(9L).app("linux").metric("disk").field("usage").expr("x").times(1).tags(new LinkedList<>()).build(); | ||||||
Mockito.when(alertDefineService.getAlertDefines(null, null, null, "id", "desc", 1, 10)).thenReturn(new PageImpl<>(Collections.singletonList(define))); | ||||||
@Test | ||||||
void deleteAlertDefines() throws Exception { | ||||||
|
||||||
mockMvc.perform(MockMvcRequestBuilders.get( | ||||||
"/api/alert/defines") | ||||||
.param("ids", ids.toString().substring(1, ids.toString().length() - 1)) | ||||||
.param("priority", priority.toString()) | ||||||
.param("sort", sort) | ||||||
.param("order", order) | ||||||
.param("pageIndex", pageIndex.toString()) | ||||||
.param("pageSize", pageSize.toString())) | ||||||
.andExpect(status().isOk()) | ||||||
.andExpect(jsonPath("$.code").value((int) CommonConstants.SUCCESS_CODE)) | ||||||
.andExpect(jsonPath("$.data.content").value(new ArrayList<>())) | ||||||
.andExpect(jsonPath("$.data.pageable").value("INSTANCE")) | ||||||
.andExpect(jsonPath("$.data.totalPages").value(1)) | ||||||
.andExpect(jsonPath("$.data.totalElements").value(0)) | ||||||
.andExpect(jsonPath("$.data.last").value(true)) | ||||||
.andExpect(jsonPath("$.data.number").value(0)) | ||||||
.andExpect(jsonPath("$.data.size").value(0)) | ||||||
.andExpect(jsonPath("$.data.first").value(true)) | ||||||
.andExpect(jsonPath("$.data.numberOfElements").value(0)) | ||||||
.andExpect(jsonPath("$.data.empty").value(true)) | ||||||
.andExpect(jsonPath("$.data.sort.empty").value(true)) | ||||||
.andExpect(jsonPath("$.data.sort.sorted").value(false)) | ||||||
.andExpect(jsonPath("$.data.sort.unsorted").value(true)) | ||||||
.andReturn(); | ||||||
} | ||||||
this.mockMvc.perform(MockMvcRequestBuilders.delete("/api/alert/defines") | ||||||
.contentType(MediaType.APPLICATION_JSON) | ||||||
.content(JsonUtil.toJson(Collections.singletonList(1)))) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The endpoint likely expects a list of
Suggested change
|
||||||
.andExpect(status().isOk()) | ||||||
.andExpect(jsonPath("$.code").value((int) CommonConstants.SUCCESS_CODE)) | ||||||
.andReturn(); | ||||||
} | ||||||
Comment on lines
+77
to
+86
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Enhance test coverage and add service method verification. The current test only verifies HTTP status and response code without testing the actual service interaction. Consider adding:
@Test
void deleteAlertDefines() throws Exception {
+ // Given
+ List<Long> idsToDelete = Collections.singletonList(1L);
+ doNothing().when(alertDefineService).deleteAlertDefines(idsToDelete);
+
+ // When & Then
this.mockMvc.perform(MockMvcRequestBuilders.delete("/api/alert/defines")
.contentType(MediaType.APPLICATION_JSON)
- .content(JsonUtil.toJson(Collections.singletonList(1))))
+ .content(JsonUtil.toJson(idsToDelete)))
.andExpected(status().isOk())
.andExpected(jsonPath("$.code").value((int) CommonConstants.SUCCESS_CODE))
.andReturn();
+
+ // Verify service method was called
+ verify(alertDefineService).deleteAlertDefines(idsToDelete);
} You'll also need to add the missing import: +import static org.mockito.Mockito.doNothing;
+import static org.mockito.Mockito.verify;
🤖 Prompt for AI Agents
|
||||||
|
||||||
@Test | ||||||
void deleteAlertDefines() throws Exception { | ||||||
this.mockMvc.perform(MockMvcRequestBuilders.delete("/api/alert/defines") | ||||||
.contentType(MediaType.APPLICATION_JSON) | ||||||
.content(JsonUtil.toJson(ids))) | ||||||
.andExpect(status().isOk()) | ||||||
.andExpect(jsonPath("$.code").value((int) CommonConstants.SUCCESS_CODE)) | ||||||
.andReturn(); | ||||||
} | ||||||
} |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,81 @@ | ||||||
/* | ||||||
* 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.hertzbeat.alert.controller; | ||||||
|
||||||
import org.apache.hertzbeat.alert.service.AlertSilenceService; | ||||||
import org.apache.hertzbeat.common.constants.CommonConstants; | ||||||
import org.apache.hertzbeat.common.entity.alerter.AlertSilence; | ||||||
import org.junit.jupiter.api.BeforeEach; | ||||||
import org.junit.jupiter.api.Test; | ||||||
import org.junit.jupiter.api.extension.ExtendWith; | ||||||
import org.mockito.InjectMocks; | ||||||
import org.mockito.Mock; | ||||||
import org.mockito.junit.jupiter.MockitoExtension; | ||||||
import org.springframework.http.MediaType; | ||||||
import org.springframework.test.web.servlet.MockMvc; | ||||||
|
||||||
import static org.mockito.ArgumentMatchers.any; | ||||||
import static org.mockito.Mockito.doNothing; | ||||||
import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.delete; | ||||||
import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.jsonPath; | ||||||
import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; | ||||||
import static org.springframework.test.web.servlet.setup.MockMvcBuilders.standaloneSetup; | ||||||
|
||||||
/** | ||||||
* test case for {@link AlertSilencesController} | ||||||
*/ | ||||||
|
||||||
@ExtendWith(MockitoExtension.class) | ||||||
class AlertSilencesControllerTest { | ||||||
|
||||||
private MockMvc mockMvc; | ||||||
|
||||||
@Mock | ||||||
private AlertSilenceService alertSilenceService; | ||||||
|
||||||
private AlertSilence alertSilence; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||||||
|
||||||
@InjectMocks | ||||||
private AlertSilencesController alertSilencesController; | ||||||
|
||||||
@BeforeEach | ||||||
void setUp() { | ||||||
|
||||||
this.mockMvc = standaloneSetup(alertSilencesController).build(); | ||||||
|
||||||
alertSilence = AlertSilence | ||||||
.builder() | ||||||
.id(1L) | ||||||
.type((byte) 1) | ||||||
.name("Test Silence") | ||||||
.build(); | ||||||
Comment on lines
+61
to
+66
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||||||
} | ||||||
|
||||||
@Test | ||||||
void testDeleteAlertDefines() throws Exception { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The test method name
Suggested change
|
||||||
|
||||||
doNothing().when(alertSilenceService).deleteAlertSilences(any()); | ||||||
|
||||||
mockMvc.perform(delete("/api/alert/silences") | ||||||
.param("ids", "1,2,3") | ||||||
.accept(MediaType.APPLICATION_JSON)) | ||||||
.andExpect(status().isOk()) | ||||||
.andExpect(jsonPath("$.code").value((int) CommonConstants.SUCCESS_CODE)); | ||||||
} | ||||||
|
||||||
} |
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,96 @@ | ||||||||||||||||||||||||||||||
/* | ||||||||||||||||||||||||||||||
* 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.hertzbeat.common.config; | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
import java.text.SimpleDateFormat; | ||||||||||||||||||||||||||||||
import java.time.LocalDate; | ||||||||||||||||||||||||||||||
import java.time.LocalDateTime; | ||||||||||||||||||||||||||||||
import java.time.LocalTime; | ||||||||||||||||||||||||||||||
import java.time.format.DateTimeFormatter; | ||||||||||||||||||||||||||||||
import java.util.List; | ||||||||||||||||||||||||||||||
import java.util.TimeZone; | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
import com.fasterxml.jackson.annotation.JsonAutoDetect; | ||||||||||||||||||||||||||||||
import com.fasterxml.jackson.annotation.PropertyAccessor; | ||||||||||||||||||||||||||||||
import com.fasterxml.jackson.databind.ObjectMapper; | ||||||||||||||||||||||||||||||
import com.fasterxml.jackson.databind.SerializationFeature; | ||||||||||||||||||||||||||||||
import com.fasterxml.jackson.datatype.jsr310.JavaTimeModule; | ||||||||||||||||||||||||||||||
import com.fasterxml.jackson.datatype.jsr310.deser.LocalDateDeserializer; | ||||||||||||||||||||||||||||||
import com.fasterxml.jackson.datatype.jsr310.deser.LocalDateTimeDeserializer; | ||||||||||||||||||||||||||||||
import com.fasterxml.jackson.datatype.jsr310.deser.LocalTimeDeserializer; | ||||||||||||||||||||||||||||||
import com.fasterxml.jackson.datatype.jsr310.ser.LocalDateSerializer; | ||||||||||||||||||||||||||||||
import com.fasterxml.jackson.datatype.jsr310.ser.LocalDateTimeSerializer; | ||||||||||||||||||||||||||||||
import com.fasterxml.jackson.datatype.jsr310.ser.LocalTimeSerializer; | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
import org.springframework.beans.factory.annotation.Autowired; | ||||||||||||||||||||||||||||||
import org.springframework.context.annotation.Configuration; | ||||||||||||||||||||||||||||||
import org.springframework.data.web.config.SpringDataJacksonConfiguration; | ||||||||||||||||||||||||||||||
import org.springframework.http.converter.HttpMessageConverter; | ||||||||||||||||||||||||||||||
import org.springframework.http.converter.json.MappingJackson2HttpMessageConverter; | ||||||||||||||||||||||||||||||
import org.springframework.web.servlet.config.annotation.WebMvcConfigurer; | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
/** | ||||||||||||||||||||||||||||||
* MVC Configuration. | ||||||||||||||||||||||||||||||
*/ | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
@Configuration | ||||||||||||||||||||||||||||||
public class MVCConfig implements WebMvcConfigurer { | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
public static final String DEFAULT_DATE_TIME_FORMAT = "yyyy-MM-dd'T'HH:mm:ss"; | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
Comment on lines
+51
to
+55
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing Static DateTimeFormatter ConstantsThe class defines format string constants but doesn't pre-compile the DateTimeFormatter objects. This leads to repeated pattern compilation on every request, which is CPU-intensive. Each DateTimeFormatter creation involves regex pattern compilation and validation, consuming approximately 1-2ms of CPU time per formatter. Under load with thousands of requests per second, this inefficiency can significantly impact performance.
Suggested change
Rationale
References
|
||||||||||||||||||||||||||||||
public static final String DEFAULT_DATE_FORMAT = "yyyy-MM-dd"; | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
public static final String DEFAULT_TIME_FORMAT = "HH:mm:ss"; | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
@Autowired | ||||||||||||||||||||||||||||||
private SpringDataJacksonConfiguration.PageModule pageModule; | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
@Override | ||||||||||||||||||||||||||||||
public void extendMessageConverters(List<HttpMessageConverter<?>> converters) { | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
MappingJackson2HttpMessageConverter messageConverter = new MappingJackson2HttpMessageConverter(); | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
ObjectMapper objectMapper = new ObjectMapper(); | ||||||||||||||||||||||||||||||
objectMapper.setVisibility(PropertyAccessor.ALL, JsonAutoDetect.Visibility.ANY); | ||||||||||||||||||||||||||||||
objectMapper.disable(SerializationFeature.WRITE_DATES_AS_TIMESTAMPS); | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
final SimpleDateFormat simpleDateFormat = new SimpleDateFormat(DEFAULT_DATE_TIME_FORMAT); | ||||||||||||||||||||||||||||||
simpleDateFormat.setTimeZone(TimeZone.getDefault()); | ||||||||||||||||||||||||||||||
Comment on lines
+71
to
+73
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thread Safety Issue in SimpleDateFormat UsageSimpleDateFormat is not thread-safe, and this instance is being used in a shared configuration class. When multiple threads access this formatter simultaneously, it can lead to incorrect date formatting, parsing errors, or even application crashes. This violates the thread safety requirements for shared resources in a multi-threaded web application environment.
Suggested change
Rationale
References
|
||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
JavaTimeModule javaTimeModule = new JavaTimeModule(); | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
DateTimeFormatter defaultDateTimeFormatter = DateTimeFormatter.ofPattern(DEFAULT_DATE_TIME_FORMAT); | ||||||||||||||||||||||||||||||
DateTimeFormatter dateTimeFormatter = DateTimeFormatter.ofPattern(DEFAULT_DATE_FORMAT); | ||||||||||||||||||||||||||||||
DateTimeFormatter timeFormatter = DateTimeFormatter.ofPattern(DEFAULT_TIME_FORMAT); | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
javaTimeModule.addSerializer(LocalDateTime.class, new LocalDateTimeSerializer(defaultDateTimeFormatter)); | ||||||||||||||||||||||||||||||
javaTimeModule.addSerializer(LocalDate.class, new LocalDateSerializer(dateTimeFormatter)); | ||||||||||||||||||||||||||||||
javaTimeModule.addSerializer(LocalTime.class, new LocalTimeSerializer(timeFormatter)); | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
javaTimeModule.addDeserializer(LocalDateTime.class, new LocalDateTimeDeserializer(dateTimeFormatter)); | ||||||||||||||||||||||||||||||
arvi18 marked this conversation as resolved.
Show resolved
Hide resolved
arvi18 marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||
javaTimeModule.addDeserializer(LocalDate.class, new LocalDateDeserializer(dateTimeFormatter)); | ||||||||||||||||||||||||||||||
Comment on lines
+84
to
+86
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Date Format Pattern Mismatch in DeserializersThe deserializer for LocalDateTime is configured to use the date formatter (dateTimeFormatter) instead of the datetime formatter (defaultDateTimeFormatter). This mismatch will cause parsing errors when processing date-time strings, as the date formatter doesn't include time components. This violates ISO/IEC 25010 Functional Correctness requirements for data processing accuracy.
Suggested change
Rationale
References
|
||||||||||||||||||||||||||||||
javaTimeModule.addDeserializer(LocalTime.class, new LocalTimeDeserializer(timeFormatter)); | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
objectMapper.registerModule(javaTimeModule) | ||||||||||||||||||||||||||||||
.registerModule(pageModule) | ||||||||||||||||||||||||||||||
.setDateFormat(simpleDateFormat); | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
messageConverter.setObjectMapper(objectMapper); | ||||||||||||||||||||||||||||||
converters.add(0, messageConverter); | ||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change introduces an unnecessary blank line and an extra space in the
return
statement. Please remove them for better code readability and to maintain a consistent style.