Updates/srql param queries #2226

Merged
mfreeman451 merged 3 commits from refs/pull/2226/head into main 2025-09-18 03:13:20 +00:00
mfreeman451 commented 2025-09-18 02:39:08 +00:00 (Migrated from github.com)
Owner

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

flowchart LR
  A["SRQL Query"] --> B["Parse & Plan"]
  B --> C["Translate with Parameters"]
  C --> D["SQL + Params"]
  D --> E["Execute with Params"]
  E --> F["Proton Database"]

File Walkthrough

Relevant files
Enhancement
proton_client.ml
Add parameterized query execution support                               

ocaml/srql/lib/proton_client.ml

  • Add execute_with_params function for parameterized queries
  • Refactor translate_to_sql to use new parameter-aware translation
  • Extract table wrapping logic into separate function
  • Update translate_and_execute to use parameters
+27/-12 
proton_client.mli
Expose parameterized query interface                                         

ocaml/srql/lib/proton_client.mli

  • Add execute_with_params function signature
  • Define translation type with SQL and parameters
  • Add translate function for getting SQL with parameters
+13/-0   
translator.ml
Core parameterized translation implementation                       

ocaml/srql/lib/translator.ml

  • Implement parameter builder system for query values
  • Replace string concatenation with parameter placeholders
  • Add column_value_of and render_value functions
  • Return query_with_params type from translate_query
+199/-168
Tests
run_live_srql.ml
Update live test for parameterized queries                             

ocaml/srql/test/run_live_srql.ml

  • Update to use new translate function with parameters
  • Add parameter printing functionality
  • Support parameterized queries in streaming mode
  • Handle prepared statements for parameter binding
+43/-17 
test_bounded_unbounded.ml
Fix bounded/unbounded tests for new API                                   

ocaml/srql/test/test_bounded_unbounded.ml

  • Update test calls to use new translation structure
  • Extract SQL from translation object
+3/-2     
test_query_engine.ml
Comprehensive parameter validation tests                                 

ocaml/srql/test/test_query_engine.ml

  • Replace sql_of with translation_of function
  • Add parameter validation in all test cases
  • Verify parameter values match expected strings/numbers
  • Update assertions for placeholder-based SQL
+103/-39
test_sql_sanitize.ml
Update sanitization tests for parameters                                 

ocaml/srql/test/test_sql_sanitize.ml

  • Update sanitization tests for parameter-based approach
  • Remove literal escaping validation logic
  • Verify parameters capture injection payloads safely
  • Update fuzz testing for parameter validation
