fix: Modify block group api #182
Conversation
WalkthroughThis pull request introduces several changes across the codebase. In the common base module, the Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant CTRL as BlockGroupController
participant SVC as BlockGroupServiceImpl
participant M as BlockCarriersRelationMapper
C->>CTRL: Request to createBlockGroups(BlockGroup)
CTRL->>SVC: Call createBlockGroup(BlockGroup)
SVC->>SVC: Execute findBlockGroupById(id, DEFAULT_USER_ID)
SVC->>M: queryBlockGroupAndBlockById(blockId, DEFAULT_USER_ID)
M-->>SVC: Return block carrier relation
SVC->>SVC: Set block currentVersion (iterating if needed)
SVC-->>CTRL: Return Result<List<BlockGroup>>
CTRL-->>C: Respond with updated BlockGroup entity list
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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: 3
🧹 Nitpick comments (6)
base/src/main/java/com/tinyengine/it/service/material/impl/BlockGroupServiceImpl.java (3)
54-54: Externalize theDEFAULT_USER_ID.Hardcoding a user ID might cause maintenance and security issues in production environments. Consider externalizing it (e.g., configuration file, environment variable) to improve flexibility and avoid potential leaks.
73-90: Validate the block version retrieval logic.Currently, the loop retrieves only the first
BlockCarriersRelationrecord and applies its version ascurrentVersion. If multiple relations exist, only the first is used. Verify this is intended behavior, or consider retrieving the relevant version explicitly (e.g., the latest) to avoid potential data inconsistencies.
164-173: Revisit returning a singleBlockGroupin a list.This method returns a list containing only one newly created
BlockGroup. If only one group is created here, consider returning a single entity or updating the method signature and surrounding logic to avoid wrapping a single result in a list. Also, handle concurrency carefully if multiple threads attempt to create the same block group concurrently.base/src/test/java/com/tinyengine/it/service/material/impl/BlockGroupServiceImplTest.java (1)
104-112: Enhance test coverage for createBlockGroup method.While the test verifies the basic functionality, it could be more comprehensive by:
- Verifying the size of the returned list
- Checking the properties of the returned BlockGroup object
- Validating that the returned BlockGroup matches the input parameters
@Test void testCreateBlockGroup() { BlockGroup param = new BlockGroup(); when(blockGroupMapper.createBlockGroup(param)).thenReturn(1); BlockGroup blockGroupParam = new BlockGroup(); blockGroupParam.setId(1); Result<List<BlockGroup>> result = blockGroupServiceImpl.createBlockGroup(blockGroupParam); Assertions.assertNotNull(result.getData()); + Assertions.assertEquals(1, result.getData().size(), "Should return a singleton list"); + BlockGroup returnedGroup = result.getData().get(0); + Assertions.assertEquals(1, returnedGroup.getId(), "ID should match the input"); }base/src/main/java/com/tinyengine/it/controller/BlockGroupController.java (2)
141-148: Update API documentation to reflect singleton list return.The implementation correctly returns a singleton list containing the updated block group, but the API documentation could be clearer about this behavior.
@Operation(summary = "修改区块分组", description = "修改区块分组", parameters = { @Parameter(name = "id", description = "分组id"), - @Parameter(name = "blockGroups", description = "入参对象") + @Parameter(name = "blockGroup", description = "入参对象") }, responses = { @ApiResponse(responseCode = "200", description = "返回信息", content = @Content(mediaType = "application/json", - schema = @Schema(implementation = BlockGroup.class))), + schema = @Schema(implementation = List.class, description = "Returns a singleton list containing the updated BlockGroup"))), @ApiResponse(responseCode = "400", description = "请求失败")} )
170-178:❓ Verification inconclusive
Potential race condition in delete operation.
The current implementation has a potential race condition between checking for existence and performing the deletion. Consider using a transactional approach or implementing optimistic locking.
Here's a script to check if transactions are being used in the service layer:
🏁 Script executed:
#!/bin/bash # Description: Check for @Transactional annotations in the service implementation ast-grep --pattern 'class $_ { $$$ @Transactional $$$ deleteBlockGroupById($_) { $$$ } $$$ }'Length of output: 111
Action Required: Address Potential Race Condition in Delete Operation
The deletion method in
BlockGroupController.java(lines 170-178) performs an existence check before deleting without any clear transactional safeguards. Our initial automated search for a@Transactionalannotation in the service method did not return any confirmations. This raises the risk of a race condition if a concurrent deletion occurs between the check and deletion steps. Please manually verify whether transactional management or optimistic locking is implemented elsewhere in the service layer and consider wrapping the deletion within a transaction if not.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
base/src/main/java/com/tinyengine/it/common/base/HistoryEntity.java(2 hunks)base/src/main/java/com/tinyengine/it/controller/BlockGroupController.java(4 hunks)base/src/main/java/com/tinyengine/it/service/material/BlockGroupService.java(1 hunks)base/src/main/java/com/tinyengine/it/service/material/impl/BlockGroupServiceImpl.java(4 hunks)base/src/test/java/com/tinyengine/it/controller/BlockGroupControllerTest.java(2 hunks)base/src/test/java/com/tinyengine/it/service/material/impl/BlockGroupServiceImplTest.java(1 hunks)
🔇 Additional comments (3)
base/src/main/java/com/tinyengine/it/common/base/HistoryEntity.java (1)
15-15: LGTM!Clean addition of the required import for JsonProperty annotation.
base/src/main/java/com/tinyengine/it/service/material/BlockGroupService.java (1)
74-74: Ensure consumers of the revised return type are updated.Switching from
Result<List<BlockGroupDto>>toResult<List<BlockGroup>>affects all callers of this method. Confirm that all references, especially test assertions, have been correctly updated to handle entity objects instead of DTOs.base/src/main/java/com/tinyengine/it/controller/BlockGroupController.java (1)
115-118: LGTM! Return type change aligns with service layer.The return type change from DTO to entity object is consistent with the service layer changes.
base/src/test/java/com/tinyengine/it/controller/BlockGroupControllerTest.java
Outdated
Show resolved
Hide resolved
base/src/test/java/com/tinyengine/it/controller/BlockGroupControllerTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
base/src/test/java/com/tinyengine/it/controller/BlockGroupControllerTest.java (1)
89-92:⚠️ Potential issueFix incorrect assertion in delete test.
The test compares a populated block group with an empty one, which will fail. Additionally, it should verify the success of the delete operation.
Apply this diff to fix the test:
-BlockGroup resultData = new BlockGroup(); +BlockGroup expectedGroup = new BlockGroup(); +expectedGroup.setId(1); Result<List<BlockGroup>> result = blockGroupController.deleteBlockGroups(1); -Assertions.assertEquals(resultData, result.getData().get(0)); +Assertions.assertNotNull(result, "Result should not be null"); +Assertions.assertNotNull(result.getData(), "Result data should not be null"); +Assertions.assertEquals(1, result.getData().size(), "Should return exactly one block group"); +Assertions.assertEquals(expectedGroup.getId(), result.getData().get(0).getId(), + "Deleted block group should have the expected ID"); +// Verify that the delete operation was successful +Assertions.assertEquals(0, blockGroupService.findBlockGroupById(1), + "Block group should be deleted");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
base/src/main/java/com/tinyengine/it/common/base/HistoryEntity.java(2 hunks)base/src/test/java/com/tinyengine/it/controller/BlockGroupControllerTest.java(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- base/src/main/java/com/tinyengine/it/common/base/HistoryEntity.java
base/src/test/java/com/tinyengine/it/controller/BlockGroupControllerTest.java
Outdated
Show resolved
Hide resolved
base/src/test/java/com/tinyengine/it/controller/BlockGroupControllerTest.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
base/src/main/java/com/tinyengine/it/service/material/impl/BlockGroupServiceImpl.java (1)
46-266: Extract duplicate version setting logic into a helper method.The version setting logic is duplicated between
findBlockGroupByIdandgetBlockGroupByIdsOrAppId. Consider extracting it into a helper method to improve maintainability.Add a helper method:
+/** + * Sets the current version for blocks in a block group based on their carrier relations. + * + * @param blockGroup The block group containing blocks to update + * @param hostId The ID of the host (block group) + */ +private void setBlockVersions(BlockGroup blockGroup, Integer hostId) { + if (blockGroup == null || blockGroup.getBlocks().isEmpty()) { + return; + } + + List<Integer> blockIds = blockGroup.getBlocks().stream() + .map(Block::getId) + .collect(Collectors.toList()); + + BlockCarriersRelation queryParam = new BlockCarriersRelation(); + queryParam.setHostId(hostId); + queryParam.setHostType(Enums.BlockGroup.BLOCK_GROUP.getValue()); + queryParam.setBlockIds(blockIds); + + Map<Integer, String> blockVersions = blockCarriersRelationMapper + .queryBlockCarriersRelationByCondition(queryParam).stream() + .collect(Collectors.toMap( + BlockCarriersRelation::getBlockId, + BlockCarriersRelation::getVersion, + (v1, v2) -> v1 + )); + + blockGroup.getBlocks().forEach(block -> + block.setCurrentVersion(blockVersions.get(block.getId()))); +}Then use it in both methods:
public BlockGroup findBlockGroupById(@Param("id") Integer id) { BlockGroup blockGroupResult = blockGroupMapper.queryBlockGroupAndBlockById(id, null, DEFAULT_USER_ID); - // ... version setting logic ... + setBlockVersions(blockGroupResult, id); return blockGroupResult; } public List<BlockGroup> getBlockGroupByIdsOrAppId(...) { // ... existing logic ... for (BlockGroup blockGroupTemp : blockGroupsListResult) { - // ... version setting logic ... + setBlockVersions(blockGroupTemp, blockGroupTemp.getId()); } return blockGroupsListResult; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/checkstyle.yml(1 hunks)base/src/main/java/com/tinyengine/it/service/material/impl/BlockGroupServiceImpl.java(4 hunks)base/src/test/java/com/tinyengine/it/controller/BlockGroupControllerTest.java(3 hunks)base/src/test/java/com/tinyengine/it/service/material/impl/BlockGroupServiceImplTest.java(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- base/src/test/java/com/tinyengine/it/controller/BlockGroupControllerTest.java
- base/src/test/java/com/tinyengine/it/service/material/impl/BlockGroupServiceImplTest.java
🔇 Additional comments (1)
.github/workflows/checkstyle.yml (1)
13-13:⚠️ Potential issue❌ Incorrect review comment
Verify Ubuntu runner version.
The workflow specifies Ubuntu 24.04 which is not yet released. This could cause workflow failures.
Apply this diff to use a stable Ubuntu version:
- runs-on: ubuntu-24.04 + runs-on: ubuntu-22.04
🌐 Web query:
What is the latest stable version of Ubuntu available in GitHub Actions runners?💡 Result:
The latest stable version of Ubuntu available in GitHub Actions runners as of February 2025 is Ubuntu 24.04 LTS (Noble Numbat)[2][5][9]. This became the default image for
ubuntu-latestlabels following a phased rollout that started in September 2024[9].Key details:
- YAML labels:
ubuntu-latestor explicitubuntu-24.04[5]- Migration timeline:
- Announced GA on September 25, 2024[9]
- Full migration completed by January 17, 2025[8]
- Previous versions still available but deprecated:
- Ubuntu 22.04 (explicitly via
ubuntu-22.04label)[5]- Ubuntu 20.04 (being phased out by April 1, 2025)[4]
The image includes kernel version 6.5.0-1025-azure and systemd 249.11-0ubuntu3.12[6]. Users should update workflows from older Ubuntu versions like
ubuntu-20.04before April 2025 to avoid disruptions[4].Citations:
- 1: https://github.com/actions/runner/releases
- 2: Ubuntu-latest workflows will use Ubuntu-24.04 image actions/runner-images#10636
- 3: https://docs.github.com/en/actions/using-github-hosted-runners/using-github-hosted-runners/about-github-hosted-runners
- 4: https://forum.ansible.com/t/github-actions-still-using-ubuntu-20-04/40187
- 5: https://github.com/actions/runner-images
- 6: https://github.com/actions/runner-images/blob/main/images/ubuntu/Ubuntu2204-Readme.md
- 7: https://github.blog/changelog/2022-08-09-github-actions-ubuntu-22-04-is-now-generally-available-on-github-hosted-runners/
- 8: https://dev.to/siddhantkcode/critical-changes-coming-to-github-actions-ubuntu-24-migration-guide-oo8
- 9: https://github.blog/changelog/2024-09-25-actions-new-images-and-ubuntu-latest-changes/
- 10: https://discourse.ubuntu.com/t/psa-for-folks-using-python-in-github-action-runners-and-ubuntu-latest-label/48654
Ubuntu 24.04 is now the correct and supported runner version.
The web query confirms that as of February 2025, Ubuntu 24.04 LTS is the latest stable version available for GitHub Actions and has been fully rolled out. Therefore, no changes are necessary in the workflow file.
- File:
.github/workflows/checkstyle.yml, line 13- Remove the suggestion to downgrade to Ubuntu 22.04.
Likely an incorrect or invalid review comment.
base/src/main/java/com/tinyengine/it/service/material/impl/BlockGroupServiceImpl.java
Show resolved
Hide resolved
base/src/main/java/com/tinyengine/it/service/material/impl/BlockGroupServiceImpl.java
Show resolved
Hide resolved
base/src/main/java/com/tinyengine/it/service/material/impl/BlockGroupServiceImpl.java
Show resolved
Hide resolved
base/src/main/java/com/tinyengine/it/service/material/impl/BlockGroupServiceImpl.java
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