Skip to content

Fix _attachments not forwarded across chained migration strategies#8399

Open
pubkey wants to merge 1 commit intomasterfrom
claude/fix-migration-bug-OdRYd
Open

Fix _attachments not forwarded across chained migration strategies#8399
pubkey wants to merge 1 commit intomasterfrom
claude/fix-migration-bug-OdRYd

Conversation

@pubkey
Copy link
Copy Markdown
Owner

@pubkey pubkey commented Apr 19, 2026

This PR contains:

  • A BUGFIX
  • IMPROVED TESTS

Describe the problem you have without this PR

When using multiple chained migration strategies in schema migrations, if an intermediate strategy returns a new object without forwarding the _attachments field, subsequent strategies would receive the document with _attachments as undefined. This breaks the documented public API contract where strategies receive documents typed as WithAttachments<DocData> and can mutate oldDoc._attachments.

Solution

Modified the migration pipeline in migrateDocumentData() to track attachments across chained strategies. Each strategy now receives the document with _attachments properly attached, even when a previous strategy returned a fresh object without forwarding it. If a strategy explicitly sets _attachments, that value becomes the new baseline for subsequent strategies.

The fix moves attachment restoration from the final step to each intermediate step in the promise chain, ensuring every strategy sees the correct document shape.

Changes

  • src/plugins/migration-schema/migration-helpers.ts: Track currentAttachments across strategy invocations and restore them after each strategy executes (unless the strategy explicitly provided new attachments)
  • test/unit/migration-schema.test.ts: Added regression test verifying that _attachments are preserved when strategy 1 returns a new object without forwarding attachments, and strategy 2 can still access and verify them

Test Plan

Added comprehensive regression test that:

  1. Creates a v0 database with a document containing an attachment
  2. Reopens with v2 schema using two migration strategies where strategy 1 returns a new object without _attachments
  3. Verifies strategy 2 receives _attachments and can detect the attachment
  4. Confirms the attachment data is preserved after migration

Existing tests continue to pass.

https://claude.ai/code/session_01UiLzK67sLGyehJis5YfBYb

When a migration strategy returned a new object without forwarding the
`_attachments` field, subsequent strategies received the document with
`_attachments` as `undefined`. This violated the public
`WithAttachments<DocData>` contract and prevented strategies from reading
or mutating attachment metadata as described in the docs example.

`migrateDocumentData()` now tracks the last-known `_attachments` value
across strategy invocations and re-attaches it before the next strategy
runs, while still honoring explicit rewrites (e.g. `_attachments = {}`).

https://claude.ai/code/session_01UiLzK67sLGyehJis5YfBYb
@github-actions
Copy link
Copy Markdown
Contributor

✅ Verify Test Reproduction: Tests FAILED without the fix (expected)

This confirms the changed tests correctly reproduce the bug that the source changes fix.

This workflow runs the changed tests without the source fix to verify they reproduce the bug.

