Commit @fe5ebc4c4afbac5ac4203256bf1ff0a8e8c7901c

[Notice] Announcing the End of Demo Server [Read me]

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

6ebf578

fe5ebc4

app/models/AbstractPosting.java

--- app/models/AbstractPosting.java

+++ app/models/AbstractPosting.java


         // 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);
     }
 
     /**

      * @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);

             }
         }
 
-        return Watch.findActualWatchers(actualWatchers, asResource());
+        return Watch.findActualWatchers(actualWatchers, asResource(), allowedWatchersOnly);
     }
 
     protected void updateMention() {

6ebf578

fe5ebc4

app/models/Issue.java

--- app/models/Issue.java

+++ app/models/Issue.java


         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) {

6ebf578

fe5ebc4

app/models/PullRequest.java

--- app/models/PullRequest.java

+++ app/models/PullRequest.java


     }
 
     public Set<User> getWatchers() {
+        return getWatchers(true);
+    }
+
+    public Set<User> getWatchers(boolean allowedWatchersOnly) {
         Set<User> actualWatchers = new HashSet<>();
 
         actualWatchers.add(this.contributor);

             }
         }
 
-        return Watch.findActualWatchers(actualWatchers, asResource());
+        return Watch.findActualWatchers(actualWatchers, asResource(), allowedWatchersOnly);
     }
 
     /**

6ebf578

fe5ebc4

app/models/Watch.java

--- app/models/Watch.java

+++ app/models/Watch.java


         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);
 

         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;
     }
 

6ebf578

fe5ebc4

app/playRepository/Commit.java

--- app/playRepository/Commit.java

+++ app/playRepository/Commit.java


     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

             }
         }
 
-        return Watch.findActualWatchers(actualWatchers, asResource(project));
+        return Watch.findActualWatchers(actualWatchers, asResource(project), allowedWatchersOnly);
     }
 
     public static Project getProjectFromResourceId(String resourceId) {

6ebf578

fe5ebc4

app/views/board/view.scala.html

--- app/views/board/view.scala.html

+++ app/views/board/view.scala.html


     	<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>

6ebf578

fe5ebc4

app/views/code/diff.scala.html

--- app/views/code/diff.scala.html

+++ app/views/code/diff.scala.html


             </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>

6ebf578

fe5ebc4

app/views/code/svnDiff.scala.html

--- app/views/code/svnDiff.scala.html

+++ app/views/code/svnDiff.scala.html


             @** // 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>
 

6ebf578

fe5ebc4

app/views/git/view.scala.html

--- app/views/git/view.scala.html

+++ app/views/git/view.scala.html


         <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")

6ebf578

fe5ebc4

app/views/issue/view.scala.html

--- app/views/issue/view.scala.html

+++ app/views/issue/view.scala.html


                     <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")

6ebf578

fe5ebc4

test/models/WatchTest.java

--- test/models/WatchTest.java

+++ test/models/WatchTest.java


         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

Open 0
Closed 0

List

...	...	@@ -220,13 +220,9 @@
220	220	// default implementation for convenience
221	221	}
222	222
223		- /**
224		- * @see {@link #getWatchers()}
225		- * @see <a href="https://github.com/nforge/yobi/blob/master/docs/technical/watch.md>watch.md</a>
226		- */
227	223	@Transient
228	224	public Set<User> getWatchers() {
229		- return getWatchers(new HashSet<User>());
	225	+ return getWatchers(true);
230	226	}
231	227
232	228	/**
...	...	@@ -234,7 +230,16 @@
234	230	* @see <a href="https://github.com/nforge/yobi/blob/master/docs/technical/watch.md>watch.md</a>
235	231	*/
236	232	@Transient
237		- public Set<User> getWatchers(Set<User> baseWatchers) {
	233	+ public Set<User> getWatchers(boolean allowedWatchersOnly) {
	234	+ return getWatchers(new HashSet<User>(), allowedWatchersOnly);
	235	+ }
	236	+
	237	+ /**
	238	+ * @see {@link #getWatchers()}
	239	+ * @see <a href="https://github.com/nforge/yobi/blob/master/docs/technical/watch.md>watch.md</a>
	240	+ */
	241	+ @Transient
	242	+ public Set<User> getWatchers(Set<User> baseWatchers, boolean allowedWatchersOnly) {
238	243	Set<User> actualWatchers = new HashSet<>();
239	244
240	245	actualWatchers.addAll(baseWatchers);
...	...	@@ -247,7 +252,7 @@
247	252	}
248	253	}
249	254
250		- return Watch.findActualWatchers(actualWatchers, asResource());
	255	+ return Watch.findActualWatchers(actualWatchers, asResource(), allowedWatchersOnly);
