base/src/main/java/com/tinyengine/it/controller/BlockController.java (2)

Line range hint 154-171: Add input validation for BlockParam fields.

While the method signature has been updated correctly, consider adding validation annotations on the BlockParam fields to ensure data integrity.

---

Line range hint 382-403: Consider adding transaction boundary.

The update operation involves multiple database operations (updating block and group associations). Consider adding transaction boundaries to ensure data consistency.

+    @Transactional
     public Result<BlockDto> updateBlocks(@Valid @RequestBody BlockParam blockParam, @PathVariable Integer id,
                                          @RequestParam(value = "appId", required = false) Integer appId) {
         blockParam.setId(id);
         return blockService.updateBlockById(blockParam, appId);
     }

base/src/main/java/com/tinyengine/it/service/material/impl/BlockServiceImpl.java (7)

Line range hint 154-204: Add null checks and validation.

The method should validate the BlockParam object before processing to prevent NullPointerException.

     public Result<BlockDto> updateBlockById(BlockParam blockParam, Integer appId) {
+        if (blockParam == null || blockParam.getId() == null) {
+            return Result.failed(ExceptionEnum.CM001);
+        }
         Block blockResult = blockMapper.queryBlockById(blockParam.getId());

---

Line range hint 210-239: Add error handling for group creation failure.

The method should handle potential failures when creating block group associations.

         if (groups != null && !groups.isEmpty()) {
             Integer groupId = groups.get(0);
             BlockGroupBlock blockGroupBlock = new BlockGroupBlock();
             blockGroupBlock.setBlockGroupId(groupId);
             blockGroupBlock.setBlockId(id);
-            blockGroupBlockMapper.createBlockGroupBlock(blockGroupBlock);
+            int groupResult = blockGroupBlockMapper.createBlockGroupBlock(blockGroupBlock);
+            if (groupResult < 1) {
+                return Result.failed(ExceptionEnum.CM001);
+            }
         }

---

Line range hint 154-203: Transaction management missing.

The method performs multiple database operations without transaction management.

Add transaction management to ensure data consistency:

+    @Transactional
     @Override
     public Result<BlockDto> updateBlockById(BlockParam blockParam, Integer appId) {
         Block blockResult = blockMapper.queryBlockById(blockParam.getId());

Also, the method has potential null pointer issues:

Add null checks:

     public Result<BlockDto> updateBlockById(BlockParam blockParam, Integer appId) {
+        if (blockParam == null || blockParam.getId() == null) {
+            return Result.failed(ExceptionEnum.CM001);
+        }
         Block blockResult = blockMapper.queryBlockById(blockParam.getId());
+        if (blockResult == null) {
+            return Result.failed(ExceptionEnum.CM001);
+        }

---

Line range hint 210-239: Hardcoded values and missing validation.

The method contains hardcoded values and lacks input validation.

Add validation and extract constants:

+    private static final int DEFAULT_PLATFORM_ID = 1;
+    private static final String DEFAULT_USER_ID = "1";
+
     @Override
+    @Transactional
     public Result<BlockDto> createBlock(BlockParam blockParam) {
+        if (blockParam == null) {
+            return Result.failed(ExceptionEnum.CM001);
+        }
         Block blocks = new Block();
         if (blockParam.getOccupier() != null) {
             blocks.setOccupierBy(String.valueOf(blockParam.getOccupier().getId()));
         }
         BeanUtils.copyProperties(blockParam, blocks);
         blocks.setIsDefault(false);
         blocks.setIsOfficial(false);
-        blocks.setPlatformId(1); // 新建区块给默认值
+        blocks.setPlatformId(DEFAULT_PLATFORM_ID);

---

Line range hint 505-543: Error handling needs improvement.

The deploy method lacks proper error handling for edge cases.

Add error handling:

     public Result<BlockDto> deploy(BlockBuildDto blockBuildDto) {
+        if (blockBuildDto == null || blockBuildDto.getBlock() == null) {
+            return Result.failed(ExceptionEnum.CM001);
+        }
         Map<String, Object> content = blockBuildDto.getBlock().getContent();
         if (content.isEmpty()) {
             return Result.failed(ExceptionEnum.CM204);
         }
+        try {
             BlockParam blockParam = blockBuildDto.getBlock();
             List<I18nEntryDto> i18nList = i18nEntryMapper.findI18nEntriesByHostandHostType(id, "block");
             // ... rest of the method
             return updateBlockById(blockParam, blockParam.getAppId());
+        } catch (Exception e) {
+            log.error("Failed to deploy block: {}", e.getMessage(), e);
+            return Result.failed(ExceptionEnum.CM008);
+        }
     }

---

Line range hint 154-204: Implement transaction management for block updates.

The method performs multiple database operations:

1. Updating block data
2. Managing block group associations
   These operations should be wrapped in a transaction to maintain data consistency.

---

Line range hint 210-239: Add null checks and validation.

The method should validate:

1. Required fields in BlockParam
2. Business rules for block creation
3. Proper error handling for database operations

.github/workflows/checkstyle.yml (1)

28-30: Optimize the Maven command for Checkstyle.

The current mvn clean install command runs the full build cycle including tests, which is more than needed for style checking. Consider using a more focused command.

-          mvn clean install
+          mvn checkstyle:check

base/src/main/java/com/tinyengine/it/model/dto/BlockParam.java (2)

152-155: Consider adding validation for publicStatus values.

The getPublic() method returns publicStatus directly. Since the field is documented to accept values 0,1,2, consider adding validation.

@JsonProperty("public")
public Integer getPublic() {
+    if (this.publicStatus != null && !Arrays.asList(0, 1, 2).contains(this.publicStatus)) {
+        throw new IllegalStateException("Invalid public status value: " + this.publicStatus);
+    }
    return this.publicStatus;
}

---

1-157: Document the relationship between BlockDto and BlockParam.

Since this is a new class that seems to be replacing BlockDto in some contexts, consider adding documentation to explain:

• The purpose of having both DTOs
• When to use each one
• How they relate to each other

Add the following to the class-level documentation:

/**
 * <p>
 * 区块param
 * </p>
 *
+ * This class serves as a parameter object for block creation and updates,
+ * working in conjunction with {@link BlockDto}. While BlockDto represents
+ * the full block data model, BlockParam is specifically designed for
+ * handling input parameters during block operations.
 *
 * @author lu-yg
 * @since 2024-01-27
 */

base/src/test/java/com/tinyengine/it/controller/BlockControllerTest.java (4)

184-191: Consider adding more test scenarios.

While the basic test case is correct, consider adding test scenarios for:

• Invalid block parameters
• Missing required fields
• Edge cases with different group configurations

---

101-106: Test needs additional assertions.

The test only verifies the returned data but doesn't validate if the BlockParam properties were correctly passed to the service layer.

Add assertions to verify the BlockParam properties:

 @Test
 void testCreateBlocks() {
     BlockParam mockParam = new BlockParam();
+    mockParam.setName("Test Block");
+    mockParam.setLabel("test-block");
     BlockDto mockData = new BlockDto();
+    mockData.setName("Test Block");
+    mockData.setLabel("test-block");
     when(blockService.createBlock(any(BlockParam.class))).thenReturn(Result.success(mockData));

     Result<BlockDto> result = blockController.createBlocks(mockParam);
     Assertions.assertEquals(mockData, result.getData());
+    verify(blockService).createBlock(argThat(param -> 
+        param.getName().equals("Test Block") && 
+        param.getLabel().equals("test-block")
+    ));
 }

---

184-191: Test needs additional assertions and error case coverage.

The test only covers the happy path but doesn't verify error scenarios or parameter validation.

Add test cases for error scenarios:

 @Test
 void testUpdateBlocks() {
     BlockParam blockParam = new BlockParam();
+    blockParam.setName("Updated Block");
     BlockDto returnData = new BlockDto();
+    returnData.setName("Updated Block");
     when(blockService.updateBlockById(any(BlockParam.class), anyInt())).thenReturn(Result.success(returnData));
     when(blockService.queryBlockById(anyInt())).thenReturn(returnData);

     Result<BlockDto> result = blockController.updateBlocks(blockParam, Integer.valueOf(0), Integer.valueOf(1));
     Assertions.assertEquals(returnData, result.getData());
+    verify(blockService).updateBlockById(argThat(param -> 
+        param.getName().equals("Updated Block") &&
+        param.getId().equals(0)
+    ), eq(1));
 }

+@Test
+void testUpdateBlocksWithInvalidId() {
+    BlockParam blockParam = new BlockParam();
+    when(blockService.updateBlockById(any(BlockParam.class), anyInt()))
+        .thenReturn(Result.failed(ExceptionEnum.CM007));
+
+    Result<BlockDto> result = blockController.updateBlocks(blockParam, -1, 1);
+    Assertions.assertTrue(result.isFailed());
+    Assertions.assertEquals(ExceptionEnum.CM007.getCode(), result.getCode());
+}

---

184-191: Verify test coverage for error scenarios.

While the happy path is tested, consider adding test cases for:

• Invalid block parameters
• Non-existent block ID
• Permission denied scenarios

base/src/main/java/com/tinyengine/it/service/material/impl/BlockServiceImpl.java (4)

Line range hint 505-543: Consider extracting build info creation to a separate method.

The build info creation logic can be extracted to improve readability and reusability.

+    private Map<String, Object> createBuildInfo(String version, LocalDateTime buildTime) {
+        Map<String, Object> buildInfo = new HashMap<>();
+        buildInfo.put("result", true);
+        buildInfo.put("versions", Arrays.asList(version));
+        DateTimeFormatter formatter = DateTimeFormatter.ofPattern("yyyy-MM-dd HH:mm:ss");
+        buildInfo.put("endTime", buildTime.format(formatter));
+        return buildInfo;
+    }

---

669-685: Add logging for block creation.

Consider adding debug logging to track block creation and updates.

     public int ensureBlockId(BlockParam blockParam) {
+        log.debug("Ensuring block ID for label: {}", blockParam.getLabel());
         if (blockParam.getId() != null) {
+            log.debug("Block ID already exists: {}", blockParam.getId());
             return blockParam.getId();
         }

---

Line range hint 505-543: Consider extracting deployment logic.

The deploy method handles multiple responsibilities:

1. Block creation/update
2. History tracking
3. I18n handling
4. Build info management

Consider extracting these into separate methods for better maintainability.

---

669-685: Add logging for block creation events.

Consider adding debug logs to track:

1. Block creation attempts
2. Existing block lookups
3. Creation success/failure

base/src/main/java/com/tinyengine/it/controller/BlockController.java (2)

Line range hint 154-171: API documentation needs improvement.

The method documentation lacks details about the BlockParam structure and validation requirements.

Add detailed documentation:

     /**
      * 创建block
      *
-     * @param blockParam the blockParam
+     * @param blockParam The block parameters containing:
+     *                  - name: Block name (required)
+     *                  - label: Unique identifier (required)
+     *                  - framework: Framework type (required)
+     *                  - groups: List of group IDs (optional)
+     *                  - content: Block content (optional)
+     *                  - screenshot: Block screenshot (optional)
      * @return BlockDto with created block details
+     * @throws ValidationException if required fields are missing
      */

---

Line range hint 154-171: Update API documentation to reflect parameter changes.

The Swagger documentation should include:

1. Detailed description of required fields in BlockParam
2. Example request payload
3. Possible error responses

base/src/test/java/com/tinyengine/it/service/material/impl/BlockServiceImplTest.java (2)

118-121: Add validation for required fields.

The test initializes an empty BlockParam object. Consider adding test cases that validate the required fields and their constraints.

---

126-134: Enhance test coverage for block update scenarios.

The test should verify:

1. Block group associations are correctly updated
2. History tracking is maintained
3. Proper error handling when block doesn't exist

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

base/src/test/java/com/tinyengine/it/controller/BlockControllerTest.java (2)

104-116: 🛠️ Refactor suggestion

Add more test cases for block creation.

The test only verifies basic properties. Consider adding test cases for validation and error scenarios.

Enhance the test coverage:

 @Test
 void testCreateBlocks() {
     BlockParam mockParam = new BlockParam();
     mockParam.setName("Test Block");
     mockParam.setLabel("test-block");
+    mockParam.setFramework("vue");
+    mockParam.setDescription("Test Description");
+    mockParam.setIsOfficial(false);
+    mockParam.setIsDefault(false);
+    mockParam.setGroups(Arrays.asList(1, 2));
+
     BlockDto mockData = new BlockDto();
     mockData.setName("Test Block");
     mockData.setLabel("test-block");
+    mockData.setFramework("vue");
+    mockData.setDescription("Test Description");
     when(blockService.createBlock(any(BlockParam.class))).thenReturn(Result.success(mockData));

     Result<BlockDto> result = blockController.createBlocks(mockParam);
     Assertions.assertEquals(mockData, result.getData());
     verify(blockService).createBlock(argThat(param ->
             param.getName().equals("Test Block") && 
             param.getLabel().equals("test-block") &&
+            param.getFramework().equals("vue") &&
+            param.getDescription().equals("Test Description") &&
+            !param.getIsOfficial() &&
+            !param.getIsDefault() &&
+            param.getGroups().containsAll(Arrays.asList(1, 2))
     ));
+    
+    // Test validation failure
+    BlockParam invalidParam = new BlockParam();
+    when(blockService.createBlock(invalidParam)).thenReturn(Result.failed("Validation failed"));
+    Result<BlockDto> invalidResult = blockController.createBlocks(invalidParam);
+    Assertions.assertFalse(invalidResult.isSuccess());
 }

---

195-207: 🛠️ Refactor suggestion

Add test cases for block update validation.

The test should verify error handling and validation during block updates.

Enhance the test coverage:

 @Test
 void testUpdateBlocks() {
     BlockParam blockParam = new BlockParam();
     blockParam.setName("Updated Block");
+    blockParam.setId(0);
+    blockParam.setLabel("updated-block");
+    blockParam.setFramework("vue");
+    blockParam.setGroups(Arrays.asList(1, 2));
+
     BlockDto returnData = new BlockDto();
     returnData.setName("Updated Block");
     when(blockService.updateBlockById(any(BlockParam.class), anyInt())).thenReturn(Result.success(returnData));
     when(blockService.queryBlockById(anyInt())).thenReturn(returnData);

     Result<BlockDto> result = blockController.updateBlocks(blockParam, Integer.valueOf(0), Integer.valueOf(1));
     Assertions.assertEquals(returnData, result.getData());
     verify(blockService).updateBlockById(argThat(param ->
             param.getName().equals("Updated Block") &&
-                    param.getId().equals(0)
+                    param.getId().equals(0) &&
+                    param.getLabel().equals("updated-block") &&
+                    param.getFramework().equals("vue") &&
+                    param.getGroups().containsAll(Arrays.asList(1, 2))
     ), eq(1));
+    
+    // Test null ID validation
+    BlockParam invalidParam = new BlockParam();
+    when(blockService.updateBlockById(invalidParam, 1)).thenReturn(Result.failed("Invalid ID"));
+    Result<BlockDto> invalidResult = blockController.updateBlocks(invalidParam, null, 1);
+    Assertions.assertFalse(invalidResult.isSuccess());
 }

base/src/test/java/com/tinyengine/it/service/material/impl/BlockServiceImplTest.java (1)

118-129: 🛠️ Refactor suggestion

Add test cases for block creation validation.

The test should verify validation of required fields and error scenarios.

Enhance the test coverage:

 @Test
 void testCreateBlock() {
     when(blockMapper.createBlock(any())).thenReturn(1);
     BlockDto t = new BlockDto();
     t.setName("test");
     when(blockMapper.findBlockAndGroupAndHistoByBlockId(any())).thenReturn(t);
     BlockParam blockParam = new BlockParam();
     blockParam.setId(0);
     blockParam.setName("test");
     blockParam.setLabel("test-label");
     blockParam.setFramework("vue");
+    blockParam.setDescription("Test Description");
+    blockParam.setIsOfficial(false);
+    blockParam.setIsDefault(false);
+    blockParam.setGroups(Arrays.asList(1, 2));
+
     Result<BlockDto> result = blockServiceImpl.createBlock(blockParam);
     Assertions.assertEquals("test", result.getData().getName());
+    verify(blockMapper).createBlock(argThat(block ->
+        block.getName().equals("test") &&
+        block.getLabel().equals("test-label") &&
+        block.getFramework().equals("vue") &&
+        block.getDescription().equals("Test Description") &&
+        !block.getIsOfficial() &&
+        !block.getIsDefault()
+    ));
     
     // Test validation failure
     BlockParam invalidParam = new BlockParam();
     Result<BlockDto> invalidResult = blockServiceImpl.createBlock(invalidParam);
     Assertions.assertFalse(invalidResult.isSuccess());
+    
+    // Test duplicate label validation
+    when(blockMapper.queryBlockByCondition(any())).thenReturn(Arrays.asList(new Block()));
+    Result<BlockDto> duplicateResult = blockServiceImpl.createBlock(blockParam);
+    Assertions.assertFalse(duplicateResult.isSuccess());
 }

base/src/main/java/com/tinyengine/it/model/dto/BlockParam.java (1)

157-162: Enhance error handling in getPublic() method.

The method could be improved with more descriptive error messages and null handling.

Consider this enhancement:

 @JsonProperty("public")
 public Integer getPublic() {
+    if (this.publicStatus == null) {
+        return null;
+    }
     if (this.publicStatus != null && !Arrays.asList(0, 1, 2).contains(this.publicStatus)) {
-        throw new IllegalStateException("Invalid public status value: " + this.publicStatus);
+        throw new IllegalStateException(
+            String.format("Invalid public status value: %d. Expected values are: 0 (private), 1 (public), 2 (tenant)", 
+            this.publicStatus)
+        );
     }
     return this.publicStatus;
 }

base/src/main/java/com/tinyengine/it/service/material/impl/BlockServiceImpl.java (3)

96-97: Consider making constants configurable.

Hard-coded values should be moved to configuration.

Move these constants to configuration:

-    private static final int DEFAULT_PLATFORM_ID = 1;
-    private static final String DEFAULT_USER_ID = "1";
+    @Value("${block.default.platform-id}")
+    private int defaultPlatformId;
+    @Value("${block.default.user-id}")
+    private String defaultUserId;

---

158-165: Add logging for error scenarios.

The error handling could be improved with proper logging.

Add logging statements:

 public Result<BlockDto> updateBlockById(BlockParam blockParam, Integer appId) {
     if (blockParam == null || blockParam.getId() == null) {
+        log.error("Invalid block parameter: blockParam={}, appId={}", blockParam, appId);
         return Result.failed(ExceptionEnum.CM002);
     }
     Block blockResult = blockMapper.queryBlockById(blockParam.getId());
     if (blockResult == null) {
+        log.error("Block not found: id={}", blockParam.getId());
         return Result.failed(ExceptionEnum.CM001);
     }

---

725-732: Add input validation to createBuildInfo method.

The method should validate its input parameters.

Add validation:

 private Map<String, Object> createBuildInfo(String version, LocalDateTime buildTime) {
+    if (version == null || buildTime == null) {
+        throw new IllegalArgumentException("Version and buildTime cannot be null");
+    }
+    if (!version.matches("^\\d+\\.\\d+\\.\\d+(-[a-zA-Z0-9]+)?$")) {
+        throw new IllegalArgumentException("Invalid version format. Expected: x.y.z or x.y.z-suffix");
+    }
     Map<String, Object> buildInfo = new HashMap<>();
     buildInfo.put("result", true);
     buildInfo.put("versions", Collections.singletonList(version));
     DateTimeFormatter formatter = DateTimeFormatter.ofPattern("yyyy-MM-dd HH:mm:ss");
     buildInfo.put("endTime", buildTime.format(formatter));
     return buildInfo;
 }

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

base/src/main/java/com/tinyengine/it/service/material/impl/BlockServiceImpl.java (4)

96-97: Consider externalizing hardcoded constants.

These values should be configurable through application properties or environment variables to support different deployment environments and improve maintainability.

-    private static final int DEFAULT_PLATFORM_ID = 1;
-    private static final String DEFAULT_USER_ID = "1";
+    @Value("${app.default.platform-id}")
+    private int defaultPlatformId;
+    @Value("${app.default.user-id}")
+    private String defaultUserId;

---

189-204: Consider extracting group handling logic to a separate method.

The block group handling logic could be moved to a dedicated method to improve readability and maintainability.

+    private void deleteBlockGroups(Integer blockId, List<BlockGroup> blockGroups) {
+        if(blockGroups != null && !blockGroups.isEmpty()){
+            List<Integer> blockGroupIds = blockGroups.stream()
+                .map(BlockGroup::getId)
+                .collect(Collectors.toList());
+            for (Integer id : blockGroupIds) {
+                blockGroupBlockMapper.deleteByGroupIdAndBlockId(id, blockId);
+            }
+        }
+    }

---

702-703: Remove unnecessary semicolon.

There's a stray semicolon that should be removed.

-        List<Integer> groups = blockDto.getGroups().stream().map(BlockGroup::getId).collect(Collectors.toList());
-        ;
+        List<Integer> groups = blockDto.getGroups().stream().map(BlockGroup::getId).collect(Collectors.toList());

---

734-748: Consider making the date format configurable.

The date format pattern is hardcoded. Consider making it configurable through application properties to support different date format requirements.

+    @Value("${app.date-format.pattern:yyyy-MM-dd HH:mm:ss}")
+    private String dateFormatPattern;

     private Map<String, Object> createBuildInfo(String version, LocalDateTime buildTime) {
         Map<String, Object> buildInfo = new HashMap<>();
         buildInfo.put("result", true);
         buildInfo.put("versions", Collections.singletonList(version));
-        DateTimeFormatter formatter = DateTimeFormatter.ofPattern("yyyy-MM-dd HH:mm:ss");
+        DateTimeFormatter formatter = DateTimeFormatter.ofPattern(dateFormatPattern);
         buildInfo.put("endTime", buildTime.format(formatter));
         return buildInfo;
     }

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro