carverauto/serviceradar

Fork 0

registry update #2528

Merged

mfreeman451 merged 2 commits from refs/pull/2528/head into main

2025-12-08 16:09:43 +00:00

mfreeman451 commented

2025-12-08 07:02:35 +00:00

(Migrated from github.com)

Owner

Imported from GitHub pull request.

Original GitHub pull request: #2085
Original author: @mfreeman451
Original URL: https://github.com/carverauto/serviceradar/pull/2085
Original created: 2025-12-08T07:02:35Z
Original updated: 2025-12-08T16:09:52Z
Original head: carverauto/serviceradar:bug/device_count_regression
Original base: main
Original merged: 2025-12-08T16:09:43Z by @mfreeman451

User description

IMPORTANT: Please sign the Developer Certificate of Origin

Thank you for your contribution to ServiceRadar. Please note, when contributing, the developer must include
a DCO sign-off statement indicating the DCO acceptance in one commit message. Here
is an example DCO Signed-off-by line in a commit message:

Signed-off-by: J. Doe <j.doe@domain.com>

Describe your changes

Issue ticket number and link

Code checklist before requesting a review

I have signed the DCO?
The build completes without errors?
All tests are passing when running make test?

PR Type

Bug fix

Description

Fix device count regression by ensuring canonical_device_id metadata consistency
Set canonical_device_id in metadata after IdentityEngine resolves device IDs
Prevents stats aggregator from filtering devices as non-canonical
Resolves fluctuating device count issue in registry

Diagram Walkthrough

flowchart LR
  A["IdentityEngine<br/>ResolveDeviceIDs"] -->|Updates DeviceID| B["ProcessBatchDeviceUpdates"]
  B -->|Sets canonical_device_id<br/>in metadata| C["Stats Aggregator<br/>isCanonicalRecord"]
  C -->|Correctly identifies<br/>canonical records| D["Accurate Device Count"]

File Walkthrough

Relevant files

Bug fix

registry.go

Add canonical_device_id metadata synchronization

pkg/registry/registry.go

Added metadata synchronization loop after identity resolution
Sets canonical_device_id field to match resolved DeviceID
Initializes metadata map if nil before setting the field
Ensures stats aggregator can correctly identify canonical records

+13/-0

Documentation

proposal.md

Document canonical_device_id metadata bug fix

openspec/changes/fix-dire-engine/proposal.md

Documented completed bug fix for canonical_device_id metadata
consistency
Explained the root cause of device count fluctuation
Recorded the fix location and implementation details
Marked issue as resolved with checkmark

+7/-0