+18/-41 

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 ```mermaid flowchart LR A["SRQL Query"] --> B["Parse & Plan"] B --> C["Translate with Parameters"] C --> D["SQL + Params"] D --> E["Execute with Params"] E --> F["Proton Database"] ``` <details> <summary><h3> File Walkthrough</h3></summary> <table><thead><tr><th></th><th align="left">Relevant files</th></tr></thead><tbody><tr><td><strong>Enhancement</strong></td><td><table> <tr> <td> <details> <summary><strong>proton_client.ml</strong><dd><code>Add parameterized query execution support</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></summary> <hr> ocaml/srql/lib/proton_client.ml <ul><li>Add <code>execute_with_params</code> function for parameterized queries<br> <li> Refactor <code>translate_to_sql</code> to use new parameter-aware translation<br> <li> Extract table wrapping logic into separate function<br> <li> Update <code>translate_and_execute</code> to use parameters</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/1645/files#diff-474590ab3468b98ee363f3cd78535c90bc83cf3b2e77ae9871db741e2d8dea06">+27/-12</a>&nbsp; </td> </tr> <tr> <td> <details> <summary><strong>proton_client.mli</strong><dd><code>Expose parameterized query interface</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></summary> <hr> ocaml/srql/lib/proton_client.mli <ul><li>Add <code>execute_with_params</code> function signature<br> <li> Define <code>translation</code> type with SQL and parameters<br> <li> Add <code>translate</code> function for getting SQL with parameters</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/1645/files#diff-2b549609ea8b875de4a039282ea09be7655ebda3f27f078ea603b8e77b9e31c6">+13/-0</a>&nbsp; &nbsp; </td> </tr> <tr> <td> <details> <summary><strong>translator.ml</strong><dd><code>Core parameterized translation implementation</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></summary> <hr> ocaml/srql/lib/translator.ml <ul><li>Implement parameter builder system for query values<br> <li> Replace string concatenation with parameter placeholders<br> <li> Add <code>column_value_of</code> and <code>render_value</code> functions<br> <li> Return <code>query_with_params</code> type from <code>translate_query</code></ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/1645/files#diff-c33a65c30e4433358fbe3a1806b6c35291fd999f3abfdaad0ca38419e94764b9">+199/-168</a></td> </tr> </table></td></tr><tr><td><strong>Tests</strong></td><td><table> <tr> <td> <details> <summary><strong>run_live_srql.ml</strong><dd><code>Update live test for parameterized queries</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></summary> <hr> ocaml/srql/test/run_live_srql.ml <ul><li>Update to use new <code>translate</code> function with parameters<br> <li> Add parameter printing functionality<br> <li> Support parameterized queries in streaming mode<br> <li> Handle prepared statements for parameter binding</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/1645/files#diff-ea25a2c5be7493fef8b79f6c49ab84140d70b2458f61c001139f3eec92bfb5e9">+43/-17</a>&nbsp; </td> </tr> <tr> <td> <details> <summary><strong>test_bounded_unbounded.ml</strong><dd><code>Fix bounded/unbounded tests for new API</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></summary> <hr> ocaml/srql/test/test_bounded_unbounded.ml <ul><li>Update test calls to use new translation structure<br> <li> Extract SQL from translation object</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/1645/files#diff-3840c3919e65488587d26254fb2c1781bb64fc4da29d0afd8ae3b821acc7a9fe">+3/-2</a>&nbsp; &nbsp; &nbsp; </td> </tr> <tr> <td> <details> <summary><strong>test_query_engine.ml</strong><dd><code>Comprehensive parameter validation tests</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></summary> <hr> ocaml/srql/test/test_query_engine.ml <ul><li>Replace <code>sql_of</code> with <code>translation_of</code> function<br> <li> Add parameter validation in all test cases<br> <li> Verify parameter values match expected strings/numbers<br> <li> Update assertions for placeholder-based SQL</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/1645/files#diff-0c1dcb695d94269d0e3f899c01455e21cb50242b4665e3a0ef5f2b198c5cbe2f">+103/-39</a></td> </tr> <tr> <td> <details> <summary><strong>test_sql_sanitize.ml</strong><dd><code>Update sanitization tests for parameters</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></summary> <hr> ocaml/srql/test/test_sql_sanitize.ml <ul><li>Update sanitization tests for parameter-based approach<br> <li> Remove literal escaping validation logic<br> <li> Verify parameters capture injection payloads safely<br> <li> Update fuzz testing for parameter validation</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/1645/files#diff-8e9284704ffb8e9d0b5ad1d4641f03b26b291dc87a714647a5d8a0f129ca3453">+18/-41</a>&nbsp; </td> </tr> </table></td></tr></tr></tbody></table> </details> ___
qodo-code-review[bot] commented 2025-09-18 02:39:51 +00:00 (Migrated from github.com)
Author
Owner

Imported GitHub PR comment.

Original author: @qodo-code-review[bot]
Original URL: https://github.com/carverauto/serviceradar/pull/1645#issuecomment-3305200738
Original created: 2025-09-18T02:39:51Z

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵
🧪 PR contains tests
🔒 No security concerns identified
 Recommended focus areas for review

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.

let create_builder () = { counter = 0; params = [] }

let add_param builder value =
  builder.counter <- builder.counter + 1;
  let name = Printf.sprintf "p%d" builder.counter in
  builder.params <- (name, value) :: builder.params;
  Printf.sprintf "{{%s}}" name

let finalize_params builder = List.rev builder.params

let column_value_of = function
  | String s -> Column.String s
  | Int i -> Column.Int64 (Int64.of_int i)
  | Bool b -> Column.Int32 (if b then 1l else 0l)
  | Float f -> Column.Float64 f
  | Expr _ -> invalid_arg "column_value_of: Expr cannot be parameterized"

let render_value builder = function
  | Expr e ->
      ensure_safe_expression ~context:"expression value" e;
      e
  | v ->
      let column_value = column_value_of v in
      add_param builder column_value

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.

