Skip to content

Add overwrite and skip options to Appwrite destination#168

Open
premtsd-code wants to merge 6 commits intomainfrom
csv-import-upsert
Open

Add overwrite and skip options to Appwrite destination#168
premtsd-code wants to merge 6 commits intomainfrom
csv-import-upsert

Conversation

@premtsd-code
Copy link
Copy Markdown
Contributor

Summary

  • Adds setOverwrite() and setSkip() methods to the Appwrite destination
  • When overwrite is true, uses upsertDocuments() to update existing rows with matching IDs
  • When skip is true, passes ignore: true to createDocuments() to silently skip duplicates
  • Points utopia-php/database to csv-import-upsert branch which adds the ignore parameter to createDocuments

Depends on utopia-php/database#850

Test plan

  • CSV import with skip: true — duplicate rows silently skipped
  • CSV import with overwrite: true — existing rows updated
  • CSV import with neither — duplicate rows throw error
  • JSON import with same scenarios

- Add setOverwrite() / setSkip() methods to Appwrite destination
- When overwrite is true, use upsertDocuments to update existing rows
- When skip is true, pass ignore: true to createDocuments to silently
  skip duplicates
- Point utopia-php/database to csv-import-upsert branch for ignore support
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Apr 8, 2026

Greptile Summary

This PR adds setOverwrite() and setSkip() methods to the Appwrite destination, routing bulk row inserts through upsertDocuments or skipDuplicates respectively, and fixes the cache-bypass gap for the overwrite path. The concerns raised in the previous thread (cache short-circuit, mutual-exclusion validation) have been addressed in this revision.

Confidence Score: 4/5

Safe to merge once the upstream utopia-php/database PR is merged and the dependency is pinned to a stable release; code logic is otherwise sound.

All P1 concerns from the prior review thread are resolved. The one remaining new finding is P2 (unnecessary skipDuplicates wrapping on the default path). Score is 4 rather than 5 because the dev-branch dependency means the PR is not yet ready to merge as-is.

composer.json / composer.lock — dev-branch dependency on utopia-php/database must be resolved before this can ship.

Important Files Changed

Filename Overview
src/Migration/Destinations/Appwrite.php Adds setOverwrite/setSkip methods with mutual-exclusion guards, fixes the cache bypass for overwrite, and branches createRecord into upsertDocuments vs skipDuplicates; the default path now passes through skipDuplicates(..., false) rather than calling createDocuments directly.
composer.json Pins utopia-php/database to the unmerged dev branch dev-csv-import-upsert-v2 — noted in prior review thread.
composer.lock Lock file reflects the dev-branch switch: utopia-php/database points to the csv-import-upsert-v2 branch, utopia-php/framework is dropped as a transitive dep in favour of utopia-php/console, and the stability flag for database is set to 20 (dev).

Reviews (5): Last reviewed commit: "Use skipDuplicates() scope guard instead..." | Re-trigger Greptile

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Apr 8, 2026

Tip:

Greploops — Automatically fix all review issues by running /greploops in Claude Code. It iterates: fix, push, re-review, repeat until 5/5 confidence.

Use the Greptile plugin for Claude Code to query reviews, search comments, and manage custom context directly from your terminal.

Comment on lines +1087 to +1097
if ($this->overwrite) {
$dbForDatabases->skipRelationshipsExistCheck(fn () => $dbForDatabases->upsertDocuments(
$collectionName,
$this->rowBuffer
));
} else {
$dbForDatabases->skipRelationshipsExistCheck(fn () => $dbForDatabases->createDocuments(
$collectionName,
$this->rowBuffer,
ignore: $this->skip
));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P1 Successful upserts not reflected in resource status

When overwrite is true, upsertDocuments is called but the return value is silently discarded and the method unconditionally returns true (line 1106). If the upsert fails for any reason (other than throwing an exception), the caller has no way to know — and even in the success case, there is no cache update after a successful upsert. A successfully upserted row won't be added to the cache, so a re-run would attempt to upsert it again instead of treating it as already migrated.

The same gap exists for the skip path: rows silently skipped at the database level (because ignore: true suppresses the duplicate error) are returned as true — indistinguishable from a genuine insert — meaning the status report will show them as successfully migrated when they were actually dropped.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Cache is already updated at the import() level (line 368: $this->cache->update($responseResource)) for every resource regardless of the code path in createRecord(), so re-runs will see the row as cached.

For the skip path: returning true is the intended behavior — skip means "do not fail on duplicates, keep going." The migration completed successfully from the caller's perspective.

The bool $ignore parameter on Database::createDocuments() was replaced
with a skipDuplicates(callable, bool) scope guard in utopia-php/database
csv-import-upsert-v2. The previous named-argument call would throw
"Unknown named argument" at runtime against the current database branch.

Wrap createDocuments in skipDuplicates() so the orchestrator picks up
the flag through its instance state instead of via parameter.
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