1947 srql rewrite for pg #2416
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!2416
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "refs/pull/2416/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: #1948
Original author: @mfreeman451
Original URL: https://github.com/carverauto/serviceradar/pull/1948
Original created: 2025-11-17T01:20:30Z
Original updated: 2025-11-17T08:26:40Z
Original head: carverauto/serviceradar:1947-srql-rewrite-for-pg
Original base: main
Original merged: 2025-11-17T08:25:53Z 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, Bug fix
Description
Complete rewrite of SRQL query service from OCaml to Rust with Diesel ORM backend targeting CNPG (CloudNativePG)
Implemented new Rust SRQL service with modular architecture:
parser.rs: DSL parser converting key:value syntax to AST with support for filters, ordering, limits, and time rangesquery/modules: Diesel-based query builders for devices, events, and logs entities with field filtering and orderingserver.rs: Axum HTTP server with/healthz,/api/query, and/translateendpoints and API key authenticationmodels.rs: Diesel data models for DeviceRow, EventRow, LogRow with JSON serializationtime.rs: Time range parsing utilities supporting relative durations, keywords, and absolute rangespagination.rs: Cursor-based pagination with base64 URL-safe encodingconfig.rs: Environment-based configuration loader with SRQL_* prefixed settingsdb.rs: Async PostgreSQL connection pooling using diesel-async and bb8Removed deprecated Proton-specific
_tp_timefields from TypeScript types and UI componentsUpdated all build infrastructure:
rust/srqlcrateUpdated Kubernetes manifests with SRQL service deployment for base, staging, and production environments
Updated documentation and specifications to reflect Rust implementation and CNPG backend migration
Removed entire OCaml SRQL codebase (~200 files) including Proton client integration
Diagram Walkthrough
File Walkthrough
3 files
logs.ts
Remove deprecated timestamp field from logs typeweb/src/types/logs.ts
_tp_timefield from theLoginterfaceSweepResultsTable.tsx
Remove Proton-specific timestamp field from sweep resultsweb/src/components/Network/SweepResultsTable.tsx
_tp_timefield fromSweepResultinterface (Proton-specifictimestamp)
timestampfieldinstead of
_tp_timeDashboard.tsx
Remove Proton-specific timestamp field from events dashboardweb/src/components/Events/Dashboard.tsx
event._tp_timefrom timestamp candidates array(Proton-specific field)
event.event_timestamp16 files
events.rs
Add Diesel query builder for events entityrust/srql/src/query/events.rs
datacontenttype, remote_addr, host, specversion, severity,
short_message, version, level)
event_timestampwith default descending sortexecute()andto_debug_sql()functions for query planninglogs.rs
Add Diesel query builder for logs entityrust/srql/src/query/logs.rs
service_version, service_instance, scope_name, scope_version,
severity_text, body, severity_number)
timestampandseverity_numberwith defaultdescending sort
execute()andto_debug_sql()functions for query planningparser.rs
Implement SRQL DSL parser with AST generationrust/srql/src/parser.rs
in:), filters, ordering (sort:/order:),limits, and time filters
complex values
time filters
devices.rs
Add Diesel query builder for devices entityrust/srql/src/query/devices.rs
entity
ip, mac, poller_id, agent_id, is_available, device_type, service_type,
service_status, and discovery_sources
version_info
execute()andto_debug_sql()functions with default orderingby last_seen descending
time.rs
Add time range parsing utilities for SRQLrust/srql/src/time.rs
ranges
and absolute ranges
YYYY-MM-DD HH:MM:SS
mod.rs
Add query engine orchestrating entity-specific executorsrust/srql/src/query/mod.rs
devices, events, and logs entities
QueryEnginestruct managing connection pooling and queryplanning
execute_query()andtranslate()methods for query executionand SQL debugging
models.rs
Add Diesel data models for CNPG entitiesrust/srql/src/models.rs
DeviceRow,EventRow, andLogRowstructs with Diesel queryablederives
into_json()methods for each model to serialize databaserows to JSON
entities
config.rs
Add environment-based configuration loaderrust/srql/src/config.rs
environment variables
size, API key, and limits
settings
server.rs
Add Axum-based HTTP server with endpointsrust/srql/src/server.rs
/healthz,/api/query, and/translateendpointsx-api-keyheaderschema.rs
Add Diesel schema definitions for CNPG tablesrust/srql/src/schema.rs
unified_devices,events, andlogstable structures with columntypes
Jsonb, Bool)
type→event_type)pagination.rs
Add cursor-based pagination helpersrust/srql/src/pagination.rs
encode_cursor()anddecode_cursor()functions with validationdb.rs
Add async Postgres connection poolingrust/srql/src/db.rs
connect_pool()function creating async Postgres connectionpool
lib.rs
Add library root and bootstrap functionrust/srql/src/lib.rs
run()bootstrap function loading config and starting serverand query engine
telemetry.rs
Add tracing initialization utilitiesrust/srql/src/telemetry.rs
EnvFilterfor configurable log levels (defaults to info)OnceCellstate.rs
Add application state containerrust/srql/src/state.rs
AppStatestruct for Axum state managementmain.rs
Add main entry point for SRQL binaryrust/srql/src/main.rs
run()function1 files
error.rs
Add error types and Axum response handlingrust/srql/src/error.rs
ServiceErrorenum with Axumresponse conversion
error variants
serialization
20 files
entrypoint-srql.sh
Migrate entrypoint from OCaml Proton to Rust CNPGdocker/compose/entrypoint-srql.sh
configuration
mappings
parameter support
packages.bzl
Update SRQL package metadata for Rust buildpackaging/packages.bzl
//ocaml/srql:srql_serverto//rust/srql:srql_binlibraries
Dockerfile.rbe
Replace OCaml RBE image with Rust-compatible basedocker/Dockerfile.rbe
support
Dockerfile.rpm.srql
Migrate SRQL RPM build from OCaml to Rustdocker/rpm/Dockerfile.rpm.srql
rust:1.86-slimserviceradar-srql.yaml
Add Kubernetes deployment for Rust SRQL servicek8s/demo/base/serviceradar-srql.yaml
/healthzendpointdefinition
Dockerfile.srql-builder
Migrate SRQL builder image from OCaml to Rustdocker/builders/Dockerfile.srql-builder
rust:1.86-slimdocker-compose.yml
Update docker-compose SRQL config for CNPG backenddocker-compose.yml
PROTON_PORT, PROTON_TLS, etc.)
CNPG_USERNAME, CNPG_SSLMODE)
/healthto/healthzBUILD
Remove OCaml RBE platforms and update executor versionbuild/rbe/BUILD
ocaml_rbe_ocamloptandocaml_rbe_ocamlc)executor
BUILD.bazel
Switch SRQL Docker image from OCaml to Rust binarydocker/images/BUILD.bazel
//ocaml/srql:srql_serverto//rust/srql:srql_bin@serviceradar_srql_linux_amd64to@ubuntu_noblecomponents.json
Update SRQL packaging from OCaml to Rust build configurationpackaging/components.json
libev4,libgmp10) toPostgreSQL client (
libpq5)ocamltorustand source path fromocaml/srqltorust/srqldocker/builders/Dockerfile.srql-builderBUILD.bazel
Update build platform RBE executor image referencesbuild/platforms/BUILD.bazel
gcr.io/buildbuddy-io/executor:latesttodocker://ghcr.io/carverauto/serviceradar/rbe-executor:v1.0.11kustomization.yaml
Add SRQL service image to production Kustomize configurationk8s/demo/prod/kustomization.yaml
ghcr.io/carverauto/serviceradar-srqlwithlatesttagbuildbuddy.yaml
Update BuildBuddy RBE executor container imagebuildbuddy.yaml
gcr.io/buildbuddy-io/executor:latesttodocker://ghcr.io/carverauto/serviceradar/rbe-executor:v1.0.11BUILD.bazel
New Bazel build rules for Rust SRQL servicerust/srql/BUILD.bazel
srql_liblibrary with glob source patterns andsrql_binbinaryentry point
all_crate_depsmacrokustomization.yaml
Add SRQL service image to staging Kustomize configurationk8s/demo/staging/kustomization.yaml
ghcr.io/carverauto/serviceradar-srqlwithlatesttagBUILD.bazel
Update RBE executor version and cwalk alias targetBUILD.bazel
cwalkalias target from external repository to local path//third_party/cwalk:cwalkDockerfile.srql
Update Docker Compose SRQL image to use Rust builddocker/compose/Dockerfile.srql
latestRustimage
stack
kustomization.yaml
Add SRQL service to base Kustomize resourcesk8s/demo/base/kustomization.yaml
serviceradar-srql.yamlto resources listCargo.toml
Add Rust SRQL crate to workspace membersCargo.toml
rust/srqlto workspace members listmain.yml
Remove OCaml test filter from CI workflow.github/workflows/main.yml
-ocamlfrom test tag filters in CI pipeline2 files
MODULE.bazel
Remove OCaml dependencies and update RBE imageMODULE.bazel
makeheaders,rules_ocaml,tools_opam)Cargo.toml
New Rust SRQL service Cargo dependencies manifestrust/srql/Cargo.toml
PostgreSQL support
and database pooling (diesel-async)
handling
11 files
spec.md
Add SRQL Rust implementation specificationopenspec/changes/replace-srql-dsl/specs/srql/spec.md
dashboards
service
srql-language-reference.md
Update SRQL documentation for Rust implementationdocs/docs/srql-language-reference.md
OCaml
ocaml/srqltorust/srqlrust/srql/src/queryAGENTS.md
Update developer documentation for Rust SRQLAGENTS.md
OCaml
ocaml/srql/torust/srql/project.md
Update project overview for Rust SRQL serviceopenspec/project.md
ocaml/srqltorust/srqlREADME.md
Add RBE executor image build and deployment guidek8s/buildbuddy/README.md
distinction
image
use upstream
03-proton.md
Complete architecture rewrite from Proton to CNPG unified telemetrystoresr-architecture-and-design/prd/03-proton.md
+ Apache AGE stack
retention policies
discovery outputs
traversals
→ dashboards/alerts
tasks.md
SRQL Rust translator implementation task breakdownopenspec/changes/replace-srql-dsl/tasks.md
integration with CNPG
plumbing
service cutover
proposal.md
SRQL DSL replacement proposal from OCaml to Rustopenspec/changes/replace-srql-dsl/proposal.md
schemas
documentation updates
search-planner-operations.md
Update SRQL service path in operational documentationdocs/docs/search-planner-operations.md
ocaml/srqltorust/srqldocumentation
BUILD.md
Remove OCaml from build system dependenciesBUILD.md
ocamlfrom system dependencies installation commandto Rust
repo_lint.md
Update repo lint documentation test file referencedocs/LF/repo_lint.md
ocaml/test_boolean_query.mltorust/srql/src/lib.rs101 files
Imported GitHub PR comment.
Original author: @qodo-code-review[bot]
Original URL: https://github.com/carverauto/serviceradar/pull/1948#issuecomment-3539606843
Original created: 2025-11-17T01:22:01Z
PR Compliance Guide 🔍
(Compliance updated until commit
github.com/carverauto/serviceradar@66ae3e4358)Below is a summary of compliance checks for this PR:
Missing authentication
Description: API key authentication compares the provided header only to an optional configured key,
and if none is configured it fully disables authentication, which could expose query
endpoints if SRQL_API_KEY or KV key is not set.
server.rs [96-111]
Referred Code
Credential exposure via env
Description: DATABASE_URL is constructed by embedding the plaintext password in the URL, risking
exposure via process/env or logs; although percent-encoded, it still sets DATABASE_URL env
var which can leak in diagnostics.
entrypoint-srql.sh [120-133]
Referred Code
SQL disclosure
Description: Translate endpoint returns generated SQL text to authorized clients which could aid an
attacker in reconnaissance if API key is weak or disabled.
mod.rs [120-136]
Referred Code
🎫 #1947
limits, and time ranges.
secured by API key.
limiting, timeouts, API key).
packaging.
_tp_time fields.
published.
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:
Missing audit logs: New endpoints authenticate via API key but do not emit structured audit logs for critical
actions like query execution or translation requests (who, what, result), making
auditability uncertain.
Referred Code
Generic: Robust Error Handling and Edge Case Management
Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation
Status:
Generic internal wrap: Database pool acquisition and query load errors are wrapped as generic internal errors
without contextual details about the operation or inputs, which may limit actionable
debugging.
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:
Secret exposure risk: The entrypoint echoes connection target details and constructs DATABASE_URL; although it
avoids printing passwords, it logs database target which could be sensitive in some
environments and lacks structured logging.
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:
Limited validation: The SRQL parser accepts arbitrary field names as filters and defers validation to query
layers, which may allow unsupported fields to pass silently in some paths and merits
verification of full validation coverage across entities.
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 66ae3e4
Weak API key auth
Description: API key authentication relies solely on the presence and equality of the X-API-Key header
without secondary protections (e.g., per-user scoping or rotation enforcement), making
header leakage sufficient to access query endpoints.
server.rs [97-111]
Referred Code
Credential exposure risk
Description: DATABASE_URL is constructed by embedding a percent-encoded password directly in the URL,
which can be exposed via process env or runtime diagnostics; consider using libpq env vars
or secret mounts to avoid credential exposure.
entrypoint-srql.sh [120-138]
Referred Code
SQL injection surface
Description: The text filter macros pass user-supplied strings into Diesel eq/ilike clauses; while
Diesel parameterizes values, ensure no raw SQL fragments are constructed from user input
elsewhere to prevent SQL injection (verify all uses of sql! macros avoid user
concatenation).
mod.rs [1-66]
Referred Code
🎫 #1947
and time ranges.
key auth.
limiting.
service.
connectivity.
production workloads.
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:
Missing audit logs: New HTTP endpoints and API key checks do not emit audit logs for access attempts,
authentication outcomes, or query executions, making it unclear if critical actions are
recorded.
Referred Code
Generic: Robust Error Handling and Edge Case Management
Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation
Status:
Input validation gaps: Filters accept raw strings for LIKE and IN without visible normalization or length limits,
and server returns parsed SQL without pagination limits in translate, which may require
additional validation and safeguards.
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:
Logs may expose DB: Entrypoint echoes database target details which may reveal infrastructure metadata in
container logs; also no evidence that application avoids logging sensitive request data.
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:
Unbounded inputs: The SRQL parser accepts arbitrary tokens, list sizes, and patterns without explicit
maximum lengths or field whitelisting enforcement at the parser level, which could lead to
denial-of-service or excessive query complexity.
Referred Code
Compliance check up to commit 49b3464
Weak API key auth
Description: API key authentication relies on exact header match of 'x-api-key' without rate limiting
or optional rotation, which risks brute-force if exposed; ensure TLS and consider rate
limiting and secret management.
server.rs [81-90]
Referred Code
Info exposure in logs
Description: DATABASE_URL is constructed from environment values and printed partially to logs, which
may leak host/user/database info; ensure secrets (password) are not logged and consider
suppressing URL output entirely.
entrypoint-srql.sh [103-136]
Referred Code
Broad LIKE patterns
Description: The DSL parser accepts list and scalar values and maps '%' to LIKE without explicit
escaping; while Diesel parameterizes inputs, LIKE patterns could enable broad scans or
unexpected matches—verify expected behavior and consider explicit wildcard policy.
parser.rs [206-286]
Referred Code
Secrets in env vars
Description: Falls back to reading DATABASE_URL from environment which may include credentials in plain
text; ensure secrets are injected securely and not logged elsewhere.
config.rs [74-83]
Referred Code
🎫 #1947
CloudNativePG (PostgreSQL).
ordering, limits, and time ranges.
filters and ordering.
authentication.
maximums.
address, API key, limits, and CORS.
absolute ranges.
RPM packaging.
availability and query performance.
(manual browser testing).
systemd units and RPM install.
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:
Missing audit logging: New API endpoints execute queries without explicit audit logs of requester identity,
action, and outcome beyond generic tracing, which may be acceptable if centralized logging
is configured elsewhere.
Referred Code
Generic: Robust Error Handling and Edge Case Management
Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation
Status:
Partial edge handling: Filters fall through silently for unsupported fields (no-op) which may hide user errors
and lacks explicit validation feedback for unknown filter names.
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:
Potential sensitive data: The JSON responses include raw attributes fields (e.g., logs attributes and
resource_attributes) whose contents are not scrubbed and may contain sensitive data
depending on upstream ingestion.
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 scope: The SRQL parser and query builders validate many cases, but unknown filter fields are
accepted as no-ops and body size/rate limits are not visible here, requiring confirmation
that upstream limits and auth are enforced.
Referred Code
Imported GitHub PR comment.
Original author: @qodo-code-review[bot]
Original URL: https://github.com/carverauto/serviceradar/pull/1948#issuecomment-3539609094
Original created: 2025-11-17T01:23:34Z
PR Code Suggestions ✨
Latest suggestions up to
3cdc1ddDo not trust client-exposed env
Remove the fallback to
process.env.NEXT_PUBLIC_API_KEYingetApiKeyto avoidusing a client-exposed key for server-side requests.
web/src/lib/config.ts [82-85]
[To ensure code accuracy, apply this suggestion manually]Suggestion importance[1-10]: 9
__
Why: The suggestion correctly identifies a security risk where a client-exposed environment variable (
NEXT_PUBLIC_API_KEY) could be used for server-side authentication, which should be avoided.✅
Enforce SRQL Nginx route precedenceSuggestion Impact:
The /api/query location was updated to use the ^~ modifier, ensuring correct route precedence.code diff:
Add the
^~modifier to thelocation /api/queryblock in the Nginx configurationto ensure it takes precedence over the more general regex-based
/api/locationblock.
k8s/demo/base/configmap.yaml [34-40]
[Suggestion processed]Suggestion importance[1-10]: 8
__
Why: This is a critical fix for Nginx configuration. Without the
^~modifier, a request for/api/querywould be incorrectly handled by the subsequent regex location block, causing SRQL queries to fail. The suggestion correctly identifies and fixes this routing bug.✅
Fix SRQL ingress routing targetSuggestion Impact:
The commit changed the ingress backend service from serviceradar-web to serviceradar-kong and updated the port from 3000 to 8000 for the /api/query path.code diff:
Update the ingress for
/api/queryto route traffic directly toserviceradar-konginstead of
serviceradar-webto simplify the request path.k8s/demo/staging/ingress.yaml [97-103]
[Suggestion processed]Suggestion importance[1-10]: 6
__
Why: The suggestion correctly identifies that routing
/api/querythroughserviceradar-webis inefficient. While the current setup does proxy to Kong, the proposed change to route directly from the ingress toserviceradar-kongsimplifies the request path and improves maintainability.✅
Fix SRQL ingress routingSuggestion Impact:
The commit updated the ingress backend service name from serviceradar-web to serviceradar-kong and changed the port from 3000 to 8000, aligning with the suggestion.code diff:
Correct the ingress routing for
/api/queryin the staging environment. Itcurrently points to the web service instead of the Kong gateway, which would
cause queries to fail and bypass authentication.
k8s/demo/staging/ingress.yaml [97-103]
[Suggestion processed]Suggestion importance[1-10]: 9
__
Why: This suggestion identifies a critical misconfiguration in the staging ingress that would route authenticated queries to the wrong service, bypassing the new SRQL backend and its authentication layer.
✅
Support numeric IN/NOT IN on severitySuggestion Impact:
The commit implemented IN/NOT IN handling for severity_number, parsing list values, applying eq_any/ne_all, and updated the error message as suggested (with a minor difference of early return on empty lists).code diff:
Add support for
INandNOT INoperations on theseverity_numberfilter to handlelist values like
severity_number:(1,2,3), aligning its behavior with otherfilterable fields.
rust/srql/src/query/logs.rs [100-113]
[To ensure code accuracy, apply this suggestion manually]Suggestion importance[1-10]: 7
__
Why: The suggestion correctly identifies that
severity_numberfiltering lacks support for list operations (IN/NOT IN), which is inconsistent with other fields and improves feature completeness.✅
Sanitize RPM version correctlySuggestion Impact:
The commit replaced the simple sed substitution with the proposed RAW and SANITIZED variables and used RPM_VERSION_SANITIZED in the rpmbuild define, matching the suggested robust sanitization.code diff:
Improve RPM version string sanitization to be more robust. The current method of
replacing hyphens with underscores is insufficient and may lead to invalid
version strings for
rpmbuild.docker/rpm/Dockerfile.rpm.srql [60-64]
[Suggestion processed]Suggestion importance[1-10]: 6
__
Why: The suggestion correctly identifies that the current RPM version sanitization is not robust, but the proposed
sedcommand is overly complex and may not be necessary if the inputVERSIONformat is controlled.✅
Fix unsafe URL encoding for passwordsSuggestion Impact:
The commit rewrote urlencode to iterate by bytes with LC_ALL=C, switched to building 'out' instead of 'output', used printf -v for hex encoding, and handled characters similarly, implementing the suggested safer UTF-8 aware encoding.code diff:
Fix the
urlencodeshell function to correctly handle multi-byte UTF-8 andspecial characters by iterating byte-wise, preventing malformed database
connection URLs.
docker/compose/entrypoint-srql.sh [20-41]
[Suggestion processed]Suggestion importance[1-10]: 9
__
Why: This is a critical bug fix for the
urlencodefunction, which would fail to correctly encode multi-byte UTF-8 characters or certain special characters in passwords, leading to connection failures.Avoid global PG path pollution
Avoid globally setting PostgreSQL library paths in the RBE image. Instead, make
them optional and controlled by a build argument to prevent potential conflicts
with other build actions.
docker/Dockerfile.rbe [55-62]
[To ensure code accuracy, apply this suggestion manually]Suggestion importance[1-10]: 7
__
Why: The suggestion correctly points out a potential issue with globally setting linker paths in a shared build image, which could cause conflicts. Making these paths optional improves the image's reusability and robustness.
✅
Enforce positive limit during parseSuggestion Impact:
The commit added a check for parsed <= 0 and returns an InvalidRequest error with the message "limit must be a positive integer", matching the suggestion.code diff:
Validate that the
limitparameter is a positive integer during parsing toprovide earlier and clearer error feedback to the user.
rust/srql/src/parser.rs [95-101]
[To ensure code accuracy, apply this suggestion manually]Suggestion importance[1-10]: 4
__
Why: The suggestion correctly points out that negative limits are not validated during parsing, but the impact is low because the value is clamped to a valid range later in the
determine_limitfunction.Previous suggestions
✅ Suggestions up to commit
49b3464Refactor query builders to reduce duplication
The query building logic in
rust/srql/src/query/fordevices,events, andlogshas highly duplicated
apply_filterfunctions. This can be refactored using ageneric abstraction like a macro to reduce boilerplate and improve
maintainability.
Examples:
rust/srql/src/query/events.rs [76-314]
rust/srql/src/query/logs.rs [71-271]
Solution Walkthrough:
Before:
After:
Suggestion importance[1-10]: 8
__
Why: The suggestion correctly identifies significant code duplication in the
apply_filterfunctions across three core files, and addressing this would substantially improve the new service's maintainability and extensibility.✅
Handle negated array containment filtersSuggestion Impact:
The commit updated the discovery_sources filter to apply a NOT around the array containment expression when the operator is NotIn, implementing the suggested behavior.code diff:
Implement handling for negated filters (
FilterOp::NotIn) on thediscovery_sourcesfield by wrapping the SQL expression with aNOToperator.rust/srql/src/query/devices.rs [212-223]
Suggestion importance[1-10]: 8
__
Why: The suggestion correctly identifies a bug where the negation operator for the
discovery_sourcesfilter is parsed but not implemented, leading to incorrect query results for negated filters.✅
Error on unsupported filter fieldsSuggestion Impact:
The commit changed the default match arm to return ServiceError::InvalidRequest for unsupported filter fields, matching the suggestion.code diff:
Modify the
apply_filterfunction inevents.rsto return an error for unsupportedfilter fields instead of silently ignoring them.
rust/srql/src/query/events.rs [76-314]
Suggestion importance[1-10]: 6
__
Why: The suggestion correctly identifies that silently ignoring unknown filters is poor API design and can hide user errors, so returning an error improves robustness and provides better feedback.
✅
Improve graph ingestion performanceSuggestion Impact:
The commit replaced the FOREACH loop with WITH + UNWIND over services and preserved the MERGE operations, implementing the suggested performance improvement.code diff:
Replace the
FOREACHclause with a more performantUNWINDclause in the Cypherquery to optimize bulk graph ingestion.
sr-architecture-and-design/prd/03-proton.md [143-152]
Suggestion importance[1-10]: 6
__
Why: The suggestion correctly identifies a performance anti-pattern in the Cypher query example and proposes using
UNWINDinstead ofFOREACHfor better bulk ingestion performance, which is a valid best practice.Reduce health check interval
Reduce the health check interval from 30 seconds to 10 seconds for faster
failure detection and recovery in the local development environment.
docker-compose.yml [363-367]
Suggestion importance[1-10]: 4
__
Why: The suggestion correctly points out that a 30-second health check interval is long for a local development environment, and reducing it would allow for faster failure detection.