Imported from GitHub pull request. Original GitHub pull request: #2085 Original author: @mfreeman451 Original URL: https://github.com/carverauto/serviceradar/pull/2085 Original created: 2025-12-08T07:02:35Z Original updated: 2025-12-08T16:09:52Z Original head: carverauto/serviceradar:bug/device_count_regression Original base: main Original merged: 2025-12-08T16:09:43Z by @mfreeman451 --- ### **User description** ## IMPORTANT: Please sign the Developer Certificate of Origin Thank you for your contribution to ServiceRadar. Please note, when contributing, the developer must include a [DCO sign-off statement]( https://developercertificate.org/) indicating the DCO acceptance in one commit message. Here is an example DCO Signed-off-by line in a commit message: ``` Signed-off-by: J. Doe <j.doe@domain.com> ``` ## Describe your changes ## Issue ticket number and link ## Code checklist before requesting a review - [ ] I have signed the DCO? - [ ] The build completes without errors? - [ ] All tests are passing when running make test? ___ ### **PR Type** Bug fix ___ ### **Description** - Fix device count regression by ensuring canonical_device_id metadata consistency - Set canonical_device_id in metadata after IdentityEngine resolves device IDs - Prevents stats aggregator from filtering devices as non-canonical - Resolves fluctuating device count issue in registry ___ ### Diagram Walkthrough ```mermaid flowchart LR A["IdentityEngine<br/>ResolveDeviceIDs"] -->|Updates DeviceID| B["ProcessBatchDeviceUpdates"] B -->|Sets canonical_device_id<br/>in metadata| C["Stats Aggregator<br/>isCanonicalRecord"] C -->|Correctly identifies<br/>canonical records| D["Accurate Device Count"] ``` <details><summary><h3>File Walkthrough</h3></summary> <table><thead><tr><th></th><th align="left">Relevant files</th></tr></thead><tbody><tr><td><strong>Bug fix</strong></td><td><table> <tr> <td> <details> <summary><strong>registry.go</strong><dd><code>Add canonical_device_id metadata synchronization</code>                  </dd></summary> <hr> pkg/registry/registry.go <ul><li>Added metadata synchronization loop after identity resolution<br> <li> Sets <code>canonical_device_id</code> field to match resolved <code>DeviceID</code><br> <li> Initializes metadata map if nil before setting the field<br> <li> Ensures stats aggregator can correctly identify canonical records</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2085/files#diff-cb61d8f79451b9541de4a8cc0811523a68d15452b2f5971c7618ea5b423cf4ec">+13/-0</a>    </td> </tr> </table></td></tr><tr><td><strong>Documentation</strong></td><td><table> <tr> <td> <details> <summary><strong>proposal.md</strong><dd><code>Document canonical_device_id metadata bug fix</code>                        </dd></summary> <hr> openspec/changes/fix-dire-engine/proposal.md <ul><li>Documented completed bug fix for canonical_device_id metadata <br>consistency<br> <li> Explained the root cause of device count fluctuation<br> <li> Recorded the fix location and implementation details<br> <li> Marked issue as resolved with checkmark</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2085/files#diff-5ee57f76af37fa8150a9d3a2584a2b21c46699c982a27f7ef641bdff365c6fc8">+7/-0</a>      </td> </tr> </table></td></tr></tbody></table> </details> ___

qodo-code-review[bot] commented

2025-12-08 07:03:08 +00:00

(Migrated from github.com)

Author

Owner

Imported GitHub PR comment.

