Yi EungJun 2015-08-17
perf: Avoid unnecessary permission check
Problem: When getting watchers only to know whether a specific user is watching
the resource, checking every user, who watches the resource, has the proper
permission to read the resource is not necessary.

Solution: Add a boolean parameter to getWatcher() methods to tell whether
permission check is necessary.

This makes it two times faster, to get an issue with 19 comments, in a project
watched by 344 users.
@fe5ebc4c4afbac5ac4203256bf1ff0a8e8c7901c
app/models/AbstractPosting.java
--- app/models/AbstractPosting.java
+++ app/models/AbstractPosting.java
@@ -220,13 +220,9 @@
         // default implementation for convenience
     }
 
-    /**
-     * @see {@link #getWatchers()}
-     * @see <a href="https://github.com/nforge/yobi/blob/master/docs/technical/watch.md>watch.md</a>
-     */
     @Transient
     public Set<User> getWatchers() {
-        return getWatchers(new HashSet<User>());
+        return getWatchers(true);
     }
 
     /**
@@ -234,7 +230,16 @@
      * @see <a href="https://github.com/nforge/yobi/blob/master/docs/technical/watch.md>watch.md</a>
      */
     @Transient
-    public Set<User> getWatchers(Set<User> baseWatchers) {
+    public Set<User> getWatchers(boolean allowedWatchersOnly) {
+        return getWatchers(new HashSet<User>(), allowedWatchersOnly);
+    }
+
+    /**
+     * @see {@link #getWatchers()}
+     * @see <a href="https://github.com/nforge/yobi/blob/master/docs/technical/watch.md>watch.md</a>
+     */
+    @Transient
+    public Set<User> getWatchers(Set<User> baseWatchers, boolean allowedWatchersOnly) {
         Set<User> actualWatchers = new HashSet<>();
 
         actualWatchers.addAll(baseWatchers);
@@ -247,7 +252,7 @@
             }
         }
 
-        return Watch.findActualWatchers(actualWatchers, asResource());
+        return Watch.findActualWatchers(actualWatchers, asResource(), allowedWatchersOnly);
     }
 
     protected void updateMention() {
app/models/Issue.java
--- app/models/Issue.java
+++ app/models/Issue.java
@@ -348,20 +348,25 @@
         return AbstractPosting.findByNumber(finder, project, number);
     }
 
+    @Transient
+    public Set<User> getWatchers() {
+        return getWatchers(true);
+    }
+
     /**
      * Returns all users watching or voting the issue.
      *
      * @return The set watching and voting the issue.
      */
     @Transient
-    public Set<User> getWatchers() {
+    public Set<User> getWatchers(boolean allowedWatchersOnly) {
         Set<User> baseWatchers = new HashSet<>();
         if (assignee != null) {
             baseWatchers.add(assignee.user);
         }
         baseWatchers.addAll(this.voters);
 
-        return super.getWatchers(baseWatchers);
+        return super.getWatchers(baseWatchers, allowedWatchersOnly);
     }
 
     public boolean assignedUserEquals(Assignee otherAssignee) {
app/models/PullRequest.java
--- app/models/PullRequest.java
+++ app/models/PullRequest.java
@@ -584,6 +584,10 @@
     }
 
     public Set<User> getWatchers() {
+        return getWatchers(true);
+    }
+
+    public Set<User> getWatchers(boolean allowedWatchersOnly) {
         Set<User> actualWatchers = new HashSet<>();
 
         actualWatchers.add(this.contributor);
@@ -596,7 +600,7 @@
             }
         }
 
