carverauto/serviceradar
1
1
Fork 0
Code Issues 190 Pull requests 16 Projects Releases 173 Packages Wiki Activity Actions

unbounded query fix #2400

Merged
mfreeman451 merged 5 commits from refs/pull/2400/head into main 2025-11-03 07:50:10 +00:00
Conversation 2 Commits 5 Files changed 2 +35 -15
mfreeman451 commented 2025-11-03 06:51:20 +00:00 (Migrated from github.com)
Owner
Copy link

Imported from GitHub pull request.

Original GitHub pull request: #1920
Original author: @mfreeman451
Original URL: https://github.com/carverauto/serviceradar/pull/1920
Original created: 2025-11-03T06:51:20Z
Original updated: 2025-11-03T07:50:40Z
Original head: carverauto/serviceradar:1919-bugcore
Original base: main
Original merged: 2025-11-03T07:50: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 unbounded query vulnerability in GetUnifiedDevice by escaping deviceID parameter

  • Improve joinValueTuples to handle empty values and prevent malformed SQL tuples

  • Add defensive checks for empty string values in tuple construction

Diagram Walkthrough

flowchart LR
  A["GetUnifiedDevice Query"] -->|"Apply escapeLiteral"| B["Escaped deviceID Parameter"]
  C["joinValueTuples Function"] -->|"Filter empty values"| D["Valid SQL Tuples"]
  C -->|"Handle edge cases"| E["Return safe tuple string"]

File Walkthrough
Relevant files
Bug fix
unified_devices.go
Secure SQL queries and improve tuple handling                       

pkg/db/unified_devices.go

  • Apply escapeLiteral() to deviceID parameter in GetUnifiedDevice query
    to prevent SQL injection
  • Refactor joinValueTuples to filter out empty string values before
    building tuples
  • Add early return for empty values list to prevent malformed SQL
  • Improve robustness by checking if filtered literals list is empty
    before joining
 +16/-5   
Imported from GitHub pull request. Original GitHub pull request: #1920 Original author: @mfreeman451 Original URL: https://github.com/carverauto/serviceradar/pull/1920 Original created: 2025-11-03T06:51:20Z Original updated: 2025-11-03T07:50:40Z Original head: carverauto/serviceradar:1919-bugcore Original base: main Original merged: 2025-11-03T07:50: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 unbounded query vulnerability in `GetUnifiedDevice` by escaping deviceID parameter - Improve `joinValueTuples` to handle empty values and prevent malformed SQL tuples - Add defensive checks for empty string values in tuple construction ___ ### Diagram Walkthrough ```mermaid flowchart LR A["GetUnifiedDevice Query"] -->|"Apply escapeLiteral"| B["Escaped deviceID Parameter"] C["joinValueTuples Function"] -->|"Filter empty values"| D["Valid SQL Tuples"] C -->|"Handle edge cases"| E["Return safe tuple string"] ``` <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>unified_devices.go</strong><dd><code>Secure SQL queries and improve tuple handling</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></summary> <hr> pkg/db/unified_devices.go <ul><li>Apply <code>escapeLiteral()</code> to <code>deviceID</code> parameter in <code>GetUnifiedDevice</code> query <br>to prevent SQL injection<br> <li> Refactor <code>joinValueTuples</code> to filter out empty string values before <br>building tuples<br> <li> Add early return for empty values list to prevent malformed SQL<br> <li> Improve robustness by checking if filtered literals list is empty <br>before joining</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/1920/files#diff-165bffede411b9a031b4d85af0ae459a1913960593d59bfc19554cb838e39679">+16/-5</a>&nbsp; &nbsp; </td> </tr> </table></td></tr></tr></tbody></table> </details> ___
qodo-code-review[bot] commented 2025-11-03 06:51:48 +00:00 (Migrated from github.com)
Author
Owner
Copy link

Imported GitHub PR comment.

Original author: @qodo-code-review[bot]
Original URL: https://github.com/carverauto/serviceradar/pull/1920#issuecomment-3479140434
Original created: 2025-11-03T06:51:48Z

PR Compliance Guide 🔍

(Compliance updated until commit github.com/carverauto/serviceradar@360221de44)

Below is a summary of compliance checks for this PR:

Security Compliance
⚪
Potential heavy query

Description: Using ORDER BY _tp_time DESC NULLS LAST LIMIT 1 without an index or proper query plan may
still allow expensive scans on large tables; however, the primary injection risk is
mitigated by parameterization and escaping—this is a possible performance-related concern
requiring verification rather than a security exploit.
unified_devices.go [42-49]

Referred Code 
	query := `SELECT
        device_id, ip, poller_id, hostname, mac, discovery_sources,
        is_available, first_seen, last_seen, metadata, agent_id, device_type, 
        service_type, service_status, last_heartbeat, os_info, version_info
    FROM table(unified_devices)
    WHERE device_id = $1
    ORDER BY _tp_time DESC NULLS LAST
    LIMIT 1`
Ticket Compliance
🟡
🎫 #1919
🟢 Ensure core queries against unified devices are not causing timeouts or unbounded scans.
Add safeguards to prevent malformed SQL from empty inputs when batching deviceIDs or IPs.
Provide tests covering edge cases related to empty inputs to avoid regressions.
Maintain correct ordering and limiting to fetch latest unified device safely.
⚪ Investigate and fix errors and broken connections occurring after update
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

Generic: Robust Error Handling and Edge Case Management

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

Status: Passed

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

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

Generic: Security-First Input Validation and Data Handling

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

Status: Passed

⚪
Generic: Comprehensive Audit Trails

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

Status:
Missing Auditing: The new query paths for fetching unified devices and batch querying do not add or
reference any audit logging for these read operations, making it unclear if critical
access to device records is logged.

Referred Code 
	query := `SELECT
        device_id, ip, poller_id, hostname, mac, discovery_sources,
        is_available, first_seen, last_seen, metadata, agent_id, device_type, 
        service_type, service_status, last_heartbeat, os_info, version_info
    FROM table(unified_devices)
    WHERE device_id = $1
    ORDER BY _tp_time DESC NULLS LAST
    LIMIT 1`

	// This function has special handling for the case where no rows are returned,
	// so we can't use the queryUnifiedDevices helper
	rows, err := db.Conn.Query(ctx, query, deviceID)
	if err != nil {
		return nil, fmt.Errorf("%w: %w", errFailedToQueryUnifiedDevice, err)
	}
	defer func() { _ = rows.Close() }()
Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

Previous compliance checks

Compliance check up to commit 360221d
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

Generic: Robust Error Handling and Edge Case Management

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

Status: Passed

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

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

Generic: Security-First Input Validation and Data Handling

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

Status: Passed

⚪
Generic: Comprehensive Audit Trails

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

Status:
Missing Auditing: The new query paths for fetching unified devices and batch lookups do not indicate any
audit logging of access to potentially sensitive device data, and the diff provides no
evidence that such access is logged elsewhere.

Referred Code 
	query := `SELECT
        device_id, ip, poller_id, hostname, mac, discovery_sources,
        is_available, first_seen, last_seen, metadata, agent_id, device_type, 
        service_type, service_status, last_heartbeat, os_info, version_info
    FROM table(unified_devices)
    WHERE device_id = $1
    ORDER BY _tp_time DESC NULLS LAST
    LIMIT 1`

	// This function has special handling for the case where no rows are returned,
	// so we can't use the queryUnifiedDevices helper
	rows, err := db.Conn.Query(ctx, query, deviceID)
	if err != nil {
		return nil, fmt.Errorf("%w: %w", errFailedToQueryUnifiedDevice, err)
	}
	defer func() { _ = rows.Close() }()
Compliance check up to commit baaae70
Security Compliance
⚪
Non-parameterized SQL

Description: Query is built via string formatting with an interpolated literal even though
escapeLiteral is used; lack of parameterized queries means any missed escaping edge case
could enable SQL injection or query breakage.
unified_devices.go [41-49]

Referred Code 
	query := fmt.Sprintf(`SELECT
        device_id, ip, poller_id, hostname, mac, discovery_sources,
        is_available, first_seen, last_seen, metadata, agent_id, device_type, 
        service_type, service_status, last_heartbeat, os_info, version_info
    FROM table(unified_devices)
    WHERE device_id = '%s'
    ORDER BY _tp_time DESC
    LIMIT 1`, escapeLiteral(deviceID))
Unsafe default tuple

Description: Function joinValueTuples returns "('')" for empty inputs which may be interpreted as a
valid value and could lead to unintended query logic or data leakage if used to build
IN-like clauses.
unified_devices.go [382-400]

Referred Code 
func joinValueTuples(values []string) string {
	if len(values) == 0 {
		return "('')"
	}

	literals := make([]string, 0, len(values))
	for _, v := range values {
		if v == "" {
			continue
		}
		literals = append(literals, fmt.Sprintf("('%s')", escapeLiteral(v)))
	}

	if len(literals) == 0 {
		return "('')"
	}

	return strings.Join(literals, ", ")
}
Ticket Compliance
🟡
🎫 #1919
🟢 Provide a bug-fix implementation targeting the core/proton area implicated by the logs.
🔴 Investigate and fix errors and broken connections occurring after update
Ensure database queries involved in unified devices lookups and registrations are robust
and not causing timeouts or failures.
⚪ Validate in staging/production that context deadline exceeded errors no longer occur
during unified device queries and registrations after this change.
Confirm proton broken connections diminish and core logs are clean in real workloads.
Performance validation that LIMIT 1 and tuple handling changes do not introduce
regressions under load.
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

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

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

Generic: Security-First Input Validation and Data Handling

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

Status: Passed

⚪
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 new query handling and input sanitization do not add or reference any auditing or
logging of critical actions, and it's unclear whether querying sensitive device data
is captured in audit trails elsewhere.

