Updates/srql param queries #2226
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!2226
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "refs/pull/2226/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: #1645
Original author: @mfreeman451
Original URL: https://github.com/carverauto/serviceradar/pull/1645
Original created: 2025-09-18T02:39:08Z
Original updated: 2025-09-18T03:13:24Z
Original head: carverauto/serviceradar:updates/srql_param_queries
Original base: main
Original merged: 2025-09-18T03:13:20Z by @mfreeman451
PR Type
Enhancement
Description
Add parameterized query support to SRQL translator
Replace string literal concatenation with parameter placeholders
Update client interface for parameter binding
Enhance test coverage for parameter validation
Diagram Walkthrough
File Walkthrough
proton_client.ml
Add parameterized query execution supportocaml/srql/lib/proton_client.ml
execute_with_paramsfunction for parameterized queriestranslate_to_sqlto use new parameter-aware translationtranslate_and_executeto use parametersproton_client.mli
Expose parameterized query interfaceocaml/srql/lib/proton_client.mli
execute_with_paramsfunction signaturetranslationtype with SQL and parameterstranslatefunction for getting SQL with parameterstranslator.ml
Core parameterized translation implementationocaml/srql/lib/translator.ml
column_value_ofandrender_valuefunctionsquery_with_paramstype fromtranslate_queryrun_live_srql.ml
Update live test for parameterized queriesocaml/srql/test/run_live_srql.ml
translatefunction with parameterstest_bounded_unbounded.ml
Fix bounded/unbounded tests for new APIocaml/srql/test/test_bounded_unbounded.ml
test_query_engine.ml
Comprehensive parameter validation testsocaml/srql/test/test_query_engine.ml
sql_ofwithtranslation_offunctiontest_sql_sanitize.ml
Update sanitization tests for parametersocaml/srql/test/test_sql_sanitize.ml
Imported GitHub PR comment.
Original author: @qodo-code-review[bot]
Original URL: https://github.com/carverauto/serviceradar/pull/1645#issuecomment-3305200738
Original created: 2025-09-18T02:39:51Z
PR Reviewer Guide 🔍
Here are some key observations to aid the review process:
Placeholder Format
The translator emits placeholders as "{{pN}}". Ensure Proton prepare/execute APIs accept this exact named placeholder syntax. If Proton expects a different style (e.g., {name}, :name, ?), parameter binding may fail at runtime.
Type Coercion
Mapping SRQL values to Proton.Column uses Int64 for Int and Int32 for Bool. Verify Proton server-side types/operators (e.g., position, IN, LIKE, has) accept these column types and implicit casts; mismatches could cause errors or incorrect filtering.
Wrapper Logic
The new wrap_table_if_needed function operates on arbitrary SQL. Confirm it preserves case/spacing and does not incorrectly wrap complex FROM clauses (joins, subqueries, CTEs). Edge cases might regress valid queries.
Imported GitHub PR comment.
Original author: @qodo-code-review[bot]
Original URL: https://github.com/carverauto/serviceradar/pull/1645#issuecomment-3305202170
Original created: 2025-09-18T02:40:53Z
PR Code Suggestions ✨
Explore these optional code suggestions:
Remove insecure direct value interpolation
Remove the special handling for
Exprvalues inINclauses to prevent potentialSQL injection, forcing all values to be parameterized.
ocaml/srql/lib/translator.ml [89-96]
Suggestion importance[1-10]: 8
__
Why: The suggestion correctly points out that directly interpolating an
Exprfor anINclause undermines the PR's goal of preventing SQL injection and should be removed for better security.Correctly handle special date strings
Always translate "today" and "yesterday" to their respective SQL functions
(
today(),yesterday()) instead of parameterizing them as strings.ocaml/srql/lib/translator.ml [57-70]
Suggestion importance[1-10]: 7
__
Why: The suggestion correctly identifies a functional bug in the new logic where special date strings
todayandyesterdayare incorrectly parameterized instead of being translated to SQL functions.Imported GitHub PR comment.
Original author: @qodo-code-review[bot]
Original URL: https://github.com/carverauto/serviceradar/pull/1645#issuecomment-3305254597
Original created: 2025-09-18T03:08:03Z
CI Feedback 🧐
A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
Action: build-test
Failed stage: Install dependencies (with tests) [❌]
Failure summary:
The action failed during OPAM dependency resolution due to incompatible OCaml compiler constraints:
- The environment is pinned to
ocaml-base-compiler = 5.3.0(invariant), but dependencies requirepackages that only support OCaml < 5.0.
- Dependency chain:
deps-of-srql-translator → dream >=1.0.0, which conflicts withalpha5 → dream-httpaf >= 1.0.0alpha4 → h2 < 0.13.0 → ocaml < 5.0ocaml-base-compiler = 5.3.0.- OPAM proposed alternative compilers (
dkml-base-compiler,ocaml-variants) are incompatible or require unmet availability conditions:-
ocaml-variants →ocaml-betarequiresenable-ocaml-beta-repository.-
ocaml-variants → system-msvcrequiresos ="win32", not satisfied on the runner.- As a result, OPAM reported a package conflict and exited
with code 20.
Relevant error logs: