Alter table LOCK=SHARED #752
Conversation
WalkthroughAdds an adapter-level capability flag for altering per-column locks, a runtime toggle on Adapter and Database, SQL-level lock fragment insertion, per-adapter support declarations, pool delegation, and test setups enabling locks for MySQL/MariaDB. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test/Caller
participant DB as Database
participant Pool as AdapterPool
participant Adapter as Adapter(SQL/MariaDB/...)
participant SQL as SQLAdapter
Note over Test,DB: Configure test DB with locks enabled
Test->>DB: setNamespace(...).enableLocks(true)
DB->>Pool: getSupportForAlterLocks()
Pool->>Adapter: getSupportForAlterLocks()
alt supports alter locks
DB->>Adapter: enableAlterLocks(true)
else does not support
DB-->>Adapter: (no-op)
end
Note over Test,SQL: When generating ADD/ALTER COLUMN SQL
SQL->>Adapter: getLockType()
Adapter-->>SQL: ",LOCK=SHARED" or ""
SQL->>Test: SQL statement (with appended lock fragment)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/Database/Adapter/SQL.php (2)
250-250: Minor SQL formatting: space before comma in lock clause.The concatenation
{$type} {$this->getLockType()}wheregetLockType()returns,LOCK=SHAREDresults in SQL likeALTER TABLE ... ADD COLUMN col int ,LOCK=SHARED;with a space before the comma. While MariaDB may tolerate this, it's not standard SQL formatting.Consider either:
- Removing the space in concatenation:
{$type}{$this->getLockType()}- Or removing the leading comma from MariaDB's return value and adding it in the concatenation:
{$type}, {$this->getLockType()}
292-292: Same SQL formatting issue as line 250.This line has the same space-before-comma concatenation pattern as line 250. See the comment on that line for details and suggested improvements.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Database/Adapter/MariaDB.php(1 hunks)src/Database/Adapter/SQL.php(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/Database/Adapter/MariaDB.php (1)
src/Database/Adapter/SQL.php (1)
getLockType(262-265)
src/Database/Adapter/SQL.php (2)
src/Database/Adapter/SQLite.php (1)
getSQLTable(1128-1131)src/Database/Adapter/MariaDB.php (1)
getLockType(2218-2221)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Setup & Build Docker Image
🔇 Additional comments (2)
src/Database/Adapter/SQL.php (1)
262-265: LGTM!The base implementation correctly returns an empty string, allowing adapters that don't support or need the LOCK clause to continue working without changes. This is a clean extension point for database-specific locking behavior.
src/Database/Adapter/MariaDB.php (1)
2217-2221: ****The apparent inconsistency is actually consistent design across adapters. Both MariaDB and Postgres implementations of
createRelationshipomit locks from their ADD COLUMN statements (lines 471, 474, 478, 481 in MariaDB; equivalent lines in Postgres). This consistent pattern across multiple adapters indicates intentional design rather than an oversight—relationship columns may warrant different locking behavior than regular attributes created viacreateAttribute.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/Database/Adapter/SQLite.php (1)
1870-1873: Add docblock documentation for the new public API.The implementation is correct—SQLite doesn't support the
LOCKclause inALTER TABLEstatements, so returning an empty string is appropriate. However, as a new public API method, it should include a docblock explaining its purpose and why it returns an empty string for SQLite.Apply this diff to add documentation:
+ /** + * Get lock type clause for ALTER TABLE operations + * SQLite does not support the LOCK clause + * + * @return string + */ public function getLockType(): string { return ''; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Database/Adapter/SQLite.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/Database/Adapter/SQLite.php (2)
src/Database/Adapter/MariaDB.php (1)
getLockType(2218-2221)src/Database/Adapter/SQL.php (1)
getLockType(262-265)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Setup & Build Docker Image
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/Database/Adapter/Pool.php (1)
636-639: Improve method placement for consistency.The method follows the correct delegation pattern. However, for consistency with the existing code organization, this capability method should be placed alongside other
getSupportFor*methods (around lines 430-458) rather than at the end of the class aftersetAuthorization().Apply this diff to improve placement:
public function setAuthorization(Authorization $authorization): self { $this->authorization = $authorization; return $this; } - - public function getSupportForAlterLocks(): bool - { - return $this->delegate(__FUNCTION__, \func_get_args()); - } }Then add the method near line 458, after the other
getSupportFor*methods:public function getSupportForSpatialIndexNull(): bool { return $this->delegate(__FUNCTION__, \func_get_args()); } public function getSupportForAlterLocks(): bool { return $this->delegate(__FUNCTION__, \func_get_args()); } public function getCountOfAttributes(Document $collection): intsrc/Database/Adapter.php (2)
34-34: Consider a more descriptive property name.The property name
$alterLockscould be more explicit about its purpose. Consider renaming to$useAlterLocksor$enableAlterTableLocksto clarify that it controls whether ALTER TABLE operations should use explicit locking modes.
1446-1457: Enhance method documentation.The method implementation is correct and follows the fluent interface pattern. However, the docblock could be more descriptive about the method's purpose and when it should be used.
Consider enhancing the docblock:
/** + * Enable or disable explicit locking modes for ALTER TABLE operations. + * + * When enabled on supported adapters (e.g., MySQL/MariaDB), ALTER TABLE + * statements will include LOCK=SHARED to allow reads while preventing + * writes during schema modifications. + * * @param bool $bool * * @return $this - * @throws Exception */ public function enableLocks(bool $bool): selfNote: The
@throws Exceptiontag appears unnecessary as the method doesn't throw any exceptions.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/Database/Adapter.php(2 hunks)src/Database/Adapter/MariaDB.php(1 hunks)src/Database/Adapter/Mongo.php(1 hunks)src/Database/Adapter/Pool.php(1 hunks)src/Database/Adapter/SQL.php(3 hunks)src/Database/Adapter/SQLite.php(1 hunks)src/Database/Database.php(1 hunks)tests/e2e/Adapter/SharedTables/MariaDBTest.php(1 hunks)tests/e2e/Adapter/SharedTables/MySQLTest.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Database/Adapter/SQL.php
- src/Database/Adapter/MariaDB.php
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-29T12:27:57.071Z
Learnt from: ArnabChatterjee20k
Repo: utopia-php/database PR: 747
File: src/Database/Adapter/Mongo.php:1449-1453
Timestamp: 2025-10-29T12:27:57.071Z
Learning: In src/Database/Adapter/Mongo.php, when getSupportForAttributes() returns false (schemaless mode), the updateDocument method intentionally uses a raw document without $set operator for replacement-style updates, as confirmed by the repository maintainer ArnabChatterjee20k.
Applied to files:
src/Database/Adapter/Mongo.php
🧬 Code graph analysis (7)
src/Database/Adapter/Pool.php (6)
src/Database/Adapter.php (1)
getSupportForAlterLocks(1444-1444)src/Database/Adapter/MariaDB.php (1)
getSupportForAlterLocks(2218-2221)src/Database/Adapter/Mongo.php (1)
getSupportForAlterLocks(3213-3216)src/Database/Adapter/SQL.php (1)
getSupportForAlterLocks(3518-3521)src/Database/Adapter/SQLite.php (1)
getSupportForAlterLocks(1870-1873)src/Database/Mirror.php (1)
delegate(88-103)
src/Database/Adapter/Mongo.php (5)
src/Database/Adapter.php (1)
getSupportForAlterLocks(1444-1444)src/Database/Adapter/MariaDB.php (1)
getSupportForAlterLocks(2218-2221)src/Database/Adapter/Pool.php (1)
getSupportForAlterLocks(636-639)src/Database/Adapter/SQL.php (1)
getSupportForAlterLocks(3518-3521)src/Database/Adapter/SQLite.php (1)
getSupportForAlterLocks(1870-1873)
src/Database/Adapter/SQLite.php (5)
src/Database/Adapter.php (1)
getSupportForAlterLocks(1444-1444)src/Database/Adapter/MariaDB.php (1)
getSupportForAlterLocks(2218-2221)src/Database/Adapter/Mongo.php (1)
getSupportForAlterLocks(3213-3216)src/Database/Adapter/Pool.php (1)
getSupportForAlterLocks(636-639)src/Database/Adapter/SQL.php (1)
getSupportForAlterLocks(3518-3521)
src/Database/Adapter.php (6)
src/Database/Adapter/MariaDB.php (1)
getSupportForAlterLocks(2218-2221)src/Database/Adapter/Mongo.php (1)
getSupportForAlterLocks(3213-3216)src/Database/Adapter/Pool.php (1)
getSupportForAlterLocks(636-639)src/Database/Adapter/SQL.php (1)
getSupportForAlterLocks(3518-3521)src/Database/Adapter/SQLite.php (1)
getSupportForAlterLocks(1870-1873)src/Database/Database.php (1)
enableLocks(1159-1166)
tests/e2e/Adapter/SharedTables/MySQLTest.php (2)
src/Database/Adapter.php (1)
enableLocks(1452-1457)src/Database/Database.php (1)
enableLocks(1159-1166)
tests/e2e/Adapter/SharedTables/MariaDBTest.php (2)
src/Database/Adapter.php (2)
setNamespace(116-121)enableLocks(1452-1457)src/Database/Database.php (2)
setNamespace(832-837)enableLocks(1159-1166)
src/Database/Database.php (6)
src/Database/Adapter.php (2)
enableLocks(1452-1457)getSupportForAlterLocks(1444-1444)src/Database/Adapter/MariaDB.php (1)
getSupportForAlterLocks(2218-2221)src/Database/Adapter/Mongo.php (1)
getSupportForAlterLocks(3213-3216)src/Database/Adapter/Pool.php (1)
getSupportForAlterLocks(636-639)src/Database/Adapter/SQL.php (1)
getSupportForAlterLocks(3518-3521)src/Database/Adapter/SQLite.php (1)
getSupportForAlterLocks(1870-1873)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: Adapter Tests (MongoDB)
- GitHub Check: Adapter Tests (Schemaless/MongoDB)
- GitHub Check: Adapter Tests (SharedTables/Postgres)
- GitHub Check: Adapter Tests (SharedTables/MySQL)
- GitHub Check: Adapter Tests (Pool)
- GitHub Check: Adapter Tests (SharedTables/MongoDB)
- GitHub Check: Adapter Tests (SharedTables/SQLite)
- GitHub Check: Adapter Tests (MariaDB)
- GitHub Check: Adapter Tests (SharedTables/MariaDB)
- GitHub Check: Adapter Tests (Mirror)
- GitHub Check: Adapter Tests (MySQL)
- GitHub Check: Adapter Tests (Postgres)
- GitHub Check: Adapter Tests (SQLite)
- GitHub Check: Unit Test
🔇 Additional comments (4)
src/Database/Adapter/Mongo.php (1)
3213-3216: LGTM! Correct implementation for MongoDB.The method correctly returns
false, indicating that MongoDB does not support alter locks. This is appropriate since MongoDB is schemaless and doesn't have ALTER TABLE operations like SQL databases.tests/e2e/Adapter/SharedTables/MariaDBTest.php (1)
56-58: LGTM! Correctly enables locks for MariaDB test setup.The addition of
enableLocks(true)appropriately activates the alter lock feature for testing. Since MariaDB'sgetSupportForAlterLocks()returnstrue, theDatabase.enableLocks()method will successfully delegate to the adapter, enabling the LOCK=SHARED behavior during ALTER TABLE operations.src/Database/Adapter/SQLite.php (1)
1870-1873: LGTM! Correct implementation for SQLite.The method correctly returns
false, reflecting SQLite's limited ALTER TABLE support. This is consistent with SQLite's architectural constraints (as noted in the file header: "9. MODIFY COLUMN is not supported") and appropriately indicates that alter locks are not supported.tests/e2e/Adapter/SharedTables/MySQLTest.php (1)
58-60: LGTM!The fluent chaining correctly enables lock behavior for the MySQL test adapter. This aligns with the PR objective to support ALTER TABLE with LOCK=SHARED for MySQL/MariaDB adapters during schema modifications.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
To avoid inserts during ALTER TABLES when the default lock mode is NONE
Allows reads, Restricts write
We do not want duplicate violations when altering tables
Summary by CodeRabbit
New Features
Tests