Skip to content

refactor: drop compound databaseId:tableId resourceId#176

Open
abnegate wants to merge 3 commits into
mainfrom
feat/drop-compound-resource-id
Open

refactor: drop compound databaseId:tableId resourceId#176
abnegate wants to merge 3 commits into
mainfrom
feat/drop-compound-resource-id

Conversation

@abnegate

@abnegate abnegate commented May 1, 2026

Copy link
Copy Markdown
Member

Summary

  • CSV/JSON sources and destinations now take string \$databaseId, string \$tableId directly. The colon-splitting on \$resourceId is gone.
  • Target::run / Source::run / Destination::run / Transfer::run gain a new optional \$rootResourceChildId parameter. For database roots, this scopes the transfer to a specific table within the root database. \$rootResourceId keeps its existing semantics (always matches \$rootResourceType).
  • Sources/Appwrite no longer inspects `rootResourceId` for ":" or splits it; uses `$this->rootResourceChildId` for per-table filtering.

Test plan

  • composer lint passes locally
  • composer check (PHPStan level 3) passes locally
  • CSV/JSON unit tests updated to pass separate IDs

Linked

🤖 Generated with Claude Code

The CSV/JSON sources and destinations took a single composite
"databaseId:tableId" string and split it on ':' internally. The Appwrite
source did the same trick on `rootResourceId` to scope a transfer to a
specific collection within a database. That mixed two identifiers in
one slot, broke queryability for callers, and forced consumers to
recompose the colon-form at the boundary.

This change replaces the compound with explicit fields:

- CSV/JSON Source/Destination constructors now take `string $databaseId,
  string $tableId` directly. No more `explode(':', $resourceId)`.
- Target/Source/Destination/Transfer::run gain a new optional
  `$rootResourceChildId` parameter that, for database roots, scopes the
  transfer to a specific table/collection inside the root database.
  `$rootResourceId` keeps its existing semantics — it always matches
  `$rootResourceType` (a top-level resource).
- Sources/Appwrite no longer inspects `rootResourceId` for ':' or
  splits it; it reads `$this->rootResourceChildId` when the caller wants
  per-collection filtering.

Tests updated to call the new constructor signatures.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@greptile-apps

greptile-apps Bot commented May 1, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR replaces the implicit databaseId:tableId compound string with explicit, typed $databaseId / $tableId constructor parameters in the CSV and JSON adapters, and introduces a first-class $rootResourceChildId parameter on the run() pipeline to scope transfers to a specific table within a root database — removing all str_contains/explode(':') logic from Sources/Appwrite.php.

  • CSV/JSON sources and destinations now accept separate $databaseId and $tableId constructor arguments, eliminating the fragile colon-split that would panic at runtime if a caller omitted the delimiter.
  • Target::run / Source::run / Destination::run / Transfer::run gain an optional $rootResourceChildId parameter (default ''), fully backward-compatible; all signatures match the abstract declaration.
  • Sources/Appwrite table-filter logic is simplified to use $this->rootResourceChildId directly instead of parsing $rootResourceId.

Confidence Score: 5/5

All changed code paths are backward-compatible additions or straightforward string-parsing removals; no logic was dropped that wasn't replaced by equivalent, cleaner code.

The refactor is mechanically consistent across every layer of the pipeline: abstract signature, concrete overrides, forwarding calls, and tests all align. The previous abstract-signature mismatch flagged in an earlier review is confirmed fixed. No pre-existing colon-split edge case was dropped without replacement, and the $rootResourceChildId default of '' keeps all existing callers working without change.

No files require special attention.

Important Files Changed

Filename Overview
src/Migration/Target.php Adds $rootResourceChildId protected property and updates the abstract run() signature with matching parameter order across all overrides.
src/Migration/Source.php Adds $rootResourceChildId parameter to run() and assigns it to the new property; signature matches the abstract declaration.
src/Migration/Destination.php Adds $rootResourceChildId parameter and forwards it to the source's run() call; signatures are consistent.
src/Migration/Transfer.php Adds nullable $rootResourceChildId parameter (null-coalesced to '' before forwarding), consistent with the existing $rootResourceType pattern.
src/Migration/Destinations/Appwrite.php Adds $rootResourceChildId to the overriding run() and passes it to parent::run(); no logic changes.
src/Migration/Destinations/CSV.php Replaces single $resourceId constructor param with explicit $databaseId + $tableId; error messages and warning context updated accordingly.
src/Migration/Destinations/JSON.php Same split-param refactor as CSV destination; consistent error messaging.
src/Migration/Sources/Appwrite.php Removes all str_contains/explode logic on $rootResourceId; uses $rootResourceChildId for per-table filtering in exportEntities.
src/Migration/Sources/CSV.php Replaces $resourceId with $databaseId + $tableId; eliminates the explode(':') call that would panic on inputs without a colon.
src/Migration/Sources/JSON.php Same split-param refactor as CSV source; exportRows now builds Database/Table objects directly from the typed properties.
tests/Migration/Unit/General/CSVTest.php All six test instantiations of TestCSV updated to pass separate 'test_db' and 'test_table_id' arguments.
tests/Migration/Unit/General/JSONTest.php Both DestinationJSON constructor calls updated to pass separate database and table IDs.

Reviews (3): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile

Comment thread src/Migration/Target.php Outdated
The abstract Target::run() omitted $rootResourceType, so its 4th
positional parameter was $rootResourceChildId while both concrete
overrides (Source::run, Destination::run) take $rootResourceType
there. Callers typed against Target passing four positional args
would silently route the child ID into the resource type. All three
signatures now match exactly.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings June 10, 2026 05:12

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

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