Copilot commented on code in PR #11843:
URL: https://github.com/apache/cloudstack/pull/11843#discussion_r2455135399
##########
plugins/user-authenticators/ldap/src/main/java/org/apache/cloudstack/ldap/OpenLdapUserManagerImpl.java:
##########
@@ -205,18 +197,17 @@ public LdapUser getUser(final String username, final
String type, final String n
memberOfFilter.append(")");
}
- final StringBuilder searchQuery = new StringBuilder();
- searchQuery.append("(&");
- searchQuery.append(userObjectFilter);
- searchQuery.append(usernameFilter);
- searchQuery.append(memberOfFilter);
- searchQuery.append(")");
+ String searchQuery = "(&" +
+ userObjectFilter +
+ usernameFilter +
+ memberOfFilter +
+ ")";
- return searchUser(basedn, searchQuery.toString(), context, domainId);
+ return searchUser(basedn, searchQuery, context, domainId);
}
protected String getMemberOfAttribute(final Long domainId) {
- return _ldapConfiguration.getUserMemberOfAttribute(domainId);
+ return LdapConfiguration.getUserMemberOfAttribute(domainId);
Review Comment:
Calling `getUserMemberOfAttribute()` statically on `LdapConfiguration`
suggests this method was changed from instance to static. If this is an
instance method being called statically, this will cause a compilation error.
Verify that the method signature has been properly updated to be static, or
revert to calling it on the `_ldapConfiguration` instance.
```suggestion
return _ldapConfiguration.getUserMemberOfAttribute(domainId);
```
##########
plugins/user-authenticators/ldap/src/main/java/org/apache/cloudstack/ldap/OpenLdapUserManagerImpl.java:
##########
@@ -286,17 +277,15 @@ protected boolean isUserDisabled(SearchResult result)
throws NamingException {
return false;
}
- public LdapUser searchUser(final String basedn, final String searchString,
final LdapContext context, Long domainId) throws NamingException, IOException {
+ public LdapUser searchUser(final String basedn, final String searchString,
final LdapContext context, Long domainId) throws NamingException {
Review Comment:
Removing `IOException` from the throws clause is a breaking change to the
method signature. If any calling code explicitly catches `IOException` from
this method, it will cause compilation errors. Verify that all callers have
been updated accordingly, or consider whether this exception should remain for
backward compatibility.
##########
plugins/user-authenticators/ldap/src/main/java/org/apache/cloudstack/ldap/ADLdapUserManagerImpl.java:
##########
@@ -75,10 +73,18 @@ String generateADGroupSearchFilter(String groupName, Long
domainId) {
result.append(memberOfFilter);
result.append(")");
- logger.debug("group search filter = " + result);
+ logger.debug("group search filter = {}", result);
return result.toString();
}
Review Comment:
This method should be marked with `@Override` annotation since it overrides
the parent class method in `OpenLdapUserManagerImpl`. Adding the annotation
makes the inheritance relationship explicit and allows the compiler to verify
the override.
```suggestion
@Override
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
