initial #2579

Merged
mfreeman451 merged 1 commit from refs/pull/2579/head into staging 2025-12-16 05:46:02 +00:00
mfreeman451 commented 2025-12-16 05:42:04 +00:00 (Migrated from github.com)
Owner

Imported from GitHub pull request.

Original GitHub pull request: #2155
Original author: @mfreeman451
Original URL: https://github.com/carverauto/serviceradar/pull/2155
Original created: 2025-12-16T05:42:04Z
Original updated: 2025-12-16T05:46:05Z
Original head: carverauto/serviceradar:2140-batch-identifier-lookup-ignores-partition-merging-devices-across-tenants
Original base: staging
Original merged: 2025-12-16T05:46:02Z 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

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

  • Add partition parameter to batch identifier lookup to prevent cross-tenant device merging

  • Group batch updates by partition before querying to maintain isolation

  • Update database layer SQL and API to filter identifiers by partition

  • Add regression test for mixed-partition batch identifier resolution


Diagram Walkthrough

flowchart LR
  A["Batch Device Updates"] -->|Group by Partition| B["Per-Partition Update Groups"]
  B -->|Query with Partition Filter| C["Database Layer"]
  C -->|Return Partition-Scoped Results| D["Identity Matches"]
  D -->|Map to Correct Devices| E["Partition-Isolated Device IDs"]

File Walkthrough

Relevant files
Bug fix
3 files
cnpg_identity_engine.go
Add partition filter to batch identifier SQL query             
+8/-3     
interfaces.go
Add partition parameter to batch lookup interface               
+1/-1     
identity_engine.go
Implement partition-grouped batch identifier lookup           
+124/-54
Tests
4 files
mock_db.go
Update mock to accept partition parameter                               
+6/-5     
canon_simulation_test.go
Update mock setup to use partition-aware cache keys           
+4/-4     
identity_engine_partition_test.go
Add regression test for partition-scoped batch lookups     
+51/-0   
registry_test.go
Update test mocks for new partition parameter                       
+5/-5     
Documentation
4 files
proposal.md
Document change rationale and impact                                         
+22/-0   
design.md
Document design decisions and alternatives                             
+32/-0   
spec.md
Add partition-scoped batch lookup requirements                     
+13/-0   
tasks.md
Document implementation tasks and checklist                           
+17/-0   

