feat: add custom SQL filtering support for packet stream#46
Conversation
Backend: Implement dynamic SQL execution in DataFrameActor and PushService based on user_sql config. Backend: Add verify_user_sql command to validate query syntax using Polars context. Frontend: Add SQL filter input in Settings page with pre-save validation logic. Refactor: Simplify PushService initialization and expose shared last_index in AppState.
There was a problem hiding this comment.
Pull request overview
This PR adds custom SQL filtering support for the packet stream, enabling users to dynamically filter captured network packets using SQL queries. The implementation spans both frontend and backend, with validation logic to ensure SQL syntax correctness before application.
Key Changes:
- Added
user_sqlconfiguration field with validation via newverify_user_sqlcommand - Refactored
PushServiceto support dynamic SQL execution based on user configuration - Introduced
shared_last_indexinAppStatefor tracking stream position across capture sessions
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
src/types/index.ts |
Added user_sql field to Configs interface and removed detailed inline comments |
src/services/apiService.ts |
Added verifyUserSql method and updated modifyConfigs parameter from patch to newConfigs |
src/pages/SettingsPage.tsx |
Added SQL filter input UI with error display and validation integration |
src/hooks/useConfigs.ts |
Integrated pre-save SQL validation and refactored to send full configs instead of patches |
src-tauri/src/tauri_bridge/state.rs |
Added user_sql and shared_last_index fields to AppState and Configs |
src-tauri/src/tauri_bridge/commands.rs |
Implemented verify_user_sql command and refactored modify_configs to accept full config objects with normalization |
src-tauri/src/services/push_service.rs |
Removed handle-based architecture, added SQL-based conditional query execution with shared index tracking |
src-tauri/src/lib.rs |
Registered new verify_user_sql command and initialized user_sql and shared_last_index in app state |
src-tauri/src/core/queries.rs |
Added new_packets_customized_no_payload function to build wrapped SQL queries from user input |
src-tauri/src/core/actor.rs |
Added get_packets_customized_no_payload method and made create_capture_df public for validation |
docs/custom_sql_examples.md |
Added documentation with SQL filtering guidelines and examples |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| ### 模式 A：简易过滤模式 (推荐) | ||
|
|
||
| 仅需编写 `WHERE` 子句之后的条件逻辑。系统会自动补全 `SELECT ... FROM packets WHERE (...)` 并通过括号保护您的逻辑优先级。 |
There was a problem hiding this comment.
Example 4 demonstrates a SELECT query that selects * from packets, which contradicts the guideline on line 13 that says to "Avoid using ORDER BY or LIMIT". More importantly, this example would fail because the backend wraps user SQL and re-selects only specific columns (index, timestamp, uuid, etc.) from the result. If the user's SELECT statement doesn't include all required columns, the outer query will fail. The example should either select all required columns explicitly or be a WHERE clause example instead.
| } | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
[nitpick] There's an extra blank line here that's inconsistent with the rest of the file. Remove this blank line to maintain consistent spacing.
| <p className="text-xs text-gray-500 dark:text-gray-400 space-y-1"> | ||
| <span className="block">• Leave empty to stream all packets.</span> | ||
| <span className="block">• SQL will be validated against an empty schema before saving.</span> | ||
| <span className="block">• Must select the <code className="font-mono">index</code> column for incremental updates.</span> |
There was a problem hiding this comment.
The documentation states "Must select the index column for incremental updates" (line 224), but the backend code in new_packets_customized_no_payload automatically selects a fixed set of columns including index. This means the user doesn't need to select index explicitly in their custom SQL. The documentation should clarify that the index column is automatically included in the result set, or specify that custom SELECT statements must include the index column for the wrapping query to work correctly.
| <span className="block">• Must select the <code className="font-mono">index</code> column for incremental updates.</span> | |
| <span className="block">• The <code className="font-mono">index</code> column is automatically included for incremental updates; you do not need to select it explicitly.</span> |
| let normalized_user_sql = user_sql.and_then(|s| { | ||
| let trimmed = s.trim(); | ||
| if trimmed.is_empty() { | ||
| None | ||
| } else { | ||
| Some(trimmed.to_string()) | ||
| } | ||
| }); |
There was a problem hiding this comment.
The normalization logic for empty strings to None is duplicated between verify_user_sql (lines 206-213) and modify_configs (lines 177-184). This duplication violates the DRY principle and could lead to inconsistent behavior if one is updated but not the other. Consider extracting this into a shared helper function.
| { | ||
| let mut configs_lock = self.configs.lock().await; | ||
| *configs_lock = Some(configs.clone()); | ||
| } |
There was a problem hiding this comment.
[nitpick] The scope block (lines 107-110) around configs_lock is unnecessary since the lock is immediately dropped at the end of the expression anyway. The code can be simplified to just *self.configs.lock().await = Some(configs.clone()); without the extra braces.
| { | |
| let mut configs_lock = self.configs.lock().await; | |
| *configs_lock = Some(configs.clone()); | |
| } | |
| *self.configs.lock().await = Some(configs.clone()); |
| const userSqlChanged = configs.user_sql !== originalConfigs.user_sql; | ||
| if (userSqlChanged) { | ||
| // Validate SQL before saving | ||
| await ApiService.verifyUserSql(configs.user_sql ?? null); |
There was a problem hiding this comment.
[nitpick] The frontend sends empty strings for user_sql when the textarea is empty, but the backend normalizes empty strings to None in modify_configs. However, the validation in verifyUserSql is called with configs.user_sql ?? null which converts undefined to null but passes empty strings as-is. If configs.user_sql is an empty string, it will be sent to the backend for validation where it gets normalized to None and passes. This works correctly but the logic flow is convoluted. Consider normalizing empty strings to null on the frontend before calling verifyUserSql for clarity.
| await ApiService.verifyUserSql(configs.user_sql ?? null); | |
| const normalizedUserSql = configs.user_sql === '' || configs.user_sql === undefined ? null : configs.user_sql; | |
| await ApiService.verifyUserSql(normalizedUserSql); |
| ) -> Result<()> { | ||
| // 创建 Worker 实例 | ||
| let mut worker = Self { | ||
| df_actor_handle: handle, | ||
| tauri_interface: interface, | ||
| handle: service_handle.clone(), | ||
| done, | ||
| app_handle, | ||
| user_sql, | ||
| last_index, | ||
| shared_last_index, | ||
| }; | ||
|
|
||
| tokio::spawn(async move { | ||
| worker.run().await; | ||
| }); | ||
|
|
||
| service_handle | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
The PushService::new() function spawns a task and returns Result<()>, which means callers can't detect if the spawned task fails or track its lifecycle. This is a departure from the previous design where a handle was returned. Consider returning a join handle or implementing a supervision mechanism to detect and report failures in the spawned task, or at minimum document this fire-and-forget behavior.
| .and_then(|lf| lf.collect()) | ||
| .map_err(|e| { | ||
| error!("SQL validation failed: {}", e); | ||
| format!("SQL validation failed: {}", e) |
There was a problem hiding this comment.
[nitpick] The SQL validation in verify_user_sql executes the query with last_index = 0 against an empty DataFrame. This validates syntax but doesn't catch runtime errors that might occur with actual data (e.g., type mismatches, invalid column references in complex expressions). Consider adding a note in the error message that validation is performed against an empty schema, so users understand that some errors may only appear at runtime.
| format!("SQL validation failed: {}", e) | |
| format!("SQL validation failed: {} (Note: validation is performed against an empty schema; some errors may only appear at runtime with actual data)", e) |
| info!("Validating SQL: {}", sql_text); | ||
|
|
||
| let mut ctx = SQLContext::new(); | ||
| let df = create_capture_df(); |
There was a problem hiding this comment.
[nitpick] In the validation logic, a new SQLContext is created and the entire DataFrame is cloned (df.clone().lazy()) on every validation. For large DataFrames, this could be expensive. Since validation uses an empty DataFrame (created via create_capture_df()), this is not a concern in practice, but the comment or code could clarify that validation intentionally uses an empty schema for efficiency.
| let df = create_capture_df(); | |
| let df = create_capture_df(); | |
| // Note: create_capture_df() returns an empty DataFrame for schema validation, | |
| // so registering it here is efficient and does not clone or materialize a large DataFrame. |
| pub fn new_packets_customized_no_payload(last_index: &u64, user_sql: &str) -> String { | ||
| let trimmed_sql = user_sql.trim(); | ||
|
|
||
| const TARGET_COLS: &str = "index, timestamp, uuid, src_ip, src_port, dst_ip, dst_port, pid, pname, type, length, is_binary"; | ||
|
|
||
| let is_full_select = trimmed_sql.to_lowercase().starts_with("select"); | ||
|
|
||
| if is_full_select { | ||
|
|
||
| let clean_sql = trimmed_sql.trim_end_matches(|c| c == ';' || c == ' '); | ||
|
|
||
| format!( | ||
| "SELECT {} FROM ({}) AS user_view WHERE index > {} ORDER BY index ASC", | ||
| TARGET_COLS, clean_sql, last_index | ||
| ) | ||
| } else { | ||
|
|
||
| let condition = if trimmed_sql.is_empty() { "1=1" } else { trimmed_sql }; | ||
|
|
||
| format!( | ||
| "SELECT {} FROM packets WHERE ({}) AND index > {} ORDER BY index ASC", | ||
| TARGET_COLS, condition, last_index | ||
| ) | ||
| } |
There was a problem hiding this comment.
The user-provided SQL is used directly in string formatting without proper sanitization, which could lead to SQL injection vulnerabilities. While Polars SQL context provides some protection, malicious SQL could still cause denial of service or unexpected behavior. Consider using parameterized queries or implementing stricter validation (e.g., allowlist of table names, column names, and operators) to prevent potential SQL injection attacks.
|
🔧 Debug Build Complete (PR 46, RunID 19636774289) 📦 Download Links: ⏰ Files will be retained for 7 days, please download and test promptly. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| static async modifyConfigs(newConfigs: Configs): Promise<void> { | ||
| try { | ||
| await invoke('modify_configs', { newConfigs: newConfigs }); |
There was a problem hiding this comment.
The parameter name change from patch to newConfigs represents a breaking API design change. The backend now expects the full configuration object instead of a partial patch. This inconsistency could be confusing - the frontend now sends all configs but the backend still has apply_patch logic (albeit unused). Consider removing the unused apply_patch method from the Rust Configs struct to maintain consistency.
| await invoke('modify_configs', { newConfigs: newConfigs }); | |
| await invoke('modify_configs', { configs: newConfigs }); |
|
|
||
| const TARGET_COLS: &str = "index, timestamp, uuid, src_ip, src_port, dst_ip, dst_port, pid, pname, type, length, is_binary"; | ||
|
|
||
| let is_full_select = trimmed_sql.to_lowercase().starts_with("select"); |
There was a problem hiding this comment.
The check to_lowercase().starts_with(\"select\") allocates a new string for case conversion on every call. For better performance, consider using trimmed_sql.trim_start().starts_with(|c| c == 's' || c == 'S') followed by a more precise check, or use a case-insensitive string comparison function if available.
| let is_full_select = trimmed_sql.to_lowercase().starts_with("select"); | |
| let is_full_select = trimmed_sql.get(..6).map_or(false, |s| s.eq_ignore_ascii_case("select")); |
|
|
||
| if is_full_select { | ||
|
|
||
| let clean_sql = trimmed_sql.trim_end_matches(|c| c == ';' || c == ' '); |
There was a problem hiding this comment.
The semicolon and space trimming logic only handles these two characters. Consider using .trim_end() after removing semicolons to handle all whitespace characters (tabs, newlines, etc.) more comprehensively: trimmed_sql.trim_end_matches(';').trim_end()
| let clean_sql = trimmed_sql.trim_end_matches(|c| c == ';' || c == ' '); | |
| let clean_sql = trimmed_sql.trim_end_matches(';').trim_end(); |
| Ok(()) | ||
| } | ||
|
|
||
| #[tauri::command] |
There was a problem hiding this comment.
The verify_user_sql function lacks documentation explaining its purpose, parameters, and validation approach. This is a public API that developers need to understand. Add a doc comment explaining that it validates user SQL against an empty Polars DataFrame schema, what constitutes valid SQL, and what errors might be returned.
| #[tauri::command] | |
| #[tauri::command] | |
| /// Validates a user-supplied SQL query string against an empty Polars DataFrame schema. | |
| /// | |
| /// # Parameters | |
| /// - `user_sql`: An optional SQL query string provided by the user. The string is trimmed of whitespace; | |
| /// if it is empty or `None`, validation passes automatically. | |
| /// | |
| /// # Validation Approach | |
| /// If a non-empty SQL string is provided, this function attempts to parse and validate it using | |
| /// Polars' `SQLContext` with an empty DataFrame schema. The SQL is considered valid if it can be | |
| /// successfully parsed and planned by Polars. | |
| /// | |
| /// # Returns | |
| /// - `Ok(())` if the SQL is valid, empty, or not provided. | |
| /// - `Err(String)` with an error message if the SQL is invalid or cannot be parsed by Polars. | |
| /// | |
| /// # Errors | |
| /// Returns an error if the SQL is syntactically incorrect, references unknown tables or columns, | |
| /// or if there is an internal error during validation. |
| @@ -0,0 +1,157 @@ | |||
| # 自定义数据包过滤 SQL 编写指南 | |||
|
|
|||
| 本系统支持使用标准 SQL 语法对捕获的网络数据包进行实时过滤。为了满足不同用户的需求，系统提供了\*\*简易过滤（仅条件）**和**高级查询（完整 SQL）\*\*两种模式。 | |||
There was a problem hiding this comment.
Corrected markdown formatting: '\\简易过滤（仅条件）和高级查询（完整 SQL）\\' - the escaped asterisks should not be escaped and there should be spaces around the operators.
| 本系统支持使用标准 SQL 语法对捕获的网络数据包进行实时过滤。为了满足不同用户的需求，系统提供了\*\*简易过滤（仅条件）**和**高级查询（完整 SQL）\*\*两种模式。 | |
| 本系统支持使用标准 SQL 语法对捕获的网络数据包进行实时过滤。为了满足不同用户的需求，系统提供了 **简易过滤（仅条件）** 和 **高级查询（完整 SQL）** 两种模式。 |
|
🔧 Debug Build Complete (PR 46, RunID 19657642742) 📦 Download Links: ⏰ Files will be retained for 7 days, please download and test promptly. |
|
🔧 Debug Build Complete (PR 46, RunID 19658487042) 📦 Download Links: ⏰ Files will be retained for 7 days, please download and test promptly. |
Backend: Implement dynamic SQL execution in DataFrameActor and PushService based on user_sql config.
Backend: Add verify_user_sql command to validate query syntax using Polars context.
Frontend: Add SQL filter input in Settings page with pre-save validation logic.
Refactor: Simplify PushService initialization and expose shared last_index in AppState.