Skip to content

Fix composite primary auto-fill for preInsert hook fields and add regression test#8550

Merged
pubkey merged 2 commits into
masterfrom
copilot/add-test-for-setting-name-and-number
May 19, 2026
Merged

Fix composite primary auto-fill for preInsert hook fields and add regression test#8550
pubkey merged 2 commits into
masterfrom
copilot/add-test-for-setting-name-and-number

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented May 19, 2026

This PR fixes a composite primary key insertion path where id should be derived automatically when a preInsert hook sets only name and number.

Problem scope

  • Composite primary schema uses id composed from name and number.
  • Inserting {} with a preInsert hook that populates those fields should succeed and materialize id from the composite key.
  • Previous behavior threw DOC18 before hook-derived fields could be used for composite primary key generation.

Changes in this PR

  • Added a focused regression test in test/unit/hooks.test.ts under describeParallel('issues'):

    • Defines an inline composite-primary schema.
    • Uses a preInsert hook that sets:
      • docData.name = 'alice'
      • docData.number = 5
    • Verifies insert succeeds and id === 'alice|5'.
  • Implemented fix in insert flow:

    • In src/rx-collection.ts, when preInsert hooks exist, document preparation now skips composite primary fill before hooks.
    • After hooks run, composite primary key is computed from the modified document data.
    • This allows hook-populated composite fields to be used for id generation.
  • Updated helper API in src/rx-collection-helper.ts:

    • fillObjectDataBeforeInsert() now accepts an optional skipPrimaryKeyFill flag (default false) to support the hook-first flow.

Changelog

  • orga/changelog/add-test-composite-primary-preinsert-hook.md
  • orga/changelog/fix-composite-primary-preinsert-hook.md
collection.preInsert((docData) => {
  docData.name = 'alice';
  docData.number = 5;
}, false);

const doc = await collection.insert({} as any);
assert.strictEqual(doc.id, 'alice|5');

@pubkey
Copy link
Copy Markdown
Owner

pubkey commented May 19, 2026

rel #8527

@pubkey
Copy link
Copy Markdown
Owner

pubkey commented May 19, 2026

@copilot propose a fix

Copy link
Copy Markdown
Contributor Author

Copilot AI commented May 19, 2026

@copilot propose a fix

Implemented in 541bc57. I updated the insert path so preInsert runs before composite primary key filling, then id is computed afterwards. The reproduction test now passes (name + number set in hook without manually setting id).

