[Backend] KnowledgeDomainRegistry.ActionIdsByKey is write-only — dead code, or a missing action key→id accessor? #660

New Issue

2026-07-17T16:03:30Z

spikerj commented

2026-07-17 16:03:30 +00:00

Found via SonarQube S1450 triage (2026-07-17), verified in source. The static ctor of KnowledgeDomainRegistry builds a full forward lookup 'ActionIdsByKey' (Dictionary<domainKey, Dictionary<actionKey,int>>): allocated (line 817), populated per domain (847, building an inner actionMap per domain). But it is NEVER READ anywhere in the codebase.

The REVERSE map, ActionKeysById (id→key), IS used — line 888 does the id→key decode (ActionKeysById[domain].TryGetValue(actionId, out key)). So there's an asymmetry: the manifest system assigns numeric action ids and can DECODE id→key, but the ENCODE direction (action-key → numeric id, which ActionIdsByKey would serve) is built and thrown away.

Two possibilities, needs a design call:

Dead code: nothing needs key→id encoding; remove ActionIdsByKey + its population (the 'var actionMap' building at ~838 and the two ActionIdsByKey lines) to drop wasted startup allocation. Safe (verified no reads).
Missing accessor: something SHOULD encode action keys → ids (e.g. when writing activity events with compact numeric ids) and currently does it another way or not at all — in which case a public GetActionId(domain, actionKey) that reads ActionIdsByKey is the fix.

Low severity (wasted startup work, no correctness bug). Flagging rather than removing blind, since deleting the forward map could remove intended encode infrastructure. The S1450 hotspot was reopened in SonarQube pending this decision.

Found via SonarQube S1450 triage (2026-07-17), verified in source. The static ctor of KnowledgeDomainRegistry builds a full forward lookup 'ActionIdsByKey' (Dictionary<domainKey, Dictionary<actionKey,int>>): allocated (line 817), populated per domain (847, building an inner actionMap per domain). But it is NEVER READ anywhere in the codebase. The REVERSE map, ActionKeysById (id→key), IS used — line 888 does the id→key decode (ActionKeysById[domain].TryGetValue(actionId, out key)). So there's an asymmetry: the manifest system assigns numeric action ids and can DECODE id→key, but the ENCODE direction (action-key → numeric id, which ActionIdsByKey would serve) is built and thrown away. Two possibilities, needs a design call: 1. **Dead code:** nothing needs key→id encoding; remove ActionIdsByKey + its population (the 'var actionMap' building at ~838 and the two ActionIdsByKey lines) to drop wasted startup allocation. Safe (verified no reads). 2. **Missing accessor:** something SHOULD encode action keys → ids (e.g. when writing activity events with compact numeric ids) and currently does it another way or not at all — in which case a public GetActionId(domain, actionKey) that reads ActionIdsByKey is the fix. Low severity (wasted startup work, no correctness bug). Flagging rather than removing blind, since deleting the forward map could remove intended encode infrastructure. The S1450 hotspot was reopened in SonarQube pending this decision.

spikerj commented

2026-07-18 18:12:50 +00:00

Design call made: option 2 — missing accessor, not dead code. Fix up as backend PR #414.

Deciding evidence: the encode direction is documented, existing public API at the domain level — TryGetDomainId ("forward lookup used by the optional server-side encoder for outbound recent-activity feeds") — and decode (TryGetActionKey) is live in ActivityController + the DTO converter. The action-level encode map was built with no accessor: the asymmetry was an omission, not intent. Deleting it would strand TryGetDomainId and cut against the registry's versioned-manifest design for a trivial startup saving.

Added TryGetActionId(domain, actionKey) (alias-resolving, null on unknown) + KnowledgeDomainRegistryTests with a full-manifest encode/decode round-trip proof — which also gives TryGetDomainId/TryGetDomainKey their first direct coverage. Business.Tests 7595 green. The reopened S1450 hotspot is satisfied (map now read). Will close on merge.

**Design call made: option 2 — missing accessor, not dead code.** Fix up as backend PR [#414](https://git.spikersoft.com/spikerj/spikersoft-backend/pulls/414). Deciding evidence: the encode direction is *documented, existing public API* at the domain level — `TryGetDomainId` ("forward lookup used by the optional server-side encoder for outbound recent-activity feeds") — and decode (`TryGetActionKey`) is live in `ActivityController` + the DTO converter. The action-level encode map was built with no accessor: the asymmetry was an omission, not intent. Deleting it would strand `TryGetDomainId` and cut against the registry's versioned-manifest design for a trivial startup saving. Added `TryGetActionId(domain, actionKey)` (alias-resolving, null on unknown) + `KnowledgeDomainRegistryTests` with a full-manifest encode/decode round-trip proof — which also gives `TryGetDomainId`/`TryGetDomainKey` their first direct coverage. Business.Tests 7595 green. The reopened S1450 hotspot is satisfied (map now read). Will close on merge.

spikerj commented

2026-07-18 18:17:20 +00:00

Resolved in backend PR #414 (merged to master 2026-07-18). TryGetActionId encode accessor added, completing the manifest id symmetry; full-manifest round-trip test proves encode/decode consistency. S1450 hotspot satisfied. Closing.

Resolved in backend PR [#414](https://git.spikersoft.com/spikerj/spikersoft-backend/pulls/414) (merged to `master` 2026-07-18). `TryGetActionId` encode accessor added, completing the manifest id symmetry; full-manifest round-trip test proves encode/decode consistency. S1450 hotspot satisfied. Closing.

spikerj closed this issue

2026-07-18 18:17:20 +00:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: spikerj/spikersoft-issues#660