carverauto/serviceradar

Fork 0

linted #2585

Merged

mfreeman451 merged 2 commits from refs/pull/2585/head into staging

2025-12-16 23:20:10 +00:00

mfreeman451 commented

2025-12-16 22:20:15 +00:00

(Migrated from github.com)

Owner

Imported from GitHub pull request.

Original GitHub pull request: #2161
Original author: @mfreeman451
Original URL: https://github.com/carverauto/serviceradar/pull/2161
Original created: 2025-12-16T22:20:15Z
Original updated: 2025-12-16T23:22:12Z
Original head: carverauto/serviceradar:2153-insertevents-closes-pgx-batchresults-without-reading-results-discarding-insert-errors
Original base: staging
Original merged: 2025-12-16T23:20:10Z 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 pgx batch error handling to surface per-command INSERT failures
Add sendBatchExecAll helper to consistently read all batch results
Update InsertEvents, StoreBatchUsers, DeleteDevices to use helper
Add comprehensive tests and design documentation for batch handling

Diagram Walkthrough

flowchart LR
  A["Batch Write Operations<br/>InsertEvents, StoreBatchUsers,<br/>DeleteDevices"] -->|"Previously: Close without reading"| B["Silent Error Loss<br/>Undetected data loss"]
  A -->|"Now: Use sendBatchExecAll"| C["Read All Results<br/>br.Exec() for each command"]
  C -->|"Defer Close"| D["Always Close BatchResults<br/>Even on error"]
  D -->|"Return Error"| E["Surface Per-Command Errors<br/>With operation + index context"]

File Walkthrough

Relevant files

Enhancement

1 files

pgx_batch_helper.go
Add shared batch execution helper function +29/-0

Tests

2 files

pgx_batch_helper_test.go `Add comprehensive tests for batch helper`	+99/-0
pgx_batch_behavior_test.go `Add integration tests for batch error surfacing`	+79/-0

Bug fix

3 files

events.go `Update InsertEvents to use batch helper`	+1/-2
auth.go `Update StoreBatchUsers to use batch helper`	+1/-2
cnpg_unified_devices.go `Update DeleteDevices audit batch handling`	+1/-2

Documentation

4 files

proposal.md `Add change proposal documentation`	+19/-0
design.md `Add detailed design and decision rationale`	+39/-0
spec.md `Add batch write requirements specification`	+23/-0
tasks.md `Add implementation tasks checklist`	+13/-0