let column_value_of = function
  | String s -> Column.String s
  | Int i -> Column.Int64 (Int64.of_int i)
  | Bool b -> Column.Int32 (if b then 1l else 0l)
  | Float f -> Column.Float64 f
  | Expr _ -> invalid_arg "column_value_of: Expr cannot be parameterized"

let render_value builder = function
  | Expr e ->
      ensure_safe_expression ~context:"expression value" e;
      e
  | v ->
      let column_value = column_value_of v in
      add_param builder column_value

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.

let wrap_table_if_needed sql =
  (* Proton requires FROM table(<name>) for snapshot semantics in many cases.
     Wrap FROM target with table(...) if not already present. *)
  let lsql = String.lowercase_ascii sql in
  let contains s sub =
    let len_s = String.length s and len_sub = String.length sub in
    let rec loop i =
      if i + len_sub > len_s then false
      else if String.sub s i len_sub = sub then true
      else loop (i + 1)
    in
    loop 0
  in
  let has_from = contains lsql " from " in
  let has_table_wrapper = contains lsql " from table(" in
  if (not has_from) || has_table_wrapper then sql
  else
    try
      let lfrom = " from " in
      let idx_from =
        let rec find_from i =
          if i >= String.length lsql then raise Not_found
          else if String.sub lsql i (String.length lfrom) = lfrom then i
          else find_from (i + 1)
        in
        find_from 0
      in
      let start_tbl = idx_from + String.length lfrom in
      let rec skip_spaces j =
        if j < String.length lsql && lsql.[j] = ' ' then skip_spaces (j + 1) else j
      in
      let tbl_start = skip_spaces start_tbl in
      let keywords = [ " where "; " limit "; " group "; " order "; " settings "; " union " ] in
      let next_kw_pos =
        List.filter_map
          (fun kw ->
            let rec find i =
              if i >= String.length lsql then None
              else if String.sub lsql i (String.length kw) = kw then Some i
              else find (i + 1)
            in
            find tbl_start)
          keywords
        |> List.sort compare
        |> function
        | x :: _ -> x
        | [] -> String.length lsql
      in
      let tbl_end = next_kw_pos in
      let before = String.sub sql 0 tbl_start in
      let tbl = String.sub sql tbl_start (tbl_end - tbl_start) |> String.trim in
      let after = String.sub sql tbl_end (String.length sql - tbl_end) in
      before ^ "table(" ^ tbl ^ ")" ^ after
    with _ -> sql

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: <table> <tr><td>⏱️&nbsp;<strong>Estimated effort to review</strong>: 3 🔵🔵🔵⚪⚪</td></tr> <tr><td>🧪&nbsp;<strong>PR contains tests</strong></td></tr> <tr><td>🔒&nbsp;<strong>No security concerns identified</strong></td></tr> <tr><td>⚡&nbsp;<strong>Recommended focus areas for review</strong><br><br> <details><summary><a href='https://github.com/carverauto/serviceradar/pull/1645/files#diff-c33a65c30e4433358fbe3a1806b6c35291fd999f3abfdaad0ca38419e94764b9R17-R41'><strong>Placeholder Format</strong></a> 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. </summary> ```ocaml let create_builder () = { counter = 0; params = [] } let add_param builder value = builder.counter <- builder.counter + 1; let name = Printf.sprintf "p%d" builder.counter in builder.params <- (name, value) :: builder.params; Printf.sprintf "{{%s}}" name let finalize_params builder = List.rev builder.params let column_value_of = function | String s -> Column.String s | Int i -> Column.Int64 (Int64.of_int i) | Bool b -> Column.Int32 (if b then 1l else 0l) | Float f -> Column.Float64 f | Expr _ -> invalid_arg "column_value_of: Expr cannot be parameterized" let render_value builder = function | Expr e -> ensure_safe_expression ~context:"expression value" e; e | v -> let column_value = column_value_of v in add_param builder column_value ``` </details> <details><summary><a href='https://github.com/carverauto/serviceradar/pull/1645/files#diff-c33a65c30e4433358fbe3a1806b6c35291fd999f3abfdaad0ca38419e94764b9R27-R41'><strong>Type Coercion</strong></a> 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. </summary> ```ocaml let column_value_of = function | String s -> Column.String s | Int i -> Column.Int64 (Int64.of_int i) | Bool b -> Column.Int32 (if b then 1l else 0l) | Float f -> Column.Float64 f | Expr _ -> invalid_arg "column_value_of: Expr cannot be parameterized" let render_value builder = function | Expr e -> ensure_safe_expression ~context:"expression value" e; e | v -> let column_value = column_value_of v in add_param builder column_value ``` </details> <details><summary><a href='https://github.com/carverauto/serviceradar/pull/1645/files#diff-474590ab3468b98ee363f3cd78535c90bc83cf3b2e77ae9871db741e2d8dea06R135-R189'><strong>Wrapper Logic</strong></a> 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. </summary> ```ocaml let wrap_table_if_needed sql = (* Proton requires FROM table(<name>) for snapshot semantics in many cases. Wrap FROM target with table(...) if not already present. *) let lsql = String.lowercase_ascii sql in let contains s sub = let len_s = String.length s and len_sub = String.length sub in let rec loop i = if i + len_sub > len_s then false else if String.sub s i len_sub = sub then true else loop (i + 1) in loop 0 in let has_from = contains lsql " from " in let has_table_wrapper = contains lsql " from table(" in if (not has_from) || has_table_wrapper then sql else try let lfrom = " from " in let idx_from = let rec find_from i = if i >= String.length lsql then raise Not_found else if String.sub lsql i (String.length lfrom) = lfrom then i else find_from (i + 1) in find_from 0 in let start_tbl = idx_from + String.length lfrom in let rec skip_spaces j = if j < String.length lsql && lsql.[j] = ' ' then skip_spaces (j + 1) else j in let tbl_start = skip_spaces start_tbl in let keywords = [ " where "; " limit "; " group "; " order "; " settings "; " union " ] in let next_kw_pos = List.filter_map (fun kw -> let rec find i = if i >= String.length lsql then None else if String.sub lsql i (String.length kw) = kw then Some i else find (i + 1) in find tbl_start) keywords |> List.sort compare |> function | x :: _ -> x | [] -> String.length lsql in let tbl_end = next_kw_pos in let before = String.sub sql 0 tbl_start in let tbl = String.sub sql tbl_start (tbl_end - tbl_start) |> String.trim in let after = String.sub sql tbl_end (String.length sql - tbl_end) in before ^ "table(" ^ tbl ^ ")" ^ after with _ -> sql ``` </details> </td></tr> </table>
qodo-code-review[bot] commented 2025-09-18 02:40:53 +00:00 (Migrated from github.com)
Author
Owner