251	256	}
252	257
253	258	protected void updateMention() {

...	...	@@ -348,20 +348,25 @@
348	348	return AbstractPosting.findByNumber(finder, project, number);
349	349	}
350	350
	351	+ @Transient
	352	+ public Set<User> getWatchers() {
	353	+ return getWatchers(true);
	354	+ }
	355	+
351	356	/**
352	357	* Returns all users watching or voting the issue.
353	358	*
354	359	* @return The set watching and voting the issue.
355	360	*/
356	361	@Transient
357		- public Set<User> getWatchers() {
	362	+ public Set<User> getWatchers(boolean allowedWatchersOnly) {
358	363	Set<User> baseWatchers = new HashSet<>();
359	364	if (assignee != null) {
360	365	baseWatchers.add(assignee.user);
361	366	}
362	367	baseWatchers.addAll(this.voters);
363	368
364		- return super.getWatchers(baseWatchers);
	369	+ return super.getWatchers(baseWatchers, allowedWatchersOnly);
365	370	}
366	371
367	372	public boolean assignedUserEquals(Assignee otherAssignee) {

...	...	@@ -584,6 +584,10 @@
584	584	}
585	585
586	586	public Set<User> getWatchers() {
	587	+ return getWatchers(true);
	588	+ }
	589	+
	590	+ public Set<User> getWatchers(boolean allowedWatchersOnly) {
587	591	Set<User> actualWatchers = new HashSet<>();
588	592
589	593	actualWatchers.add(this.contributor);
...	...	@@ -596,7 +600,7 @@
596	600	}
597	601	}
598	602
599		- return Watch.findActualWatchers(actualWatchers, asResource());
	603	+ return Watch.findActualWatchers(actualWatchers, asResource(), allowedWatchersOnly);