Copilot AI changed the title Add regression test for composite primary auto-fill from preInsert hook fields Fix composite primary auto-fill for preInsert hook fields and add regression test May 19, 2026
Copilot finished work on behalf of pubkey May 19, 2026 10:42
Copilot AI requested a review from pubkey May 19, 2026 10:42
@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 2833 lines)

  idle-queue.test.js
    integration
      �[32m✓ �[39mshould be able to call queue on database
      �[32m✓ �[39minserts should always be faster than idle-call

  reactivity.test.ts
    RxDocument
      �[32m✓ �[39mRxDocument.$$
      �[32m✓ �[39mRxDocument.get$$()
      �[32m✓ �[39mRxDocument.deleted$$
      �[32m✓ �[39mRxDocument[proxy]$$
    RxLocalDocument
      �[32m✓ �[39mRxLocalDocument.$$
      �[32m✓ �[39mRxLocalDocument.get$$()
      �[32m✓ �[39mRxLocalDocument.deleted$$
    RxQuery
      �[32m✓ �[39mRxQuery.find().$$
      �[32m✓ �[39mRxQuery.findOne().$$
    preact-signals.test.ts
      �[32m✓ �[39mshould get the signal and clean up correctly
    issues
      �[32m✓ �[39mRxLocalDocument.get$() should not emit spurious values on nested object paths
      �[32m✓ �[39mRxLocalDocument.get$$() should not emit spurious values on nested object paths

  reactive-collection.test.js
    .insert()
      positive
        �[32m✓ �[39mshould get a valid event on insert
    .bulkInsert()
      positive
        �[32m✓ �[39mshould fire on bulk insert
    .bulkRemove()
      positive
        �[32m✓ �[39mshould fire on bulk remove
    .remove()
      positive
        �[32m✓ �[39mshould fire on remove
    .insert$
      �[32m✓ �[39mshould only emit inserts
    .update$
      �[32m✓ �[39mshould only emit updates
    .remove$
      �[32m✓ �[39mshould only emit removes

  reactive-document.test.js
    .save()
      positive
        �[32m✓ �[39mshould fire on save
        �[32m✓ �[39mshould observe a single field
        �[32m✓ �[39mshould observe a nested field
        �[32m✓ �[39mget equal values when subscribing again later
      negative
        �[32m✓ �[39mcannot observe non-existent field
    .deleted$
      �[32m✓ �[39mshould not emit when deleted state has not changed
      positive
        �[32m✓ �[39mdeleted$ is true, on delete
    .$
      �[32m✓ �[39mshould emit a RxDocument, not only the document data
    .get$()
      negative
        �[32m✓ �[39mprimary cannot be observed
        �[32m✓ �[39mfinal fields cannot be observed
    issues
      �[32m✓ �[39m#4546 - should return the same valueObj for the same objPath
      �[32m✓ �[39mget$() on nested object path should not emit when unrelated field changes

  cleanup.test.js
    basics
      �[32m✓ �[39mshould clean up the deleted documents
      �[32m✓ �[39mshould work by manually calling RxCollection.cleanup()
    cleanup and replication
      �[32m✓ �[39mshould pause the cleanup when a replication is not in sync
      �[32m✓ �[39mshould also run a cleanup on the replication state meta data
    issues
      �[32m✓ �[39mminimumDeletedTime not respected
      �[32m✓ �[39mshould correctly loop cleanup when storage cleanup returns false (batched cleanup)
      �[32m✓ �[39mcleanup() should return true as stated by the TypeScript return type
      �[32m✓ �[39mfields with umlauts and emojis could break the state after cleanup in some storages

  hooks.test.js
    get/set
      �[32m✓ �[39mshould set a hook
      �[32m✓ �[39mshould get a hook
      �[32m✓ �[39mshould get a parallel hook
    insert
      pre
        positive
          �[32m✓ �[39mseries
          �[32m✓ �[39mparallel
          �[32m✓ �[39mshould save a modified document
          �[32m✓ �[39masync: should save a modified document
          �[32m✓ �[39mshould not insert if hook throws
          �[32m✓ �[39mshould have the collection bound to the this-scope
      post
        positive
          �[32m✓ �[39mseries
          �[32m✓ �[39mparallel
          �[32m✓ �[39mshould call post insert hook after bulkInsert
    save
      pre
        positive
          �[32m✓ �[39mseries
          �[32m✓ �[39mparallel
          �[32m✓ �[39mshould save a modified document
          �[32m✓ �[39masync: should save a modified document
          �[32m✓ �[39mshould not save if hook throws
      post
        positive
          �[32m✓ �[39mseries
          �[32m✓ �[39mparallel
          �[32m✓ �[39mshould receive the RxDocument instance as second argument
    remove
      pre
        positive
          �[32m✓ �[39mseries
          �[32m✓ �[39mparallel
          �[32m✓ �[39mshould not remove if hook throws
          �[32m✓ �[39mshould call pre remove hook before bulkRemove
          �[32m✓ �[39mshould keep the field value that was added by the hook
      post
        positive
          �[32m✓ �[39mseries
          �[32m✓ �[39mparallel
          �[32m✓ �[39mshould have the collection bound to the this-scope
          �[32m✓ �[39mshould call post remove hook after bulkRemove
    postCreate
      positive
        �[32m✓ �[39mshould define a getter
      negative
        �[32m✓ �[39mshould throw when adding an async-hook
    issues
      �[32m✓ �[39mISSUE #158 : Throwing error in async preInsert does not prevent insert
      �[31m✗ �[39m�[31mshould auto-generate composite primary key when set in preInsert hook�[39m
	RxError (DOC18): 
	
	Error message: Document property for composed primary key is missing
	Error code: DOC18
	Cause: A field required for the composite primary key is missing in the document data.
	Fix: Ensure all fields of the composite primary key are set.
	Docs: https://rxdb.info/rx-schema.html?console=errors&code=DOC18#composite-primary-key
	Find out more about this error here: https://rxdb.info/errors.html?console=errors#DOC18 
	
	--------------------
	Parameters:
	args: {
	  "field": "name",
	  "documentData": {}
	}
	
	    at newRxError (webpack://rxdb/./dist/esm/rx-error.js?:121:10)
	    at eval (webpack://rxdb/./dist/esm/rx-schema-helper.js?:128:69)
	    at Array.map (<anonymous>)
	    at getComposedPrimaryKeyOfDocumentData (webpack://rxdb/./dist/esm/rx-schema-helper.js?:125:34)
	    at fillPrimaryKey (webpack://rxdb/./dist/esm/rx-schema-helper.js?:89:20)
	    at fillObjectDataBeforeInsert (webpack://rxdb/./dist/esm/rx-collection-helper.js?:35:80)
	    at eval (webpack://rxdb/./dist/esm/rx-collection.js?:254:111)
	    at Array.map (<anonymous>)
	    at RxCollectionBase.bulkInsert (webpack://rxdb/./dist/esm/rx-collection.js?:253:47)
	    at RxCollectionBase.insert (webpack://rxdb/./dist/esm/rx-collection.js?:211:34)
	    at Context.eval (webpack://rxdb/./test_tmp/unit/hooks.test.js?:582:34)


Chrome Headless 148.0.0.0 (Linux 0.0.0): Executed 880 of 1295�[31m (1 FAILED)�[39m (24.323 secs / 24.003 secs)
�[31mTOTAL: 1 FAILED, 879 SUCCESS�[39m


�[31m1) should auto-generate composite primary key when set in preInsert hook
�[39m�[31m     hooks.test.js issues
�[39m�[90m     RxError (DOC18): 

Error message: Document property for composed primary key is missing
Error code: DOC18
Cause: A field required for the composite primary key is missing in the document data.
Fix: Ensure all fields of the composite primary key are set.
Docs: https://rxdb.info/rx-schema.html?console=errors&code=DOC18#composite-primary-key
Find out more about this error here: https://rxdb.info/errors.html?console=errors#DOC18 

--------------------
Parameters:
args: {
  "field": "name",
  "documentData": {}
}

    at newRxError (webpack://rxdb/./dist/esm/rx-error.js?:121:10)
    at eval (webpack://rxdb/./dist/esm/rx-schema-helper.js?:128:69)
    at Array.map (<anonymous>)
    at getComposedPrimaryKeyOfDocumentData (webpack://rxdb/./dist/esm/rx-schema-helper.js?:125:34)
    at fillPrimaryKey (webpack://rxdb/./dist/esm/rx-schema-helper.js?:89:20)
    at fillObjectDataBeforeInsert (webpack://rxdb/./dist/esm/rx-collection-helper.js?:35:80)
    at eval (webpack://rxdb/./dist/esm/rx-collection.js?:254:111)
    at Array.map (<anonymous>)
    at RxCollectionBase.bulkInsert (webpack://rxdb/./dist/esm/rx-collection.js?:253:47)
    at RxCollectionBase.insert (webpack://rxdb/./dist/esm/rx-collection.js?:211:34)
    at Context.eval (webpack://rxdb/./test_tmp/unit/hooks.test.js?:582:34)
�[39m



View full workflow run

@pubkey pubkey marked this pull request as ready for review May 19, 2026 11:41
@pubkey pubkey merged commit a58b09e into master May 19, 2026
23 of 24 checks passed
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