add validation for LOAD ... FROM CONFIG #5036
Conversation
- Add pre-validation phase to check for duplicate primary keys within config file - Validate mandatory fields (hostname, hostgroup_id) before any database operations - Return error code (-1) on validation failures instead of continuing - Update Admin_Handler to properly handle validation errors and send error responses - Preserve existing INSERT OR REPLACE behavior for legitimate updates - Ensure atomic operations - either all entries load successfully or none do Primary key validation prevents: hostgroup_id + hostname + port duplicates 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Add test_mysql_servers_config_validation-t.cpp for MySQL servers validation * Tests duplicate primary key detection (hostgroup_id + hostname + port) * Tests mandatory field validation (hostname, hostgroup_id) * Tests proper error responses for validation failures * Tests atomic operation behavior (all-or-nothing loading) - Add test_config_validation_comprehensive-t.cpp for all config sections * Tests MySQL users validation (username + backend duplicates) * Tests ProxySQL servers validation (hostname + port duplicates) * Tests missing mandatory field detection across all config types * Tests successful loading of valid comprehensive configurations These tests validate the cherry-picked changes that add pre-validation to prevent primary key violations and ensure data integrity when loading configurations from config files. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @takaidohigasi, 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 improves the reliability and integrity of ProxySQL's configuration loading process. It introduces robust pre-validation checks to prevent invalid configurations from being loaded, enhances error reporting to the client, and guarantees that configuration changes are applied atomically. The changes are backed by comprehensive new test cases to ensure correctness.
Highlights
- Enhanced Configuration Validation: Implemented pre-validation checks for various configuration sections (MySQL/PgSQL users, MySQL/PgSQL/ProxySQL servers) to prevent primary key violations and ensure mandatory fields are present before loading data from the config file. This includes checks for duplicate
(username, backend)for users,(hostgroup_id, hostname, port)for MySQL/PgSQL servers, and(hostname, port)for ProxySQL servers. - Improved Error Handling: Modified the
LOAD ... FROM CONFIGcommands inAdmin_Handler.cppto check the return value of config loading functions. If a negative value is returned (indicating a validation failure), an explicit error message 'Configuration validation failed - check error log for details' is now sent to the client, providing clearer feedback. - Atomic Configuration Loading: Ensured that configuration loading operations are atomic. If any validation error (e.g., duplicate entry or missing mandatory field) is detected during the pre-validation phase, the entire load operation is aborted, preventing partial or corrupted configurations from being applied.
- Comprehensive Test Coverage: Added new, dedicated test files (
test_config_validation_comprehensive-t.cppandtest_mysql_servers_config_validation-t.cpp) to thoroughly cover the new validation logic. These tests include scenarios for duplicate entries, missing mandatory fields, and verification of atomic load behavior across different configuration types.
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and 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 to provide feedback.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
|
Can one of the admins verify this patch? |
There was a problem hiding this comment.
Code Review
This pull request introduces important validation logic when loading configurations, preventing primary key violations and ensuring mandatory fields are present. The changes are well-implemented across multiple configuration sections (MySQL/PgSQL users and servers). The addition of new test files to cover these validation scenarios is also a great improvement.
My review focuses on a few areas for improvement:
- Code Duplication: There's significant code duplication in the validation logic across different
Read_*_from_configfilefunctions. - Correctness: There are some minor correctness issues related to C-style casts and SQLite PRAGMA handling in error paths.
- Testing: The new test files have incorrect TAP plans (the number of assertions doesn't match the plan), and one of the test files doesn't cover all the cases mentioned in its header.
Overall, this is a valuable change that improves the robustness of configuration loading. Addressing the feedback will further enhance code quality and maintainability.
|
test seems failed. I once change status to draft and change to ready when test passed |
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Signed-off-by: takaidohigasi <[email protected]>
This commit eliminates code duplication by introducing a generic template function `validate_config_entries` that centralizes the validation pattern used across all config reading functions. Key improvements: - Reduced ~150 lines of duplicated validation code - Centralized validation logic in a single template function - Type-safe primary key validation using tuples - Consistent error handling across all config sections - Maintainable extractor functions for each config type Refactored functions: - Read_MySQL_Users_from_configfile() - Read_MySQL_Servers_from_configfile() - Read_ProxySQL_Servers_from_configfile() - Read_PgSQL_Servers_from_configfile() - Read_PgSQL_Users_from_configfile() The template approach provides: - Compile-time type safety for primary key tuples - Flexible extractor functions for different validation logic - Consistent PRAGMA foreign key handling - Unified error message formatting This refactoring maintains full backward compatibility while significantly improving code maintainability and reducing the risk of inconsistencies in validation logic across different config sections. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
This commit adds the missing PostgreSQL validation tests to make the test_config_validation_comprehensive-t.cpp file truly comprehensive as promised in its header comment. New test functions added: - test_pgsql_servers_validation() - Tests PostgreSQL servers validation * Duplicate server detection (hostgroup_id + hostname + port) * Missing mandatory hostgroup_id validation - test_pgsql_users_validation() - Tests PostgreSQL users validation * Duplicate user detection (username + backend) * Missing mandatory username validation Enhanced comprehensive valid config test: - Added pgsql_servers section with 3 valid server entries - Added pgsql_users section with 3 valid user entries - Updated test assertions to verify all PostgreSQL components load correctly Test coverage now includes: ✅ MySQL Servers validation (hostgroup_id + hostname + port) ✅ MySQL Users validation (username + backend) ✅ ProxySQL Servers validation (hostname + port) ✅ PostgreSQL Servers validation (hostgroup_id + hostname + port) ✅ PostgreSQL Users validation (username + backend) ✅ Comprehensive valid configuration loading for all components Updated test plan from 13 to 23 assertions to account for the additional PostgreSQL validation tests, ensuring the header comment accurately reflects the complete test scope. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
…idohigasi/proxysql into add-config-validation-tests
…tion test Updated plan() from 10 to 9 to correctly reflect the actual number of ok() assertions in the test suite. The MYSQL_QUERY_T macros are error-handling wrappers and don't count as test assertions. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
|
I added some commits to improve for the gemini review comments. |
Changed config file paths from /tmp/ to current directory to avoid security issues with publicly writable directories. This follows the pattern used in mysql_hostgroup_attributes_config_file-t.cpp. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
|
|
I think we can reduce duplication but for readability, it is better for current state. Please tell me if you think I need to reduce duplication. |
takaidohigasi
changed the title
WHAT
add validation for config load.
WHY
#5035