feat: Add JsonUtils and modify related methods #243
Conversation
…ava into feat/develop
WalkthroughThis update centralizes JSON serialization and deserialization by introducing utility classes ( Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller
participant JsonUtils
participant Service
Client->>Controller: HTTP request with JSON body
Controller->>Controller: Read InputStream from request
Controller->>JsonUtils: decode(jsonString, TargetClass)
JsonUtils-->>Controller: TargetClass instance
Controller->>Service: Call service method with parsed object
Service-->>Controller: Result
Controller-->>Client: HTTP response
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
✨ Finishing Touches
🧪 Generate Unit Tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (6)
base/src/test/java/com/tinyengine/it/controller/PageControllerTest.java (1)
84-90: Test correctly updated for new controller signature.The mock HTTP request setup is properly configured with JSON content and correct content type.
Consider removing the unused
Pageobject creation on lines 88-89 since the method now acceptsHttpServletRequest:- Page page = new Page(); - page.setIsPage(true); Result<Page> result = pageController.updatePage(request);base/src/test/java/com/tinyengine/it/controller/BlockControllerTest.java (1)
199-212: Verify JSON content structure and consider more comprehensive test data.The test correctly simulates HTTP request with JSON content, but the JSON payload
{"isDefault":true}seems minimal compared to a typical BlockParam object. Consider using more comprehensive test data that better represents actual usage.- String json = "{\"isDefault\":true}"; + String json = "{\"id\":1,\"name\":\"Updated Block\",\"label\":\"test-block\",\"isDefault\":true,\"appId\":1}";Also consider adding a test case for malformed JSON to verify error handling:
@Test void testUpdateBlocksWithInvalidJson() throws IOException { String invalidJson = "{\"isDefault\":true"; // Missing closing brace MockHttpServletRequest request = new MockHttpServletRequest(); request.setContent(invalidJson.getBytes(StandardCharsets.UTF_8)); request.setContentType("application/json"); // Should handle JsonProcessingException gracefully assertThrows(Exception.class, () -> blockController.updateBlocks(request, Integer.valueOf(0), Integer.valueOf(1))); }base/src/main/java/com/tinyengine/it/common/utils/OffsetDateTimeDeserializer.java (1)
62-67: Simplify the complex DateTimeFormatter construction.The static formatter construction is overly complex and difficult to read due to the deeply nested builder pattern. Consider extracting this to a more readable format or using predefined formatters where possible.
static { - ISO_8601_FORMATTER = (new DateTimeFormatterBuilder().parseCaseInsensitive() - .append(DateTimeFormatter.ISO_LOCAL_DATE).optionalStart() - .appendLiteral('T').optionalEnd().optionalEnd().appendLiteral(' ').optionalEnd() - .append(DateTimeFormatter.ISO_LOCAL_TIME).optionalStart().appendOffsetId().optionalEnd().toFormatter()); + ISO_8601_FORMATTER = new DateTimeFormatterBuilder() + .parseCaseInsensitive() + .append(DateTimeFormatter.ISO_LOCAL_DATE) + .optionalStart() + .appendLiteral('T') + .optionalEnd() + .optionalStart() + .appendLiteral(' ') + .optionalEnd() + .append(DateTimeFormatter.ISO_LOCAL_TIME) + .optionalStart() + .appendOffsetId() + .optionalEnd() + .toFormatter(); }base/src/main/java/com/tinyengine/it/gateway/ai/AiChatClient.java (1)
42-44: Consider using constructor injection instead of setter injection.While the
@Setterannotation facilitates testing, it creates a mutable field that could be changed after construction. Consider using constructor injection with a test-specific constructor or@VisibleForTestingannotation.-// 新增 Setter 方法，便于测试时注入 Mock 对象 -@Setter -private RestTemplate restTemplate; +private final RestTemplate restTemplate; +@VisibleForTesting +AiChatClient(String model, String token, RestTemplate restTemplate) { + this.config = AiChatConfig.getAiChatConfig(model, token); + this.restTemplate = restTemplate; +}base/src/main/java/com/tinyengine/it/common/utils/MapperFactory.java (1)
45-52: Consider potential performance impact of SpringFactoriesLoader.The
SpringFactoriesLoader.loadFactories()call inregisterModules()method might have performance implications if called frequently. Consider caching the loaded modules or calling this method only during application startup.+private static final List<Module> SPRING_MODULES = SpringFactoriesLoader.loadFactories(Module.class, null); public static void registerModules(ObjectMapper mapper) { - List<Module> modules = SpringFactoriesLoader.loadFactories(Module.class, (ClassLoader) null); - mapper.registerModules((Module[]) modules.toArray(new Module[0])); + mapper.registerModules(SPRING_MODULES.toArray(new Module[0])); mapper.registerModule(new SimpleModule()); mapper.registerModule(new JavaTimeModule()); mapper.registerModules(new Module[]{(new SimpleModule()).addDeserializer(OffsetDateTime.class, new OffsetDateTimeDeserializer())}); }base/src/main/java/com/tinyengine/it/common/utils/JsonUtils.java (1)
235-252: Simplify resource management with try-with-resources.The manual resource management can be simplified and made more readable.
Use try-with-resources:
public static <T> T decode(ByteBuf src, Class<T> valueType) { - return (T) invoke(() -> { - InputStream inputStream = new ByteBufInputStream(src); - Object var3; - try { - var3 = MAPPER.readValue(inputStream, valueType); - } catch (Throwable var6) { - try { - inputStream.close(); - } catch (Throwable var5) { - var6.addSuppressed(var5); - } - throw var6; - } - inputStream.close(); - return var3; - }); + return invoke(() -> { + try (InputStream inputStream = new ByteBufInputStream(src)) { + return MAPPER.readValue(inputStream, valueType); + } + }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
app/src/main/java/com/tinyengine/it/config/filter/FilterConfig.java(0 hunks)app/src/main/java/com/tinyengine/it/config/filter/WebConfig.java(2 hunks)base/src/main/java/com/tinyengine/it/common/handler/ListTypeHandler.java(3 hunks)base/src/main/java/com/tinyengine/it/common/handler/MapTypeHandler.java(3 hunks)base/src/main/java/com/tinyengine/it/common/utils/JsonUtils.java(1 hunks)base/src/main/java/com/tinyengine/it/common/utils/MapperFactory.java(1 hunks)base/src/main/java/com/tinyengine/it/common/utils/OffsetDateTimeDeserializer.java(1 hunks)base/src/main/java/com/tinyengine/it/common/utils/SecurityFileCheckUtil.java(1 hunks)base/src/main/java/com/tinyengine/it/common/utils/Utils.java(1 hunks)base/src/main/java/com/tinyengine/it/controller/BlockController.java(5 hunks)base/src/main/java/com/tinyengine/it/controller/PageController.java(5 hunks)base/src/main/java/com/tinyengine/it/controller/PageHistoryController.java(5 hunks)base/src/main/java/com/tinyengine/it/controller/PageTemplateController.java(5 hunks)base/src/main/java/com/tinyengine/it/gateway/ai/AiChatClient.java(4 hunks)base/src/main/java/com/tinyengine/it/service/app/impl/I18nEntryServiceImpl.java(2 hunks)base/src/main/java/com/tinyengine/it/service/app/impl/v1/AppV1ServiceImpl.java(3 hunks)base/src/main/java/com/tinyengine/it/service/material/BlockService.java(1 hunks)base/src/main/java/com/tinyengine/it/service/material/impl/BlockServiceImpl.java(5 hunks)base/src/main/java/com/tinyengine/it/service/material/impl/ComponentServiceImpl.java(1 hunks)base/src/test/java/com/tinyengine/it/common/utils/UtilsTest.java(0 hunks)base/src/test/java/com/tinyengine/it/controller/BlockControllerTest.java(2 hunks)base/src/test/java/com/tinyengine/it/controller/PageControllerTest.java(2 hunks)base/src/test/java/com/tinyengine/it/controller/PageHistoryControllerTest.java(2 hunks)base/src/test/java/com/tinyengine/it/controller/PageTemplateControllerTest.java(2 hunks)base/src/test/java/com/tinyengine/it/gateway/ai/AiChatClientTest.java(2 hunks)base/src/test/java/com/tinyengine/it/service/app/impl/v1/AppV1ServiceImplTest.java(2 hunks)base/src/test/java/com/tinyengine/it/service/material/impl/BlockServiceImplTest.java(1 hunks)pom.xml(4 hunks)
💤 Files with no reviewable changes (2)
- app/src/main/java/com/tinyengine/it/config/filter/FilterConfig.java
- base/src/test/java/com/tinyengine/it/common/utils/UtilsTest.java
🔇 Additional comments (42)
base/src/test/java/com/tinyengine/it/service/material/impl/BlockServiceImplTest.java (2)
141-141: Good API refactoring alignment.The test correctly sets the
appIdon theBlockParamobject to match the updated service method signature.
148-148: Method call correctly updated.The test properly calls
updateBlockByIdwith only theblockParamargument, reflecting the removal of the separateappIdparameter.base/src/test/java/com/tinyengine/it/service/app/impl/v1/AppV1ServiceImplTest.java (1)
21-21: Excellent centralization of JSON processing.The replacement of local
ObjectMapperinstantiation withJsonUtils.MAPPERimproves consistency and reduces duplication across the codebase.Also applies to: 119-119
base/src/test/java/com/tinyengine/it/controller/PageControllerTest.java (1)
30-30: Appropriate imports for HTTP request mocking.The imports correctly support the updated test methodology using
MockHttpServletRequest.Also applies to: 32-32
base/src/main/java/com/tinyengine/it/service/app/impl/I18nEntryServiceImpl.java (1)
24-24: Consistent JsonUtils adoption.The replacement of local
ObjectMapperwithJsonUtils.MAPPERmaintains functionality while following the established centralization pattern for JSON processing.Also applies to: 499-500
base/src/main/java/com/tinyengine/it/common/utils/SecurityFileCheckUtil.java (1)
194-194: Proper JsonUtils integration for file validation.The use of
JsonUtils.MAPPERfor JSON validation maintains the same functionality while ensuring consistent JSON processing across the application.base/src/main/java/com/tinyengine/it/service/material/impl/ComponentServiceImpl.java (3)
150-151: ```shell
#!/bin/bashExtract full ComponentLibraryByCondition SQL block to verify all filtering fields
sed -n '//,/</sql>/p' base/src/main/resources/mappers/ComponentLibraryMapper.xml
--- `150-151`: **Verify the intentional removal of version handling.** The version setting for component libraries has been commented out, which removes version-based differentiation and enforces a default overwrite update behavior. This is a significant business logic change that could affect how component libraries are managed. Please confirm this change is intentional and ensure that: 1. All dependent systems can handle the absence of version differentiation 2. The overwrite behavior aligns with business requirements 3. Consider documenting this behavior change in the API documentation ```shell #!/bin/bash # Description: Search for other usages of ComponentLibrary version handling to assess impact # Search for version-related code in ComponentLibrary usage rg -A 3 -B 3 "ComponentLibrary.*[Vv]ersion|[Vv]ersion.*ComponentLibrary" # Search for any version comparison logic rg -A 5 "componentLibrary\.getVersion|library\.setVersion"
150-151: Verify the impact of disabling version-based component library queries.The commented code disables version-based differentiation for component libraries, which means the system will now find existing libraries by name only, not by name+version. This could lead to unintended overwrites of different versions of the same library.
Please verify that this change aligns with the intended business logic and doesn't break existing functionality:
#!/bin/bash # Search for other usages of ComponentLibrary version field to understand impact ast-grep --pattern 'componentLibrary.setVersion($_)' # Check if version field is used elsewhere in queries rg -A 3 -B 3 "\.setVersion\(" --type java # Look for any tests that might be affected by this change fd -e java -x grep -l "ComponentLibrary.*version" {} \;base/src/main/java/com/tinyengine/it/service/material/BlockService.java (3)
68-72: I want to confirm that theBlockParamclass actually defines anappIdfield (and accessor) so callers can set it. Please run:#!/bin/bash # Locate the BlockParam source file block_param_file=$(fd BlockParam.java -t f) echo "Found BlockParam at: $block_param_file" # Search for appId declaration and accessors rg -n "appId" "$block_param_file"
68-72: Method signature simplified - verify consistency across implementations.The
updateBlockByIdmethod signature has been simplified to remove theappIdparameter, which is now likely obtained from theBlockParamobject internally.Please ensure all implementations and callers have been updated consistently:
#!/bin/bash # Description: Verify that all implementations and callers of updateBlockById have been updated # Find all implementations of updateBlockById ast-grep --pattern 'updateBlockById($$$) { $$$ }' # Find all calls to updateBlockById to check if they match the new signature rg -A 2 -B 2 "updateBlockById\s*\("
68-72: Method signature simplification looks good.The removal of the
appIdparameter and encapsulation withinBlockParamimproves API cohesion and reduces parameter count. This is a clean refactoring.Verify that all callers of this method have been updated to match the new signature:
#!/bin/bash # Search for calls to updateBlockById to ensure they've been updated rg -A 3 -B 3 "updateBlockById\(" --type java # Check for any remaining two-parameter calls that might have been missed rg "updateBlockById\s*\([^,)]+,\s*[^)]+\)" --type javabase/src/main/java/com/tinyengine/it/common/handler/ListTypeHandler.java (6)
18-18: Good: Centralized JSON processing with JsonUtils.Adding the JsonUtils import to replace local ObjectMapper instantiation aligns with the codebase-wide effort to standardize JSON handling.
46-46: LGTM: Consistent use of centralized JsonUtils.MAPPER.Replacing local ObjectMapper instances with the shared
JsonUtils.MAPPERimproves consistency and reduces redundant object creation while maintaining the same functionality.Also applies to: 91-91, 94-94
18-18: LGTM! Successful centralization of JSON processing.The migration from local
ObjectMapperinstances to the centralizedJsonUtils.MAPPERimproves consistency and maintainability across the codebase. The error handling and logic remain unchanged, which is good.Also applies to: 46-46, 91-91, 94-94
18-18: Good centralization of JSON processing.Adding the JsonUtils import aligns with the project's move to centralize JSON handling through a shared utility class.
46-46: Consistent migration to centralized ObjectMapper.Using
JsonUtils.MAPPERinstead of a local ObjectMapper instance improves consistency and maintainability across the codebase.
91-94: JSON deserialization properly migrated to JsonUtils.The replacement of local ObjectMapper calls with
JsonUtils.MAPPERmaintains the same functionality while using the centralized JSON utility.base/src/main/java/com/tinyengine/it/common/handler/MapTypeHandler.java (5)
19-19: Good: Centralized JSON processing with JsonUtils.Adding the JsonUtils import supports the consistent use of shared ObjectMapper across the codebase.
44-44: LGTM: Consistent use of centralized JsonUtils.MAPPER.The replacement of local ObjectMapper instances with
JsonUtils.MAPPERstandardizes JSON processing and maintains the same functionality while improving code consistency.Also applies to: 72-72, 74-74
19-19: LGTM! Consistent with JSON processing centralization.The migration to
JsonUtils.MAPPERmaintains the same functionality while contributing to the codebase-wide effort to centralize JSON processing. This improves maintainability and consistency.Also applies to: 44-44, 72-72, 74-74
19-19: Consistent import addition for JsonUtils.Adding JsonUtils import follows the established pattern for centralizing JSON processing.
44-44: Proper migration to centralized JSON utilities.The replacement of local ObjectMapper instances with
JsonUtils.MAPPERmaintains existing functionality while improving code consistency. The error handling and logic flow remain unchanged.Also applies to: 72-72, 74-74
base/src/main/java/com/tinyengine/it/service/app/impl/v1/AppV1ServiceImpl.java (6)
15-15: Good: Enhanced type safety with JsonUtils.convertValue.The static import of
JsonUtils.convertValueandTypeReferenceimproves type safety over the genericUtils.convertmethod.Also applies to: 18-18
240-240: LGTM: Improved type safety with JsonUtils.convertValue.Replacing
Utils.convertwithJsonUtils.convertValueand explicitTypeReferenceprovides better type safety and aligns with the centralized JSON processing approach.Also applies to: 469-469
15-15: Good improvement with type-safe JSON conversion imports.The addition of
JsonUtils.convertValuestatic import andTypeReferenceimport enables more type-safe and specific JSON conversions compared to the genericUtils.convertmethod.Also applies to: 18-18
240-240: LGTM! Enhanced type safety with JsonUtils.convertValue.The migration from
Utils.converttoJsonUtils.convertValuewithTypeReferenceprovides better type safety and aligns with the codebase-wide effort to centralize JSON processing. The explicitTypeReference<Map<String, Object>>()ensures compile-time type checking.Also applies to: 469-469
15-15: Good migration to specific JSON conversion utilities.The static import of
JsonUtils.convertValueand addition ofTypeReferenceimport support the move from generic utility methods to specific JSON conversion functions, making the code intent clearer.Also applies to: 18-18
240-240: Proper replacement of generic conversion with specific JSON conversion.Replacing
Utils.convertwithJsonUtils.convertValueimproves type safety and makes the JSON conversion intent explicit. The usage ofTypeReferenceensures proper type conversion.Verify that all instances of
Utils.converthave been replaced throughout the codebase:#!/bin/bash # Search for any remaining Utils.convert calls that might have been missed rg "Utils\.convert\(" --type java # Check if the Utils.convert method still exists and might be unused now ast-grep --pattern 'class Utils { $$$ convert($$$) { $$$ } $$$ }'Also applies to: 469-469
pom.xml (2)
33-33: LGTM: Proper migration from WebFlux to Web MVC.The dependency changes correctly reflect the architectural shift from reactive Spring WebFlux to servlet-based Spring Web MVC.
Also applies to: 106-107
36-36: Verify the necessity of netty-buffer dependency.The addition of
netty-bufferdependency seems inconsistent with the migration away from WebFlux (which typically uses Netty). Please verify if this dependency is still needed in the Web MVC context.#!/bin/bash # Description: Search for netty-buffer usage in the codebase to verify necessity # Expected: Find usage patterns that justify keeping this dependency rg -A 3 -B 3 "netty.*buffer|Buffer.*netty" --type javaAlso applies to: 133-137
base/src/test/java/com/tinyengine/it/controller/PageTemplateControllerTest.java (1)
52-61: LGTM: Proper test adaptation for manual JSON parsing.The test correctly simulates the new HTTP request-based approach by creating a
MockHttpServletRequestwith JSON content, which aligns with the controller method changes.base/src/test/java/com/tinyengine/it/controller/PageHistoryControllerTest.java (1)
72-85: LGTM: Consistent test adaptation with comprehensive JSON content.The test properly simulates the HTTP request approach and includes comprehensive JSON content appropriate for
PageHistory. The service mock return value change from0to1appears intentional and consistent with the test expectations.base/src/main/java/com/tinyengine/it/common/utils/Utils.java (1)
371-371: LGTM: Proper centralization of JSON processing.Using
JsonUtils.MAPPERinstead of localObjectMapperinstances promotes consistency and reduces code duplication across the codebase.base/src/test/java/com/tinyengine/it/gateway/ai/AiChatClientTest.java (1)
58-98: LGTM: Well-structured test refactor from reactive to synchronous approach.The test has been properly updated to match the migration from WebClient to RestTemplate. The mock setup correctly uses argument matchers and the test logic is clear and comprehensive.
base/src/main/java/com/tinyengine/it/controller/PageTemplateController.java (1)
77-82: Let’s dump the fullinvokemethod to see how it handles exceptions:#!/bin/bash set -e file="base/src/main/java/com/tinyengine/it/common/utils/JsonUtils.java" # Find the line number of the invoke declaration start=$(rg -n "private static <T> T invoke" "$file" | head -n1 | cut -d: -f1) if [ -z "$start" ]; then echo "invoke declaration not found" exit 1 fi # Print the method body (next 50 lines should cover the entire method) sed -n "${start},$((start+50))p" "$file"base/src/main/java/com/tinyengine/it/service/material/impl/BlockServiceImpl.java (3)
27-27: LGTM: Centralized JSON processing with JsonUtils.The replacement of local ObjectMapper instances with
JsonUtils.MAPPERis a good practice that centralizes JSON configuration and processing across the application.
281-367: LGTM: Consistent use of centralized JSON utilities.All JSON processing operations have been properly updated to use
JsonUtils.MAPPERinstead of local instances. This improves consistency and maintainability.
563-565: Verify appId setting before updateBlockById call.The code correctly sets
appIdon theBlockParambefore calling the updatedupdateBlockByIdmethod. This maintains the validation logic while simplifying the method signature.base/src/main/java/com/tinyengine/it/controller/BlockController.java (1)
18-18: ```shell
#!/bin/bashShow imports and usage in AiChatController
sed -n '1,200p' base/src/main/java/com/tinyengine/it/controller/AiChatController.java
Search for JsonUtils usage across the repository
rg -n "JsonUtils" --type java
</details> <details> <summary>app/src/main/java/com/tinyengine/it/config/filter/WebConfig.java (2)</summary> `26-26`: **LGTM! Clean migration from WebFlux to WebMVC.** The change from `WebFluxConfigurer` to `WebMvcConfigurer` is consistent with the overall framework migration in this PR. --- `31-47`: **CORS configuration correctly migrated to servlet-based implementation.** The migration from `CorsWebFilter` to `CorsFilter` maintains the same CORS policy while switching to the servlet-based implementation. The configuration settings remain identical, ensuring no behavioral changes. </details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
base/src/main/java/com/tinyengine/it/controller/PageController.java
Outdated
Show resolved
Hide resolved
base/src/main/java/com/tinyengine/it/controller/PageHistoryController.java
Outdated
Show resolved
Hide resolved
base/src/main/java/com/tinyengine/it/service/material/impl/BlockServiceImpl.java
Show resolved
Hide resolved
base/src/main/java/com/tinyengine/it/common/utils/OffsetDateTimeDeserializer.java
Show resolved
Hide resolved
base/src/main/java/com/tinyengine/it/controller/BlockController.java
Outdated
Show resolved
Hide resolved
base/src/main/java/com/tinyengine/it/common/utils/JsonUtils.java
Outdated
Show resolved
Hide resolved
base/src/main/java/com/tinyengine/it/common/utils/JsonUtils.java
Outdated
Show resolved
Hide resolved
English | 简体中文
PR
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
Background and solution
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Chores
Tests