Merge branch bugfix_illegal_sortby into 2.0.0-M3

scm-webapp/src/main/java/sonia/scm/api/rest/resources/AbstractManagerResource.java

@@ -55,6 +55,11 @@ import javax.ws.rs.core.Request;
 import javax.ws.rs.core.Response;
 import javax.ws.rs.core.Response.Status;
 import javax.ws.rs.core.UriInfo;
+import java.beans.BeanInfo;
+import java.beans.IntrospectionException;
+import java.beans.Introspector;
+import java.beans.PropertyDescriptor;
+import java.util.Arrays;
 import java.util.Collection;
 import java.util.Comparator;
 import java.util.Date;
@@ -76,17 +81,15 @@ public abstract class AbstractManagerResource<T extends ModelObject,
   private static final Logger logger =
     LoggerFactory.getLogger(AbstractManagerResource.class);
 
-  //~--- constructors ---------------------------------------------------------
+  protected final Manager<T, E> manager;
+  private final Class<T> type;
 
-  /**
-   * Constructs ...
-   *
-   *
-   * @param manager
-   */
-  public AbstractManagerResource(Manager<T, E> manager)
-  {
+  protected int cacheMaxAge = 0;
+  protected boolean disableCache = false;
+
+  public AbstractManagerResource(Manager<T, E> manager, Class<T> type) {
     this.manager = manager;
+    this.type = type;
   }
 
   //~--- methods --------------------------------------------------------------
@@ -526,45 +529,25 @@ public abstract class AbstractManagerResource<T extends ModelObject,
     return builder.build();
   }
 
-  /**
-   * Method description
-   *
-   *
-   * @param sortby
-   * @param desc
-   *
-   * @return
-   */
   @SuppressWarnings("unchecked")
-  private Comparator<T> createComparator(String sortby, boolean desc)
+  private Comparator<T> createComparator(String sortBy, boolean desc)
   {
+    checkSortByField(sortBy);
     Comparator comparator;
 
     if (desc)
     {
-      comparator = new BeanReverseComparator(sortby);
+      comparator = new BeanReverseComparator(sortBy);
     }
     else
     {
-      comparator = new BeanComparator(sortby);
+      comparator = new BeanComparator(sortBy);
     }
 
     return comparator;
   }
 