Original author: @qodo-code-review[bot]
Original URL: https://github.com/carverauto/serviceradar/pull/2085#issuecomment-3625312315
Original created: 2025-12-08T07:03:08Z

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢	No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
⚪	🎫 No ticket provided Create ticket/issue
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
🟢	Generic: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
⚪	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: Missing audit logs: The newly added metadata synchronization for canonical_device_id performs a critical state change without explicit audit logging of the action, actor, or outcome. Referred Code `// Ensure canonical_device_id metadata matches the resolved device ID. // This is required for the stats aggregator to correctly count devices. for _, u := range valid { if u == nil \|\| u.DeviceID == "" { continue } if u.Metadata == nil { u.Metadata = make(map[string]string) } // Always set canonical_device_id to match the resolved device ID u.Metadata["canonical_device_id"] = u.DeviceID }` Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: No failure path: The loop that sets canonical_device_id does not handle or log potential issues (e.g., conflicting existing metadata values) or verify post-conditions, reducing robustness. Referred Code `// Ensure canonical_device_id metadata matches the resolved device ID. // This is required for the stats aggregator to correctly count devices. for _, u := range valid { if u == nil \|\| u.DeviceID == "" { continue } if u.Metadata == nil { u.Metadata = make(map[string]string) } // Always set canonical_device_id to match the resolved device ID u.Metadata["canonical_device_id"] = u.DeviceID }` Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: Metadata trust: The code writes canonical_device_id into user/device metadata without validating existing values or source trust, which may have security implications depending on metadata provenance. Referred Code `if u.Metadata == nil { u.Metadata = make(map[string]string) } // Always set canonical_device_id to match the resolved device ID u.Metadata["canonical_device_id"] = u.DeviceID }` Learn more about managing compliance generic rules or creating your own custom rules
Update

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/2085#issuecomment-3625312315 Original created: 2025-12-08T07:03:08Z --- ## PR Compliance Guide 🔍  Below is a summary of compliance checks for this PR:<br> <table><tbody><tr><td colspan='2'><strong>Security Compliance</strong></td></tr> <tr><td>🟢</td><td><details><summary><strong>No security concerns identified</strong></summary> No security vulnerabilities detected by AI analysis. Human verification advised for critical code. </details></td></tr> <tr><td colspan='2'><strong>Ticket Compliance</strong></td></tr> <tr><td>⚪</td><td><details><summary>🎫 <strong>No ticket provided </strong></summary> - [ ] Create ticket/issue  </details></td></tr> <tr><td colspan='2'><strong>Codebase Duplication Compliance</strong></td></tr> <tr><td>⚪</td><td><details><summary><strong>Codebase context is not defined </strong></summary> Follow the <a href='https://qodo-merge-docs.qodo.ai/core-abilities/rag_context_enrichment/'>guide</a> to enable codebase context checks. </details></td></tr> <tr><td colspan='2'><strong>Custom Compliance</strong></td></tr> <tr><td rowspan=3>🟢</td><td> <details><summary><strong>Generic: Meaningful Naming and Self-Documenting Code</strong></summary><br> **Objective:** Ensure all identifiers clearly express their purpose and intent, making code <br>self-documenting<br> **Status:** Passed<br> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td> <details><summary><strong>Generic: Secure Error Handling</strong></summary><br> **Objective:** To prevent the leakage of sensitive system information through error messages while <br>providing sufficient detail for internal debugging.<br> **Status:** Passed<br> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td> <details><summary><strong>Generic: Secure Logging Practices</strong></summary><br> **Objective:** To ensure logs are useful for debugging and auditing without exposing sensitive <br>information like PII, PHI, or cardholder data.<br> **Status:** Passed<br> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td rowspan=3>⚪</td> <td><details> <summary><strong>Generic: Comprehensive Audit Trails</strong></summary><br> **Objective:** To create a detailed and reliable record of critical system actions for security analysis <br>and compliance.<br> **Status:** <br><a href='https://github.com/carverauto/serviceradar/pull/2085/files#diff-cb61d8f79451b9541de4a8cc0811523a68d15452b2f5971c7618ea5b423cf4ecR251-R262'><strong>Missing audit logs</strong></a>: The newly added metadata synchronization for canonical_device_id performs a critical state <br>change without explicit audit logging of the action, actor, or outcome.<br> <details open><summary>Referred Code</summary> ```go // Ensure canonical_device_id metadata matches the resolved device ID. // This is required for the stats aggregator to correctly count devices. for _, u := range valid { if u == nil || u.DeviceID == "" { continue } if u.Metadata == nil { u.Metadata = make(map[string]string) } // Always set canonical_device_id to match the resolved device ID u.Metadata["canonical_device_id"] = u.DeviceID } ``` </details> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td><details> <summary><strong>Generic: Robust Error Handling and Edge Case Management</strong></summary><br> **Objective:** Ensure comprehensive error handling that provides meaningful context and graceful <br>degradation<br> **Status:** <br><a href='https://github.com/carverauto/serviceradar/pull/2085/files#diff-cb61d8f79451b9541de4a8cc0811523a68d15452b2f5971c7618ea5b423cf4ecR251-R262'><strong>No failure path</strong></a>: The loop that sets canonical_device_id does not handle or log potential issues (e.g., <br>conflicting existing metadata values) or verify post-conditions, reducing robustness.<br> <details open><summary>Referred Code</summary> ```go // Ensure canonical_device_id metadata matches the resolved device ID. // This is required for the stats aggregator to correctly count devices. for _, u := range valid { if u == nil || u.DeviceID == "" { continue } if u.Metadata == nil { u.Metadata = make(map[string]string) } // Always set canonical_device_id to match the resolved device ID u.Metadata["canonical_device_id"] = u.DeviceID } ``` </details> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td><details> <summary><strong>Generic: Security-First Input Validation and Data Handling</strong></summary><br> **Objective:** Ensure all data inputs are validated, sanitized, and handled securely to prevent <br>vulnerabilities<br> **Status:** <br><a href='https://github.com/carverauto/serviceradar/pull/2085/files#diff-cb61d8f79451b9541de4a8cc0811523a68d15452b2f5971c7618ea5b423cf4ecR257-R262'><strong>Metadata trust</strong></a>: The code writes canonical_device_id into user/device metadata without validating existing <br>values or source trust, which may have security implications depending on metadata <br>provenance.<br> <details open><summary>Referred Code</summary> ```go if u.Metadata == nil { u.Metadata = make(map[string]string) } // Always set canonical_device_id to match the resolved device ID u.Metadata["canonical_device_id"] = u.DeviceID } ``` </details> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td align="center" colspan="2"> - [ ] Update  </td></tr></tbody></table> <details><summary>Compliance status legend</summary> 🟢 - Fully Compliant<br> 🟡 - Partial Compliant<br> 🔴 - Not Compliant<br> ⚪ - Requires Further Human Verification<br> 🏷️ - Compliance label<br> </details>