Imported GitHub PR comment.

Original author: @qodo-code-review[bot]
Original URL: https://github.com/carverauto/serviceradar/pull/1645#issuecomment-3305202170
Original created: 2025-09-18T02:40:53Z

PR Code Suggestions

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Security
Remove insecure direct value interpolation

Remove the special handling for Expr values in IN clauses to prevent potential
SQL injection, forcing all values to be parameterized.

ocaml/srql/lib/translator.ml [89-96]

-| In -> (
-    match value with
-    | Expr e ->
-        ensure_safe_expression ~context:"IN expression" e;
-        Printf.sprintf "%s IN %s" field e
-    | _ ->
-        let placeholder = render_value builder value in
-        Printf.sprintf "%s IN %s" field placeholder)
+| In ->
+    let placeholder = render_value builder value in
+    Printf.sprintf "%s IN %s" field placeholder
  • Apply / Chat
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly points out that directly interpolating an Expr for an IN clause undermines the PR's goal of preventing SQL injection and should be removed for better security.

Medium
Possible issue
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]

 | Eq -> (
     match value with
     | String s
       when let u = lc s in
            u = "today" || u = "yesterday" ->
         let date_fun = if lc s = "today" then "today()" else "yesterday()" in
-        if String.length field >= 8 && String.sub (lc field) 0 8 = "to_date(" then
-          Printf.sprintf "%s = %s" field date_fun
-        else
-          let placeholder = render_value builder value in
-          Printf.sprintf "%s = %s" field placeholder
+        Printf.sprintf "%s = %s" field date_fun
     | _ ->
         let placeholder = render_value builder value in
         Printf.sprintf "%s = %s" field placeholder)
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies a functional bug in the new logic where special date strings today and yesterday are incorrectly parameterized instead of being translated to SQL functions.

