feat: Sync infra & initial sync logic for existing syncables #34
Conversation
nfebe
force-pushed
the
feat/13/syncing-infrastructure-init
branch
2 times, most recently
from
6f40b59 to
14dffd1
Compare
nfebe
force-pushed
the
feat/13/syncing-infrastructure-init
branch
2 times, most recently
from
a114a66 to
dadadde
Compare
nfebe
changed the title
nfebe
force-pushed
the
feat/13/syncing-infrastructure-init
branch
from
dadadde to
e72a451
Compare
nfebe
force-pushed
the
feat/13/syncing-infrastructure-init
branch
2 times, most recently
from
80d3b49 to
8bcca06
Compare
nfebe
force-pushed
the
feat/13/syncing-infrastructure-init
branch
from
8bcca06 to
6f2dac7
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🔭 Outside diff range comments (2)
app/Http/Controllers/API/v1/CategoryController.php (1)
101-109: 💡 Verification agent🧩 Analysis chain
Potential issue with
firstOrCreatehandling of new fields.While validation rules have been updated to accept
client_idandcreated_at, the controller usesfirstOrCreatewhich might not properly trigger the trait hooks that handle these synchronization fields.Consider explicitly creating and saving the category instead of using
firstOrCreate:$data = $validator->validated(); $user = $request->user(); $data['user_id'] = $user->id; $category_exists = $user->categories()->where('name', $data['name'])->where('user_id', $user->id)->first(); if ($category_exists) { return $this->failure('Category already exists', 400); } -$category = $user->categories()->firstOrCreate($data); +$category = $user->categories()->create($data); + +// Set client ID if provided +if (!empty($data['client_id'])) { + $category->syncState()->update(['client_generated_id' => $data['client_id']]); +} + +// Mark the category as synced +$category->markAsSynced();
🏁 Script executed:
#!/bin/bash # Check if there's a Syncable trait implementation that handles this automatically rg -A 10 "trait Syncable" app/Traits/ | grep -E "markAsSynced|setClientId"Length of output: 76
🏁 Script executed:
#!/bin/bash # Locate the Syncable trait definition rg "trait Syncable" -n . # If found, show its first 200 lines for inspection syncable_file=$(rg -l "trait Syncable" -n .) if [[ -n "$syncable_file" ]]; then echo "==== $syncable_file ====" sed -n '1,200p' "$syncable_file" else echo "Syncable trait not found" fi # Search for trait helper methods and fields rg "markAsSynced" -n . rg "syncState" -n . rg "client_generated_id" -n .Length of output: 4772
Ensure Syncable trait hooks run for new sync fields
The
Syncabletrait auto-creates a sync state on model creation, but callingfirstOrCreate($data)will not invoke the customsetClientGeneratedId()or update the sync timestamp after setting a client‐generated ID. Refactor to usecreate()and explicitly call those methods:• File:
app/Http/Controllers/API/v1/CategoryController.php
• Lines: ~101–109- $category = $user->categories()->firstOrCreate($data); + $category = $user->categories()->create($data); + + // Persist client‐generated ID, if provided + if (!empty($data['client_id'])) { + $category->setClientGeneratedId($data['client_id']); + } + + // Update last_synced_at after syncing client ID + $category->markAsSynced();tests/Feature/SyncableModelsTest.php (1)
1-79: 🛠️ Refactor suggestionMissing test for Wallet model.
While tests are provided for Transaction, Category, Party, Group, and Transfer models, there's no test for the Wallet model, which based on the summary also implements the Syncable trait.
Add a test for the Wallet model:
/** @test */ public function wallet_creates_a_sync_state_on_creation() { $wallet = Wallet::factory()->create([ 'user_id' => User::factory()->create()->id, ]); $this->assertNotNull($wallet->syncState); $this->assertEquals($wallet->syncState->syncable_type, Wallet::class); $this->assertEquals($wallet->syncState->syncable_id, $wallet->id); }Make sure to add the Wallet model import at the top:
use App\Models\Wallet;
♻️ Duplicate comments (13)
app/Models/Group.php (2)
34-34: Ensure proper initialization of appended attributes.Adding
last_synced_atto the appended attributes ensures synchronization metadata is included in serialization.This seems to be the line referenced in a previous review comment: "I am sure the same issue also applies here". Without context about what the issue was, please verify that appended attributes are properly initialized throughout the model lifecycle.
55-58: Consider removing redundant accessor method.The
Syncabletrait already defines thegetLastSyncedAtAttributemethod with identical functionality. This local implementation overrides the trait's implementation unnecessarily.-public function getLastSyncedAtAttribute() -{ - return $this->syncState->last_synced_at ?? null; -}Note: This same pattern appears in other models too, so this might be intentional. If keeping this pattern for consistency, consider adding a comment explaining why.
app/Models/Transfer.php (1)
42-42: Added last_synced_at to appended attributesThe
last_synced_atattribute is now properly included in the model's JSON representation.public/docs/api.json (10)
548-552: Add examples and clarify optionalclient_id.(Apply the same suggestion for the
/groupsendpoint.)
561-563: Incorrect OpenAPIformatforcreated_at.(See above diff to use
"date-time".)
947-951: Add examples and clarify optionalclient_id.(Refer to the initial comment for
/categories.)
961-963: Incorrect OpenAPIformatforcreated_at.(See above diff to use
"date-time".)
1192-1196: Add examples and clarify optionalclient_id.(Applicable for the
/transactionscreation schema as well.)
1215-1217: Incorrect OpenAPIformatforcreated_at.(See above diff for
"date-time".)
1451-1455: Add examples and clarify optionalclient_id.(Consistent with earlier suggestions.)
1473-1476: Incorrect OpenAPIformatforcreated_at.(See above diff to use
"date-time".)
1556-1560: Add examples and clarify optionalclient_id.(Refer to the initial comment.)
1584-1586: Incorrect OpenAPIformatforcreated_at.(See above diff to use
"date-time".)
🧹 Nitpick comments (8)
database/migrations/2025_04_08_194334_create_model_sync_states_table.php (1)
14-21: Well-structured migration for synchronization state tracking.The migration creates a polymorphic relationship table that will track synchronization states across multiple model types. The design is clean and follows Laravel conventions.
Consider adding a unique compound index on
(syncable_type, syncable_id, source)to ensure each model can only have one sync state record per source:$table->morphs('syncable'); $table->string('source')->nullable(); $table->uuid('client_generated_id')->index()->nullable(); $table->timestamp('last_synced_at'); $table->timestamps(); +$table->unique(['syncable_type', 'syncable_id', 'source']);Also, you might want to set a default value for
last_synced_atto simplify record creation:-$table->timestamp('last_synced_at'); +$table->timestamp('last_synced_at')->useCurrent();app/Models/Party.php (1)
37-40: Consider removing redundant accessor method.The
Syncabletrait already defines thegetLastSyncedAtAttributemethod with identical functionality. This local implementation overrides the trait's implementation unnecessarily.-public function getLastSyncedAtAttribute() -{ - return $this->syncState->last_synced_at ?? null; -}Note: This same pattern appears in other models too, so this might be intentional. If keeping this pattern for consistency, consider adding a comment explaining why.
tests/Feature/TransactionsTest.php (1)
169-169: Minor inconsistency in datetime format.This update request uses
now()for the updated_at value, while the earlier test (line 84) uses a fixed string '2025-02-02 14:25:45'. Consider using a consistent approach for better test maintainability.-->putJson("/api/v1/transactions/{$expense['id']}", ['amount' => 200, 'updated_at' => now()]) +->putJson("/api/v1/transactions/{$expense['id']}", ['amount' => 200, 'updated_at' => '2025-02-02 14:25:45'])app/Models/Group.php (1)
36-36: Unconventional trait usage placement.The
usestatement for traits is positioned after property definitions, which is unconventional in PHP. Typically, traits are defined immediately after the class declaration and before properties.class Group extends Model { + use HasClientCreatedAt, HasFactory, Sluggable, Syncable; + /** * The attributes that are mass assignable. * * @var array<int, string> */ protected $fillable = [ 'name', 'description', 'slug', ]; protected $appends = ['last_synced_at']; - - use HasClientCreatedAt, HasFactory, Sluggable, Syncable;database/factories/TransferFactory.php (1)
21-24: Consider adding user_id to the returned attributesThe factory doesn't explicitly set the
user_idon the transfer model. While it might be handled through relationships, it would be clearer to include it in the returned attributes.return [ 'amount' => 100, 'from_wallet_id' => $fromWallet->id, 'to_wallet_id' => $toWallet->id, + 'user_id' => $user->id, ];app/Traits/HasClientCreatedAt.php (2)
10-35: Consider renaming the boot method to follow Laravel conventions.The boot method name
bootSetsCreatedAtFromClientdoesn't follow Laravel's typical convention for trait boot methods, which is usuallyboot{TraitName}. Consider renaming it tobootHasClientCreatedAtfor consistency with Laravel's conventions.- protected static function bootSetsCreatedAtFromClient() + protected static function bootHasClientCreatedAt()
15-26: Consider adding validation for future timestamps.The current implementation accepts any valid timestamp, including those in the future. Depending on your application's requirements, you might want to add validation to prevent future timestamps from being set, as this could potentially be a security or data integrity concern.
try { $parsed = Carbon::parse($input); + // Prevent future timestamps + if ($parsed->isFuture()) { + Log::warning('[Sync] Future timestamp received from client', [ + 'model' => class_basename($model), + 'input' => $input, + 'parsed' => $parsed->toDateTimeString(), + ]); + $parsed = now(); + } $model->created_at = $parsed;app/Http/Controllers/API/v1/TransactionController.php (1)
290-292: Missing conflict detection for updated_at.The controller requires updated_at but doesn't use it to detect potential sync conflicts where the client might be updating an older version of the record.
Consider adding conflict detection logic:
+ // Check for potential sync conflict + if (strtotime($validatedData['updated_at']) < strtotime($transaction->updated_at)) { + return $this->failure('Sync conflict: The record has been modified more recently', 409); + } + $transaction->update(array_filter($validatedData, fn ($value) => $value !== null)); $transaction->markAsSynced();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
app/Http/Controllers/API/v1/CategoryController.php(2 hunks)app/Http/Controllers/API/v1/GroupController.php(3 hunks)app/Http/Controllers/API/v1/PartyController.php(3 hunks)app/Http/Controllers/API/v1/TransactionController.php(7 hunks)app/Http/Controllers/API/v1/TransferController.php(2 hunks)app/Http/Controllers/API/v1/WalletController.php(2 hunks)app/Models/Category.php(4 hunks)app/Models/Group.php(3 hunks)app/Models/ModelSyncState.php(1 hunks)app/Models/Party.php(3 hunks)app/Models/Transaction.php(4 hunks)app/Models/Transfer.php(4 hunks)app/Models/Wallet.php(3 hunks)app/Traits/HasClientCreatedAt.php(1 hunks)app/Traits/Syncable.php(1 hunks)database/factories/TransactionFactory.php(1 hunks)database/factories/TransferFactory.php(1 hunks)database/migrations/2024_07_20_125117_create_transactions_table.php(0 hunks)database/migrations/2025_04_08_194334_create_model_sync_states_table.php(1 hunks)public/docs/api.json(12 hunks)tests/Feature/SyncableModelsTest.php(1 hunks)tests/Feature/TransactionsTest.php(5 hunks)
💤 Files with no reviewable changes (1)
- database/migrations/2024_07_20_125117_create_transactions_table.php
🧰 Additional context used
🧬 Code Graph Analysis (7)
database/factories/TransactionFactory.php (1)
database/factories/TransferFactory.php (1)
definition(19-30)
app/Models/Transfer.php (6)
app/Models/Group.php (1)
getLastSyncedAtAttribute(55-58)app/Models/Category.php (1)
getLastSyncedAtAttribute(57-60)app/Models/Transaction.php (1)
getLastSyncedAtAttribute(92-95)app/Models/Wallet.php (1)
getLastSyncedAtAttribute(36-39)app/Models/Party.php (1)
getLastSyncedAtAttribute(37-40)app/Traits/Syncable.php (2)
getLastSyncedAtAttribute(30-33)syncState(35-38)
database/factories/TransferFactory.php (3)
app/Models/User.php (1)
User(12-79)database/factories/TransactionFactory.php (1)
definition(17-25)app/Models/Transfer.php (1)
user(44-47)
app/Models/Wallet.php (1)
app/Traits/Syncable.php (2)
getLastSyncedAtAttribute(30-33)syncState(35-38)
app/Models/Group.php (6)
app/Models/Category.php (1)
getLastSyncedAtAttribute(57-60)app/Models/Transaction.php (1)
getLastSyncedAtAttribute(92-95)app/Models/Transfer.php (1)
getLastSyncedAtAttribute(69-72)app/Models/Wallet.php (1)
getLastSyncedAtAttribute(36-39)app/Models/Party.php (1)
getLastSyncedAtAttribute(37-40)app/Traits/Syncable.php (2)
getLastSyncedAtAttribute(30-33)syncState(35-38)
app/Models/Transaction.php (6)
app/Models/Group.php (1)
getLastSyncedAtAttribute(55-58)app/Models/Category.php (1)
getLastSyncedAtAttribute(57-60)app/Models/Transfer.php (1)
getLastSyncedAtAttribute(69-72)app/Models/Wallet.php (1)
getLastSyncedAtAttribute(36-39)app/Models/Party.php (1)
getLastSyncedAtAttribute(37-40)app/Traits/Syncable.php (2)
getLastSyncedAtAttribute(30-33)syncState(35-38)
app/Traits/Syncable.php (7)
app/Models/ModelSyncState.php (1)
ModelSyncState(8-22)app/Models/Group.php (1)
getLastSyncedAtAttribute(55-58)app/Models/Category.php (1)
getLastSyncedAtAttribute(57-60)app/Models/Transaction.php (1)
getLastSyncedAtAttribute(92-95)app/Models/Transfer.php (1)
getLastSyncedAtAttribute(69-72)app/Models/Wallet.php (1)
getLastSyncedAtAttribute(36-39)app/Models/Party.php (1)
getLastSyncedAtAttribute(37-40)
🔇 Additional comments (37)
app/Http/Controllers/API/v1/CategoryController.php (2)
61-67: OpenAPI documentation properly updated for synchronization support.The documentation now includes the optional client_id and created_at fields with appropriate descriptions and formats.
89-95: Validation rules properly updated for synchronization fields.The validation rules now accept optional client_id (UUID) and created_at (date) fields, consistent with the OpenAPI documentation.
app/Http/Controllers/API/v1/TransferController.php (1)
30-32: LGTM! API documentation updates for synchronization.The addition of
client_idandcreated_atproperties to the OpenAPI documentation provides proper API contract documentation for the sync functionality.Also applies to: 36-37
app/Models/Party.php (2)
5-6: LGTM! Added synchronization traits.Adding the
HasClientCreatedAtandSyncabletraits enables client-side creation timestamps and synchronization capabilities. This is consistent with changes across other models.Also applies to: 22-22
35-35: LGTM! Added last_synced_at to appended attributes.Adding
last_synced_atto the appended attributes ensures this synchronization metadata is included in model serialization.tests/Feature/TransactionsTest.php (2)
41-41: LGTM! Updated datetime format in tests.Updating test data to use a full datetime with time component (instead of just date) aligns with the API changes for more precise timestamp handling.
84-85: LGTM! Added updated_at timestamp to update request.Including the updated_at timestamp in the update request matches the new API requirements.
app/Models/Group.php (1)
5-6: LGTM! Added synchronization trait imports.Adding imports for
HasClientCreatedAtandSyncabletraits enables client-side creation timestamps and synchronization capabilities.app/Http/Controllers/API/v1/GroupController.php (4)
64-66: OpenAPI documentation enhanced with client_id property: LGTMThe addition of the
client_idproperty to the OpenAPI documentation properly specifies it as a UUID format with a clear description of its purpose for client-side synchronization.
76-76: OpenAPI documentation enhanced with created_at property: LGTMThe addition of the
created_atproperty to the OpenAPI documentation correctly defines it as a datetime format, which aligns with the synchronization features being implemented.
100-100: Validation rule added for client_id: LGTMThe validation rule for
client_idcorrectly enforces that it must be a valid UUID when provided, while allowing it to be nullable.
103-103: Validation rule added for created_at: LGTMThe validation rule for
created_atproperly enforces that it must be a valid date when provided, while allowing it to be nullable.app/Models/Transfer.php (3)
5-7: Added traits for client creation timestamps and synchronization: LGTMThe added traits
HasClientCreatedAtandSyncableappropriately integrate the model with the synchronization framework being implemented.
27-27: Applied necessary traits: LGTMCorrectly applied the
HasClientCreatedAtandSyncabletraits to enable client-generated creation timestamps and synchronization state management.
69-72: Implemented getter for last_synced_at attribute: LGTMThe getter properly retrieves the
last_synced_attimestamp from the relatedsyncStaterecord, falling back to null if not available. This implementation matches the pattern used in other models across the codebase.database/factories/TransferFactory.php (1)
19-30: Factory definition creates valid transfer test data: LGTMThe factory correctly:
- Gets or creates a user for the transfer
- Creates source and destination wallets with the appropriate balances
- Defines a transfer of a fixed amount between these wallets
This setup will facilitate testing of the synchronization functionality.
app/Http/Controllers/API/v1/WalletController.php (5)
66-68: OpenAPI documentation enhanced with client_id property: LGTMThe addition of the
client_idproperty to the OpenAPI documentation properly specifies it as a UUID format with a clear description of its purpose for client-side synchronization.
73-74: OpenAPI documentation enhanced with created_at property: LGTMThe addition of the
created_atproperty to the OpenAPI documentation correctly defines it as a datetime format, supporting the client-side creation timestamp functionality.
102-102: Validation rule added for client_id: LGTMThe validation rule for
client_idcorrectly enforces that it must be a valid UUID when provided, while allowing it to be nullable.
108-108: Validation rule added for created_at: LGTMThe validation rule for
created_atproperly enforces that it must be a valid date when provided, while allowing it to be nullable.
113-113: Consider the implications of firstOrCreate with client dataUsing
firstOrCreatewith client-provided data may have unintended consequences when synchronizing. If a client attempts to create a wallet with aclient_idthat already exists but with different attributes, it will silently return the existing record instead of creating a new one or updating it.Consider whether
updateOrCreatemight be more appropriate to ensure synchronization consistency, or add specific handling for conflicts.app/Models/Wallet.php (1)
5-6: Implementation of Syncable trait is complete and correctly done.The Wallet model has been updated to include synchronization capabilities with the addition of
HasClientCreatedAtandSyncabletraits, along with thelast_synced_atappended attribute and its accessor method. This resolves the previous issue where the method was missing.Also applies to: 24-24, 34-39
app/Models/Category.php (1)
5-6: Implementation of Syncable trait is complete and correctly done.The Category model has been properly updated to include synchronization capabilities with the
HasClientCreatedAtandSyncabletraits, and correctly implements thelast_synced_ataccessor method.Also applies to: 24-24, 43-43, 57-60
app/Models/Transaction.php (3)
5-6: Fillable attributes properly updated for sync capability.The Transaction model has been correctly updated to use the sync-related traits and the timestamp fields (
created_atandupdated_at) have been appropriately added to the fillable array to support client-provided timestamps.Also applies to: 35-35, 51-52
55-55: Appended attributes updated to include sync metadata.The
last_synced_atattribute has been correctly added to the$appendsarray, ensuring it will be included in serialized outputs.
92-95: Implementation of last_synced_at accessor is complete.The accessor method for
last_synced_athas been correctly implemented, resolving the previously noted issue in the comment "Here too".app/Http/Controllers/API/v1/PartyController.php (2)
21-22: Documentation properly describes the client_id field.The OpenAPI documentation correctly explains that the
client_idis a UUID format string that represents a unique identifier for the local client.
71-76: Documentation for request body properties looks good.The OpenAPI documentation properly includes the client_id and created_at fields with appropriate descriptions and formats.
tests/Feature/SyncableModelsTest.php (1)
1-79: Good test coverage for syncable models.The tests properly verify that each model creates a sync state record upon creation. Each test follows a consistent pattern of creating the model and asserting the relationship properties.
app/Traits/Syncable.php (3)
11-18: Automatic sync state creation looks good.The
bootSyncablemethod correctly hooks into the model lifecycle to create a sync state record when a model is created.
20-28: Properly handles empty client IDs.The
setClientGeneratedIdmethod appropriately logs and skips updating when given an empty ID.
40-43: Mark as synced implementation looks good.The
markAsSyncedmethod properly updates or creates the sync state with the current timestamp.app/Http/Controllers/API/v1/TransactionController.php (3)
73-80: OpenAPI documentation for client ID and timestamps looks good.The documentation properly documents the client_id and created_at fields with appropriate descriptions and formats.
128-133: Synchronization logic properly implemented.The code correctly sets the client ID (if provided) and marks the transaction as synced after creation.
228-238: Required updated_at field appropriately documented.The OpenAPI schema now correctly requires the updated_at field, which is important for sync conflict detection.
public/docs/api.json (2)
315-319: Add examples and clarify optionalclient_id.The
client_idproperty is correctly marked as non-required, but adding anexamplefield (e.g.,"example": "3fa85f64-5717-4562-b3fc-2c963f66afa6") and/or"nullable": truecould improve clarity for API consumers. Please verify that the server-side validation (e.g.,nullable|uuid) matches this schema.
1345-1345: Ensure server-side validation forupdated_atis applied.The schema now requires
updated_at. Confirm that the controller and request validation rules enforce a date-time format (e.g.,required|date_format:Y-m-d\TH:i:sP).
nfebe
force-pushed
the
feat/13/syncing-infrastructure-init
branch
from
6f2dac7 to
93b1524
Compare
- Add framework to sync models - Add code syncing logic accross existing syncable modles Signed-off-by: nfebe <fenn25.fn@gmail.com>
Signed-off-by: nfebe <fenn25.fn@gmail.com>
nfebe
force-pushed
the
feat/13/syncing-infrastructure-init
branch
from
93b1524 to
617f611
Compare
No description provided.