Make handling of web links consistent

The WebLinks class now provides all web links as FluentIterables which
avoids unnecessary copying in the caller. Web links that do not have a
name or URL are now always filtered out in the WebLinks class and
callers don't need to do this anymore. In addition, if there are no web
links, the web_links field is not set in the JSON that is sent to
clients.

Implementations of *WebLink can now also return null to indicate that
no web link should be shown for the resource described by the provided
parameters.

Some of these improvements were already done for file web links by
b62414c0c and 2ae832e7e.

Change-Id: I28b98c25ab0f8c4411a6be845181560a636f5e8d

Documentation/rest-api-changes.txt

@@ -3360,13 +3360,14 @@ used instead of ab.
 === DiffFileMetaInfo
 The `DiffFileMetaInfo` entity contains meta information about a file diff.
 
-[options="header",width="50%",cols="1,6"]
+[options="header",width="50%",cols="1,^1,5"]
 |==========================
-|Field Name    |Description
-|`name`        |The name of the file.
-|`content_type`|The content type of the file.
-|`lines`       |The total number of lines in the file.
-|'web_links'   |Links to the file in external sites as a list of
+|Field Name    ||Description
+|`name`        ||The name of the file.
+|`content_type`||The content type of the file.
+|`lines`       ||The total number of lines in the file.
+|'web_links'   |optional|
+Links to the file in external sites as a list of
 link:rest-api-changes.html#web-link-info[WebLinkInfo] entries.
 |==========================
 

gerrit-extension-api/src/main/java/com/google/gerrit/extensions/webui/BranchWebLink.java

@@ -18,14 +18,21 @@ import com.google.gerrit.extensions.annotations.ExtensionPoint;
 import com.google.gerrit.extensions.common.WebLinkInfo;
 
 @ExtensionPoint
