Handle missing database config entries #192
Conversation
|
Warning Rate limit exceeded@MrIbrahem has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 11 minutes and 46 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
✨ Finishing touches
🧪 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 |
Summary of ChangesHello @MrIbrahem, 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 of database connection setup by introducing explicit validation for the 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
|
![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 effectively adds necessary validation for the database configuration file, with corresponding unit tests to ensure the error handling works as expected. The changes improve the robustness of the database connection logic. My review includes a few suggestions to further enhance the code, mainly around error handling specificity in mdwiki_sql.php and improvements to the new test cases for clarity, robustness, and coverage.
| $ts_mycnf = @parse_ini_file($config_path); | ||
| if ($ts_mycnf === false) { | ||
| throw new RuntimeException(sprintf('Database configuration file "%s" could not be read or parsed.', $config_path)); | ||
| } |
There was a problem hiding this comment.
Using the error suppression operator (@) can hide issues and makes debugging harder. It's better to explicitly check for the file's readability first. This allows for more specific error messages about whether the file is unreadable versus being unparsable.
if (!is_readable($config_path)) {
throw new RuntimeException(sprintf('Database configuration file "%s" could not be read.', $config_path));
}
$ts_mycnf = parse_ini_file($config_path);
if ($ts_mycnf === false) {
throw new RuntimeException(sprintf('Database configuration file "%s" could not be parsed due to syntax errors.', $config_path));
}| final class DatabaseConfigTest extends TestCase | ||
| { |
There was a problem hiding this comment.
Great job on adding tests for the new error handling! To make the test suite even more comprehensive, consider adding a few more test cases:
- A test for when the
userkey is missing from the config file. - A test for when a required key (like
userorpassword) exists but its value is an empty string.
Here is an example for a missing user, which also incorporates the suggestion to move cleanup to tearDown():
public function testSetDbThrowsExceptionWhenUserMissing(): void
{
$this->tempHome = sys_get_temp_dir() . '/dbconfig_test_incomplete_' . uniqid();
$this->createTempHome($this->tempHome, ['password' => 'secret']); // No 'user' key
$this->expectException(RuntimeException::class);
$this->expectExceptionMessageMatches('/missing required "user" entry/');
new Database('tools');
}| try { | ||
| new Database('localhost'); | ||
| } finally { | ||
| $this->cleanupTempHome($tempHome); | ||
| } |
There was a problem hiding this comment.
The try...finally block for cleanup can be simplified by moving the cleanup logic into the tearDown() method. PHPUnit automatically calls tearDown() after each test, ensuring cleanup happens even if assertions fail. This makes the test methods cleaner and more focused on the test logic itself. You can add a property to the class to hold the temporary path to be cleaned up.
Example:
class DatabaseConfigTest extends TestCase
{
// ...
private ?string $tempHome = null;
protected function tearDown(): void
{
// ... restore HOME env
if ($this->tempHome !== null) {
$this->cleanupTempHome($this->tempHome);
$this->tempHome = null;
}
}
public function testSetDbThrowsExceptionWhenConfigMissing(): void
{
$this->tempHome = sys_get_temp_dir() . '/dbconfig_test_missing_' . uniqid();
$this->createTempHome($this->tempHome);
$this->expectException(RuntimeException::class);
$this->expectExceptionMessage('could not be read or parsed');
new Database('localhost');
}
// ...
}| $this->createTempHome($tempHome, ['user' => 'exampleuser']); | ||
|
|
||
| $this->expectException(RuntimeException::class); | ||
| $this->expectExceptionMessage('password'); |
There was a problem hiding this comment.
The assertion for the exception message is very broad. A more specific assertion makes the test more robust and less likely to pass for the wrong reasons. You can use expectExceptionMessageMatches for a regex-based match to check for the key parts of the message.
$this->expectExceptionMessageMatches('/missing required "password" entry/');| { | ||
| $configDir = $path . '/confs'; | ||
| if (!is_dir($configDir)) { | ||
| mkdir($configDir, 0777, true); |
There was a problem hiding this comment.
The file permission 0777 is overly permissive. Even for temporary directories, it's a good practice to use the most restrictive permissions possible to follow the principle of least privilege. 0700 would be more appropriate here, as it grants access only to the owner.
mkdir($configDir, 0700, true);| foreach ($items as $item) { | ||
| if ($item->isDir()) { | ||
| @rmdir($item->getPathname()); | ||
| } else { | ||
| @unlink($item->getPathname()); | ||
| } | ||
| } | ||
|
|
||
| @rmdir($path); |
There was a problem hiding this comment.
Using the error suppression operator (@) should be avoided as it can hide underlying problems. If cleanup fails, it might indicate an issue (like a file lock) that could affect other tests. It's better to let exceptions be thrown during cleanup. If a test fails due to a cleanup issue, it's a signal that something is wrong.
foreach ($items as $item) {
if ($item->isDir()) {
rmdir($item->getPathname());
} else {
unlink($item->getPathname());
}
}
rmdir($path);
Summary
Database::set_dband throw descriptive exceptions when it is missing or incompleteTesting
./vendor/bin/phpunit tests --testdox --colors=always -c phpunit.xml(fails: binary not present because dev dependencies are unavailable in the environment)composer install(fails: curl error 56 while downloading https://repo.packagist.org/packages.json, CONNECT tunnel failed with response 403)https://chatgpt.com/codex/tasks/task_e_69086eefb7908322b36466a05df42e5d