Query Processor rule management unit tests (Phase 2.4)#5487
Query Processor rule management unit tests (Phase 2.4)#5487renecannao merged 3 commits intov3.0-5473from
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the unit testing framework by adding comprehensive tests for the Query Processor's rule management capabilities for both MySQL and PostgreSQL. It ensures the correct behavior of rule creation, storage, retrieval, and various rule attributes, laying a robust foundation for future development and maintenance of query processing logic. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe PR adds a new Query Processor unit test and updates test initialization to ensure Prometheus registry and global thread-handler globals (GloMTH, GloPTH) are allocated and initialized before constructing MySQL/PgSQL query processors. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Harness
participant Init as test_init_query_processor()
participant MTH as MySQL_Threads_Handler
participant PTH as PgSQL_Threads_Handler
participant Prom as Prometheus Registry
participant QP as Query Processor (MySQL/PgSQL)
Test->>Init: start
Init->>Prom: allocate if null (GloVars.prometheus_registry)
Init->>MTH: new if null (GloMTH)
Init->>MTH: get_variables_list() -> init VariablesPointers
Init->>PTH: new if null (GloPTH)
Init->>PTH: get_variables_list() -> init VariablesPointers
Init->>QP: construct MySQL_Query_Processor / PgSQL_Query_Processor
Test->>QP: run rule creation/insertion/sort/commit/get_current_query_rules/reset tests
QP-->>Test: return results/assertions
Test->>Init: cleanup query processors (handlers intentionally not deleted)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive suite of unit tests for the query processor's rule management capabilities, covering both MySQL and PgSQL implementations. The tests are well-structured and cover various aspects of rule creation, storage, and retrieval. I've identified a minor memory leak within one of the new test cases and a misleading comment that should be corrected for clarity. Overall, this is a valuable addition to the test suite.
| mysql_simple_rule(10, true); | ||
| mysql_simple_rule(20, true); |
There was a problem hiding this comment.
These two calls to mysql_simple_rule create query rules, but their return values are discarded. This results in a memory leak, as the memory allocated by mysql_simple_rule (via malloc and strdup) is never freed. Since these lines appear to have no effect on the test's outcome, they should be removed.
| // Don't free — rule will be inserted into QP | ||
| // (QP manages rule lifecycle after insert) |
There was a problem hiding this comment.
The comment here is misleading. It states that the memory for the rule should not be freed because it will be inserted into the query processor. However, this test function (test_mysql_rule_creation) does not insert the rule into the QP, and the following lines correctly free the allocated memory. The comment should be updated to reflect the actual behavior.
// This rule is not inserted into the QP, so we must free its memory manually.There was a problem hiding this comment.
Pull request overview
Adds a fast, infrastructure-free TAP unit test suite for Query Processor rule management (Phase 2.4), validating rule creation and basic rule table operations for both MySQL and PgSQL processors.
Changes:
- Introduces
query_processor_unit-t.cppwith 42 TAP assertions covering rule creation, insertion/retrieval, sorting, regex modifier parsing, reset, and basic PgSQL parity. - Extends unit-test init helper to provide required thread handlers / variable map initialization for Query Processor construction.
- Updates the unit-test Makefile to build and run the new test binary.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| test/tap/tests/unit/query_processor_unit-t.cpp | New TAP unit tests for MySQL/PgSQL query processor rule management APIs. |
| test/tap/tests/unit/Makefile | Adds query_processor_unit-t to the unit test build targets. |
| test/tap/test_helpers/test_init.cpp | Ensures required globals (threads handlers, Prometheus registry) exist before constructing query processors. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| mysql_simple_rule(10, true); | ||
| mysql_simple_rule(20, true); | ||
|
|
There was a problem hiding this comment.
In test_mysql_reset_all(), two rules are created and discarded (mysql_simple_rule(10/20, true)) before the inserted ones. Since new_query_rule() allocates memory (malloc/strdup), these unused rules leak and can trip ASAN. Remove these calls or store and free the returned rules instead of discarding them.
| mysql_simple_rule(10, true); | |
| mysql_simple_rule(20, true); |
| // Don't free — rule will be inserted into QP | ||
| // (QP manages rule lifecycle after insert) | ||
| free(rule->username); free(rule->schemaname); | ||
| free(rule->match_digest); free(rule->match_pattern); | ||
| free(rule->client_addr); free(rule->comment); | ||
| free(rule); | ||
| } |
There was a problem hiding this comment.
The comment above the manual frees says “Don't free — rule will be inserted into QP”, but this test never inserts the rule and does free it. This is misleading for future maintenance; update the comment to match the actual ownership/lifecycle in this test.
| @@ -126,4 +150,7 @@ void test_cleanup_query_processor() { | |||
| delete GloPgQPro; | |||
| GloPgQPro = nullptr; | |||
| } | |||
| // NOTE: We do NOT delete GloMTH/GloPTH here because other | |||
| // components may still reference them. They are cleaned up | |||
| // by test_cleanup_minimal() or at process exit. | |||
| } | |||
There was a problem hiding this comment.
test_init_query_processor() allocates GloMTH/GloPTH with new, but neither test_cleanup_query_processor() nor test_globals_cleanup()/test_cleanup_minimal() deletes them. This contradicts the NOTE and will leak in unit tests (especially under ASAN). Track whether the handlers were created here and delete/reset them in the corresponding cleanup, or extend test_globals_cleanup() to clean up any initialized Glo* singletons.
test/tap/test_helpers/test_init.cpp
Outdated
| if (vl) free(vl); | ||
| vl = GloPTH->get_variables_list(); | ||
| if (vl) free(vl); |
There was a problem hiding this comment.
test_init_query_processor() calls get_variables_list() and then only free(vl). get_variables_list() returns a NULL-terminated array whose elements are individually malloc/strdup’d (see MySQL_Threads_Handler::get_variables_list / PgSQL_Threads_Handler::get_variables_list), so freeing just the array leaks all strings. Iterate over vl[i] freeing each entry before freeing the array to avoid leaks under ASAN.
| if (vl) free(vl); | |
| vl = GloPTH->get_variables_list(); | |
| if (vl) free(vl); | |
| if (vl) { | |
| for (char **p = vl; *p != nullptr; ++p) { | |
| free(*p); | |
| } | |
| free(vl); | |
| } | |
| vl = GloPTH->get_variables_list(); | |
| if (vl) { | |
| for (char **p = vl; *p != nullptr; ++p) { | |
| free(*p); | |
| } | |
| free(vl); | |
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/tap/tests/unit/query_processor_unit-t.cpp (1)
430-446: Dead code creating unused rules.Lines 433-434 create rules that are immediately discarded without being used or freed. This appears to be unintentional—the actual test uses newly created rules on Lines 436-437. Consider removing the dead lines.
♻️ Proposed fix
static void test_mysql_reset_all() { GloMyQPro->reset_all(true); - mysql_simple_rule(10, true); - mysql_simple_rule(20, true); - GloMyQPro->insert((QP_rule_t *)mysql_simple_rule(10, true)); GloMyQPro->insert((QP_rule_t *)mysql_simple_rule(20, true)); GloMyQPro->commit();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/tap/tests/unit/query_processor_unit-t.cpp` around lines 430 - 446, In test_mysql_reset_all remove the dead/unnecessary calls that create unused rules — the standalone calls to mysql_simple_rule(10, true) and mysql_simple_rule(20, true) (lines that just call the constructor and discard the result) — so only the intended insertions via GloMyQPro->insert((QP_rule_t *)mysql_simple_rule(...)) remain; this eliminates creating unused objects and avoids potential leaks in the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/tap/test_helpers/test_init.cpp`:
- Around line 153-155: Update the misleading comment: change the note that
currently says GloMTH/GloPTH are cleaned up by test_cleanup_minimal() to
accurately state that they are not cleaned up by test_cleanup_minimal() or
test_globals_cleanup(), and that their cleanup relies solely on process exit;
edit the comment near the GloMTH/GloPTH mention in test_init.cpp to remove the
reference to test_cleanup_minimal() and instead reference that only process exit
performs their cleanup.
---
Nitpick comments:
In `@test/tap/tests/unit/query_processor_unit-t.cpp`:
- Around line 430-446: In test_mysql_reset_all remove the dead/unnecessary calls
that create unused rules — the standalone calls to mysql_simple_rule(10, true)
and mysql_simple_rule(20, true) (lines that just call the constructor and
discard the result) — so only the intended insertions via
GloMyQPro->insert((QP_rule_t *)mysql_simple_rule(...)) remain; this eliminates
creating unused objects and avoids potential leaks in the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0db4ce9f-0db5-41bf-bea0-c85ce1b36da7
📒 Files selected for processing (3)
test/tap/test_helpers/test_init.cpptest/tap/tests/unit/Makefiletest/tap/tests/unit/query_processor_unit-t.cpp
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: CI-builds / builds (debian12,-dbg)
- GitHub Check: CI-builds / builds (ubuntu22,-tap)
- GitHub Check: Agent
- GitHub Check: run / trigger
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2026-02-13T05:55:42.693Z
Learnt from: mevishalr
Repo: sysown/proxysql PR: 5364
File: lib/MySQL_Logger.cpp:1211-1232
Timestamp: 2026-02-13T05:55:42.693Z
Learning: In ProxySQL, the MySQL_Logger and PgSQL_Logger destructors run after all worker threads have been joined during shutdown. The sequence in src/main.cpp is: (1) join all worker threads, (2) call ProxySQL_Main_shutdown_all_modules() which deletes the loggers. Therefore, there is no concurrent thread access during logger destruction, and lock ordering in the destructors cannot cause deadlocks.
Applied to files:
test/tap/test_helpers/test_init.cpp
📚 Learning: 2026-01-20T09:34:19.124Z
Learnt from: yuji-hatakeyama
Repo: sysown/proxysql PR: 5307
File: test/tap/tests/reg_test_5306-show_warnings_with_comment-t.cpp:39-48
Timestamp: 2026-01-20T09:34:19.124Z
Learning: In ProxySQL's TAP test suite, resource leaks (e.g., not calling mysql_close() on early return paths) are commonly tolerated because test processes are short-lived and OS frees resources on exit. This pattern applies to all C++ test files under test/tap/tests. When reviewing, recognize this as a project-wide test convention and focus on test correctness and isolation rather than insisting on fixing such leaks in these test files.
Applied to files:
test/tap/tests/unit/query_processor_unit-t.cpp
🪛 Clang (14.0.6)
test/tap/tests/unit/query_processor_unit-t.cpp
[error] 23-23: 'tap.h' file not found
(clang-diagnostic-error)
🪛 Cppcheck (2.20.0)
test/tap/tests/unit/query_processor_unit-t.cpp
[error] 149-149: Common realloc mistake
(memleakOnRealloc)
[warning] 86-86: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 31-31: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 138-138: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 84-84: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
[warning] 85-85: If memory allocation fails, then there is a possible null pointer dereference
(nullPointerOutOfMemory)
🔇 Additional comments (15)
test/tap/test_helpers/test_init.cpp (2)
36-38: LGTM!The extern declaration for
GloPTHis correctly added here since it has no header declaration, unlikeGloMTHwhich is declared inproxysql_utils.h.
118-136: Initialization logic is sound.The prerequisite setup correctly ensures:
- Prometheus registry exists for metric registration
- Thread handlers are created before query processors reference them
get_variables_list()triggers lazy population ofVariablesPointers_boolandVariablesPointers_intmaps (as seen inlib/MySQL_Thread.cpp:2494-2555)One minor observation: the returned
char**fromget_variables_list()is freed correctly withfree(vl)since the individual string pointers reference static/member data, not heap-allocated copies.test/tap/tests/unit/Makefile (1)
234-234: LGTM!The new
query_processor_unit-ttarget follows the same build pattern assmoke_test-t, with consistent use of$(TEST_HELPERS_OBJ),$(LIBPROXYSQLAR_FULL),$(STATIC_LIBS),$(MYLIBS), and$(ALLOW_MULTI_DEF). The target is correctly added toUNIT_TESTSfor inclusion in the default build.Also applies to: 253-257
test/tap/tests/unit/query_processor_unit-t.cpp (12)
1-35: LGTM!The file header documentation clearly scopes what's tested and what's deferred to E2E tests. The includes and extern declarations are appropriate.
Note: The static analysis hint about
tap.hnot found is a false positive—the Makefile sets up-I$(PROXYSQL_PATH)/test/tap/test_helpersand-I$(TAP_IDIR)which include the TAP header path.
47-124: LGTM!The helper functions provide a clean API for creating test rules with sensible defaults. The parameter selection (rule_id, active, match_pattern, destination_hostgroup, apply, username, flagIN, flagOUT) covers the most commonly varied fields for testing different scenarios.
Note: The static analysis warnings about null pointer dereference after allocation failure are false positives for test code—if allocation fails in a unit test environment, crashing is acceptable behavior.
133-214: LGTM!Thorough field-by-field verification of
new_query_rule(). Memory management on Lines 210-213 is correct—since this rule is never inserted into the QP (which would take ownership), manually freeing the strdup'd string fields before the struct is the right approach.
223-245: LGTM!Tests the core insertion workflow correctly. The QP takes ownership of inserted rules, and the
SQLite3_resultis properly deleted after use.
250-287: LGTM!Effectively tests that
sort()orders rules by ascendingrule_id. The verification correctly inspectsfields[0](the rule_id column perget_current_query_rules()column definitions inlib/MySQL_Query_Processor.cpp:957).
296-327: LGTM!Good coverage of regex modifier parsing scenarios: null (no modifiers), single modifier (CASELESS), and combined modifiers (CASELESS,GLOBAL). Memory management is correct for each case.
336-369: LGTM!Tests the special action fields (
error_msg,OK_msg,replace_pattern) that trigger specific query processor behaviors. Memory cleanup is correctly handled.
378-397: LGTM!Correctly sets up rules for flagIN/flagOUT chaining. While actual chaining evaluation requires
process_query()(appropriately deferred to E2E tests per the file header), this validates the rules can be stored with the expected flag values.
406-421: LGTM!Tests that rules with username filters can be created and stored. Runtime filtering behavior is appropriately left to E2E tests that have session context.
455-466: LGTM!Validates that
get_stats_commands_counters()returns a properly structured result with columns defined.
475-501: LGTM!Good parity testing for
PgSQL_Query_Processor. The tests mirror the MySQL tests to ensure both implementations behave consistently for rule creation, insertion, retrieval, and reset operations.
507-532: LGTM!The test plan totals correctly (18+2+4+5+3+1+1+1+2+4+1 = 42 tests). Initialization and cleanup ordering is proper, and
exit_status()returns the TAP exit code.
- test_mysql_reset_all: remove 2 leaked mysql_simple_rule() calls whose return values were discarded (memory leak under ASAN) - test_mysql_rule_creation: fix misleading "Don't free" comment — rule is NOT inserted into QP and IS freed manually - test_init.cpp: free individual strings from get_variables_list() before freeing the array (was leaking all variable name strings) - test_init.cpp: fix misleading comment claiming GloMTH/GloPTH are cleaned up by test_cleanup_minimal() — they rely on process exit
Comprehensive unit tests for MySQL_Query_Processor and PgSQL_Query_ Processor covering 42 test cases across 10 test functions. Runs in <0.01s with no infrastructure dependencies. Test coverage: - Rule creation via new_query_rule() with all 35 fields verified - Rule field storage: username, schemaname, match_digest, match_pattern, destination_hostgroup, cache_ttl, timeout, retries, sticky_conn, multiplex, apply, log, client_addr, comment - Regex modifier parsing: CASELESS, GLOBAL, combined (CASELESS,GLOBAL) - Rule insertion, sorting by rule_id, and commit - Rule retrieval via get_current_query_rules() SQLite3 result - Sort verification: rules inserted in reverse order, sorted ascending - Special fields: error_msg, OK_msg, replace_pattern - flagIN/flagOUT chaining setup - Username filter rules - reset_all() clears all rules - get_stats_commands_counters() returns valid result - PgSQL rule creation, insertion, and reset Note: Full process_query() testing requires a MySQL_Session with populated connection data, which is beyond isolated unit tests. The existing E2E TAP tests cover those scenarios.
- tests/unit/Makefile: Register query_processor_unit-t in build system - test_init.cpp: Create MySQL_Threads_Handler and PgSQL_Threads_Handler in test_init_query_processor() since QP constructors read variables from GloMTH/GloPTH. Trigger lazy initialization of VariablesPointers maps via get_variables_list() before constructing QP instances. Add extern declaration for GloPTH (not declared in any header).
- test_mysql_reset_all: remove 2 leaked mysql_simple_rule() calls whose return values were discarded (memory leak under ASAN) - test_mysql_rule_creation: fix misleading "Don't free" comment — rule is NOT inserted into QP and IS freed manually - test_init.cpp: free individual strings from get_variables_list() before freeing the array (was leaking all variable name strings) - test_init.cpp: fix misleading comment claiming GloMTH/GloPTH are cleaned up by test_cleanup_minimal() — they rely on process exit
|
Summary
Implements Phase 2.4 (#5476) of the Unit Testing Framework: unit tests for
MySQL_Query_ProcessorandPgSQL_Query_Processorrule management.Test Coverage
Implementation Notes
GloMTH->get_variable_int("query_rules_fast_routing_algorithm"). The init helper now createsMySQL_Threads_HandlerandPgSQL_Threads_Handlerand triggers lazy initialization of theirVariablesPointersmaps.MySQL_Session*with populated connection data (username, schema, client address). Those scenarios are covered by existing E2E TAP tests. This phase tests the rule storage/retrieval layer thatprocess_query()operates on.Depends On
Test plan
Summary by CodeRabbit