-public interface BranchWebLink {
+public interface BranchWebLink extends WebLink {
 
   /**
-   * URL to branch in external service.
+   * {@link com.google.gerrit.extensions.common.WebLinkInfo}
+   * describing a link from a branch to an external service.
+   *
+   * <p>In order for the web link to be visible
+   * {@link com.google.gerrit.extensions.common.WebLinkInfo#url}
+   * and {@link com.google.gerrit.extensions.common.WebLinkInfo#name}
+   * must be set.<p>
    *
    * @param projectName Name of the project
    * @param branchName Name of the branch
-   * @return WebLinkInfo that links to branch in external service.
+   * @return WebLinkInfo that links to branch in external service,
+   * null if there should be no link.
    */
   WebLinkInfo getBranchWebLink(String projectName, String branchName);
 }

gerrit-extension-api/src/main/java/com/google/gerrit/extensions/webui/FileWebLink.java

@@ -18,15 +18,22 @@ import com.google.gerrit.extensions.annotations.ExtensionPoint;
 import com.google.gerrit.extensions.common.WebLinkInfo;
 
 @ExtensionPoint
-public interface FileWebLink {
+public interface FileWebLink extends WebLink {
 
   /**
-   * URL to file in external service.
+   * {@link com.google.gerrit.extensions.common.WebLinkInfo}
+   * describing a link from a file to an external service.
+   *
+   * <p>In order for the web link to be visible
+   * {@link com.google.gerrit.extensions.common.WebLinkInfo#url}
+   * and {@link com.google.gerrit.extensions.common.WebLinkInfo#name}
+   * must be set.<p>
    *
    * @param projectName Name of the project
    * @param revision Name of the revision (e.g. branch or commit ID)
    * @param fileName Name of the file
-   * @return WebLinkInfo that links to project in external service.
+   * @return WebLinkInfo that links to project in external service,
+   * null if there should be no link.
    */
   WebLinkInfo getFileWebLink(String projectName, String revision, String fileName);
 }

gerrit-extension-api/src/main/java/com/google/gerrit/extensions/webui/PatchSetWebLink.java

@@ -11,20 +11,28 @@
 // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 // See the License for the specific language governing permissions and
 // limitations under the License.
+
 package com.google.gerrit.extensions.webui;
 
 import com.google.gerrit.extensions.annotations.ExtensionPoint;
 import com.google.gerrit.extensions.common.WebLinkInfo;
 
 @ExtensionPoint
-public interface PatchSetWebLink {
+public interface PatchSetWebLink extends WebLink{
 
   /**
-   * URL to patch set in external service.
+   * {@link com.google.gerrit.extensions.common.WebLinkInfo}
+   * describing a link from a patch set to an external service.
+   *
+   * <p>In order for the web link to be visible
+   * {@link com.google.gerrit.extensions.common.WebLinkInfo#url}
+   * and {@link com.google.gerrit.extensions.common.WebLinkInfo#name}
+   * must be set.<p>
    *
    * @param projectName Name of the project
    * @param commit Commit of the patch set
-   * @return WebLinkInfo that links to patch set in external service.
+   * @return WebLinkInfo that links to patch set in external service,
+   * null if there should be no link.
    */
   WebLinkInfo getPathSetWebLink(final String projectName, final String commit);
 }

gerrit-extension-api/src/main/java/com/google/gerrit/extensions/webui/ProjectWebLink.java

@@ -18,13 +18,20 @@ import com.google.gerrit.extensions.annotations.ExtensionPoint;
 import com.google.gerrit.extensions.common.WebLinkInfo;
 
 @ExtensionPoint
-public interface ProjectWebLink {
+public interface ProjectWebLink extends WebLink {
 
   /**
-   * URL to project in external service.
+   * {@link com.google.gerrit.extensions.common.WebLinkInfo}
+   * describing a link from a project to an external service.
+   *
+   * <p>In order for the web link to be visible
+   * {@link com.google.gerrit.extensions.common.WebLinkInfo#url}
+   * and {@link com.google.gerrit.extensions.common.WebLinkInfo#name}
+   * must be set.<p>
    *
    * @param projectName Name of the project
-   * @return WebLinkInfo that links to project in external service.
+   * @return WebLinkInfo that links to project in external service,
+   * null if there should be no link.
    */
   WebLinkInfo getProjectWeblink(String projectName);
 }

gerrit-extension-api/src/main/java/com/google/gerrit/extensions/webui/WebLink.java

@@ -11,26 +11,35 @@
 // WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 // See the License for the specific language governing permissions and
 // limitations under the License.
+
 package com.google.gerrit.extensions.webui;
 
+
 /**
- * Class that holds target defaults for WebLink anchors.
+ * Marks that the implementor has a method that provides
+ * a weblinkInfo
+ *
  */
-public class WebLinkTarget {
+public interface WebLink {
   /**
-   * Opens the link in a new window or tab
+   * Class that holds target defaults for WebLink anchors.
    */
-  public final static String BLANK = "_blank";
-  /**
-   * Opens the link in the frame it was clicked.
-   */
-  public final static String SELF = "_self";
-  /**
-   * Opens link in parent frame.
-   */
-  public final static String PARENT = "_parent";
-  /**
-   * Opens link in the full body of the window.
-   */
-  public final static String TOP = "_top";
+  public static class Target {
+    /**
+     * Opens the link in a new window or tab
+     */
+    public final static String BLANK = "_blank";
+    /**
+     * Opens the link in the frame it was clicked.
+     */
+    public final static String SELF = "_self";
+    /**
+     * Opens link in parent frame.
+     */
+    public final static String PARENT = "_parent";
+    /**
+     * Opens link in the full body of the window.
+     */
+    public final static String TOP = "_top";
+  }
 }

gerrit-server/src/main/java/com/google/gerrit/server/WebLinks.java

@@ -14,22 +14,42 @@
 
 package com.google.gerrit.server;
 
+import com.google.common.base.Function;
+import com.google.common.base.Predicate;
 import com.google.common.base.Strings;
-import com.google.common.collect.Lists;
+import com.google.common.collect.FluentIterable;
 import com.google.gerrit.extensions.common.WebLinkInfo;
 import com.google.gerrit.extensions.registration.DynamicSet;
 import com.google.gerrit.extensions.webui.BranchWebLink;
 import com.google.gerrit.extensions.webui.FileWebLink;
 import com.google.gerrit.extensions.webui.PatchSetWebLink;
 import com.google.gerrit.extensions.webui.ProjectWebLink;
+import com.google.gerrit.extensions.webui.WebLink;
 import com.google.inject.Inject;
 import com.google.inject.Singleton;
 
-import java.util.ArrayList;
-import java.util.List;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 @Singleton
 public class WebLinks {
+  private static final Logger log = LoggerFactory.getLogger(WebLinks.class);
+  private static final Predicate<WebLinkInfo> INVALID_WEBLINK =
+      new Predicate<WebLinkInfo>() {
+
+        @Override
+        public boolean apply(WebLinkInfo link) {
+          if (link == null){
+            return false;
+          } else if (Strings.isNullOrEmpty(link.name)
+              || Strings.isNullOrEmpty(link.url)) {
+            log.warn(String.format("%s is missing name and/or url",
+                link.getClass().getName()));
+            return false;
+          }
+          return true;
+        }
+      };
 
   private final DynamicSet<PatchSetWebLink> patchSetLinks;
   private final DynamicSet<FileWebLink> fileLinks;
@@ -47,39 +67,77 @@ public class WebLinks {
     this.branchLinks = branchLinks;
   }
 
-  public List<WebLinkInfo> getPatchSetLinks(String project, String commit) {
-    List<WebLinkInfo> links = new ArrayList<>(4);
-    for (PatchSetWebLink webLink : patchSetLinks) {
-      links.add(webLink.getPathSetWebLink(project, commit));
-    }
-    return links;
-  }
+  /**
+   *
+   * @param project Project name.
+   * @param commit SHA1 of commit.
+   * @return Links for patch sets.
+   */
+  public FluentIterable<WebLinkInfo> getPatchSetLinks(final String project,
+      final String commit) {
+    return filterLinks(patchSetLinks, new Function<WebLink, WebLinkInfo>() {
 
-  public List<WebLinkInfo> getFileLinks(String project, String revision,
-      String file) {
-    List<WebLinkInfo> links = new ArrayList<>(4);
-    for (FileWebLink webLink : fileLinks) {
-      WebLinkInfo info = webLink.getFileWebLink(project, revision, file);
-      if (!Strings.isNullOrEmpty(info.name) && !Strings.isNullOrEmpty(info.url)) {
-        links.add(info);
+      @Override
+      public WebLinkInfo apply(WebLink webLink) {
+        return ((PatchSetWebLink)webLink).getPathSetWebLink(project, commit);
       }
-    }
-    return links;
+    });
   }
 
-  public Iterable<WebLinkInfo> getProjectLinks(String project) {
-    List<WebLinkInfo> links = Lists.newArrayList();
-    for (ProjectWebLink webLink : projectLinks) {
-      links.add(webLink.getProjectWeblink(project));
-    }
-    return links;
+  /**
+   *
+   * @param project Project name.
+   * @param revision SHA1 of revision.
+   * @param file File name.
+   * @return Links for files.
+   */
+  public FluentIterable<WebLinkInfo> getFileLinks(final String project, final String revision,
+      final String file) {
+    return filterLinks(fileLinks, new Function<WebLink, WebLinkInfo>() {
+
+      @Override
+      public WebLinkInfo apply(WebLink webLink) {
+        return ((FileWebLink)webLink).getFileWebLink(project, revision, file);
+      }
+    });
   }
 
-  public Iterable<WebLinkInfo> getBranchLinks(String project, String branch) {
-    List<WebLinkInfo> links = Lists.newArrayList();
-    for (BranchWebLink webLink : branchLinks) {
-      links.add(webLink.getBranchWebLink(project, branch));
-    }
-    return links;
+  /**
+   *
+   * @param project Project name.
+   * @return Links for projects.
+   */
+  public FluentIterable<WebLinkInfo> getProjectLinks(final String project) {
+    return filterLinks(projectLinks, new Function<WebLink, WebLinkInfo>() {
+
+      @Override
+      public WebLinkInfo apply(WebLink webLink) {
+        return ((ProjectWebLink)webLink).getProjectWeblink(project);
+      }
+    });
   }
-}
+
+  /**
+   *
+   * @param project Project name
+   * @param branch Branch name
+   * @return Links for branches.
+   */
+  public FluentIterable<WebLinkInfo> getBranchLinks(final String project, final String branch) {
+    return filterLinks(branchLinks, new Function<WebLink, WebLinkInfo>() {
+
+      @Override
+      public WebLinkInfo apply(WebLink webLink) {
+        return ((BranchWebLink)webLink).getBranchWebLink(project, branch);
+      }
+    });
+  }
+
+  private FluentIterable<WebLinkInfo> filterLinks(DynamicSet<? extends WebLink> links,
+      Function<WebLink, WebLinkInfo> transformer) {
+    return FluentIterable
+        .from(links)
+        .transform(transformer)
+        .filter(INVALID_WEBLINK);
+  }
+}

gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java

@@ -33,6 +33,7 @@ import static com.google.gerrit.extensions.common.ListChangesOption.WEB_LINKS;
 import com.google.common.base.Joiner;
 import com.google.common.base.MoreObjects;
 import com.google.common.base.Optional;
+import com.google.common.collect.FluentIterable;
 import com.google.common.collect.HashBasedTable;
 import com.google.common.collect.HashMultimap;
 import com.google.common.collect.ImmutableList;
@@ -843,9 +844,9 @@ public class ChangeJson {
     }
 
     if (has(WEB_LINKS)) {
-      List<WebLinkInfo> links =
+      FluentIterable<WebLinkInfo> links =
           webLinks.getPatchSetLinks(project, in.getRevision().get());
-      out.webLinks = !links.isEmpty() ? links : null;
+      out.webLinks = links.isEmpty() ? null : links.toList();
     }
     return out;
   }

gerrit-server/src/main/java/com/google/gerrit/server/change/GetDiff.java

@@ -17,6 +17,7 @@ package com.google.gerrit.server.change;
 import static com.google.common.base.Preconditions.checkState;
 
 import com.google.common.base.MoreObjects;
+import com.google.common.collect.FluentIterable;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Lists;
 import com.google.gerrit.common.data.PatchScript;
@@ -204,9 +205,9 @@ public class GetDiff implements RestReadView<FileResource> {
 
   private List<WebLinkInfo> getFileWebLinks(Project project, String rev,
       String file) {
-    List<WebLinkInfo> links =
+    FluentIterable<WebLinkInfo> links =
         webLinks.getFileLinks(project.getName(), rev, file);
-    return !links.isEmpty() ? links : null;
+    return links.isEmpty() ? null : links.toList();
   }
 
   static class Result {

gerrit-server/src/main/java/com/google/gerrit/server/project/ListBranches.java

@@ -15,7 +15,7 @@
 package com.google.gerrit.server.project;
 
 import com.google.common.base.Predicate;
-import com.google.common.base.Strings;
+import com.google.common.collect.FluentIterable;
 import com.google.common.collect.Iterables;
 import com.google.common.collect.Lists;
 import com.google.common.collect.Sets;
@@ -240,13 +240,10 @@ public class ListBranches implements RestReadView<ProjectResource> {
       }
       info.actions.put(d.getId(), new ActionInfo(d));
     }
-    info.webLinks = Lists.newArrayList();
-    for (WebLinkInfo link : webLinks.getBranchLinks(
-        refControl.getProjectControl().getProject().getName(), ref.getName())) {
-      if (!Strings.isNullOrEmpty(link.name) && !Strings.isNullOrEmpty(link.url)) {
-        info.webLinks.add(link);
-      }
-    }
+    FluentIterable<WebLinkInfo> links =
+        webLinks.getBranchLinks(
+            refControl.getProjectControl().getProject().getName(), ref.getName());
+    info.webLinks = links.isEmpty() ? null : links.toList();
     return info;
   }
 

gerrit-server/src/main/java/com/google/gerrit/server/project/ListProjects.java

@@ -16,6 +16,7 @@ package com.google.gerrit.server.project;
 
 import com.google.common.base.Predicate;
 import com.google.common.base.Strings;
+import com.google.common.collect.FluentIterable;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Iterables;
 import com.google.common.collect.Lists;
@@ -382,13 +383,9 @@ public class ListProjects implements RestReadView<TopLevelResource> {
             log.warn("Unexpected error reading " + projectName, err);
             continue;
           }
-
-          info.webLinks = Lists.newArrayList();
-          for (WebLinkInfo link : webLinks.getProjectLinks(projectName.get())) {
-            if (!Strings.isNullOrEmpty(link.name) && !Strings.isNullOrEmpty(link.url)) {
-              info.webLinks.add(link);
-            }
-          }
+          FluentIterable<WebLinkInfo> links =
+              webLinks.getProjectLinks(projectName.get());
+          info.webLinks = links.isEmpty() ? null : links.toList();
         }
 
         if (foundIndex++ < start) {

gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectJson.java

@@ -15,7 +15,7 @@
 package com.google.gerrit.server.project;
 
 import com.google.common.base.Strings;
-import com.google.common.collect.Lists;
+import com.google.common.collect.FluentIterable;
 import com.google.gerrit.extensions.common.ProjectInfo;
 import com.google.gerrit.extensions.common.WebLinkInfo;
 import com.google.gerrit.extensions.restapi.Url;
@@ -51,14 +51,9 @@ public class ProjectJson {
     info.description = Strings.emptyToNull(p.getDescription());
     info.state = p.getState();
     info.id = Url.encode(info.name);
-
-    info.webLinks = Lists.newArrayList();
-    for (WebLinkInfo link : webLinks.getProjectLinks(p.getName())) {
-      if (!Strings.isNullOrEmpty(link.name) && !Strings.isNullOrEmpty(link.url)) {
-        info.webLinks.add(link);
-      }
-    }
-
+    FluentIterable<WebLinkInfo> links =
+        webLinks.getProjectLinks(p.getName());
+    info.webLinks = links.isEmpty() ? null : links.toList();
     return info;
   }
 }