feat(devices): add and remove tags on a device at runtime [CU-86ak6uwjn] - #2134
aliaska-varieva wants to merge 8 commits 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
There was a problem hiding this comment.
Leaving as is. There is no shared decodeId() helper on the device/tag side — the only one is private to GraphQLKnowledgeBaseMapper. Inline RELAY.fromGlobalId(..).getId() is what this file already does for device(id) and what TagDataFetcher does for updateTag / deleteTag, so this matches the neighbours. A malformed id throws IllegalArgumentException, which GraphQLExceptionHandler already maps to a client error. Now covered by DeviceDataFetcherTest.removeDeviceTag_decodesGlobalId.
| 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
There was a problem hiding this comment.
Half right. Duplicate keys can't happen: Tag has a unique (tenantId, key, entityType) index and TagAssignment a unique (tenantId, entityId, tagId, entityType) one, and auto-index-creation is on in the base configs. What did happen is that the request losing the race got a DuplicateKeyException surfaced as a database error. Fixed in ada5ab6: both creates now catch it, re-read the winner's document and continue through the normal merge path. Covered by three new cases in DeviceTagServiceTest.
| 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
There was a problem hiding this comment.
Not in this PR. Duplicates are already handled — values go through an insertion-ordered set, an unchanged list isn't saved at all, and each value is length/charset-validated by TagValidation. A count cap is a product limit rather than a bug: the key's option list is the tenant-wide union (a site key legitimately has hundreds), and the same unbounded path exists for agent registration, so a limit belongs in TagValidation for both callers. The mutation is dashboard-only behind auth, so leaving it for a follow-up if we decide on a number.
…g 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.
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.
🦩 Flamingo Code Review2 finding(s) — 0 action required · 2 recommended · 0 informational Mode: advisory · Rules cited: Inline comments: 2 new Need another pass? Commits pushed after this review are not reviewed automatically.
Prefer typing? Comment React 👍/👎 on inline comments to teach the reviewer. Started 2026-09-19 20:29 UTC · updated 2026-09-19 20:30 UTC · workflow run |
| @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 Tag setDeviceTagValues(@InputArgument @NotBlank String machineId, | ||
| @InputArgument @NotBlank String key, | ||
| @InputArgument List<String> values) { | ||
| log.debug("Setting values of tag '{}' on machineId: {}", key, machineId); | ||
| return deviceTagService.setTagValues(machineId, key, values); | ||
| } |
There was a problem hiding this comment.
🦩 🟠 [warn/recommended] OFJAVA-024 GraphQL mutations return the domain model Tag directly instead of a dedicated API response class
assignDeviceTag and setDeviceTagValues return com.openframe.data.document.tag.Tag — the domain/persistence model — directly from the DGS resolver instead of a dedicated API-layer class named by intent (e.g. DeviceTagResponse). OFJAVA-024 requires REST/GraphQL endpoints to accept and return dedicated API classes, never the domain model, so the domain model can evolve independently of the GraphQL contract. Introduce a DeviceTagResponse (or similar) API type and map Tag to it at the resolver boundary.
Evidence
@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 Tag setDeviceTagValues(@InputArgument @NotBlank String machineId,
@InputArgument @NotBlank String key,
@InputArgument List<String> values) {
🤖 Prompt for AI agents
In openframe-api-service-core/src/main/java/com/openframe/api/datafetcher/DeviceDataFetcher.java around lines 136-150, address this code-review finding: GraphQL mutations return the domain model Tag directly instead of a dedicated API response class.
assignDeviceTag and setDeviceTagValues return com.openframe.data.document.tag.Tag — the domain/persistence model — directly from the DGS resolver instead of a dedicated API-layer class named by intent (e.g. DeviceTagResponse). OFJAVA-024 requires REST/GraphQL endpoints to accept and return dedicated API classes, never the domain model, so the domain model can evolve independently of the GraphQL contract. Introduce a DeviceTagResponse (or similar) API type and map Tag to it at the resolver boundary.
The flagged code:
```
@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 Tag setDeviceTagValues(@InputArgument @NotBlank String machineId,
@InputArgument @NotBlank String key,
@InputArgument List<String> values) {
log.debug("Setting values of tag '{}' on machineId: {}", key, machineId);
return deviceTagService.setTagValues(machineId, key, values);
}
```
Make the minimal change that resolves the finding; do not refactor unrelated code.
confidence: 45 — react 👍/👎 to teach the reviewer
There was a problem hiding this comment.
Leaving as is. Tag is already the GraphQL contract for this domain: createTag, updateTag, tags and Machine.tags all resolve the same document to the Tag schema type, with TagDataFetcher mapping id to a Relay global id. A DeviceTagResponse here would give the dashboard a second, differently-named type for the same thing it already caches as Tag. If we want the domain model off the GraphQL boundary, that's a change for the whole tag API, not for two mutations.
| private Tag findOrCreateTag(String key, List<String> values) { | ||
| Tag existing = tagRepository.findByKeyAndEntityType(key, DEVICE); | ||
| if (existing == null) { | ||
| if (tagRepository.existsByKeyIgnoreCaseAndEntityType(key, DEVICE)) { | ||
| throw new ConflictException("Tag with key '" + key + "' already exists"); | ||
| } | ||
| 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; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🦩 🟠 [warn/recommended] findOrCreateTag has a TOCTOU race between the existsByKeyIgnoreCase check and save that can allow duplicate keys differing only by case
In findOrCreateTag, the code checks existsByKeyIgnoreCaseAndEntityType(key, DEVICE) and, if false, proceeds to save. Between the check and the save, another request could create a tag with a different case of the same key. The DuplicateKeyException catch block only guards against an exact-key race (the same case), because the underlying unique index is stated to be case-sensitive ('Keys are unique ignoring case ... the Mongo index is case-sensitive, so without the check "Site" would quietly become a second key next to "site"'). This means the case-insensitive uniqueness guarantee promised in the class doc is not actually race-safe: two concurrent requests with 'Site' and 'site' could both pass the exists check and both succeed at save (since the underlying index is case-sensitive), producing two case-variant keys — the exact failure mode the check claims to prevent.
Evidence
private Tag findOrCreateTag(String key, List<String> values) {
Tag existing = tagRepository.findByKeyAndEntityType(key, DEVICE);
if (existing == null) {
if (tagRepository.existsByKeyIgnoreCaseAndEntityType(key, DEVICE)) {
throw new ConflictException("Tag with key '" + key + "' already exists");
}
try {
Tag created = tagRepository.save(Tag.builder()
.key(key)
.values(merge(null, values))
.entityType(DEVICE)
.createdAt(Instant.now())
🤖 Prompt for AI agents
In openframe-api-lib/src/main/java/com/openframe/api/service/device/DeviceTagService.java around lines 141-164, address this code-review finding: findOrCreateTag has a TOCTOU race between the existsByKeyIgnoreCase check and save that can allow duplicate keys differing only by case.
In findOrCreateTag, the code checks existsByKeyIgnoreCaseAndEntityType(key, DEVICE) and, if false, proceeds to save. Between the check and the save, another request could create a tag with a different case of the same key. The DuplicateKeyException catch block only guards against an exact-key race (the same case), because the underlying unique index is stated to be case-sensitive ('Keys are unique ignoring case ... the Mongo index is case-sensitive, so without the check "Site" would quietly become a second key next to "site"'). This means the case-insensitive uniqueness guarantee promised in the class doc is not actually race-safe: two concurrent requests with 'Site' and 'site' could both pass the exists check and both succeed at save (since the underlying index is case-sensitive), producing two case-variant keys — the exact failure mode the check claims to prevent.
The flagged code:
```
private Tag findOrCreateTag(String key, List<String> values) {
Tag existing = tagRepository.findByKeyAndEntityType(key, DEVICE);
if (existing == null) {
if (tagRepository.existsByKeyIgnoreCaseAndEntityType(key, DEVICE)) {
throw new ConflictException("Tag with key '" + key + "' already exists");
}
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;
}
}
}
```
Make the minimal change that resolves the finding; do not refactor unrelated code.
confidence: 35 — react 👍/👎 to teach the reviewer
There was a problem hiding this comment.
Correct, and knowingly left: the ignore-case check is best-effort, exactly like TagService.createTag which has the same check-then-save shape. The exact-case race is closed by the unique index (hence the DuplicateKeyException handling); a Site/site race within the same few milliseconds is not, and the outcome would be a second key an admin can delete — no data loss. The real fix is a case-insensitive collation on tenant_key_entity_idx, which is an index migration across every tenant DB and would cover createTag/updateTag/registration too, so it belongs in its own change.
3 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 three 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.setDeviceTagValuesreplaces the device's values for a key with exactly the list sent (create-if-missing, like assign). This is how one value is taken off a device; null/empty keeps the key on the device as a plain label. Only the assignment is replaced — the key's predefined options stay additive, since a dropped value may still be in use elsewhere. An unchanged list is not saved, so no Pinot republish.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.@Transactionalon the service: noMongoTransactionManageris configured anywhere, so it was a no-op. The tag key is written before the assignment, so a failure in between leaves at worst an unused key.DeviceTagServiceTest(create-on-first-use, additive merge, replace / single-value removal, idempotency, case-variant conflict, validation, removal through the aspect-intercepted delete) and the three mutations inDeviceDataFetcherTest.TagService.createTag: assigningSitewhilesiteexists is aConflictExceptioninstead of a second key.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.