qodo-code-review[bot] commented

2025-12-08 07:03:57 +00:00

(Migrated from github.com)

Author

Owner

Imported GitHub PR comment.

Original author: @qodo-code-review[bot]
Original URL: https://github.com/carverauto/serviceradar/pull/2085#issuecomment-3625314576
Original created: 2025-12-08T07:03:57Z

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	✅ ~~Prevent setting incorrect canonical IDs~~ Suggestion Impact: The commit refactored the metadata update into a helper function and invoked it right after the ResolveDeviceIDs call. While not placed strictly in an else block, this change centralizes the logic; however, based on the shown diff, it does not conditionally execute only on success. Still, the change directly relates to the suggested area by modifying how canonical_device_id is set. code diff: @@ -247,19 +247,7 @@ if err := r.identityEngine.ResolveDeviceIDs(ctx, valid); err != nil { r.logger.Warn().Err(err).Msg("Device identity resolution failed") } - - // Ensure canonical_device_id metadata matches the resolved device ID. - // This is required for the stats aggregator to correctly count devices. - for _, u := range valid { - if u == nil \|\| u.DeviceID == "" { - continue - } - if u.Metadata == nil { - u.Metadata = make(map[string]string) - } - // Always set canonical_device_id to match the resolved device ID - u.Metadata["canonical_device_id"] = u.DeviceID - } + ensureCanonicalDeviceIDMetadata(valid) To prevent data integrity issues, execute the metadata synchronization logic for `canonical_device_id` only if the `ResolveDeviceIDs` function call succeeds, by placing it in an `else` block. pkg/registry/registry.go [247-262] if err := r.identityEngine.ResolveDeviceIDs(ctx, valid); err != nil { r.logger.Warn().Err(err).Msg("Device identity resolution failed") + } else { + // Ensure canonical_device_id metadata matches the resolved device ID. + // This is required for the stats aggregator to correctly count devices. + for _, u := range valid { + if u == nil \|\| u.DeviceID == "" { + continue + } + if u.Metadata == nil { + u.Metadata = make(map[string]string) + } + // Always set canonical_device_id to match the resolved device ID + u.Metadata["canonical_device_id"] = u.DeviceID + } } - // Ensure canonical_device_id metadata matches the resolved device ID. - // This is required for the stats aggregator to correctly count devices. - for _, u := range valid { - if u == nil \|\| u.DeviceID == "" { - continue - } - if u.Metadata == nil { - u.Metadata = make(map[string]string) - } - // Always set canonical_device_id to match the resolved device ID - u.Metadata["canonical_device_id"] = u.DeviceID - } - `[Suggestion processed]` Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies a potential data integrity issue where `canonical_device_id` could be set incorrectly if `ResolveDeviceIDs` fails, and proposes a valid fix.	Medium
High-level	Encapsulate metadata update within IdentityEngine Move the `canonical_device_id` metadata update logic from `ProcessBatchDeviceUpdates` into the `IdentityEngine.ResolveDeviceIDs` method. This change would encapsulate the logic, improve the API design, and reduce the chance of similar bugs in the future. Examples: pkg/registry/registry.go [251-262] `// Ensure canonical_device_id metadata matches the resolved device ID. // This is required for the stats aggregator to correctly count devices. for _, u := range valid { if u == nil \|\| u.DeviceID == "" { continue } if u.Metadata == nil { u.Metadata = make(map[string]string) } // Always set canonical_device_id to match the resolved device ID ... (clipped 2 lines)` Solution Walkthrough: Before: `// in pkg/registry/registry.go:ProcessBatchDeviceUpdates func (r DeviceRegistry) ProcessBatchDeviceUpdates(ctx context.Context, updates []models.DeviceUpdate) error { // ... if r.identityEngine != nil { if err := r.identityEngine.ResolveDeviceIDs(ctx, valid); err != nil { // ... } // Caller must remember to sync metadata for _, u := range valid { if u.Metadata == nil { u.Metadata = make(map[string]string) } u.Metadata["canonical_device_id"] = u.DeviceID } } // ... }` After: // in pkg/registry/registry.go:ProcessBatchDeviceUpdates func (r DeviceRegistry) ProcessBatchDeviceUpdates(ctx context.Context, updates []models.DeviceUpdate) error { // ... if r.identityEngine != nil { // Logic is now encapsulated within the engine's method. if err := r.identityEngine.ResolveDeviceIDs(ctx, valid); err != nil { // ... } } // ... } // in IdentityEngine (conceptual) func (ie IdentityEngine) ResolveDeviceIDs(ctx context.Context, updates []models.DeviceUpdate) error { // ... existing resolution logic ... for _, u := range updates { // ... logic to resolve and set u.DeviceID ... if u.Metadata == nil { u.Metadata = make(map[string]string) } u.Metadata["canonical_device_id"] = u.DeviceID } return nil } Suggestion importance[1-10]: 7 __ Why: This is a strong design suggestion that correctly identifies a leaky abstraction; encapsulating the logic within `IdentityEngine` would make the API more robust and prevent future bugs.	Medium
Update

Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/2085#issuecomment-3625314576 Original created: 2025-12-08T07:03:57Z --- ## PR Code Suggestions ✨  Explore these optional code suggestions: <table><thead><tr><td><strong>Category</strong></td><td align=left><strong>Suggestion                                                                                                                                    </strong></td><td align=center><strong>Impact</strong></td></tr><tbody><tr><td rowspan=1>Possible issue</td> <td> <details><summary>✅ <s>Prevent setting incorrect canonical IDs</s></summary> ___ <details><summary><b>Suggestion Impact:</b></summary>The commit refactored the metadata update into a helper function and invoked it right after the ResolveDeviceIDs call. While not placed strictly in an else block, this change centralizes the logic; however, based on the shown diff, it does not conditionally execute only on success. Still, the change directly relates to the suggested area by modifying how canonical_device_id is set. code diff: ```diff @@ -247,19 +247,7 @@ if err := r.identityEngine.ResolveDeviceIDs(ctx, valid); err != nil { r.logger.Warn().Err(err).Msg("Device identity resolution failed") } - - // Ensure canonical_device_id metadata matches the resolved device ID. - // This is required for the stats aggregator to correctly count devices. - for _, u := range valid { - if u == nil || u.DeviceID == "" { - continue - } - if u.Metadata == nil { - u.Metadata = make(map[string]string) - } - // Always set canonical_device_id to match the resolved device ID - u.Metadata["canonical_device_id"] = u.DeviceID - } + ensureCanonicalDeviceIDMetadata(valid) ``` </details> ___ **To prevent data integrity issues, execute the metadata synchronization logic for <br><code>canonical_device_id</code> only if the <code>ResolveDeviceIDs</code> function call succeeds, by <br>placing it in an <code>else</code> block.** [pkg/registry/registry.go [247-262]](https://github.com/carverauto/serviceradar/pull/2085/files#diff-cb61d8f79451b9541de4a8cc0811523a68d15452b2f5971c7618ea5b423cf4ecR247-R262) ```diff if err := r.identityEngine.ResolveDeviceIDs(ctx, valid); err != nil { r.logger.Warn().Err(err).Msg("Device identity resolution failed") + } else { + // Ensure canonical_device_id metadata matches the resolved device ID. + // This is required for the stats aggregator to correctly count devices. + for _, u := range valid { + if u == nil || u.DeviceID == "" { + continue + } + if u.Metadata == nil { + u.Metadata = make(map[string]string) + } + // Always set canonical_device_id to match the resolved device ID + u.Metadata["canonical_device_id"] = u.DeviceID + } } - // Ensure canonical_device_id metadata matches the resolved device ID. - // This is required for the stats aggregator to correctly count devices. - for _, u := range valid { - if u == nil || u.DeviceID == "" { - continue - } - if u.Metadata == nil { - u.Metadata = make(map[string]string) - } - // Always set canonical_device_id to match the resolved device ID - u.Metadata["canonical_device_id"] = u.DeviceID - } - ``` `[Suggestion processed]` <details><summary>Suggestion importance[1-10]: 8</summary> __ Why: The suggestion correctly identifies a potential data integrity issue where `canonical_device_id` could be set incorrectly if `ResolveDeviceIDs` fails, and proposes a valid fix. </details></details></td><td align=center>Medium </td></tr><tr><td rowspan=1>High-level</td> <td> <details><summary>Encapsulate metadata update within IdentityEngine</summary> ___ **Move the <code>canonical_device_id</code> metadata update logic from <br><code>ProcessBatchDeviceUpdates</code> into the <code>IdentityEngine.ResolveDeviceIDs</code> method. This <br>change would encapsulate the logic, improve the API design, and reduce the <br>chance of similar bugs in the future.** ### Examples: <details> <summary> <a href="https://github.com/carverauto/serviceradar/pull/2085/files#diff-cb61d8f79451b9541de4a8cc0811523a68d15452b2f5971c7618ea5b423cf4ecR251-R262">pkg/registry/registry.go [251-262]</a> </summary> ```go // Ensure canonical_device_id metadata matches the resolved device ID. // This is required for the stats aggregator to correctly count devices. for _, u := range valid { if u == nil || u.DeviceID == "" { continue } if u.Metadata == nil { u.Metadata = make(map[string]string) } // Always set canonical_device_id to match the resolved device ID ... (clipped 2 lines) ``` </details> ### Solution Walkthrough: #### Before: ```go // in pkg/registry/registry.go:ProcessBatchDeviceUpdates func (r *DeviceRegistry) ProcessBatchDeviceUpdates(ctx context.Context, updates []*models.DeviceUpdate) error { // ... if r.identityEngine != nil { if err := r.identityEngine.ResolveDeviceIDs(ctx, valid); err != nil { // ... } // Caller must remember to sync metadata for _, u := range valid { if u.Metadata == nil { u.Metadata = make(map[string]string) } u.Metadata["canonical_device_id"] = u.DeviceID } } // ... } ``` #### After: ```go // in pkg/registry/registry.go:ProcessBatchDeviceUpdates func (r *DeviceRegistry) ProcessBatchDeviceUpdates(ctx context.Context, updates []*models.DeviceUpdate) error { // ... if r.identityEngine != nil { // Logic is now encapsulated within the engine's method. if err := r.identityEngine.ResolveDeviceIDs(ctx, valid); err != nil { // ... } } // ... } // in IdentityEngine (conceptual) func (ie *IdentityEngine) ResolveDeviceIDs(ctx context.Context, updates []*models.DeviceUpdate) error { // ... existing resolution logic ... for _, u := range updates { // ... logic to resolve and set u.DeviceID ... if u.Metadata == nil { u.Metadata = make(map[string]string) } u.Metadata["canonical_device_id"] = u.DeviceID } return nil } ``` <details><summary>Suggestion importance[1-10]: 7</summary> __ Why: This is a strong design suggestion that correctly identifies a leaky abstraction; encapsulating the logic within `IdentityEngine` would make the API more robust and prevent future bugs. </details></details></td><td align=center>Medium </td></tr> <tr><td align="center" colspan="2"> - [ ] Update  </td><td></td></tr></tbody></table>