Skip to content

Comments

Fix stock egg UUID migration#2259

Open
Boy132 wants to merge 2 commits intomainfrom
boy132/fix-egg-uuid-migration
Open

Fix stock egg UUID migration#2259
Boy132 wants to merge 2 commits intomainfrom
boy132/fix-egg-uuid-migration

Conversation

@Boy132
Copy link
Member

@Boy132 Boy132 commented Feb 20, 2026

We messed up the UUIDs of the former stock eggs and used the former stock egg UUID instead of the UUID of the existing egg. This means the migration is wrong.

I also added an additional check to make sure the migration doesn't fail if you have both egg versions installed.

@Boy132 Boy132 self-assigned this Feb 20, 2026
@coderabbitai
Copy link

coderabbitai bot commented Feb 20, 2026

📝 Walkthrough

Walkthrough

This pull request modifies a database migration file to convert stock egg UUIDs. The migration logic is restructured to be idempotent, replacing the previous swap-on-every-iteration approach with conditional updates that check if the target UUID already exists before performing modifications.

Changes

Cohort / File(s) Summary
Database Migration
database/migrations/2026_01_14_221937_convert_stock_egg_uuids.php
Refactored migration mapping structure and loop logic to prevent duplicate UUIDs on repeated runs. Changed from unconditional UUID swaps to conditional updates that only update update_url if target UUID exists, or perform the original swap if it doesn't.

Possibly related PRs

  • PR #2108: Modifies the same migration file with the original swap logic that this PR replaces with an idempotent implementation.
🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Fix stock egg UUID migration' directly addresses the main change: correcting a UUID migration issue for stock eggs, which aligns with the PR's primary objective.
Description check ✅ Passed The description explains the problem (wrong UUIDs used in migration), the fix (correcting UUID handling), and an additional safeguard (check for both egg versions), all directly related to the changeset.

✏️ 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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
database/migrations/2026_01_14_221937_convert_stock_egg_uuids.php (1)

43-46: ⚠️ Potential issue | 🟡 Minor

Irreversible migration — down() is a no-op

Rolling back this migration will not restore the old UUIDs. This is acceptable for a production data-normalisation step, but in dev/CI environments where migrate:rollback is routinely used it can leave the eggs table in an inconsistent state relative to the seeded data. If rollback support is ever needed, the inverse mapping (new → old) is straightforward to add.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@database/migrations/2026_01_14_221937_convert_stock_egg_uuids.php` around
lines 43 - 46, The down() currently no-ops — implement it to revert the UUID
normalization by applying the inverse mapping used in up(): reconstruct the
new→old map (by swapping keys/values of the original old→new mapping used in
up()) and run an update inside a transaction to set eggs.uuid = oldUuid where
eggs.uuid = newUuid for each pair; ensure you use the same identifying symbols
(the migration class's up() mapping array and the down() method) and fail loudly
if any expected newUuid is missing so CI/dev rollbacks restore the original
seeded UUIDs.
🧹 Nitpick comments (1)
database/migrations/2026_01_14_221937_convert_stock_egg_uuids.php (1)

29-40: Consider wrapping the loop in a DB transaction

Without a transaction, a failure mid-loop (e.g., a constraint violation on the second or third entry) leaves earlier entries already committed while the rest are not, resulting in a partially-migrated state that's difficult to diagnose or re-run.

♻️ Proposed transaction wrapping
-        foreach ($mappings as $oldUuid => $newData) {
-            if (DB::table('eggs')->where('uuid', $newData['new_uuid'])->exists()) {
-                DB::table('eggs')->where('uuid', $newData['new_uuid'])->update([
-                    'update_url' => $newData['new_update_url'],
-                ]);
-            } else {
-                DB::table('eggs')->where('uuid', $oldUuid)->update([
-                    'uuid' => $newData['new_uuid'],
-                    'update_url' => $newData['new_update_url'],
-                ]);
-            }
-        }
+        DB::transaction(function () use ($mappings) {
+            foreach ($mappings as $oldUuid => $newData) {
+                if (DB::table('eggs')->where('uuid', $newData['new_uuid'])->exists()) {
+                    DB::table('eggs')->where('uuid', $newData['new_uuid'])->update([
+                        'update_url' => $newData['new_update_url'],
+                    ]);
+                } else {
+                    DB::table('eggs')->where('uuid', $oldUuid)->update([
+                        'uuid' => $newData['new_uuid'],
+                        'update_url' => $newData['new_update_url'],
+                    ]);
+                }
+            }
+        });
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@database/migrations/2026_01_14_221937_convert_stock_egg_uuids.php` around
lines 29 - 40, Wrap the foreach that iterates $mappings and updates
DB::table('eggs') in a single database transaction so all updates either commit
or rollback together; use DB::transaction (or beginTransaction/commit/rollBack)
around the loop in the migration's up() method, let exceptions bubble to trigger
rollback, and keep the same conditional logic that checks exists() before update
so a failed update will revert all prior changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@database/migrations/2026_01_14_221937_convert_stock_egg_uuids.php`:
- Around line 43-46: The down() currently no-ops — implement it to revert the
UUID normalization by applying the inverse mapping used in up(): reconstruct the
new→old map (by swapping keys/values of the original old→new mapping used in
up()) and run an update inside a transaction to set eggs.uuid = oldUuid where
eggs.uuid = newUuid for each pair; ensure you use the same identifying symbols
(the migration class's up() mapping array and the down() method) and fail loudly
if any expected newUuid is missing so CI/dev rollbacks restore the original
seeded UUIDs.

---

Nitpick comments:
In `@database/migrations/2026_01_14_221937_convert_stock_egg_uuids.php`:
- Around line 29-40: Wrap the foreach that iterates $mappings and updates
DB::table('eggs') in a single database transaction so all updates either commit
or rollback together; use DB::transaction (or beginTransaction/commit/rollBack)
around the loop in the migration's up() method, let exceptions bubble to trigger
rollback, and keep the same conditional logic that checks exists() before update
so a failed update will revert all prior changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant