SHIBUI-1740

Group controller refactoring
TIER · Jul 3, 2021 · 95a1529 · 95a1529
1 parent a04be8d
commit 95a1529
Show file tree

Hide file tree

Showing 7 changed files with 123 additions and 79 deletions.
diff --git a/...main/java/edu/internet2/tier/shibboleth/admin/ui/security/controller/GroupController.java b/...main/java/edu/internet2/tier/shibboleth/admin/ui/security/controller/GroupController.java
@@ -17,6 +17,9 @@
 import org.springframework.web.servlet.support.ServletUriComponentsBuilder;
 
 import edu.internet2.tier.shibboleth.admin.ui.controller.ErrorResponse;
+import edu.internet2.tier.shibboleth.admin.ui.exception.EntityNotFoundException;
+import edu.internet2.tier.shibboleth.admin.ui.security.exception.GroupDeleteException;
+import edu.internet2.tier.shibboleth.admin.ui.security.exception.GroupExistsConflictException;
 import edu.internet2.tier.shibboleth.admin.ui.security.model.Group;
 import edu.internet2.tier.shibboleth.admin.ui.security.service.IGroupService;
 
@@ -29,41 +32,16 @@ public class GroupController {
     @Secured("ROLE_ADMIN")
     @PostMapping
     @Transactional
-    public ResponseEntity<?> create(@RequestBody Group group) {
-        // If already defined, we can't create a new one, nor will this call update the definition
-        Group foundGroup = groupService.find(group.getResourceId());
-
-        if (foundGroup != null) {
-            HttpHeaders headers = new HttpHeaders();
-            headers.setLocation(ServletUriComponentsBuilder.fromCurrentServletMapping().path("/api/admin/groups").build().toUri());
-
-            return ResponseEntity.status(HttpStatus.METHOD_NOT_ALLOWED).headers(headers)
-                            .body(new ErrorResponse(String.valueOf(HttpStatus.METHOD_NOT_ALLOWED.value()),
-                                            String.format("The group with resource id: [%s] and name: [%s] already exists.",
-                                                            group.getResourceId(), group.getName())));
-        }
-
-        Group result = groupService.createOrUpdateGroup(group);
+    public ResponseEntity<?> create(@RequestBody Group group) throws GroupExistsConflictException {
+        Group result = groupService.createGroup(group);
         return ResponseEntity.status(HttpStatus.CREATED).body(result);
     }
 
     @Secured("ROLE_ADMIN")
     @PutMapping
     @Transactional
-    public ResponseEntity<?> update(@RequestBody Group group) {
-        Group g = groupService.find(group.getResourceId());
-
-        if (g == null) {
-            HttpHeaders headers = new HttpHeaders();
-            headers.setLocation(ServletUriComponentsBuilder.fromCurrentServletMapping().path("/api/admin/groups/{resourceId}").build().toUri());
-
-            return ResponseEntity.status(HttpStatus.NOT_FOUND).headers(headers)
-                            .body(new ErrorResponse(String.valueOf(HttpStatus.NOT_FOUND.value()),
-                                            String.format("Unable to find group with resource id: [%s] and name: [%s]",
-                                                            group.getResourceId(), group.getName())));
-        }
-
-        Group result = groupService.createOrUpdateGroup(group);
+    public ResponseEntity<?> update(@RequestBody Group group) throws EntityNotFoundException {
+        Group result = groupService.updateGroup(group);
         return ResponseEntity.ok(result);
     }
 
@@ -75,44 +53,19 @@ public ResponseEntity<?> getAll() {
 
     @GetMapping("/{resourceId}")
     @Transactional(readOnly = true)
-    public ResponseEntity<?> getOne(@PathVariable String resourceId) {
+    public ResponseEntity<?> getOne(@PathVariable String resourceId) throws EntityNotFoundException {
         Group g = groupService.find(resourceId);
-
         if (g == null) {
-            HttpHeaders headers = new HttpHeaders();
-            headers.setLocation(ServletUriComponentsBuilder.fromCurrentServletMapping().path("/api/admin/groups").build().toUri());
-
-            return ResponseEntity.status(HttpStatus.NOT_FOUND).headers(headers)
-                            .body(new ErrorResponse(String.valueOf(HttpStatus.NOT_FOUND.value()),
-                                            String.format("Unable to find group with resource id: [%s]", resourceId)));
+            throw new EntityNotFoundException(String.format("Unable to find group with resource id: [%s]", resourceId));
         }
         return ResponseEntity.ok(g);
     }
 
     @Secured("ROLE_ADMIN")
     @DeleteMapping("/{resourceId}")
     @Transactional
-    public ResponseEntity<?> delete(@PathVariable String resourceId) {
-        Group g = groupService.find(resourceId);
-
-        if (g == null) {
-            HttpHeaders headers = new HttpHeaders();
-            headers.setLocation(ServletUriComponentsBuilder.fromCurrentServletMapping().path("/api/admin/groups").build().toUri());
-
-            return ResponseEntity.status(HttpStatus.NOT_FOUND).headers(headers)
-                            .body(new ErrorResponse(String.valueOf(HttpStatus.NOT_FOUND.value()),
-                                            String.format("Unable to find group with resource id: [%s]", resourceId)));
-        }
-        if (!g.getUsers().isEmpty() || !g.getEntityDescriptors().isEmpty()) {
-            HttpHeaders headers = new HttpHeaders();
-            headers.setLocation(ServletUriComponentsBuilder.fromCurrentServletMapping().path("/api/admin/groups/{resourceId}").build().toUri());
-
-            return ResponseEntity.status(HttpStatus.CONFLICT).headers(headers)
-                            .body(new ErrorResponse(String.valueOf(HttpStatus.CONFLICT.value()), String.format(
-                                            "Unable to delete group with resource id: [%s] - remove all users and entities from group first",
-                                            resourceId)));
-        }
-        groupService.deleteDefinition(g);
+    public ResponseEntity<?> delete(@PathVariable String resourceId) throws EntityNotFoundException, GroupDeleteException {
+        groupService.deleteDefinition(resourceId);
         return ResponseEntity.noContent().build();
     }
 }
diff --git a/...ternet2/tier/shibboleth/admin/ui/security/controller/GroupControllerExceptionHandler.java b/...ternet2/tier/shibboleth/admin/ui/security/controller/GroupControllerExceptionHandler.java
@@ -0,0 +1,46 @@
+package edu.internet2.tier.shibboleth.admin.ui.security.controller;
+
+import org.springframework.http.HttpHeaders;
+import org.springframework.http.HttpStatus;
+import org.springframework.http.ResponseEntity;
+import org.springframework.web.bind.annotation.ControllerAdvice;
+import org.springframework.web.bind.annotation.ExceptionHandler;
+import org.springframework.web.context.request.WebRequest;
+import org.springframework.web.servlet.mvc.method.annotation.ResponseEntityExceptionHandler;
+import org.springframework.web.servlet.support.ServletUriComponentsBuilder;
+
+import edu.internet2.tier.shibboleth.admin.ui.controller.ErrorResponse;
+import edu.internet2.tier.shibboleth.admin.ui.exception.EntityNotFoundException;
+import edu.internet2.tier.shibboleth.admin.ui.security.exception.GroupDeleteException;
+import edu.internet2.tier.shibboleth.admin.ui.security.exception.GroupExistsConflictException;
+
+@ControllerAdvice(assignableTypes = {GroupController.class})
+public class GroupControllerExceptionHandler extends ResponseEntityExceptionHandler {
+
+    @ExceptionHandler({ EntityNotFoundException.class })
+    public ResponseEntity<?> handleEntityNotFoundException(EntityNotFoundException e, WebRequest request) {
+        HttpHeaders headers = new HttpHeaders();
+        headers.setLocation(ServletUriComponentsBuilder.fromCurrentServletMapping().path("/api/admin/groups").build().toUri());
+
+        return ResponseEntity.status(HttpStatus.NOT_FOUND).headers(headers)
+                        .body(new ErrorResponse(String.valueOf(HttpStatus.NOT_FOUND.value()), e.getMessage()));
+    }
+
+    @ExceptionHandler({ GroupDeleteException.class })
+    public ResponseEntity<?> handleForbiddenAccess(GroupDeleteException e, WebRequest request) {
+        HttpHeaders headers = new HttpHeaders();
+        headers.setLocation(ServletUriComponentsBuilder.fromCurrentServletMapping().path("/api/admin/groups/{resourceId}").build().toUri());
+
+        return ResponseEntity.status(HttpStatus.CONFLICT).headers(headers)
+                        .body(new ErrorResponse(String.valueOf(HttpStatus.CONFLICT.value()), e.getMessage()));
+    }
+
+    @ExceptionHandler({ GroupExistsConflictException.class })
+    public ResponseEntity<?> handleGroupExistsConflict(GroupExistsConflictException e, WebRequest request) {
+        HttpHeaders headers = new HttpHeaders();
+        headers.setLocation(ServletUriComponentsBuilder.fromCurrentServletMapping().path("/api/admin/groups").build().toUri());
+
+        return ResponseEntity.status(HttpStatus.METHOD_NOT_ALLOWED).headers(headers)
+                        .body(new ErrorResponse(String.valueOf(HttpStatus.METHOD_NOT_ALLOWED.value()), e.getMessage()));
+    }
+}
diff --git a/.../java/edu/internet2/tier/shibboleth/admin/ui/security/exception/GroupDeleteException.java b/.../java/edu/internet2/tier/shibboleth/admin/ui/security/exception/GroupDeleteException.java
@@ -0,0 +1,7 @@
+package edu.internet2.tier.shibboleth.admin.ui.security.exception;
+
+public class GroupDeleteException extends Exception {
+    public GroupDeleteException(String message) {
+        super(message);
+    }
+}
diff --git a/...u/internet2/tier/shibboleth/admin/ui/security/exception/GroupExistsConflictException.java b/...u/internet2/tier/shibboleth/admin/ui/security/exception/GroupExistsConflictException.java
@@ -0,0 +1,7 @@
+package edu.internet2.tier.shibboleth.admin.ui.security.exception;
+
+public class GroupExistsConflictException extends Exception {
+    public GroupExistsConflictException(String message) {
+        super(message);
+    }
+}
diff --git a/...c/main/java/edu/internet2/tier/shibboleth/admin/ui/security/service/GroupServiceImpl.java b/...c/main/java/edu/internet2/tier/shibboleth/admin/ui/security/service/GroupServiceImpl.java
@@ -5,6 +5,9 @@
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.stereotype.Service;
 
+import edu.internet2.tier.shibboleth.admin.ui.exception.EntityNotFoundException;
+import edu.internet2.tier.shibboleth.admin.ui.security.exception.GroupDeleteException;
+import edu.internet2.tier.shibboleth.admin.ui.security.exception.GroupExistsConflictException;
 import edu.internet2.tier.shibboleth.admin.ui.security.model.Group;
 import edu.internet2.tier.shibboleth.admin.ui.security.repository.GroupsRepository;
 
@@ -14,13 +17,26 @@ public class GroupServiceImpl implements IGroupService {
     private GroupsRepository repo;
 
     @Override
-    public Group createOrUpdateGroup(Group group) {
+    public Group createGroup(Group group) throws GroupExistsConflictException {
+        Group foundGroup = find(group.getResourceId());
+        // If already defined, we can't create a new one, nor do we want this call update the definition
+        if (foundGroup != null) {
+            throw new GroupExistsConflictException(
+                            String.format("Call update (PUT) to modify the group with resource id: [%s] and name: [%s]",
+                                            foundGroup.getResourceId(), foundGroup.getName()));
+        }
         return repo.save(group);
     }
 
     @Override
-    public void deleteDefinition(Group group) {
-        repo.delete(group);
+    public void deleteDefinition(String resourceId) throws EntityNotFoundException, GroupDeleteException {
+        Group g = find(resourceId);
+        if (!g.getUsers().isEmpty() || !g.getEntityDescriptors().isEmpty()) {
+            throw new GroupDeleteException(String.format(
+                            "Unable to delete group with resource id: [%s] - remove all users and entities from group first",
+                            resourceId));
+        }
+        repo.delete(g);
     }
 
     @Override
@@ -33,4 +49,14 @@ public List<Group> findAll() {
         return repo.findAll();
     }
 
+    @Override
+    public Group updateGroup(Group group) throws EntityNotFoundException {
+        Group g = find(group.getResourceId());
+        if (g == null) {
+            throw new EntityNotFoundException(String.format("Unable to find group with resource id: [%s] and name: [%s]",
+                            group.getResourceId(), group.getName()));
+        }
+        return repo.save(group);
+    }
+
 }
diff --git a/.../src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/service/IGroupService.java b/.../src/main/java/edu/internet2/tier/shibboleth/admin/ui/security/service/IGroupService.java
@@ -2,16 +2,21 @@
 
 import java.util.List;
 
+import edu.internet2.tier.shibboleth.admin.ui.exception.EntityNotFoundException;
+import edu.internet2.tier.shibboleth.admin.ui.security.exception.GroupDeleteException;
+import edu.internet2.tier.shibboleth.admin.ui.security.exception.GroupExistsConflictException;
 import edu.internet2.tier.shibboleth.admin.ui.security.model.Group;
 
 public interface IGroupService {
 
-    Group createOrUpdateGroup(Group g);
+    Group createGroup(Group group) throws GroupExistsConflictException;
 
-    void deleteDefinition(Group g);
+    void deleteDefinition(String resourceId) throws EntityNotFoundException, GroupDeleteException;
 
     Group find(String resourceId);
 
     List<Group> findAll();
 
+    Group updateGroup(Group g) throws EntityNotFoundException;
+
 }
diff --git a/...net2/tier/shibboleth/admin/ui/security/controller/GroupsControllerIntegrationTests.groovy b/...net2/tier/shibboleth/admin/ui/security/controller/GroupsControllerIntegrationTests.groovy
@@ -13,6 +13,9 @@ import org.springframework.test.web.servlet.MockMvc
 import org.springframework.test.web.servlet.result.MockMvcResultHandlers
 import org.springframework.transaction.annotation.Transactional
 
+import edu.internet2.tier.shibboleth.admin.ui.exception.EntityNotFoundException
+import edu.internet2.tier.shibboleth.admin.ui.security.exception.GroupDeleteException
+import edu.internet2.tier.shibboleth.admin.ui.security.exception.GroupExistsConflictException
 import spock.lang.Ignore
 import spock.lang.Specification
 
@@ -68,13 +71,11 @@ class GroupsControllerIntegrationTests extends Specification {
               .andExpect(content().json(expectedJson, false))
 
         when: 'Try to create with an existing resource id'
-        result = mockMvc.perform(post(RESOURCE_URI)
-            .contentType(MediaType.APPLICATION_JSON)
-            .content(JsonOutput.toJson(newGroup))
-            .accept(MediaType.APPLICATION_JSON))
+        def exceptionExpected = mockMvc.perform(post(RESOURCE_URI).contentType(MediaType.APPLICATION_JSON).content(JsonOutput.toJson(newGroup))
+                                       .accept(MediaType.APPLICATION_JSON)).andReturn().getResolvedException()
 
         then: 'Expecting method not allowed'
-        result.andExpect(status().isMethodNotAllowed())
+        exceptionExpected instanceof GroupExistsConflictException == true
     }
 
     @Rollback
@@ -107,13 +108,11 @@ class GroupsControllerIntegrationTests extends Specification {
         def newGroup = [name: 'XXXXX',
             description: 'should not work',
             resourceId: 'XXXX']
-        def notFoundresult = mockMvc.perform(put(RESOURCE_URI)
-            .contentType(MediaType.APPLICATION_JSON)
-            .content(JsonOutput.toJson(newGroup))
-            .accept(MediaType.APPLICATION_JSON))
-
+        def exceptionExpected = mockMvc.perform(put(RESOURCE_URI).contentType(MediaType.APPLICATION_JSON).content(JsonOutput.toJson(newGroup))
+                                    .accept(MediaType.APPLICATION_JSON)).andReturn().getResolvedException()
+
         then: 'Expecting nothing happened because the object was not found'
-        notFoundresult.andExpect(status().isNotFound())
+        exceptionExpected instanceof EntityNotFoundException == true
     }
 
     @WithMockUser(value = "admin", roles = ["ADMIN"])
@@ -147,10 +146,10 @@ class GroupsControllerIntegrationTests extends Specification {
         singleGroupRequest.andExpect(status().isOk())
         
         when: 'GET request for a single non-existent group in a system that has groups'
-        def nonexistentGroupRequest = mockMvc.perform(get("$RESOURCE_URI/CCC"))
+        def exceptionExpected = mockMvc.perform(get("$RESOURCE_URI/CCC")).andReturn().getResolvedException()
         
         then: 'The group not found'
-        nonexistentGroupRequest.andExpect(status().isNotFound())
+        exceptionExpected instanceof EntityNotFoundException == true
     }
       
     @Rollback
@@ -197,9 +196,10 @@ class GroupsControllerIntegrationTests extends Specification {
         when: 'DELETE request is made'
         entityManager.flush()
         entityManager.clear()
-        result = mockMvc.perform(delete("$RESOURCE_URI/$group.resourceId"))
+
+        def exceptionExpected = mockMvc.perform(delete("$RESOURCE_URI/$group.resourceId")).andReturn().getResolvedException()
 
-        then: 'DELETE was not successful'
-        result.andExpect(status().isConflict())
+        then: 'Expecting method not allowed'
+        exceptionExpected instanceof GroupDeleteException == true
     }
 }