cleanup LDAP code according to warnings #11436

DaanHoogland · 2025-08-12T09:19:31Z

Description

This PR was part of investigation into the current LDAP workings, to find out that the issue under investigation was a user issue. I think this can be merged, but it has no functional changes.

Types of changes

Breaking change (fix or feature that would cause existing functionality to change)
New feature (non-breaking change which adds functionality)
Bug fix (non-breaking change which fixes an issue)
Enhancement (improves an existing feature and functionality)
Cleanup (Code refactoring and cleanup, that may add test cases)
build/CI
test (unit or integration test code)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Major
Minor

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

How did you try to break this feature and the system with this change?

codecov · 2025-08-12T09:27:47Z

Codecov Report

❌ Patch coverage is 41.17647% with 110 lines in your changes missing coverage. Please review.
✅ Project coverage is 17.50%. Comparing base (2ac2e5b) to head (2983839).
⚠️ Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
...va/org/apache/cloudstack/ldap/LdapManagerImpl.java	0.00%	52 Missing ⚠️
...pache/cloudstack/ldap/OpenLdapUserManagerImpl.java	5.88%	16 Missing ⚠️
.../org/apache/cloudstack/ldap/LdapConfiguration.java	74.35%	9 Missing and 1 partial ⚠️
...org/apache/cloudstack/ldap/LdapContextFactory.java	0.00%	7 Missing ⚠️
.../org/apache/cloudstack/ldap/LdapAuthenticator.java	40.00%	5 Missing and 1 partial ⚠️
...che/cloudstack/api/command/LdapImportUsersCmd.java	58.33%	5 Missing ⚠️
...e/cloudstack/api/command/LinkAccountToLdapCmd.java	0.00%	3 Missing ⚠️
...oudstack/api/command/LdapListConfigurationCmd.java	0.00%	2 Missing ⚠️
...pache/cloudstack/api/command/LdapListUsersCmd.java	92.30%	1 Missing and 1 partial ⚠️
...ache/cloudstack/api/command/LdapUserSearchCmd.java	0.00%	2 Missing ⚠️
... and 4 more

Additional details and impacted files

@@             Coverage Diff              @@
##              main   #11436       +/-   ##
============================================
+ Coverage     3.58%   17.50%   +13.91%     
- Complexity       0    15564    +15564     
============================================
  Files          445     5914     +5469     
  Lines        37571   529642   +492071     
  Branches      6921    64702    +57781     
============================================
+ Hits          1347    92691    +91344     
- Misses       36058   426503   +390445     
- Partials       166    10448    +10282