Show output
...(truncated, showing last 200 of 3489 lines)
        �[32m✓ �[39mshould have exactly one master iydcxby - uemttre
    basic CRUD
      �[32m✓ �[39mshould stream changes over the replication to other collections

  encryption.test.ts
    init
      �[32m✓ �[39mcreate storage
    basics
      .encryptString()
        �[32m✓ �[39mstring
      .decryptString()
        �[32m✓ �[39mstring
        �[32m✓ �[39mshould encrypt and decrypt an extremely long string
        �[32m✓ �[39mshould encrypt and decrypt an extremely long password
    RxDatabase creation
      �[32m✓ �[39mshould crash with invalid password (empty object)
      �[32m✓ �[39mBUG: should have stored the password hash when creating the database
      �[32m✓ �[39mprevent 2 instances with different passwords on same adapter
    RxCollection creation
      �[32m✓ �[39mcreate encrypted collection
    RxCollection.insert()
      �[32m✓ �[39mshould insert one encrypted value (string)
      �[32m✓ �[39mshould insert one encrypted value (object)
      �[32m✓ �[39m#5624 insert with really big encrypted string
    RxDocument.save()
      �[32m✓ �[39mshould save one encrypted value (string)
      �[32m✓ �[39mshould save one encrypted value (object)
    replication
      �[32m✓ �[39mreplication state meta should not contain a secret in cleartext
    ISSUES
      �[32m✓ �[39m#837 Recover from wrong database password
      �[32m✓ �[39m#917 Unexpected end of JSON input
      �[32m✓ �[39mshould work with encrypted fields that have maxLength in schema
      �[32m✓ �[39mshould work with encrypted object fields that have nested properties
      �[32m✓ �[39mshould correctly encrypt and decrypt nested fields with dot-notation paths and non-string types
      �[32m✓ �[39mshould throw when encrypted contains overlapping parent and child paths
      #157 Cannot sort on field(s) "XXX" when using the default index
        �[32m✓ �[39mschema example 1
        �[32m✓ �[39mschema example 2
    SECURITY
      �[32m✓ �[39mshould not expose the database password as an enumerable property
      �[32m✓ �[39mshould not leak the password in error parameters when password is too short

  rx-state.test.ts (useSchemaValidator: true)
    helper
      .nextRxStateId()
        �[32m✓ �[39mshould increment in sort order
    creation
      �[32m✓ �[39mcalling addState twice should give the same instance
    write state data
      �[32m✓ �[39mshould write without error
      �[32m✓ �[39mwrite multiple times at once
      �[32m✓ �[39mupdate nested
      �[32m✓ �[39mupdate complete state at once
      �[32m✓ �[39mdoing many writes should end up in a single persistence write to the storage
    .get()
      �[32m✓ �[39mshould get root state
      �[32m✓ �[39mshould get the updated value
      �[32m✓ �[39mshould get nested values
      �[32m✓ �[39mshould not throw on undefined values
    .get$()
      �[32m✓ �[39mshould emit the correct data
      �[32m✓ �[39mshould emit the correct data via proxy-getter a$
    .get$$()
      �[32m✓ �[39mshould get the correct object
    cleanup
      �[32m✓ �[39mshould merge the state documents data on cleanup
    multiInstance
      �[32m✓ �[39mwrite with two states at once
      �[32m✓ �[39mwrite with two states to nested at once
      �[32m✓ �[39mget changes from other state
      �[32m✓ �[39mshould have a deterministic output when 2 instances write at the same time
      �[32m✓ �[39mshould have a deterministic output when 2 instances write to different fields
      �[32m✓ �[39mshould recover the same state from disc on the other side
      �[32m✓ �[39mshould emit the correct data for all states
    issues
      �[32m✓ �[39mRxState.property$ should be stable for initial synchronous get and subsequent subscription
      �[32m✓ �[39mset() with empty path should pass the current state to the modifier
      �[32m✓ �[39mshould recover correct state from disk after full-state replacement with set('')
      �[32m✓ �[39mbad rx-state after cleanup
      �[32m✓ �[39m$ observable should not emit duplicate values on a single write
      �[32m✓ �[39mget$() should emit the current value when subscribed, not a stale value from creation time
      �[32m✓ �[39m_cleanup() should return true to signal completion

  rx-state.test.ts (useSchemaValidator: false)
    helper
      .nextRxStateId()
        �[32m✓ �[39mshould increment in sort order
    creation
      �[32m✓ �[39mcalling addState twice should give the same instance
    write state data
      �[32m✓ �[39mshould write without error
      �[32m✓ �[39mwrite multiple times at once
      �[32m✓ �[39mupdate nested
      �[32m✓ �[39mupdate complete state at once
      �[32m✓ �[39mdoing many writes should end up in a single persistence write to the storage
    .get()
      �[32m✓ �[39mshould get root state
      �[32m✓ �[39mshould get the updated value
      �[32m✓ �[39mshould get nested values
      �[32m✓ �[39mshould not throw on undefined values
    .get$()
      �[32m✓ �[39mshould emit the correct data
      �[32m✓ �[39mshould emit the correct data via proxy-getter a$
    .get$$()
      �[32m✓ �[39mshould get the correct object
    cleanup
      �[32m✓ �[39mshould merge the state documents data on cleanup
    multiInstance
      �[32m✓ �[39mwrite with two states at once
      �[32m✓ �[39mwrite with two states to nested at once
      �[32m✓ �[39mget changes from other state
      �[32m✓ �[39mshould have a deterministic output when 2 instances write at the same time
      �[32m✓ �[39mshould have a deterministic output when 2 instances write to different fields
      �[32m✓ �[39mshould recover the same state from disc on the other side
      �[32m✓ �[39mshould emit the correct data for all states
    issues
      �[32m✓ �[39mRxState.property$ should be stable for initial synchronous get and subsequent subscription
      �[32m✓ �[39mset() with empty path should pass the current state to the modifier
      �[32m✓ �[39mshould recover correct state from disk after full-state replacement with set('')
      �[32m✓ �[39mbad rx-state after cleanup
      �[32m✓ �[39m$ observable should not emit duplicate values on a single write
      �[32m✓ �[39mget$() should emit the current value when subscribed, not a stale value from creation time
      �[32m✓ �[39m_cleanup() should return true to signal completion

  migration-schema.test.ts
    .create() with migrationStrategies
      positive
        �[32m✓ �[39mok to create with strategies
        �[32m✓ �[39mcreate same collection with different schema-versions
      negative
        �[32m✓ �[39mshould throw when array
        �[32m✓ �[39mshould throw when property no number
        �[32m✓ �[39mshould throw when property no non-float-number
        �[32m✓ �[39mshould throw when property-value no function
        �[32m✓ �[39mthrow when strategy missing
    getOldCollectionMeta()
      �[32m✓ �[39mshould NOT get an older version
      �[32m✓ �[39mshould get an older version
    migration basics
      .remove()
        �[32m✓ �[39mshould delete the old storage instance with all its content
      .migrate()
        �[32m✓ �[39mshould resolve finished when no docs
        �[32m✓ �[39mshould resolve finished when some docs are in the collection
        �[32m✓ �[39mshould emit status updates
        �[32m✓ �[39mshould remove the document when migration-strategy returns null
        �[32m✓ �[39mshould throw when document cannot be migrated
      .migratePromise()
        positive
          �[32m✓ �[39mshould resolve when nothing to migrate
          �[32m✓ �[39mshould resolve when migrating data
        negative
          �[32m✓ �[39mshould reject when migration fails
    integration into collection
      run
        �[32m✓ �[39mshould auto-run on creation
        �[32m✓ �[39mshould be able to change the primary key during migration
        �[32m✓ �[39mshould auto-run on creation (async)
        �[32m✓ �[39mshould keep the _meta.lwt value of the documents
      .migrationNeeded()
        �[32m✓ �[39mreturn true if schema-version is 0
        �[32m✓ �[39mreturn false if nothing to migrate
        �[32m✓ �[39mreturn true if something to migrate
    RxDatabase.migrationStates()
      �[32m✓ �[39mshould emit the ongoing migration state
    migration and replication
      �[32m✓ �[39mshould have migrated the replication state
      �[32m✓ �[39missue #7204 schema migration failing when returning null
    issues
      �[32m✓ �[39m#7226 db.addCollections fails after it failed for a missing migration strategy
      �[32m✓ �[39mduplicate meta states found if migration is canceled in between and restarted with a new version
      �[32m✓ �[39m#7791 should delete old collection meta doc even when _rev has changed
      �[32m✓ �[39m#7786 should properly cancel internal replication after migratePromise completes
      �[32m✓ �[39m#7008 migrate schema with multiple connected storages
      �[32m✓ �[39m#212 migration runs into infinity-loop
      �[32m✓ �[39m#3460 migrate attachments
      �[32m✓ �[39mopening an older RxDB database state with a new major version should throw an error
      �[32m✓ �[39mversion data should be compatible with next version code
      �[32m✓ �[39mmigratePromise() should return 100 percent when collection has 0 documents
      �[32m✓ �[39mshould not resurrect deleted documents when migration strategy returns a new object
      �[32m✓ �[39mmigratePromise() should return percent 100 when status is DONE
      �[32m✓ �[39mshould NOT auto-apply schema default values during migration
      �[32m✓ �[39mshould keep attachments when migration strategy returns a new object
      �[31m✗ �[39m�[31mshould preserve _attachments across chained migration strategies�[39m
	AssertionError [ERR_ASSERTION]: strategy 2 must see _attachments even when strategy 1 returned a new object
	    at Context.eval (webpack://rxdb/./test_tmp/unit/migration-schema.test.js?:2126:43)


Chrome Headless 147.0.0.0 (Linux 0.0.0): Executed 1113 of 1281�[31m (1 FAILED)�[39m (45.465 secs / 44.94 secs)
�[31mTOTAL: 1 FAILED, 1112 SUCCESS�[39m


�[31m1) should preserve _attachments across chained migration strategies
�[39m�[31m     migration-schema.test.ts issues
�[39m�[90m     AssertionError [ERR_ASSERTION]: strategy 2 must see _attachments even when strategy 1 returned a new object
    at Context.eval (webpack://rxdb/./test_tmp/unit/migration-schema.test.js?:2126:43)
�[39m



View full workflow run

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