Skip to content

Conversation

@ItzNotABug
Copy link
Member

@ItzNotABug ItzNotABug commented Nov 25, 2025

Summary by CodeRabbit

  • New Features
    • Enhanced debug logging for migrations with per-project tracking and tagged output.
    • Detailed per-table export instrumentation: start/end markers, timing, batch counts, and memory usage.
    • Improved progress tracing across export operations with iteration counts and row-level reporting.

✏️ Tip: You can customize this high-level summary in your review settings.

@ItzNotABug ItzNotABug self-assigned this Nov 25, 2025
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 25, 2025

Walkthrough

Adds project-scoped debug logging to the Appwrite migration source and its Database reader. Introduces a public static array debugProjects and a private logDebugTrackedProject(string $message): void on the Appwrite source; instruments exportRecords with detailed per-table and iteration logs (timestamps, row counts, memory, response sizes, before/after callbacks). Updates the Database reader: adds an optional ?string $projectId constructor parameter and a private logDebug(string $message): void, and emits logs before/after listRows calls.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Confirm constructor signature change for Database (+ ?string $projectId) is safe for all callers or handled with defaulting.
  • Review logging gating: ensure Appwrite::$debugProjects checks and tag selection are correct.
  • Inspect instrumentation in exportRecords for correct placement, minimal side-effects, and accurate metrics (iterationCount, memory, sizes).
  • Verify imports and visibility (public static property, private methods) follow project conventions.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
