SRQL: adding comprehensive tests #2426
No reviewers
Labels
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
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
carverauto/serviceradar!2426
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "refs/pull/2426/head"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Imported from GitHub pull request.
Original GitHub pull request: #1958
Original author: @mfreeman451
Original URL: https://github.com/carverauto/serviceradar/pull/1958
Original created: 2025-11-19T07:43:54Z
Original updated: 2025-11-19T08:40:41Z
Original head: carverauto/serviceradar:srql/comp_tests
Original base: main
Original merged: 2025-11-19T08:40:06Z 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:
Describe your changes
Issue ticket number and link
Code checklist before requesting a review
PR Type
Enhancement, Tests
Description
Add comparison operators (>, >=, <, <=) to SRQL parser and query builders
Implement range filtering for numeric fields across all query modules
Add comprehensive integration tests covering all SRQL entities
Refactor agent host resolution with dependency injection for testability
Expand test fixtures with complete schema and seed data for all entities
Diagram Walkthrough
File Walkthrough
6 files
Add comparison operators to FilterOp enumImplement range filtering for usage_percent fieldRemove if_index equality filter, add error handlingSupport text array bindings and range operatorsImplement Gt, Gte, Lt, Lte SQL generationInject lookup function for host resolution testability3 files
Add error handling for unsupported operatorsAdd error handling for unsupported operatorsAdd error handling in text filter macros4 files
New comprehensive integration test suiteAdd complete schema for all SRQL entitiesAdd seed data for pollers, services, metrics, logs, tracesAdd mock lookup function and test cases1 files
Add dead_code allow attribute to unused method1 files
Add comprehensive_queries test target configuration1 files
Remove trailing blank line in lookupHostIPs function2 files
Document range query operators and syntaxAdd comprehensive entity coverage requirementImported GitHub PR comment.
Original author: @qodo-code-review[bot]
Original URL: https://github.com/carverauto/serviceradar/pull/1958#issuecomment-3551255426
Original created: 2025-11-19T07:44:46Z
PR Compliance Guide 🔍
(Compliance updated until commit
github.com/carverauto/serviceradar@98038c941d)Below is a summary of compliance checks for this PR:
No security concerns identified
No security vulnerabilities detected by AI analysis. Human verification advised for critical code.🎫 No ticket provided
Codebase context is not defined
Follow the guide to enable codebase context checks.
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 audit logs: New range filtering paths for numeric fields execute without any added auditing/logging of
critical queries or outcomes, so it is unclear whether these actions are captured for
audit trails.
Referred Code
Compliance status legend
🟢 - Fully Compliant🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label
Previous compliance checks
Compliance check up to commit 6a45972
Log data exposure
Description: The
logstable uses a composite PRIMARY KEY that includestimestampwith second/subsecondgranularity, which may enable unintended enumeration or correlation of log events across
entities when exposed via SRQL if timestamps are directly filterable and returned without
redaction, potentially facilitating sensitive log mining; consider minimizing precision or
adding access controls in SRQL exposure.
schema.sql [66-81]
Referred Code
Sensitive information exposure
Description: The test asserts on the exact log message value
Connection failed, which hard-codespotentially sensitive operational strings into test outputs and may leak into CI artifacts
or logs; prefer using synthetic/non-sensitive placeholders or masking in assertions.
comprehensive_queries.rs [85-89]
Referred Code
Test data sensitivity
Description: Seeded logs include an ERROR entry with concrete trace/span IDs and message content that
could be treated as sensitive in some environments and may propagate to shared test
infrastructure or artifacts; replace with anonymized synthetic values or ensure test data
isolation.
seed.sql [149-157]
Referred Code
Telemetry PII exposure
Description: The
otel_tracestable stores potentially sensitive telemetry attributes and resourceattributes as TEXT without constraints, increasing risk if SRQL exposes these fields
broadly; ensure PII/secret scrubbing at ingestion or redaction in query responses.
schema.sql [120-142]
Referred Code
Infra fingerprinting risk
Description: The
cpu_metricsschema records host identifiers and device IDs which, when combined withprecise timestamps and usage data, could reveal infrastructure topology via SRQL; consider
access scoping or aggregation to prevent detailed host fingerprinting.
schema.sql [189-204]
Referred Code
🎫 No ticket provided
Codebase context is not defined
Follow the guide to enable codebase context checks.
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: Comprehensive Audit Trails
Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.
Status:
No audit logs: The new tests execute queries against multiple entities but do not assert that critical
actions (query execution, reads) are recorded with user ID, timestamp, and outcome, which
may be expected if the system requires audit trails.
Referred Code
Generic: Robust Error Handling and Edge Case Management
Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation
Status:
Sparse assertions: Tests primarily assert HTTP 200 and simple field counts without exercising or validating
error paths or edge cases (e.g., empty results, invalid queries, pagination cursors),
leaving robustness unverified.
Referred Code
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:
Log PII risk: Seeded logs include free-form 'body' and 'attributes' text which in
real environments could contain sensitive data; tests do not verify redaction or
structure, leaving logging practices unvalidated.
Referred Code
Generic: Security-First Input Validation and Data Handling
Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities
Status:
Input validation gaps: Tests send raw SRQL strings but do not cover validation for malformed inputs,
injection-like payloads, or auth/authorization checks, so secure handling of external
inputs is not verified.
Referred Code
Imported GitHub PR comment.
Original author: @qodo-code-review[bot]
Original URL: https://github.com/carverauto/serviceradar/pull/1958#issuecomment-3551258752
Original created: 2025-11-19T07:45:49Z
PR Code Suggestions ✨
Explore these optional code suggestions:
Adopt data-driven testing for scalability
Refactor the duplicated test functions into a single data-driven test. This
involves creating a structure for test cases and iterating over them in one test
function to improve scalability and reduce code duplication.
Examples:
rust/srql/tests/comprehensive_queries.rs [6-113]
Solution Walkthrough:
Before:
After:
Suggestion importance[1-10]: 7
__
Why: The suggestion correctly identifies significant code duplication across the new test functions and proposes a valid data-driven pattern that would improve the test suite's scalability and maintainability.
✅
Avoid exact floating-point value comparisonsSuggestion Impact:
The cpu_metrics test query was changed from an exact value (usage_percent:88.2) to a range (usage_percent:>88.1 usage_percent:<88.3), aligning with the suggestion. The tests were also refactored into a table-driven format, but the key suggested change was applied.code diff:
Modify the
cpu_metricstest to query for a range ofusage_percentvalues insteadof an exact match to avoid potential flakiness from floating-point inaccuracies.
rust/srql/tests/comprehensive_queries.rs [49-69]
[Suggestion processed]Suggestion importance[1-10]: 7
__
Why: The suggestion correctly identifies the risk of flaky tests due to exact floating-point comparisons and proposes a robust alternative using a range query, which is a best practice.
✅
Use a more informative panic messageSuggestion Impact:
The commit replaced expect("results should be an array") with unwrap_or_else that panics with a detailed message including the body when 'results' is not an array or missing.code diff:
Replace
expect()with a more descriptive panic message that includes theresponse body to aid debugging when the 'results' field is missing or not an
array.
rust/srql/tests/comprehensive_queries.rs [21]
[Suggestion processed]Suggestion importance[1-10]: 4
__
Why: The suggestion correctly points out that using
expectcan hide useful debug information, and the proposed change improves test failure diagnostics by including the response body in the panic message.Imported GitHub PR comment.
Original author: @qodo-code-review[bot]
Original URL: https://github.com/carverauto/serviceradar/pull/1958#issuecomment-3551428238
Original created: 2025-11-19T08:35:13Z
PR Compliance Guide 🔍
Below is a summary of compliance checks for this PR:
No security concerns identified
No security vulnerabilities detected by AI analysis. Human verification advised for critical code.🎫 No ticket provided
Codebase context is not defined
Follow the guide to enable codebase context checks.
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: Comprehensive Audit Trails
Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.
Status:
No audit logs: The new code and tests add query features and DNS lookup indirection without adding or
verifying audit logging for critical actions like queries or host resolution.
Referred Code
Generic: Security-First Input Validation and Data Handling
Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities
Status:
Input operators: The parser now accepts comparison operators in scalar filter values without visible
numeric type validation or bounds checking, which may be safe but warrants verification
against injection and type handling in downstream query builders.
Referred Code
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/1958#issuecomment-3551432066
Original created: 2025-11-19T08:36:27Z
PR Code Suggestions ✨
Explore these optional code suggestions:
Fix use-after-move error in match
In
build_filter, fix a compile error by binding the list value in theFilterValue::Listmatch arm and reconstructing theFilterValueinstead of movingthe partially-moved
value.rust/srql/src/parser.rs [196-240]
[To ensure code accuracy, apply this suggestion manually]Suggestion importance[1-10]: 10
__
Why: The suggestion correctly identifies a compile-time error in the new code due to a "use of partially moved value" in the
matchstatement, which is a critical bug that prevents the code from compiling.Correctly handle empty IN list filter
In
build_text_clause, fix the handling of an emptyINlist to return a1=0(always false) condition instead of
1=1(always true) to ensure correct queryresults.
rust/srql/src/query/otel_metrics.rs [413-430]
Suggestion importance[1-10]: 9
__
Why: The suggestion correctly identifies a critical bug where an empty
INclause evaluates totrueinstead offalse, leading to incorrect query results. The proposed fix of returning1=0is correct and essential for query correctness.Correctly handle empty NOT IN clause
In
add_text_condition, for an emptyNOT INclause, explicitly add a1=1SQLcondition to the
clausesvector to represent an always-true condition.rust/srql/src/query/trace_summaries.rs [292-300]
Suggestion importance[1-10]: 2
__
Why: The suggestion correctly states that
NOT IN ()is always true, but the current code's behavior of adding no clause is logically equivalent toAND TRUEand is not a bug. The proposed change to add1=1is a minor style improvement for explicitness, not a bug fix.Refactor filter logic into shared helpers
The filter logic is duplicated across several query modules, leading to
inconsistent SQL generation for operators like 'IN'. This should be refactored
into a centralized, reusable utility to ensure consistency and improve
maintainability.
Examples:
rust/srql/src/query/logs.rs [441-487]
rust/srql/src/query/otel_metrics.rs [391-436]
Solution Walkthrough:
Before:
After:
Suggestion importance[1-10]: 9
__
Why: The suggestion correctly identifies significant code duplication and resulting inconsistencies in SQL generation for filters across multiple query modules, which is a critical design issue.
Imported GitHub PR comment.
Original author: @qodo-code-review[bot]
Original URL: https://github.com/carverauto/serviceradar/pull/1958#issuecomment-3551439129
Original created: 2025-11-19T08:37:52Z
CI Feedback 🧐
A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
Action: build
Failed stage: Test [❌]
Failed test name: ""
Failure summary:
The action failed due to Bazel loading-phase errors caused by OCI base image fetch failures from
Docker Hub, which resulted in many docker/image targets failing to load:
- Multiple
oci_pullfetcheshit Docker Hub rate limiting and/or unauthenticated access errors (HTTP 429 Too Many Requests) when
retrieving image manifests:
- Examples:
-
@@rules_oci++oci+nginx_alpine_linux_amd64atexternal/rules_oci+/oci/private/pull.bzl:155:13-
@@rules_oci++oci+node_20_alpine_linux_amd64atexternal/rules_oci+/oci/private/pull.bzl:155:13-
@@rules_oci++oci+ubuntu_jammy_linux_amd64atexternal/rules_oci+/oci/private/pull.bzl:155:13-
@@rules_oci++oci+ubuntu_nobleand@@rules_oci++oci+debian_bookworm_slim_linux_amd64similarly failed- Explicit warnings show 429
responses from Docker Hub for
nginx,node,ubuntu,debian,alpineimage manifest URLs (e.g., lines6190, 6339, 6465, 6783, 7085).
- Bazel then reported “no such package” for the above external
repositories and “errors encountered while analyzing target … it will not be built,” culminating in:
- ERROR: command succeeded, but there were loading phase errors
- ERROR: Build did NOT complete
successfully
- Although all tests passed, the overall build failed because many top-level image
targets (under
//docker/images:...) could not be analyzed/built due to missing image manifests.In short: Docker Hub rate limiting/unauthenticated pulls prevented
rules_ocifrom fetching requiredbase images, causing Bazel loading-phase errors and failing the CI job.
Relevant error logs: