diff --git a/src/main/java/org/phoebus/channelfinder/repository/ChannelRepository.java b/src/main/java/org/phoebus/channelfinder/repository/ChannelRepository.java index 29689d2a..d320746a 100644 --- a/src/main/java/org/phoebus/channelfinder/repository/ChannelRepository.java +++ b/src/main/java/org/phoebus/channelfinder/repository/ChannelRepository.java @@ -293,7 +293,7 @@ public Iterable saveAll(Iterable channels) { } BulkResponse result; try { - result = client.bulk(br.refresh(Refresh.True).build()); + result = client.bulk(br.refresh(Refresh.WaitFor).build()); // Log errors, if any if (result.errors()) { logger.log(Level.SEVERE, TextUtil.BULK_HAD_ERRORS); @@ -427,17 +427,29 @@ public Iterable findAll() { @Override public List findAllById(Iterable channelIds) { try { - List ids = - StreamSupport.stream(channelIds.spliterator(), false).collect(Collectors.toList()); + List ids = normalizeIds(channelIds); - SearchRequest.Builder searchBuilder = - new SearchRequest.Builder() - .index(esService.getES_CHANNEL_INDEX()) - .query(IdsQuery.of(q -> q.values(ids))._toQuery()) - .size(esService.getES_QUERY_SIZE()) - .sort(SortOptions.of(s -> s.field(FieldSort.of(f -> f.field("name"))))); - SearchResponse response = client.search(searchBuilder.build(), Channel.class); - return response.hits().hits().stream().map(Hit::source).collect(Collectors.toList()); + if (ids.isEmpty()) { + return Collections.emptyList(); + } + + int lookupBatchSize = Math.max(1, Math.min(chunkSize, esService.getES_QUERY_SIZE())); + List result = new ArrayList<>(); + + for (int i = 0; i < ids.size(); i += lookupBatchSize) { + List chunk = ids.subList(i, Math.min(i + lookupBatchSize, ids.size())); + SearchRequest.Builder searchBuilder = + new SearchRequest.Builder() + .index(esService.getES_CHANNEL_INDEX()) + .query(IdsQuery.of(q -> q.values(chunk))._toQuery()) + .size(chunk.size()) + .sort(SortOptions.of(s -> s.field(FieldSort.of(f -> f.field("name"))))); + SearchResponse response = client.search(searchBuilder.build(), Channel.class); + result.addAll( + response.hits().hits().stream().map(Hit::source).collect(Collectors.toList())); + } + + return result; } catch (ElasticsearchException | IOException e) { logger.log(Level.SEVERE, TextUtil.FAILED_TO_FIND_ALL_CHANNELS, e); throw new ResponseStatusException( @@ -805,9 +817,70 @@ public Scroll scroll(String scrollId, MultiValueMap searchParame } @Override + @SuppressWarnings("unchecked") public void deleteAllById(Iterable ids) { - // TODO Auto-generated method stub + deleteAllByIdBestEffort((Iterable) ids); + } + + public long deleteAllByIdBestEffort(Iterable ids) { + List idList = normalizeIds(ids); + if (idList.isEmpty()) { + return 0; + } + + long deletedCount = 0; + + for (int i = 0; i < idList.size(); i += chunkSize) { + List chunk = idList.subList(i, Math.min(i + chunkSize, idList.size())); + BulkRequest.Builder br = new BulkRequest.Builder(); + for (String id : chunk) { + br.operations(op -> op.delete(del -> del.index(esService.getES_CHANNEL_INDEX()).id(id))); + } + br.refresh(Refresh.True); + + try { + BulkResponse result = client.bulk(br.build()); + for (BulkResponseItem item : result.items()) { + if (item.error() != null) { + logger.log( + Level.SEVERE, + () -> + MessageFormat.format( + "Failed to delete channel id {0}: {1}", item.id(), item.error().reason())); + continue; + } + if (Integer.valueOf(200).equals(item.status())) { + deletedCount++; + } + } + } catch (IOException e) { + logger.log( + Level.SEVERE, + MessageFormat.format( + "Bulk delete failed for chunk starting at index {0} with size {1}", + i, chunk.size()), + e); + } + } + + return deletedCount; + } + + /** + * Normalizes channel IDs by dropping null/blank values and removing duplicates while preserving + * encounter order. + * + * @param ids raw channel IDs from request/repository callers + * @return distinct, non-blank channel IDs in encounter order + */ + private static List normalizeIds(Iterable ids) { + // TODO: Consider rejecting blank/whitespace-only IDs with 400 at the API boundary. + return StreamSupport.stream(ids.spliterator(), false) + .filter(id -> id != null && !id.isBlank()) + .collect(Collectors.toCollection(LinkedHashSet::new)) + .stream() + .toList(); } @PreDestroy diff --git a/src/main/java/org/phoebus/channelfinder/service/ChannelService.java b/src/main/java/org/phoebus/channelfinder/service/ChannelService.java index c3b874fc..79458016 100644 --- a/src/main/java/org/phoebus/channelfinder/service/ChannelService.java +++ b/src/main/java/org/phoebus/channelfinder/service/ChannelService.java @@ -179,6 +179,24 @@ public void remove(String channelName) { channelRepository.deleteById(channelName); } + public long remove(Iterable channelNames) { + requireRole(ROLES.CF_CHANNEL, "channels batch"); + List existingChannels = channelRepository.findAllById(channelNames); + + for (Channel existing : existingChannels) { + requireOwner(existing); + audit.log( + Level.INFO, () -> MessageFormat.format(TextUtil.DELETE_CHANNEL, existing.getName())); + } + + if (existingChannels.isEmpty()) { + return 0; + } + + return channelRepository.deleteAllByIdBestEffort( + existingChannels.stream().map(Channel::getName).toList()); + } + private Map findExistingChannels(List channels) { return channelRepository.findAllById(channels.stream().map(Channel::getName).toList()).stream() .collect(Collectors.toMap(Channel::getName, c -> c)); diff --git a/src/main/java/org/phoebus/channelfinder/web/v0/api/IChannel.java b/src/main/java/org/phoebus/channelfinder/web/v0/api/IChannel.java index 4295d710..0fa494b3 100644 --- a/src/main/java/org/phoebus/channelfinder/web/v0/api/IChannel.java +++ b/src/main/java/org/phoebus/channelfinder/web/v0/api/IChannel.java @@ -269,4 +269,24 @@ long queryCount( }) @DeleteMapping("/{channelName}") void remove(@PathVariable("channelName") String channelName); + + @Operation( + summary = "Delete multiple channels", + description = "Delete multiple channel instances identified by a request-body list of names.", + operationId = "deleteChannels", + tags = {"Channel"}) + @ApiResponses( + value = { + @ApiResponse(responseCode = "200", description = "Number of channels deleted"), + @ApiResponse( + responseCode = "401", + description = "Unauthorized", + content = @Content(schema = @Schema(implementation = ResponseStatusException.class))), + @ApiResponse( + responseCode = "500", + description = "Error while trying to delete channels", + content = @Content(schema = @Schema(implementation = ResponseStatusException.class))) + }) + @DeleteMapping + long remove(@RequestBody List channelNames); } diff --git a/src/main/java/org/phoebus/channelfinder/web/v0/controller/ChannelController.java b/src/main/java/org/phoebus/channelfinder/web/v0/controller/ChannelController.java index ab094ad1..3fa52a00 100644 --- a/src/main/java/org/phoebus/channelfinder/web/v0/controller/ChannelController.java +++ b/src/main/java/org/phoebus/channelfinder/web/v0/controller/ChannelController.java @@ -65,4 +65,9 @@ public Iterable update(Iterable channels) { public void remove(String channelName) { channelService.remove(channelName); } + + @Override + public long remove(List channelNames) { + return channelService.remove(channelNames); + } } diff --git a/src/test/java/org/phoebus/channelfinder/ChannelRepositoryIT.java b/src/test/java/org/phoebus/channelfinder/ChannelRepositoryIT.java index aa5d9350..d7d04ef3 100644 --- a/src/test/java/org/phoebus/channelfinder/ChannelRepositoryIT.java +++ b/src/test/java/org/phoebus/channelfinder/ChannelRepositoryIT.java @@ -380,6 +380,127 @@ void deleteXmlTag() { "Failed to delete the channel"); } + /** delete multiple channels */ + @Test + void deleteXmlChannels() { + Channel testChannel = new Channel("testChannel", "testOwner", testProperties, testTags); + Channel testChannel1 = new Channel("testChannel1", "testOwner1", testProperties, testTags); + List testChannels = Arrays.asList(testChannel, testChannel1); + Iterable createdChannels = channelRepository.indexAll(testChannels); + // Start with all created channels in cleanup list in case deleteAll fails + cleanupTestChannels = new ArrayList<>(Lists.newArrayList(createdChannels)); + + channelRepository.deleteAll(testChannels); + // verify the channels were deleted as expected + Assertions.assertFalse( + channelRepository.existsById(testChannel.getName()), + "Failed to delete the channel " + testChannel.getName()); + cleanupTestChannels.removeIf(ch -> ch.getName().equals(testChannel.getName())); + Assertions.assertFalse( + channelRepository.existsById(testChannel1.getName()), + "Failed to delete the channel " + testChannel1.getName()); + cleanupTestChannels.removeIf(ch -> ch.getName().equals(testChannel1.getName())); + } + + + /** Test best effort delete - should delete what it can, even when some channels don't exist */ + @Test + void deleteChannelsBestEffort() { + Channel testChannel = new Channel("testChannel", "testOwner", testProperties, testTags); + Channel testChannel1 = new Channel("testChannel1", "testOwner1", testProperties, testTags); + List testChannels = Arrays.asList(testChannel, testChannel1); + Iterable createdChannels = channelRepository.indexAll(testChannels); + cleanupTestChannels = new ArrayList<>(Lists.newArrayList(createdChannels)); + + // Create a list with existing and non-existing channel IDs + List channelIdsToDelete = + Arrays.asList( + testChannel.getName(), // exists + "non-existent-channel-1", // doesn't exist + testChannel1.getName(), // exists + "non-existent-channel-2" // doesn't exist + ); + + // Delete using best effort - should succeed for existing channels + long deletedCount = channelRepository.deleteAllByIdBestEffort(channelIdsToDelete); + + // Verify that both existing channels were deleted (count should be 2) + Assertions.assertEquals( + 2, deletedCount, "Expected 2 channels to be deleted, but got " + deletedCount); + + // Verify the first channel was actually deleted + Assertions.assertFalse( + channelRepository.existsById(testChannel.getName()), + "Failed to delete the channel " + testChannel.getName()); + + // Verify the second channel was actually deleted + Assertions.assertFalse( + channelRepository.existsById(testChannel1.getName()), + "Failed to delete the channel " + testChannel1.getName()); + + cleanupTestChannels.clear(); + } + + /** Test best effort delete with all non-existent channels - should return 0 deleted count */ + @Test + void deleteChannelsBestEffortWithNoExistingChannels() { + // Create a list with only non-existing channel IDs + List channelIdsToDelete = + Arrays.asList("non-existent-channel-1", "non-existent-channel-2", "non-existent-channel-3"); + + // Delete using best effort - should handle gracefully with no deletions + long deletedCount = channelRepository.deleteAllByIdBestEffort(channelIdsToDelete); + + // Verify that no channels were deleted + Assertions.assertEquals( + 0, deletedCount, "Expected 0 channels to be deleted, but got " + deletedCount); + } + + /** Test best effort delete with empty list - should return 0 deleted count */ + @Test + void deleteChannelsBestEffortWithEmptyList() { + // Delete using best effort with an empty list + long deletedCount = channelRepository.deleteAllByIdBestEffort(Collections.emptyList()); + + // Verify that no channels were deleted + Assertions.assertEquals( + 0, + deletedCount, + "Expected 0 channels to be deleted from empty list, but got " + deletedCount); + } + + /** Test best effort delete with duplicate IDs - should handle duplicates correctly */ + @Test + void deleteChannelsBestEffortWithDuplicateIds() { + Channel testChannel = new Channel("testChannel", "testOwner", testProperties, testTags); + Iterable createdChannels = channelRepository.indexAll(Arrays.asList(testChannel)); + cleanupTestChannels = new ArrayList<>(Lists.newArrayList(createdChannels)); + + // Create a list with duplicate IDs + List channelIdsToDelete = + Arrays.asList( + testChannel.getName(), // first occurrence + "non-existent-channel", + testChannel.getName() // duplicate of existing channel + ); + + // Delete using best effort + long deletedCount = channelRepository.deleteAllByIdBestEffort(channelIdsToDelete); + + // Should delete the channel only once (duplicates are normalized) + Assertions.assertEquals( + 1, + deletedCount, + "Expected 1 channel to be deleted (duplicates should be normalized), but got " + + deletedCount); + + // Verify the channel was actually deleted + Assertions.assertFalse( + channelRepository.existsById(testChannel.getName()), "Failed to delete the channel"); + + cleanupTestChannels.clear(); + } + /** * Update a channel with 1. additional list of tags and properties 2. update the values of * existing properties @@ -510,27 +631,6 @@ private List createTestProperties() { } } - /** - * A utility class which will create the requested number of test tags named 'test-tag#' - * - * @return list of created tags - */ - private List createTestTags(int count) { - List testTags = new ArrayList(); - for (int i = 0; i < count; i++) { - Tag testTag = new Tag(); - testTag.setName("test-tag" + i); - testTag.setOwner("test-owner"); - testTags.add(testTag); - } - try { - return Lists.newArrayList(tagRepository.indexAll(testTags)); - } catch (Exception e) { - tagRepository.deleteAll(testTags); - return Collections.emptyList(); - } - } - // Helper operations to create and clean up the resources needed for successful // testing of the channelRepository operations @@ -575,4 +675,5 @@ public void cleanup() { void tearDown() throws IOException { ElasticConfigIT.teardown(esService); } + } diff --git a/src/test/java/org/phoebus/channelfinder/docker/ChannelFinderChannelsIT.java b/src/test/java/org/phoebus/channelfinder/docker/ChannelFinderChannelsIT.java index 829ae555..e84398da 100644 --- a/src/test/java/org/phoebus/channelfinder/docker/ChannelFinderChannelsIT.java +++ b/src/test/java/org/phoebus/channelfinder/docker/ChannelFinderChannelsIT.java @@ -585,9 +585,9 @@ void handleChannelsCreateUpdateCheck() { + "]"; ITUtilChannels.assertCreateReplaceMultipleChannels( - AuthorizationChoice.ADMIN, "", json_multiple, HttpURLConnection.HTTP_INTERNAL_ERROR); + AuthorizationChoice.ADMIN, "", json_multiple, HttpURLConnection.HTTP_BAD_REQUEST); - ITUtilChannels.assertUpdateChannels("", json_multiple, HttpURLConnection.HTTP_INTERNAL_ERROR); + ITUtilChannels.assertUpdateChannels("", json_multiple, HttpURLConnection.HTTP_BAD_REQUEST); json_multiple = "[" diff --git a/src/test/java/org/phoebus/channelfinder/service/ChannelServiceTest.java b/src/test/java/org/phoebus/channelfinder/service/ChannelServiceTest.java index 5d49034d..e74c7b28 100644 --- a/src/test/java/org/phoebus/channelfinder/service/ChannelServiceTest.java +++ b/src/test/java/org/phoebus/channelfinder/service/ChannelServiceTest.java @@ -1,9 +1,14 @@ package org.phoebus.channelfinder.service; import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; +import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; import java.util.List; @@ -138,4 +143,38 @@ void createChannel_validChannelWithTagAndProperty_noException() { assertDoesNotThrow(() -> channelService.create("ch", channel)); } + + @Test + void removeMultipleChannels_validChannels_returnsDeletedCount() { + when(authorizationService.isAuthorizedOwner(any(), any(Channel.class))).thenReturn(true); + when(channelRepository.findAllById(any())) + .thenReturn( + List.of( + new Channel("ch1", "owner"), + new Channel("ch2", "owner"), + new Channel("ch3", "owner"))); + when(channelRepository.deleteAllByIdBestEffort(any())).thenReturn(3L); + + long deleted = channelService.remove(List.of("ch1", "ch2", "ch3")); + + assertEquals(3L, deleted); + verify(channelRepository, times(1)).deleteAllByIdBestEffort(any()); + verify(channelRepository, never()).deleteAllById(any()); + verify(channelRepository, never()).deleteById(anyString()); + } + + @Test + void removeMultipleChannels_whenOneMissing_deletesExistingAndReturnsDeletedCount() { + when(authorizationService.isAuthorizedOwner(any(), any(Channel.class))).thenReturn(true); + when(channelRepository.findAllById(any())) + .thenReturn(List.of(new Channel("ch1", "owner"), new Channel("ch3", "owner"))); + when(channelRepository.deleteAllByIdBestEffort(eq(List.of("ch1", "ch3")))).thenReturn(2L); + + long deleted = channelService.remove(List.of("ch1", "missing", "ch3")); + + assertEquals(2L, deleted); + verify(channelRepository, times(1)).deleteAllByIdBestEffort(eq(List.of("ch1", "ch3"))); + verify(channelRepository, never()).deleteAllById(any()); + verify(channelRepository, never()).deleteById(anyString()); + } }