600	604	}
601	605
602	606	/**

...	...	@@ -157,7 +157,10 @@
157	157	return isWatching(UserApp.currentUser(), resource.getType(), resource.getId());
158	158	}
159	159
160		- public static Set<User> findActualWatchers(final Set<User> baseWatchers, final Resource resource) {
	160	+ public static Set<User> findActualWatchers(
	161	+ final Set<User> baseWatchers,
	162	+ final Resource resource,
	163	+ boolean allowedWatchersOnly) {
161	164	Set<User> actualWatchers = new HashSet<>();
162	165	actualWatchers.addAll(baseWatchers);
163	166
...	...	@@ -172,12 +175,14 @@
172	175	actualWatchers.removeAll(findUnwatchers(resource));
173	176
174	177	// Filter the watchers who has no permission to read this resource.
175		- CollectionUtils.filter(actualWatchers, new Predicate() {
176		- @Override
177		- public boolean evaluate(Object watcher) {
178		- return AccessControl.isAllowed((User) watcher, resource, Operation.READ);
179		- }
180		- });
	178	+ if (allowedWatchersOnly) {
	179	+ CollectionUtils.filter(actualWatchers, new Predicate() {
	180	+ @Override
	181	+ public boolean evaluate(Object watcher) {
	182	+ return AccessControl.isAllowed((User) watcher, resource, Operation.READ);
	183	+ }
	184	+ });
	185	+ }
181	186	return actualWatchers;
182	187	}
183	188

...	...	@@ -44,6 +44,10 @@
44	44	public abstract int getParentCount();
45	45
46	46	public Set<User> getWatchers(Project project) {
	47	+ return getWatchers(project, true);
	48	+ }
	49	+
	50	+ public Set<User> getWatchers(Project project, boolean allowedWatchersOnly) {
47	51	Set<User> actualWatchers = new HashSet<>();
48	52
49	53	// Add the author
...	...	@@ -76,7 +80,7 @@
76	80	}
77	81	}
78	82
79		- return Watch.findActualWatchers(actualWatchers, asResource(project));
	83	+ return Watch.findActualWatchers(actualWatchers, asResource(project), allowedWatchersOnly);
80	84	}
81	85
82	86	public static Project getProjectFromResourceId(String resourceId) {

...	...	@@ -72,8 +72,8 @@
72	72	<div class="board-footer board-actrow">
73	73	<div class="pull-left">
74	74	@if(isAllowed(UserApp.currentUser(), post.asResource(), Operation.WATCH)){
75		- <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}">
76		- @if(post.getWatchers.contains(UserApp.currentUser())) { @Messages("project.unwatch") } else { @Messages("project.watch") }
	75	+ <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}">
	76	+ @if(post.getWatchers(false).contains(UserApp.currentUser())) { @Messages("project.unwatch") } else { @Messages("project.watch") }
77	77	</button>
78	78	}
79	79	</div>

...	...	@@ -176,7 +176,9 @@
176	176	</div>
177	177	</div>
178	178
179		- <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>
	179	+ <button id="watch-button" type="button" class="pull-left ybtn @if(commit.getWatchers
	180	+ (project, false).contains(UserApp.currentUser())) { active ybtn-watching }"
	181	+ data-toggle="button">@Messages("notification.watch")</button>
180	182
181	183	<a href="@routes.CodeHistoryApp.history(project.owner, project.name, selectedBranch, path)" class="ybtn pull-right">@Messages("button.list")</a>
182	184	</div>

...	...	@@ -141,7 +141,8 @@
141	141	@ // Comment @
142	142	</div>
143	143
144		- <button id="watch-button" type="button" class="ybtn @if(commit.getWatchers(project).contains(UserApp.currentUser())) { active }" data-toggle="button">@Messages("notification.watch")</button>
	144	+ <button id="watch-button" type="button" class="ybtn @if(commit.getWatchers(project, false)
	145	+ .contains(UserApp.currentUser())) { active }" data-toggle="button">@Messages("notification.watch")</button>
145	146
146	147	<a href="@routes.CodeHistoryApp.history(project.owner, project.name, selectedBranch, path)" class="ybtn pull-right">@Messages("button.list")</a>
147	148

...	...	@@ -57,8 +57,8 @@
57	57	<div class="board-footer board-actrow">
58	58	<div class="pull-left">
59	59	@if(isAllowed(UserApp.currentUser(), pull.asResource(), Operation.WATCH)) {
60		- <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}">
61		- @if(pull.getWatchers.contains(UserApp.currentUser())) {
	60	+ <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}">
	61	+ @if(pull.getWatchers(false).contains(UserApp.currentUser())) {
62	62	@Messages("project.unwatch")
63	63	} else {
64	64	@Messages("project.watch")

...	...	@@ -107,9 +107,9 @@
107	107	<div class="pull-left">
108	108	<div>
109	109	@if(isAllowed(UserApp.currentUser(), issue.asResource(), Operation.WATCH)) {
110		- <button id="watch-button" type="button" class="ybtn @if(issue.getWatchers.contains(UserApp.currentUser())) {ybtn-watching}"
111		- data-toggle="button" data-watching="@if(issue.getWatchers.contains(UserApp.currentUser())){true}else{false}">
112		- @if(issue.getWatchers.contains(UserApp.currentUser())) {
	110	+ <button id="watch-button" type="button" class="ybtn @if(issue.getWatchers(false).contains(UserApp.currentUser())) {ybtn-watching}"
	111	+ data-toggle="button" data-watching="@if(issue.getWatchers(false).contains(UserApp.currentUser())){true}else{false}">
	112	+ @if(issue.getWatchers(false).contains(UserApp.currentUser())) {
113	113	@Messages("issue.unwatch")
114	114	} else {
115	115	@Messages("issue.watch")

...	...	@@ -165,7 +165,8 @@
165	165	baseWatchers.add(base_watcher_not_member);
166	166
167	167	// When
168		- Set<User> actualWatchers = Watch.findActualWatchers(baseWatchers, resource_of_private_project);
	168	+ Set<User> actualWatchers = Watch.findActualWatchers(baseWatchers,
	169	+ resource_of_private_project, true);
169	170
170	171	// Then
171	172	assertThat(actualWatchers).containsOnly(watch_isssue, watch_project);

Delete comment