SHIBUI-754: Refactoring based on code review feedback

backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/controller/MetadataFiltersController.java

@@ -48,7 +48,6 @@ public class MetadataFiltersController {
 
     private static final Supplier<HttpClientErrorException> HTTP_404_CLIENT_ERROR_EXCEPTION = () -> new HttpClientErrorException(NOT_FOUND);
 
-    //TODO: refactor to use RestControllerSupport class
     @ExceptionHandler
     public ResponseEntity<?> notFoundHandler(HttpClientErrorException ex) {
         if(ex.getStatusCode() == NOT_FOUND) {
@@ -101,7 +100,6 @@ public ResponseEntity<?> update(@PathVariable String metadataResolverId,
         // check to make sure that the relationship exists
         if (!metadataResolver.getMetadataFilters().contains(filterTobeUpdated)) {
             // TODO: find a better response
-            // TODO: refactor to use RestControllerSupport class
             return new ResponseEntity<>(HttpStatus.BAD_REQUEST);
         }
 
@@ -145,7 +143,6 @@ public ResponseEntity<?> delete(@PathVariable String metadataResolverId,
         return ResponseEntity.noContent().build();
     }
 
-    //TODO: refactor to use RestControllerSupport class
     private MetadataResolver findResolverOrThrowHttp404(String resolverResourceId) {
         MetadataResolver resolver = repository.findByResourceId(resolverResourceId);
         if(resolver == null) {

backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/controller/MetadataFiltersPositionOrderController.java

@@ -13,12 +13,10 @@
 import org.springframework.web.bind.annotation.RequestBody;
 import org.springframework.web.bind.annotation.RequestMapping;
 import org.springframework.web.bind.annotation.RestController;
-import org.springframework.web.bind.annotation.RestControllerAdvice;
 
 import java.util.ArrayList;
-import java.util.Arrays;
+import java.util.Comparator;
 import java.util.List;
-import java.util.Optional;
 import java.util.stream.Collectors;
 
 import static java.util.stream.Collectors.toList;
@@ -43,23 +41,33 @@ public ResponseEntity<?> updateFiltersPositionOrder(@PathVariable String metadat
 
         MetadataResolver resolver = restControllersSupport.findResolverOrThrowHttp404(metadataResolverId);
         List<MetadataFilter> currentFilters = resolver.getMetadataFilters();
-        List<MetadataFilter> reOrderedFilters = new ArrayList<>();
-
-        filtersResourceIds.forEach(it ->
-                currentFilters.stream()
-                        .filter(f -> f.getResourceId().equals(it))
-                        .findFirst()
-                        .ifPresent(reOrderedFilters::add)
-        );
-
-        if(currentFilters.size() == reOrderedFilters.size()) {
-            resolver.setMetadataFilters(reOrderedFilters);
-            metadataResolverRepository.save(resolver);
-            return ResponseEntity.noContent().build();
+
+        //Check for bad data upfront. We could avoid this check and take wrong size and/or filter ids and blindly pass to sort below.
+        //In that case, the sort operation will silently NOT do anything and leave original filters order,
+        //but we will not be able to indicate to calling clients HTTP 400 in that case.
+        if ((filtersResourceIds.size() != currentFilters.size()) ||
+                (!currentFilters.stream()
+                        .map(MetadataFilter::getResourceId)
+                        .collect(toList())
+                        .containsAll(filtersResourceIds))) {
+
+            return ResponseEntity
+                    .badRequest()
+                    .body("Number of filters to reorder or filters resource ids do not match current filters");
         }
-        return ResponseEntity
-                .badRequest()
-                .body("Number of filters to reorder or filters resource ids do not match current filters");
+
+        //This is needed in order to set reference to persistent filters collection to be able to merge the persistent collection
+        //Otherwise if we manipulate the original collection directly and try to save, we'll get RDBMS constraint violation exception
+        List<MetadataFilter> reOrderedFilters = new ArrayList<>(currentFilters);
+
+        //Main re-ordering operation
+        reOrderedFilters.sort(Comparator.comparingInt(f -> filtersResourceIds.indexOf(f.getResourceId())));
+
+        //re-set the reference and save to DB
+        resolver.setMetadataFilters(reOrderedFilters);
+        metadataResolverRepository.save(resolver);
+
+        return ResponseEntity.noContent().build();
     }
 
     @GetMapping

backend/src/test/groovy/edu/internet2/tier/shibboleth/admin/ui/controller/MetadataFiltersPositionOrderControllerIntegrationTests.groovy

@@ -114,7 +114,7 @@ class MetadataFiltersPositionOrderControllerIntegrationTests extends Specificati
         resolverResult.body.metadataFilters.collect {it.resourceId} == originalFiltersPosition
 
         and: 'POST is made to re-order filters position with invalid resource ids'
-        def reorderPOSTResult_2 = reorderFilters(resolver.resourceId, ['invalid', 'resource ids'])
+        def reorderPOSTResult_2 = reorderFilters(resolver.resourceId, [resolver.secondFilterResourceId, 'invalid-id'])
 
         then: 'Request completed successfully with 400'
         reorderPOSTResult_2.statusCodeValue == 400