Imported from GitHub pull request. Original GitHub pull request: #2155 Original author: @mfreeman451 Original URL: https://github.com/carverauto/serviceradar/pull/2155 Original created: 2025-12-16T05:42:04Z Original updated: 2025-12-16T05:46:05Z Original head: carverauto/serviceradar:2140-batch-identifier-lookup-ignores-partition-merging-devices-across-tenants Original base: staging Original merged: 2025-12-16T05:46:02Z 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** - Add partition parameter to batch identifier lookup to prevent cross-tenant device merging - Group batch updates by partition before querying to maintain isolation - Update database layer SQL and API to filter identifiers by partition - Add regression test for mixed-partition batch identifier resolution ___ ### Diagram Walkthrough ```mermaid flowchart LR A["Batch Device Updates"] -->|Group by Partition| B["Per-Partition Update Groups"] B -->|Query with Partition Filter| C["Database Layer"] C -->|Return Partition-Scoped Results| D["Identity Matches"] D -->|Map to Correct Devices| E["Partition-Isolated Device IDs"] ``` <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><details><summary>3 files</summary><table> <tr> <td><strong>cnpg_identity_engine.go</strong><dd><code>Add partition filter to batch identifier SQL query</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2155/files#diff-756702fbe82153fa86ae9b6bd6e5b109901d5fb8ee27cb9bc19f51277d3632f0">+8/-3</a>&nbsp; &nbsp; &nbsp; </td> </tr> <tr> <td><strong>interfaces.go</strong><dd><code>Add partition parameter to batch lookup interface</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2155/files#diff-c230fe0c315251837357bfde4ae7f7b34080398d8e48af6bf78badb2124271f3">+1/-1</a>&nbsp; &nbsp; &nbsp; </td> </tr> <tr> <td><strong>identity_engine.go</strong><dd><code>Implement partition-grouped batch identifier lookup</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2155/files#diff-496f24b3784656e1d6bb97cafe5c528bbecea5a0b74b4be578d3b83e858038c7">+124/-54</a></td> </tr> </table></details></td></tr><tr><td><strong>Tests</strong></td><td><details><summary>4 files</summary><table> <tr> <td><strong>mock_db.go</strong><dd><code>Update mock to accept partition parameter</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2155/files#diff-30e38f888d4849fc40d7ebb1559c2a84c43aa8cd13b3b89fd7ec6cf873b243c7">+6/-5</a>&nbsp; &nbsp; &nbsp; </td> </tr> <tr> <td><strong>canon_simulation_test.go</strong><dd><code>Update mock setup to use partition-aware cache keys</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2155/files#diff-73b73409b3941af4ac860561c87772763feca734fd8ab597f36e5e4047c6bf3c">+4/-4</a>&nbsp; &nbsp; &nbsp; </td> </tr> <tr> <td><strong>identity_engine_partition_test.go</strong><dd><code>Add regression test for partition-scoped batch lookups</code>&nbsp; &nbsp; &nbsp; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2155/files#diff-77aee244beb622dec723a0a62d2d11785b5b76a9fe5df912ba559227d3f4ba70">+51/-0</a>&nbsp; &nbsp; </td> </tr> <tr> <td><strong>registry_test.go</strong><dd><code>Update test mocks for new partition parameter</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2155/files#diff-f010972d104404be52d2a8e6e784cb56e31194f90795a69571a12696bcbdc075">+5/-5</a>&nbsp; &nbsp; &nbsp; </td> </tr> </table></details></td></tr><tr><td><strong>Documentation</strong></td><td><details><summary>4 files</summary><table> <tr> <td><strong>proposal.md</strong><dd><code>Document change rationale and impact</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2155/files#diff-cd52b3fd553ef7792c277ff50c4e9a9e1f3499b62def55e845f1311f5da7eb0d">+22/-0</a>&nbsp; &nbsp; </td> </tr> <tr> <td><strong>design.md</strong><dd><code>Document design decisions and alternatives</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2155/files#diff-7120eaa6a5f458aa2e1649559057f77ecd7355c3eb9079a3cbe53fdfac97b93b">+32/-0</a>&nbsp; &nbsp; </td> </tr> <tr> <td><strong>spec.md</strong><dd><code>Add partition-scoped batch lookup requirements</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2155/files#diff-a212262a53d3ee2d68753329b05835cd20f2aa7671ffa686b4a6083f1d330256">+13/-0</a>&nbsp; &nbsp; </td> </tr> <tr> <td><strong>tasks.md</strong><dd><code>Document implementation tasks and checklist</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2155/files#diff-0b26ad240d4dcd68f67721a1352e54bcf78ee404d2ee167feca6675008466f61">+17/-0</a>&nbsp; &nbsp; </td> </tr> </table></details></td></tr></tbody></table> </details> ___
qodo-code-review[bot] commented 2025-12-16 05:42:38 +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/2155#issuecomment-3658936712
Original created: 2025-12-16T05:42:38Z

You are nearing your monthly Qodo Merge usage quota. For more information, please visit here.

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
🟢
🎫 #2140
🟢 Ensure batchLookupByStrongIdentifiers performs identifier resolution scoped to the
update's partition and does not match identifiers across partitions in a mixed-partition
batch.
Update the DB batch identifier lookup API to accept a partition parameter and filter
results by partition in SQL (e.g., add AND partition = ...).
Prevent cross-tenant device merging/data leakage caused by batch lookup returning device
IDs from the wrong partition when identifiers collide.
Add regression test coverage demonstrating that two updates in the same batch with the
same identifier value but different partitions resolve to different device IDs
(partition-correct).
Ensure empty partition values default consistently (treated as default) for batch
identifier lookups (matching single-lookup behavior).
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

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: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status: Passed

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:
Silent DB failure: The new batch lookup path silently ignores database errors (returns empty matches) without
logging or propagating context, which can mask production failures and lead to incorrect
identity resolution behavior.

Referred Code
results, err := e.db.BatchGetDeviceIDsByIdentifier(ctx, identifierType, values, partition)
if err != nil {
	return matches
}

