snoopdave commented on code in PR #222:
URL: https://github.com/apache/roller/pull/222#discussion_r4419618189
##########
app/src/main/java/org/apache/roller/weblogger/business/jpa/JPAUserManagerImpl.java:
##########
@@ -289,6 +289,36 @@ public List<User> getUsersStartingWith(String startsWith,
Boolean enabled,
return query.getResultList();
}
+ @Override
+ public List<User> getUsersWithUserNameStartingWith(String startsWith,
Boolean enabled,
+ int offset, int length) throws WebloggerException {
+ TypedQuery<User> query;
+ if (enabled != null) {
+ query =
strategy.getNamedQuery("User.getByEnabled&UserNameStartsWith", User.class);
+ query.setParameter(1, enabled);
+ query.setParameter(2, likePrefix(startsWith == null ? "" :
startsWith));
+ } else {
+ query = strategy.getNamedQuery("User.getByUserNameStartsWith",
User.class);
+ query.setParameter(1, likePrefix(startsWith == null ? "" :
startsWith));
+ }
+ if (offset != 0) {
+ query.setFirstResult(offset);
+ }
+ if (length != -1) {
+ query.setMaxResults(length);
+ }
+ return query.getResultList();
+ }
+
+ /**
+ * LIKE pattern that matches values starting with the given text. The
+ * named queries use '!' as the escape character, so '%' and '_' in the
+ * text match only themselves.
+ */
+ static String likePrefix(String startsWith) {
Review Comment:
Two things:
- Other queries build LIKE patterns without escaping:
`JPAWeblogEntryManagerImpl:449`, `:648` and `:1098`, and
`JPAMediaFileManagerImpl:595`. A shared helper (a JPA util class) would let
those be fixed the same way later.
- This is package-private and pure, so it's easy to unit-test directly.
Nothing tests `!` yet: `a!b` gives `a!!b%`, `a%b` gives `a!%b%`, `""` gives `%`.
##########
app/src/main/java/org/apache/roller/weblogger/ui/struts2/ajax/UserDataServlet.java:
##########
@@ -106,8 +106,10 @@ public void doGet(HttpServletRequest request,
HttpServletResponse response)
Weblogger roller = WebloggerFactory.getWeblogger();
try {
UserManager umgr = roller.getUserManager();
- List<User> users = umgr.getUsersStartingWith(startsWith,
- enabledOnly, offset, length);
+ // only admins, who see email addresses, may search by them
+ List<User> users = admin
+ ? umgr.getUsersStartingWith(startsWith, enabledOnly,
offset, length)
+ : umgr.getUsersWithUserNameStartingWith(startsWith,
enabledOnly, offset, length);
Review Comment:
For non-admins, could we also limit the parameters?
- Force `enabled=true`. Inviting a disabled account makes no sense, and
today any signed-in user can list them with `enabled=false`.
- Cap `length` at `MAX_LENGTH`. Now `length` replaces the cap, so an empty
`startsWith` with a large `length` lists every user.
About four lines, in the method this PR already changes.
##########
app/src/main/java/org/apache/roller/weblogger/business/UserManager.java:
##########
@@ -162,6 +162,18 @@ List<User> getUsers(
*/
List<User> getUsersStartingWith(String startsWith,
Boolean enabled, int offset, int length) throws WebloggerException;
+
+ /**
+ * Lookup users whose usernames start with a string.
+ *
+ * @param startsWith String to match userNames against
+ * @param enabled True for only enabled, false for disabled, null for
all
+ * @param offset Offset into results (for paging)
+ * @param length Max to return (for paging)
+ * @return List of (up to length) users that match startsWith string
+ */
+ List<User> getUsersWithUserNameStartingWith(String startsWith,
Review Comment:
Minor: next to this, `getUsersStartingWith` doesn't say it also matches
email addresses. A javadoc line there ("also matches email addresses; for admin
callers") would warn the next caller. Renaming it would break the API in a
patch release, so a comment seems enough.
##########
app/src/main/java/org/apache/roller/weblogger/business/jpa/JPAUserManagerImpl.java:
##########
@@ -289,6 +289,36 @@ public List<User> getUsersStartingWith(String startsWith,
Boolean enabled,
return query.getResultList();
}
+ @Override
+ public List<User> getUsersWithUserNameStartingWith(String startsWith,
Boolean enabled,
Review Comment:
This repeats the enabled/null branching and paging of
`getUsersStartingWith`. A private method with a flag for whether to match
email, with both public methods calling it, would keep them from drifting.
Keeping two public methods is good, since callers have to choose.
--
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]