Flag	Coverage Δ
uitests	`3.58% <ø> (ø)`
unittests	`18.56% <41.17%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copilot

Pull Request Overview

This PR performs cleanup of the LDAP code according to compiler warnings. It removes unused methods, simplifies string concatenations, eliminates dead code, and modernizes various patterns without changing functionality.

Removes deprecated/unused methods and constructors
Simplifies string building patterns and improves logging
Updates method visibility and parameters for better consistency

Reviewed Changes

Copilot reviewed 25 out of 25 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
ADLdapUserManagerImplTest.java	Updates test setup to use static mocking and fixes assertion methods
LinkAccountToLdapCmdTest.java	Adds missing test assertion for account name
LdapListUsersCmdTest.java	Fixes method name from isACloudstackUser to isACloudStackUser
LdapConfigurationDaoImpl.java	Removes unused find method overload
LdapConfigurationDao.java	Removes unused method signature and improves javadoc
OpenLdapUserManagerImpl.java	Major refactoring: simplifies string building, uses static methods, improves logging
LdapUserManager.java	Removes public modifiers from interface methods
LdapUser.java	Makes memberships field final
LdapTrustMapVO.java	Removes empty constructor and updates hashCode implementation
LdapManagerImpl.java	Simplifies logging and removes unused method
LdapManager.java	Removes unused method signature
LdapContextFactory.java	Uses static methods for configuration access
LdapConfiguration.java	Makes many methods static and removes deprecated constructor
LdapAuthenticator.java	Improves logging and simplifies conditional logic
ADLdapUserManagerImpl.java	Uses static configuration methods and improves string building
LinkAccountToLdapResponse.java	Adds missing getter method
Various command classes	Removes empty constructors, improves parameter documentation, and modernizes patterns

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

...er-authenticators/ldap/src/main/java/org/apache/cloudstack/ldap/OpenLdapUserManagerImpl.java

Pearl1594

overall LGTM, some comments around formatting, etc..

...-authenticators/ldap/src/main/java/org/apache/cloudstack/api/command/LdapImportUsersCmd.java

...er-authenticators/ldap/src/main/java/org/apache/cloudstack/api/command/LdapListUsersCmd.java

Pearl1594 · 2025-12-10T14:56:00Z

...user-authenticators/ldap/src/main/java/org/apache/cloudstack/ldap/ADLdapUserManagerImpl.java

        }

-        String basedn = _ldapConfiguration.getBaseDn(domainId);
+        String basedn = LdapConfiguration.getBaseDn(domainId);


should we rather have

Suggested change

String basedn = LdapConfiguration.getBaseDn(domainId);

String basedn = ldapConfiguration.getBaseDn(domainId);

no, calling a static method

...ins/user-authenticators/ldap/src/main/java/org/apache/cloudstack/ldap/LdapAuthenticator.java

plugins/user-authenticators/ldap/src/main/java/org/apache/cloudstack/ldap/LdapManagerImpl.java

Pearl1594 · 2025-12-10T15:00:00Z

plugins/user-authenticators/ldap/src/main/java/org/apache/cloudstack/ldap/LdapManagerImpl.java

        String domainUuid = "<unknown>";
        if (domain == null) {
-            logger.error("no domain in database for id " + vo.getDomainId());
+            logger.error("no domain in database for id {}", vo.getDomainId());


Suggested change

logger.error("no domain in database for id {}", vo.getDomainId());

logger.error("No domain in database for id {}", vo.getDomainId());

However, do we need to log this with the database ID?

I think we need to no which domain is ill-configured, do we?

Suggested change

logger.error("no domain in database for id {}", vo.getDomainId());

logger.error(“No Domain in the database for ID {}", vo.getDomainId());

Now that I looked at the code implementation - should we check if the domain exists before persisting it into the trust map and fail the process if domain isn't found, rather than just logging the error?

Co-authored-by: Pearl Dsilva <pearl1594@gmail.com>

weizhouapache

code lgtm
Not tested

Pearl1594

code lgtm. Thanks @DaanHoogland

blueorangutan · 2025-12-17T13:51:42Z

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 16076

blueorangutan · 2025-12-18T06:05:08Z

[SF] Trillian test result (tid-15017)
Environment: kvm-ol9 (x2), zone: Advanced Networking with Mgmt server ol9
Total time taken: 55148 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr11436-t15017-kvm-ol9.zip
Smoke tests completed. 149 look OK, 1 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File
test_03_deploy_and_scale_kubernetes_cluster	`Failure`	26.65	test_kubernetes_clusters.py

boring-cyborg bot added the component:LDAP label Aug 12, 2025

DaanHoogland marked this pull request as ready for review August 13, 2025 07:25

DaanHoogland added the type:cleanup label Aug 13, 2025

DaanHoogland requested review from Copilot, shwstppr and sureshanaparti August 13, 2025 07:35

Copilot AI reviewed Aug 13, 2025

View reviewed changes

...er-authenticators/ldap/src/main/java/org/apache/cloudstack/ldap/OpenLdapUserManagerImpl.java Show resolved Hide resolved

DaanHoogland force-pushed the ldapCleanup branch from 8deca7b to 3b806be Compare August 15, 2025 19:15

DaanHoogland marked this pull request as draft August 16, 2025 15:57

DaanHoogland force-pushed the ldapCleanup branch 2 times, most recently from 2c3bddd to 44724f6 Compare August 19, 2025 10:47

DaanHoogland marked this pull request as ready for review August 20, 2025 07:49

DaanHoogland added this to the 4.22.0 milestone Aug 20, 2025

github-actions bot added status:has-conflicts and removed status:has-conflicts labels Sep 27, 2025

DaanHoogland force-pushed the ldapCleanup branch 2 times, most recently from 8865be3 to 85a647e Compare October 10, 2025 08:10

apache deleted a comment from blueorangutan Oct 10, 2025

harikrishna-patnala removed this from the 4.22.0 milestone Nov 12, 2025

apache deleted a comment from blueorangutan Dec 9, 2025

Pearl1594 reviewed Dec 10, 2025

View reviewed changes

Apply suggestions from code review

fc317f3

Co-authored-by: Pearl Dsilva <pearl1594@gmail.com>

weizhouapache approved these changes Dec 10, 2025

View reviewed changes

add validation of existing domain

f575a70

Pearl1594 approved these changes Dec 11, 2025

View reviewed changes

apache deleted a comment from blueorangutan Dec 15, 2025

github-actions bot added the status:has-conflicts label Dec 17, 2025

Merge branch 'main' into ldapCleanup

2983839

github-actions bot removed the status:has-conflicts label Dec 17, 2025

apache deleted a comment from github-actions bot Dec 17, 2025

apache deleted a comment from blueorangutan Dec 17, 2025

apache deleted a comment from github-actions bot Dec 17, 2025

DaanHoogland merged commit 91376c7 into apache:main Dec 18, 2025
27 of 28 checks passed

DaanHoogland deleted the ldapCleanup branch December 18, 2025 10:09

DaanHoogland modified the milestones: 4.22.1, 4.23.0 Dec 18, 2025

	String basedn = LdapConfiguration.getBaseDn(domainId);
	String basedn = ldapConfiguration.getBaseDn(domainId);

	logger.error("no domain in database for id {}", vo.getDomainId());
	logger.error("No domain in database for id {}", vo.getDomainId());

	logger.error("no domain in database for id {}", vo.getDomainId());
	logger.error(“No Domain in the database for ID {}", vo.getDomainId());

cleanup LDAP code according to warnings #11436

cleanup LDAP code according to warnings #11436

Uh oh!

Conversation

DaanHoogland commented Aug 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

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?

Uh oh!

codecov bot commented Aug 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Pearl1594 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Pearl1594 Dec 10, 2025

Choose a reason for hiding this comment

Uh oh!

DaanHoogland Dec 10, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Pearl1594 Dec 10, 2025

Choose a reason for hiding this comment

Uh oh!

DaanHoogland Dec 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Pearl1594 Dec 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weizhouapache left a comment

Choose a reason for hiding this comment

Uh oh!

Pearl1594 left a comment

Choose a reason for hiding this comment

Uh oh!

blueorangutan commented Dec 17, 2025

Uh oh!

blueorangutan commented Dec 18, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

DaanHoogland commented Aug 12, 2025 •

edited

Loading

codecov bot commented Aug 12, 2025 •

edited

Loading

DaanHoogland Dec 10, 2025 •

edited

Loading

Pearl1594 Dec 10, 2025 •

edited

Loading