Medium
  • Update
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 ✨ <!-- fb85793 --> Explore these optional code suggestions: <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>Security</td> <td> <details><summary>Remove insecure direct value interpolation</summary> ___ **Remove the special handling for <code>Expr</code> values in <code>IN</code> clauses to prevent potential <br>SQL injection, forcing all values to be parameterized.** [ocaml/srql/lib/translator.ml [89-96]](https://github.com/carverauto/serviceradar/pull/1645/files#diff-c33a65c30e4433358fbe3a1806b6c35291fd999f3abfdaad0ca38419e94764b9R89-R96) ```diff -| In -> ( - match value with - | Expr e -> - ensure_safe_expression ~context:"IN expression" e; - Printf.sprintf "%s IN %s" field e - | _ -> - let placeholder = render_value builder value in - Printf.sprintf "%s IN %s" field placeholder) +| In -> + let placeholder = render_value builder value in + Printf.sprintf "%s IN %s" field placeholder ``` - [ ] **Apply / Chat** <!-- /improve --apply_suggestion=0 --> <details><summary>Suggestion importance[1-10]: 8</summary> __ Why: The suggestion correctly points out that directly interpolating an `Expr` for an `IN` clause undermines the PR's goal of preventing SQL injection and should be removed for better security. </details></details></td><td align=center>Medium </td></tr><tr><td rowspan=1>Possible issue</td> <td> <details><summary>Correctly handle special date strings</summary> ___ **Always translate "today" and "yesterday" to their respective SQL functions <br>(<code>today()</code>, <code>yesterday()</code>) instead of parameterizing them as strings.** [ocaml/srql/lib/translator.ml [57-70]](https://github.com/carverauto/serviceradar/pull/1645/files#diff-c33a65c30e4433358fbe3a1806b6c35291fd999f3abfdaad0ca38419e94764b9R57-R70) ```diff | Eq -> ( match value with | String s when let u = lc s in u = "today" || u = "yesterday" -> let date_fun = if lc s = "today" then "today()" else "yesterday()" in - if String.length field >= 8 && String.sub (lc field) 0 8 = "to_date(" then - Printf.sprintf "%s = %s" field date_fun - else - let placeholder = render_value builder value in - Printf.sprintf "%s = %s" field placeholder + Printf.sprintf "%s = %s" field date_fun | _ -> let placeholder = render_value builder value in Printf.sprintf "%s = %s" field placeholder) ``` - [ ] **Apply / Chat** <!-- /improve --apply_suggestion=1 --> <details><summary>Suggestion importance[1-10]: 7</summary> __ Why: The suggestion correctly identifies a functional bug in the new logic where special date strings `today` and `yesterday` are incorrectly parameterized instead of being translated to SQL functions. </details></details></td><td align=center>Medium </td></tr> <tr><td align="center" colspan="2"> - [ ] Update <!-- /improve_multi --more_suggestions=true --> </td><td></td></tr></tbody></table>
qodo-code-review[bot] commented 2025-09-18 03:08:03 +00:00 (Migrated from github.com)
Author
Owner

Imported GitHub PR comment.

Original author: @qodo-code-review[bot]
Original URL: https://github.com/carverauto/serviceradar/pull/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 require
packages that only support OCaml < 5.0.
- Dependency chain: deps-of-srql-translator → dream >=
1.0.0alpha5 → dream-httpaf >= 1.0.0alpha4 → h2 < 0.13.0 → ocaml < 5.0, which conflicts with
ocaml-base-compiler = 5.3.0.
- OPAM proposed alternative compilers (dkml-base-compiler,
ocaml-variants) are incompatible or require unmet availability conditions:
- ocaml-variants →
ocaml-beta requires enable-ocaml-beta-repository.
- ocaml-variants → system-msvc requires os =
"win32", not satisfied on the runner.
- As a result, OPAM reported a package conflict and exited
with code 20.

Relevant error logs:
1:  ##[group]Runner Image Provisioner
2:  Hosted Compute Agent
...

634:  shell: /usr/bin/bash -e {0}
635:  env:
636:  OPAMCOLOR: always
637:  OPAMCONFIRMLEVEL: unsafe-yes
638:  OPAMDOWNLOADJOBS: 4
639:  OPAMERRLOGLEN: 0
640:  OPAMEXTERNALSOLVER: builtin-0install
641:  OPAMPRECISETRACKING: 1
642:  OPAMRETRIES: 10
643:  OPAMROOT: /home/runner/.opam
644:  OPAMSOLVERTIMEOUT: 600
645:  OPAMYES: 1
646:  DUNE_CACHE_ROOT: /home/runner/.cache/dune
647:  CLICOLOR_FORCE: 1
648:  ##[endgroup]
649:  ^[[31m[ERROR]^[[0m Package conflict!
650:  ^[[31m  * ^[[0mNo agreement on the version of ^[[01mocaml-base-compiler^[[0m:
...

667:  ^[[31m  * ^[[0mIncompatible packages:
668:  - (invariant)^[[33m → ^[[0m^[[31;01mocaml-base-compiler = 5.3.0^[[0m
669:  - deps-of-srql-translator^[[33m → ^[[0mdream >= 1.0.0~alpha5^[[33m → ^[[0mdream-httpaf >= 1.0.0~alpha4^[[33m → ^[[0mh2 < 0.13.0^[[33m → ^[[0mocaml < 5.0^[[33m → ^[[0m^[[31;01mdkml-base-compiler^[[0m
670:  ^[[31m  * ^[[0mIncompatible packages:
671:  - (invariant)^[[33m → ^[[0m^[[31;01mocaml-base-compiler = 5.3.0^[[0m
672:  - deps-of-srql-translator^[[33m → ^[[0mdream >= 1.0.0~alpha5^[[33m → ^[[0mdream-httpaf >= 1.0.0~alpha4^[[33m → ^[[0mh2 < 0.13.0^[[33m → ^[[0mocaml < 5.0^[[33m → ^[[0m^[[31;01mocaml-variants^[[0m
673:  ^[[31m  * ^[[0mIncompatible packages:
674:  - deps-of-srql-translator^[[33m → ^[[0mdream >= 1.0.0~alpha5^[[33m → ^[[0mdream-httpaf >= 1.0.0~alpha4^[[33m → ^[[0m^[[31;01mh2-lwt-unix^[[0m
675:  - deps-of-srql-translator^[[33m → ^[[0mproton >= 1.0.15^[[33m → ^[[0mtls-lwt^[[33m → ^[[0m^[[31;01mtls (= 1.0.4 | = 2.0.1 | >= 2.0.2)^[[0m
676:  ^[[31m  * ^[[0mMissing dependency:
677:  - deps-of-srql-translator^[[33m → ^[[0mdream >= 1.0.0~alpha5^[[33m → ^[[0mdream-httpaf >= 1.0.0~alpha4^[[33m → ^[[0mh2 < 0.13.0^[[33m → ^[[0mocaml < 5.0^[[33m → ^[[0mocaml-variants^[[33m → ^[[0m^[[31;01mocaml-beta^[[0m
678:  unmet availability conditions: 'enable-ocaml-beta-repository'
679:  ^[[31m  * ^[[0mMissing dependency:
680:  - deps-of-srql-translator^[[33m → ^[[0mdream >= 1.0.0~alpha5^[[33m → ^[[0mdream-httpaf >= 1.0.0~alpha4^[[33m → ^[[0mh2 < 0.13.0^[[33m → ^[[0mocaml < 5.0^[[33m → ^[[0mocaml-variants^[[33m → ^[[0m^[[31;01msystem-msvc^[[0m
681:  unmet availability conditions: 'os = "win32"'
682:  ##[error]Process completed with exit code 20.
683:  Post job cleanup.

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: <table><tr><td> **Action:** build-test</td></tr> <tr><td> **Failed stage:** [Install dependencies (with tests)](https://github.com/carverauto/serviceradar/actions/runs/17816706220/job/50651258301) [❌] </td></tr> <tr><td> **Failure summary:** The action failed during OPAM dependency resolution due to incompatible OCaml compiler constraints:<br> <br>- The environment is pinned to <code>ocaml-base-compiler = 5.3.0</code> (invariant), but dependencies require <br>packages that only support OCaml < 5.0.<br> - Dependency chain: <code>deps-of-srql-translator → dream >= </code><br><code>1.0.0~alpha5 → dream-httpaf >= 1.0.0~alpha4 → h2 < 0.13.0 → ocaml < 5.0</code>, which conflicts with <br><code>ocaml-base-compiler = 5.3.0</code>.<br> - OPAM proposed alternative compilers (<code>dkml-base-compiler</code>, <br><code>ocaml-variants</code>) are incompatible or require unmet availability conditions:<br> - <code>ocaml-variants → </code><br><code>ocaml-beta</code> requires <code>enable-ocaml-beta-repository</code>.<br> - <code>ocaml-variants → system-msvc</code> requires <code>os = </code><br><code>"win32"</code>, not satisfied on the runner.<br> - As a result, OPAM reported a package conflict and exited <br>with code 20.<br> </td></tr> <tr><td> <details><summary>Relevant error logs:</summary> ```yaml 1: ##[group]Runner Image Provisioner 2: Hosted Compute Agent ... 634: shell: /usr/bin/bash -e {0} 635: env: 636: OPAMCOLOR: always 637: OPAMCONFIRMLEVEL: unsafe-yes 638: OPAMDOWNLOADJOBS: 4 639: OPAMERRLOGLEN: 0 640: OPAMEXTERNALSOLVER: builtin-0install 641: OPAMPRECISETRACKING: 1 642: OPAMRETRIES: 10 643: OPAMROOT: /home/runner/.opam 644: OPAMSOLVERTIMEOUT: 600 645: OPAMYES: 1 646: DUNE_CACHE_ROOT: /home/runner/.cache/dune 647: CLICOLOR_FORCE: 1 648: ##[endgroup] 649: ^[[31m[ERROR]^[[0m Package conflict! 650: ^[[31m * ^[[0mNo agreement on the version of ^[[01mocaml-base-compiler^[[0m: ... 667: ^[[31m * ^[[0mIncompatible packages: 668: - (invariant)^[[33m → ^[[0m^[[31;01mocaml-base-compiler = 5.3.0^[[0m 669: - deps-of-srql-translator^[[33m → ^[[0mdream >= 1.0.0~alpha5^[[33m → ^[[0mdream-httpaf >= 1.0.0~alpha4^[[33m → ^[[0mh2 < 0.13.0^[[33m → ^[[0mocaml < 5.0^[[33m → ^[[0m^[[31;01mdkml-base-compiler^[[0m 670: ^[[31m * ^[[0mIncompatible packages: 671: - (invariant)^[[33m → ^[[0m^[[31;01mocaml-base-compiler = 5.3.0^[[0m 672: - deps-of-srql-translator^[[33m → ^[[0mdream >= 1.0.0~alpha5^[[33m → ^[[0mdream-httpaf >= 1.0.0~alpha4^[[33m → ^[[0mh2 < 0.13.0^[[33m → ^[[0mocaml < 5.0^[[33m → ^[[0m^[[31;01mocaml-variants^[[0m 673: ^[[31m * ^[[0mIncompatible packages: 674: - deps-of-srql-translator^[[33m → ^[[0mdream >= 1.0.0~alpha5^[[33m → ^[[0mdream-httpaf >= 1.0.0~alpha4^[[33m → ^[[0m^[[31;01mh2-lwt-unix^[[0m 675: - deps-of-srql-translator^[[33m → ^[[0mproton >= 1.0.15^[[33m → ^[[0mtls-lwt^[[33m → ^[[0m^[[31;01mtls (= 1.0.4 | = 2.0.1 | >= 2.0.2)^[[0m 676: ^[[31m * ^[[0mMissing dependency: 677: - deps-of-srql-translator^[[33m → ^[[0mdream >= 1.0.0~alpha5^[[33m → ^[[0mdream-httpaf >= 1.0.0~alpha4^[[33m → ^[[0mh2 < 0.13.0^[[33m → ^[[0mocaml < 5.0^[[33m → ^[[0mocaml-variants^[[33m → ^[[0m^[[31;01mocaml-beta^[[0m 678: unmet availability conditions: 'enable-ocaml-beta-repository' 679: ^[[31m * ^[[0mMissing dependency: 680: - deps-of-srql-translator^[[33m → ^[[0mdream >= 1.0.0~alpha5^[[33m → ^[[0mdream-httpaf >= 1.0.0~alpha4^[[33m → ^[[0mh2 < 0.13.0^[[33m → ^[[0mocaml < 5.0^[[33m → ^[[0mocaml-variants^[[33m → ^[[0m^[[31;01msystem-msvc^[[0m 681: unmet availability conditions: 'os = "win32"' 682: ##[error]Process completed with exit code 20. 683: Post job cleanup. ``` </details></td></tr></table>
Sign in to join this conversation.
No reviewers
No milestone
No project
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!2226
No description provided.