Skip to content

fix: ignore unrequested row/document types in status counters#193

Merged
abnegate merged 1 commit into
mainfrom
fix-status-counters-stray-rows
Jun 4, 2026
Merged

fix: ignore unrequested row/document types in status counters#193
abnegate merged 1 commit into
mainfrom
fix-status-counters-stray-rows

Conversation

@abnegate
Copy link
Copy Markdown
Member

@abnegate abnegate commented Jun 4, 2026

Problem

Transfer::getStatusCounters() seeds a status bucket only for requested resource types, and the non-row branch correctly guards with if (isset($status[$resource->getName()])) so it ignores resource types that weren't requested.

The row/document branch had no such guard. Row and document counts are aggregated into the cache by status (cache['row'][$status] = "count"), and that cache can contain a row/document entry for a type that was not part of the migration request (e.g. a stray/skipped document the destination still tallied). When that happens, the branch:

  1. auto-vivifies $status[$resourceType][$k],
  2. reads $status[$resourceType]['pending'] on a bucket that was never seeded → Undefined array key "pending" warning, and
  3. leaves a phantom, non-zero entry that survives the "remove all empty resources" prune.

The result is a non-empty statusCounters after an otherwise clean migration. Downstream (Appwrite server-ce / cloud) this surfaced as a flaky assertEmpty($statusCounters) in the Appwrite→Appwrite migration e2e test, failing intermittently across unrelated PRs.

Fix

Add the same isset($status[$resourceType]) guard the non-row branch already uses, so unrequested row/document counts are ignored consistently. No behaviour change for requested row/document types.

Test

TransferTest::testStatusCountersIgnoreUnrequestedRowCounts adds a row count to the cache for a type that was not requested and asserts getStatusCounters() ignores it and returns [].

  • Without the fix: fails with Failed asserting that an array does not have the key 'row' plus the Undefined array key "pending" warning.
  • With the fix: passes.

Verified locally: full Unit suite green, composer lint (Pint) clean, PHPStan clean on the changed files.

🤖 Generated with Claude Code

getStatusCounters() seeds a status bucket only for requested resource
types and guards the non-row branch with isset(). The row/document branch
had no such guard: a row/document count aggregated into the cache for a
type that was not requested would auto-vivify $status[$type], read an
unseeded 'pending' key (emitting an "Undefined array key" warning) and
leave a phantom, non-empty entry that survives the empty-bucket prune.

Downstream this surfaced as a flaky assertEmpty($statusCounters) after an
otherwise clean migration. Add the same isset() guard the non-row branch
already uses so unrequested row/document counts are ignored.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings June 4, 2026 04:03
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes a consistency bug in Transfer::getStatusCounters() where aggregated row/document status counts present in the cache could incorrectly “auto-vivify” counters for unrequested resource types, leading to undefined-array-key warnings and phantom non-empty statusCounters.

Changes:

  • Add an isset($status[$resourceType]) guard for the row/document aggregated-count branch to ignore unrequested types (matching the existing guard for non-row resources).
  • Add a unit test that reproduces the scenario by injecting a row status count into the cache without requesting row migration and asserting the counters remain empty.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
src/Migration/Transfer.php Adds a guard to ignore aggregated row/document cache entries when the resource type wasn’t requested, preventing undefined key warnings and phantom counters.
tests/Migration/Unit/General/TransferTest.php Adds a regression test ensuring unrequested row counts in the cache are ignored by getStatusCounters().

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Jun 4, 2026

Greptile Summary

This PR fixes a bug in Transfer::getStatusCounters() where the row/document string-counter branch lacked the isset() guard that the non-row branch already had, causing phantom status entries and an "Undefined array key 'pending'" warning whenever the cache held counts for a resource type that was never part of the migration request.

  • src/Migration/Transfer.php: Adds if (!isset($status[$resourceType])) { continue; } before the row/document string-counter processing block, mirroring the guard on the non-row branch.
  • tests/Migration/Unit/General/TransferTest.php: Adds a regression test that injects a row count into the cache for an unrequested type (via Cache::add(), which stores Row objects as string counters) and asserts getStatusCounters() returns [].

Confidence Score: 5/5

Safe to merge — the change is a minimal, targeted guard with no impact on requested resource types.

The fix is a single-line guard that exactly mirrors the pre-existing pattern used in the adjacent non-row branch. The new test directly exercises the bug-triggering code path (via Cache::add(), which stores Row objects as string counters) and asserts the correct outcome. No existing behaviour for requested resource types is affected.

No files require special attention.

Important Files Changed

Filename Overview
src/Migration/Transfer.php Adds an isset($status[$resourceType]) guard to the row/document string-counter branch in getStatusCounters(), preventing phantom entries and an "Undefined array key" warning when the cache holds counts for resource types that were never requested.
tests/Migration/Unit/General/TransferTest.php Adds testStatusCountersIgnoreUnrequestedRowCounts, which exercises the exact bug path: adding a Row to the cache (stored as a string counter by Cache::add()) without requesting any resource types, then asserting getStatusCounters() returns [].

Reviews (1): Last reviewed commit: "fix: ignore unrequested row/document typ..." | Re-trigger Greptile

@abnegate abnegate merged commit 7638a3a into main Jun 4, 2026
5 checks passed
@abnegate abnegate deleted the fix-status-counters-stray-rows branch June 4, 2026 04:17
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.

2 participants