Referred Code 
func (db *DB) GetUnifiedDevice(ctx context.Context, deviceID string) (*models.UnifiedDevice, error) {
	query := fmt.Sprintf(`SELECT
        device_id, ip, poller_id, hostname, mac, discovery_sources,
        is_available, first_seen, last_seen, metadata, agent_id, device_type, 
        service_type, service_status, last_heartbeat, os_info, version_info
    FROM table(unified_devices)
    WHERE device_id = '%s'
    ORDER BY _tp_time DESC
    LIMIT 1`, escapeLiteral(deviceID))

	// This function has special handling for the case where no rows are returned,
	// so we can't use the queryUnifiedDevices helper
	rows, err := db.Conn.Query(ctx, query)
Generic: Robust Error Handling and Edge Case Management

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

Status:
Edge case return: The function joinValueTuples returns "('')" for empty or all-empty
inputs, which may hide an upstream validation issue or produce ambiguous SQL semantics
without explicit error handling or logging.

Referred Code 
func joinValueTuples(values []string) string {
	if len(values) == 0 {
		return "('')"
	}

	literals := make([]string, 0, len(values))
	for _, v := range values {
		if v == "" {
			continue
		}
		literals = append(literals, fmt.Sprintf("('%s')", escapeLiteral(v)))
	}

	if len(literals) == 0 {
		return "('')"
	}

	return strings.Join(literals, ", ")
}
Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/1920#issuecomment-3479140434 Original created: 2025-11-03T06:51:48Z --- ## PR Compliance Guide 🔍 <!-- https://github.com/carverauto/serviceradar/commit/360221de44e4bf7e636ca3e1c19d5373fa7d0c0a --> #### (Compliance updated until commit https://github.com/carverauto/serviceradar/commit/360221de44e4bf7e636ca3e1c19d5373fa7d0c0a) 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 rowspan=1>⚪</td> <td><details><summary><strong>Potential heavy query </strong></summary><br> <b>Description:</b> Using ORDER BY _tp_time DESC NULLS LAST LIMIT 1 without an index or proper query plan may <br>still allow expensive scans on large tables; however, the primary injection risk is <br>mitigated by parameterization and escaping—this is a possible performance-related concern <br>requiring verification rather than a security exploit.<br> <strong><a href='https://github.com/carverauto/serviceradar/pull/1920/files#diff-165bffede411b9a031b4d85af0ae459a1913960593d59bfc19554cb838e39679R42-R49'>unified_devices.go [42-49]</a></strong><br> <details open><summary>Referred Code</summary> ```go query := `SELECT device_id, ip, poller_id, hostname, mac, discovery_sources, is_available, first_seen, last_seen, metadata, agent_id, device_type, service_type, service_status, last_heartbeat, os_info, version_info FROM table(unified_devices) WHERE device_id = $1 ORDER BY _tp_time DESC NULLS LAST LIMIT 1` ``` </details></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/1919>#1919</a></summary> <table width='100%'><tbody> <tr><td rowspan=4>🟢</td> <td>Ensure core queries against unified devices are not causing timeouts or unbounded scans.</td></tr> <tr><td>Add safeguards to prevent malformed SQL from empty inputs when batching deviceIDs or IPs.</td></tr> <tr><td>Provide tests covering edge cases related to empty inputs to avoid regressions.</td></tr> <tr><td>Maintain correct ordering and limiting to fetch latest unified device safely.</td></tr> <tr><td rowspan=1>⚪</td> <td>Investigate and fix errors and broken connections occurring after update</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: 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> </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> </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> </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> </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> </details></td></tr> <tr><td rowspan=1>⚪</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/1920/files#diff-165bffede411b9a031b4d85af0ae459a1913960593d59bfc19554cb838e39679R42-R57'><strong>Missing Auditing</strong></a>: The new query paths for fetching unified devices and batch querying do not add or <br>reference any audit logging for these read operations, making it unclear if critical <br>access to device records is logged.<br> <details open><summary>Referred Code</summary> ```go query := `SELECT device_id, ip, poller_id, hostname, mac, discovery_sources, is_available, first_seen, last_seen, metadata, agent_id, device_type, service_type, service_status, last_heartbeat, os_info, version_info FROM table(unified_devices) WHERE device_id = $1 ORDER BY _tp_time DESC NULLS LAST LIMIT 1` // This function has special handling for the case where no rows are returned, // so we can't use the queryUnifiedDevices helper rows, err := db.Conn.Query(ctx, query, deviceID) if err != nil { return nil, fmt.Errorf("%w: %w", errFailedToQueryUnifiedDevice, err) } defer func() { _ = rows.Close() }() ``` </details></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> ___ #### Previous compliance checks <details> <summary>Compliance check up to commit <a href='https://github.com/carverauto/serviceradar/commit/360221de44e4bf7e636ca3e1c19d5373fa7d0c0a'>360221d</a></summary><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 </summary></strong> - [ ] Create ticket/issue <!-- /create_ticket --create_ticket=true --> </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: 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> </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> </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> </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> </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> </details></td></tr> <tr><td rowspan=1>⚪</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/1920/files#diff-165bffede411b9a031b4d85af0ae459a1913960593d59bfc19554cb838e39679R42-R57'><strong>Missing Auditing</strong></a>: The new query paths for fetching unified devices and batch lookups do not indicate any <br>audit logging of access to potentially sensitive device data, and the diff provides no <br>evidence that such access is logged elsewhere.<br> <details open><summary>Referred Code</summary> ```go query := `SELECT device_id, ip, poller_id, hostname, mac, discovery_sources, is_available, first_seen, last_seen, metadata, agent_id, device_type, service_type, service_status, last_heartbeat, os_info, version_info FROM table(unified_devices) WHERE device_id = $1 ORDER BY _tp_time DESC NULLS LAST LIMIT 1` // This function has special handling for the case where no rows are returned, // so we can't use the queryUnifiedDevices helper rows, err := db.Conn.Query(ctx, query, deviceID) if err != nil { return nil, fmt.Errorf("%w: %w", errFailedToQueryUnifiedDevice, err) } defer func() { _ = rows.Close() }() ``` </details></details></td></tr> <tr><td align="center" colspan="2"> <!-- placeholder --> <!-- /compliance --update_compliance=true --> </td></tr></tbody></table> </details> <details> <summary>Compliance check up to commit <a href='https://github.com/carverauto/serviceradar/commit/baaae7076739d55ff5531e138d6e35fd6170c791'>baaae70</a></summary><br> <table><tbody><tr><td colspan='2'><strong>Security Compliance</strong></td></tr> <tr><td rowspan=2>⚪</td> <td><details><summary><strong>Non-parameterized SQL </strong></summary><br> <b>Description:</b> Query is built via string formatting with an interpolated literal even though <br>escapeLiteral is used; lack of parameterized queries means any missed escaping edge case <br>could enable SQL injection or query breakage.<br> <strong><a href='https://github.com/carverauto/serviceradar/pull/1920/files#diff-165bffede411b9a031b4d85af0ae459a1913960593d59bfc19554cb838e39679R41-R49'>unified_devices.go [41-49]</a></strong><br> <details open><summary>Referred Code</summary> ```go query := fmt.Sprintf(`SELECT device_id, ip, poller_id, hostname, mac, discovery_sources, is_available, first_seen, last_seen, metadata, agent_id, device_type, service_type, service_status, last_heartbeat, os_info, version_info FROM table(unified_devices) WHERE device_id = '%s' ORDER BY _tp_time DESC LIMIT 1`, escapeLiteral(deviceID)) ``` </details></details></td></tr> <tr><td><details><summary><strong>Unsafe default tuple </strong></summary><br> <b>Description:</b> Function joinValueTuples returns "('')" for empty inputs which may be interpreted as a <br>valid value and could lead to unintended query logic or data leakage if used to build <br>IN-like clauses.<br> <strong><a href='https://github.com/carverauto/serviceradar/pull/1920/files#diff-165bffede411b9a031b4d85af0ae459a1913960593d59bfc19554cb838e39679R382-R400'>unified_devices.go [382-400]</a></strong><br> <details open><summary>Referred Code</summary> ```go func joinValueTuples(values []string) string { if len(values) == 0 { return "('')" } literals := make([]string, 0, len(values)) for _, v := range values { if v == "" { continue } literals = append(literals, fmt.Sprintf("('%s')", escapeLiteral(v))) } if len(literals) == 0 { return "('')" } return strings.Join(literals, ", ") } ``` </details></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/1919>#1919</a></summary> <table width='100%'><tbody> <tr><td rowspan=1>🟢</td> <td>Provide a bug-fix implementation targeting the core/proton area implicated by the logs.</td></tr> <tr><td rowspan=2>🔴</td> <td>Investigate and fix errors and broken connections occurring after update</td></tr> <tr><td>Ensure database queries involved in unified devices lookups and registrations are robust <br>and not causing timeouts or failures.</td></tr> <tr><td rowspan=3>⚪</td> <td>Validate in staging/production that context deadline exceeded errors no longer occur <br>during unified device queries and registrations after this change.</td></tr> <tr><td>Confirm proton broken connections diminish and core logs are clean in real workloads.</td></tr> <tr><td>Performance validation that LIMIT 1 and tuple handling changes do not introduce <br>regressions under load.</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=4>🟢</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> </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> </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> </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> </details></td></tr> <tr><td rowspan=2>⚪</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/1920/files#diff-165bffede411b9a031b4d85af0ae459a1913960593d59bfc19554cb838e39679R40-R52'><strong>Missing audit logs</strong></a>: The new query handling and input sanitization do not add or reference any auditing or <br>logging of critical actions, and it&#x27;s unclear whether querying sensitive device data <br>is captured in audit trails elsewhere.<br> <details open><summary>Referred Code</summary> ```go func (db *DB) GetUnifiedDevice(ctx context.Context, deviceID string) (*models.UnifiedDevice, error) { query := fmt.Sprintf(`SELECT device_id, ip, poller_id, hostname, mac, discovery_sources, is_available, first_seen, last_seen, metadata, agent_id, device_type, service_type, service_status, last_heartbeat, os_info, version_info FROM table(unified_devices) WHERE device_id = '%s' ORDER BY _tp_time DESC LIMIT 1`, escapeLiteral(deviceID)) // This function has special handling for the case where no rows are returned, // so we can't use the queryUnifiedDevices helper rows, err := db.Conn.Query(ctx, query) ``` </details></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/1920/files#diff-165bffede411b9a031b4d85af0ae459a1913960593d59bfc19554cb838e39679R382-R400'><strong>Edge case return</strong></a>: The function <code>joinValueTuples</code> returns &quot;(&#x27;&#x27;)&quot; for empty or all-empty <br>inputs, which may hide an upstream validation issue or produce ambiguous SQL semantics <br>without explicit error handling or logging.<br> <details open><summary>Referred Code</summary> ```go func joinValueTuples(values []string) string { if len(values) == 0 { return "('')" } literals := make([]string, 0, len(values)) for _, v := range values { if v == "" { continue } literals = append(literals, fmt.Sprintf("('%s')", escapeLiteral(v))) } if len(literals) == 0 { return "('')" } return strings.Join(literals, ", ") } ``` </details></details></td></tr> <tr><td align="center" colspan="2"> <!-- /compliance --update_compliance=true --> </td></tr></tbody></table> </details>
qodo-code-review[bot] commented 2025-11-03 06:52:46 +00:00 (Migrated from github.com)
Author
Owner
Copy link

Imported GitHub PR comment.

Original author: @qodo-code-review[bot]
Original URL: https://github.com/carverauto/serviceradar/pull/1920#issuecomment-3479142142
Original created: 2025-11-03T06:52:46Z

PR Code Suggestions ✨

Latest suggestions up to c3388b5

CategorySuggestion                                                                                                                                    Impact
Incremental [*]
Return empty slice, not nil

*In queryUnifiedDeviceBatch, return an empty slice []models.UnifiedDevice{}
instead of nil when there are no conditions to provide a clearer "no results"
signal to callers.

pkg/db/unified_devices.go [304-306]

 if len(conditions) == 0 {
-    return nil, nil
+    return []*models.UnifiedDevice{}, nil
 }
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: The suggestion improves the code by adhering to Go's best practice of returning an empty slice instead of a nil slice for "no results", which enhances caller safety and code clarity.

Medium
✅ Make ordering deterministically latest
Suggestion Impact:The query was updated to "ORDER BY _tp_time DESC NULLS LAST" as suggested, ensuring deterministic ordering.

code diff:

-    ORDER BY _tp_time DESC
+    ORDER BY _tp_time DESC NULLS LAST

Add NULLS LAST to the ORDER BY _tp_time DESC clause in the GetUnifiedDevice
query to ensure consistent ordering if _tp_time contains NULL values.

pkg/db/unified_devices.go [41-48]

 query := `SELECT
         device_id, ip, poller_id, hostname, mac, discovery_sources,
         is_available, first_seen, last_seen, metadata, agent_id, device_type, 
         service_type, service_status, last_heartbeat, os_info, version_info
     FROM table(unified_devices)
     WHERE device_id = $1
-    ORDER BY _tp_time DESC
+    ORDER BY _tp_time DESC NULLS LAST
     LIMIT 1`

[Suggestion processed]

Suggestion importance[1-10]: 5

__

Why: The suggestion correctly identifies a potential robustness issue in the SQL query's ORDER BY clause and proposes a standard fix, which improves query determinism if _tp_time can be NULL.

Low
Possible issue
✅ Error on empty filter
Suggestion Impact:The code was changed to return an error (fmt.Errorf("no deviceIDs or ips provided")) when len(conditions) == 0 instead of returning nil, nil.

code diff:

 	if len(conditions) == 0 {
-		return nil, nil
+		return nil, fmt.Errorf("no deviceIDs or ips provided")
 	}

Modify the function to return an error instead of nil, nil when no valid
deviceIDs or ips are provided, making invalid usage explicit to the caller.

pkg/db/unified_devices.go [304-306]

 if len(conditions) == 0 {
-    return nil, nil
+    return nil, fmt.Errorf("no deviceIDs or ips provided")
 }

[Suggestion processed]

Suggestion importance[1-10]: 6

__

Why: The suggestion correctly identifies that returning nil, nil for no provided filters can mask incorrect usage. Returning an error would create a more robust and explicit API contract, which is a good practice.

Low
  • Update

Previous suggestions

✅ Suggestions up to commit baaae70
CategorySuggestion                                                                                                                                    Impact
High-level
Use parameterized queries to prevent SQL injection

Instead of manually escaping single quotes to prevent SQL injection, the code
should be refactored to use parameterized queries. This is a more secure and
robust industry-standard practice.

Examples:

pkg/db/unified_devices.go [41-48] 
	query := fmt.Sprintf(`SELECT
        device_id, ip, poller_id, hostname, mac, discovery_sources,
        is_available, first_seen, last_seen, metadata, agent_id, device_type, 
        service_type, service_status, last_heartbeat, os_info, version_info
    FROM table(unified_devices)
    WHERE device_id = '%s'
    ORDER BY _tp_time DESC
    LIMIT 1`, escapeLiteral(deviceID))

Solution Walkthrough:

Before:

func (db *DB) GetUnifiedDevice(ctx context.Context, deviceID string) (*models.UnifiedDevice, error) {
	query := fmt.Sprintf(`SELECT
        ...
    FROM table(unified_devices)
    WHERE device_id = '%s'
    ORDER BY _tp_time DESC
    LIMIT 1`, escapeLiteral(deviceID))

	rows, err := db.Conn.Query(ctx, query)
    // ...
}

func escapeLiteral(value string) string {
	return strings.ReplaceAll(value, "'", "''")
}

After:

func (db *DB) GetUnifiedDevice(ctx context.Context, deviceID string) (*models.UnifiedDevice, error) {
	query := `SELECT
        ...
    FROM table(unified_devices)
    WHERE device_id = $1
    ORDER BY _tp_time DESC
    LIMIT 1`

	// Use parameterized query to prevent SQL injection
	rows, err := db.Conn.Query(ctx, query, deviceID)
    // ...
}

// The escapeLiteral function is no longer needed for this query.
Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies that manual string escaping is an inadequate fix for SQL injection and proposes the industry-standard best practice of using parameterized queries, which is a critical security improvement.

High
Possible issue
✅ Avoid returning incorrect query results
Suggestion Impact:The commit removed the fallback returns of "('')" in joinValueTuples and additionally trimmed whitespace from values. Call sites were updated to skip adding clauses when joinValueTuples returns an empty string, aligning with the suggestion's intent.

code diff:

 func joinValueTuples(values []string) string {
-	if len(values) == 0 {
-		return "('')"
-	}
-
 	literals := make([]string, 0, len(values))
 	for _, v := range values {
-		if v == "" {
+		if v = strings.TrimSpace(v); v == "" {
 			continue
 		}
 		literals = append(literals, fmt.Sprintf("('%s')", escapeLiteral(v)))
-	}
-
-	if len(literals) == 0 {
-		return "('')"
 	}
 
 	return strings.Join(literals, ", ")

Modify joinValueTuples to return an empty string instead of ('') when the input
list is empty or contains only empty strings. This prevents queries from
returning incorrect results by matching on empty string values.

pkg/db/unified_devices.go [382-400]

 func joinValueTuples(values []string) string {
-	if len(values) == 0 {
-		return "('')"
-	}
-
 	literals := make([]string, 0, len(values))
 	for _, v := range values {
 		if v == "" {
 			continue
 		}
 		literals = append(literals, fmt.Sprintf("('%s')", escapeLiteral(v)))
 	}
 
 	if len(literals) == 0 {
-		return "('')"
+		return ""
 	}
 
 	return strings.Join(literals, ", ")
 }

[Suggestion processed]

Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies a logical bug where returning ('') for an empty list would cause a query to incorrectly search for empty strings instead of returning no results. This is a significant correctness issue.

Medium
Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/1920#issuecomment-3479142142 Original created: 2025-11-03T06:52:46Z --- ## PR Code Suggestions ✨ <!-- c3388b5 --> Latest suggestions up to c3388b5 <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>Incremental <sup><a href='https://qodo-merge-docs.qodo.ai/core-abilities/incremental_update/'>[*]</a></sup></td> <td> <details><summary>Return empty slice, not nil<!-- not_implemented --></summary> ___ **In <code>queryUnifiedDeviceBatch</code>, return an empty slice <code>[]*models.UnifiedDevice{}</code> <br>instead of <code>nil</code> when there are no conditions to provide a clearer "no results" <br>signal to callers.** [pkg/db/unified_devices.go [304-306]](https://github.com/carverauto/serviceradar/pull/1920/files#diff-165bffede411b9a031b4d85af0ae459a1913960593d59bfc19554cb838e39679R304-R306) ```diff if len(conditions) == 0 { - return nil, nil + return []*models.UnifiedDevice{}, nil } ``` - [ ] **Apply / Chat** <!-- /improve --apply_suggestion=0 --> <details><summary>Suggestion importance[1-10]: 7</summary> __ Why: The suggestion improves the code by adhering to Go's best practice of returning an empty slice instead of a nil slice for "no results", which enhances caller safety and code clarity. </details></details></td><td align=center>Medium </td></tr><tr><td> <details><summary>✅ <s>Make ordering deterministically latest</s></summary> ___ <details><summary><b>Suggestion Impact:</b></summary>The query was updated to "ORDER BY _tp_time DESC NULLS LAST" as suggested, ensuring deterministic ordering. code diff: ```diff - ORDER BY _tp_time DESC + ORDER BY _tp_time DESC NULLS LAST ``` </details> ___ **Add <code>NULLS LAST</code> to the <code>ORDER BY _tp_time DESC</code> clause in the <code>GetUnifiedDevice</code> <br>query to ensure consistent ordering if <code>_tp_time</code> contains <code>NULL</code> values.** [pkg/db/unified_devices.go [41-48]](https://github.com/carverauto/serviceradar/pull/1920/files#diff-165bffede411b9a031b4d85af0ae459a1913960593d59bfc19554cb838e39679R41-R48) ```diff query := `SELECT device_id, ip, poller_id, hostname, mac, discovery_sources, is_available, first_seen, last_seen, metadata, agent_id, device_type, service_type, service_status, last_heartbeat, os_info, version_info FROM table(unified_devices) WHERE device_id = $1 - ORDER BY _tp_time DESC + ORDER BY _tp_time DESC NULLS LAST LIMIT 1` ``` `[Suggestion processed]` <details><summary>Suggestion importance[1-10]: 5</summary> __ Why: The suggestion correctly identifies a potential robustness issue in the SQL query's `ORDER BY` clause and proposes a standard fix, which improves query determinism if `_tp_time` can be `NULL`. </details></details></td><td align=center>Low </td></tr><tr><td rowspan=1>Possible issue</td> <td> <details><summary>✅ <s>Error on empty filter</s></summary> ___ <details><summary><b>Suggestion Impact:</b></summary>The code was changed to return an error (fmt.Errorf("no deviceIDs or ips provided")) when len(conditions) == 0 instead of returning nil, nil. code diff: ```diff if len(conditions) == 0 { - return nil, nil + return nil, fmt.Errorf("no deviceIDs or ips provided") } ``` </details> ___ **Modify the function to return an error instead of <code>nil, nil</code> when no valid <br><code>deviceIDs</code> or <code>ips</code> are provided, making invalid usage explicit to the caller.** [pkg/db/unified_devices.go [304-306]](https://github.com/carverauto/serviceradar/pull/1920/files#diff-165bffede411b9a031b4d85af0ae459a1913960593d59bfc19554cb838e39679R304-R306) ```diff if len(conditions) == 0 { - return nil, nil + return nil, fmt.Errorf("no deviceIDs or ips provided") } ``` `[Suggestion processed]` <details><summary>Suggestion importance[1-10]: 6</summary> __ Why: The suggestion correctly identifies that returning `nil, nil` for no provided filters can mask incorrect usage. Returning an error would create a more robust and explicit API contract, which is a good practice. </details></details></td><td align=center>Low </td></tr> <tr><td align="center" colspan="2"> - [ ] Update <!-- /improve_multi --more_suggestions=true --> </td><td></td></tr></tbody></table> ___ #### Previous suggestions <details><summary>✅ Suggestions up to commit baaae70</summary> <br><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=1>High-level</td> <td> <details><summary>Use parameterized queries to prevent SQL injection</summary> ___ **Instead of manually escaping single quotes to prevent SQL injection, the code <br>should be refactored to use parameterized queries. This is a more secure and <br>robust industry-standard practice.** ### Examples: <details> <summary> <a href="https://github.com/carverauto/serviceradar/pull/1920/files#diff-165bffede411b9a031b4d85af0ae459a1913960593d59bfc19554cb838e39679R41-R48">pkg/db/unified_devices.go [41-48]</a> </summary> ```go query := fmt.Sprintf(`SELECT device_id, ip, poller_id, hostname, mac, discovery_sources, is_available, first_seen, last_seen, metadata, agent_id, device_type, service_type, service_status, last_heartbeat, os_info, version_info FROM table(unified_devices) WHERE device_id = '%s' ORDER BY _tp_time DESC LIMIT 1`, escapeLiteral(deviceID)) ``` </details> ### Solution Walkthrough: #### Before: ```go func (db *DB) GetUnifiedDevice(ctx context.Context, deviceID string) (*models.UnifiedDevice, error) { query := fmt.Sprintf(`SELECT ... FROM table(unified_devices) WHERE device_id = '%s' ORDER BY _tp_time DESC LIMIT 1`, escapeLiteral(deviceID)) rows, err := db.Conn.Query(ctx, query) // ... } func escapeLiteral(value string) string { return strings.ReplaceAll(value, "'", "''") } ``` #### After: ```go func (db *DB) GetUnifiedDevice(ctx context.Context, deviceID string) (*models.UnifiedDevice, error) { query := `SELECT ... FROM table(unified_devices) WHERE device_id = $1 ORDER BY _tp_time DESC LIMIT 1` // Use parameterized query to prevent SQL injection rows, err := db.Conn.Query(ctx, query, deviceID) // ... } // The escapeLiteral function is no longer needed for this query. ``` <details><summary>Suggestion importance[1-10]: 9</summary> __ Why: The suggestion correctly identifies that manual string escaping is an inadequate fix for SQL injection and proposes the industry-standard best practice of using parameterized queries, which is a critical security improvement. </details></details></td><td align=center>High </td></tr><tr><td rowspan=1>Possible issue</td> <td> <details><summary>✅ <s>Avoid returning incorrect query results</s></summary> ___ <details><summary><b>Suggestion Impact:</b></summary>The commit removed the fallback returns of "('')" in joinValueTuples and additionally trimmed whitespace from values. Call sites were updated to skip adding clauses when joinValueTuples returns an empty string, aligning with the suggestion's intent. code diff: ```diff func joinValueTuples(values []string) string { - if len(values) == 0 { - return "('')" - } - literals := make([]string, 0, len(values)) for _, v := range values { - if v == "" { + if v = strings.TrimSpace(v); v == "" { continue } literals = append(literals, fmt.Sprintf("('%s')", escapeLiteral(v))) - } - - if len(literals) == 0 { - return "('')" } return strings.Join(literals, ", ") ``` </details> ___ **Modify <code>joinValueTuples</code> to return an empty string instead of <code>('')</code> when the input <br>list is empty or contains only empty strings. This prevents queries from <br>returning incorrect results by matching on empty string values.** [pkg/db/unified_devices.go [382-400]](https://github.com/carverauto/serviceradar/pull/1920/files#diff-165bffede411b9a031b4d85af0ae459a1913960593d59bfc19554cb838e39679R382-R400) ```diff func joinValueTuples(values []string) string { - if len(values) == 0 { - return "('')" - } - literals := make([]string, 0, len(values)) for _, v := range values { if v == "" { continue } literals = append(literals, fmt.Sprintf("('%s')", escapeLiteral(v))) } if len(literals) == 0 { - return "('')" + return "" } return strings.Join(literals, ", ") } ``` `[Suggestion processed]` <details><summary>Suggestion importance[1-10]: 8</summary> __ Why: The suggestion correctly identifies a logical bug where returning `('')` for an empty list would cause a query to incorrectly search for empty strings instead of returning no results. This is a significant correctness issue. </details></details></td><td align=center>Medium </td></tr> <tr><td align="center" colspan="2"> <!-- /improve_multi --more_suggestions=true --> </td><td></td></tr></tbody></table> </details>
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
1week

   
2weeks

   
Failed compliance check

   
IP cameras

   
NATS

NATS JetStream

  
Possible security concern

   
Review effort 1/5

   
Review effort 2/5

   
Review effort 3/5

   
Review effort 4/5

   
Review effort 5/5

   
UI

   
aardvark

   
accessibility

   
amd64

   
api

   
arm64

   
auth

   
back-end

   
bgp

   
blog

   
bug

Something isn't working

  
build

   
checkers

   
ci-cd

continuous integration-continuous deployments

  
cleanup

   
cnpg

cloud-native postgres

  
codex

   
core

core service

  
dependencies

Pull requests that update a dependency file

  
device-management

   
documentation

Improvements or additions to documentation

  
duplicate

This issue or pull request already exists

  
dusk

   
ebpf

   
enhancement

New feature or request

  
eta 1d

   
eta 1hr

   
eta 3d

   
eta 3hr

   
feature

   
fieldsurvey

   
github_actions

Pull requests that update GitHub Actions code

  
go

Pull requests that update Go code

  
good first issue

Good for newcomers

  
help wanted

Extra attention is needed

  
invalid

This doesn't seem right

  
javascript

Pull requests that update Javascript code

  
k8s

   
log-collector

   
mapper

   
mtr

multi traceroute

  
needs-triage

   
netflow

   
network-sweep

   
observability

   
oracle

Oracle Linux related issues

  
otel

opentelemetry logs, traces, metrics

  
plug-in

   
proton

timeplus proton streaming database

  
python

   
question

Further information is requested

  
reddit

   
redhat

   
research

   
rperf

   
rperf-checker

   
rust

Pull requests that update rust code

  
sdk

   
security

   
serviceradar-agent

   
serviceradar-agent-gateway

   
serviceradar-web

   
serviceradar-web-ng

   
siem

   
snmp

   
sysmon

   
topology

   
ubiquiti

   
wasm

   
wontfix

This will not be worked on

  
zen-engine
No labels
1week
2weeks
Failed compliance check
IP cameras
NATS
Possible security concern
Review effort 1/5
Review effort 2/5
Review effort 3/5
Review effort 4/5
Review effort 5/5
UI
aardvark
accessibility
amd64
api
arm64
auth
back-end
bgp
blog
bug
build
checkers
ci-cd
cleanup
cnpg
codex
core
dependencies
device-management
documentation
duplicate
dusk
ebpf
enhancement
eta 1d
eta 1hr
eta 3d
eta 3hr
feature
fieldsurvey
github_actions
go
good first issue
help wanted
invalid
javascript
k8s
log-collector
mapper
mtr
needs-triage
netflow
network-sweep
observability
oracle
otel
plug-in
proton
python
question
reddit
redhat
research
rperf
rperf-checker
rust
sdk
security
serviceradar-agent
serviceradar-agent-gateway
serviceradar-web
serviceradar-web-ng
siem
snmp
sysmon
topology
ubiquiti
wasm
wontfix
zen-engine
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
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!2400
No description provided.