feat: support auto incrementing on Model #258
Conversation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/Eloquent/ModelTest.php (1)
505-511: Enhance test coverage for auto-increment functionality.While the test verifies the initial identity value, consider adding:
- Multiple insert tests to verify the incremental behavior
- Assertions for the non-key fields
- Comments explaining why 4611686018427387904 is the expected first value
public function test_insertAndSetId(): void { + // First value in Spanner's bit-reversed sequence $test = new IdentityTest(); $test->name = 'test'; $test->saveOrFail(); $this->assertSame(4611686018427387904, $test->getKey()); + $this->assertSame('test', $test->name); + + // Verify incremental behavior + $test2 = new IdentityTest(); + $test2->name = 'test2'; + $test2->saveOrFail(); + $this->assertGreaterThan($test->getKey(), $test2->getKey()); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
phpstan.neon(1 hunks)src/Eloquent/Concerns/DoesNotAutoIncrement.php(0 hunks)src/Eloquent/Model.php(1 hunks)src/Query/Grammar.php(1 hunks)src/Query/Processor.php(1 hunks)tests/Eloquent/ModelTest.php(2 hunks)tests/test.ddl(1 hunks)
💤 Files with no reviewable changes (1)
- src/Eloquent/Concerns/DoesNotAutoIncrement.php
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Run-PHPUnit
🔇 Additional comments (5)
tests/test.ddl (1)
68-71: Well-structured table definition with optimal identity configuration!Good choice using
BIT_REVERSED_POSITIVEfor the identity column. This helps prevent hotspots in distributed systems by ensuring keys are distributed across multiple servers.src/Eloquent/Model.php (1)
Line range hint
31-39: Consider making$incrementingconfigurable per modelWhile removing the
DoesNotAutoIncrementtrait is correct for supporting auto-increment, the hardcoded$incrementing = falsemight prevent models from utilizing auto-increment functionality. Consider making this property configurable in child classes.Let's check if any models need to override this property:
phpstan.neon (1)
42-44: LGTM!The PHPStan ignore rule is correctly scoped to the specific file and occurrence count.
src/Query/Grammar.php (1)
54-54: LGTM! Verify SQL syntax with Spanner documentation.The implementation correctly combines the insert query with a return clause for auto-incrementing columns.
Let's verify the SQL syntax against Spanner's documentation:
tests/Eloquent/ModelTest.php (1)
178-184: LGTM! Model configuration aligns with Spanner's IDENTITY requirements.The model is correctly configured for auto-incrementing with:
- Primary key set to 'identityTestId'
- $incrementing set to true
- Timestamps disabled for focused testing
src/Query/Processor.php
Outdated
| public function processInsertGetId(Builder $query, $sql, $values, $sequence = null) | ||
| { | ||
| $connection = $query->getConnection(); | ||
|
|
||
| $connection->recordsHaveBeenModified(); | ||
|
|
||
| $result = $connection->select($sql, $values)[0]; | ||
|
|
||
| $sequence = $sequence ?: 'id'; | ||
|
|
||
| return is_object($result) | ||
| ? $result->{$sequence} | ||
| : $result[$sequence]; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and type hints
The implementation looks good but could be improved in two areas:
- Add error handling for empty results array
- Add proper type hints for parameters and return value
Here's the suggested implementation:
- public function processInsertGetId(Builder $query, $sql, $values, $sequence = null)
+ public function processInsertGetId(Builder $query, string $sql, array $values, ?string $sequence = null): int
{
$connection = $query->getConnection();
$connection->recordsHaveBeenModified();
- $result = $connection->select($sql, $values)[0];
+ $results = $connection->select($sql, $values);
+ if (empty($results)) {
+ throw new \RuntimeException('Failed to get last inserted ID');
+ }
+ $result = $results[0];
$sequence = $sequence ?: 'id';
return is_object($result)
- ? $result->{$sequence}
- : $result[$sequence];
+ ? (int) $result->{$sequence}
+ : (int) $result[$sequence];
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public function processInsertGetId(Builder $query, $sql, $values, $sequence = null) | |
| { | |
| $connection = $query->getConnection(); | |
| $connection->recordsHaveBeenModified(); | |
| $result = $connection->select($sql, $values)[0]; | |
| $sequence = $sequence ?: 'id'; | |
| return is_object($result) | |
| ? $result->{$sequence} | |
| : $result[$sequence]; | |
| } | |
| public function processInsertGetId(Builder $query, string $sql, array $values, ?string $sequence = null): int | |
| { | |
| $connection = $query->getConnection(); | |
| $connection->recordsHaveBeenModified(); | |
| $results = $connection->select($sql, $values); | |
| if (empty($results)) { | |
| throw new \RuntimeException('Failed to get last inserted ID'); | |
| } | |
| $result = $results[0]; | |
| $sequence = $sequence ?: 'id'; | |
| return is_object($result) | |
| ? (int) $result->{$sequence} | |
| : (int) $result[$sequence]; | |
| } |
There was a problem hiding this comment.
I think we should add a test for Builder::insertGetId().
When I tried to run IdentityTest::query()->insertGetId(['name' => 'foobar'], 'identityTestId'), I got an error.
Illuminate\Database\QueryException: {
"message": "DML statements may not be performed in single-use transactions, to avoid replay.",
"code": 3,
"status": "INVALID_ARGUMENT",
"details": [
{
"@type": "grpc-status-details-bin",
"data": "<Unknown Binary Data>"
}
]
} (Connection: main, SQL: insert into `IdentityTest` (`name`) values (@p0) then return `identityTestId`)
Syntax appears to be correct.
The error suggests that you forgot to wrap it in a transaction? |
|
I don't think it's a transaction issue, because |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/Query/BuilderTest.php (1)
986-989: Enhance test coverage for insertGetId functionality.While the test verifies the ID generation, consider these improvements:
- Verify that the row was actually inserted by fetching and asserting the record.
- Add test cases for transaction handling given the reported issues in PR comments.
- Document why
4611686018427387904is the expected ID value.Consider expanding the test like this:
public function test_insertGetId(): void { $conn = $this->getDefaultConnection(); - $id = $conn->query()->from('IdentityTest')->insertGetId(['name' => 'foobar'], 'identityTestId'); - $this->assertSame(4611686018427387904, $id); + + // Verify the row was actually inserted + $insertedRow = $conn->query()->from('IdentityTest')->where('identityTestId', $id)->first(); + $this->assertNotNull($insertedRow); + $this->assertSame('foobar', $insertedRow['name']); + + // Test with transaction + $conn->transaction(function($conn) { + $id = $conn->query()->from('IdentityTest')->insertGetId(['name' => 'transaction_test'], 'identityTestId'); + $this->assertIsInt($id); + }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Query/Grammar.php(1 hunks)src/Query/Processor.php(1 hunks)tests/Query/BuilderTest.php(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Query/Processor.php
- src/Query/Grammar.php
# Conflicts: # phpstan.neon # src/Eloquent/Concerns/DoesNotAutoIncrement.php
taka-oyama
force-pushed
the
feature/add-insert-set-id
branch
from
20a114f to
0321a1a
Compare
|
Should be fixed now. I forgot laravel-spanner does auto commit under the hood. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/Query/Processor.php (1)
32-34: 🛠️ Refactor suggestionAdd return type hint to method signature.
The method signature should include the return type hint since it always returns an integer ID.
- public function processInsertGetId(Builder $query, $sql, $values, $sequence = null) + public function processInsertGetId(Builder $query, string $sql, array $values, ?string $sequence = null): int
🧹 Nitpick comments (3)
src/Query/Processor.php (2)
48-57: Add error handling for edge cases.While the type checking is good, consider adding explicit error handling:
- Handle null/empty result from
selectOne- Validate ID is numeric before assertion
+ if ($result === null) { + throw new LogicException('No result returned from insert operation'); + } + $id = match(true) { is_object($result) => $result->{$sequence}, is_array($result) => $result[$sequence], default => throw new LogicException('Unknown result type : ' . gettype($result)), }; + if (!is_numeric($id)) { + throw new LogicException("Retrieved ID is not numeric: " . gettype($id)); + } + assert(is_int($id)); return $id;
34-34: Add comprehensive PHPDoc.Add PHPDoc to document parameters, return value, and transaction behavior.
+ /** + * Process an insert get ID query and return the ID. + * + * @param Builder $query The query builder instance + * @param string $sql The SQL query to execute + * @param array<array-key, mixed> $values The values to bind to the query + * @param string|null $sequence The sequence/column name to retrieve the ID from + * @throws LogicException If the result is invalid or ID cannot be retrieved + * @return int The auto-incremented ID + * + * Note: This method will execute the query in a transaction if not already in one. + */ public function processInsertGetId(Builder $query, $sql, $values, $sequence = null)tests/Eloquent/ModelTest.php (1)
178-184: Add PHPDoc block for property types and document auto-incrementing behavior.Consider adding property type documentation for better IDE support and code clarity:
+/** + * @property int $identityTestId Auto-incrementing primary key starting at 4611686018427387904 + * @property string $name + */ class IdentityTest extends Model
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
phpstan.neon(1 hunks)src/Eloquent/Concerns/DoesNotAutoIncrement.php(0 hunks)src/Eloquent/Model.php(1 hunks)src/Query/Grammar.php(1 hunks)src/Query/Processor.php(1 hunks)tests/Eloquent/ModelTest.php(2 hunks)tests/Query/BuilderTest.php(2 hunks)tests/test.ddl(1 hunks)
💤 Files with no reviewable changes (1)
- src/Eloquent/Concerns/DoesNotAutoIncrement.php
🚧 Files skipped from review as they are similar to previous changes (5)
- tests/test.ddl
- tests/Query/BuilderTest.php
- phpstan.neon
- src/Eloquent/Model.php
- src/Query/Grammar.php
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Run-PHPUnit
🔇 Additional comments (3)
src/Query/Processor.php (2)
26-26: LGTM!The
LogicExceptionimport is correctly placed and properly used in the new method.
40-44: LGTM! Good transaction handling.The implementation correctly handles transactions by checking the transaction level and wrapping the query in a transaction if needed. This addresses the transaction-related issue mentioned in the PR comments.
tests/Eloquent/ModelTest.php (1)
505-511: 🛠️ Refactor suggestionAdd test cases for transaction behavior.
Based on the reported issues in the PR comments, we should add test cases to verify the behavior both with and without transactions:
+ public function test_insertAndSetId_with_transaction(): void + { + $test = new IdentityTest(); + $test->name = 'test'; + + DB::transaction(function() use ($test) { + $test->saveOrFail(); + }); + + $this->assertSame(self::SPANNER_IDENTITY_START, $test->getKey()); + }Let's verify the transaction behavior in the codebase:
WalkthroughThis pull request introduces support for auto-incrementing IDs in Spanner by removing the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/Eloquent/ModelTest.php (1)
505-512: 🛠️ Refactor suggestionEnhance test coverage for auto-incrementing behavior.
The test should verify sequential increments and document the significance of the starting value.
Apply this diff to improve the test:
+ private const SPANNER_IDENTITY_START = 4611686018427387904; // 1 << 62 + public function test_insertAndSetId(): void { $test = new IdentityTest(); $test->name = 'test'; $test->saveOrFail(); - $this->assertIsInt($test->getKey()); - $this->assertTrue($test->getKey() > 1000000000000000000); + $this->assertSame(self::SPANNER_IDENTITY_START, $test->getKey()); + + $test2 = new IdentityTest(); + $test2->name = 'test2'; + $test2->saveOrFail(); + $this->assertSame(self::SPANNER_IDENTITY_START + 1, $test2->getKey()); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
phpstan.neon(1 hunks)src/Eloquent/Concerns/DoesNotAutoIncrement.php(0 hunks)src/Eloquent/Model.php(1 hunks)src/Query/Grammar.php(1 hunks)src/Query/Processor.php(1 hunks)tests/Eloquent/ModelTest.php(2 hunks)tests/Query/BuilderTest.php(2 hunks)tests/test.ddl(1 hunks)
💤 Files with no reviewable changes (1)
- src/Eloquent/Concerns/DoesNotAutoIncrement.php
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Run-PHPUnit
🔇 Additional comments (10)
phpstan.neon (1)
5-8: LGTM! The PHPStan configuration is appropriate.The error suppression is correctly scoped to a single occurrence of the undefined method error, which is necessary since
recordsHaveBeenModified()exists in Laravel's implementation but isn't detected by PHPStan's static analysis.tests/test.ddl (1)
68-71: LGTM! Well-structured schema with performance considerations.Good choice using
BIT_REVERSED_POSITIVEfor the identity column, as it helps prevent hotspots in Spanner by distributing the generated IDs across multiple servers.src/Eloquent/Model.php (1)
31-31: Consider the default value of$incrementing.The class removes the
DoesNotAutoIncrementtrait to support auto-incrementing, but still has$incrementing = falseby default. This might cause issues for models that use auto-incrementing IDs, as Laravel uses this property to determine whether to expect auto-generated IDs.Let's verify if any models with auto-incrementing IDs exist:
Also applies to: 39-41
src/Query/Processor.php (5)
32-33: LGTM! Good docblock for values parameter.The
@paramannotation correctly specifies the type for the$valuesparameter.
34-34: Add type hints for parameters and return value.The implementation looks good but would benefit from explicit type hints.
Apply this diff:
- public function processInsertGetId(Builder $query, $sql, $values, $sequence = null) + public function processInsertGetId(Builder $query, string $sql, array $values, ?string $sequence = null): intAlso applies to: 56-56
40-44: LGTM! Good transaction handling.The code correctly handles both transactional and non-transactional contexts using a closure, which ensures consistent behavior.
48-52: LGTM! Well-structured match expression.The match expression provides clear error handling with a descriptive message for unknown result types.
54-54: LGTM! Good type assertion.The assertion helps catch any type mismatches early, ensuring the returned ID is always an integer.
src/Query/Grammar.php (1)
56-60: LGTM! The implementation correctly handles auto-incrementing IDs.The method now properly generates a SQL statement for inserting a record and retrieving its ID, with proper column name escaping and support for custom sequence names.
tests/Eloquent/ModelTest.php (1)
178-184: LGTM! The model is correctly configured for auto-incrementing.The
IdentityTestmodel is properly set up with the required configuration for auto-incrementing IDs.
|
|
||
| $id = $conn->query()->from('IdentityTest')->insertGetId(['name' => 'foobar'], 'identityTestId'); | ||
|
|
||
| $this->assertIsInt($id); | ||
| $this->assertTrue($id > 1000000000000000000); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Align test assertions with ModelTest.
The test should use the same assertions and constants as test_insertAndSetId in ModelTest.php for consistency.
Apply this diff to align the assertions:
+ private const SPANNER_IDENTITY_START = 4611686018427387904; // 1 << 62
+
public function test_insertGetId(): void
{
$conn = $this->getDefaultConnection();
$id = $conn->query()->from('IdentityTest')->insertGetId(['name' => 'foobar'], 'identityTestId');
- $this->assertIsInt($id);
- $this->assertTrue($id > 1000000000000000000);
+ $this->assertSame(self::SPANNER_IDENTITY_START, $id);
}Committable suggestion skipped: line range outside the PR's diff.
taka-oyama
changed the title
|
Thank you for reviewing |
Closes #257
Closes #256
Summary by CodeRabbit
Release Notes
New Features
IdentityTestwith auto-generated IDs.Bug Fixes
Tests
IdentityTestand corresponding test cases for ID generation.Refactor
DoesNotAutoIncrementtrait from the model.