feat: inventory, equipment, shop and trade system (V1)#193
Open
danielhe4rt wants to merge 1 commit into4.xfrom
Open
feat: inventory, equipment, shop and trade system (V1)#193danielhe4rt wants to merge 1 commit into4.xfrom
danielhe4rt wants to merge 1 commit into4.xfrom
Conversation
…system Implement the V1 item system with cosmetic items for character avatars. Gamification module (extended): - Item catalog: Item, ItemSlot, ItemRarity models with SlotType and AcquisitionMethod enums - Inventory: CharacterItem model with AddItemToInventory action (validates active, level, duplicates) - Equipment: CharacterEquipment model with EquipItem (auto-swap) and UnequipItem actions - 5 migrations, 5 factories, 9 Pest tests Economy module (extended): - Shop: ShopListing model with PurchaseItem action (atomic wallet debit + stock + inventory) - Trade P2P: Trade/TradeItem models with Create, Accept, Reject, Cancel actions - Full validation: ownership, tradeable, not equipped, not in pending trade - 3 migrations, 3 factories, 6 exception classes
📝 WalkthroughWalkthroughThis PR introduces comprehensive database infrastructure and business logic for inventory management, item equipment, shop listing purchases, and peer-to-peer trading functionality. It includes database migrations for items, rarities, slots, character inventory, equipment tracking, shop listings, and trade records; corresponding Eloquent models with relationship definitions; factories for test data generation; action classes implementing purchase, inventory addition, equipment management, and trade lifecycle workflows; DTOs and enums for type-safe data handling; custom exceptions for validation and error scenarios; and integration of polymorphic morph aliases in service providers. Feature tests validate key inventory and equipment operations. Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 14
🧹 Nitpick comments (7)
app-modules/gamification/database/migrations/2026_03_25_000005_create_character_equipment_table.php (1)
22-23: Consider removing the extracharacter_idindex.The composite unique index on (
character_id,slot_id) already indexescharacter_idas the leftmost prefix, so Line 23 is likely redundant write overhead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app-modules/gamification/database/migrations/2026_03_25_000005_create_character_equipment_table.php` around lines 22 - 23, Remove the redundant single-column index on character_id because the composite unique index created by $table->unique(['character_id', 'slot_id']) already provides an index on the leftmost prefix; delete or comment out the $table->index('character_id') line so only the composite unique constraint remains.app-modules/economy/database/migrations/2026_03_25_100003_create_trade_items_table.php (1)
16-16: Consider cascade behavior forcharacter_item_idforeign key.The
character_item_idFK has no cascade behavior specified. If acharacter_itemis deleted while referenced in a pending trade, this will cause a foreign key constraint violation. Consider adding->cascadeOnDelete()or->nullOnDelete()depending on desired behavior, or ensure application logic prevents deletion of items in pending trades.Suggested change
- $table->foreignUuid('character_item_id')->constrained('character_items'); + $table->foreignUuid('character_item_id')->constrained('character_items')->cascadeOnDelete();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app-modules/economy/database/migrations/2026_03_25_100003_create_trade_items_table.php` at line 16, The foreign key definition for character_item_id in the CreateTradeItemsTable migration lacks delete behavior and can cause FK violations when a character_item is removed; update the foreignUuid('character_item_id')->constrained('character_items') call to include the desired cascade rule (e.g., ->cascadeOnDelete() to delete dependent trade_items or ->nullOnDelete() to set character_item_id to null) and run or create a new migration to apply the change so the DB enforces the chosen behavior.app-modules/gamification/database/factories/CharacterEquipmentFactory.php (1)
25-29: Keep default equipment fixtures relationship-consistent.Line 25 to Line 29 can create a
character_item_idthat belongs to a different character/tenant thancharacter_id/tenant_id. This makes tests flaky and unrealistic.♻️ Suggested refactor
final class CharacterEquipmentFactory extends Factory { @@ public function definition(): array { return [ 'id' => fake()->uuid(), 'character_id' => Character::factory(), 'slot_id' => ItemSlot::factory(), 'character_item_id' => CharacterItem::factory(), 'tenant_id' => Tenant::factory(), 'equipped_at' => now(), ]; } + + public function forCharacterItem(CharacterItem $characterItem): self + { + return $this->state(fn (): array => [ + 'character_item_id' => $characterItem->id, + 'character_id' => $characterItem->character_id, + 'tenant_id' => $characterItem->tenant_id, + ]); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app-modules/gamification/database/factories/CharacterEquipmentFactory.php` around lines 25 - 29, The CharacterEquipmentFactory currently creates character_id, tenant_id, and character_item_id independently which can produce a CharacterItem that belongs to a different character/tenant; update the factory so the Character and Tenant are created first and the CharacterItem is created/associated with that same Character and Tenant (e.g., create or resolve a single Character instance and use CharacterItem::factory()->for($thatCharacter)->for($thatCharacter->tenant) or ->state tied to the character/tenant when producing character_item_id), while slot_id can remain independent; update references in CharacterEquipmentFactory to use the shared Character/Tenant instance so relationships are consistent.app-modules/gamification/tests/Feature/Character/EquipItemTest.php (1)
58-67: Consider adding test for level requirement validation.Per the
EquipItemaction implementation, there's a level check that throwsItemException::levelTooLow(). Adding a test case for this scenario would improve coverage.📝 Suggested test case
test('cannot equip item when character level is too low', function (): void { $tenant = Tenant::factory()->create(); $slot = ItemSlot::factory()->recycle($tenant)->create(); $rarity = ItemRarity::factory()->recycle($tenant)->create(); $item = Item::factory()->recycle([$tenant, $slot, $rarity])->create(['level_required' => 50]); $character = Character::factory()->recycle($tenant)->create(['experience' => 0]); $characterItem = CharacterItem::factory()->recycle([$character, $item, $tenant])->create(); $dto = new EquipItemDTO( characterId: $character->id, characterItemId: $characterItem->id, ); resolve(EquipItem::class)->handle($dto); })->throws(ItemException::class);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app-modules/gamification/tests/Feature/Character/EquipItemTest.php` around lines 58 - 67, Add a new test in EquipItemTest to cover the level requirement branch: create a Tenant, ItemSlot, ItemRarity, an Item with level_required high (e.g. 50), a low-level Character (experience 0), and a CharacterItem linking them, then call resolve(EquipItem::class)->handle($dto) and assert it throws ItemException (specifically ItemException::levelTooLow()). Target the EquipItem action and the ItemException::levelTooLow() behavior so the level-check path is exercised.app-modules/economy/src/Trade/Actions/AcceptTrade.php (2)
22-34: Consider loading trade inside transaction with lock.The trade is loaded on line 24 outside the transaction. If another process modifies the trade (e.g., cancels it) between load and transaction start, stale data could be used.
♻️ Proposed fix
public function handle(string $tradeId, string $receiverCharacterId): Trade { - $trade = Trade::query()->with('items')->findOrFail($tradeId); - - if (!$trade->isPending()) { - throw InvalidTradeException::notPending($tradeId); - } - - if ($trade->receiver_character_id !== $receiverCharacterId) { - throw InvalidTradeException::notAuthorized(); - } - - return DB::transaction(function () use ($trade): Trade { + return DB::transaction(function () use ($tradeId, $receiverCharacterId): Trade { + $trade = Trade::query() + ->with('items') + ->lockForUpdate() + ->findOrFail($tradeId); + + if (!$trade->isPending()) { + throw InvalidTradeException::notPending($tradeId); + } + + if ($trade->receiver_character_id !== $receiverCharacterId) { + throw InvalidTradeException::notAuthorized(); + } + foreach ($trade->items as $tradeItem) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app-modules/economy/src/Trade/Actions/AcceptTrade.php` around lines 22 - 34, The Trade is loaded before starting the DB transaction which can lead to race conditions; move the Trade load and validation into the DB::transaction closure and acquire a row lock (use lockForUpdate / forUpdate) when querying (e.g., replace the pre-transaction Trade::query()->with('items')->findOrFail($tradeId) with a locked query inside the closure), then perform the isPending check and receiver_character_id authorization inside the transaction before proceeding with the rest of handle (refer to handle, $trade, DB::transaction, and lockForUpdate/forUpdate).
60-67: UseAcquisitionMethodenum instead of raw string for consistency.The
CharacterItemmodel castsacquired_viatoAcquisitionMethod::class, andPurchaseItemcorrectly usesAcquisitionMethod::Purchase. Using the raw string'trade'is inconsistent with the rest of the codebase and less type-safe than using the enum case.♻️ Proposed fix
Add the import at the top of the file:
use He4rt\Gamification\Item\Enums\AcquisitionMethod;Then update the code:
CharacterItem::query() ->where('id', $tradeItem->character_item_id) ->update([ 'character_id' => $newOwner, - 'acquired_via' => 'trade', + 'acquired_via' => AcquisitionMethod::Trade, 'acquired_at' => now(), ]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app-modules/economy/src/Trade/Actions/AcceptTrade.php` around lines 60 - 67, Replace the raw string 'trade' with the AcquisitionMethod enum and add its import: update the CharacterItem update call in AcceptTrade (the block starting CharacterItem::query()->where('id', $tradeItem->character_item_id)->update([...])) to set 'acquired_via' => AcquisitionMethod::Trade and add use He4rt\Gamification\Item\Enums\AcquisitionMethod; at the top of the file so the casted model uses the enum case instead of a plain string.app-modules/gamification/src/Character/Inventory/Models/CharacterItem.php (1)
77-80: Consider checking if relation is already loaded before querying.When
equipmentis already eager-loaded,exists()still executes a query. This could cause unnecessary queries in loops.♻️ Proposed optimization
public function isEquipped(): bool { - return $this->equipment()->exists(); + return $this->relationLoaded('equipment') + ? $this->equipment !== null + : $this->equipment()->exists(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app-modules/gamification/src/Character/Inventory/Models/CharacterItem.php` around lines 77 - 80, The isEquipped() method currently always calls $this->equipment()->exists(), causing a DB query even when the equipment relation is eager-loaded; update isEquipped() to first check $this->relationLoaded('equipment') and, if loaded, determine equipped status from the loaded $this->equipment (e.g. non-null/empty), otherwise fall back to $this->equipment()->exists() so you avoid unnecessary queries in loops.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@app-modules/economy/database/migrations/2026_03_25_100001_create_shop_listings_table.php`:
- Line 15: The migration sets a foreignId 'tenant_id' with nullOnDelete() but
the column isn't nullable; update the migration so the tenant_id column is
nullable (e.g., call nullable() on the column before constrained()) so the
foreign key can be set to NULL on tenant deletion; locate the line with
$table->foreignId('tenant_id')->constrained('tenants')->nullOnDelete() in the
CreateShopListings migration and add nullable() to that chain to fix the
constraint.
In
`@app-modules/economy/database/migrations/2026_03_25_100002_create_trades_table.php`:
- Line 15: The migration uses a non-nullable foreign key for tenant_id with
nullOnDelete (e.g. the line
foreignId('tenant_id')->constrained('tenants')->nullOnDelete() in
CreateTradesTable), which will fail when the DB tries to set NULL; update this
(and the same pattern in other migrations like character_items, item_rarities,
item_slots, character_equipment, items, shop_listings) to make the tenant_id
column nullable before applying the foreign key action by inserting ->nullable()
on the column definition (i.e.
foreignId('tenant_id')->nullable()->constrained('tenants')->nullOnDelete()), and
run migrations/tests to verify constraints behave as expected.
In `@app-modules/economy/src/Shop/Actions/PurchaseItem.php`:
- Around line 80-85: Type mismatch: AddItemToInventoryDTO::characterId expects
string but $character->id is int; fix by converting the id to a string before
constructing the DTO (e.g., cast or strval) where PurchaseItem::handle calls new
AddItemToInventoryDTO with characterId: $character->id so the passed value is a
string; update the call in PurchaseItem (the new AddItemToInventoryDTO(...)
invocation) and ensure any related tests or usages still accept the string form
when passed to addItemToInventory->handle.
In `@app-modules/economy/src/Trade/Actions/AcceptTrade.php`:
- Around line 34-53: The CharacterItem lookups inside the DB::transaction
closure in AcceptTrade.php must acquire row-level locks to prevent races; change
the CharacterItem fetch (where CharacterItem::query()->findOrFail(...) is used)
to lock the row (e.g., use lockForUpdate()) and also apply lockForUpdate() (or
an appropriate for-update lock) to the CharacterEquipment existence check (the
CharacterEquipment::query()->where('character_item_id', ...)->exists() call) so
both the item row and its equipment state are locked during validation.
In `@app-modules/economy/src/Trade/Actions/CancelTrade.php`:
- Around line 18-31: The check-then-update on Trade
(Trade::query()->findOrFail($tradeId), isPending(), then update()) is vulnerable
to race conditions; make the transition atomic by loading the trade inside a DB
transaction with a row lock (e.g.,
Trade::query()->where('id',$tradeId)->lockForUpdate()->firstOrFail() inside
DB::transaction), perform the isPending() and initiator_character_id checks
while the lock is held, and then set status => TradeStatus::Cancelled and
resolved_at => now() within the same transaction so only one concurrent request
can succeed; preserve the existing InvalidTradeException throws when checks
fail.
In `@app-modules/economy/src/Trade/Actions/CreateTrade.php`:
- Around line 30-56: Validation is currently performed outside the
DB::transaction in CreateTrade (so TOCTOU can occur); move the calls to
validateItems($dto->offeredItemIds, $dto->initiatorCharacterId) and
validateItems($dto->requestedItemIds, $dto->receiverCharacterId) into the
closure passed to DB::transaction before creating the Trade and its items, and
update the validateItems implementation to perform its item queries using SELECT
... FOR UPDATE (e.g., use lockForUpdate() when loading CharacterItem rows) so
ownership/tradeable/equipped/pending-trades checks happen under the same
transaction lock and prevent concurrent modifications while you create the Trade
and call Trade::query()->create and $trade->items()->create.
In `@app-modules/economy/src/Trade/Actions/RejectTrade.php`:
- Around line 20-31: The current RejectTrade action reads $trade->isPending()
then calls $trade->update(...) which can race; make the status transition atomic
by performing the state check and update inside a DB-level atomic operation:
either run the logic inside a transaction and obtain a row lock via
$trade->lockForUpdate() before re-checking isPending(), or replace the
read-then-update with a conditional update like Trade::where('id',
$tradeId)->where('status', TradeStatus::Pending)->update([...]) and then assert
the affected rows > 0 (throw InvalidTradeException::notPending() if zero). Apply
this change in the RejectTrade handler around the isPending() check and the
update call.
In
`@app-modules/gamification/database/migrations/2026_03_25_000001_create_item_slots_table.php`:
- Line 15: The migration currently defines tenant_id with foreignId('tenant_id')
(which is NOT NULL) but uses nullOnDelete(), causing a constraint conflict;
update the migration so the tenant_id column is nullable before adding the
foreign constraint (e.g., change the definition to call nullable() on the column
prior to constrained()), ensuring tenant_id is declared nullable and still uses
nullOnDelete().
In
`@app-modules/gamification/database/migrations/2026_03_25_000002_create_item_rarities_table.php`:
- Line 15: The tenant_id foreign key in the CreateItemRarities migration is
declared non-nullable but uses nullOnDelete(), which will cause a constraint
error; update the migration so the tenant_id column is nullable before applying
nullOnDelete() (i.e., change the
$table->foreignId('tenant_id')->constrained('tenants')->nullOnDelete() statement
in the CreateItemRarities migration to declare tenant_id as nullable), mirroring
the fix used in create_item_slots_table.php so deletions can set NULL safely.
In
`@app-modules/gamification/database/migrations/2026_03_25_000003_create_items_table.php`:
- Line 15: The migration currently defines the foreign key for tenant_id with
nullOnDelete() but the column is not nullable; update the migration by making
the tenant_id column nullable before applying nullOnDelete() (i.e., add
->nullable() on the column definition that currently reads
$table->foreignId('tenant_id')->constrained('tenants')->nullOnDelete()); ensure
the tenant_id definition is adjusted so the database can set it to NULL on
tenant deletion and matches the model type (int|null).
In
`@app-modules/gamification/database/migrations/2026_03_25_000004_create_character_items_table.php`:
- Line 17: The migration declares the tenant_id foreign key with nullOnDelete()
but not nullable; update the migration so the column is nullable by adding
->nullable() to the chain on the foreignId call (i.e., change the
$table->foreignId('tenant_id')->constrained('tenants')->nullOnDelete();
statement so tenant_id is nullable before applying nullOnDelete()).
In
`@app-modules/gamification/database/migrations/2026_03_25_000005_create_character_equipment_table.php`:
- Line 18: The migration defines tenant_id as a non-nullable FK but calls
nullOnDelete(), which will fail when the DB tries to SET NULL; update the
create_character_equipment_table migration so tenant_id is nullable before
applying nullOnDelete() (i.e., ensure the column declaration uses nullable()
then constrained(...)->nullOnDelete()), and apply the same change to the other
gamification/economy migrations (items, item_rarities, character_items,
item_slots, etc.) that currently declare tenant_id with nullOnDelete() while not
marking the column nullable.
In `@app-modules/gamification/src/Character/Equipment/Actions/EquipItem.php`:
- Around line 38-50: The delete-then-create slot swap in CharacterEquipment (the
CharacterEquipment::query()->where(...)->delete() followed by
CharacterEquipment::query()->create(...)) must be wrapped in a database
transaction to ensure atomicity; update the EquipItem action to run both the
delete and create inside a DB transaction (e.g., DB::transaction or
DB::beginTransaction/commit/rollback), catch exceptions to rollback on error and
rethrow or return an error, and return the created CharacterEquipment model only
after the transaction commits so a failed create does not leave the slot empty.
In
`@app-modules/gamification/src/Character/Inventory/Actions/AddItemToInventory.php`:
- Around line 32-47: Replace the non-atomic exists() + create() pattern in
AddItemToInventory (using CharacterItem::query()) with an atomic operation like
CharacterItem::firstOrCreate() so the race condition cannot surface; call
firstOrCreate with the identifying attributes ['character_id' =>
$dto->characterId, 'item_id' => $dto->itemId] and the other fields as defaults,
then if the returned model was not newly created throw
ItemException::alreadyOwned($dto->characterId, $dto->itemId) (or update
timestamps if desired) to ensure the unique constraint is handled gracefully
instead of propagating a DB error.
---
Nitpick comments:
In
`@app-modules/economy/database/migrations/2026_03_25_100003_create_trade_items_table.php`:
- Line 16: The foreign key definition for character_item_id in the
CreateTradeItemsTable migration lacks delete behavior and can cause FK
violations when a character_item is removed; update the
foreignUuid('character_item_id')->constrained('character_items') call to include
the desired cascade rule (e.g., ->cascadeOnDelete() to delete dependent
trade_items or ->nullOnDelete() to set character_item_id to null) and run or
create a new migration to apply the change so the DB enforces the chosen
behavior.
In `@app-modules/economy/src/Trade/Actions/AcceptTrade.php`:
- Around line 22-34: The Trade is loaded before starting the DB transaction
which can lead to race conditions; move the Trade load and validation into the
DB::transaction closure and acquire a row lock (use lockForUpdate / forUpdate)
when querying (e.g., replace the pre-transaction
Trade::query()->with('items')->findOrFail($tradeId) with a locked query inside
the closure), then perform the isPending check and receiver_character_id
authorization inside the transaction before proceeding with the rest of handle
(refer to handle, $trade, DB::transaction, and lockForUpdate/forUpdate).
- Around line 60-67: Replace the raw string 'trade' with the AcquisitionMethod
enum and add its import: update the CharacterItem update call in AcceptTrade
(the block starting CharacterItem::query()->where('id',
$tradeItem->character_item_id)->update([...])) to set 'acquired_via' =>
AcquisitionMethod::Trade and add use
He4rt\Gamification\Item\Enums\AcquisitionMethod; at the top of the file so the
casted model uses the enum case instead of a plain string.
In `@app-modules/gamification/database/factories/CharacterEquipmentFactory.php`:
- Around line 25-29: The CharacterEquipmentFactory currently creates
character_id, tenant_id, and character_item_id independently which can produce a
CharacterItem that belongs to a different character/tenant; update the factory
so the Character and Tenant are created first and the CharacterItem is
created/associated with that same Character and Tenant (e.g., create or resolve
a single Character instance and use
CharacterItem::factory()->for($thatCharacter)->for($thatCharacter->tenant) or
->state tied to the character/tenant when producing character_item_id), while
slot_id can remain independent; update references in CharacterEquipmentFactory
to use the shared Character/Tenant instance so relationships are consistent.
In
`@app-modules/gamification/database/migrations/2026_03_25_000005_create_character_equipment_table.php`:
- Around line 22-23: Remove the redundant single-column index on character_id
because the composite unique index created by $table->unique(['character_id',
'slot_id']) already provides an index on the leftmost prefix; delete or comment
out the $table->index('character_id') line so only the composite unique
constraint remains.
In `@app-modules/gamification/src/Character/Inventory/Models/CharacterItem.php`:
- Around line 77-80: The isEquipped() method currently always calls
$this->equipment()->exists(), causing a DB query even when the equipment
relation is eager-loaded; update isEquipped() to first check
$this->relationLoaded('equipment') and, if loaded, determine equipped status
from the loaded $this->equipment (e.g. non-null/empty), otherwise fall back to
$this->equipment()->exists() so you avoid unnecessary queries in loops.
In `@app-modules/gamification/tests/Feature/Character/EquipItemTest.php`:
- Around line 58-67: Add a new test in EquipItemTest to cover the level
requirement branch: create a Tenant, ItemSlot, ItemRarity, an Item with
level_required high (e.g. 50), a low-level Character (experience 0), and a
CharacterItem linking them, then call resolve(EquipItem::class)->handle($dto)
and assert it throws ItemException (specifically ItemException::levelTooLow()).
Target the EquipItem action and the ItemException::levelTooLow() behavior so the
level-check path is exercised.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 531a3daa-068d-4a5f-8e45-37d0cb67db73
📒 Files selected for processing (54)
app-modules/economy/database/factories/ShopListingFactory.phpapp-modules/economy/database/factories/TradeFactory.phpapp-modules/economy/database/factories/TradeItemFactory.phpapp-modules/economy/database/migrations/2026_03_25_100001_create_shop_listings_table.phpapp-modules/economy/database/migrations/2026_03_25_100002_create_trades_table.phpapp-modules/economy/database/migrations/2026_03_25_100003_create_trade_items_table.phpapp-modules/economy/src/Providers/EconomyServiceProvider.phpapp-modules/economy/src/Shop/Actions/PurchaseItem.phpapp-modules/economy/src/Shop/DTOs/PurchaseItemDTO.phpapp-modules/economy/src/Shop/Exceptions/ItemAlreadyOwnedException.phpapp-modules/economy/src/Shop/Exceptions/ItemNotAvailableException.phpapp-modules/economy/src/Shop/Exceptions/ItemOutOfStockException.phpapp-modules/economy/src/Shop/Exceptions/LevelRequirementNotMetException.phpapp-modules/economy/src/Shop/Models/ShopListing.phpapp-modules/economy/src/Trade/Actions/AcceptTrade.phpapp-modules/economy/src/Trade/Actions/CancelTrade.phpapp-modules/economy/src/Trade/Actions/CreateTrade.phpapp-modules/economy/src/Trade/Actions/RejectTrade.phpapp-modules/economy/src/Trade/DTOs/CreateTradeDTO.phpapp-modules/economy/src/Trade/Enums/TradeDirection.phpapp-modules/economy/src/Trade/Enums/TradeStatus.phpapp-modules/economy/src/Trade/Exceptions/InvalidTradeException.phpapp-modules/economy/src/Trade/Exceptions/TradeItemNotValidException.phpapp-modules/economy/src/Trade/Models/Trade.phpapp-modules/economy/src/Trade/Models/TradeItem.phpapp-modules/gamification/database/factories/CharacterEquipmentFactory.phpapp-modules/gamification/database/factories/CharacterItemFactory.phpapp-modules/gamification/database/factories/ItemFactory.phpapp-modules/gamification/database/factories/ItemRarityFactory.phpapp-modules/gamification/database/factories/ItemSlotFactory.phpapp-modules/gamification/database/migrations/2026_03_25_000001_create_item_slots_table.phpapp-modules/gamification/database/migrations/2026_03_25_000002_create_item_rarities_table.phpapp-modules/gamification/database/migrations/2026_03_25_000003_create_items_table.phpapp-modules/gamification/database/migrations/2026_03_25_000004_create_character_items_table.phpapp-modules/gamification/database/migrations/2026_03_25_000005_create_character_equipment_table.phpapp-modules/gamification/src/Character/Equipment/Actions/EquipItem.phpapp-modules/gamification/src/Character/Equipment/Actions/UnequipItem.phpapp-modules/gamification/src/Character/Equipment/DTOs/EquipItemDTO.phpapp-modules/gamification/src/Character/Equipment/Exceptions/EquipmentException.phpapp-modules/gamification/src/Character/Equipment/Models/CharacterEquipment.phpapp-modules/gamification/src/Character/Inventory/Actions/AddItemToInventory.phpapp-modules/gamification/src/Character/Inventory/DTOs/AddItemToInventoryDTO.phpapp-modules/gamification/src/Character/Inventory/Models/CharacterItem.phpapp-modules/gamification/src/Character/Models/Character.phpapp-modules/gamification/src/Item/Enums/AcquisitionMethod.phpapp-modules/gamification/src/Item/Enums/SlotType.phpapp-modules/gamification/src/Item/Exceptions/ItemException.phpapp-modules/gamification/src/Item/Models/Item.phpapp-modules/gamification/src/Item/Models/ItemRarity.phpapp-modules/gamification/src/Item/Models/ItemSlot.phpapp-modules/gamification/src/Providers/GamificationServiceProvider.phpapp-modules/gamification/tests/Feature/Character/AddItemToInventoryTest.phpapp-modules/gamification/tests/Feature/Character/EquipItemTest.phpapp-modules/gamification/tests/Feature/Character/UnequipItemTest.php
| { | ||
| Schema::create('shop_listings', function (Blueprint $table): void { | ||
| $table->uuid('id')->primary(); | ||
| $table->foreignId('tenant_id')->constrained('tenants')->nullOnDelete(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify nullable + nullOnDelete consistency for tenant_id FK in this migration.
rg -n "foreignId\('tenant_id'\).*nullOnDelete\(\)" app-modules/economy/database/migrations/2026_03_25_100001_create_shop_listings_table.php
rg -n "foreignId\('tenant_id'\).*nullable\(\).*nullOnDelete\(\)|foreignId\('tenant_id'\).*nullOnDelete\(\).*nullable\(\)" app-modules/economy/database/migrations/2026_03_25_100001_create_shop_listings_table.phpRepository: he4rt/heartdevs.com
Length of output: 149
🏁 Script executed:
cat app-modules/economy/database/migrations/2026_03_25_100001_create_shop_listings_table.phpRepository: he4rt/heartdevs.com
Length of output: 1070
Make tenant_id nullable when using nullOnDelete().
The column is configured with nullOnDelete() but is non-nullable. When a tenant is deleted, the database constraint will fail because it cannot set the column to NULL. This causes referential integrity violations.
🔧 Proposed fix
- $table->foreignId('tenant_id')->constrained('tenants')->nullOnDelete();
+ $table->foreignId('tenant_id')->nullable()->constrained('tenants')->nullOnDelete();📝 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.
Suggested change
| $table->foreignId('tenant_id')->constrained('tenants')->nullOnDelete(); | |
| $table->foreignId('tenant_id')->nullable()->constrained('tenants')->nullOnDelete(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@app-modules/economy/database/migrations/2026_03_25_100001_create_shop_listings_table.php`
at line 15, The migration sets a foreignId 'tenant_id' with nullOnDelete() but
the column isn't nullable; update the migration so the tenant_id column is
nullable (e.g., call nullable() on the column before constrained()) so the
foreign key can be set to NULL on tenant deletion; locate the line with
$table->foreignId('tenant_id')->constrained('tenants')->nullOnDelete() in the
CreateShopListings migration and add nullable() to that chain to fix the
constraint.
| { | ||
| Schema::create('trades', function (Blueprint $table): void { | ||
| $table->uuid('id')->primary(); | ||
| $table->foreignId('tenant_id')->constrained('tenants')->nullOnDelete(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n app-modules/economy/database/migrations/2026_03_25_100002_create_trades_table.phpRepository: he4rt/heartdevs.com
Length of output: 1310
🏁 Script executed:
rg "nullOnDelete\(\)" --type phpRepository: he4rt/heartdevs.com
Length of output: 1763
🏁 Script executed:
grep -A1 "foreignId('tenant_id')" app-modules/gamification/database/migrations/2026_03_25_000004_create_character_items_table.php | head -5Repository: he4rt/heartdevs.com
Length of output: 194
🏁 Script executed:
grep -A1 "foreignId('tenant_id')" app-modules/economy/database/migrations/2026_03_25_100001_create_shop_listings_table.php | head -5Repository: he4rt/heartdevs.com
Length of output: 212
Add ->nullable() before foreign key constraint with nullOnDelete().
Using nullOnDelete() on a non-nullable column will cause database constraint violations when a tenant is deleted. The nullOnDelete() action attempts to set the column to NULL, which fails on non-nullable columns. This pattern appears systematically across multiple migrations.
- $table->foreignId('tenant_id')->constrained('tenants')->nullOnDelete();
+ $table->foreignId('tenant_id')->nullable()->constrained('tenants')->nullOnDelete();Note: This issue affects multiple tables in the codebase (character_items, item_rarities, item_slots, character_equipment, items, shop_listings, and others).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@app-modules/economy/database/migrations/2026_03_25_100002_create_trades_table.php`
at line 15, The migration uses a non-nullable foreign key for tenant_id with
nullOnDelete (e.g. the line
foreignId('tenant_id')->constrained('tenants')->nullOnDelete() in
CreateTradesTable), which will fail when the DB tries to set NULL; update this
(and the same pattern in other migrations like character_items, item_rarities,
item_slots, character_equipment, items, shop_listings) to make the tenant_id
column nullable before applying the foreign key action by inserting ->nullable()
on the column definition (i.e.
foreignId('tenant_id')->nullable()->constrained('tenants')->nullOnDelete()), and
run migrations/tests to verify constraints behave as expected.
Comment on lines
+80
to
+85
| return $this->addItemToInventory->handle(new AddItemToInventoryDTO( | ||
| characterId: $character->id, | ||
| itemId: $item->id, | ||
| tenantId: $listing->tenant_id, | ||
| acquiredVia: AcquisitionMethod::Purchase, | ||
| )); |
There was a problem hiding this comment.
Type mismatch: characterId expects string, receives int.
PHPStan confirms that AddItemToInventoryDTO constructor expects string for $characterId, but $character->id is int.
🐛 Proposed fix
return $this->addItemToInventory->handle(new AddItemToInventoryDTO(
- characterId: $character->id,
- itemId: $item->id,
+ characterId: (string) $character->id,
+ itemId: (string) $item->id,
tenantId: $listing->tenant_id,
acquiredVia: AcquisitionMethod::Purchase,
));📝 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.
Suggested change
| return $this->addItemToInventory->handle(new AddItemToInventoryDTO( | |
| characterId: $character->id, | |
| itemId: $item->id, | |
| tenantId: $listing->tenant_id, | |
| acquiredVia: AcquisitionMethod::Purchase, | |
| )); | |
| return $this->addItemToInventory->handle(new AddItemToInventoryDTO( | |
| characterId: (string) $character->id, | |
| itemId: (string) $item->id, | |
| tenantId: $listing->tenant_id, | |
| acquiredVia: AcquisitionMethod::Purchase, | |
| )); |
🧰 Tools
🪛 GitHub Check: Perform Phpstan Check / Run
[failure] 81-81:
Parameter $characterId of class He4rt\Gamification\Character\Inventory\DTOs\AddItemToInventoryDTO constructor expects string, int given.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app-modules/economy/src/Shop/Actions/PurchaseItem.php` around lines 80 - 85,
Type mismatch: AddItemToInventoryDTO::characterId expects string but
$character->id is int; fix by converting the id to a string before constructing
the DTO (e.g., cast or strval) where PurchaseItem::handle calls new
AddItemToInventoryDTO with characterId: $character->id so the passed value is a
string; update the call in PurchaseItem (the new AddItemToInventoryDTO(...)
invocation) and ensure any related tests or usages still accept the string form
when passed to addItemToInventory->handle.
Comment on lines
+34
to
+53
| return DB::transaction(function () use ($trade): Trade { | ||
| foreach ($trade->items as $tradeItem) { | ||
| $characterItem = CharacterItem::query()->findOrFail($tradeItem->character_item_id); | ||
|
|
||
| $expectedOwner = $tradeItem->direction === TradeDirection::Offer | ||
| ? $trade->initiator_character_id | ||
| : $trade->receiver_character_id; | ||
|
|
||
| if ($characterItem->character_id !== $expectedOwner) { | ||
| throw TradeItemNotValidException::noLongerValid($tradeItem->character_item_id); | ||
| } | ||
|
|
||
| $isEquipped = CharacterEquipment::query() | ||
| ->where('character_item_id', $tradeItem->character_item_id) | ||
| ->exists(); | ||
|
|
||
| if ($isEquipped) { | ||
| throw TradeItemNotValidException::currentlyEquipped($tradeItem->character_item_id); | ||
| } | ||
| } |
There was a problem hiding this comment.
Add row-level locking to prevent race conditions.
The CharacterItem queries lack lockForUpdate(), allowing concurrent processes to modify items during trade acceptance. This could result in transferring items that were equipped or traded elsewhere.
🔒 Recommended fix
foreach ($trade->items as $tradeItem) {
- $characterItem = CharacterItem::query()->findOrFail($tradeItem->character_item_id);
+ $characterItem = CharacterItem::query()
+ ->lockForUpdate()
+ ->findOrFail($tradeItem->character_item_id);📝 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.
Suggested change
| return DB::transaction(function () use ($trade): Trade { | |
| foreach ($trade->items as $tradeItem) { | |
| $characterItem = CharacterItem::query()->findOrFail($tradeItem->character_item_id); | |
| $expectedOwner = $tradeItem->direction === TradeDirection::Offer | |
| ? $trade->initiator_character_id | |
| : $trade->receiver_character_id; | |
| if ($characterItem->character_id !== $expectedOwner) { | |
| throw TradeItemNotValidException::noLongerValid($tradeItem->character_item_id); | |
| } | |
| $isEquipped = CharacterEquipment::query() | |
| ->where('character_item_id', $tradeItem->character_item_id) | |
| ->exists(); | |
| if ($isEquipped) { | |
| throw TradeItemNotValidException::currentlyEquipped($tradeItem->character_item_id); | |
| } | |
| } | |
| return DB::transaction(function () use ($trade): Trade { | |
| foreach ($trade->items as $tradeItem) { | |
| $characterItem = CharacterItem::query() | |
| ->lockForUpdate() | |
| ->findOrFail($tradeItem->character_item_id); | |
| $expectedOwner = $tradeItem->direction === TradeDirection::Offer | |
| ? $trade->initiator_character_id | |
| : $trade->receiver_character_id; | |
| if ($characterItem->character_id !== $expectedOwner) { | |
| throw TradeItemNotValidException::noLongerValid($tradeItem->character_item_id); | |
| } | |
| $isEquipped = CharacterEquipment::query() | |
| ->where('character_item_id', $tradeItem->character_item_id) | |
| ->exists(); | |
| if ($isEquipped) { | |
| throw TradeItemNotValidException::currentlyEquipped($tradeItem->character_item_id); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app-modules/economy/src/Trade/Actions/AcceptTrade.php` around lines 34 - 53,
The CharacterItem lookups inside the DB::transaction closure in AcceptTrade.php
must acquire row-level locks to prevent races; change the CharacterItem fetch
(where CharacterItem::query()->findOrFail(...) is used) to lock the row (e.g.,
use lockForUpdate()) and also apply lockForUpdate() (or an appropriate
for-update lock) to the CharacterEquipment existence check (the
CharacterEquipment::query()->where('character_item_id', ...)->exists() call) so
both the item row and its equipment state are locked during validation.
Comment on lines
+18
to
+31
| $trade = Trade::query()->findOrFail($tradeId); | ||
|
|
||
| if (!$trade->isPending()) { | ||
| throw InvalidTradeException::notPending($tradeId); | ||
| } | ||
|
|
||
| if ($trade->initiator_character_id !== $initiatorCharacterId) { | ||
| throw InvalidTradeException::notAuthorized(); | ||
| } | ||
|
|
||
| $trade->update([ | ||
| 'status' => TradeStatus::Cancelled, | ||
| 'resolved_at' => now(), | ||
| ]); |
There was a problem hiding this comment.
Make trade-state transition atomic to prevent race conditions.
Line 18 through Line 31 do a non-atomic check-then-update. Two concurrent requests can both pass isPending() and apply conflicting transitions.
🔧 Suggested fix
use He4rt\Economy\Trade\Enums\TradeStatus;
use He4rt\Economy\Trade\Exceptions\InvalidTradeException;
use He4rt\Economy\Trade\Models\Trade;
+use Illuminate\Support\Facades\DB;
@@
public function handle(string $tradeId, string $initiatorCharacterId): Trade
{
- $trade = Trade::query()->findOrFail($tradeId);
-
- if (!$trade->isPending()) {
- throw InvalidTradeException::notPending($tradeId);
- }
-
- if ($trade->initiator_character_id !== $initiatorCharacterId) {
- throw InvalidTradeException::notAuthorized();
- }
-
- $trade->update([
- 'status' => TradeStatus::Cancelled,
- 'resolved_at' => now(),
- ]);
-
- return $trade->fresh();
+ return DB::transaction(function () use ($tradeId, $initiatorCharacterId): Trade {
+ $trade = Trade::query()->lockForUpdate()->findOrFail($tradeId);
+
+ if (!$trade->isPending()) {
+ throw InvalidTradeException::notPending($tradeId);
+ }
+
+ if ($trade->initiator_character_id !== $initiatorCharacterId) {
+ throw InvalidTradeException::notAuthorized();
+ }
+
+ $trade->update([
+ 'status' => TradeStatus::Cancelled,
+ 'resolved_at' => now(),
+ ]);
+
+ return $trade->fresh() ?? $trade;
+ });
}
}📝 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.
Suggested change
| $trade = Trade::query()->findOrFail($tradeId); | |
| if (!$trade->isPending()) { | |
| throw InvalidTradeException::notPending($tradeId); | |
| } | |
| if ($trade->initiator_character_id !== $initiatorCharacterId) { | |
| throw InvalidTradeException::notAuthorized(); | |
| } | |
| $trade->update([ | |
| 'status' => TradeStatus::Cancelled, | |
| 'resolved_at' => now(), | |
| ]); | |
| use He4rt\Economy\Trade\Enums\TradeStatus; | |
| use He4rt\Economy\Trade\Exceptions\InvalidTradeException; | |
| use He4rt\Economy\Trade\Models\Trade; | |
| use Illuminate\Support\Facades\DB; | |
| public function handle(string $tradeId, string $initiatorCharacterId): Trade | |
| { | |
| return DB::transaction(function () use ($tradeId, $initiatorCharacterId): Trade { | |
| $trade = Trade::query()->lockForUpdate()->findOrFail($tradeId); | |
| if (!$trade->isPending()) { | |
| throw InvalidTradeException::notPending($tradeId); | |
| } | |
| if ($trade->initiator_character_id !== $initiatorCharacterId) { | |
| throw InvalidTradeException::notAuthorized(); | |
| } | |
| $trade->update([ | |
| 'status' => TradeStatus::Cancelled, | |
| 'resolved_at' => now(), | |
| ]); | |
| return $trade->fresh() ?? $trade; | |
| }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app-modules/economy/src/Trade/Actions/CancelTrade.php` around lines 18 - 31,
The check-then-update on Trade (Trade::query()->findOrFail($tradeId),
isPending(), then update()) is vulnerable to race conditions; make the
transition atomic by loading the trade inside a DB transaction with a row lock
(e.g., Trade::query()->where('id',$tradeId)->lockForUpdate()->firstOrFail()
inside DB::transaction), perform the isPending() and initiator_character_id
checks while the lock is held, and then set status => TradeStatus::Cancelled and
resolved_at => now() within the same transaction so only one concurrent request
can succeed; preserve the existing InvalidTradeException throws when checks
fail.
| { | ||
| Schema::create('items', function (Blueprint $table): void { | ||
| $table->uuid('id')->primary(); | ||
| $table->foreignId('tenant_id')->constrained('tenants')->nullOnDelete(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check if the file exists and read its content
fd "2026_03_25_000003_create_items_table.php" --type fRepository: he4rt/heartdevs.com
Length of output: 148
🏁 Script executed:
# If found, read the file with line numbers
cat -n "app-modules/gamification/database/migrations/2026_03_25_000003_create_items_table.php"Repository: he4rt/heartdevs.com
Length of output: 1766
🏁 Script executed:
rg "nullOnDelete" --type php -B 1 -A 1Repository: he4rt/heartdevs.com
Length of output: 4804
🏁 Script executed:
# Check a few of these files to see the full chain
sed -n '10,20p' "app-modules/identity/database/migrations/2025_11_02_172618_set_up_tenant_module.php" | head -20Repository: he4rt/heartdevs.com
Length of output: 386
🏁 Script executed:
# Let's search for nullable() used with foreignId in migration files
rg "foreignId\('tenant_id'\).*nullable" --type php -A 1Repository: he4rt/heartdevs.com
Length of output: 45
🏁 Script executed:
# Check one of the other migrations to see the exact pattern
cat -n "app-modules/events/database/migrations/2025_11_05_191403_create_events_table.php" | head -30Repository: he4rt/heartdevs.com
Length of output: 1201
🏁 Script executed:
# Let's also check if there's any macro or extension defined for foreignId
rg "macro.*foreignId|foreignId.*macro" --type phpRepository: he4rt/heartdevs.com
Length of output: 45
🏁 Script executed:
# Let's check the Laravel schema builder documentation approach
# Search for any .nullable() used with foreignId in the codebase
rg "foreignId.*nullable|nullable.*foreignId" --type php -B 1 -A 1Repository: he4rt/heartdevs.com
Length of output: 45
🏁 Script executed:
# Check if there are any comments or documentation about foreign key constraints
rg "nullOnDelete|onDelete|nullable.*constraint" --type php app-modules/ | head -30Repository: he4rt/heartdevs.com
Length of output: 2825
🏁 Script executed:
# Let's verify Laravel's foreignId behavior by checking if there's any documentation or tests
# Search for Laravel version or composer.json to understand which version is used
cat composer.json | grep -A 5 "laravel"Repository: he4rt/heartdevs.com
Length of output: 2332
🏁 Script executed:
# Also check if there are any other similar patterns that might have been correctly implemented
rg "nullable.*foreignId|foreignId.*nullable" --type php -B 1 -A 1 | head -20Repository: he4rt/heartdevs.com
Length of output: 45
🏁 Script executed:
# Let's verify the actual issue by understanding Laravel's foreignId behavior
# Check Laravel documentation or source patterns to confirm the behavior
# First, let's look for any successful uses of nullable foreign keys in the codebase
rg "foreignId.*nullable\(\)|nullable\(\).*foreignId" --type php -B 1 -A 1Repository: he4rt/heartdevs.com
Length of output: 45
🏁 Script executed:
# Check if there are any working examples of nullOnDelete with nullable in the codebase
rg "nullable.*nullOnDelete|nullOnDelete.*nullable" --type php -B 2 -A 2Repository: he4rt/heartdevs.com
Length of output: 45
🏁 Script executed:
# Check if there are any schema or model definitions that might override the migration behavior
rg "nullable|setNullable" app-modules/gamification --type php | grep -i tenantRepository: he4rt/heartdevs.com
Length of output: 45
🏁 Script executed:
# Let's also check if there are published migrations or schema definitions
find app-modules -name "*.php" -type f | xargs grep -l "items.*table" | head -5Repository: he4rt/heartdevs.com
Length of output: 331
🏁 Script executed:
# Let's check one more file to see the full context - check character_items
cat -n "app-modules/gamification/database/migrations/2026_03_25_000004_create_character_items_table.php" | head -25Repository: he4rt/heartdevs.com
Length of output: 1117
🏁 Script executed:
# Let's verify one more time by checking if there's any documented behavior in the codebase
# or if there are any model configurations that might affect this
find app-modules/gamification -name "*.php" -type f | xargs grep -l "Item" | grep -E "(Model|model)" | head -3Repository: he4rt/heartdevs.com
Length of output: 275
🏁 Script executed:
# Let's verify the models to see if there's any mention of nullable tenant_id
cat -n "app-modules/gamification/src/Character/Inventory/Models/CharacterItem.php" | head -40Repository: he4rt/heartdevs.com
Length of output: 1522
🏁 Script executed:
# This is very revealing - the model shows tenant_id as nullable. Let's check the Item model too
grep -n "tenant_id" "app-modules/gamification/src/Item/Models/Item.php" | head -10Repository: he4rt/heartdevs.com
Length of output: 122
🏁 Script executed:
# Let's check the Item model as well to confirm the pattern
cat -n "app-modules/gamification/src/Item/Models/Item.php" | head -50Repository: he4rt/heartdevs.com
Length of output: 1720
Add .nullable() to allow nullOnDelete() on tenant_id.
Line 15 uses nullOnDelete() without nullable(), which creates a constraint that cannot succeed. When a tenant is deleted, the database will attempt to set tenant_id to NULL, but the column is NOT NULL, causing a constraint violation. The model property is declared as int|null, confirming the intent for nullable columns.
🔧 Suggested fix
- $table->foreignId('tenant_id')->constrained('tenants')->nullOnDelete();
+ $table->foreignId('tenant_id')->nullable()->constrained('tenants')->nullOnDelete();📝 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.
Suggested change
| $table->foreignId('tenant_id')->constrained('tenants')->nullOnDelete(); | |
| $table->foreignId('tenant_id')->nullable()->constrained('tenants')->nullOnDelete(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@app-modules/gamification/database/migrations/2026_03_25_000003_create_items_table.php`
at line 15, The migration currently defines the foreign key for tenant_id with
nullOnDelete() but the column is not nullable; update the migration by making
the tenant_id column nullable before applying nullOnDelete() (i.e., add
->nullable() on the column definition that currently reads
$table->foreignId('tenant_id')->constrained('tenants')->nullOnDelete()); ensure
the tenant_id definition is adjusted so the database can set it to NULL on
tenant deletion and matches the model type (int|null).
| $table->uuid('id')->primary(); | ||
| $table->foreignUuid('character_id')->constrained('characters')->cascadeOnDelete(); | ||
| $table->foreignUuid('item_id')->constrained('items'); | ||
| $table->foreignId('tenant_id')->constrained('tenants')->nullOnDelete(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify nullable + nullOnDelete consistency for tenant_id FK in this migration.
rg -n "foreignId\('tenant_id'\).*nullOnDelete\(\)" app-modules/gamification/database/migrations/2026_03_25_000004_create_character_items_table.php
rg -n "foreignId\('tenant_id'\).*nullable\(\).*nullOnDelete\(\)|foreignId\('tenant_id'\).*nullOnDelete\(\).*nullable\(\)" app-modules/gamification/database/migrations/2026_03_25_000004_create_character_items_table.phpRepository: he4rt/heartdevs.com
Length of output: 149
🏁 Script executed:
cat -n app-modules/gamification/database/migrations/2026_03_25_000004_create_character_items_table.phpRepository: he4rt/heartdevs.com
Length of output: 1270
🏁 Script executed:
# Search for other nullOnDelete usages in migrations to see standard patterns
rg -n "nullOnDelete" app-modules/gamification/database/migrations/ -A 1 -B 1Repository: he4rt/heartdevs.com
Length of output: 2441
🏁 Script executed:
# Search for nullable() + constrained() + nullOnDelete pattern
rg -n "nullable\(\).*constrained.*nullOnDelete" app-modules/Repository: he4rt/heartdevs.com
Length of output: 45
🏁 Script executed:
# Search all migrations for nullable FKs to see correct pattern
rg -n "foreignId.*nullable\(\)" app-modules/gamification/database/migrations/ -A 1Repository: he4rt/heartdevs.com
Length of output: 45
🌐 Web query:
Laravel nullOnDelete() requires nullable foreign key constraint
💡 Result:
Yes, Laravel's nullOnDelete (or onDelete('set null')) requires the foreign key column to be nullable. This is a database constraint requirement: setting a foreign key to NULL on delete is impossible if the column is NOT NULL. Laravel's official documentation shows examples where nullable is applied before constrained or foreign methods, such as $table->foreignId('user_id')->nullable->constrained->nullOnDelete;. Multiple sources confirm that omitting nullable causes migration failures due to SQL constraint violations.
Citations:
- 1: https://openillumi.com/en/en-laravel-ondelete-set-null-not-working-fix/
- 2: [7.x] onDelete('set null') and nullable() ordering in migration laravel/framework#31656
- 3: https://stackoverflow.com/questions/20869072/laravel-schema-ondelete-set-null
- 4: https://laravel.com/docs/13.x/migrations
- 5: https://laravel.com/docs/master/migrations
- 6: http://laravel.com/docs/12.x/migrations
- 7: https://laravel.com/docs/8.x/migrations
Add .nullable() to align with nullOnDelete().
Line 17 uses nullOnDelete() on a non-nullable foreign key column. Since nullOnDelete() sets the column to NULL when the referenced row is deleted, the column must be nullable. This will cause a database constraint violation on tenant deletion.
🔧 Proposed fix
- $table->foreignId('tenant_id')->constrained('tenants')->nullOnDelete();
+ $table->foreignId('tenant_id')->nullable()->constrained('tenants')->nullOnDelete();📝 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.
Suggested change
| $table->foreignId('tenant_id')->constrained('tenants')->nullOnDelete(); | |
| $table->foreignId('tenant_id')->nullable()->constrained('tenants')->nullOnDelete(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@app-modules/gamification/database/migrations/2026_03_25_000004_create_character_items_table.php`
at line 17, The migration declares the tenant_id foreign key with nullOnDelete()
but not nullable; update the migration so the column is nullable by adding
->nullable() to the chain on the foreignId call (i.e., change the
$table->foreignId('tenant_id')->constrained('tenants')->nullOnDelete();
statement so tenant_id is nullable before applying nullOnDelete()).
| $table->foreignUuid('character_id')->constrained('characters')->cascadeOnDelete(); | ||
| $table->foreignId('slot_id')->constrained('item_slots'); | ||
| $table->foreignUuid('character_item_id')->constrained('character_items')->cascadeOnDelete(); | ||
| $table->foreignId('tenant_id')->constrained('tenants')->nullOnDelete(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== character_equipment migration definition =="
fd '2026_03_25_000005_create_character_equipment_table.php' -x sh -c 'echo "--- {} ---"; nl -ba "{}" | sed -n "1,120p"'
echo
echo "== tenants table id type + tenant_id FK usage across migrations =="
rg -n --type=php -C3 "Schema::create\('tenants'|\\$table->(id\\(|uuid\\('id'\\))|foreign(Id|Uuid)\\('tenant_id'\\)|nullOnDelete\\(|nullable\\(" app-modules databaseRepository: he4rt/heartdevs.com
Length of output: 368
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Reading character_equipment migration =="
cat -n app-modules/gamification/database/migrations/2026_03_25_000005_create_character_equipment_table.php
echo
echo "== Looking for tenants table definition =="
find app-modules database -name "*.php" -type f | xargs grep -l "Schema::create('tenants'" 2>/dev/null || echo "No tenants table found"
echo
echo "== Checking tenant_id foreign key patterns =="
rg -n "tenant_id" app-modules/gamification/database/migrations/2026_03_25_000005_create_character_equipment_table.php -A2 -B2Repository: he4rt/heartdevs.com
Length of output: 1865
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Tenants table definition =="
cat -n app-modules/identity/database/migrations/2025_11_02_172528_create_tenants_table.php
echo
echo "== All tenant_id foreign key patterns in codebase =="
rg -n "foreignId\('tenant_id'\)|foreignUuid\('tenant_id'\)" app-modules database --type=php -B2 -A2Repository: he4rt/heartdevs.com
Length of output: 10136
nullOnDelete() on non-nullable tenant_id column will cause runtime errors.
Line 18 uses nullOnDelete() on a non-nullable foreign key column. When a tenant is deleted, the database attempts to SET NULL, but the NOT NULL constraint prevents it, causing a constraint violation at runtime.
Required fix
- $table->foreignId('tenant_id')->constrained('tenants')->nullOnDelete();
+ $table->foreignId('tenant_id')->nullable()->constrained('tenants')->nullOnDelete();Note: This same pattern exists across multiple migrations (items, item_rarities, character_items, item_slots, etc.)—all gamification and economy tables define tenant_id with nullOnDelete() without making the column nullable.
📝 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.
Suggested change
| $table->foreignId('tenant_id')->constrained('tenants')->nullOnDelete(); | |
| $table->foreignId('tenant_id')->nullable()->constrained('tenants')->nullOnDelete(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@app-modules/gamification/database/migrations/2026_03_25_000005_create_character_equipment_table.php`
at line 18, The migration defines tenant_id as a non-nullable FK but calls
nullOnDelete(), which will fail when the DB tries to SET NULL; update the
create_character_equipment_table migration so tenant_id is nullable before
applying nullOnDelete() (i.e., ensure the column declaration uses nullable()
then constrained(...)->nullOnDelete()), and apply the same change to the other
gamification/economy migrations (items, item_rarities, character_items,
item_slots, etc.) that currently declare tenant_id with nullOnDelete() while not
marking the column nullable.
Comment on lines
+38
to
+50
| // Auto-unequip existing item in the same slot (swap) | ||
| CharacterEquipment::query() | ||
| ->where('character_id', $dto->characterId) | ||
| ->where('slot_id', $item->slot_id) | ||
| ->delete(); | ||
|
|
||
| return CharacterEquipment::query()->create([ | ||
| 'character_id' => $dto->characterId, | ||
| 'slot_id' => $item->slot_id, | ||
| 'character_item_id' => $characterItem->id, | ||
| 'tenant_id' => $characterItem->tenant_id, | ||
| 'equipped_at' => now(), | ||
| ]); |
There was a problem hiding this comment.
Wrap delete and create in a transaction for atomicity.
The slot swap operation (delete existing + create new) is not atomic. If the create() fails after delete() succeeds, the character ends up with no item equipped in that slot—an inconsistent state.
🔒 Proposed fix with transaction
+use Illuminate\Support\Facades\DB;
+
final readonly class EquipItem
{
/**
* `@throws` EquipmentException
* `@throws` ItemException
*/
public function handle(EquipItemDTO $dto): CharacterEquipment
{
$characterItem = CharacterItem::query()
->where('id', $dto->characterItemId)
->where('character_id', $dto->characterId)
->first();
if (!$characterItem) {
throw EquipmentException::itemNotInInventory($dto->characterItemId);
}
$item = $characterItem->item;
$character = Character::query()->findOrFail($dto->characterId);
if ($character->level < $item->level_required) {
throw ItemException::levelTooLow($character->level, $item->level_required);
}
- // Auto-unequip existing item in the same slot (swap)
- CharacterEquipment::query()
- ->where('character_id', $dto->characterId)
- ->where('slot_id', $item->slot_id)
- ->delete();
-
- return CharacterEquipment::query()->create([
- 'character_id' => $dto->characterId,
- 'slot_id' => $item->slot_id,
- 'character_item_id' => $characterItem->id,
- 'tenant_id' => $characterItem->tenant_id,
- 'equipped_at' => now(),
- ]);
+ return DB::transaction(function () use ($dto, $characterItem, $item) {
+ // Auto-unequip existing item in the same slot (swap)
+ CharacterEquipment::query()
+ ->where('character_id', $dto->characterId)
+ ->where('slot_id', $item->slot_id)
+ ->delete();
+
+ return CharacterEquipment::query()->create([
+ 'character_id' => $dto->characterId,
+ 'slot_id' => $item->slot_id,
+ 'character_item_id' => $characterItem->id,
+ 'tenant_id' => $characterItem->tenant_id,
+ 'equipped_at' => now(),
+ ]);
+ });
}📝 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.
Suggested change
| // Auto-unequip existing item in the same slot (swap) | |
| CharacterEquipment::query() | |
| ->where('character_id', $dto->characterId) | |
| ->where('slot_id', $item->slot_id) | |
| ->delete(); | |
| return CharacterEquipment::query()->create([ | |
| 'character_id' => $dto->characterId, | |
| 'slot_id' => $item->slot_id, | |
| 'character_item_id' => $characterItem->id, | |
| 'tenant_id' => $characterItem->tenant_id, | |
| 'equipped_at' => now(), | |
| ]); | |
| use Illuminate\Support\Facades\DB; | |
| final readonly class EquipItem | |
| { | |
| /** | |
| * `@throws` EquipmentException | |
| * `@throws` ItemException | |
| */ | |
| public function handle(EquipItemDTO $dto): CharacterEquipment | |
| { | |
| $characterItem = CharacterItem::query() | |
| ->where('id', $dto->characterItemId) | |
| ->where('character_id', $dto->characterId) | |
| ->first(); | |
| if (!$characterItem) { | |
| throw EquipmentException::itemNotInInventory($dto->characterItemId); | |
| } | |
| $item = $characterItem->item; | |
| $character = Character::query()->findOrFail($dto->characterId); | |
| if ($character->level < $item->level_required) { | |
| throw ItemException::levelTooLow($character->level, $item->level_required); | |
| } | |
| return DB::transaction(function () use ($dto, $characterItem, $item) { | |
| // Auto-unequip existing item in the same slot (swap) | |
| CharacterEquipment::query() | |
| ->where('character_id', $dto->characterId) | |
| ->where('slot_id', $item->slot_id) | |
| ->delete(); | |
| return CharacterEquipment::query()->create([ | |
| 'character_id' => $dto->characterId, | |
| 'slot_id' => $item->slot_id, | |
| 'character_item_id' => $characterItem->id, | |
| 'tenant_id' => $characterItem->tenant_id, | |
| 'equipped_at' => now(), | |
| ]); | |
| }); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app-modules/gamification/src/Character/Equipment/Actions/EquipItem.php`
around lines 38 - 50, The delete-then-create slot swap in CharacterEquipment
(the CharacterEquipment::query()->where(...)->delete() followed by
CharacterEquipment::query()->create(...)) must be wrapped in a database
transaction to ensure atomicity; update the EquipItem action to run both the
delete and create inside a DB transaction (e.g., DB::transaction or
DB::beginTransaction/commit/rollback), catch exceptions to rollback on error and
rethrow or return an error, and return the created CharacterEquipment model only
after the transaction commits so a failed create does not leave the slot empty.
Comment on lines
+32
to
+47
| $exists = CharacterItem::query() | ||
| ->where('character_id', $dto->characterId) | ||
| ->where('item_id', $dto->itemId) | ||
| ->exists(); | ||
|
|
||
| if ($exists) { | ||
| throw ItemException::alreadyOwned($dto->characterId, $dto->itemId); | ||
| } | ||
|
|
||
| return CharacterItem::query()->create([ | ||
| 'character_id' => $dto->characterId, | ||
| 'item_id' => $dto->itemId, | ||
| 'tenant_id' => $dto->tenantId, | ||
| 'acquired_via' => $dto->acquiredVia, | ||
| 'acquired_at' => now(), | ||
| ]); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for unique constraint in character_items migration
fd "create_character_items" --type f --exec cat {}Repository: he4rt/heartdevs.com
Length of output: 1046
Race condition in duplicate ownership check lacks proper exception handling.
The exists() check followed by create() is not atomic. Although a unique constraint on (character_id, item_id) prevents actual duplicates, a race condition between the check and insert could cause a database constraint violation exception instead of the expected ItemException::alreadyOwned().
Use firstOrCreate() to handle this atomically and gracefully:
Proposed fix using firstOrCreate
return CharacterItem::query()->firstOrCreate(
[
'character_id' => $dto->characterId,
'item_id' => $dto->itemId,
],
[
'tenant_id' => $dto->tenantId,
'acquired_via' => $dto->acquiredVia,
'acquired_at' => now(),
]
);If the item already exists, either update timestamps or throw ItemException::alreadyOwned() after checking the result.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@app-modules/gamification/src/Character/Inventory/Actions/AddItemToInventory.php`
around lines 32 - 47, Replace the non-atomic exists() + create() pattern in
AddItemToInventory (using CharacterItem::query()) with an atomic operation like
CharacterItem::firstOrCreate() so the race condition cannot surface; call
firstOrCreate with the identifying attributes ['character_id' =>
$dto->characterId, 'item_id' => $dto->itemId] and the other fields as defaults,
then if the returned model was not newly created throw
ItemException::alreadyOwned($dto->characterId, $dto->itemId) (or update
timestamps if desired) to ensure the unique constraint is handled gracefully
instead of propagating a DB error.
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.
Summary
Item,ItemSlot,ItemRaritymodels withSlotTypeandAcquisitionMethodenums — extensão do módulogamificationCharacterItemeCharacterEquipmentcomo subdomínios deCharactercom actions de equip/unequip (auto-swap de slot)ShopListingcomPurchaseItemaction (débito atômico de wallet + controle de estoque) — extensão do móduloeconomyTrade/TradeItemcom 4 actions (Create, Accept, Reject, Cancel) com validação completa de ownership, tradeable, equipped status e pending tradesArchitecture
Files (54 new, 3 modified)
Test plan
Summary by CodeRabbit
New Features
Tests