fix: Modify block group #173
Conversation
WalkthroughThis pull request modifies the control flow in two service implementations and updates a mapper XML. In the Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Client/Application
participant BGService as BlockGroupServiceImpl
participant DB as Database
Caller->>BGService: getBlockGroupByIdsOrAppId(ids)
BGService->>DB: Query block groups by IDs
DB-->>BGService: Return blockGroupsListResult
BGService->>BGService: Check if list is empty or first element id is null
alt List empty or first element id is null
BGService-->>Caller: Return early (or handle accordingly)
else Valid list with id present
BGService-->>Caller: Proceed with further processing
end
sequenceDiagram
participant Caller as Client/Application
participant BService as BlockServiceImpl
participant DB as Database
Caller->>BService: deploy(blockBuildDto)
BService->>DB: updateBlockById(blockBuildDto)
DB-->>BService: Update result
BService-->>Caller: (No return value due to removal of return statement)
Possibly related PRs
Suggested reviewers
Poem
Tip 🌐 Web search-backed reviews and chat
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 0
🔭 Outside diff range comments (2)
base/src/main/java/com/tinyengine/it/service/material/impl/BlockGroupServiceImpl.java (2)
169-171: Security concern: Remove hardcoded user IDs.Using hardcoded user IDs ("1") is a security risk. Consider injecting the actual user context.
- String groupCreatedBy = "1"; // 获取登录用户id - String blockCreatedBy = "1"; + String groupCreatedBy = userContextService.getCurrentUserId(); + String blockCreatedBy = userContextService.getCurrentUserId();
190-203: Fix incorrect block version lookup.The version lookup uses the wrong block group ID. It's using the local
blockGroupvariable instead ofblockGroupTempfrom the iteration.for (BlockGroup blockGroupTemp : blockGroupsListResult) { for (Block block : blockGroupTemp.getBlocks()) { BlockCarriersRelation queryParam = new BlockCarriersRelation(); queryParam.setBlockId(block.getId()); - queryParam.setHostId(blockGroup.getId()); + queryParam.setHostId(blockGroupTemp.getId()); queryParam.setHostType(Enums.BlockGroup.BLOCK_GROUP.getValue());
🧹 Nitpick comments (3)
base/src/main/resources/mappers/BlockGroupMapper.xml (1)
332-340: NewqueryBlockGroupByConditionSelect Statement Addition
The new select statement effectively reuses the<include>fragments for the base column list and conditional filters (BlockGroupByCondition), ensuring that SQL logic remains consistent and centralized. TheWHERE 1=1clause is a common idiom to facilitate the dynamic appending of conditions; however, if it’s not necessary in your context, consider removing it or documenting its purpose for future maintainers.base/src/main/java/com/tinyengine/it/service/material/impl/BlockGroupServiceImpl.java (2)
186-186: Improve null check implementation.While checking for null IDs is good, consider a more defensive implementation that avoids potential edge cases.
- if (blockGroupsListResult.isEmpty() || blockGroupsListResult.get(0).getId() == null) { + if (blockGroupsListResult.isEmpty() || blockGroupsListResult.stream().anyMatch(bg -> bg.getId() == null)) {
166-206: Consider refactoring to separate concerns.The method handles multiple responsibilities: querying block groups, filtering by user, and updating version information. Consider splitting these into separate methods for better maintainability.
Example refactor:
@Override public List<BlockGroup> getBlockGroupByIdsOrAppId(List<Integer> ids, Integer appId, String from) { String blockCreatedBy = getBlockCreatedBy(from); String groupCreatedBy = userContextService.getCurrentUserId(); List<BlockGroup> blockGroups = queryBlockGroups(ids, appId, blockCreatedBy, groupCreatedBy); if (blockGroups.isEmpty() || blockGroups.stream().anyMatch(bg -> bg.getId() == null)) { return blockGroups; } return enrichBlockVersions(blockGroups); } private List<BlockGroup> queryBlockGroups(List<Integer> ids, Integer appId, String blockCreatedBy, String groupCreatedBy) { if (ids != null) { return queryByIds(ids, blockCreatedBy, groupCreatedBy); } if (appId != null) { return blockGroupMapper.queryBlockGroupByAppId(appId, blockCreatedBy, groupCreatedBy); } return blockGroupMapper.queryAllBlockGroupAndBlock(blockCreatedBy, groupCreatedBy); } private List<BlockGroup> enrichBlockVersions(List<BlockGroup> blockGroups) { for (BlockGroup blockGroup : blockGroups) { enrichBlockVersionsForGroup(blockGroup); } return blockGroups; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
base/src/main/java/com/tinyengine/it/service/material/impl/BlockGroupServiceImpl.java(1 hunks)base/src/main/java/com/tinyengine/it/service/material/impl/BlockServiceImpl.java(0 hunks)base/src/main/resources/mappers/BlockGroupMapper.xml(2 hunks)
💤 Files with no reviewable changes (1)
- base/src/main/java/com/tinyengine/it/service/material/impl/BlockServiceImpl.java
🔇 Additional comments (1)
base/src/main/resources/mappers/BlockGroupMapper.xml (1)
314-321: Simplify and Validate Conditional Logic in<choose>Block
The refactored<choose>structure simplifies the control flow by consolidating the conditions for handlingblockCreatedBy. This enhances readability and maintainability. However, note that the originalotherwiseclause previously checked that all three fields—b.last_build_info,b.content, andb.assets—were not null, whereas the new implementation only verifiesb.last_build_infoandb.content. Please confirm that omitting theb.assetscondition is an intentional change in behavior.
* fix: update page history bug * fix: format code and fix style issue * fix: modify page histiry entity * feat: add code submission rules * fix: modify app schema for test * fix: modify t_i18n_entry u_idx_i18n_entity * fix: modify block update api * fix: Modify code format * fix: modify block group api * fix: modify block group mapper * fix: Update checkstyle.yml * fix: Modify block create and update api * fix: Modify code format * fix: Modify block group
2 participants
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
Bug Fixes
New Features