Update table creation for PostgreSQL compatibility#32
Merged
Conversation
Reviewer's GuideAdds PostgreSQL-aware table existence checks and PostgreSQL-compatible DDL for the magiclogin_tokens and github_issues tables, while preserving existing MySQL-specific creation logic behind a server-type conditional. ER diagram for magiclogin_tokens and github_issues tableserDiagram
magiclogin_tokens {
int id PK
int user_id
varchar token
datetime expires
timestamp created
varchar ip_address
text user_agent
}
github_issues {
int id PK
datetime execution
smallint openi
smallint closedi
smallint openp
smallint closedp
}
%% No explicit foreign key relationships defined between these tables
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The PostgreSQL branch builds a single SQL string containing both
CREATE TABLEandCREATE INDEX; if the database layer doesn’t support multiple statements per query, this should be split into two separatesetQuery/executecalls to avoid runtime failures. - The PostgreSQL-specific SQL is now embedded as a raw string while the rest of the code uses the query builder and
quoteName; consider using the query builder (or at least consistent quoting utilities) for the new DDL to keep identifier quoting and prefix handling uniform across database types.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The PostgreSQL branch builds a single SQL string containing both `CREATE TABLE` and `CREATE INDEX`; if the database layer doesn’t support multiple statements per query, this should be split into two separate `setQuery`/`execute` calls to avoid runtime failures.
- The PostgreSQL-specific SQL is now embedded as a raw string while the rest of the code uses the query builder and `quoteName`; consider using the query builder (or at least consistent quoting utilities) for the new DDL to keep identifier quoting and prefix handling uniform across database types.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Updated comments and corrected table name in magiclogin script.
Owner
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The logic for checking table existence and creating tables/indexes is duplicated between
magicloginandgithubissues; consider extracting a shared helper or method to centralize thegetServerType()branching and reduce repetition. - Instead of hardcoding
"#__..."in the SQL strings, consider using$db->quoteName()and Joomla’s table prefix replacement to avoid potential quoting and prefix substitution issues across database drivers. - You are calling
$db->setQuery($query); $db->execute();repeatedly for each index creation; you could simplify by combining related DDL statements where supported or by wrapping this pattern in a small utility to make the code more compact and maintainable.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The logic for checking table existence and creating tables/indexes is duplicated between `magiclogin` and `githubissues`; consider extracting a shared helper or method to centralize the `getServerType()` branching and reduce repetition.
- Instead of hardcoding `"#__..."` in the SQL strings, consider using `$db->quoteName()` and Joomla’s table prefix replacement to avoid potential quoting and prefix substitution issues across database drivers.
- You are calling `$db->setQuery($query); $db->execute();` repeatedly for each index creation; you could simplify by combining related DDL statements where supported or by wrapping this pattern in a small utility to make the code more compact and maintainable.
## Individual Comments
### Comment 1
<location path="src/plugins/system/magiclogin/script.php" line_range="215-224" />
<code_context>
-
- $db->setQuery($query);
- $db->execute();
+ if ($db->getServerType() === 'postgresql') {
+ $query = 'CREATE TABLE IF NOT EXISTS "#__github_issues" (
+ "id" SERIAL PRIMARY KEY,
+ "execution" TIMESTAMP DEFAULT NULL,
+ "openi" SMALLINT NOT NULL DEFAULT 0,
+ "closedi" SMALLINT NOT NULL DEFAULT 0,
+ "openp" SMALLINT NOT NULL DEFAULT 0,
+ "closedp" SMALLINT NOT NULL DEFAULT 0
+ );';
+ $db->setQuery($query);
+ $db->execute();
+ $query ='CREATE INDEX IF NOT EXISTS "idx_execution"
</code_context>
<issue_to_address>
**suggestion:** Avoid double-executing the last PostgreSQL index creation and make query execution branch-local.
In the PostgreSQL branch you call `$db->setQuery($query); $db->execute();` for each statement, then after the `if/else` you call them again with the last `$query`, causing the last index creation (e.g. `idx_magiclogin_tokens_ip_address`) to run twice. Even though `IF NOT EXISTS` makes this safe, it’s redundant and obscures control flow. Move the final `setQuery/execute` into the non-PostgreSQL branch so each branch runs all of its own queries explicitly.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Removed redundant database query execution.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Add PostgreSQL-compatible creation and existence checks for plugin database tables while preserving MySQL behavior.
New Features:
Enhancements: