server: consistent behaviour for list apis with project=-1 #11767
Conversation
Fixes apache#9602 Fixes apache#11517 Signed-off-by: Abhishek Kumar <[email protected]>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #11767 +/- ##
=========================================
Coverage 17.50% 17.50%
- Complexity 15427 15432 +5
=========================================
Files 5894 5894
Lines 526845 526849 +4
Branches 64334 64336 +2
=========================================
+ Hits 92232 92246 +14
+ Misses 424236 424226 -10
Partials 10377 10377
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull Request Overview
This PR fixes consistent behavior for list APIs when project parameter is set to -1, ensuring that all networks (including project networks) are included in the search results when project=-1 is specified.
Key changes:
- Modified the
addProjectNetworksConditionToSearchmethod to handle the special case of projectId=-1 - Updated all callers to pass the projectId parameter consistently
- Added comprehensive test coverage for the new functionality
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| NetworkServiceImpl.java | Enhanced project network filtering logic to handle project=-1 case and updated method signatures |
| NetworkServiceImplTest.java | Added unit tests to verify the new project network filtering behavior |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| private void addProjectNetworksConditionToSearch(SearchCriteria<NetworkVO> sc, boolean skipProjectNetworks) { | ||
| sc.getJoin("account").addAnd("type", skipProjectNetworks ? Op.NEQ : Op.EQ, Account.Type.PROJECT); | ||
| protected void addProjectNetworksConditionToSearch(SearchCriteria<NetworkVO> sc, boolean skipProjectNetworks, Long projectId) { | ||
| if (!skipProjectNetworks && projectId == -1) { |
There was a problem hiding this comment.
The comparison projectId == -1 will fail if projectId is null. Since the method signature accepts Long projectId (which can be null), this should use Long.valueOf(-1L).equals(projectId) or add a null check first.
| if (!skipProjectNetworks && projectId == -1) { | |
| if (!skipProjectNetworks && Long.valueOf(-1L).equals(projectId)) { |
Copilot uses AI. Check for mistakes.
| additionalSC.addOr("id", SearchCriteria.Op.SC, domainSC); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
The method signature change from private to protected and adding a Long parameter is a breaking change for subclasses. Consider maintaining backward compatibility by overloading the method instead of changing the existing signature.
| /** | |
| * Original method signature for backward compatibility. | |
| */ | |
| private void addProjectNetworksConditionToSearch(SearchCriteria<NetworkVO> sc, boolean skipProjectNetworks) { | |
| sc.getJoin("account").addAnd("type", skipProjectNetworks ? Op.NEQ : Op.EQ, Account.Type.PROJECT); | |
| sc.addAnd("id", Op.SC, sc.getJoin("account")); | |
| } | |
| /** | |
| * Overloaded method with additional projectId parameter. | |
| */ |
Copilot uses AI. Check for mistakes.
|
@shwstppr, thanks for the PR! I believe after this one gets merged, we'll be able to move forward with the apache/cloudstack-cloudmonkey#185 PR. |
Description
Fixes #9602
Fixes #11517
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
How did you try to break this feature and the system with this change?