Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

x1190 Support "ignoreExternalNames" parameter in block reg post req #387

Merged
merged 1 commit into from
Apr 18, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -50,9 +50,10 @@ public ResponseEntity<?> receiveSectionFile(@RequestParam("file") MultipartFile

@PostMapping(value = "/register/block", produces = MediaType.APPLICATION_JSON_VALUE)
public ResponseEntity<?> receiveBlockFile(@RequestParam("file") MultipartFile file,
@RequestParam(value="existingExternalNames", required=false) List<String> existingExternalNames)
@RequestParam(value="existingExternalNames", required=false) List<String> existingExternalNames,
@RequestParam(value="ignoreExternalNames", required=false) List<String> ignoreExternalNames)
throws URISyntaxException {
BiFunction<User, MultipartFile, RegisterResult> biFunction = (user, multipartFile) -> fileRegisterService.registerBlocks(user, multipartFile, existingExternalNames);
BiFunction<User, MultipartFile, RegisterResult> biFunction = (user, multipartFile) -> fileRegisterService.registerBlocks(user, multipartFile, existingExternalNames, ignoreExternalNames);
return receiveFile("block", file, biFunction);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,8 @@
import uk.ac.sanger.sccp.utils.UCMap;

import java.util.*;
import java.util.stream.Collectors;

import static java.util.Objects.requireNonNull;
import static java.util.stream.Collectors.toList;
import static java.util.stream.Collectors.toSet;

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,19 +32,20 @@ public interface FileRegisterService {
* @exception UncheckedIOException the file cannot be read
*/
default RegisterResult registerBlocks(User user, MultipartFile multipartFile) throws ValidationException, UncheckedIOException {
return registerBlocks(user, multipartFile, null);
return registerBlocks(user, multipartFile, null, null);
}

/**
* Registers blocks as described in the given file.
* @param user the user responsible
* @param multipartFile the file data
* @param existingExternalNames known existing tissue external names to reregister
* @param ignoreExternalNames external names of rows to exclude from the request
* @return the result of the registration
* @exception ValidationException the data received is invalid
* @exception UncheckedIOException the file cannot be read
*/
RegisterResult registerBlocks(User user, MultipartFile multipartFile, List<String> existingExternalNames)
RegisterResult registerBlocks(User user, MultipartFile multipartFile, List<String> existingExternalNames, List<String> ignoreExternalNames)
throws ValidationException, UncheckedIOException;

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,29 +55,33 @@ public FileRegisterServiceImp(IRegisterService<SectionRegisterRequest> sectionRe
* @param service service method to validate and perform the request
* @param existingExternalNames null or a list of tissue external names referenced in the request
* that are known to already exist
* @param ignoreExternalNames null or a list of tissue external names referenced in the request that should be ignored
* @return the result of the registration request
* @param <Req> the type of request to read from the file
* @param <Res> the type of result expected from the request
* @exception ValidationException the request fails validation
* @exception UncheckedIOException there was an IOException reading the file
*/
protected <Req, Res> Res register(User user, MultipartFile multipartFile, MultipartFileReader<Req> fileReader,
BiFunction<User, Req, Res> service, List<String> existingExternalNames)
BiFunction<User, Req, Res> service, List<String> existingExternalNames, List<String> ignoreExternalNames)
throws ValidationException {
Req req;
try {
req = fileReader.read(multipartFile);
} catch (IOException e) {
throw new UncheckedIOException(e);
}
if (!nullOrEmpty(ignoreExternalNames) && req instanceof RegisterRequest) {
updateToRemove((RegisterRequest) req, ignoreExternalNames);
}
if (!nullOrEmpty(existingExternalNames) && req instanceof RegisterRequest) {
updateWithExisting((RegisterRequest) req, existingExternalNames);
}
return transactor.transact("register", () -> service.apply(user, req));
}

/**
* Update the blocks in the given request to specify that they are existing tissue if they
* Updates the blocks in the given request to specify that they are existing tissue if they
* are found in the given list of existing external names.
* @param request a block register request
* @param existingExternalNames list of known existing external names
Expand All @@ -98,19 +102,38 @@ public void updateWithExisting(RegisterRequest request, List<String> existingExt
}
}

/**
* Updates the blocks in the given request to remove those matching the given list of external names.
* @param request block register request
* @param externalNames external names to remove
*/
public void updateToRemove(RegisterRequest request, List<String> externalNames) {
if (nullOrEmpty(externalNames) || nullOrEmpty(request.getBlocks())) {
return;
}
Set<String> ignoreUC = externalNames.stream()
.filter(Objects::nonNull)
.map(String::toUpperCase)
.collect(toSet());
List<BlockRegisterRequest> blocks = request.getBlocks().stream()
.filter(block -> block==null || block.getExternalIdentifier()==null || !ignoreUC.contains(block.getExternalIdentifier().toUpperCase()))
.toList();
request.setBlocks(blocks);
}

@Override
public RegisterResult registerSections(User user, MultipartFile multipartFile) throws ValidationException {
return register(user, multipartFile, sectionFileReader, sectionRegisterService::register, null);
return register(user, multipartFile, sectionFileReader, sectionRegisterService::register, null, null);
}

@Override
public RegisterResult registerBlocks(User user, MultipartFile multipartFile, List<String> existingExternalNames)
throws ValidationException, UncheckedIOException {
return register(user, multipartFile, blockFileReader, blockRegisterService::register, existingExternalNames);
public RegisterResult registerBlocks(User user, MultipartFile multipartFile, List<String> existingExternalNames,
List<String> ignoreExternalNames) throws ValidationException, UncheckedIOException {
return register(user, multipartFile, blockFileReader, blockRegisterService::register, existingExternalNames, ignoreExternalNames);
}

@Override
public RegisterResult registerOriginal(User user, MultipartFile multipartFile) throws ValidationException, UncheckedIOException {
return register(user, multipartFile, originalSampleFileReader, originalSampleRegisterService::register, null);
return register(user, multipartFile, originalSampleFileReader, originalSampleRegisterService::register, null, null);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ public void testClashes() throws Exception {
when(mockRegService.register(any(), any())).thenReturn(RegisterResult.clashes(
List.of(new RegisterClash(tissue1, List.of(lw1)), new RegisterClash(tissue2, List.of(lw2)))
));
var response = upload("testdata/block_reg_existing.xlsx", null, true);
var response = upload("testdata/block_reg_existing.xlsx", null, null, true);
var map = objectMapper.readValue(response.getContentAsString(), Map.class);
Object clashesObj = map.get("clashes");
assertNotNull(clashesObj);
Expand All @@ -115,7 +115,7 @@ public void testExistingExtNames() throws Exception {
User user = creator.createUser("user1");
tester.setUser(user);
when(mockRegService.register(any(), any())).thenThrow(new ValidationException(List.of("Bad reg")));
var response = upload("testdata/block_reg_existing.xlsx", List.of("Ext17"), false);
var response = upload("testdata/block_reg_existing.xlsx", List.of("Ext17"), null, false);
var map = objectMapper.readValue(response.getContentAsString(), Map.class);
ArgumentCaptor<RegisterRequest> requestCaptor = ArgumentCaptor.forClass(RegisterRequest.class);
verify(mockRegService).register(eq(user), requestCaptor.capture());
Expand All @@ -126,18 +126,42 @@ public void testExistingExtNames() throws Exception {
assertEquals("Bad reg", getProblem(map));
}

/**
* This test only goes as far as the service level, but checks that external names to ignore
* are received in the post request and used to filter the register request.
*/
@Test
@Transactional
public void testIgnoreExtNames() throws Exception {
User user = creator.createUser("user1");
tester.setUser(user);
when(mockRegService.register(any(), any())).thenThrow(new ValidationException(List.of("Bad reg")));
var response = upload("testdata/block_reg_existing.xlsx", null, List.of("Ext17"), false);
var map = objectMapper.readValue(response.getContentAsString(), Map.class);
ArgumentCaptor<RegisterRequest> requestCaptor = ArgumentCaptor.forClass(RegisterRequest.class);
verify(mockRegService).register(eq(user), requestCaptor.capture());
RegisterRequest request = requestCaptor.getValue();
assertThat(request.getBlocks()).hasSize(1);
BlockRegisterRequest br = request.getBlocks().getFirst();
assertEquals("EXT18", br.getExternalIdentifier());
assertEquals("Bad reg", getProblem(map));
}

private MockHttpServletResponse upload(String filename) throws Exception {
return upload(filename, null, false);
return upload(filename, null, null, false);
}

private MockHttpServletResponse upload(String filename, List<String> existing, boolean success) throws Exception {
private MockHttpServletResponse upload(String filename, List<String> existing, List<String> ignore, boolean success) throws Exception {
URL url = Resources.getResource(filename);
byte[] bytes = Resources.toByteArray(url);
MockMultipartFile file = new MockMultipartFile("file", bytes);
final MockMultipartHttpServletRequestBuilder rb = multipart("/register/block").file(file);
if (existing!=null) {
rb.param("existingExternalNames", existing.toArray(String[]::new));
}
if (ignore!=null) {
rb.param("ignoreExternalNames", ignore.toArray(String[]::new));
}
MvcResult mvcr = tester.getMockMvc()
.perform(rb)
.andExpect(success ? status().is2xxSuccessful() : status().is4xxClientError())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,8 +63,8 @@ void cleanup() throws Exception {
}

@ParameterizedTest
@MethodSource("regAndExtNamesArgs")
void testSectionRegister_success(Object request, List<String> existingExt) throws IOException {
@MethodSource("regExtNamesIgnoreArgs")
void testSectionRegister_success(Object request, List<String> existingExt, List<String> ignoreExt) throws IOException {
Matchers.mockTransactor(mockTransactor);
IRegisterService<?> regService;
MultipartFileReader<?> fileReader;
Expand All @@ -80,10 +80,10 @@ void testSectionRegister_success(Object request, List<String> existingExt) throw
} else {
regService = mockBlockRegisterService;
fileReader = mockBlockFileReader;
if (existingExt==null) {
if (existingExt==null && ignoreExt==null) {
functionUnderTest = service::registerBlocks;
} else {
functionUnderTest = (user, mpFile) -> service.registerBlocks(user, mpFile, existingExt);
functionUnderTest = (user, mpFile) -> service.registerBlocks(user, mpFile, existingExt, ignoreExt);
}
}
doReturn(request).when(fileReader).read(any(MultipartFile.class));
Expand All @@ -98,17 +98,25 @@ void testSectionRegister_success(Object request, List<String> existingExt) throw
} else {
verify(service, never()).updateWithExisting(any(), any());
}
if (request instanceof RegisterRequest && ignoreExt!=null) {
verify(service).updateToRemove((RegisterRequest) request, ignoreExt);
} else {
verify(service, never()).updateToRemove(any(), any());
}
verify(mockTransactor).transact(any(), any());
}

static Stream<Arguments> regAndExtNamesArgs() {
static Stream<Arguments> regExtNamesIgnoreArgs() {
return Arrays.stream(new Object[][] {
{new SectionRegisterRequest(), null},
{new RegisterRequest(), null},
{new OriginalSampleRegisterRequest(), null},
{new OriginalSampleRegisterRequest(), List.of("Alpha1")},
}).map(Arguments::of);
}
{new SectionRegisterRequest()},
{new OriginalSampleRegisterRequest()},
{new RegisterRequest()},
{new RegisterRequest(), List.of("Alpha1"), null},
{new RegisterRequest(), null, List.of("Beta")},
{new RegisterRequest(), List.of("Alpha1"), List.of("Beta")},
}).map(arr -> arr.length < 3 ? Arrays.copyOf(arr, 3) : arr)
.map(Arguments::of);
}

@ParameterizedTest
@MethodSource("regArgs")
Expand Down Expand Up @@ -151,11 +159,8 @@ static Stream<Arguments> regArgs() {
public void testUpdateWithExisting(boolean any) {
String[] extNames = { null, "Alpha1", "Beta", "Alpha2" };
List<BlockRegisterRequest> blocks = Arrays.stream(extNames)
.map(xn -> {
BlockRegisterRequest b = new BlockRegisterRequest();
b.setExternalIdentifier(xn);
return b;
}).toList();
.map(TestFileRegisterService::blockRegWithExternalName)
.toList();
RegisterRequest request = new RegisterRequest(blocks);
List<String> existing = any ? List.of("ALPHA1", "alpha2") : List.of();

Expand All @@ -164,4 +169,23 @@ public void testUpdateWithExisting(boolean any) {
assertEquals(any && (i==1 || i==3), blocks.get(i).isExistingTissue())
);
}

@ParameterizedTest
@ValueSource(booleans={false,true})
public void testUpdateToRemove(boolean anyToRemove) {
String[] extNames = { null, "Alpha1", "Beta", "Alpha2" };
RegisterRequest request = new RegisterRequest(Arrays.stream(extNames)
.map(TestFileRegisterService::blockRegWithExternalName)
.toList());
List<String> ignore = anyToRemove ? List.of("ALPHA1", "alpha2") : List.of();
service.updateToRemove(request, ignore);
String[] remaining = (anyToRemove ? new String[]{null, "Beta"} : extNames);
assertThat(request.getBlocks().stream().map(BlockRegisterRequest::getExternalIdentifier)).containsExactly(remaining);
}

private static BlockRegisterRequest blockRegWithExternalName(String xn) {
BlockRegisterRequest br = new BlockRegisterRequest();
br.setExternalIdentifier(xn);
return br;
}
}
Loading