From 29ba7bd890f0285e35745eda06453a015d77feb1 Mon Sep 17 00:00:00 2001 From: Dmitriy Kopylenko Date: Fri, 17 Aug 2018 11:13:50 -0400 Subject: [PATCH 1/7] SHIBUI-754: reorder filters work in progress --- ...etadataFiltersPositionOrderController.java | 66 +++++++++++++++++++ 1 file changed, 66 insertions(+) create mode 100644 backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/controller/MetadataFiltersPositionOrderController.java diff --git a/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/controller/MetadataFiltersPositionOrderController.java b/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/controller/MetadataFiltersPositionOrderController.java new file mode 100644 index 000000000..71af40f95 --- /dev/null +++ b/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/controller/MetadataFiltersPositionOrderController.java @@ -0,0 +1,66 @@ +package edu.internet2.tier.shibboleth.admin.ui.controller; + +import edu.internet2.tier.shibboleth.admin.ui.domain.filters.MetadataFilter; +import edu.internet2.tier.shibboleth.admin.ui.domain.resolvers.MetadataResolver; +import edu.internet2.tier.shibboleth.admin.ui.repository.MetadataResolverRepository; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.http.ResponseEntity; +import org.springframework.transaction.annotation.Transactional; +import org.springframework.web.bind.annotation.GetMapping; +import org.springframework.web.bind.annotation.PathVariable; +import org.springframework.web.bind.annotation.PostMapping; +import org.springframework.web.bind.annotation.RequestBody; +import org.springframework.web.bind.annotation.RequestMapping; +import org.springframework.web.bind.annotation.RestController; + +import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; +import java.util.Optional; +import java.util.stream.Collectors; + +import static java.util.stream.Collectors.toList; + +/** + * @author Dmitriy Kopylenko + */ +@RestController +@RequestMapping("/api/MetadataResolvers/{metadataResolverId}/FiltersPositionOrder") +public class MetadataFiltersPositionOrderController { + + @Autowired + MetadataResolverRepository metadataResolverRepository; + + @PostMapping + @Transactional + public ResponseEntity updateFiltersPositionOrder(@PathVariable String metadataResolverId, + @RequestBody List filtersResourceIds) { + + MetadataResolver resolver = metadataResolverRepository.findByResourceId(metadataResolverId); + List currentFilters = resolver.getMetadataFilters(); + List reOrderedFilters = new ArrayList<>(); + + filtersResourceIds.forEach(it -> + currentFilters.stream() + .filter(f -> f.getResourceId().equals(it)) + .findFirst() + .ifPresent(reOrderedFilters::add) + ); + + resolver.setMetadataFilters(reOrderedFilters); + metadataResolverRepository.save(resolver); + + return ResponseEntity.noContent().build(); + } + + @GetMapping + @Transactional(readOnly = true) + public ResponseEntity getFiltersPositionOrder(@PathVariable String metadataResolverId) { + MetadataResolver resolver = metadataResolverRepository.findByResourceId(metadataResolverId); + List resourceIds = resolver.getMetadataFilters().stream() + .map(MetadataFilter::getResourceId) + .collect(toList()); + + return ResponseEntity.ok(resourceIds); + } +} From 1eeff24488f634b2e57c39b0277da98ac2637e59 Mon Sep 17 00:00:00 2001 From: Dmitriy Kopylenko Date: Fri, 17 Aug 2018 11:24:22 -0400 Subject: [PATCH 2/7] SHIBUI-754: wip --- .../MetadataFiltersPositionOrderController.java | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/controller/MetadataFiltersPositionOrderController.java b/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/controller/MetadataFiltersPositionOrderController.java index 71af40f95..2763eae89 100644 --- a/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/controller/MetadataFiltersPositionOrderController.java +++ b/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/controller/MetadataFiltersPositionOrderController.java @@ -47,10 +47,14 @@ public ResponseEntity updateFiltersPositionOrder(@PathVariable String metadat .ifPresent(reOrderedFilters::add) ); - resolver.setMetadataFilters(reOrderedFilters); - metadataResolverRepository.save(resolver); - - return ResponseEntity.noContent().build(); + if(currentFilters.size() == reOrderedFilters.size()) { + resolver.setMetadataFilters(reOrderedFilters); + metadataResolverRepository.save(resolver); + return ResponseEntity.noContent().build(); + } + return ResponseEntity + .badRequest() + .body("Number of filters to reorder or filters resource ids do not match current filters"); } @GetMapping From c9520b9eeccc84814d50982ea068c01d4bb4ca7c Mon Sep 17 00:00:00 2001 From: Dmitriy Kopylenko Date: Fri, 17 Aug 2018 14:24:01 -0400 Subject: [PATCH 3/7] SHIBUI-754: filters re-order work in progress --- .../controller/MetadataFiltersController.java | 3 ++ ...etadataFiltersPositionOrderController.java | 9 ++++- .../support/RestControllersSupport.java | 40 +++++++++++++++++++ 3 files changed, 50 insertions(+), 2 deletions(-) create mode 100644 backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/controller/support/RestControllersSupport.java diff --git a/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/controller/MetadataFiltersController.java b/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/controller/MetadataFiltersController.java index 2ec5f01ab..2b4cceb2c 100644 --- a/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/controller/MetadataFiltersController.java +++ b/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/controller/MetadataFiltersController.java @@ -48,6 +48,7 @@ public class MetadataFiltersController { private static final Supplier 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) { @@ -100,6 +101,7 @@ 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); } @@ -143,6 +145,7 @@ 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) { diff --git a/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/controller/MetadataFiltersPositionOrderController.java b/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/controller/MetadataFiltersPositionOrderController.java index 2763eae89..bc3ccbffd 100644 --- a/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/controller/MetadataFiltersPositionOrderController.java +++ b/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/controller/MetadataFiltersPositionOrderController.java @@ -1,5 +1,6 @@ package edu.internet2.tier.shibboleth.admin.ui.controller; +import edu.internet2.tier.shibboleth.admin.ui.controller.support.RestControllersSupport; import edu.internet2.tier.shibboleth.admin.ui.domain.filters.MetadataFilter; import edu.internet2.tier.shibboleth.admin.ui.domain.resolvers.MetadataResolver; import edu.internet2.tier.shibboleth.admin.ui.repository.MetadataResolverRepository; @@ -12,6 +13,7 @@ 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; @@ -31,12 +33,15 @@ public class MetadataFiltersPositionOrderController { @Autowired MetadataResolverRepository metadataResolverRepository; + @Autowired + RestControllersSupport restControllersSupport; + @PostMapping @Transactional public ResponseEntity updateFiltersPositionOrder(@PathVariable String metadataResolverId, @RequestBody List filtersResourceIds) { - MetadataResolver resolver = metadataResolverRepository.findByResourceId(metadataResolverId); + MetadataResolver resolver = restControllersSupport.findResolverOrThrowHttp404(metadataResolverId); List currentFilters = resolver.getMetadataFilters(); List reOrderedFilters = new ArrayList<>(); @@ -60,7 +65,7 @@ public ResponseEntity updateFiltersPositionOrder(@PathVariable String metadat @GetMapping @Transactional(readOnly = true) public ResponseEntity getFiltersPositionOrder(@PathVariable String metadataResolverId) { - MetadataResolver resolver = metadataResolverRepository.findByResourceId(metadataResolverId); + MetadataResolver resolver = restControllersSupport.findResolverOrThrowHttp404(metadataResolverId); List resourceIds = resolver.getMetadataFilters().stream() .map(MetadataFilter::getResourceId) .collect(toList()); diff --git a/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/controller/support/RestControllersSupport.java b/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/controller/support/RestControllersSupport.java new file mode 100644 index 000000000..764ad58a0 --- /dev/null +++ b/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/controller/support/RestControllersSupport.java @@ -0,0 +1,40 @@ +package edu.internet2.tier.shibboleth.admin.ui.controller.support; + +import edu.internet2.tier.shibboleth.admin.ui.domain.resolvers.MetadataResolver; +import edu.internet2.tier.shibboleth.admin.ui.repository.MetadataResolverRepository; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.http.ResponseEntity; +import org.springframework.web.bind.annotation.ExceptionHandler; +import org.springframework.web.bind.annotation.RestControllerAdvice; +import org.springframework.web.client.HttpClientErrorException; + +import static org.springframework.http.HttpStatus.NOT_FOUND; + +/** + * Common functionality for REST controllers. + * + * @author Dmitriy Kopylenko + */ +@RestControllerAdvice +public class RestControllersSupport { + + @Autowired + MetadataResolverRepository resolverRepository; + + public MetadataResolver findResolverOrThrowHttp404(String resolverResourceId) { + MetadataResolver resolver = resolverRepository.findByResourceId(resolverResourceId); + if(resolver == null) { + throw new HttpClientErrorException(NOT_FOUND, "Metadata resolver is not found"); + } + return resolver; + } + + + @ExceptionHandler + public ResponseEntity notFoundHandler(HttpClientErrorException ex) { + if(ex.getStatusCode() == NOT_FOUND) { + return ResponseEntity.status(NOT_FOUND).body(ex.getStatusText()); + } + throw ex; + } +} From 6d4d9b5c09d0b5cc97135f50c4328f3427d1c67d Mon Sep 17 00:00:00 2001 From: Dmitriy Kopylenko Date: Mon, 20 Aug 2018 15:10:32 -0400 Subject: [PATCH 4/7] SHIBUI-754: Integration tests --- backend/build.gradle | 2 + ...tionOrderControllerIntegrationTests.groovy | 147 ++++++++++++++++++ 2 files changed, 149 insertions(+) create mode 100644 backend/src/test/groovy/edu/internet2/tier/shibboleth/admin/ui/controller/MetadataFiltersPositionOrderControllerIntegrationTests.groovy diff --git a/backend/build.gradle b/backend/build.gradle index ca484d6cb..6092b958c 100644 --- a/backend/build.gradle +++ b/backend/build.gradle @@ -108,6 +108,8 @@ dependencies { testCompile "net.shibboleth.ext:spring-extensions:5.4.0-SNAPSHOT" + testCompile "com.github.groovy-wslite:groovy-wslite:1.1.3" + //JSON schema generator testCompile 'com.kjetland:mbknor-jackson-jsonschema_2.12:1.0.29' testCompile 'javax.validation:validation-api:2.0.1.Final' diff --git a/backend/src/test/groovy/edu/internet2/tier/shibboleth/admin/ui/controller/MetadataFiltersPositionOrderControllerIntegrationTests.groovy b/backend/src/test/groovy/edu/internet2/tier/shibboleth/admin/ui/controller/MetadataFiltersPositionOrderControllerIntegrationTests.groovy new file mode 100644 index 000000000..da525e35b --- /dev/null +++ b/backend/src/test/groovy/edu/internet2/tier/shibboleth/admin/ui/controller/MetadataFiltersPositionOrderControllerIntegrationTests.groovy @@ -0,0 +1,147 @@ +package edu.internet2.tier.shibboleth.admin.ui.controller + +import edu.internet2.tier.shibboleth.admin.ui.repository.MetadataResolverRepository +import edu.internet2.tier.shibboleth.admin.ui.util.TestObjectGenerator +import edu.internet2.tier.shibboleth.admin.util.AttributeUtility + +import org.springframework.beans.factory.annotation.Autowired +import org.springframework.boot.test.context.SpringBootTest +import org.springframework.boot.test.web.client.TestRestTemplate +import org.springframework.test.context.ActiveProfiles + +import spock.lang.Specification + + +/** + * @author Dmitriy Kopylenko + */ +@SpringBootTest(webEnvironment = SpringBootTest.WebEnvironment.RANDOM_PORT) +@ActiveProfiles("no-auth") +class MetadataFiltersPositionOrderControllerIntegrationTests extends Specification { + + @Autowired + private TestRestTemplate restTemplate + + @Autowired + MetadataResolverRepository metadataResolverRepository + + @Autowired + AttributeUtility attributeUtility + + TestObjectGenerator generator + + static BASE_URI = '/api/MetadataResolvers' + + static RESOURCE_URI = "$BASE_URI/%s/FiltersPositionOrder" + + def setup() { + generator = new TestObjectGenerator(attributeUtility) + } + + def cleanup() { + metadataResolverRepository.deleteAll() + } + + def "GET Filter Position Order for non-existent resolver"() { + when: 'GET request is made with resolver resource id NOT matching any existing filter' + def result = this.restTemplate.getForEntity(resourceUriFor('non-existent-resolver-id'), String) + + then: "Request completed successfully" + result.statusCodeValue == 404 + } + + def "GET Default Filter Position Order for filters originally attached to a resolver"() { + given: 'MetadataResolver with 2 filters in data store' + def resolver = createResolverWithTwoFilters() + + when: 'GET request is made to retrieve position order of filters' + def result = getFiltersPositionOrderFor(resolver.resourceId) + + then: 'Original filters order is preserved' + result.statusCodeValue == 200 + result.body == [resolver.firstFilterResourceId, resolver.secondFilterResourceId] + } + + def "Reorder filters and verify the position within resolver persisted accordingly"() { + given: 'MetadataResolver with 2 filters in data store' + def resolver = createResolverWithTwoFilters() + def reOrderedFiltersPosition = [resolver.secondFilterResourceId, resolver.firstFilterResourceId] + + when: 'POST is made to re-order filters position' + def reorderPOSTResult = reorderFilters(resolver.resourceId, reOrderedFiltersPosition) + + then: 'Request completed successfully' + reorderPOSTResult.statusCodeValue == 204 + + and: 'GET request is made to retrieve position order of filters' + def positionOrderResult = getFiltersPositionOrderFor(resolver.resourceId) + + then: + positionOrderResult.body == reOrderedFiltersPosition + + and: "Request is made to retrieve the resolver with affected filters" + def resolverResult = this.restTemplate.getForEntity("$BASE_URI/$resolver.resourceId", Map) + + then: + resolverResult.statusCodeValue == 200 + resolverResult.body.metadataFilters.collect {it.resourceId} == reOrderedFiltersPosition + } + + def "Reorder filters with bad data"() { + given: 'MetadataResolver with 2 filters in data store' + def resolver = createResolverWithTwoFilters() + def originalFiltersPosition = [resolver.firstFilterResourceId, resolver.secondFilterResourceId] + //Only one filter in order position data, while there are two filters + def reOrderedFiltersPosition = [resolver.secondFilterResourceId] + + when: 'POST is made to re-order filters position with invalid number of filters to re-order' + def reorderPOSTResult = reorderFilters(resolver.resourceId, reOrderedFiltersPosition) + + then: 'Request completed successfully with 400' + reorderPOSTResult.statusCodeValue == 400 + + and: 'GET request is made to retrieve position order of filters' + def positionOrderResult = getFiltersPositionOrderFor(resolver.resourceId) + + then: 'Original filters position order is retrieved' + positionOrderResult.body == originalFiltersPosition + + and: "Request is made to retrieve the resolver with original filters" + def resolverResult = this.restTemplate.getForEntity("$BASE_URI/$resolver.resourceId", Map) + + then: + resolverResult.statusCodeValue == 200 + 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']) + + then: 'Request completed successfully with 400' + reorderPOSTResult_2.statusCodeValue == 400 + } + + private createResolverWithTwoFilters() { + def resolver = generator.buildRandomMetadataResolverOfType('FileBacked') + resolver.metadataFilters = [generator.signatureValidationFilter(), generator.entityRoleWhitelistFilter()] + def resolverResourceId = resolver.resourceId + def firstFilterResourceId = resolver.metadataFilters[0].resourceId + def secondFilterResourceId = resolver.metadataFilters[1].resourceId + metadataResolverRepository.save(resolver) + + [resourceId : resolverResourceId, + firstFilterResourceId : firstFilterResourceId, + secondFilterResourceId: secondFilterResourceId] + } + + private getFiltersPositionOrderFor(String resourceId) { + this.restTemplate.getForEntity(resourceUriFor(resourceId), List) + } + + private reorderFilters(String resourceId, List filterIdsPositionOrderList) { + this.restTemplate.postForEntity(resourceUriFor(resourceId), filterIdsPositionOrderList, null) + } + + private static resourceUriFor(String resolverResourceId) { + String.format(RESOURCE_URI, resolverResourceId) + } +} \ No newline at end of file From 7f3f995616ac37e8a92486efc292c77e4fdd187b Mon Sep 17 00:00:00 2001 From: Dmitriy Kopylenko Date: Mon, 20 Aug 2018 15:16:32 -0400 Subject: [PATCH 5/7] Cleanup --- backend/build.gradle | 2 -- 1 file changed, 2 deletions(-) diff --git a/backend/build.gradle b/backend/build.gradle index 6092b958c..ca484d6cb 100644 --- a/backend/build.gradle +++ b/backend/build.gradle @@ -108,8 +108,6 @@ dependencies { testCompile "net.shibboleth.ext:spring-extensions:5.4.0-SNAPSHOT" - testCompile "com.github.groovy-wslite:groovy-wslite:1.1.3" - //JSON schema generator testCompile 'com.kjetland:mbknor-jackson-jsonschema_2.12:1.0.29' testCompile 'javax.validation:validation-api:2.0.1.Final' From 97705469ff1ded2c1d7e59773554f130c3797881 Mon Sep 17 00:00:00 2001 From: Dmitriy Kopylenko Date: Mon, 20 Aug 2018 15:52:42 -0400 Subject: [PATCH 6/7] SHIBUI-754: tests polishing --- .../support/RestControllersSupport.java | 1 + ...tionOrderControllerIntegrationTests.groovy | 20 +++++++++++-------- 2 files changed, 13 insertions(+), 8 deletions(-) diff --git a/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/controller/support/RestControllersSupport.java b/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/controller/support/RestControllersSupport.java index 764ad58a0..1605b86dd 100644 --- a/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/controller/support/RestControllersSupport.java +++ b/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/controller/support/RestControllersSupport.java @@ -1,5 +1,6 @@ package edu.internet2.tier.shibboleth.admin.ui.controller.support; +import com.google.common.collect.ImmutableMap; import edu.internet2.tier.shibboleth.admin.ui.domain.resolvers.MetadataResolver; import edu.internet2.tier.shibboleth.admin.ui.repository.MetadataResolverRepository; import org.springframework.beans.factory.annotation.Autowired; diff --git a/backend/src/test/groovy/edu/internet2/tier/shibboleth/admin/ui/controller/MetadataFiltersPositionOrderControllerIntegrationTests.groovy b/backend/src/test/groovy/edu/internet2/tier/shibboleth/admin/ui/controller/MetadataFiltersPositionOrderControllerIntegrationTests.groovy index da525e35b..c1718a0fd 100644 --- a/backend/src/test/groovy/edu/internet2/tier/shibboleth/admin/ui/controller/MetadataFiltersPositionOrderControllerIntegrationTests.groovy +++ b/backend/src/test/groovy/edu/internet2/tier/shibboleth/admin/ui/controller/MetadataFiltersPositionOrderControllerIntegrationTests.groovy @@ -44,7 +44,7 @@ class MetadataFiltersPositionOrderControllerIntegrationTests extends Specificati def "GET Filter Position Order for non-existent resolver"() { when: 'GET request is made with resolver resource id NOT matching any existing filter' - def result = this.restTemplate.getForEntity(resourceUriFor('non-existent-resolver-id'), String) + def result = getFiltersPositionOrderFor('non-existent-resolver-id', String) then: "Request completed successfully" result.statusCodeValue == 404 @@ -55,7 +55,7 @@ class MetadataFiltersPositionOrderControllerIntegrationTests extends Specificati def resolver = createResolverWithTwoFilters() when: 'GET request is made to retrieve position order of filters' - def result = getFiltersPositionOrderFor(resolver.resourceId) + def result = getFiltersPositionOrderFor(resolver.resourceId, List) then: 'Original filters order is preserved' result.statusCodeValue == 200 @@ -74,13 +74,13 @@ class MetadataFiltersPositionOrderControllerIntegrationTests extends Specificati reorderPOSTResult.statusCodeValue == 204 and: 'GET request is made to retrieve position order of filters' - def positionOrderResult = getFiltersPositionOrderFor(resolver.resourceId) + def positionOrderResult = getFiltersPositionOrderFor(resolver.resourceId, List) then: positionOrderResult.body == reOrderedFiltersPosition and: "Request is made to retrieve the resolver with affected filters" - def resolverResult = this.restTemplate.getForEntity("$BASE_URI/$resolver.resourceId", Map) + def resolverResult = getResolver(resolver.resourceId) then: resolverResult.statusCodeValue == 200 @@ -101,13 +101,13 @@ class MetadataFiltersPositionOrderControllerIntegrationTests extends Specificati reorderPOSTResult.statusCodeValue == 400 and: 'GET request is made to retrieve position order of filters' - def positionOrderResult = getFiltersPositionOrderFor(resolver.resourceId) + def positionOrderResult = getFiltersPositionOrderFor(resolver.resourceId, List) then: 'Original filters position order is retrieved' positionOrderResult.body == originalFiltersPosition and: "Request is made to retrieve the resolver with original filters" - def resolverResult = this.restTemplate.getForEntity("$BASE_URI/$resolver.resourceId", Map) + def resolverResult = getResolver(resolver.resourceId) then: resolverResult.statusCodeValue == 200 @@ -133,14 +133,18 @@ class MetadataFiltersPositionOrderControllerIntegrationTests extends Specificati secondFilterResourceId: secondFilterResourceId] } - private getFiltersPositionOrderFor(String resourceId) { - this.restTemplate.getForEntity(resourceUriFor(resourceId), List) + private getFiltersPositionOrderFor(String resourceId, responseType) { + this.restTemplate.getForEntity(resourceUriFor(resourceId), responseType) } private reorderFilters(String resourceId, List filterIdsPositionOrderList) { this.restTemplate.postForEntity(resourceUriFor(resourceId), filterIdsPositionOrderList, null) } + private getResolver(String resolverResourceId) { + this.restTemplate.getForEntity("$BASE_URI/$resolverResourceId", Object) + } + private static resourceUriFor(String resolverResourceId) { String.format(RESOURCE_URI, resolverResourceId) } From 59ae189186bfdd3fbe7ada483e6cf57cad2615e9 Mon Sep 17 00:00:00 2001 From: Dmitriy Kopylenko Date: Tue, 21 Aug 2018 10:58:50 -0400 Subject: [PATCH 7/7] SHIBUI-754: Refactoring based on code review feedback --- .../controller/MetadataFiltersController.java | 3 -- ...etadataFiltersPositionOrderController.java | 46 +++++++++++-------- ...tionOrderControllerIntegrationTests.groovy | 2 +- 3 files changed, 28 insertions(+), 23 deletions(-) diff --git a/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/controller/MetadataFiltersController.java b/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/controller/MetadataFiltersController.java index 2b4cceb2c..2ec5f01ab 100644 --- a/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/controller/MetadataFiltersController.java +++ b/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/controller/MetadataFiltersController.java @@ -48,7 +48,6 @@ public class MetadataFiltersController { private static final Supplier 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) { diff --git a/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/controller/MetadataFiltersPositionOrderController.java b/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/controller/MetadataFiltersPositionOrderController.java index bc3ccbffd..4c7d70ec0 100644 --- a/backend/src/main/java/edu/internet2/tier/shibboleth/admin/ui/controller/MetadataFiltersPositionOrderController.java +++ b/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 currentFilters = resolver.getMetadataFilters(); - List 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 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 diff --git a/backend/src/test/groovy/edu/internet2/tier/shibboleth/admin/ui/controller/MetadataFiltersPositionOrderControllerIntegrationTests.groovy b/backend/src/test/groovy/edu/internet2/tier/shibboleth/admin/ui/controller/MetadataFiltersPositionOrderControllerIntegrationTests.groovy index c1718a0fd..4d163a660 100644 --- a/backend/src/test/groovy/edu/internet2/tier/shibboleth/admin/ui/controller/MetadataFiltersPositionOrderControllerIntegrationTests.groovy +++ b/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