Learn more about managing compliance generic rules or creating your own custom rules

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/2155#issuecomment-3658936712 Original created: 2025-12-16T05:42:38Z --- _You are nearing your monthly Qodo Merge usage quota. For more information, please visit [here](https://qodo-merge-docs.qodo.ai/installation/qodo_merge/#cloud-users)._ ## PR Compliance Guide 🔍 <!-- https://github.com/carverauto/serviceradar/commit/abacd4d28e0b26dac6222c02d80c6c0f0437f7b9 --> 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>🎫 <a href=https://github.com/carverauto/serviceradar/issues/2140>#2140</a></summary> <table width='100%'><tbody> <tr><td rowspan=5>🟢</td> <td>Ensure <code>batchLookupByStrongIdentifiers</code> performs identifier resolution scoped to the <br>update's <code>partition</code> and does not match identifiers across partitions in a mixed-partition <br>batch.<br></td></tr> <tr><td>Update the DB batch identifier lookup API to accept a <code>partition</code> parameter and filter <br>results by partition in SQL (e.g., add <code>AND partition = ...</code>).<br></td></tr> <tr><td>Prevent cross-tenant device merging/data leakage caused by batch lookup returning device <br>IDs from the wrong partition when identifiers collide.<br></td></tr> <tr><td>Add regression test coverage demonstrating that two updates in the same batch with the <br>same identifier value but different partitions resolve to different device IDs <br>(partition-correct).<br></td></tr> <tr><td>Ensure empty partition values default consistently (treated as <code>default</code>) for batch <br>identifier lookups (matching single-lookup behavior).<br></td></tr> </tbody></table> </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=5>🟢</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:** 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: 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> <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:** 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=1>🔴</td> <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/2155/files#diff-496f24b3784656e1d6bb97cafe5c528bbecea5a0b74b4be578d3b83e858038c7R630-R633'><strong>Silent DB failure</strong></a>: The new batch lookup path silently ignores database errors (returns empty matches) without <br>logging or propagating context, which can mask production failures and lead to incorrect <br>identity resolution behavior.<br> <details open><summary>Referred Code</summary> ```go results, err := e.db.BatchGetDeviceIDsByIdentifier(ctx, identifierType, values, partition) if err != nil { return matches } ``` </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"> <!-- placeholder --> <!-- /compliance --update_compliance=true --> </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-16 05:43:44 +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/2155#issuecomment-3658938745
Original created: 2025-12-16T05:43:44Z

You are nearing your monthly Qodo Merge usage quota. For more information, please visit here.

PR Code Suggestions

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Improve lookup logic for efficiency

Improve lookup efficiency by replacing the loop over ids.CacheKeys with a
sequence of if statements that check for identifiers in priority order. This
makes the logic more direct and avoids unnecessary slice allocation.

pkg/registry/identity_engine.go [563-568]