Imported from GitHub pull request. Original GitHub pull request: #2161 Original author: @mfreeman451 Original URL: https://github.com/carverauto/serviceradar/pull/2161 Original created: 2025-12-16T22:20:15Z Original updated: 2025-12-16T23:22:12Z Original head: carverauto/serviceradar:2153-insertevents-closes-pgx-batchresults-without-reading-results-discarding-insert-errors Original base: staging Original merged: 2025-12-16T23:20:10Z 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 pgx batch error handling to surface per-command INSERT failures - Add `sendBatchExecAll` helper to consistently read all batch results - Update `InsertEvents`, `StoreBatchUsers`, `DeleteDevices` to use helper - Add comprehensive tests and design documentation for batch handling ___ ### Diagram Walkthrough ```mermaid flowchart LR A["Batch Write Operations<br/>InsertEvents, StoreBatchUsers,<br/>DeleteDevices"] -->|"Previously: Close without reading"| B["Silent Error Loss<br/>Undetected data loss"] A -->|"Now: Use sendBatchExecAll"| C["Read All Results<br/>br.Exec() for each command"] C -->|"Defer Close"| D["Always Close BatchResults<br/>Even on error"] D -->|"Return Error"| E["Surface Per-Command Errors<br/>With operation + index context"] ``` <details><summary><h3>File Walkthrough</h3></summary> <table><thead><tr><th></th><th align="left">Relevant files</th></tr></thead><tbody><tr><td><strong>Enhancement</strong></td><td><details><summary>1 files</summary><table> <tr> <td><strong>pgx_batch_helper.go</strong><dd><code>Add shared batch execution helper function</code>                              </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2161/files#diff-9a38f78952feb40cd7f517c2d9a6c4a76a8ffcfa3989ced955298ad0ee6786b8">+29/-0</a>    </td> </tr> </table></details></td></tr><tr><td><strong>Tests</strong></td><td><details><summary>2 files</summary><table> <tr> <td><strong>pgx_batch_helper_test.go</strong><dd><code>Add comprehensive tests for batch helper</code>                                  </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2161/files#diff-67dd6237893aea0b294efd6b76f9aa138f4e8cd98424a3683f65ab9a7ce0372e">+99/-0</a>    </td> </tr> <tr> <td><strong>pgx_batch_behavior_test.go</strong><dd><code>Add integration tests for batch error surfacing</code>                    </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2161/files#diff-0c98da9a5d20f19ce43861711a001e0aebe953c4f004cb2bb38a245020f4e206">+79/-0</a>    </td> </tr> </table></details></td></tr><tr><td><strong>Bug fix</strong></td><td><details><summary>3 files</summary><table> <tr> <td><strong>events.go</strong><dd><code>Update InsertEvents to use batch helper</code>                                    </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2161/files#diff-bf1824600585d423d03392632872269bf1d5f976430b55d7a9bd1238bc8de6b5">+1/-2</a>      </td> </tr> <tr> <td><strong>auth.go</strong><dd><code>Update StoreBatchUsers to use batch helper</code>                              </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2161/files#diff-4d4e15c7e925bdd07a11ca5d2779a6f2acef74c2bebe286c4bbc9a7593cefdb7">+1/-2</a>      </td> </tr> <tr> <td><strong>cnpg_unified_devices.go</strong><dd><code>Update DeleteDevices audit batch handling</code>                                </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2161/files#diff-ab3bf557cf9bb1b281a315a73abee38748de1654941c2471542e5e9bfc1716d8">+1/-2</a>      </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>Add change proposal documentation</code>                                                </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2161/files#diff-2f311be222e7803de3df6b7b3361776eca39ecaa96c09f5699f260450154ec3d">+19/-0</a>    </td> </tr> <tr> <td><strong>design.md</strong><dd><code>Add detailed design and decision rationale</code>                              </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2161/files#diff-551f58cf66926ed96cb4df08f8de6bb718951933afbbcbb86831aa837b41d79f">+39/-0</a>    </td> </tr> <tr> <td><strong>spec.md</strong><dd><code>Add batch write requirements specification</code>                              </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2161/files#diff-113e7f6dcd42a481e54c270e3233214e97232f2c1788722655352a1e6d49590e">+23/-0</a>    </td> </tr> <tr> <td><strong>tasks.md</strong><dd><code>Add implementation tasks checklist</code>                                              </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2161/files#diff-3c4807d2b2ab7fb4b40fc23f5b929a2d0ffc4915da557ec96742b82b6d7b3fb8">+13/-0</a>    </td> </tr> </table></details></td></tr></tbody></table> </details> ___

qodo-code-review[bot] commented

2025-12-16 22:20:54 +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/2161#issuecomment-3662666783
Original created: 2025-12-16T22:20:54Z

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

🟢

🎫 #2153

🟢	Fix `InsertEvents` to read each queued `pgx` batch command result (via `BatchResults.Exec()`) before closing, so per-command INSERT errors are not silently discarded.
	Ensure batch write errors are surfaced to the caller with clear context (e.g., which operation and which command index failed).
	Follow the established codebase pattern for pgx batches: drain results for all queued commands and then close the batch results, ensuring `Close()` is called even on early error.
	Address other known occurrences of the same bug pattern in the codebase (ticket notes `pkg/db/auth.go` as also affected).

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: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

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: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status: 🏷️
DB error exposure: The helper wraps and returns the underlying pgx execution error which may include internal
database details that could be exposed if this error reaches user-facing surfaces.