Title check ❓ Inconclusive The title 'Add: logs' is vague and generic, using non-descriptive language that doesn't convey meaningful information about what logging functionality is being added. Consider a more specific title that indicates the scope, such as 'Add debug logging for Appwrite project migrations' or 'Add project-scoped debug logging to migration reader' to better reflect the actual changes.
✅ Passed checks (1 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch log-projects

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
Contributor

@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.

Actionable comments posted: 1

🧹 Nitpick comments (3)
src/Migration/Sources/Appwrite.php (2)

13-13: Hard‑coded debug project map and new Console dependency

The Utopia\CLI\Console import plus public static array $debugProjects look intentionally tailored to a couple of specific projects. That’s fine for targeted troubleshooting, but it does bake environment‑specific state into the main source:

  • If this is meant to be long‑term, consider moving the mapping to configuration (env, CLI flag, or injected array) so you don’t need a code change to add/remove projects.
  • If this is short‑term debugging, it might be safer to keep it clearly marked as such (e.g. a comment noting it should be cleaned up once incidents are resolved).

The public visibility is required for DatabaseReader to access it, but reinforces that this becomes part of the public surface of Appwrite. If you want to keep it semi‑internal, a dedicated accessor (e.g. Appwrite::getDebugTagForProject(string $id): ?string) would decouple the reader from the concrete static property.

Also applies to: 78-82


992-1127: Verbose per‑record logging in exportRecords is well‑gated but very chatty

The new logging in exportRecords() (entity start/end, per‑table start/end, per‑iteration memory usage, timestamps around listRows(), and before/after callback()) is implemented safely:

  • It doesn’t affect control flow or data transformation.
  • It’s all routed through logDebugTrackedProject(), so only projects present in $debugProjects pay the cost.

Two considerations:

  • For the tracked projects, this can generate very large log volumes on big tables (every iteration, every batch). Make sure your log sink/rotation can handle that or consider coarse‑graining (e.g. log every Nth iteration, or only when memory crosses thresholds).
  • If you expect to reuse this style of instrumentation later, it might be worth extracting the message formatting into a tiny helper (or at least centralizing the prefixes) to reduce duplication and make it easier to tweak the format.

Functionally, the loop logic and pagination behavior remain intact.

src/Migration/Sources/Appwrite/Reader/Database.php (1)

5-6: Project‑aware debug logging in reader + CI/phpstan implications

The changes here (new imports, extended constructor, and logDebug() helper) cleanly introduce project‑scoped logging:

  • ?string $projectId is optional and readonly, so existing call sites that don’t pass it remain valid.
  • logDebug() is a no‑op unless projectId is non‑null and present in Appwrite::$debugProjects, so there’s effectively zero runtime overhead for non‑tracked projects.
  • Using Appwrite::$debugProjects keeps tags consistent with the source side.

Things to watch:

  • Tight coupling to Appwrite: Database now depends on Utopia\Migration\Sources\Appwrite purely for debug metadata. If you ever want to reuse this reader in another context, it may be nicer to inject a generic callable or an interface for lookup instead of referencing the source class directly.
  • phpstan “unknown class Console”: As with Appwrite.php, CI reports Call to static method log() on an unknown class Utopia\CLI\Console. This will keep blocking the pipeline until phpstan is taught about the utopia-php/cli package or these lines are stubbed/ignored in analysis.

Overall, the constructor change and helper are backward‑compatible and safe; the only blocker is static analysis configuration around Console.

Also applies to: 19-20, 32-38, 40-55

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 18bd7d3 and e2dfc95.

📒 Files selected for processing (2)
  • src/Migration/Sources/Appwrite.php (7 hunks)
  • src/Migration/Sources/Appwrite/Reader/Database.php (4 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-07-30T12:06:02.331Z
Learnt from: abnegate
Repo: utopia-php/migration PR: 0
File: :0-0
Timestamp: 2025-07-30T12:06:02.331Z
Learning: In the utopia-php/migration codebase, the Utopia Database package does not have a Memory adapter. When testing classes that require a Database instance (like CSV), use PHPUnit's createMock() method to create proper mocks instead of trying to instantiate real database adapters.

Applied to files:

  • src/Migration/Sources/Appwrite.php
  • src/Migration/Sources/Appwrite/Reader/Database.php
📚 Learning: 2025-06-28T09:47:58.757Z
Learnt from: ItzNotABug
Repo: utopia-php/migration PR: 80
File: src/Migration/Sources/Supabase.php:300-308
Timestamp: 2025-06-28T09:47:58.757Z
Learning: In the utopia-php/migration codebase, during the terminology swap from Collection/Attribute/Document to Table/Column/Row, the user ItzNotABug prefers to keep the existing query logic unchanged even if it becomes semantically incorrect with the new naming. The focus is purely on resource type renaming, not on fixing logical issues that become apparent after the terminology change.

Applied to files:

  • src/Migration/Sources/Appwrite/Reader/Database.php
📚 Learning: 2025-06-28T09:47:08.333Z
Learnt from: ItzNotABug
Repo: utopia-php/migration PR: 80
File: src/Migration/Sources/Appwrite.php:843-851
Timestamp: 2025-06-28T09:47:08.333Z
Learning: In the utopia-php/migration codebase, during the terminology swap from Collection/Attribute/Document to Table/Column/Row, the class constructors and method parameters use the new terminology (like "relatedTable"), but the underlying data structures and API responses still use the legacy keys (like "relatedCollection"). This is an intentional design pattern to allow gradual migration while maintaining compatibility with existing data sources.

Applied to files:

  • src/Migration/Sources/Appwrite/Reader/Database.php
📚 Learning: 2025-06-28T09:45:36.026Z
Learnt from: ItzNotABug
Repo: utopia-php/migration PR: 80
File: src/Migration/Resources/Database/Row.php:60-60
Timestamp: 2025-06-28T09:45:36.026Z
Learning: In the utopia-php/migration codebase, the `fromArray` method is not used on Row objects, so mismatches between `jsonSerialize()` output keys and `fromArray()` input expectations for Row class are not problematic.

Applied to files:

  • src/Migration/Sources/Appwrite/Reader/Database.php
🧬 Code graph analysis (2)
src/Migration/Sources/Appwrite.php (2)
src/Migration/Sources/Appwrite/Reader/Database.php (1)
  • listRows (334-387)
src/Migration/Sources/Appwrite/Reader/API.php (1)
  • listRows (179-187)
src/Migration/Sources/Appwrite/Reader/Database.php (1)
src/Migration/Sources/Appwrite.php (1)
  • Appwrite (73-1968)
🪛 GitHub Actions: CodeQL
src/Migration/Sources/Appwrite.php

[error] 151-151: Call to static method info() on an unknown class Utopia\CLI\Console. (phpstan)

src/Migration/Sources/Appwrite/Reader/Database.php

[error] 54-54: Call to static method log() on an unknown class Utopia\CLI\Console. (phpstan)

🔇 Additional comments (1)
src/Migration/Sources/Appwrite/Reader/Database.php (1)

334-387: Additional logging around listRows() is safe and consistent

The new calls:

$this->logDebug("BEFORE dbInstance->find() | TableID: $tableId | Queries: " . count($queries));
$rows = $dbInstance->find($tableId, $queries);
$this->logDebug("AFTER dbInstance->find() | TableID: $tableId | Rows returned: " . count($rows));

are:

  • Correctly placed around the actual find() call.
  • Helpful for correlating source‑side export logs with reader‑side query behavior.
  • Fully gated by logDebug(), so they’re effectively free outside tracked projects.

No functional behavior changes are introduced; the existing error handling and row mapping logic are preserved.

Also applies to: 369-373

@abnegate abnegate merged commit f84a064 into main Nov 25, 2025
3 checks passed
@abnegate abnegate deleted the log-projects branch November 25, 2025 10:26
Copy link
Contributor

@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.

Actionable comments posted: 1

🧹 Nitpick comments (1)
src/Migration/Sources/Appwrite.php (1)

992-1127: Comprehensive instrumentation with minor performance consideration.

The extensive logging provides excellent visibility into the export process with well-placed checkpoints around critical operations (listRows(), callback()). The inclusion of memory usage, timestamps, and iteration counts will be valuable for debugging.

Since this logging is opt-in (only for projects in $debugProjects), the performance impact is limited to explicitly tracked migrations. Just ensure that $debugProjects remains a small set in production environments to avoid I/O overhead from frequent logging.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e2dfc95 and e8994b9.

📒 Files selected for processing (2)
  • src/Migration/Sources/Appwrite.php (7 hunks)
  • src/Migration/Sources/Appwrite/Reader/Database.php (4 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-07-30T12:06:02.331Z
Learnt from: abnegate
Repo: utopia-php/migration PR: 0
File: :0-0
Timestamp: 2025-07-30T12:06:02.331Z
Learning: In the utopia-php/migration codebase, the Utopia Database package does not have a Memory adapter. When testing classes that require a Database instance (like CSV), use PHPUnit's createMock() method to create proper mocks instead of trying to instantiate real database adapters.

Applied to files:

  • src/Migration/Sources/Appwrite.php
  • src/Migration/Sources/Appwrite/Reader/Database.php
📚 Learning: 2025-06-28T09:47:58.757Z
Learnt from: ItzNotABug
Repo: utopia-php/migration PR: 80
File: src/Migration/Sources/Supabase.php:300-308
Timestamp: 2025-06-28T09:47:58.757Z
Learning: In the utopia-php/migration codebase, during the terminology swap from Collection/Attribute/Document to Table/Column/Row, the user ItzNotABug prefers to keep the existing query logic unchanged even if it becomes semantically incorrect with the new naming. The focus is purely on resource type renaming, not on fixing logical issues that become apparent after the terminology change.

Applied to files:

  • src/Migration/Sources/Appwrite/Reader/Database.php
📚 Learning: 2025-06-28T09:47:08.333Z
Learnt from: ItzNotABug
Repo: utopia-php/migration PR: 80
File: src/Migration/Sources/Appwrite.php:843-851
Timestamp: 2025-06-28T09:47:08.333Z
Learning: In the utopia-php/migration codebase, during the terminology swap from Collection/Attribute/Document to Table/Column/Row, the class constructors and method parameters use the new terminology (like "relatedTable"), but the underlying data structures and API responses still use the legacy keys (like "relatedCollection"). This is an intentional design pattern to allow gradual migration while maintaining compatibility with existing data sources.

Applied to files:

  • src/Migration/Sources/Appwrite/Reader/Database.php
📚 Learning: 2025-06-28T09:45:36.026Z
Learnt from: ItzNotABug
Repo: utopia-php/migration PR: 80
File: src/Migration/Resources/Database/Row.php:60-60
Timestamp: 2025-06-28T09:45:36.026Z
Learning: In the utopia-php/migration codebase, the `fromArray` method is not used on Row objects, so mismatches between `jsonSerialize()` output keys and `fromArray()` input expectations for Row class are not problematic.

Applied to files:

  • src/Migration/Sources/Appwrite/Reader/Database.php
🧬 Code graph analysis (1)
src/Migration/Sources/Appwrite.php (2)
src/Migration/Sources/Appwrite/Reader/Database.php (1)
  • listRows (334-387)
src/Migration/Sources/Appwrite/Reader/API.php (1)
  • listRows (179-187)
🔇 Additional comments (7)
src/Migration/Sources/Appwrite/Reader/Database.php (4)

5-5: LGTM!

The imports are correctly added to support the new logging functionality.

Also applies to: 19-19


32-38: LGTM!

The optional $projectId parameter is well-designed with appropriate visibility and immutability constraints, maintaining backward compatibility.


40-55: LGTM!

The logging implementation is clean with efficient early returns. The consistent use of Console::info() across both files addresses the concern raised in previous reviews about method consistency.


369-372: LGTM!

The before/after instrumentation around dbInstance->find() provides valuable debugging visibility with appropriate context (table ID, query count, row count).

src/Migration/Sources/Appwrite.php (3)

13-13: LGTM!

The Console import is correctly added to support the new logging functionality.


128-132: LGTM!

The DatabaseReader construction correctly passes the project ID to enable project-scoped logging, maintaining consistency with the updated reader interface.


141-152: LGTM!

The logging helper is well-implemented with efficient early returns and consistent Console method usage. The distinct tag prefix (MIGRATIONS-SOURCE- vs MIGRATIONS-READER-) helps differentiate log sources.

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.

3 participants