Skip to content

feat(devices): add and remove tags on a device at runtime [CU-86ak6uwjn] #2134

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.

Sign up for GitHub

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

Open
aliaska-varieva wants to merge 1 commit into
base: main
Choose a base branch
from
+209 −0
Open

feat(devices): add and remove tags on a device at runtime [CU-86ak6uwjn] #2134

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
182 changes: 182 additions & 0 deletions openframe-api-lib/src/main/java/com/openframe/api/service/device/DeviceTagService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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.
*
* <p>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.
*
* <p>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.
*
* <p>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<String> values) {
TagValidation.validateKey(key);
TagValidation.validateValues(values, key);
requireMachine(machineId);

Tag tag = findOrCreateTag(key, values);
List<String> 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<String> 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;
}

Check warning on line 128 in openframe-api-lib/src/main/java/com/openframe/api/service/device/DeviceTagService.java

View workflow job for this annotation

GitHub Actions / Flamingo Code Review

findOrCreateTag / upsertAssignment read-then-write pattern is not concurrency-safe against parallel assignTag calls for the same key 

findOrCreateTag / upsertAssignment read-then-write pattern is not concurrency-safe against parallel assignTag calls for the same key

findOrCreateTag() does a findByKeyAndEntityType then conditionally save() without any uniqueness constraint enforcement or optimistic locking visible in this diff. Two concurrent assignDeviceTag calls for a brand-new key (e.g., two devices tagging the same new key simultaneously) can both see `existing == null` and each create a separate Tag document for the same key, resulting in duplicate tag keys in Mongo. Given this is a new device-facing mutation exposed to potentially concurrent client callers, a unique index on (key, entityType) with duplicate-key handling (as required elsewhere in the org's OPENFRAM-006-11 idiom for idempotent inserts) should be applied here too.
Comment on lines +117 to +128

@github-actions github-actions Bot Sep 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🟠 [warn/recommended] findOrCreateTag / upsertAssignment read-then-write pattern is not concurrency-safe against parallel assignTag calls for the same key

findOrCreateTag() does a findByKeyAndEntityType then conditionally save() without any uniqueness constraint enforcement or optimistic locking visible in this diff. Two concurrent assignDeviceTag calls for a brand-new key (e.g., two devices tagging the same new key simultaneously) can both see existing == null and each create a separate Tag document for the same key, resulting in duplicate tag keys in Mongo. Given this is a new device-facing mutation exposed to potentially concurrent client callers, a unique index on (key, entityType) with duplicate-key handling (as required elsewhere in the org's OPENFRAM-006-11 idiom for idempotent inserts) should be applied here too.

Evidence 
    private Tag findOrCreateTag(String key, List<String> 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;
        }
🤖 Prompt for AI agents 
In openframe-api-lib/src/main/java/com/openframe/api/service/device/DeviceTagService.java around lines 117-128, address this code-review finding: findOrCreateTag / upsertAssignment read-then-write pattern is not concurrency-safe against parallel assignTag calls for the same key.
findOrCreateTag() does a findByKeyAndEntityType then conditionally save() without any uniqueness constraint enforcement or optimistic locking visible in this diff. Two concurrent assignDeviceTag calls for a brand-new key (e.g., two devices tagging the same new key simultaneously) can both see `existing == null` and each create a separate Tag document for the same key, resulting in duplicate tag keys in Mongo. Given this is a new device-facing mutation exposed to potentially concurrent client callers, a unique index on (key, entityType) with duplicate-key handling (as required elsewhere in the org's OPENFRAM-006-11 idiom for idempotent inserts) should be applied here too.
The flagged code:
```
    private Tag findOrCreateTag(String key, List<String> 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;
        }
```
Make the minimal change that resolves the finding; do not refactor unrelated code.

confidence: 35 — react 👍/👎 to teach the reviewer


List<String> 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;
}

Check warning on line 138 in openframe-api-lib/src/main/java/com/openframe/api/service/device/DeviceTagService.java

View workflow job for this annotation

GitHub Actions / Flamingo Code Review

assignDeviceTag has no upper bound on tag values / no duplicate protection against unbounded tag growth per device 

assignDeviceTag has no upper bound on tag values / no duplicate protection against unbounded tag growth per device

assignTag merges new values into both the tag's global option list and the device's own assignment with no cap on collection size. Since values are user/agent supplied via a public GraphQL mutation, repeated calls with novel values will grow both `tags.values` and `tag_assignments.values` unbounded, inflating the Mongo documents and the Kafka payload republished by MachineTagEventAspect on every write. Consider enforcing a maximum values-per-tag/assignment limit similar to what TagValidation.validateValues might already do for a single call, but applied against the merged/cumulative size.
Comment on lines +117 to +138

@github-actions github-actions Bot Sep 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🟠 [warn/recommended] assignDeviceTag has no upper bound on tag values / no duplicate protection against unbounded tag growth per device

assignTag merges new values into both the tag's global option list and the device's own assignment with no cap on collection size. Since values are user/agent supplied via a public GraphQL mutation, repeated calls with novel values will grow both tags.values and tag_assignments.values unbounded, inflating the Mongo documents and the Kafka payload republished by MachineTagEventAspect on every write. Consider enforcing a maximum values-per-tag/assignment limit similar to what TagValidation.validateValues might already do for a single call, but applied against the merged/cumulative size.

Evidence 
    private Tag findOrCreateTag(String key, List<String> 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;
        }
🤖 Prompt for AI agents 
In openframe-api-lib/src/main/java/com/openframe/api/service/device/DeviceTagService.java around lines 117-138, address this code-review finding: assignDeviceTag has no upper bound on tag values / no duplicate protection against unbounded tag growth per device.
assignTag merges new values into both the tag's global option list and the device's own assignment with no cap on collection size. Since values are user/agent supplied via a public GraphQL mutation, repeated calls with novel values will grow both `tags.values` and `tag_assignments.values` unbounded, inflating the Mongo documents and the Kafka payload republished by MachineTagEventAspect on every write. Consider enforcing a maximum values-per-tag/assignment limit similar to what TagValidation.validateValues might already do for a single call, but applied against the merged/cumulative size.
The flagged code:
```
    private Tag findOrCreateTag(String key, List<String> 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<String> 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;
    }
```
Make the minimal change that resolves the finding; do not refactor unrelated code.

confidence: 30 — react 👍/👎 to teach the reviewer


/**
* 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<String> upsertAssignment(String machineId, String tagId, List<String> values) {
Optional<TagAssignment> 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<String> 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<String> merge(List<String> current, List<String> added) {
LinkedHashSet<String> merged = new LinkedHashSet<>(normalize(current));
merged.addAll(normalize(added));
return new ArrayList<>(merged);
}

private static List<String> normalize(List<String> values) {
return values != null ? values : List.of();
}

private static int size(List<String> values) {
return values != null ? values.size() : 0;
}
}
18 changes: 18 additions & 0 deletions ...frame-api-service-core/src/main/java/com/openframe/api/datafetcher/DeviceDataFetcher.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -53,6 +54,7 @@

private final DeviceService deviceService;
private final DeviceFilterService deviceFilterService;
private final DeviceTagService deviceTagService;
private final TagService tagService;
private final FleetVulnerabilityStatusService fleetVulnerabilityStatusService;
private final GraphQLDeviceMapper mapper;
Expand Down Expand Up @@ -131,6 +133,22 @@
return deviceService.updateNickname(machineId, nickname);
}

@DgsMutation
public Tag assignDeviceTag(@InputArgument @NotBlank String machineId,
@InputArgument @NotBlank String key,
@InputArgument List<String> 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);
}

Check warning on line 150 in openframe-api-service-core/src/main/java/com/openframe/api/datafetcher/DeviceDataFetcher.java

View workflow job for this annotation

GitHub Actions / Flamingo Code Review

removeDeviceTag decodes a Relay global ID with RELAY.fromGlobalId inline in the resolver rather than via the established decode helper 

removeDeviceTag decodes a Relay global ID with RELAY.fromGlobalId inline in the resolver rather than via the established decode helper

The resolver decodes the tagId Relay global ID directly with `RELAY.fromGlobalId(tagId).getId()` inline in the mutation body, whereas the established convention in this codebase (OPENFRAM-004-3) is for resolvers to call a shared decodeId()/decodeIds() helper before delegating to the service layer, keeping ID encoding concerns centralized. Inlining this call risks inconsistent handling (e.g., missing validation of a malformed global id) compared to other mutations in the same file.
Comment on lines +144 to +150

@github-actions github-actions Bot Sep 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦩 🟠 [warn/recommended] OPENFRAM-004-11 removeDeviceTag decodes a Relay global ID with RELAY.fromGlobalId inline in the resolver rather than via the established decode helper

The resolver decodes the tagId Relay global ID directly with RELAY.fromGlobalId(tagId).getId() inline in the mutation body, whereas the established convention in this codebase (OPENFRAM-004-3) is for resolvers to call a shared decodeId()/decodeIds() helper before delegating to the service layer, keeping ID encoding concerns centralized. Inlining this call risks inconsistent handling (e.g., missing validation of a malformed global id) compared to other mutations in the same file.

Evidence 
    @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);
    }
🤖 Prompt for AI agents 
In openframe-api-service-core/src/main/java/com/openframe/api/datafetcher/DeviceDataFetcher.java around lines 144-150, address this code-review finding: removeDeviceTag decodes a Relay global ID with RELAY.fromGlobalId inline in the resolver rather than via the established decode helper.
The resolver decodes the tagId Relay global ID directly with `RELAY.fromGlobalId(tagId).getId()` inline in the mutation body, whereas the established convention in this codebase (OPENFRAM-004-3) is for resolvers to call a shared decodeId()/decodeIds() helper before delegating to the service layer, keeping ID encoding concerns centralized. Inlining this call risks inconsistent handling (e.g., missing validation of a malformed global id) compared to other mutations in the same file.
The flagged code:
```
    @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);
    }
```
Make the minimal change that resolves the finding; do not refactor unrelated code.

confidence: 40 — react 👍/👎 to teach the reviewer


@DgsData(parentType = "Machine", field = "id")
public String machineNodeId(DgsDataFetchingEnvironment dfe) {
Machine machine = dfe.getSource();
Expand Down
9 changes: 9 additions & 0 deletions openframe-api-service-core/src/main/resources/schema/device.graphqls
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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!
}
Loading