Use proxy_sqlite3_* wrappers and canonical proxy symbol TU#22
Use proxy_sqlite3_* wrappers and canonical proxy symbol TU#22renecannao merged 8 commits intov3.1-MCP2_RAG1from
Conversation
…ove MAIN_PROXY_SQLITE3 from main.cpp
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis PR expands the SQLite3 proxy interface by adding 8 new function pointers for blob operations, column data access, and database lifecycle management. It introduces a new module to initialize and bind these proxy functions to native SQLite symbols, integrates proxy wrappers throughout the anomaly detector, and removes legacy test infrastructure. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 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. Comment |
Summary of ChangesHello @renecannao, 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 refactors how SQLite3 functions are accessed within the codebase by centralizing their proxy symbol definitions into a dedicated translation unit. This change addresses and prevents multiple-definition linker errors, ensuring a robust and consistent mechanism for interacting with the SQLite3 library. The modifications involve updating header declarations, adapting existing code to use the new proxied calls, and adjusting the build system accordingly. The overall impact is improved code maintainability and stability for SQLite3 integrations. Highlights
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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request successfully refactors the SQLite proxy mechanism by centralizing the definition of proxy_sqlite3_* function pointers into a dedicated translation unit (lib/proxy_sqlite3_symbols.cpp). This change correctly addresses potential multiple-definition issues by ensuring that src/main.cpp no longer defines these symbols directly, but instead relies on the extern declarations in include/sqlite3db.h. All usages of sqlite3_* functions in lib/Anomaly_Detector.cpp have been appropriately updated to use the new proxied function pointers. The lib/Makefile has been updated to include the new source file, and lib/sqlite3db.cpp correctly handles the initialization and assertion of the new function pointers. The removal of test/build_rag_test.sh and test/test_rag_schema.cpp indicates a cleanup or refactoring of test infrastructure, which is acceptable if no longer needed. Overall, the changes are well-implemented and directly resolve the stated problem without introducing new issues.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
lib/Anomaly_Detector.cpp (4)
748-779: Use proxy functions consistently for all SQLite operations on this statement.Lines 771-772 and 776 call
sqlite3_errmsg()andsqlite3_finalize()directly after using proxy functions for prepare, bind, and step operations. This mixing is unsafe in ProxySQL where SQLite symbols may be provided by a plugin. Replace with proxy counterparts.Proposed fix
- proxy_error("Anomaly: Failed to insert pattern: %s\n", sqlite3_errmsg(db)); - sqlite3_finalize(stmt); + proxy_error("Anomaly: Failed to insert pattern: %s\n", (*proxy_sqlite3_errmsg)(db)); + (*proxy_sqlite3_finalize)(stmt); return -1; } - sqlite3_finalize(stmt); + (*proxy_sqlite3_finalize)(stmt);
911-939: Keep stats queries entirely on the proxy API.
proxy_sqlite3_stepis now used, butsqlite3_prepare_v2,sqlite3_column_int,sqlite3_column_text, andsqlite3_finalizeare still direct calls. This mixes implementations in plugin mode. Use proxy functions consistently throughout.🐛 Proposed fix
- int rc = sqlite3_prepare_v2(db, count_query, -1, &stmt, NULL); + int rc = (*proxy_sqlite3_prepare_v2)(db, count_query, -1, &stmt, NULL); if (rc == SQLITE_OK) { rc = (*proxy_sqlite3_step)(stmt); if (rc == SQLITE_ROW) { - stats["threat_patterns_count"] = sqlite3_column_int(stmt, 0); + stats["threat_patterns_count"] = (*proxy_sqlite3_column_int)(stmt, 0); } - sqlite3_finalize(stmt); + (*proxy_sqlite3_finalize)(stmt); } @@ - rc = sqlite3_prepare_v2(db, type_query, -1, &stmt, NULL); + rc = (*proxy_sqlite3_prepare_v2)(db, type_query, -1, &stmt, NULL); if (rc == SQLITE_OK) { json by_type = json::object(); while ((*proxy_sqlite3_step)(stmt) == SQLITE_ROW) { - const char* type = reinterpret_cast<const char*>(sqlite3_column_text(stmt, 0)); - int count = sqlite3_column_int(stmt, 1); + const char* type = reinterpret_cast<const char*>((*proxy_sqlite3_column_text)(stmt, 0)); + int count = (*proxy_sqlite3_column_int)(stmt, 1); if (type) { by_type[type] = count; } } - sqlite3_finalize(stmt); + (*proxy_sqlite3_finalize)(stmt); stats["threat_patterns_by_type"] = by_type; }
449-500: Keep statement lifecycle on the same SQLite API surface.The statement is prepared, stepped, and accessed via proxy functions (
proxy_sqlite3_prepare_v2,proxy_sqlite3_step,proxy_sqlite3_column_*) but finalized via directsqlite3_finalizeat line 61. This creates an inconsistency that can cross library boundaries in plugin mode. Use the proxy finalize for consistency with the rest of the statement lifecycle.🐛 Proposed fix
- sqlite3_finalize(stmt); + (*proxy_sqlite3_finalize)(stmt);
810-836: Use proxy prepare/errmsg/finalize to match proxy step/column calls.The loop uses
proxy_sqlite3_stepand proxy column accessors, but the statement preparation, error handling, and finalization use directsqlite3_*APIs. This mixing breaks the abstraction when SQLite is provided by a plugin. Lines 452 and 755 in the same file correctly use(*proxy_sqlite3_prepare_v2)and(*proxy_sqlite3_errmsg)—follow that pattern here.🐛 Proposed fix
- int rc = sqlite3_prepare_v2(db, query, -1, &stmt, NULL); + int rc = (*proxy_sqlite3_prepare_v2)(db, query, -1, &stmt, NULL); if (rc != SQLITE_OK) { - proxy_error("Anomaly: Failed to query threat patterns: %s\n", sqlite3_errmsg(db)); + proxy_error("Anomaly: Failed to query threat patterns: %s\n", (*proxy_sqlite3_errmsg)(db)); return "[]"; } @@ - sqlite3_finalize(stmt); + (*proxy_sqlite3_finalize)(stmt);
🤖 Fix all issues with AI agents
In `@include/sqlite3db.h`:
- Around line 31-44: Add the missing definition for
proxy_sqlite3_enable_load_extension inside the MAIN_PROXY_SQLITE3 block so the
symbol declared as extern (int (*proxy_sqlite3_enable_load_extension)(sqlite3*,
int)) has a corresponding definition; follow the pattern used by the other proxy
symbols in that block (declare/initialize the function pointer there, e.g., to
NULL or wired to the real sqlite3_enable_load_extension during init) so test
compilation units that define MAIN_PROXY_SQLITE3 get the symbol.
| extern int (*proxy_sqlite3_bind_blob)(sqlite3_stmt*, int, const void*, int, void(*)(void*)); | ||
| extern const char *(*proxy_sqlite3_column_name)(sqlite3_stmt*, int N); | ||
| extern const unsigned char *(*proxy_sqlite3_column_text)(sqlite3_stmt*, int iCol); | ||
| extern int (*proxy_sqlite3_column_bytes)(sqlite3_stmt*, int iCol); | ||
| extern int (*proxy_sqlite3_column_type)(sqlite3_stmt*, int iCol); | ||
| extern int (*proxy_sqlite3_column_count)(sqlite3_stmt *pStmt); | ||
| extern int (*proxy_sqlite3_column_int)(sqlite3_stmt*, int iCol); | ||
| extern sqlite3_int64 (*proxy_sqlite3_column_int64)(sqlite3_stmt*, int iCol); | ||
| extern double (*proxy_sqlite3_column_double)(sqlite3_stmt*, int iCol); | ||
| extern sqlite3_int64 (*proxy_sqlite3_last_insert_rowid)(sqlite3*); | ||
| extern const char *(*proxy_sqlite3_errstr)(int); | ||
| extern sqlite3* (*proxy_sqlite3_db_handle)(sqlite3_stmt*); | ||
| extern int (*proxy_sqlite3_enable_load_extension)(sqlite3*, int); | ||
| extern int (*proxy_sqlite3_auto_extension)(void(*)(void)); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n '\bMAIN_PROXY_SQLITE3\b'Repository: ProxySQL/proxysql-vec
Length of output: 186
🏁 Script executed:
head -150 include/sqlite3db.h | cat -nRepository: ProxySQL/proxysql-vec
Length of output: 7784
🏁 Script executed:
rg -n 'define\s+MAIN_PROXY_SQLITE3' -t c -t cpp -t hRepository: ProxySQL/proxysql-vec
Length of output: 254
🏁 Script executed:
rg -l 'MAIN_PROXY_SQLITE3' | head -20Repository: ProxySQL/proxysql-vec
Length of output: 186
🏁 Script executed:
sed -n '82,139p' include/sqlite3db.h | grep -n 'enable_load_extension'Repository: ProxySQL/proxysql-vec
Length of output: 47
🏁 Script executed:
sed -n '82,139p' include/sqlite3db.hRepository: ProxySQL/proxysql-vec
Length of output: 2902
Add proxy_sqlite3_enable_load_extension definition to the MAIN_PROXY_SQLITE3 block.
The extern declaration exists at line 43, but the MAIN_PROXY_SQLITE3 definition block (lines 82–139) is missing the corresponding definition. Since MAIN_PROXY_SQLITE3 is actively defined in test files (test/tap/tests/setparser_test.cpp, test/tap/tests/sqlite3-t.cpp, test/tap/tests/setparser_test_common.h), this asymmetry will cause missing symbol definitions in those compilation units.
Fix
sqlite3* (*proxy_sqlite3_db_handle)(sqlite3_stmt*);
+int (*proxy_sqlite3_enable_load_extension)(sqlite3*, int);
int (*proxy_sqlite3_auto_extension)(void(*)(void));🤖 Prompt for AI Agents
In `@include/sqlite3db.h` around lines 31 - 44, Add the missing definition for
proxy_sqlite3_enable_load_extension inside the MAIN_PROXY_SQLITE3 block so the
symbol declared as extern (int (*proxy_sqlite3_enable_load_extension)(sqlite3*,
int)) has a corresponding definition; follow the pattern used by the other proxy
symbols in that block (declare/initialize the function pointer there, e.g., to
NULL or wired to the real sqlite3_enable_load_extension during init) so test
compilation units that define MAIN_PROXY_SQLITE3 get the symbol.
…ents (address code-review)
da030a8 to
6ce0538
Compare
This addresses an issue from PR #22 where LoadPlugin() was completely disabled. The function performs necessary initialization even when no plugin is loaded (initializes built-in sqlite3 function pointers). Changes: - Added `const bool allow_load_plugin = false` flag in LoadPlugin() - Modified `if (plugin_name)` to `if (plugin_name && allow_load_plugin == true)` - Re-enabled the LoadPlugin() call in LoadPlugins() The plugin loading code remains disabled (allow_load_plugin=false) while the function pointer initialization from built-in SQLite3 now works correctly. TODO: Revisit plugin loading safety mechanism to allow actual plugin loading.
This PR adds lib/proxy_sqlite3_symbols.cpp to provide a single translation unit that defines proxy_sqlite3_* function pointers, updates lib/Makefile to include it, and removes MAIN_PROXY_SQLITE3 from src/main.cpp to avoid multiple-definition issues. Build was validated with 'make debug'.