-	for _, key := range ids.CacheKeys {
-		if deviceID := identifierToDevice[key]; deviceID != "" {
-			matches[update] = deviceID
-			break
+		if ids.ArmisID != "" {
+			if deviceID, ok := identifierToDevice[strongIdentifierCacheKey(ids.Partition, IdentifierTypeArmis, ids.ArmisID)]; ok {
+				matches[update] = deviceID
+				continue
+			}
 		}
-	}
+		if ids.IntegrationID != "" {
+			if deviceID, ok := identifierToDevice[strongIdentifierCacheKey(ids.Partition, IdentifierTypeIntegration, ids.IntegrationID)]; ok {
+				matches[update] = deviceID
+				continue
+			}
+		}
+		if ids.NetboxID != "" {
+			if deviceID, ok := identifierToDevice[strongIdentifierCacheKey(ids.Partition, IdentifierTypeNetbox, ids.NetboxID)]; ok {
+				matches[update] = deviceID
+				continue
+			}
+		}
+		if ids.MAC != "" {
+			if deviceID, ok := identifierToDevice[strongIdentifierCacheKey(ids.Partition, IdentifierTypeMAC, ids.MAC)]; ok {
+				matches[update] = deviceID
+				continue
+			}
+		}
  • Apply / Chat
Suggestion importance[1-10]: 4

__

Why: The suggestion proposes replacing a loop over pre-computed cache keys with a series of if statements. This is functionally equivalent and maintains the required priority order, but makes the priority explicit and avoids the overhead of creating the ids.CacheKeys slice.

Low
Refactor loop for improved readability

Refactor the loop over identifier types to use a map instead of a slice of
structs. This can improve readability and slightly reduce allocation overhead.

pkg/registry/identity_engine.go [543-555]

-	for _, entry := range []struct {
-		idType string
-		values map[string]struct{}
-	}{
-		{IdentifierTypeArmis, identifierSets.armisIDs},
-		{IdentifierTypeIntegration, identifierSets.integrationIDs},
-		{IdentifierTypeNetbox, identifierSets.netboxIDs},
-		{IdentifierTypeMAC, identifierSets.macs},
+	for idType, values := range map[string]map[string]struct{}{
+		IdentifierTypeArmis:       identifierSets.armisIDs,
+		IdentifierTypeIntegration: identifierSets.integrationIDs,
+		IdentifierTypeNetbox:      identifierSets.netboxIDs,
+		IdentifierTypeMAC:         identifierSets.macs,
 	} {
-		for key, deviceID := range e.batchLookupIdentifierType(ctx, entry.idType, entry.values, partition) {
+		for key, deviceID := range e.batchLookupIdentifierType(ctx, idType, values, partition) {
 			identifierToDevice[key] = deviceID
 		}
 	}
  • Apply / Chat
Suggestion importance[1-10]: 3

__

Why: The suggestion correctly identifies that iterating over a map is a valid alternative to a slice of structs, as the order of populating the identifierToDevice map does not affect the final logic. This is a minor stylistic improvement that could enhance readability.

Low
  • More
Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/2155#issuecomment-3658938745 Original created: 2025-12-16T05:43:44Z --- _You are nearing your monthly Qodo Merge usage quota. For more information, please visit [here](https://qodo-merge-docs.qodo.ai/installation/qodo_merge/#cloud-users)._ ## PR Code Suggestions ✨ <!-- abacd4d --> Explore these optional code suggestions: <table><thead><tr><td><strong>Category</strong></td><td align=left><strong>Suggestion&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </strong></td><td align=center><strong>Impact</strong></td></tr><tbody><tr><td rowspan=2>General</td> <td> <details><summary>Improve lookup logic for efficiency</summary> ___ **Improve lookup efficiency by replacing the loop over <code>ids.CacheKeys</code> with a <br>sequence of <code>if</code> statements that check for identifiers in priority order. This <br>makes the logic more direct and avoids unnecessary slice allocation.** [pkg/registry/identity_engine.go [563-568]](https://github.com/carverauto/serviceradar/pull/2155/files#diff-496f24b3784656e1d6bb97cafe5c528bbecea5a0b74b4be578d3b83e858038c7R563-R568) ```diff - for _, key := range ids.CacheKeys { - if deviceID := identifierToDevice[key]; deviceID != "" { - matches[update] = deviceID - break + if ids.ArmisID != "" { + if deviceID, ok := identifierToDevice[strongIdentifierCacheKey(ids.Partition, IdentifierTypeArmis, ids.ArmisID)]; ok { + matches[update] = deviceID + continue + } } - } + if ids.IntegrationID != "" { + if deviceID, ok := identifierToDevice[strongIdentifierCacheKey(ids.Partition, IdentifierTypeIntegration, ids.IntegrationID)]; ok { + matches[update] = deviceID + continue + } + } + if ids.NetboxID != "" { + if deviceID, ok := identifierToDevice[strongIdentifierCacheKey(ids.Partition, IdentifierTypeNetbox, ids.NetboxID)]; ok { + matches[update] = deviceID + continue + } + } + if ids.MAC != "" { + if deviceID, ok := identifierToDevice[strongIdentifierCacheKey(ids.Partition, IdentifierTypeMAC, ids.MAC)]; ok { + matches[update] = deviceID + continue + } + } ``` - [ ] **Apply / Chat** <!-- /improve --apply_suggestion=0 --> <details><summary>Suggestion importance[1-10]: 4</summary> __ Why: The suggestion proposes replacing a loop over pre-computed cache keys with a series of `if` statements. This is functionally equivalent and maintains the required priority order, but makes the priority explicit and avoids the overhead of creating the `ids.CacheKeys` slice. </details></details></td><td align=center>Low </td></tr><tr><td> <details><summary>Refactor loop for improved readability</summary> ___ **Refactor the loop over identifier types to use a map instead of a slice of <br>structs. This can improve readability and slightly reduce allocation overhead.** [pkg/registry/identity_engine.go [543-555]](https://github.com/carverauto/serviceradar/pull/2155/files#diff-496f24b3784656e1d6bb97cafe5c528bbecea5a0b74b4be578d3b83e858038c7R543-R555) ```diff - for _, entry := range []struct { - idType string - values map[string]struct{} - }{ - {IdentifierTypeArmis, identifierSets.armisIDs}, - {IdentifierTypeIntegration, identifierSets.integrationIDs}, - {IdentifierTypeNetbox, identifierSets.netboxIDs}, - {IdentifierTypeMAC, identifierSets.macs}, + for idType, values := range map[string]map[string]struct{}{ + IdentifierTypeArmis: identifierSets.armisIDs, + IdentifierTypeIntegration: identifierSets.integrationIDs, + IdentifierTypeNetbox: identifierSets.netboxIDs, + IdentifierTypeMAC: identifierSets.macs, } { - for key, deviceID := range e.batchLookupIdentifierType(ctx, entry.idType, entry.values, partition) { + for key, deviceID := range e.batchLookupIdentifierType(ctx, idType, values, partition) { identifierToDevice[key] = deviceID } } ``` - [ ] **Apply / Chat** <!-- /improve --apply_suggestion=1 --> <details><summary>Suggestion importance[1-10]: 3</summary> __ Why: The suggestion correctly identifies that iterating over a map is a valid alternative to a slice of structs, as the order of populating the `identifierToDevice` map does not affect the final logic. This is a minor stylistic improvement that could enhance readability. </details></details></td><td align=center>Low </td></tr> <tr><td align="center" colspan="2"> - [ ] More <!-- /improve --more_suggestions=true --> </td><td></td></tr></tbody></table>
Sign in to join this conversation.
No reviewers
No milestone
No project
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
carverauto/serviceradar!2579
No description provided.