feat(devices): add and remove tags on a device at runtime [CU-86ak6uwjn] - #2134
aliaska-varieva wants to merge 1 commit into
Conversation
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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014Fv6BHWQQPGjLheZcX38J9
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @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); | ||
| } |
There was a problem hiding this comment.
🦩 🟠 [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
| 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; | ||
| } |
There was a problem hiding this comment.
🦩 🟠 [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
| 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; | ||
| } |
There was a problem hiding this comment.
🦩 🟠 [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
🦩 Flamingo Code Review3 finding(s) — 0 action required · 3 recommended · 0 informational Mode: advisory · Rules cited: Need another pass? Commits pushed after this review are not reviewed automatically.
Prefer typing? Comment React 👍/👎 on inline comments to teach the reviewer. |
2 participants
Why
Device tags could only be attached during agent registration, and could never be detached.
TagAssignmentRepository.deleteByEntityIdAndTagIdAndEntityTypeexisted andMachineTagEventAspectalready intercepted it to re-sync Pinot forDEVICE— but the only callers wereTicketTagServiceandKnowledgeBaseTagService. The only device untagging that happened was collateral:deleteTag(id)drops the whole key and cascades across every device.What
New
DeviceTagServiceinopenframe-api-lib, plus two GraphQL mutations:assignDeviceTagfinds or creates theDEVICEtag by key, so tagging with a new key is one round trip rather thancreateTag+ assign.RegistrationTagAssignmentService, so a tag applied from the UI behaves identically to one sent in the registration payload.falsewhen the device didn't carry the tag.removeDeviceTagdecodes the Relay global id likeupdateTag/deleteTag, since that's the form clients hold.assignDeviceTagreturns the tag carrying this device's values, not the key's full option list.Pinot sync
Every write goes through a repository method that
MachineTagEventAspectintercepts (save, ordeleteByEntityIdAndTagIdAndEntityType), so thetags/tagKeyValuescolumns backing the device filter facets stay in sync.This is the trap for anyone adding a bulk variant later —
deleteAllanddeleteByEntityIdAndEntityTypeare not intercepted, and using them would leave a device's Pinot row carrying tags it no longer has. Spelled out in the service javadoc.Notes for review
updateDeviceNicknameandcreateTagdon't take anAuthPrincipaleither, so this follows the device/tag convention rather than the ticket one (validateAdminAccess). Happy to add it if that's wrong.taggedBy/createdByleft unset — nothing in the codebase populates them today.RegistrationTagAssignmentService:openframe-client-coredoesn't depend onopenframe-api-lib, and pushing the logic into a common module would drag the machine and tag repositories with it.openframe-api-lib+openframe-api-service-core.Out of scope, found while working
AgentRegistrationService.register()mints a freshUUID.randomUUID()machineId with no lookup for an existing machine, so an agent re-registering after a config wipe or reimage appears as a new device — its tags don't carry over, and the stale row keeps its assignments and keeps inflating the tag facet counts. Related:AgentRegistrationRequestcarriesserialNumber,osUuid,macAddress,ip,manufacturer,model,displayName,osVersion,osBuild, butapplyRegistrationRequestFieldscopies none of them to theMachine, so there's currently no persisted identifier to dedup on.🤖 Generated with Claude Code
https://claude.ai/code/session_014Fv6BHWQQPGjLheZcX38J9