mike-jumper commented on a change in pull request #550:
URL: https://github.com/apache/guacamole-client/pull/550#discussion_r460610448
##########
File path:
extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/group/UserGroupService.java
##########
@@ -208,13 +208,21 @@ private ExprNode getGroupSearchFilter() throws
GuacamoleException {
}
}
+ // Gather all attributes relevant for a group
+ ArrayList<String> groupAttributes = new ArrayList<String>();
Review comment:
1. The size of the required backing array can be anticipated here. This
will work as-is, but unnecessarily relies on automatic array resizing or
sufficiently-large defaults.
2. Unless code consuming this relies on the object being an `ArrayList`, the
type declared for the variable should be kept minimal, presumably
`List<String>` or `Collection<String>`.
##########
File path:
extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/ObjectQueryService.java
##########
@@ -197,6 +197,10 @@ public ExprNode generateQuery(ExprNode filter,
* @param searchHop
* The current level of referral depth for this search, used for
* limiting the maximum depth to which referrals can go.
+ *
+ * @param relevantAttributes
+ * The attribute(s) relevant to return for this search,
+ * if all available should be returned pass null as value.
Review comment:
1. Please correct the alignment here. It looks like a tab character may
have snuck in.
2. Documentation should describe the nature/meaning of things, rather than
commanding the developer to do a specific thing under specific circumstances.
The first half of the documentation for this parameter looks good, but the
latter half switches over from descriptive to imperative. I think we should
maintain the established descriptive phrasing (for example: "or null if all
attributes should be returned").
##########
File path:
extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/group/UserGroupService.java
##########
@@ -208,13 +208,21 @@ private ExprNode getGroupSearchFilter() throws
GuacamoleException {
}
}
+ // Gather all attributes relevant for a group
+ ArrayList<String> groupAttributes = new ArrayList<String>();
+ groupAttributes.add(confService.getMemberAttribute());
+ confService.getGroupNameAttributes().forEach(
+ attribute -> groupAttributes.add(attribute)
+ );
Review comment:
Why not `addAll()`?
And what if one of the attribute names within
`confService.getGroupNameAttributes()` is equal to
`confService.getMemberAttribute()`? Will the duplicate attribute cause any
problems for the search?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
