Upsert fix for postgres #613
Conversation
|
""" WalkthroughThe changes refactor and centralize upsert (create-or-update) logic for SQL adapters. The MariaDB and Postgres adapters now provide a Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant SQLAdapter
participant DB
Client->>SQLAdapter: createOrUpdateDocuments(collection, attribute, changes)
SQLAdapter->>SQLAdapter: Prepare attributes, permissions, and batch keys
SQLAdapter->>SQLAdapter: getUpsertStatement(...)
SQLAdapter->>DB: Execute upsert statement
SQLAdapter->>DB: Execute permission removals/additions
SQLAdapter->>DB: Fetch updated sequences (if needed)
SQLAdapter-->>Client: Return updated Document objects
Estimated code review effort
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🧠 Learnings (2)📓 Common learningssrc/Database/Adapter/MariaDB.php (1)Learnt from: ArnabChatterjee20k 🔇 Additional comments (1)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
src/Database/Adapter/MariaDB.php (1)
1151-1166: Fix PHPDoc type hints for accuracyThe PHPDoc type hints appear to be inaccurate. The
$attributesparameter likely contains mixed values (strings, integers, booleans, etc.), not just strings. Similarly,$bindValuesis typed asarray<mixed>which is correct, but$batchKeysmight also need clarification./** * @param string $tableName * @param string $columns * @param array<string> $batchKeys - * @param array<string> $attributes + * @param array<string, mixed> $attributes * @param array<mixed> $bindValues * @param string $attribute * @return mixed */src/Database/Adapter/Postgres.php (1)
1256-1271: Fix PHPDoc type hints and consider return typeSimilar to the MariaDB adapter, the PHPDoc type hints need adjustment. Also, the return type
mixedcould be more specific./** * @param string $tableName * @param string $columns * @param array<string> $batchKeys - * @param array<string> $attributes + * @param array<string, mixed> $attributes * @param array<mixed> $bindValues * @param string $attribute - * @return mixed + * @return \PDOStatement */ protected function getUpsertStatement( string $tableName, string $columns, array $batchKeys, array $attributes, array $bindValues, string $attribute = '', - ): mixed { + ): \PDOStatement {src/Database/Adapter/SQL.php (1)
1924-2099: Consider breaking down this complex method.The
createOrUpdateDocumentsmethod is 175 lines long with high cyclomatic complexity, handling multiple responsibilities including document preparation, upsert execution, and permission synchronization.Consider extracting the permission synchronization logic (lines 2002-2081) into a separate protected method like
synchronizePermissions(string $collection, array $changes)to improve readability and maintainability.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Database/Adapter/MariaDB.php(1 hunks)src/Database/Adapter/Postgres.php(8 hunks)src/Database/Adapter/SQL.php(4 hunks)
🔇 Additional comments (6)
src/Database/Adapter/MariaDB.php (1)
1167-1213: Well-implemented UPSERT statement builderThe implementation is clean and well-structured:
- Good use of closure for reusable update clause generation
- Proper tenant isolation with conditional updates
- Secure parameter binding prevents SQL injection
- Clear separation between increment and full update modes
src/Database/Adapter/Postgres.php (4)
241-242: Good addition of UNIQUE constraint on _uidAdding the UNIQUE constraint on
_uidis essential for:
- Ensuring document ID uniqueness at the database level
- Enabling the UPSERT functionality with
ON CONFLICT ("_uid")- Maintaining consistency with the MariaDB adapter
276-285: Fix for SQL string concatenationGood catch fixing the string concatenation. The previous approach with string interpolation inside the SQL would have resulted in malformed SQL statements.
1272-1319: Well-implemented Postgres UPSERT with tenant isolationThe implementation correctly adapts the UPSERT logic for Postgres:
- Proper use of
ON CONFLICT ("_uid") DO UPDATEsyntax- Correct usage of
EXCLUDEDpseudo-table for new values- Tenant-aware updates using
CASE WHENstatements- Maintains feature parity with MariaDB adapter
2037-2040: Enable upsert support for PostgresCorrectly enables upsert support now that the
getUpsertStatementmethod is implemented.src/Database/Adapter/SQL.php (1)
1544-1544: LGTM!Good formatting improvement for consistency.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
src/Database/Adapter/SQL.php (3)
1398-1414: Fix PHPDoc syntax error and improve documentation.The PHPDoc has a trailing comma that should be removed, and the parameter documentation needs improvement.
1944-1953: Clarify variable usage to avoid confusion.The
$attributesvariable is initialized as an empty array at line 1944 but then overwritten in the loop at line 1953. This could lead to confusion about its purpose.
1975-1979: Fix incorrect PHPDoc comment.The PHPDoc comment references the wrong variable name.
🧹 Nitpick comments (1)
src/Database/Adapter/SQL.php (1)
1924-2099: Consider refactoring this complex method for better maintainability.This method is quite large (175 lines) and handles multiple responsibilities. The permission handling logic appears to be duplicated from other methods in the class.
Consider extracting helper methods for:
- Permission difference calculation
- Permission removal query building
- Permission addition query building
- Document attribute preparation
This would improve readability, reduce duplication, and make the code more testable.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
log.logis excluded by!**/*.loglog_pg.logis excluded by!**/*.log
📒 Files selected for processing (1)
src/Database/Adapter/SQL.php(4 hunks)
🔇 Additional comments (2)
src/Database/Adapter/SQL.php (2)
8-8: LGTM!The import is correctly added for the
Changeclass used in the newcreateOrUpdateDocumentsmethod.
1544-1544: LGTM!Good formatting improvement with consistent spacing around the concatenation operator.
ArnabChatterjee20k
force-pushed
the
dat-557
branch
from
5fd882e to
630329f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Database/Adapter/Postgres.php(9 hunks)
🧰 Additional context used
🪛 PHPMD (2.15.0)
src/Database/Adapter/Postgres.php
1271-1271: The method getUpsertStatement has a boolean flag argument $increment, which is a certain sign of a Single Responsibility Principle violation. (Clean Code Rules)
(BooleanArgumentFlag)
1275-1277: The method getUpsertStatement uses an else expression. Else clauses are basically not necessary and you can simplify the code by not using them. (Clean Code Rules)
(ElseExpression)
1294-1303: The method getUpsertStatement uses an else expression. Else clauses are basically not necessary and you can simplify the code by not using them. (Clean Code Rules)
(ElseExpression)
🔇 Additional comments (7)
src/Database/Adapter/Postgres.php (7)
148-150: LGTM - Clean formatting improvement.The indentation fix improves code readability for the method chaining.
235-235: LGTM - Improved unique index creation.The changes correctly implement explicit quoting for unique indexes on
_uid(and_tenantfor shared tables) instead of usingLOWER(). This aligns with the past review comment about unique keys for shared tables requiring(_uid, _tenant).Also applies to: 246-246, 253-253
275-275: LGTM - Consistent SQL formatting.The formatting improvements enhance readability of the permission index creation statements.
Also applies to: 277-277, 282-282, 284-284
577-577: LGTM - Consistent indentation.Minor formatting improvement for better code readability.
1528-1528: LGTM - Consistent formatting.Minor improvement to ORDER BY clause formatting.
1811-1811: LGTM - Improved readability.The spacing improvement makes the condition concatenation more readable.
2039-2039: LGTM - Enables upsert functionality.This change activates the upsert support for Postgres, which is consistent with the new
getUpsertStatementimplementation. Ensure that the upsert functionality is thoroughly tested, especially the tenant isolation logic for shared tables.
3 participants
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Style