From d9f147a6ebbc0ab4fdb1475ee8d7d68c84c54039 Mon Sep 17 00:00:00 2001 From: aliaska-varieva Date: Thu, 10 Sep 2026 01:50:47 +0200 Subject: [PATCH 1/6] feat(devices): add and remove tags on a device at runtime MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Device tags could only be attached during agent registration, and could never be detached: the repository method and its Pinot-resync aspect existed, but only the ticket and knowledge-base services called them. DeviceTagService mirrors RegistrationTagAssignmentService so a tag applied from the UI behaves exactly like one sent in the registration payload — the tag key is created on first use and new values are merged into both the device's assignment and the key's predefined options, never replacing them. Removal drops the assignment and keeps the key, which stays available to other devices and in the filter dropdowns. Both writes go through repository methods that MachineTagEventAspect intercepts, so the Pinot tags/tagKeyValues columns backing the device filter facets stay in sync. That constraint is easy to break from a bulk variant later, so it is spelled out on the service. removeDeviceTag decodes the Relay global id the way updateTag and deleteTag do, since that is the form the clients hold. CU-86ak6uwjn Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_014Fv6BHWQQPGjLheZcX38J9 --- .../api/service/device/DeviceTagService.java | 182 ++++++++++++++++++ .../api/datafetcher/DeviceDataFetcher.java | 18 ++ .../src/main/resources/schema/device.graphqls | 9 + 3 files changed, 209 insertions(+) create mode 100644 openframe-api-lib/src/main/java/com/openframe/api/service/device/DeviceTagService.java diff --git a/openframe-api-lib/src/main/java/com/openframe/api/service/device/DeviceTagService.java b/openframe-api-lib/src/main/java/com/openframe/api/service/device/DeviceTagService.java new file mode 100644 index 0000000000..5433c34ca1 --- /dev/null +++ b/openframe-api-lib/src/main/java/com/openframe/api/service/device/DeviceTagService.java @@ -0,0 +1,182 @@ +package com.openframe.api.service.device; + +import com.openframe.api.exception.DeviceNotFoundException; +import com.openframe.data.document.tag.Tag; +import com.openframe.data.document.tag.TagAssignment; +import com.openframe.data.document.tag.TagValidation; +import com.openframe.data.repository.device.MachineRepository; +import com.openframe.data.repository.tag.TagAssignmentRepository; +import com.openframe.data.repository.tag.TagRepository; +import jakarta.validation.constraints.NotBlank; +import lombok.RequiredArgsConstructor; +import lombok.extern.slf4j.Slf4j; +import org.springframework.stereotype.Service; +import org.springframework.transaction.annotation.Transactional; +import org.springframework.validation.annotation.Validated; + +import java.time.Instant; +import java.util.ArrayList; +import java.util.LinkedHashSet; +import java.util.List; +import java.util.Optional; + +import static com.openframe.data.document.tag.TagEntityType.DEVICE; + +/** + * Attaches and detaches DEVICE tags at runtime — the counterpart to + * {@code RegistrationTagAssignmentService}, which does the same thing from the agent-registration + * payload. Both write only Mongo ({@code tags} + {@code tag_assignments}); the Pinot facet columns + * are refreshed by {@code MachineTagEventAspect}, which intercepts the repository calls made here + * and republishes the machine's full tag list to Kafka. + * + *

Every write therefore has to go through an intercepted repository method — {@code save} or + * {@code deleteByEntityIdAndTagIdAndEntityType}. Bypassing them (e.g. {@code deleteAll}) would + * leave the device's Pinot row carrying tags it no longer has. + * + *

Not reused from the client-core service because {@code openframe-client-core} does not depend + * on {@code openframe-api-lib}, and pulling the logic down into a shared module would drag the + * machine/tag repositories along with it. + */ +@Service +@Slf4j +@Validated +@RequiredArgsConstructor +@Transactional(readOnly = true) +public class DeviceTagService { + + private final TagRepository tagRepository; + private final TagAssignmentRepository tagAssignmentRepository; + private final MachineRepository machineRepository; + + /** + * Tags a device with {@code key}, creating the tag key on first use. + * + *

Additive in both directions: new {@code values} are merged into the device's existing + * assignment rather than replacing it, and into the tag's list of predefined options. Calling + * it twice with the same key and values is a no-op beyond the Pinot republish. + * + * @return the tag, carrying this device's values (not the tag's full option list) + */ + @Transactional + public Tag assignTag(@NotBlank String machineId, @NotBlank String key, List values) { + TagValidation.validateKey(key); + TagValidation.validateValues(values, key); + requireMachine(machineId); + + Tag tag = findOrCreateTag(key, values); + List assignedValues = upsertAssignment(machineId, tag.getId(), values); + + log.info("Assigned tag '{}' to machine {} with values {}", key, machineId, assignedValues); + return Tag.builder() + .id(tag.getId()) + .key(tag.getKey()) + .description(tag.getDescription()) + .color(tag.getColor()) + .values(assignedValues) + .entityType(tag.getEntityType()) + .createdAt(tag.getCreatedAt()) + .build(); + } + + /** + * Detaches a tag from a device. The tag key itself survives — it stays available for other + * devices and in the filter dropdowns; use {@code TagService.deleteTag} to drop the key + * everywhere. + * + * @return {@code true} if the device had the tag, {@code false} if there was nothing to remove + */ + @Transactional + public boolean removeTag(@NotBlank String machineId, @NotBlank String tagId) { + requireMachine(machineId); + + boolean assigned = tagAssignmentRepository + .findByEntityIdAndTagIdAndEntityType(machineId, tagId, DEVICE) + .isPresent(); + if (!assigned) { + log.info("Tag {} is not assigned to machine {}, nothing to remove", tagId, machineId); + return false; + } + + // Aspect-intercepted: publishes the machine's remaining tags before the delete proceeds. + tagAssignmentRepository.deleteByEntityIdAndTagIdAndEntityType(machineId, tagId, DEVICE); + log.info("Removed tag {} from machine {}", tagId, machineId); + return true; + } + + private void requireMachine(String machineId) { + if (machineRepository.findByMachineId(machineId).isEmpty()) { + throw new DeviceNotFoundException("Device not found: " + machineId); + } + } + + /** + * Finds the DEVICE tag for {@code key}, or creates it. On an existing tag any previously unseen + * values are appended to its predefined options, so a value typed on one device becomes a + * suggestion for the next. + */ + private Tag findOrCreateTag(String key, List values) { + Tag existing = tagRepository.findByKeyAndEntityType(key, DEVICE); + if (existing == null) { + Tag created = tagRepository.save(Tag.builder() + .key(key) + .values(normalize(values)) + .entityType(DEVICE) + .createdAt(Instant.now()) + .build()); + log.info("Created DEVICE tag '{}' (id={})", key, created.getId()); + return created; + } + + List merged = merge(existing.getValues(), values); + if (merged.size() != size(existing.getValues())) { + existing.setValues(merged); + // Aspect-intercepted: refreshes every device already carrying this tag. + existing = tagRepository.save(existing); + log.info("Appended values {} to existing tag '{}'", values, key); + } + return existing; + } + + /** + * Merges {@code values} into the device's assignment, creating it if the device does not carry + * the tag yet. Saving through the repository is what triggers the Pinot republish. + */ + private List upsertAssignment(String machineId, String tagId, List values) { + Optional existing = tagAssignmentRepository + .findByEntityIdAndTagIdAndEntityType(machineId, tagId, DEVICE); + + if (existing.isEmpty()) { + TagAssignment saved = tagAssignmentRepository.save(TagAssignment.builder() + .entityId(machineId) + .tagId(tagId) + .entityType(DEVICE) + .values(normalize(values)) + .taggedAt(Instant.now()) + .build()); + return saved.getValues(); + } + + TagAssignment assignment = existing.get(); + List merged = merge(assignment.getValues(), values); + if (merged.size() != size(assignment.getValues())) { + assignment.setValues(merged); + return tagAssignmentRepository.save(assignment).getValues(); + } + return assignment.getValues(); + } + + /** Insertion-ordered union — existing values keep their order, new ones are appended. */ + private static List merge(List current, List added) { + LinkedHashSet merged = new LinkedHashSet<>(normalize(current)); + merged.addAll(normalize(added)); + return new ArrayList<>(merged); + } + + private static List normalize(List values) { + return values != null ? values : List.of(); + } + + private static int size(List values) { + return values != null ? values.size() : 0; + } +} diff --git a/openframe-api-service-core/src/main/java/com/openframe/api/datafetcher/DeviceDataFetcher.java b/openframe-api-service-core/src/main/java/com/openframe/api/datafetcher/DeviceDataFetcher.java index 2b5f08a930..84d16645c9 100644 --- a/openframe-api-service-core/src/main/java/com/openframe/api/datafetcher/DeviceDataFetcher.java +++ b/openframe-api-service-core/src/main/java/com/openframe/api/datafetcher/DeviceDataFetcher.java @@ -19,6 +19,7 @@ import com.openframe.api.mapper.GraphQLDeviceMapper; import com.openframe.api.service.device.DeviceFilterService; import com.openframe.api.service.device.DeviceService; +import com.openframe.api.service.device.DeviceTagService; import com.openframe.api.service.FleetVulnerabilityStatusService; import com.openframe.api.service.TagService; import com.openframe.data.document.device.Machine; @@ -53,6 +54,7 @@ public class DeviceDataFetcher { private final DeviceService deviceService; private final DeviceFilterService deviceFilterService; + private final DeviceTagService deviceTagService; private final TagService tagService; private final FleetVulnerabilityStatusService fleetVulnerabilityStatusService; private final GraphQLDeviceMapper mapper; @@ -131,6 +133,22 @@ public Machine updateDeviceNickname(@InputArgument @NotBlank String machineId, return deviceService.updateNickname(machineId, nickname); } + @DgsMutation + public Tag assignDeviceTag(@InputArgument @NotBlank String machineId, + @InputArgument @NotBlank String key, + @InputArgument List values) { + log.debug("Assigning tag '{}' to machineId: {}", key, machineId); + return deviceTagService.assignTag(machineId, key, values); + } + + @DgsMutation + public boolean removeDeviceTag(@InputArgument @NotBlank String machineId, + @InputArgument @NotBlank String tagId) { + String rawTagId = RELAY.fromGlobalId(tagId).getId(); + log.debug("Removing tag {} (rawId: {}) from machineId: {}", tagId, rawTagId, machineId); + return deviceTagService.removeTag(machineId, rawTagId); + } + @DgsData(parentType = "Machine", field = "id") public String machineNodeId(DgsDataFetchingEnvironment dfe) { Machine machine = dfe.getSource(); diff --git a/openframe-api-service-core/src/main/resources/schema/device.graphqls b/openframe-api-service-core/src/main/resources/schema/device.graphqls index b02e91312a..caaa4b64e2 100644 --- a/openframe-api-service-core/src/main/resources/schema/device.graphqls +++ b/openframe-api-service-core/src/main/resources/schema/device.graphqls @@ -146,4 +146,13 @@ type InstalledAgent implements Node { extend type Mutation { updateDeviceNickname(machineId: String!, nickname: String): Machine! + + # Tag a device, creating the tag key on first use. Values are merged into whatever the + # device already carries for that key, so this only ever adds. Returns the tag with + # THIS device's values. + assignDeviceTag(machineId: String!, key: String!, values: [String!]): Tag! + + # Detach a tag from a device. The tag key itself is kept (use deleteTag to drop it + # everywhere). False when the device did not carry the tag. + removeDeviceTag(machineId: String!, tagId: ID!): Boolean! } From 0fd5bef64304992e4e91c2dac03cf5d679ddfc85 Mon Sep 17 00:00:00 2001 From: Aliaska Varieva Date: Sat, 19 Sep 2026 22:17:05 +0200 Subject: [PATCH 2/6] test(devices): pass DeviceTagService to DeviceDataFetcher in selection-set test --- .../api/datafetcher/DeviceFiltersSelectionSetTest.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/openframe-api-service-core/src/test/java/com/openframe/api/datafetcher/DeviceFiltersSelectionSetTest.java b/openframe-api-service-core/src/test/java/com/openframe/api/datafetcher/DeviceFiltersSelectionSetTest.java index a9c309033d..19233f3d7b 100644 --- a/openframe-api-service-core/src/test/java/com/openframe/api/datafetcher/DeviceFiltersSelectionSetTest.java +++ b/openframe-api-service-core/src/test/java/com/openframe/api/datafetcher/DeviceFiltersSelectionSetTest.java @@ -7,6 +7,7 @@ import com.openframe.api.mapper.GraphQLDeviceMapper; import com.openframe.api.service.device.DeviceFilterService; import com.openframe.api.service.device.DeviceService; +import com.openframe.api.service.device.DeviceTagService; import com.openframe.api.service.FleetVulnerabilityStatusService; import com.openframe.api.service.TagService; import graphql.ExecutionResult; @@ -70,6 +71,7 @@ class DeviceFiltersSelectionSetTest { @Mock private DeviceService deviceService; @Mock private DeviceFilterService deviceFilterService; + @Mock private DeviceTagService deviceTagService; @Mock private TagService tagService; @Mock private GraphQLDeviceMapper mapper; @Mock private FleetVulnerabilityStatusService fleetVulnerabilityStatusService; @@ -80,7 +82,7 @@ class DeviceFiltersSelectionSetTest { @BeforeEach void setUp() { - DeviceDataFetcher dataFetcher = new DeviceDataFetcher(deviceService, deviceFilterService, tagService, fleetVulnerabilityStatusService, mapper); + DeviceDataFetcher dataFetcher = new DeviceDataFetcher(deviceService, deviceFilterService, deviceTagService, tagService, fleetVulnerabilityStatusService, mapper); TypeDefinitionRegistry registry = new SchemaParser().parse(SDL); RuntimeWiring wiring = RuntimeWiring.newRuntimeWiring() From 2043c0226d3c86e56cc50928371943ca42a31874 Mon Sep 17 00:00:00 2001 From: Aliaska Varieva Date: Sat, 19 Sep 2026 22:19:29 +0200 Subject: [PATCH 3/6] fix(devices): reject a device tag key that differs from an existing one only by case --- .../com/openframe/api/service/device/DeviceTagService.java | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/openframe-api-lib/src/main/java/com/openframe/api/service/device/DeviceTagService.java b/openframe-api-lib/src/main/java/com/openframe/api/service/device/DeviceTagService.java index 5433c34ca1..0723a3c843 100644 --- a/openframe-api-lib/src/main/java/com/openframe/api/service/device/DeviceTagService.java +++ b/openframe-api-lib/src/main/java/com/openframe/api/service/device/DeviceTagService.java @@ -1,6 +1,7 @@ package com.openframe.api.service.device; import com.openframe.api.exception.DeviceNotFoundException; +import com.openframe.core.exception.ConflictException; import com.openframe.data.document.tag.Tag; import com.openframe.data.document.tag.TagAssignment; import com.openframe.data.document.tag.TagValidation; @@ -113,10 +114,16 @@ private void requireMachine(String machineId) { * Finds the DEVICE tag for {@code key}, or creates it. On an existing tag any previously unseen * values are appended to its predefined options, so a value typed on one device becomes a * suggestion for the next. + * + *

Keys are unique ignoring case, same as {@code TagService.createTag}: the Mongo index is + * case-sensitive, so without the check "Site" would quietly become a second key next to "site". */ private Tag findOrCreateTag(String key, List values) { Tag existing = tagRepository.findByKeyAndEntityType(key, DEVICE); if (existing == null) { + if (tagRepository.existsByKeyIgnoreCaseAndEntityType(key, DEVICE)) { + throw new ConflictException("Tag with key '" + key + "' already exists"); + } Tag created = tagRepository.save(Tag.builder() .key(key) .values(normalize(values)) From b64a82950f6c19ef456f8bafcd7d50b5afaab972 Mon Sep 17 00:00:00 2001 From: Aliaska Varieva Date: Sat, 19 Sep 2026 22:19:29 +0200 Subject: [PATCH 4/6] test(devices): cover DeviceTagService and the assign/remove device tag mutations --- .../service/device/DeviceTagServiceTest.java | 273 ++++++++++++++++++ .../datafetcher/DeviceDataFetcherTest.java | 51 ++++ 2 files changed, 324 insertions(+) create mode 100644 openframe-api-lib/src/test/java/com/openframe/api/service/device/DeviceTagServiceTest.java diff --git a/openframe-api-lib/src/test/java/com/openframe/api/service/device/DeviceTagServiceTest.java b/openframe-api-lib/src/test/java/com/openframe/api/service/device/DeviceTagServiceTest.java new file mode 100644 index 0000000000..f58a400ed1 --- /dev/null +++ b/openframe-api-lib/src/test/java/com/openframe/api/service/device/DeviceTagServiceTest.java @@ -0,0 +1,273 @@ +package com.openframe.api.service.device; + +import com.openframe.api.exception.DeviceNotFoundException; +import com.openframe.core.exception.ConflictException; +import com.openframe.data.document.device.Machine; +import com.openframe.data.document.tag.Tag; +import com.openframe.data.document.tag.TagAssignment; +import com.openframe.data.repository.device.MachineRepository; +import com.openframe.data.repository.tag.TagAssignmentRepository; +import com.openframe.data.repository.tag.TagRepository; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.ArgumentCaptor; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; +import org.mockito.junit.jupiter.MockitoSettings; +import org.mockito.quality.Strictness; + +import java.time.Instant; +import java.util.ArrayList; +import java.util.List; +import java.util.Optional; + +import static com.openframe.data.document.tag.TagEntityType.DEVICE; +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyNoInteractions; +import static org.mockito.Mockito.when; + +@ExtendWith(MockitoExtension.class) +@MockitoSettings(strictness = Strictness.LENIENT) +class DeviceTagServiceTest { + + private static final String MACHINE_ID = "machine-1"; + private static final String TAG_ID = "tag-1"; + + @Mock private TagRepository tagRepository; + @Mock private TagAssignmentRepository tagAssignmentRepository; + @Mock private MachineRepository machineRepository; + + private DeviceTagService service; + + @BeforeEach + void setUp() { + service = new DeviceTagService(tagRepository, tagAssignmentRepository, machineRepository); + + when(machineRepository.findByMachineId(MACHINE_ID)).thenReturn(Optional.of(new Machine())); + when(tagRepository.save(any())).thenAnswer(inv -> { + Tag tag = inv.getArgument(0); + if (tag.getId() == null) { + tag.setId(TAG_ID); + } + return tag; + }); + when(tagAssignmentRepository.save(any())).thenAnswer(inv -> inv.getArgument(0)); + when(tagAssignmentRepository.findByEntityIdAndTagIdAndEntityType(MACHINE_ID, TAG_ID, DEVICE)) + .thenReturn(Optional.empty()); + } + + // ---- assignTag ---- + + @Test + void assignTagCreatesKeyAndAssignmentOnFirstUse() { + when(tagRepository.findByKeyAndEntityType("site", DEVICE)).thenReturn(null); + + Tag result = service.assignTag(MACHINE_ID, "site", List.of("chicago")); + + ArgumentCaptor tag = ArgumentCaptor.forClass(Tag.class); + verify(tagRepository).save(tag.capture()); + assertThat(tag.getValue().getKey()).isEqualTo("site"); + assertThat(tag.getValue().getEntityType()).isEqualTo(DEVICE); + assertThat(tag.getValue().getValues()).containsExactly("chicago"); + assertThat(tag.getValue().getCreatedAt()).isNotNull(); + + ArgumentCaptor assignment = ArgumentCaptor.forClass(TagAssignment.class); + verify(tagAssignmentRepository).save(assignment.capture()); + assertThat(assignment.getValue().getEntityId()).isEqualTo(MACHINE_ID); + assertThat(assignment.getValue().getTagId()).isEqualTo(TAG_ID); + assertThat(assignment.getValue().getEntityType()).isEqualTo(DEVICE); + assertThat(assignment.getValue().getValues()).containsExactly("chicago"); + assertThat(assignment.getValue().getTaggedAt()).isNotNull(); + + assertThat(result.getId()).isEqualTo(TAG_ID); + assertThat(result.getKey()).isEqualTo("site"); + assertThat(result.getValues()).containsExactly("chicago"); + } + + @Test + void assignTagWithoutValuesStoresLabelTag() { + when(tagRepository.findByKeyAndEntityType("vip", DEVICE)).thenReturn(null); + + Tag result = service.assignTag(MACHINE_ID, "vip", null); + + ArgumentCaptor assignment = ArgumentCaptor.forClass(TagAssignment.class); + verify(tagAssignmentRepository).save(assignment.capture()); + assertThat(assignment.getValue().getValues()).isEmpty(); + assertThat(result.getValues()).isEmpty(); + } + + @Test + void assignTagRejectsCaseVariantOfExistingKey() { + when(tagRepository.findByKeyAndEntityType("Site", DEVICE)).thenReturn(null); + when(tagRepository.existsByKeyIgnoreCaseAndEntityType("Site", DEVICE)).thenReturn(true); + + assertThatThrownBy(() -> service.assignTag(MACHINE_ID, "Site", List.of("chicago"))) + .isInstanceOf(ConflictException.class) + .hasMessageContaining("Site"); + + verify(tagRepository, never()).save(any()); + verify(tagAssignmentRepository, never()).save(any()); + } + + @Test + void assignTagReusesExistingKeyWithoutResavingItWhenValuesAreKnown() { + when(tagRepository.findByKeyAndEntityType("site", DEVICE)) + .thenReturn(existingTag("chicago", "boston")); + + service.assignTag(MACHINE_ID, "site", List.of("boston")); + + // Tag.save republishes every device carrying the key — must not fire for a no-op merge. + verify(tagRepository, never()).save(any()); + verify(tagAssignmentRepository).save(any()); + } + + @Test + void assignTagAppendsUnseenValuesToTheKeysOptionsKeepingOrder() { + when(tagRepository.findByKeyAndEntityType("site", DEVICE)) + .thenReturn(existingTag("chicago", "boston")); + + service.assignTag(MACHINE_ID, "site", List.of("boston", "austin")); + + ArgumentCaptor tag = ArgumentCaptor.forClass(Tag.class); + verify(tagRepository).save(tag.capture()); + assertThat(tag.getValue().getId()).isEqualTo(TAG_ID); + assertThat(tag.getValue().getValues()).containsExactly("chicago", "boston", "austin"); + } + + @Test + void assignTagMergesIntoTheDevicesExistingValuesInsteadOfReplacing() { + when(tagRepository.findByKeyAndEntityType("site", DEVICE)) + .thenReturn(existingTag("chicago", "boston")); + TagAssignment current = existingAssignment("chicago"); + when(tagAssignmentRepository.findByEntityIdAndTagIdAndEntityType(MACHINE_ID, TAG_ID, DEVICE)) + .thenReturn(Optional.of(current)); + + Tag result = service.assignTag(MACHINE_ID, "site", List.of("boston")); + + ArgumentCaptor assignment = ArgumentCaptor.forClass(TagAssignment.class); + verify(tagAssignmentRepository).save(assignment.capture()); + assertThat(assignment.getValue().getId()).isEqualTo("assignment-1"); + assertThat(assignment.getValue().getValues()).containsExactly("chicago", "boston"); + assertThat(result.getValues()).containsExactly("chicago", "boston"); + } + + @Test + void assignTagIsIdempotentForValuesTheDeviceAlreadyCarries() { + when(tagRepository.findByKeyAndEntityType("site", DEVICE)) + .thenReturn(existingTag("chicago", "boston")); + when(tagAssignmentRepository.findByEntityIdAndTagIdAndEntityType(MACHINE_ID, TAG_ID, DEVICE)) + .thenReturn(Optional.of(existingAssignment("chicago", "boston"))); + + Tag result = service.assignTag(MACHINE_ID, "site", List.of("boston", "chicago")); + + verify(tagRepository, never()).save(any()); + verify(tagAssignmentRepository, never()).save(any()); + assertThat(result.getValues()).containsExactly("chicago", "boston"); + } + + @Test + void assignTagReturnsTheDevicesValuesNotTheKeysFullOptionList() { + Tag tag = existingTag("chicago", "boston", "austin"); + tag.setDescription("Office location"); + tag.setColor("#FF8800"); + when(tagRepository.findByKeyAndEntityType("site", DEVICE)).thenReturn(tag); + + Tag result = service.assignTag(MACHINE_ID, "site", List.of("austin")); + + assertThat(result.getValues()).containsExactly("austin"); + assertThat(result.getDescription()).isEqualTo("Office location"); + assertThat(result.getColor()).isEqualTo("#FF8800"); + // The stored key must keep its full option list — the return value is a copy. + assertThat(tag.getValues()).containsExactly("chicago", "boston", "austin"); + } + + @Test + void assignTagFailsForUnknownDeviceBeforeTouchingTags() { + when(machineRepository.findByMachineId("ghost")).thenReturn(Optional.empty()); + + assertThatThrownBy(() -> service.assignTag("ghost", "site", List.of("chicago"))) + .isInstanceOf(DeviceNotFoundException.class) + .hasMessageContaining("ghost"); + + verifyNoInteractions(tagRepository, tagAssignmentRepository); + } + + @Test + void assignTagRejectsInvalidKeyBeforeAnyLookup() { + assertThatThrownBy(() -> service.assignTag(MACHINE_ID, "bad key!", null)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("bad key!"); + + verifyNoInteractions(machineRepository, tagRepository, tagAssignmentRepository); + } + + @Test + void assignTagRejectsInvalidValueBeforeAnyLookup() { + assertThatThrownBy(() -> service.assignTag(MACHINE_ID, "site", List.of("chicago", "new york"))) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("new york"); + + verifyNoInteractions(machineRepository, tagRepository, tagAssignmentRepository); + } + + // ---- removeTag ---- + + @Test + void removeTagDeletesThroughTheAspectInterceptedMethodAndKeepsTheKey() { + when(tagAssignmentRepository.findByEntityIdAndTagIdAndEntityType(MACHINE_ID, TAG_ID, DEVICE)) + .thenReturn(Optional.of(existingAssignment("chicago"))); + + boolean removed = service.removeTag(MACHINE_ID, TAG_ID); + + assertThat(removed).isTrue(); + // Only this delete is intercepted by MachineTagEventAspect; any other one skips the Pinot re-sync. + verify(tagAssignmentRepository).deleteByEntityIdAndTagIdAndEntityType(MACHINE_ID, TAG_ID, DEVICE); + verify(tagAssignmentRepository, never()).deleteByTagId(any()); + verify(tagAssignmentRepository, never()).deleteByEntityIdAndEntityType(any(), any()); + verify(tagAssignmentRepository, never()).delete(any()); + verifyNoInteractions(tagRepository); + } + + @Test + void removeTagReturnsFalseWhenTheDeviceDoesNotCarryTheTag() { + boolean removed = service.removeTag(MACHINE_ID, TAG_ID); + + assertThat(removed).isFalse(); + verify(tagAssignmentRepository, never()).deleteByEntityIdAndTagIdAndEntityType(any(), any(), any()); + } + + @Test + void removeTagFailsForUnknownDevice() { + when(machineRepository.findByMachineId("ghost")).thenReturn(Optional.empty()); + + assertThatThrownBy(() -> service.removeTag("ghost", TAG_ID)) + .isInstanceOf(DeviceNotFoundException.class); + + verifyNoInteractions(tagAssignmentRepository); + } + + private static Tag existingTag(String... values) { + return Tag.builder() + .id(TAG_ID) + .key("site") + .entityType(DEVICE) + .values(new ArrayList<>(List.of(values))) + .createdAt(Instant.parse("2026-01-01T00:00:00Z")) + .build(); + } + + private static TagAssignment existingAssignment(String... values) { + return TagAssignment.builder() + .id("assignment-1") + .entityId(MACHINE_ID) + .tagId(TAG_ID) + .entityType(DEVICE) + .values(new ArrayList<>(List.of(values))) + .build(); + } +} diff --git a/openframe-api-service-core/src/test/java/com/openframe/api/datafetcher/DeviceDataFetcherTest.java b/openframe-api-service-core/src/test/java/com/openframe/api/datafetcher/DeviceDataFetcherTest.java index 6f532a76d0..f2227744f2 100644 --- a/openframe-api-service-core/src/test/java/com/openframe/api/datafetcher/DeviceDataFetcherTest.java +++ b/openframe-api-service-core/src/test/java/com/openframe/api/datafetcher/DeviceDataFetcherTest.java @@ -3,12 +3,15 @@ import com.openframe.api.mapper.GraphQLDeviceMapper; import com.openframe.api.service.device.DeviceFilterService; import com.openframe.api.service.device.DeviceService; +import com.openframe.api.service.device.DeviceTagService; import com.openframe.api.service.FleetVulnerabilityStatusService; import com.openframe.api.service.TagService; import com.netflix.graphql.dgs.DgsDataFetchingEnvironment; import com.openframe.data.document.device.Machine; +import com.openframe.data.document.tag.Tag; import com.openframe.data.document.tool.ToolConnection; import com.openframe.data.document.tool.ToolType; +import graphql.relay.Relay; import org.junit.jupiter.api.DisplayName; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; @@ -17,6 +20,7 @@ import org.mockito.junit.jupiter.MockitoExtension; import java.time.Instant; +import java.util.List; import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.Mockito.never; @@ -28,6 +32,7 @@ class DeviceDataFetcherTest { @Mock private DeviceService deviceService; @Mock private DeviceFilterService deviceFilterService; + @Mock private DeviceTagService deviceTagService; @Mock private TagService tagService; @Mock private GraphQLDeviceMapper mapper; @Mock private FleetVulnerabilityStatusService fleetVulnerabilityStatusService; @@ -90,4 +95,50 @@ void toolConnectionVulnerabilitiesUpdatedAt_meshcentral_nullWithoutServiceCall() assertThat(result).isNull(); verify(fleetVulnerabilityStatusService, never()).getLastCompletedVulnerabilityRunAt(); } + + @Test + @DisplayName("assignDeviceTag: delegates to DeviceTagService.assignTag and returns the device-scoped tag") + void assignDeviceTag_delegates() { + Tag assigned = Tag.builder().id("tag-1").key("site").values(List.of("chicago")).build(); + when(deviceTagService.assignTag("m1", "site", List.of("chicago"))).thenReturn(assigned); + + Tag result = fetcher.assignDeviceTag("m1", "site", List.of("chicago")); + + assertThat(result).isSameAs(assigned); + verify(deviceTagService).assignTag("m1", "site", List.of("chicago")); + } + + @Test + @DisplayName("assignDeviceTag: passes null values through for a plain label tag") + void assignDeviceTag_nullValues() { + Tag assigned = Tag.builder().id("tag-1").key("vip").values(List.of()).build(); + when(deviceTagService.assignTag("m1", "vip", null)).thenReturn(assigned); + + Tag result = fetcher.assignDeviceTag("m1", "vip", null); + + assertThat(result).isSameAs(assigned); + } + + @Test + @DisplayName("removeDeviceTag: decodes the Relay global id before delegating") + void removeDeviceTag_decodesGlobalId() { + String globalId = new Relay().toGlobalId("Tag", "tag-1"); + when(deviceTagService.removeTag("m1", "tag-1")).thenReturn(true); + + boolean result = fetcher.removeDeviceTag("m1", globalId); + + assertThat(result).isTrue(); + verify(deviceTagService).removeTag("m1", "tag-1"); + verify(deviceTagService, never()).removeTag("m1", globalId); + } + + @Test + @DisplayName("removeDeviceTag: returns false when the device did not carry the tag") + void removeDeviceTag_notAssigned() { + when(deviceTagService.removeTag("m1", "tag-1")).thenReturn(false); + + boolean result = fetcher.removeDeviceTag("m1", new Relay().toGlobalId("Tag", "tag-1")); + + assertThat(result).isFalse(); + } } From 51ef62f894d7cb03f1f82b8fbd121898856986c9 Mon Sep 17 00:00:00 2001 From: Aliaska Varieva Date: Sat, 19 Sep 2026 22:25:28 +0200 Subject: [PATCH 5/6] feat(devices): setDeviceTagValues replaces a device's values for a tag key Lets the dashboard take a single value off a device without detaching and re-assigning the whole key. Also drops @Transactional from DeviceTagService: no Mongo transaction manager is configured, so it was a no-op. --- .../api/service/device/DeviceTagService.java | 51 +++++--- .../service/device/DeviceTagServiceTest.java | 119 ++++++++++++++++++ .../api/datafetcher/DeviceDataFetcher.java | 8 ++ .../src/main/resources/schema/device.graphqls | 5 + .../datafetcher/DeviceDataFetcherTest.java | 13 ++ 5 files changed, 181 insertions(+), 15 deletions(-) diff --git a/openframe-api-lib/src/main/java/com/openframe/api/service/device/DeviceTagService.java b/openframe-api-lib/src/main/java/com/openframe/api/service/device/DeviceTagService.java index 0723a3c843..558280b4f1 100644 --- a/openframe-api-lib/src/main/java/com/openframe/api/service/device/DeviceTagService.java +++ b/openframe-api-lib/src/main/java/com/openframe/api/service/device/DeviceTagService.java @@ -12,7 +12,6 @@ import lombok.RequiredArgsConstructor; import lombok.extern.slf4j.Slf4j; import org.springframework.stereotype.Service; -import org.springframework.transaction.annotation.Transactional; import org.springframework.validation.annotation.Validated; import java.time.Instant; @@ -34,6 +33,10 @@ * {@code deleteByEntityIdAndTagIdAndEntityType}. Bypassing them (e.g. {@code deleteAll}) would * leave the device's Pinot row carrying tags it no longer has. * + *

Deliberately not {@code @Transactional}: no Mongo transaction manager is configured, so the + * annotation would promise atomicity it cannot give. The tag key is written before the assignment, + * so a failure in between leaves at worst an unused key. + * *

Not reused from the client-core service because {@code openframe-client-core} does not depend * on {@code openframe-api-lib}, and pulling the logic down into a shared module would drag the * machine/tag repositories along with it. @@ -42,7 +45,6 @@ @Slf4j @Validated @RequiredArgsConstructor -@Transactional(readOnly = true) public class DeviceTagService { private final TagRepository tagRepository; @@ -58,16 +60,34 @@ public class DeviceTagService { * * @return the tag, carrying this device's values (not the tag's full option list) */ - @Transactional public Tag assignTag(@NotBlank String machineId, @NotBlank String key, List values) { + return writeTag(machineId, key, values, false); + } + + /** + * Sets the device's values for {@code key} to exactly {@code values}, creating the tag key and + * the assignment on first use. This is how a single value is taken off a device: send the list + * without it. {@code null} or empty leaves the device carrying the key as a plain label; use + * {@link #removeTag} to detach the key altogether. + * + *

Only the device's assignment is replaced. The tag's predefined options stay additive — a + * value dropped here may still be in use on other devices. + * + * @return the tag, carrying this device's values (not the tag's full option list) + */ + public Tag setTagValues(@NotBlank String machineId, @NotBlank String key, List values) { + return writeTag(machineId, key, values, true); + } + + private Tag writeTag(String machineId, String key, List values, boolean replace) { TagValidation.validateKey(key); TagValidation.validateValues(values, key); requireMachine(machineId); Tag tag = findOrCreateTag(key, values); - List assignedValues = upsertAssignment(machineId, tag.getId(), values); + List assignedValues = upsertAssignment(machineId, tag.getId(), values, replace); - log.info("Assigned tag '{}' to machine {} with values {}", key, machineId, assignedValues); + log.info("{} tag '{}' on machine {}, values now {}", replace ? "Set" : "Assigned", key, machineId, assignedValues); return Tag.builder() .id(tag.getId()) .key(tag.getKey()) @@ -86,7 +106,6 @@ public Tag assignTag(@NotBlank String machineId, @NotBlank String key, List values) { } Tag created = tagRepository.save(Tag.builder() .key(key) - .values(normalize(values)) + .values(merge(null, values)) .entityType(DEVICE) .createdAt(Instant.now()) .build()); @@ -145,10 +164,12 @@ private Tag findOrCreateTag(String key, List values) { } /** - * Merges {@code values} into the device's assignment, creating it if the device does not carry - * the tag yet. Saving through the repository is what triggers the Pinot republish. + * Writes {@code values} to the device's assignment, creating it if the device does not carry the + * tag yet: merged into the current values, or in place of them when {@code replace} is set. + * Saving through the repository is what triggers the Pinot republish, so an unchanged list is + * not saved at all. */ - private List upsertAssignment(String machineId, String tagId, List values) { + private List upsertAssignment(String machineId, String tagId, List values, boolean replace) { Optional existing = tagAssignmentRepository .findByEntityIdAndTagIdAndEntityType(machineId, tagId, DEVICE); @@ -157,19 +178,19 @@ private List upsertAssignment(String machineId, String tagId, List merged = merge(assignment.getValues(), values); - if (merged.size() != size(assignment.getValues())) { - assignment.setValues(merged); + List updated = merge(replace ? null : assignment.getValues(), values); + if (!updated.equals(normalize(assignment.getValues()))) { + assignment.setValues(updated); return tagAssignmentRepository.save(assignment).getValues(); } - return assignment.getValues(); + return normalize(assignment.getValues()); } /** Insertion-ordered union — existing values keep their order, new ones are appended. */ diff --git a/openframe-api-lib/src/test/java/com/openframe/api/service/device/DeviceTagServiceTest.java b/openframe-api-lib/src/test/java/com/openframe/api/service/device/DeviceTagServiceTest.java index f58a400ed1..15966598f0 100644 --- a/openframe-api-lib/src/test/java/com/openframe/api/service/device/DeviceTagServiceTest.java +++ b/openframe-api-lib/src/test/java/com/openframe/api/service/device/DeviceTagServiceTest.java @@ -215,6 +215,125 @@ void assignTagRejectsInvalidValueBeforeAnyLookup() { verifyNoInteractions(machineRepository, tagRepository, tagAssignmentRepository); } + @Test + void assignTagDropsDuplicateValuesFromTheRequest() { + when(tagRepository.findByKeyAndEntityType("site", DEVICE)).thenReturn(null); + + Tag result = service.assignTag(MACHINE_ID, "site", List.of("chicago", "chicago", "boston")); + + ArgumentCaptor tag = ArgumentCaptor.forClass(Tag.class); + verify(tagRepository).save(tag.capture()); + assertThat(tag.getValue().getValues()).containsExactly("chicago", "boston"); + assertThat(result.getValues()).containsExactly("chicago", "boston"); + } + + // ---- setTagValues ---- + + @Test + void setTagValuesReplacesTheDevicesValuesSoASingleValueCanBeRemoved() { + when(tagRepository.findByKeyAndEntityType("site", DEVICE)) + .thenReturn(existingTag("chicago", "boston", "austin")); + when(tagAssignmentRepository.findByEntityIdAndTagIdAndEntityType(MACHINE_ID, TAG_ID, DEVICE)) + .thenReturn(Optional.of(existingAssignment("chicago", "boston", "austin"))); + + Tag result = service.setTagValues(MACHINE_ID, "site", List.of("chicago", "austin")); + + ArgumentCaptor assignment = ArgumentCaptor.forClass(TagAssignment.class); + verify(tagAssignmentRepository).save(assignment.capture()); + assertThat(assignment.getValue().getId()).isEqualTo("assignment-1"); + assertThat(assignment.getValue().getValues()).containsExactly("chicago", "austin"); + assertThat(result.getValues()).containsExactly("chicago", "austin"); + } + + @Test + void setTagValuesKeepsTheDroppedValueInTheKeysOptions() { + Tag tag = existingTag("chicago", "boston"); + when(tagRepository.findByKeyAndEntityType("site", DEVICE)).thenReturn(tag); + when(tagAssignmentRepository.findByEntityIdAndTagIdAndEntityType(MACHINE_ID, TAG_ID, DEVICE)) + .thenReturn(Optional.of(existingAssignment("chicago", "boston"))); + + service.setTagValues(MACHINE_ID, "site", List.of("chicago")); + + // Other devices may still carry "boston"; the key's options only ever grow. + verify(tagRepository, never()).save(any()); + assertThat(tag.getValues()).containsExactly("chicago", "boston"); + } + + @Test + void setTagValuesAppendsUnseenValuesToTheKeysOptions() { + when(tagRepository.findByKeyAndEntityType("site", DEVICE)) + .thenReturn(existingTag("chicago")); + when(tagAssignmentRepository.findByEntityIdAndTagIdAndEntityType(MACHINE_ID, TAG_ID, DEVICE)) + .thenReturn(Optional.of(existingAssignment("chicago"))); + + Tag result = service.setTagValues(MACHINE_ID, "site", List.of("austin")); + + ArgumentCaptor tag = ArgumentCaptor.forClass(Tag.class); + verify(tagRepository).save(tag.capture()); + assertThat(tag.getValue().getValues()).containsExactly("chicago", "austin"); + assertThat(result.getValues()).containsExactly("austin"); + } + + @Test + void setTagValuesWithNoValuesKeepsTheKeyOnTheDeviceAsALabel() { + when(tagRepository.findByKeyAndEntityType("site", DEVICE)) + .thenReturn(existingTag("chicago")); + when(tagAssignmentRepository.findByEntityIdAndTagIdAndEntityType(MACHINE_ID, TAG_ID, DEVICE)) + .thenReturn(Optional.of(existingAssignment("chicago"))); + + Tag result = service.setTagValues(MACHINE_ID, "site", null); + + ArgumentCaptor assignment = ArgumentCaptor.forClass(TagAssignment.class); + verify(tagAssignmentRepository).save(assignment.capture()); + assertThat(assignment.getValue().getValues()).isEmpty(); + assertThat(result.getValues()).isEmpty(); + verify(tagAssignmentRepository, never()).deleteByEntityIdAndTagIdAndEntityType(any(), any(), any()); + } + + @Test + void setTagValuesCreatesKeyAndAssignmentOnFirstUse() { + when(tagRepository.findByKeyAndEntityType("site", DEVICE)).thenReturn(null); + + Tag result = service.setTagValues(MACHINE_ID, "site", List.of("chicago")); + + verify(tagRepository).save(any()); + ArgumentCaptor assignment = ArgumentCaptor.forClass(TagAssignment.class); + verify(tagAssignmentRepository).save(assignment.capture()); + assertThat(assignment.getValue().getEntityId()).isEqualTo(MACHINE_ID); + assertThat(assignment.getValue().getValues()).containsExactly("chicago"); + assertThat(result.getValues()).containsExactly("chicago"); + } + + @Test + void setTagValuesSkipsTheSaveWhenNothingChanges() { + when(tagRepository.findByKeyAndEntityType("site", DEVICE)) + .thenReturn(existingTag("chicago", "boston")); + when(tagAssignmentRepository.findByEntityIdAndTagIdAndEntityType(MACHINE_ID, TAG_ID, DEVICE)) + .thenReturn(Optional.of(existingAssignment("chicago", "boston"))); + + service.setTagValues(MACHINE_ID, "site", List.of("chicago", "boston")); + + verify(tagRepository, never()).save(any()); + verify(tagAssignmentRepository, never()).save(any()); + } + + @Test + void setTagValuesAppliesTheSameGuardsAsAssignTag() { + when(machineRepository.findByMachineId("ghost")).thenReturn(Optional.empty()); + when(tagRepository.findByKeyAndEntityType("Site", DEVICE)).thenReturn(null); + when(tagRepository.existsByKeyIgnoreCaseAndEntityType("Site", DEVICE)).thenReturn(true); + + assertThatThrownBy(() -> service.setTagValues("ghost", "site", List.of("chicago"))) + .isInstanceOf(DeviceNotFoundException.class); + assertThatThrownBy(() -> service.setTagValues(MACHINE_ID, "site", List.of("new york"))) + .isInstanceOf(IllegalArgumentException.class); + assertThatThrownBy(() -> service.setTagValues(MACHINE_ID, "Site", List.of("chicago"))) + .isInstanceOf(ConflictException.class); + + verify(tagRepository, never()).save(any()); + verify(tagAssignmentRepository, never()).save(any()); + } + // ---- removeTag ---- @Test diff --git a/openframe-api-service-core/src/main/java/com/openframe/api/datafetcher/DeviceDataFetcher.java b/openframe-api-service-core/src/main/java/com/openframe/api/datafetcher/DeviceDataFetcher.java index 84d16645c9..3e82cc30b6 100644 --- a/openframe-api-service-core/src/main/java/com/openframe/api/datafetcher/DeviceDataFetcher.java +++ b/openframe-api-service-core/src/main/java/com/openframe/api/datafetcher/DeviceDataFetcher.java @@ -141,6 +141,14 @@ public Tag assignDeviceTag(@InputArgument @NotBlank String machineId, return deviceTagService.assignTag(machineId, key, values); } + @DgsMutation + public Tag setDeviceTagValues(@InputArgument @NotBlank String machineId, + @InputArgument @NotBlank String key, + @InputArgument List values) { + log.debug("Setting values of tag '{}' on machineId: {}", key, machineId); + return deviceTagService.setTagValues(machineId, key, values); + } + @DgsMutation public boolean removeDeviceTag(@InputArgument @NotBlank String machineId, @InputArgument @NotBlank String tagId) { diff --git a/openframe-api-service-core/src/main/resources/schema/device.graphqls b/openframe-api-service-core/src/main/resources/schema/device.graphqls index caaa4b64e2..84f0197ce3 100644 --- a/openframe-api-service-core/src/main/resources/schema/device.graphqls +++ b/openframe-api-service-core/src/main/resources/schema/device.graphqls @@ -152,6 +152,11 @@ extend type Mutation { # THIS device's values. assignDeviceTag(machineId: String!, key: String!, values: [String!]): Tag! + # Set a device's values for a tag key to exactly this list, creating the key on first use. + # Send the list without a value to take that value off the device; null/empty keeps the + # key on the device as a plain label. Returns the tag with THIS device's values. + setDeviceTagValues(machineId: String!, key: String!, values: [String!]): Tag! + # Detach a tag from a device. The tag key itself is kept (use deleteTag to drop it # everywhere). False when the device did not carry the tag. removeDeviceTag(machineId: String!, tagId: ID!): Boolean! diff --git a/openframe-api-service-core/src/test/java/com/openframe/api/datafetcher/DeviceDataFetcherTest.java b/openframe-api-service-core/src/test/java/com/openframe/api/datafetcher/DeviceDataFetcherTest.java index f2227744f2..42a70ec3a5 100644 --- a/openframe-api-service-core/src/test/java/com/openframe/api/datafetcher/DeviceDataFetcherTest.java +++ b/openframe-api-service-core/src/test/java/com/openframe/api/datafetcher/DeviceDataFetcherTest.java @@ -119,6 +119,19 @@ void assignDeviceTag_nullValues() { assertThat(result).isSameAs(assigned); } + @Test + @DisplayName("setDeviceTagValues: delegates to DeviceTagService.setTagValues, not the additive assignTag") + void setDeviceTagValues_delegates() { + Tag updated = Tag.builder().id("tag-1").key("site").values(List.of("chicago")).build(); + when(deviceTagService.setTagValues("m1", "site", List.of("chicago"))).thenReturn(updated); + + Tag result = fetcher.setDeviceTagValues("m1", "site", List.of("chicago")); + + assertThat(result).isSameAs(updated); + verify(deviceTagService).setTagValues("m1", "site", List.of("chicago")); + verify(deviceTagService, never()).assignTag("m1", "site", List.of("chicago")); + } + @Test @DisplayName("removeDeviceTag: decodes the Relay global id before delegating") void removeDeviceTag_decodesGlobalId() { From ada5ab6a8fa78f4b4c3dbc442eac1f4c956776ef Mon Sep 17 00:00:00 2001 From: Aliaska Varieva Date: Sat, 19 Sep 2026 22:29:05 +0200 Subject: [PATCH 6/6] fix(devices): survive a lost first-use race when tagging a device Two requests creating the same tag key (or the same assignment) at once: the unique index already keeps the data single, but the loser surfaced a DuplicateKeyException as a database error. Re-read and carry on instead. --- .../api/service/device/DeviceTagService.java | 51 +++++++++++++------ .../service/device/DeviceTagServiceTest.java | 43 ++++++++++++++++ 2 files changed, 78 insertions(+), 16 deletions(-) diff --git a/openframe-api-lib/src/main/java/com/openframe/api/service/device/DeviceTagService.java b/openframe-api-lib/src/main/java/com/openframe/api/service/device/DeviceTagService.java index 558280b4f1..6c8915e83f 100644 --- a/openframe-api-lib/src/main/java/com/openframe/api/service/device/DeviceTagService.java +++ b/openframe-api-lib/src/main/java/com/openframe/api/service/device/DeviceTagService.java @@ -11,6 +11,7 @@ import jakarta.validation.constraints.NotBlank; import lombok.RequiredArgsConstructor; import lombok.extern.slf4j.Slf4j; +import org.springframework.dao.DuplicateKeyException; import org.springframework.stereotype.Service; import org.springframework.validation.annotation.Validated; @@ -143,14 +144,23 @@ private Tag findOrCreateTag(String key, List values) { if (tagRepository.existsByKeyIgnoreCaseAndEntityType(key, DEVICE)) { throw new ConflictException("Tag with key '" + key + "' already exists"); } - Tag created = tagRepository.save(Tag.builder() - .key(key) - .values(merge(null, values)) - .entityType(DEVICE) - .createdAt(Instant.now()) - .build()); - log.info("Created DEVICE tag '{}' (id={})", key, created.getId()); - return created; + try { + Tag created = tagRepository.save(Tag.builder() + .key(key) + .values(merge(null, values)) + .entityType(DEVICE) + .createdAt(Instant.now()) + .build()); + log.info("Created DEVICE tag '{}' (id={})", key, created.getId()); + return created; + } catch (DuplicateKeyException e) { + // Lost a first-use race to another request: the unique (tenant, key, entityType) + // index kept the key single, so carry on with the winner's tag. + existing = tagRepository.findByKeyAndEntityType(key, DEVICE); + if (existing == null) { + throw e; + } + } } List merged = merge(existing.getValues(), values); @@ -174,14 +184,23 @@ private List upsertAssignment(String machineId, String tagId, List assignment = ArgumentCaptor.forClass(TagAssignment.class); + verify(tagAssignmentRepository).save(assignment.capture()); + assertThat(assignment.getValue().getTagId()).isEqualTo(TAG_ID); + } + + @Test + void assignTagMergesIntoTheWinnersAssignmentWhenTheAssignmentRaceIsLost() { + when(tagRepository.findByKeyAndEntityType("site", DEVICE)) + .thenReturn(existingTag("chicago", "boston")); + when(tagAssignmentRepository.findByEntityIdAndTagIdAndEntityType(MACHINE_ID, TAG_ID, DEVICE)) + .thenReturn(Optional.empty()) + .thenReturn(Optional.of(existingAssignment("chicago"))); + doThrow(new DuplicateKeyException("tenant_entity_tag_idx")) + .doAnswer(inv -> inv.getArgument(0)) + .when(tagAssignmentRepository).save(any()); + + Tag result = service.assignTag(MACHINE_ID, "site", List.of("boston")); + + assertThat(result.getValues()).containsExactly("chicago", "boston"); + } + + @Test + void assignTagRethrowsADuplicateKeyItCannotExplain() { + when(tagRepository.findByKeyAndEntityType("site", DEVICE)).thenReturn(null); + doThrow(new DuplicateKeyException("tenant_key_entity_idx")).when(tagRepository).save(any()); + + assertThatThrownBy(() -> service.assignTag(MACHINE_ID, "site", List.of("chicago"))) + .isInstanceOf(DuplicateKeyException.class); + verify(tagAssignmentRepository, never()).save(any()); + } + // ---- setTagValues ---- @Test