-        return Watch.findActualWatchers(actualWatchers, asResource());
+        return Watch.findActualWatchers(actualWatchers, asResource(), allowedWatchersOnly);
     }
 
     /**
app/models/Watch.java
--- app/models/Watch.java
+++ app/models/Watch.java
@@ -157,7 +157,10 @@
         return isWatching(UserApp.currentUser(), resource.getType(), resource.getId());
     }
 
-    public static Set<User> findActualWatchers(final Set<User> baseWatchers, final Resource resource) {
+    public static Set<User> findActualWatchers(
+            final Set<User> baseWatchers,
+            final Resource resource,
+            boolean allowedWatchersOnly) {
         Set<User> actualWatchers = new HashSet<>();
         actualWatchers.addAll(baseWatchers);
 
@@ -172,12 +175,14 @@
         actualWatchers.removeAll(findUnwatchers(resource));
 
         // Filter the watchers who has no permission to read this resource.
-        CollectionUtils.filter(actualWatchers, new Predicate() {
-            @Override
-            public boolean evaluate(Object watcher) {
-                return AccessControl.isAllowed((User) watcher, resource, Operation.READ);
-            }
-        });
+        if (allowedWatchersOnly) {
+            CollectionUtils.filter(actualWatchers, new Predicate() {
+                @Override
+                public boolean evaluate(Object watcher) {
+                    return AccessControl.isAllowed((User) watcher, resource, Operation.READ);
+                }
+            });
+        }
         return actualWatchers;
     }
 
app/playRepository/Commit.java
--- app/playRepository/Commit.java
+++ app/playRepository/Commit.java
@@ -44,6 +44,10 @@
     public abstract int getParentCount();
 
     public Set<User> getWatchers(Project project) {
+        return getWatchers(project, true);
+    }
+
+    public Set<User> getWatchers(Project project, boolean allowedWatchersOnly) {
         Set<User> actualWatchers = new HashSet<>();
 
         // Add the author
@@ -76,7 +80,7 @@
             }
         }
 
-        return Watch.findActualWatchers(actualWatchers, asResource(project));
+        return Watch.findActualWatchers(actualWatchers, asResource(project), allowedWatchersOnly);
     }
 
     public static Project getProjectFromResourceId(String resourceId) {
app/views/board/view.scala.html
--- app/views/board/view.scala.html
+++ app/views/board/view.scala.html
@@ -72,8 +72,8 @@
     	<div class="board-footer board-actrow">
     	    <div class="pull-left">
                 @if(isAllowed(UserApp.currentUser(), post.asResource(), Operation.WATCH)){
-                    <button id="watch-button" type="button" class="ybtn @if(post.getWatchers.contains(UserApp.currentUser())){ybtn-watching}" data-toggle="button" data-watching="@if(post.getWatchers.contains(UserApp.currentUser())){true}else{false}">
-                    @if(post.getWatchers.contains(UserApp.currentUser())) { @Messages("project.unwatch") } else { @Messages("project.watch") }
+                    <button id="watch-button" type="button" class="ybtn @if(post.getWatchers(false).contains(UserApp.currentUser())){ybtn-watching}" data-toggle="button" data-watching="@if(post.getWatchers(false).contains(UserApp.currentUser())){true}else{false}">
+                    @if(post.getWatchers(false).contains(UserApp.currentUser())) { @Messages("project.unwatch") } else { @Messages("project.watch") }
                     </button>
                 }
             </div>
app/views/code/diff.scala.html
--- app/views/code/diff.scala.html
+++ app/views/code/diff.scala.html
@@ -176,7 +176,9 @@
             </div>
         </div>
 
-        <button id="watch-button" type="button" class="pull-left ybtn @if(commit.getWatchers(project).contains(UserApp.currentUser())) { active ybtn-watching }" data-toggle="button">@Messages("notification.watch")</button>
+        <button id="watch-button" type="button" class="pull-left ybtn @if(commit.getWatchers
+                (project, false).contains(UserApp.currentUser())) { active ybtn-watching }"
+        data-toggle="button">@Messages("notification.watch")</button>
 
         <a href="@routes.CodeHistoryApp.history(project.owner, project.name, selectedBranch, path)" class="ybtn pull-right">@Messages("button.list")</a>
     </div>
app/views/code/svnDiff.scala.html
--- app/views/code/svnDiff.scala.html
+++ app/views/code/svnDiff.scala.html
@@ -141,7 +141,8 @@
             @** // Comment **@
         </div>
 
-        <button id="watch-button" type="button" class="ybtn @if(commit.getWatchers(project).contains(UserApp.currentUser())) { active }" data-toggle="button">@Messages("notification.watch")</button>
+        <button id="watch-button" type="button" class="ybtn @if(commit.getWatchers(project, false)
+                .contains(UserApp.currentUser())) { active }" data-toggle="button">@Messages("notification.watch")</button>
 
         <a href="@routes.CodeHistoryApp.history(project.owner, project.name, selectedBranch, path)" class="ybtn pull-right">@Messages("button.list")</a>
 
app/views/git/view.scala.html
--- app/views/git/view.scala.html
+++ app/views/git/view.scala.html
@@ -57,8 +57,8 @@
         <div class="board-footer board-actrow">
             <div class="pull-left">
                 @if(isAllowed(UserApp.currentUser(), pull.asResource(), Operation.WATCH)) {
-                    <button id="watch-button" type="button" class="ybtn @if(pull.getWatchers.contains(UserApp.currentUser())) { ybtn-watching }" data-toggle="button" data-watching="@if(pull.getWatchers.contains(UserApp.currentUser())){true}else{false}">
-                    @if(pull.getWatchers.contains(UserApp.currentUser())) {
+                    <button id="watch-button" type="button" class="ybtn @if(pull.getWatchers(false).contains(UserApp.currentUser())) { ybtn-watching }" data-toggle="button" data-watching="@if(pull.getWatchers(false).contains(UserApp.currentUser())){true}else{false}">
+                    @if(pull.getWatchers(false).contains(UserApp.currentUser())) {
                         @Messages("project.unwatch")
                     } else {
                         @Messages("project.watch")
app/views/issue/view.scala.html
--- app/views/issue/view.scala.html
+++ app/views/issue/view.scala.html
@@ -107,9 +107,9 @@
                     <div class="pull-left">
                         <div>
                             @if(isAllowed(UserApp.currentUser(), issue.asResource(), Operation.WATCH)) {
-                                <button id="watch-button" type="button" class="ybtn @if(issue.getWatchers.contains(UserApp.currentUser())) {ybtn-watching}"
-                                        data-toggle="button" data-watching="@if(issue.getWatchers.contains(UserApp.currentUser())){true}else{false}">
-                                @if(issue.getWatchers.contains(UserApp.currentUser())) {
+                                <button id="watch-button" type="button" class="ybtn @if(issue.getWatchers(false).contains(UserApp.currentUser())) {ybtn-watching}"
+                                        data-toggle="button" data-watching="@if(issue.getWatchers(false).contains(UserApp.currentUser())){true}else{false}">
+                                @if(issue.getWatchers(false).contains(UserApp.currentUser())) {
                                     @Messages("issue.unwatch")
                                 } else {
                                     @Messages("issue.watch")
test/models/WatchTest.java
--- test/models/WatchTest.java
+++ test/models/WatchTest.java
@@ -165,7 +165,8 @@
         baseWatchers.add(base_watcher_not_member);
 
         // When
-        Set<User> actualWatchers = Watch.findActualWatchers(baseWatchers, resource_of_private_project);
+        Set<User> actualWatchers = Watch.findActualWatchers(baseWatchers,
+                resource_of_private_project, true);
 
         // Then
         assertThat(actualWatchers).containsOnly(watch_isssue, watch_project);
Add a comment
List