Referred Code

func sendBatchExecAll(ctx context.Context, batch *pgx.Batch, send func(context.Context, *pgx.Batch) pgx.BatchResults, operation string) (err error) {
	if batch == nil || batch.Len() == 0 {
		return nil
	}

	br := send(ctx, batch)
	defer func() {
		if closeErr := br.Close(); closeErr != nil && err == nil {
			err = fmt.Errorf("%s batch close: %w", operation, closeErr)
		}
	}()

	for i := 0; i < batch.Len(); i++ {
		if _, err = br.Exec(); err != nil {
			return fmt.Errorf("%s batch exec (command %d): %w", operation, i, err)
		}

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/2161#issuecomment-3662666783 Original created: 2025-12-16T22:20:54Z --- _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 🔍  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/2153>#2153</a></summary> <table width='100%'><tbody> <tr><td rowspan=4>🟢</td> <td>Fix <code>InsertEvents</code> to read each queued <code>pgx</code> batch command result (via <code>BatchResults.Exec()</code>) <br>before closing, so per-command INSERT errors are not silently discarded.<br></td></tr> <tr><td>Ensure batch write errors are surfaced to the caller with clear context (e.g., which <br>operation and which command index failed).<br></td></tr> <tr><td>Follow the established codebase pattern for pgx batches: drain results for all queued <br>commands and then close the batch results, ensuring <code>Close()</code> is called even on early error.<br> <br></td></tr> <tr><td>Address other known occurrences of the same bug pattern in the codebase (ticket notes <br><code>pkg/db/auth.go</code> as also affected).<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: 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:** 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: 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:** 🏷️<br><a href='https://github.com/carverauto/serviceradar/pull/2161/files#diff-9a38f78952feb40cd7f517c2d9a6c4a76a8ffcfa3989ced955298ad0ee6786b8R10-R25'><strong>DB error exposure</strong></a>: The helper wraps and returns the underlying <code>pgx</code> execution error which may include internal <br>database details that could be exposed if this error reaches user-facing surfaces.<br> <details open><summary>Referred Code</summary> ```go func sendBatchExecAll(ctx context.Context, batch *pgx.Batch, send func(context.Context, *pgx.Batch) pgx.BatchResults, operation string) (err error) { if batch == nil || batch.Len() == 0 { return nil } br := send(ctx, batch) defer func() { if closeErr := br.Close(); closeErr != nil && err == nil { err = fmt.Errorf("%s batch close: %w", operation, closeErr) } }() for i := 0; i < batch.Len(); i++ { if _, err = br.Exec(); err != nil { return fmt.Errorf("%s batch exec (command %d): %w", operation, i, err) } ``` </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-16 22:22:12 +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/2161#issuecomment-3662673282
Original created: 2025-12-16T22:22:12Z

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

PR Code Suggestions ✨

Explore these optional code suggestions:

Category Suggestion Impact

General

Category	Suggestion	Impact
General	Improve test to cover error prioritization Enhance `TestSendBatchExecAll_ExecErrorIncludesCommandIndexAndCloses` to verify that an `Exec` error is prioritized and returned even when `br.Close()` also returns an error. pkg/db/pgx_batch_helper_test.go [66-84] func TestSendBatchExecAll_ExecErrorIncludesCommandIndexAndCloses(t testing.T) { ctx := context.Background() batch := &pgx.Batch{} batch.Queue("SELECT 1") batch.Queue("SELECT 2") batch.Queue("SELECT 3") br := &fakeBatchResults{ execErrAt: 1, execErr: errBoom, + closeErr: errCloseFailed, // Add a close error } err := sendBatchExecAll(ctx, batch, func(context.Context, pgx.Batch) pgx.BatchResults { return br }, "op-name") require.Error(t, err) + // The exec error should be returned, not the close error. + require.ErrorIs(t, err, errBoom) + require.NotContains(t, err.Error(), "close failed") require.Contains(t, err.Error(), "op-name batch exec (command 1)") require.Equal(t, 1, br.closeCalls) } Apply / Chat Suggestion importance[1-10]: 6 __ Why: This is a valid suggestion that improves test coverage by adding a test case for error prioritization, ensuring the implementation correctly handles concurrent `Exec` and `Close` errors.	Low
Update

Improve test to cover error prioritization

Enhance TestSendBatchExecAll_ExecErrorIncludesCommandIndexAndCloses to verify
that an Exec error is prioritized and returned even when br.Close() also returns
an error.

pkg/db/pgx_batch_helper_test.go [66-84]

 func TestSendBatchExecAll_ExecErrorIncludesCommandIndexAndCloses(t *testing.T) {
 	ctx := context.Background()
 	batch := &pgx.Batch{}
 	batch.Queue("SELECT 1")
 	batch.Queue("SELECT 2")
 	batch.Queue("SELECT 3")
 
 	br := &fakeBatchResults{
 		execErrAt: 1,
 		execErr:   errBoom,
+		closeErr:  errCloseFailed, // Add a close error
 	}
 
 	err := sendBatchExecAll(ctx, batch, func(context.Context, *pgx.Batch) pgx.BatchResults {
 		return br
 	}, "op-name")
 	require.Error(t, err)
+	// The exec error should be returned, not the close error.
+	require.ErrorIs(t, err, errBoom)
+	require.NotContains(t, err.Error(), "close failed")
 	require.Contains(t, err.Error(), "op-name batch exec (command 1)")
 	require.Equal(t, 1, br.closeCalls)
 }

Apply / Chat

Suggestion importance[1-10]: 6

Why: This is a valid suggestion that improves test coverage by adding a test case for error prioritization, ensuring the implementation correctly handles concurrent Exec and Close errors.

Low

Update

Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/2161#issuecomment-3662673282 Original created: 2025-12-16T22:22:12Z --- _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 ✨  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>General</td> <td> <details><summary>Improve test to cover error prioritization</summary> ___ **Enhance <code>TestSendBatchExecAll_ExecErrorIncludesCommandIndexAndCloses</code> to verify <br>that an <code>Exec</code> error is prioritized and returned even when <code>br.Close()</code> also returns <br>an error.** [pkg/db/pgx_batch_helper_test.go [66-84]](https://github.com/carverauto/serviceradar/pull/2161/files#diff-67dd6237893aea0b294efd6b76f9aa138f4e8cd98424a3683f65ab9a7ce0372eR66-R84) ```diff func TestSendBatchExecAll_ExecErrorIncludesCommandIndexAndCloses(t *testing.T) { ctx := context.Background() batch := &pgx.Batch{} batch.Queue("SELECT 1") batch.Queue("SELECT 2") batch.Queue("SELECT 3") br := &fakeBatchResults{ execErrAt: 1, execErr: errBoom, + closeErr: errCloseFailed, // Add a close error } err := sendBatchExecAll(ctx, batch, func(context.Context, *pgx.Batch) pgx.BatchResults { return br }, "op-name") require.Error(t, err) + // The exec error should be returned, not the close error. + require.ErrorIs(t, err, errBoom) + require.NotContains(t, err.Error(), "close failed") require.Contains(t, err.Error(), "op-name batch exec (command 1)") require.Equal(t, 1, br.closeCalls) } ``` - [ ] **Apply / Chat**  <details><summary>Suggestion importance[1-10]: 6</summary> __ Why: This is a valid suggestion that improves test coverage by adding a test case for error prioritization, ensuring the implementation correctly handles concurrent `Exec` and `Close` errors. </details></details></td><td align=center>Low </td></tr> <tr><td align="center" colspan="2"> - [ ] Update  </td><td></td></tr></tbody></table>