carverauto/serviceradar

Fork 0

2672 bugsrql cant search inflows dst port #2836

Merged

mfreeman451 merged 2 commits from refs/pull/2836/head into staging

2026-02-03 02:05:35 +00:00

mfreeman451 commented

2026-02-03 02:04:08 +00:00

(Migrated from github.com)

Owner

Imported from GitHub pull request.

Original GitHub pull request: #2677
Original author: @mfreeman451
Original URL: https://github.com/carverauto/serviceradar/pull/2677
Original created: 2026-02-03T02:04:08Z
Original updated: 2026-02-03T02:06:09Z
Original head: carverauto/serviceradar:2672-bugsrql-cant-search-inflows-dst_port
Original base: staging
Original merged: 2026-02-03T02:05:35Z 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, Tests

Description

Enable wildcard pattern matching for port filters in SRQL flows queries
Support Like/NotLike operators for src_port and dst_port fields
Cast port columns to text for wildcard matching while preserving integer validation for equality operators
Add comprehensive tests for wildcard port filtering and validation behavior

Diagram Walkthrough

flowchart LR
  A["Port Filter Request"] --> B{"Filter Operator Type"}
  B -->|"Eq/NotEq"| C["Parse as Integer"]
  B -->|"Like/NotLike"| D["Cast to Text"]
  C --> E["Apply Equality Filter"]
  D --> F["Apply Wildcard Filter"]
  E --> G["Query Result"]
  F --> G

File Walkthrough

Relevant files

Bug fix

flows.rs

Add wildcard port filter support with text casting

rust/srql/src/query/flows.rs

Added imports for diesel::dsl::sql and diesel::sql_types::Text to
support text casting
Enhanced apply_filter function to handle Like/NotLike operators for
src_port and dst_port by casting to text
Created new collect_port_params helper function to handle parameter
binding for both integer and text port values
Added three new tests: wildcard port filter support, non-integer
rejection with equality, and text parameter binding verification

+172/-24

Documentation

proposal.md `OpenSpec proposal for wildcard port filters` openspec/changes/fix-srql-flow-port-wildcards/proposal.md Documents the issue where SRQL rejects wildcard port filters with integer validation error Outlines solution to accept `%` patterns for port fields with like-style operators Specifies that integer validation remains for equality and list operators Lists affected files and test additions	+16/-0
spec.md `OpenSpec requirements for port wildcard patterns` openspec/changes/fix-srql-flow-port-wildcards/specs/srql/spec.md Defines requirement for wildcard pattern matching on flow port fields Provides three test scenarios: contains matching, starts_with matching, and integer validation for equality Specifies that equality and list operators must continue requiring integer values	+19/-0
tasks.md `OpenSpec implementation and testing tasks` openspec/changes/fix-srql-flow-port-wildcards/tasks.md Lists implementation tasks for updating filter handling and parameter binding Includes testing tasks for running unit tests and verifying API behavior Marks implementation and unit testing as complete	+10/-0

Imported from GitHub pull request. Original GitHub pull request: #2677 Original author: @mfreeman451 Original URL: https://github.com/carverauto/serviceradar/pull/2677 Original created: 2026-02-03T02:04:08Z Original updated: 2026-02-03T02:06:09Z Original head: carverauto/serviceradar:2672-bugsrql-cant-search-inflows-dst_port Original base: staging Original merged: 2026-02-03T02:05:35Z 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, Tests ___ ### **Description** - Enable wildcard pattern matching for port filters in SRQL flows queries - Support Like/NotLike operators for src_port and dst_port fields - Cast port columns to text for wildcard matching while preserving integer validation for equality operators - Add comprehensive tests for wildcard port filtering and validation behavior ___ ### Diagram Walkthrough ```mermaid flowchart LR A["Port Filter Request"] --> B{"Filter Operator Type"} B -->|"Eq/NotEq"| C["Parse as Integer"] B -->|"Like/NotLike"| D["Cast to Text"] C --> E["Apply Equality Filter"] D --> F["Apply Wildcard Filter"] E --> G["Query Result"] F --> G ``` <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>flows.rs</strong><dd><code>Add wildcard port filter support with text casting</code>              </dd></summary> <hr> rust/srql/src/query/flows.rs <ul><li>Added imports for <code>diesel::dsl::sql</code> and <code>diesel::sql_types::Text</code> to <br>support text casting<br> <li> Enhanced <code>apply_filter</code> function to handle Like/NotLike operators for <br>src_port and dst_port by casting to text<br> <li> Created new <code>collect_port_params</code> helper function to handle parameter <br>binding for both integer and text port values<br> <li> Added three new tests: wildcard port filter support, non-integer <br>rejection with equality, and text parameter binding verification</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2677/files#diff-47734c9613794616c2c3b7c6a5765fc4d285e4ed12ea7b0bd1317a77a22aaa1c">+172/-24</a></td> </tr> </table></td></tr><tr><td><strong>Documentation</strong></td><td><table> <tr> <td> <details> <summary><strong>proposal.md</strong><dd><code>OpenSpec proposal for wildcard port filters</code>                            </dd></summary> <hr> openspec/changes/fix-srql-flow-port-wildcards/proposal.md <ul><li>Documents the issue where SRQL rejects wildcard port filters with <br>integer validation error<br> <li> Outlines solution to accept <code>%</code> patterns for port fields with like-style <br>operators<br> <li> Specifies that integer validation remains for equality and list <br>operators<br> <li> Lists affected files and test additions</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2677/files#diff-904a90d9fbcb6237c0b778f147e319c43696687bc7b705b66e95ce991597c7ad">+16/-0</a>    </td> </tr> <tr> <td> <details> <summary><strong>spec.md</strong><dd><code>OpenSpec requirements for port wildcard patterns</code>                  </dd></summary> <hr> openspec/changes/fix-srql-flow-port-wildcards/specs/srql/spec.md <ul><li>Defines requirement for wildcard pattern matching on flow port fields<br> <li> Provides three test scenarios: contains matching, starts_with <br>matching, and integer validation for equality<br> <li> Specifies that equality and list operators must continue requiring <br>integer values</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2677/files#diff-f0302dc469fee6aaac864c025afaec6221e88e283293221fcd0fb91197b59273">+19/-0</a>    </td> </tr> <tr> <td> <details> <summary><strong>tasks.md</strong><dd><code>OpenSpec implementation and testing tasks</code>                                </dd></summary> <hr> openspec/changes/fix-srql-flow-port-wildcards/tasks.md <ul><li>Lists implementation tasks for updating filter handling and parameter <br>binding<br> <li> Includes testing tasks for running unit tests and verifying API <br>behavior<br> <li> Marks implementation and unit testing as complete</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2677/files#diff-6b3d03aee3bdcfe593350d4f054e325874b27d01a231a4ef83b24769f6f399da">+10/-0</a>    </td> </tr> </table></td></tr></tbody></table> </details> ___

qodo-code-review[bot] commented

2026-02-03 02:04:41 +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/2677#issuecomment-3838440701
Original created: 2026-02-03T02:04:41Z

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance

🟢

No security concerns identified

No security vulnerabilities detected by AI analysis. Human verification advised for critical code.

Ticket Compliance

🟡

🎫 #2672

🟢	SRQL flows queries must allow searching `dst_port` using wildcard patterns (e.g., `dst_port:%443%`) without raising an "must be an integer" error.
🟢	Integer validation for `dst_port` must remain enforced for equality-style comparisons (e.g., `dst_port:443`), rejecting non-integers.
⚪	Verify end-to-end that the SRQL API accepts and correctly executes `in:flows time:last_24h` `dst_port:%443% sort:time:desc limit:50` (not just query-building tests).

Codebase Duplication Compliance

⚪

Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance

🟢

Generic: Comprehensive Audit Trails

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

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Robust Error Handling and Edge Case Management

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

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

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

Learn more about managing compliance generic rules or creating your own custom rules

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

Learn more about managing compliance generic rules or creating your own custom rules

⚪

Generic: Security-First Input Validation and Data Handling

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