-  /**
-   * Method description
-   *
-   *
-   *
-   * @param sortby
-   * @param desc
-   * @param start
-   * @param limit
-   *
-   * @return
-   */
-  private Collection<T> fetchItems(String sortby, boolean desc, int start,
+  private Collection<T> fetchItems(String sortBy, boolean desc, int start,
     int limit)
   {
     AssertUtil.assertPositive(start);
@@ -573,18 +556,18 @@ public abstract class AbstractManagerResource<T extends ModelObject,
 
     if (limit > 0)
     {
-      if (Util.isEmpty(sortby))
+      if (Util.isEmpty(sortBy))
       {
 
         // replace with something useful
-        sortby = "id";
+        sortBy = "id";
       }
 
-      items = manager.getAll(createComparator(sortby, desc), start, limit);
+      items = manager.getAll(createComparator(sortBy, desc), start, limit);
     }
-    else if (Util.isNotEmpty(sortby))
+    else if (Util.isNotEmpty(sortBy))
     {
-      items = manager.getAll(createComparator(sortby, desc));
+      items = manager.getAll(createComparator(sortBy, desc));
     }
     else
     {
@@ -594,17 +577,32 @@ public abstract class AbstractManagerResource<T extends ModelObject,
     return items;
   }
 
-  protected PageResult<T> fetchPage(String sortby, boolean desc, int pageNumber,
+  // We have to handle IntrospectionException here, because it's a checked exception
+  // It shouldn't occur really - so creating a new unchecked exception would be over-engineered here
+  @SuppressWarnings("squid:S00112")
+  private void checkSortByField(String sortBy) {
+    try {
+      BeanInfo info = Introspector.getBeanInfo(type);
+      PropertyDescriptor[] pds = info.getPropertyDescriptors();
+      if (Arrays.stream(pds).noneMatch(p -> p.getName().equals(sortBy))) {
+        throw new IllegalArgumentException("sortBy");
+      }
+    } catch (IntrospectionException e) {
+      throw new RuntimeException("error introspecting model type " + type.getName(), e);
+    }
+  }
+
+  protected PageResult<T> fetchPage(String sortBy, boolean desc, int pageNumber,
     int pageSize) {
     AssertUtil.assertPositive(pageNumber);
     AssertUtil.assertPositive(pageSize);
 
-    if (Util.isEmpty(sortby)) {
+    if (Util.isEmpty(sortBy)) {
       // replace with something useful
-      sortby = "id";
+      sortBy = "id";
     }
 
-    return manager.getPage(createComparator(sortby, desc), pageNumber, pageSize);
+    return manager.getPage(createComparator(sortBy, desc), pageNumber, pageSize);
   }
 
   //~--- get methods ----------------------------------------------------------
@@ -676,16 +674,4 @@ public abstract class AbstractManagerResource<T extends ModelObject,
       return super.compare(o1, o2) * -1;
     }
   }
-
-
-  //~--- fields ---------------------------------------------------------------
-
-  /** Field description */
-  protected int cacheMaxAge = 0;
-
-  /** Field description */
-  protected boolean disableCache = false;
-
-  /** Field description */
-  protected Manager<T, E> manager;
 }

scm-webapp/src/main/java/sonia/scm/api/rest/resources/GroupResource.java

@@ -41,18 +41,12 @@ import com.webcohesion.enunciate.metadata.rs.ResponseCode;
 import com.webcohesion.enunciate.metadata.rs.ResponseHeader;
 import com.webcohesion.enunciate.metadata.rs.StatusCodes;
 import com.webcohesion.enunciate.metadata.rs.TypeHint;
-
 import org.apache.shiro.SecurityUtils;
-
 import sonia.scm.group.Group;
 import sonia.scm.group.GroupException;
 import sonia.scm.group.GroupManager;
 import sonia.scm.security.Role;
 
-//~--- JDK imports ------------------------------------------------------------
-
-import java.util.Collection;
-
 import javax.ws.rs.Consumes;
 import javax.ws.rs.DELETE;
 import javax.ws.rs.DefaultValue;
@@ -69,6 +63,9 @@ import javax.ws.rs.core.MediaType;
 import javax.ws.rs.core.Request;
 import javax.ws.rs.core.Response;
 import javax.ws.rs.core.UriInfo;
+import java.util.Collection;
+
+//~--- JDK imports ------------------------------------------------------------
 
 /**
  * RESTful Web Service Resource to manage groups and their members.
@@ -97,7 +94,7 @@ public class GroupResource
   @Inject
   public GroupResource(GroupManager groupManager)
   {
-    super(groupManager);
+    super(groupManager, Group.class);
   }
 
   //~--- methods --------------------------------------------------------------

scm-webapp/src/main/java/sonia/scm/api/rest/resources/RepositoryResource.java

@@ -131,7 +131,7 @@ public class RepositoryResource extends AbstractManagerResource<Repository, Repo
     RepositoryManager repositoryManager,
     RepositoryServiceFactory servicefactory, HealthChecker healthChecker)
   {
-    super(repositoryManager);
+    super(repositoryManager, Repository.class);
     this.configuration = configuration;
     this.repositoryManager = repositoryManager;
     this.servicefactory = servicefactory;

scm-webapp/src/main/java/sonia/scm/api/rest/resources/UserResource.java

@@ -41,10 +41,8 @@ import com.webcohesion.enunciate.metadata.rs.ResponseCode;
 import com.webcohesion.enunciate.metadata.rs.ResponseHeader;
 import com.webcohesion.enunciate.metadata.rs.StatusCodes;
 import com.webcohesion.enunciate.metadata.rs.TypeHint;
-
 import org.apache.shiro.SecurityUtils;
 import org.apache.shiro.authc.credential.PasswordService;
-
 import sonia.scm.security.Role;
 import sonia.scm.user.User;
 import sonia.scm.user.UserException;
@@ -52,11 +50,6 @@ import sonia.scm.user.UserManager;
 import sonia.scm.util.AssertUtil;
 import sonia.scm.util.Util;
 
-//~--- JDK imports ------------------------------------------------------------
-
-import java.util.Collection;
-
-import javax.ws.rs.Consumes;
 import javax.ws.rs.DELETE;
 import javax.ws.rs.DefaultValue;
 import javax.ws.rs.GET;
@@ -72,6 +65,9 @@ import javax.ws.rs.core.MediaType;
 import javax.ws.rs.core.Request;
 import javax.ws.rs.core.Response;
 import javax.ws.rs.core.UriInfo;
+import java.util.Collection;
+
+//~--- JDK imports ------------------------------------------------------------
 
 /**
  * RESTful Web Service Resource to manage users.
@@ -101,7 +97,7 @@ public class UserResource extends AbstractManagerResource<User, UserException>
   @Inject
   public UserResource(UserManager userManager, PasswordService passwordService)
   {
-    super(userManager);
+    super(userManager, User.class);
     this.passwordService = passwordService;
   }
 

scm-webapp/src/main/java/sonia/scm/api/v2/resources/GroupCollectionResource.java

@@ -1,13 +1,23 @@
 package sonia.scm.api.v2.resources;
 
-import com.webcohesion.enunciate.metadata.rs.*;
+import com.webcohesion.enunciate.metadata.rs.ResponseCode;
+import com.webcohesion.enunciate.metadata.rs.ResponseHeader;
+import com.webcohesion.enunciate.metadata.rs.ResponseHeaders;
+import com.webcohesion.enunciate.metadata.rs.StatusCodes;
+import com.webcohesion.enunciate.metadata.rs.TypeHint;
 import sonia.scm.group.Group;
 import sonia.scm.group.GroupException;
 import sonia.scm.group.GroupManager;
 import sonia.scm.web.VndMediaType;
 
 import javax.inject.Inject;
-import javax.ws.rs.*;
+import javax.ws.rs.Consumes;
+import javax.ws.rs.DefaultValue;
+import javax.ws.rs.GET;
+import javax.ws.rs.POST;
+import javax.ws.rs.Path;
+import javax.ws.rs.Produces;
+import javax.ws.rs.QueryParam;
 import javax.ws.rs.core.Response;
 import java.io.IOException;
 
@@ -26,16 +36,17 @@ public class GroupCollectionResource {
     this.dtoToGroupMapper = dtoToGroupMapper;
     this.groupCollectionToDtoMapper = groupCollectionToDtoMapper;
     this.resourceLinks = resourceLinks;
-    this.adapter = new ResourceManagerAdapter<>(manager);
+    this.adapter = new ResourceManagerAdapter<>(manager, Group.class);
   }
 
   /**
    * Returns all groups for a given page number with a given page size (default page size is {@value DEFAULT_PAGE_SIZE}).
-   * 
+   *
    * <strong>Note:</strong> This method requires "group" privilege.
-   *  @param page     the number of the requested page
+   *
+   * @param page     the number of the requested page
    * @param pageSize the page size (default page size is {@value DEFAULT_PAGE_SIZE})
-   * @param sortBy   sort parameter
+   * @param sortBy   sort parameter (if empty - undefined sorting)
    * @param desc     sort direction desc or aesc
    */
   @GET
@@ -44,6 +55,7 @@ public class GroupCollectionResource {
   @TypeHint(GroupDto[].class)
   @StatusCodes({
     @ResponseCode(code = 200, condition = "success"),
+    @ResponseCode(code = 400, condition = "\"sortBy\" field unknown"),
     @ResponseCode(code = 401, condition = "not authenticated / invalid credentials"),
     @ResponseCode(code = 403, condition = "not authorized, the current user does not have the \"group\" privilege"),
     @ResponseCode(code = 500, condition = "internal server error")

scm-webapp/src/main/java/sonia/scm/api/v2/resources/GroupResource.java

@@ -29,7 +29,7 @@ public class GroupResource {
                        GroupDtoToGroupMapper groupDtoToGroupMapper) {
     this.groupToGroupDtoMapper = groupToGroupDtoMapper;
     this.dtoToGroupMapper = groupDtoToGroupMapper;
-    this.adapter = new ResourceManagerAdapter<>(manager);
+    this.adapter = new ResourceManagerAdapter<>(manager, Group.class);
   }
 
   /**

scm-webapp/src/main/java/sonia/scm/api/v2/resources/ResourceManagerAdapter.java

@@ -30,8 +30,8 @@ class ResourceManagerAdapter<MODEL_OBJECT extends ModelObject,
                              DTO extends HalRepresentation,
                              EXCEPTION extends Exception> extends AbstractManagerResource<MODEL_OBJECT, EXCEPTION> {
 
-  ResourceManagerAdapter(Manager<MODEL_OBJECT, EXCEPTION> manager) {
-    super(manager);
+  ResourceManagerAdapter(Manager<MODEL_OBJECT, EXCEPTION> manager, Class<MODEL_OBJECT> type) {
+    super(manager, type);
   }
 
   /**

scm-webapp/src/main/java/sonia/scm/api/v2/resources/UserCollectionResource.java

@@ -1,13 +1,23 @@
 package sonia.scm.api.v2.resources;
 
-import com.webcohesion.enunciate.metadata.rs.*;
+import com.webcohesion.enunciate.metadata.rs.ResponseCode;
+import com.webcohesion.enunciate.metadata.rs.ResponseHeader;
+import com.webcohesion.enunciate.metadata.rs.ResponseHeaders;
+import com.webcohesion.enunciate.metadata.rs.StatusCodes;
+import com.webcohesion.enunciate.metadata.rs.TypeHint;
 import sonia.scm.user.User;
 import sonia.scm.user.UserException;
 import sonia.scm.user.UserManager;
 import sonia.scm.web.VndMediaType;
 
 import javax.inject.Inject;
-import javax.ws.rs.*;
+import javax.ws.rs.Consumes;
+import javax.ws.rs.DefaultValue;
+import javax.ws.rs.GET;
+import javax.ws.rs.POST;
+import javax.ws.rs.Path;
+import javax.ws.rs.Produces;
+import javax.ws.rs.QueryParam;
 import javax.ws.rs.core.Response;
 import java.io.IOException;
 
@@ -25,7 +35,7 @@ public class UserCollectionResource {
     UserCollectionToDtoMapper userCollectionToDtoMapper, ResourceLinks resourceLinks) {
     this.dtoToUserMapper = dtoToUserMapper;
     this.userCollectionToDtoMapper = userCollectionToDtoMapper;
-    this.adapter = new ResourceManagerAdapter<>(manager);
+    this.adapter = new ResourceManagerAdapter<>(manager, User.class);
     this.resourceLinks = resourceLinks;
   }
 
@@ -33,9 +43,10 @@ public class UserCollectionResource {
    * Returns all users for a given page number with a given page size (default page size is {@value DEFAULT_PAGE_SIZE}).
    *
    * <strong>Note:</strong> This method requires "user" privilege.
-   *  @param page     the number of the requested page
+   *
+   * @param page     the number of the requested page
    * @param pageSize the page size (default page size is {@value DEFAULT_PAGE_SIZE})
-   * @param sortBy   sort parameter
+   * @param sortBy   sort parameter (if empty - undefined sorting)
    * @param desc     sort direction desc or asc
    */
   @GET
@@ -44,6 +55,7 @@ public class UserCollectionResource {
   @TypeHint(UserDto[].class)
   @StatusCodes({
     @ResponseCode(code = 200, condition = "success"),
+    @ResponseCode(code = 400, condition = "\"sortBy\" field unknown"),
     @ResponseCode(code = 401, condition = "not authenticated / invalid credentials"),
     @ResponseCode(code = 403, condition = "not authorized, the current user does not have the \"user\" privilege"),
     @ResponseCode(code = 500, condition = "internal server error")

scm-webapp/src/main/java/sonia/scm/api/v2/resources/UserResource.java

@@ -29,7 +29,7 @@ public class UserResource {
   public UserResource(UserDtoToUserMapper dtoToUserMapper, UserToUserDtoMapper userToDtoMapper, UserManager manager) {
     this.dtoToUserMapper = dtoToUserMapper;
     this.userToDtoMapper = userToDtoMapper;
-    this.adapter = new ResourceManagerAdapter<>(manager);
+    this.adapter = new ResourceManagerAdapter<>(manager, User.class);
   }
 
   /**

scm-webapp/src/test/java/sonia/scm/api/rest/resources/AbstractManagerResourceTest.java

@@ -0,0 +1,122 @@
+package sonia.scm.api.rest.resources;
+
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.ArgumentCaptor;
+import org.mockito.Captor;
+import org.mockito.Mock;
+import org.mockito.runners.MockitoJUnitRunner;
+import sonia.scm.Manager;
+import sonia.scm.ModelObject;
+
+import javax.ws.rs.core.GenericEntity;
+import javax.ws.rs.core.Request;
+import java.util.Collection;
+import java.util.Comparator;
+
+import static java.util.Collections.emptyList;
+import static org.junit.Assert.assertTrue;
+import static org.mockito.Matchers.eq;
+import static org.mockito.Mockito.when;
+
+@RunWith(MockitoJUnitRunner.class)
+public class AbstractManagerResourceTest {
+
+  @Mock
+  private Manager<Simple, Exception> manager;
+  @Mock
+  private Request request;
+  @Captor
+  private ArgumentCaptor<Comparator<Simple>> comparatorCaptor;
+
+  private AbstractManagerResource<Simple, Exception> abstractManagerResource;
+
+  @Before
+  public void captureComparator() {
+    when(manager.getAll(comparatorCaptor.capture(), eq(0), eq(1))).thenReturn(emptyList());
+    abstractManagerResource = new SimpleManagerResource();
+  }
+
+  @Test
+  public void shouldAcceptDefaultSortByParameter() {
+    abstractManagerResource.getAll(request, 0, 1, null, true);
+
+    Comparator<Simple> comparator = comparatorCaptor.getValue();
+    assertTrue(comparator.compare(new Simple("1", null), new Simple("2", null)) > 0);
+  }
+
+  @Test
+  public void shouldAcceptValidSortByParameter() {
+    abstractManagerResource.getAll(request, 0, 1, "data", true);
+
+    Comparator<Simple> comparator = comparatorCaptor.getValue();
+    assertTrue(comparator.compare(new Simple("", "1"), new Simple("", "2")) > 0);
+  }
+
+  @Test(expected = IllegalArgumentException.class)
+  public void shouldFailForIllegalSortByParameter() {
+    abstractManagerResource.getAll(request, 0, 1, "x", true);
+  }
+
+
+  private class SimpleManagerResource extends AbstractManagerResource<Simple, Exception> {
+
+    {
+      disableCache = true;
+    }
+
+    private SimpleManagerResource() {
+      super(AbstractManagerResourceTest.this.manager, Simple.class);
+    }
+
+    @Override
+    protected GenericEntity<Collection<Simple>> createGenericEntity(Collection<Simple> items) {
+      return null;
+    }
+
+    @Override
+    protected String getId(Simple item) {
+      return null;
+    }
+
+    @Override
+    protected String getPathPart() {
+      return null;
+    }
+  }
+
+  public static class Simple implements ModelObject {
+
+    private String id;
+    private String data;
+
+    Simple(String id, String data) {
+      this.id = id;
+      this.data = data;
+    }
+
+    public String getData() {
+      return data;
+    }
+
+    @Override
+    public String getId() {
+      return id;
+    }
+
+    @Override
+    public Long getLastModified() {
+      return null;
+    }
+
+    @Override
+    public String getType() {
+      return null;
+    }
+    @Override
+    public boolean isValid() {
+      return false;
+    }
+  }
+}