Merged
Conversation
9 tasks
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 3 minutes and 51 seconds. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR extends the Asset domain model with lifecycle tracking ( Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Infrastructure/Persistence/MySQLAssetRepository.php`:
- Around line 32-73: In save() in MySQLAssetRepository, close the TOCTOU window
on the INSERT path by catching duplicate-key PDOException from the
$statement->execute($this->assetParameters($asset)) call; on duplicate-key, call
findById($asset->getId()) again and if the returned entity is non-null and
assetParameters($asset) === assetParameters($persisted) treat the save as
successful (return), otherwise rethrow the exception (or convert to the original
error flow); alternatively implement the insert path as an idempotent upsert
(e.g., INSERT ... ON DUPLICATE KEY UPDATE) that performs a no-op update for
identical rows. Ensure you reference save(), findById(), and assetParameters()
when making these changes and preserve assertSafeUpdate()/updateMutableFields()
behavior for true updates.
- Around line 211-223: assetParameters() is serializing created_at/updated_at
without timezone (using self::DATETIME_FORMAT), while the deserialization uses
DateTimeImmutable::createFromFormat() with no timezone, causing incorrect
round-trips across system timezones; update the serialization to produce
UTC-aware timestamps (either change self::DATETIME_FORMAT to an ISO-8601/offset
format like DateTime::ATOM or explicitly call ->setTimezone(new
DateTimeZone('UTC')) before formatting in assetParameters) and update the
corresponding parsing code that uses
DateTimeImmutable::createFromFormat()/DateTimeImmutable(...) to pass new
DateTimeZone('UTC') (or parse ISO-8601 directly) so both storage and
reconstruction consistently use UTC for created_at and updated_at.
In `@tests/Integration/Infrastructure/Persistence/AssetsTableBootstrapTest.php`:
- Around line 67-230: Tests contain inline boundary/equivalence matrices and
non-conforming names; extract those matrices into static provider methods and
annotate the tests with #[DataProvider('providerName')] and rename the test
methods to follow the itReturns…When… / itThrows…When… convention (e.g., change
itAcceptsRowsForEveryValidAssetLifecycleState to
itReturnsPersistedLifecycleStateWhenRowIsValid and move $validRows into a static
provider like provideValidLifecycleRows(), and change
itRejectsRowsThatViolateAssetLifecycleConstraints to
itThrowsConstraintViolationWhenRowInvalid with a static provider
provideInvalidLifecycleCases()); update the tests to accept provider parameters
and reuse helper methods like insertAssetRow, assertInsertFails,
validPendingRow, validFailedRow, validUploadedRow, and ensure provider methods
are static and referenced by name in the #[DataProvider] attribute.
In `@tests/Integration/Infrastructure/Persistence/MySQLAssetRepositoryTest.php`:
- Around line 35-444: Rename the test methods that use verbs like itSaves…,
itUpdates…, itAccepts…, and itKeeps… to the repository's required naming
convention (itReturns{X}When{Condition} / itThrows{Exception}When{Condition}),
e.g. rename itSavesAndReadsAPendingAsset ->
itReturnsAssetWhenSavingAndReadingAPendingAsset, itSavesAndReadsAnUploadedAsset
-> itReturnsAssetWhenSavingAndReadingAnUploadedAsset,
itUpdatesAnExistingRowAfterAnAssetStateChange ->
itReturnsUpdatedRowWhenAssetStateChanges,
itAcceptsALegitimateStateChangeWhenUpdatedAtRemainsEqual ->
itReturnsAcceptedStateWhenUpdatedAtRemainsEqual, and
itKeepsASingleRowWhenSavingTheSameUnchangedAssetTwice ->
itReturnsSingleRowWhenSavingUnchangedAssetTwice; update the method names in the
test class (MySQLAssetRepositoryTest) and any references to them so they follow
the itReturns/itThrows…When… pattern.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: bb2e41cb-cee8-46f7-b66c-e19d263e4f43
📒 Files selected for processing (21)
docs/adr/04-asset-domain-contracts.mddocs/api/01-agree-api-schema.mddocs/logs/03-data-local-storage-setup.mdmigrations/20260401120000_create_assets_table.sqlmkdocs.ymlsrc/Domain/Asset/Asset.phpsrc/Domain/Asset/AssetRepositoryInterface.phpsrc/Domain/Asset/ClockInterface.phpsrc/Domain/Asset/Exception/StaleAssetWriteException.phpsrc/Domain/Asset/SystemClock.phpsrc/Domain/Asset/ValueObject/UploadId.phpsrc/Domain/Asset/ValueObject/UploadTarget.phpsrc/Infrastructure/Persistence/MySQLAssetRepository.phpsrc/Infrastructure/Storage/MockStorageAdapter.phptests/Integration/Infrastructure/Persistence/AssetsTableBootstrapTest.phptests/Integration/Infrastructure/Persistence/MySQLAssetRepositoryTest.phptests/Integration/Support/CompareAndSwapRacePdo.phptests/Integration/Support/CompareAndSwapRaceStatement.phptests/Unit/Domain/Asset/AssetTest.phptests/Unit/Domain/Asset/ValueObject/UploadTargetTest.phptests/Unit/Infrastructure/Storage/MockStorageAdapterTest.php
… MySQLAssetRepository; split large bootstrap test
…HPUnit abstract-class warning
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Infrastructure/Persistence/MySQLAssetRepository.php`:
- Around line 194-210: In updateMutableFields, do not immediately throw
StaleAssetWriteException when $statement->rowCount() === 0; instead re-read the
current row (e.g. via the repository's findById/fetch method or by selecting the
asset by id) and compare its mutable fields to the desired state produced by
mutableUpdateParameters($asset, $persistedAsset); if the re-read row already
matches the intended status/chunk_count/completion_proof/updated_at, treat the
update as successful (return normally), otherwise throw
StaleAssetWriteException(self::STALE_WRITE_MESSAGE). Ensure you reference
updateMutableFields, mutableUpdateParameters and StaleAssetWriteException when
making the change.
In `@tests/Integration/Infrastructure/Persistence/AssetsTableSchemaTest.php`:
- Around line 12-13: Rename the test method
itCreatesTheExpectedAssetsTableSchemaAndCanBeReappliedSafely to follow the
required pattern; change the method name (the #[Test] method) to a form like
itReturnsExpectedSchemaWhenReappliedSafely (or another
itReturns/itThrows/itEmits/itDoesNot variant that matches the assertion intent)
so the test name matches the itReturns…When… style enforced for tests/**/*.php.
In `@tests/Integration/Infrastructure/Persistence/BaseAssetsTableTest.php`:
- Line 11: Rename the abstract test helper class BaseAssetsTableTest to
BaseAssetsTableFixture to avoid PHPUnit discovering it as a test; update the
class declaration for the abstract helper and then update any inheritance
references where other test classes extend it—specifically change extends
BaseAssetsTableTest to extends BaseAssetsTableFixture in AssetsTableSchemaTest
and AssetsTableLifecycleTest so the abstract fixture is no longer treated as a
concrete test by PHPUnit.
In `@tests/Integration/Infrastructure/Persistence/MySQLAssetRepositoryTest.php`:
- Line 168: Several test methods still use disallowed names like
itRejectsALaterSaveWhenTheSameAssetIdentityChangesAnImmutableField and the other
itSearches… / itSurfaces… methods; rename each to the approved pattern
(itReturns{X}When{Condition}, itThrows{Exception}When{Condition},
itEmits{Event}When{Condition}, or itDoesNot{X}When{Condition}). Concretely,
rename itRejectsALaterSaveWhenTheSameAssetIdentityChangesAnImmutableField to an
itThrows{Exception}WhenSameAssetIdentityChangesImmutableField form (choose the
actual Exception name used in the test), rename the itSearches… methods to
itReturns{ExpectedResult}When{Condition}, and rename the itSurfaces… methods to
itEmits{Event}When{Condition}; update the method declarations (e.g., the test
method names listed at lines 168, 212, 257, 337, 409) and any test references so
they match the new names.
- Around line 735-740: The tests are creating DateTimeImmutable instances
without specifying timezone, causing flaky failures on non-UTC CI; update
persistedState() to construct DateTimeImmutable objects using an explicit UTC
timezone (e.g., new \DateTimeZone('UTC')) for both createdAt and updatedAt, and
update assertAssetMatches() to normalize/compare timestamps in UTC (either by
creating expected DateTimeImmutable with UTC or converting actual values to UTC
before comparison) so assertions are timezone-independent; adjust any
parse/format logic in assertAssetMatches() to use UTC when comparing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 557006bd-61d5-458b-ab04-17ceefdc0343
📒 Files selected for processing (5)
src/Infrastructure/Persistence/MySQLAssetRepository.phptests/Integration/Infrastructure/Persistence/AssetsTableLifecycleTest.phptests/Integration/Infrastructure/Persistence/AssetsTableSchemaTest.phptests/Integration/Infrastructure/Persistence/BaseAssetsTableTest.phptests/Integration/Infrastructure/Persistence/MySQLAssetRepositoryTest.php
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/Integration/Infrastructure/Persistence/AssetsTableLifecycleTest.php`:
- Around line 45-71: The test itThrowsConstraintViolationWhenRowInvalid
currently takes a boolean preInsertBaseRow to control inserting a base row;
extract that conditional pre-insert behaviour into a separate helper (e.g.,
create a new method like maybeInsertBaseRow(PDO $connection, ?array $baseRow,
bool $shouldInsert) or better yet split into two focused tests/useDataProvider
variants) and call it from inside the withTemporarySchema closure instead of
passing the flag; update usages of insertAssetRow, validPendingRow,
validFailedRow, validUploadedRow and the DataProvider cases to remove the
boolean parameter so the method signature for
itThrowsConstraintViolationWhenRowInvalid no longer needs the preInsertBaseRow
flag.
In `@tests/Integration/Infrastructure/Persistence/BaseAssetsTableTestCase.php`:
- Around line 141-152: The createConnection method returns a PDO without any
timeout—update createConnection to set a connection timeout so tests don't hang:
when constructing PDO in createConnection(string $dsn, string $user, string
$password) add a timeout option (e.g. include PDO::ATTR_TIMEOUT => <seconds> in
the options array, or if using a driver that requires it, append the appropriate
timeout parameter to the $dsn) and choose a reasonable short value (e.g. 5
seconds) so database connect attempts fail fast during tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6d7cbea3-5ce7-4afa-b51c-a4f0aa1bd6b3
📒 Files selected for processing (3)
tests/Integration/Infrastructure/Persistence/AssetsTableLifecycleTest.phptests/Integration/Infrastructure/Persistence/AssetsTableSchemaTest.phptests/Integration/Infrastructure/Persistence/BaseAssetsTableTestCase.php
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
1 participant
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.
Overview
This PR implements durable local data persistence for the DAM system, extending the Asset domain model with lifecycle tracking and introducing a complete MySQL repository implementation with local-development mock storage support.
Domain Model Enhancements
The
Assetdomain model now tracks additional lifecycle state:chunkCount(minimum 1) andupdatedAtfields to complement existingcreatedAtClockInterfaceabstraction for time-based operations, withSystemClockproviding system timecreatePending,reconstitute,reconstituteUploaded) to accept lifecycle state as structured data rather than individual parametersmarkUploaded,markFailed) now advanceupdatedAtwith monotonic timestamp validation ensuringupdatedAt >= createdAtchunkCountandupdatedAtRepository & Persistence
AssetRepositoryInterface extended with:
searchByFileName(AccountId $accountId, string $query): array- performs account-scoped, case-insensitive substring matching with results ordered bycreatedAtdescending, thenidascending; returns empty list for blank queries after trimmingMySQLAssetRepository implementation provides:
StaleAssetWriteExceptionon concurrent modificationsfindById,findByUploadId, andsearchByFileNamewith prepared statementsDatabase Schema (
migrations/20260401120000_create_assets_table.sql):assetstable with columns for identity (id,upload_id), tenancy (account_id), file metadata, processing state, chunking, and optional completion proofchunk_count >= 1, lifecycle invariants, andupload_iduniquenessStorage Adapter
MockStorageAdapterprovides deterministic local-development upload targets:mock://uploads/{uploadId}/chunk/0UploadTarget & URL Validation
UploadTarget.normalizeUrl()enhanced to:mock://uploads/{uploadId}/chunk/0) for local developmentuploadIdsegments against UUID patternTesting
Comprehensive test coverage added:
Helper utilities:
CompareAndSwapRacePdoandCompareAndSwapRaceStatementfor simulating concurrent write races in integration testsDocumentation
searchByFileNamesemanticsUploadTarget.urlto specify HTTPS for production and deterministic mock form for local development