Status:
Raw SQL expression: The new wildcard port filtering uses
diesel::dsl::sql::<Text>("...::text"), which is safe here because the
fragment is constant, but it requires human verification that no user-controlled SQL can
reach this path and that parameter binding is always preserved.

Referred Code

        FilterOp::Like | FilterOp::NotLike => {
            let value = filter.value.as_scalar()?.to_string();
            let text_column = sql::<Text>("src_endpoint_port::text");
            query = match filter.op {
                FilterOp::Like => query.filter(text_column.ilike(value)),
                FilterOp::NotLike => query.filter(text_column.not_ilike(value)),
                _ => query,
            };
        }
        _ => {
            return Err(ServiceError::InvalidRequest(
                "src_port filter only supports equality or wildcard matching".into(),
            ));
        }
    }
}
"dst_port" | "dst_endpoint_port" => {
    match filter.op {
        FilterOp::Eq | FilterOp::NotEq => {
            let value = filter.value.as_scalar()?.parse::<i32>().map_err(|_| {
                ServiceError::InvalidRequest("dst_port must be an integer".into())


 ... (clipped 18 lines)

Learn more about managing compliance generic rules or creating your own custom rules

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/2677#issuecomment-3838440701 Original created: 2026-02-03T02:04:41Z --- ## PR Compliance Guide 🔍  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>🟢</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>🎫 <a href=https://github.com/carverauto/serviceradar/issues/2672>#2672</a></summary> <table width='100%'><tbody> <tr><td rowspan=2>🟢</td> <td>SRQL flows queries must allow searching <code>dst_port</code> using wildcard patterns (e.g., <br><code>dst_port:%443%</code>) without raising an "must be an integer" error.<br></td></tr> <tr><td>Integer validation for <code>dst_port</code> must remain enforced for equality-style comparisons (e.g., <br><code>dst_port:443</code>), rejecting non-integers.<br></td></tr> <tr><td rowspan=1>⚪</td> <td>Verify end-to-end that the SRQL API accepts and correctly executes <code>in:flows time:last_24h </code><br><code>dst_port:%443% sort:time:desc limit:50</code> (not just query-building tests).<br></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: 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:** Passed<br> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><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> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </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> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </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> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </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> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td rowspan=1>⚪</td> <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:** <br><a href='https://github.com/carverauto/serviceradar/pull/2677/files#diff-47734c9613794616c2c3b7c6a5765fc4d285e4ed12ea7b0bd1317a77a22aaa1cR186-R224'><strong>Raw SQL expression</strong></a>: The new wildcard port filtering uses <br><code>diesel::dsl::sql::<Text>("...::text")</code>, which is safe here because the <br>fragment is constant, but it requires human verification that no user-controlled SQL can <br>reach this path and that parameter binding is always preserved.<br> <details open><summary>Referred Code</summary> ```rust FilterOp::Like | FilterOp::NotLike => { let value = filter.value.as_scalar()?.to_string(); let text_column = sql::<Text>("src_endpoint_port::text"); query = match filter.op { FilterOp::Like => query.filter(text_column.ilike(value)), FilterOp::NotLike => query.filter(text_column.not_ilike(value)), _ => query, }; } _ => { return Err(ServiceError::InvalidRequest( "src_port filter only supports equality or wildcard matching".into(), )); } } } "dst_port" | "dst_endpoint_port" => { match filter.op { FilterOp::Eq | FilterOp::NotEq => { let value = filter.value.as_scalar()?.parse::<i32>().map_err(|_| { ServiceError::InvalidRequest("dst_port must be an integer".into()) ... (clipped 18 lines) ``` </details> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td align="center" colspan="2">   </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>

qodo-code-review[bot] commented

2026-02-03 02:06:09 +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/2677#issuecomment-3838449622
Original created: 2026-02-03T02:06:09Z

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
High-level	The change may cause performance issues Casting integer port columns to text for wildcard searches will bypass database indexes, causing poor performance. To fix this, create function-based indexes on the text-casted columns. Examples: rust/srql/src/query/flows.rs [188] `let text_column = sql::<Text>("src_endpoint_port::text");` rust/srql/src/query/flows.rs [218] `let text_column = sql::<Text>("dst_endpoint_port::text");` Solution Walkthrough: Before: // rust/srql/src/query/flows.rs fn apply_filter<'a>(mut query: FlowsQuery<'a>, filter: &Filter) -> Result<FlowsQuery<'a>> { match filter.field.as_str() { // ... "src_port" \| "src_endpoint_port" => { match filter.op { FilterOp::Like \| FilterOp::NotLike => { let value = filter.value.as_scalar()?.to_string(); // This cast prevents the database from using an index on `src_endpoint_port` let text_column = sql::<Text>("src_endpoint_port::text"); query = query.filter(text_column.ilike(value)); } // ... } } // ... } Ok(query) } After: `-- In a database migration file (no code change in the PR) -- To support performant wildcard searches on port columns, -- function-based indexes should be created in the database. -- The application code remains the same. CREATE INDEX idx_flows_src_port_text_pattern ON ocsf_network_activity USING btree ((src_endpoint_port::text) text_pattern_ops); CREATE INDEX idx_flows_dst_port_text_pattern ON ocsf_network_activity USING btree ((dst_endpoint_port::text) text_pattern_ops);` Suggestion importance[1-10]: 9 __ Why: The suggestion correctly identifies a critical performance issue where casting port columns to text (`::text`) for wildcard searches will prevent index usage, leading to full table scans on a potentially large table.	High
Possible issue	Support list operators for ports Implement support for `In` and `NotIn` list operators for port filters to align with the PR's specification, which requires integer-only list operations. rust/srql/src/query/flows.rs [203-230] match filter.op { FilterOp::Eq \| FilterOp::NotEq => { let value = filter.value.as_scalar()?.parse::<i32>().map_err(\|_\| { ServiceError::InvalidRequest("dst_port must be an integer".into()) })?; query = apply_eq_filter!( query, filter, dst_endpoint_port, value, "dst_port filter only supports equality" )?; } FilterOp::Like \| FilterOp::NotLike => { let value = filter.value.as_scalar()?.to_string(); let text_column = sql::<Text>("dst_endpoint_port::text"); query = match filter.op { FilterOp::Like => query.filter(text_column.ilike(value)), FilterOp::NotLike => query.filter(text_column.not_ilike(value)), _ => query, }; } + FilterOp::In \| FilterOp::NotIn => { + // parse list of port integers + let items = filter.value.as_list()?; + let ints: Vec<i32> = items + .iter() + .map(\|v\| v.parse::<i32>().map_err(\|_\| { + ServiceError::InvalidRequest("dst_port list items must be integers".into()) + })) + .collect::<Result<_>>()?; + query = match filter.op { + FilterOp::In => query.filter(dst_endpoint_port.eq_any(ints)), + FilterOp::NotIn => query.filter(dst_endpoint_port.ne_all(ints)), + _ => query, + }; + } _ => { return Err(ServiceError::InvalidRequest( - "dst_port filter only supports equality or wildcard matching".into(), + "dst_port filter only supports equality, wildcard, or list matching".into(), )); } } Apply / Chat Suggestion importance[1-10]: 9 __ Why: This suggestion correctly identifies that the implementation is missing support for list operators (`In`, `NotIn`) on port fields, which is explicitly required by the specification document included in this PR.	High
General	Add NotLike wildcard port test Add a new unit test to verify that wildcard filters using the `NotLike` operator on port fields bind parameters correctly, improving test coverage. rust/srql/src/query/flows.rs [694-719] #[test] -fn wildcard_port_filter_binds_text_param() { - // ... +fn wildcard_port_notlike_filter_binds_text_param() { + let plan = QueryPlan { + entity: Entity::Flows, + filters: vec![Filter { + field: "dst_port".into(), + op: FilterOp::NotLike, + value: FilterValue::Scalar("%443%".to_string()), + }], + order: Vec::new(), + limit: 50, + offset: 0, + time_range: None, + stats: None, + downsample: None, + rollup_stats: None, + include_deleted: false, + }; + let (_, params) = to_sql_and_params(&plan).expect("should build SQL for wildcard port"); + let has_notlike = params.iter().any(\|param\| match param { + BindParam::Text(value) => value == "%443%", + _ => false, + }); + assert!(has_notlike, "expected NotLike wildcard port to bind text param"); } `[To ensure code accuracy, apply this suggestion manually]` Suggestion importance[1-10]: 7 __ Why: The suggestion correctly points out that while the implementation supports `NotLike`, there is no corresponding test case to ensure its correctness, which is a valuable addition for test coverage.	Medium
	Refactor duplicated code into a function Refactor the duplicated logic for `src_port` and `dst_port` filters into a single helper function to improve maintainability and reduce redundancy. rust/srql/src/query/flows.rs [172-231] "src_port" \| "src_endpoint_port" => { - match filter.op { - FilterOp::Eq \| FilterOp::NotEq => { - let value = filter.value.as_scalar()?.parse::<i32>().map_err(\|_\| { - ServiceError::InvalidRequest("src_port must be an integer".into()) - })?; - query = apply_eq_filter!( - query, - filter, - src_endpoint_port, - value, - "src_port filter only supports equality" - )?; - } - FilterOp::Like \| FilterOp::NotLike => { - let value = filter.value.as_scalar()?.to_string(); - let text_column = sql::<Text>("src_endpoint_port::text"); - query = match filter.op { - FilterOp::Like => query.filter(text_column.ilike(value)), - FilterOp::NotLike => query.filter(text_column.not_ilike(value)), - _ => query, - }; - } - _ => { - return Err(ServiceError::InvalidRequest( - "src_port filter only supports equality or wildcard matching".into(), - )); - } - } + query = apply_port_filter(query, filter, src_endpoint_port, "src_port")?; } "dst_port" \| "dst_endpoint_port" => { - match filter.op { - FilterOp::Eq \| FilterOp::NotEq => { - let value = filter.value.as_scalar()?.parse::<i32>().map_err(\|_\| { - ServiceError::InvalidRequest("dst_port must be an integer".into()) - })?; - query = apply_eq_filter!( - query, - filter, - dst_endpoint_port, - value, - "dst_port filter only supports equality" - )?; - } - FilterOp::Like \| FilterOp::NotLike => { - let value = filter.value.as_scalar()?.to_string(); - let text_column = sql::<Text>("dst_endpoint_port::text"); - query = match filter.op { - FilterOp::Like => query.filter(text_column.ilike(value)), - FilterOp::NotLike => query.filter(text_column.not_ilike(value)), - _ => query, - }; - } - _ => { - return Err(ServiceError::InvalidRequest( - "dst_port filter only supports equality or wildcard matching".into(), - )); - } - } + query = apply_port_filter(query, filter, dst_endpoint_port, "dst_port")?; } Apply / Chat Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies significant code duplication between `src_port` and `dst_port` filter handling and proposes a valid refactoring into a helper function, which would improve code maintainability.	Low
	Use Diesel cast for text Replace the raw SQL `::text` cast with Diesel's built-in `dsl::cast` function for improved type safety and code clarity when casting port columns. rust/srql/src/query/flows.rs [188] `-let text_column = sql::<Text>("src_endpoint_port::text"); +let text_column = diesel::dsl::cast(src_endpoint_port, Text);` Apply / Chat Suggestion importance[1-10]: 5 __ Why: The suggestion correctly proposes using Diesel's type-safe `cast` function instead of a raw SQL string, which improves code clarity and leverages the ORM's features more effectively.	Low
	Remove unreachable code from match statement Remove an unreachable code branch in the `match` statement for `filter.op` and simplify the logic to an `if-else` block. rust/srql/src/query/flows.rs [186-194] `FilterOp::Like \| FilterOp::NotLike => { let value = filter.value.as_scalar()?.to_string(); let text_column = sql::<Text>("src_endpoint_port::text"); - query = match filter.op { - FilterOp::Like => query.filter(text_column.ilike(value)), - FilterOp::NotLike => query.filter(text_column.not_ilike(value)), - _ => query, - }; + if filter.op == FilterOp::Like { + query = query.filter(text_column.ilike(value)); + } else { + query = query.filter(text_column.not_ilike(value)); + } }` Apply / Chat Suggestion importance[1-10]: 4 __ Why: The suggestion correctly identifies unreachable code within a `match` statement and proposes a simplification, which is a valid but minor code quality improvement.	Low
More

Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/2677#issuecomment-3838449622 Original created: 2026-02-03T02:06:09Z --- ## PR Code Suggestions ✨  Explore these optional code suggestions: <table><thead><tr><td><strong>Category</strong></td><td align=left><strong>Suggestion                                                                                                                                    </strong></td><td align=center><strong>Impact</strong></td></tr><tbody><tr><td rowspan=1>High-level</td> <td> <details><summary>The change may cause performance issues</summary> ___ **Casting integer port columns to text for wildcard searches will bypass database <br>indexes, causing poor performance. To fix this, create function-based indexes on <br>the text-casted columns.** ### Examples: <details> <summary> <a href="https://github.com/carverauto/serviceradar/pull/2677/files#diff-47734c9613794616c2c3b7c6a5765fc4d285e4ed12ea7b0bd1317a77a22aaa1cR188-R188">rust/srql/src/query/flows.rs [188]</a> </summary> ```rust let text_column = sql::<Text>("src_endpoint_port::text"); ``` </details> <details> <summary> <a href="https://github.com/carverauto/serviceradar/pull/2677/files#diff-47734c9613794616c2c3b7c6a5765fc4d285e4ed12ea7b0bd1317a77a22aaa1cR218-R218">rust/srql/src/query/flows.rs [218]</a> </summary> ```rust let text_column = sql::<Text>("dst_endpoint_port::text"); ``` </details> ### Solution Walkthrough: #### Before: ```rust // rust/srql/src/query/flows.rs fn apply_filter<'a>(mut query: FlowsQuery<'a>, filter: &Filter) -> Result<FlowsQuery<'a>> { match filter.field.as_str() { // ... "src_port" | "src_endpoint_port" => { match filter.op { FilterOp::Like | FilterOp::NotLike => { let value = filter.value.as_scalar()?.to_string(); // This cast prevents the database from using an index on `src_endpoint_port` let text_column = sql::<Text>("src_endpoint_port::text"); query = query.filter(text_column.ilike(value)); } // ... } } // ... } Ok(query) } ``` #### After: ```rust -- In a database migration file (no code change in the PR) -- To support performant wildcard searches on port columns, -- function-based indexes should be created in the database. -- The application code remains the same. CREATE INDEX idx_flows_src_port_text_pattern ON ocsf_network_activity USING btree ((src_endpoint_port::text) text_pattern_ops); CREATE INDEX idx_flows_dst_port_text_pattern ON ocsf_network_activity USING btree ((dst_endpoint_port::text) text_pattern_ops); ``` <details><summary>Suggestion importance[1-10]: 9</summary> __ Why: The suggestion correctly identifies a critical performance issue where casting port columns to text (`::text`) for wildcard searches will prevent index usage, leading to full table scans on a potentially large table. </details></details></td><td align=center>High </td></tr><tr><td rowspan=1>Possible issue</td> <td> <details><summary>Support list operators for ports</summary> ___ **Implement support for <code>In</code> and <code>NotIn</code> list operators for port filters to align with <br>the PR's specification, which requires integer-only list operations.** [rust/srql/src/query/flows.rs [203-230]](https://github.com/carverauto/serviceradar/pull/2677/files#diff-47734c9613794616c2c3b7c6a5765fc4d285e4ed12ea7b0bd1317a77a22aaa1cR203-R230) ```diff match filter.op { FilterOp::Eq | FilterOp::NotEq => { let value = filter.value.as_scalar()?.parse::<i32>().map_err(|_| { ServiceError::InvalidRequest("dst_port must be an integer".into()) })?; query = apply_eq_filter!( query, filter, dst_endpoint_port, value, "dst_port filter only supports equality" )?; } FilterOp::Like | FilterOp::NotLike => { let value = filter.value.as_scalar()?.to_string(); let text_column = sql::<Text>("dst_endpoint_port::text"); query = match filter.op { FilterOp::Like => query.filter(text_column.ilike(value)), FilterOp::NotLike => query.filter(text_column.not_ilike(value)), _ => query, }; } + FilterOp::In | FilterOp::NotIn => { + // parse list of port integers + let items = filter.value.as_list()?; + let ints: Vec<i32> = items + .iter() + .map(|v| v.parse::<i32>().map_err(|_| { + ServiceError::InvalidRequest("dst_port list items must be integers".into()) + })) + .collect::<Result<_>>()?; + query = match filter.op { + FilterOp::In => query.filter(dst_endpoint_port.eq_any(ints)), + FilterOp::NotIn => query.filter(dst_endpoint_port.ne_all(ints)), + _ => query, + }; + } _ => { return Err(ServiceError::InvalidRequest( - "dst_port filter only supports equality or wildcard matching".into(), + "dst_port filter only supports equality, wildcard, or list matching".into(), )); } } ``` - [ ] **Apply / Chat**  <details><summary>Suggestion importance[1-10]: 9</summary> __ Why: This suggestion correctly identifies that the implementation is missing support for list operators (`In`, `NotIn`) on port fields, which is explicitly required by the specification document included in this PR. </details></details></td><td align=center>High </td></tr><tr><td rowspan=4>General</td> <td> <details><summary>Add NotLike wildcard port test</summary> ___ **Add a new unit test to verify that wildcard filters using the <code>NotLike</code> operator <br>on port fields bind parameters correctly, improving test coverage.** [rust/srql/src/query/flows.rs [694-719]](https://github.com/carverauto/serviceradar/pull/2677/files#diff-47734c9613794616c2c3b7c6a5765fc4d285e4ed12ea7b0bd1317a77a22aaa1cR694-R719) ```diff #[test] -fn wildcard_port_filter_binds_text_param() { - // ... +fn wildcard_port_notlike_filter_binds_text_param() { + let plan = QueryPlan { + entity: Entity::Flows, + filters: vec![Filter { + field: "dst_port".into(), + op: FilterOp::NotLike, + value: FilterValue::Scalar("%443%".to_string()), + }], + order: Vec::new(), + limit: 50, + offset: 0, + time_range: None, + stats: None, + downsample: None, + rollup_stats: None, + include_deleted: false, + }; + let (_, params) = to_sql_and_params(&plan).expect("should build SQL for wildcard port"); + let has_notlike = params.iter().any(|param| match param { + BindParam::Text(value) => value == "%443%", + _ => false, + }); + assert!(has_notlike, "expected NotLike wildcard port to bind text param"); } ``` `[To ensure code accuracy, apply this suggestion manually]` <details><summary>Suggestion importance[1-10]: 7</summary> __ Why: The suggestion correctly points out that while the implementation supports `NotLike`, there is no corresponding test case to ensure its correctness, which is a valuable addition for test coverage. </details></details></td><td align=center>Medium </td></tr><tr><td> <details><summary>Refactor duplicated code into a function</summary> ___ **Refactor the duplicated logic for <code>src_port</code> and <code>dst_port</code> filters into a single <br>helper function to improve maintainability and reduce redundancy.** [rust/srql/src/query/flows.rs [172-231]](https://github.com/carverauto/serviceradar/pull/2677/files#diff-47734c9613794616c2c3b7c6a5765fc4d285e4ed12ea7b0bd1317a77a22aaa1cR172-R231) ```diff "src_port" | "src_endpoint_port" => { - match filter.op { - FilterOp::Eq | FilterOp::NotEq => { - let value = filter.value.as_scalar()?.parse::<i32>().map_err(|_| { - ServiceError::InvalidRequest("src_port must be an integer".into()) - })?; - query = apply_eq_filter!( - query, - filter, - src_endpoint_port, - value, - "src_port filter only supports equality" - )?; - } - FilterOp::Like | FilterOp::NotLike => { - let value = filter.value.as_scalar()?.to_string(); - let text_column = sql::<Text>("src_endpoint_port::text"); - query = match filter.op { - FilterOp::Like => query.filter(text_column.ilike(value)), - FilterOp::NotLike => query.filter(text_column.not_ilike(value)), - _ => query, - }; - } - _ => { - return Err(ServiceError::InvalidRequest( - "src_port filter only supports equality or wildcard matching".into(), - )); - } - } + query = apply_port_filter(query, filter, src_endpoint_port, "src_port")?; } "dst_port" | "dst_endpoint_port" => { - match filter.op { - FilterOp::Eq | FilterOp::NotEq => { - let value = filter.value.as_scalar()?.parse::<i32>().map_err(|_| { - ServiceError::InvalidRequest("dst_port must be an integer".into()) - })?; - query = apply_eq_filter!( - query, - filter, - dst_endpoint_port, - value, - "dst_port filter only supports equality" - )?; - } - FilterOp::Like | FilterOp::NotLike => { - let value = filter.value.as_scalar()?.to_string(); - let text_column = sql::<Text>("dst_endpoint_port::text"); - query = match filter.op { - FilterOp::Like => query.filter(text_column.ilike(value)), - FilterOp::NotLike => query.filter(text_column.not_ilike(value)), - _ => query, - }; - } - _ => { - return Err(ServiceError::InvalidRequest( - "dst_port filter only supports equality or wildcard matching".into(), - )); - } - } + query = apply_port_filter(query, filter, dst_endpoint_port, "dst_port")?; } ``` - [ ] **Apply / Chat**  <details><summary>Suggestion importance[1-10]: 6</summary> __ Why: The suggestion correctly identifies significant code duplication between `src_port` and `dst_port` filter handling and proposes a valid refactoring into a helper function, which would improve code maintainability. </details></details></td><td align=center>Low </td></tr><tr><td> <details><summary>Use Diesel cast for text</summary> ___ **Replace the raw SQL <code>::text</code> cast with Diesel's built-in <code>dsl::cast</code> function for <br>improved type safety and code clarity when casting port columns.** [rust/srql/src/query/flows.rs [188]](https://github.com/carverauto/serviceradar/pull/2677/files#diff-47734c9613794616c2c3b7c6a5765fc4d285e4ed12ea7b0bd1317a77a22aaa1cR188-R188) ```diff -let text_column = sql::<Text>("src_endpoint_port::text"); +let text_column = diesel::dsl::cast(src_endpoint_port, Text); ``` - [ ] **Apply / Chat**  <details><summary>Suggestion importance[1-10]: 5</summary> __ Why: The suggestion correctly proposes using Diesel's type-safe `cast` function instead of a raw SQL string, which improves code clarity and leverages the ORM's features more effectively. </details></details></td><td align=center>Low </td></tr><tr><td> <details><summary>Remove unreachable code from match statement</summary> ___ **Remove an unreachable code branch in the <code>match</code> statement for <code>filter.op</code> and <br>simplify the logic to an <code>if-else</code> block.** [rust/srql/src/query/flows.rs [186-194]](https://github.com/carverauto/serviceradar/pull/2677/files#diff-47734c9613794616c2c3b7c6a5765fc4d285e4ed12ea7b0bd1317a77a22aaa1cR186-R194) ```diff FilterOp::Like | FilterOp::NotLike => { let value = filter.value.as_scalar()?.to_string(); let text_column = sql::<Text>("src_endpoint_port::text"); - query = match filter.op { - FilterOp::Like => query.filter(text_column.ilike(value)), - FilterOp::NotLike => query.filter(text_column.not_ilike(value)), - _ => query, - }; + if filter.op == FilterOp::Like { + query = query.filter(text_column.ilike(value)); + } else { + query = query.filter(text_column.not_ilike(value)); + } } ``` - [ ] **Apply / Chat**  <details><summary>Suggestion importance[1-10]: 4</summary> __ Why: The suggestion correctly identifies unreachable code within a `match` statement and proposes a simplification, which is a valid but minor code quality improvement. </details></details></td><td align=center>Low </td></tr> <tr><td align="center" colspan="2"> - [ ] More  </td><td></td></tr></tbody></table>