Skip to content

(Fix) Upsert no longer rewrites position on existing records#21375

Open
ijreilly wants to merge 4 commits into
mainfrom
fix--do-not-update-position-each-time-we-upsert
Open

(Fix) Upsert no longer rewrites position on existing records#21375
ijreilly wants to merge 4 commits into
mainfrom
fix--do-not-update-position-each-time-we-upsert

Conversation

@ijreilly

@ijreilly ijreilly commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

Fix: upsert no longer rewrites position on existing records

Problem

createX(..., upsert: true) resets the position of records that resolve to an update, even when the payload doesn't include a position.

The create-many/upsert runner backfills position (to "first") in computeArgs over the whole batch, before records are split into insert vs update. So existing rows get a freshly recomputed position written on every upsert. For callers that re-upsert their full dataset on a schedule (e.g. a daily sync), this rewrites position for every record on each run and drifts the values steadily negative — and it floods audit/event logs with position churn.

The dedicated updateOne/updateMany runners already pass shouldBackfillPositionIfUndefined: false; the upsert path did not.

Fix

Only backfill position for records that are actually inserted:

  • computeArgs now passes shouldBackfillPositionIfUndefined: !args.upsert in both the create-many and create-one runners, so undefined positions are left untouched on upsert.
  • performUpsertOperation backfills "first" positions for recordsToInsert only, after categorization, via RecordPositionService.

Explicit position values ("first", "last", or a number) in the payload are still honored. Plain (non-upsert) create behavior is unchanged.

Behavior

Scenario Before After
Upsert updates existing row, no position sent position rewritten position untouched
Upsert inserts new row, no position sent gets "first" gets "first" (unchanged)
Explicit position on upsert applied applied
Plain create unchanged unchanged

@twenty-ci-bot-public

Copy link
Copy Markdown

👋 Thanks for contributing to Twenty!

Your PR has been set to draft while you work on it. Once you're done, mark it as Ready for review and our automated checks will run.

Looking forward to your contribution!

@cubic-dev-ai cubic-dev-ai Bot left a comment

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.

No issues found across 3 files

Re-trigger cubic

Copilot AI left a comment

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.

Pull request overview

This PR fixes an upsert-side effect where createX(..., upsert: true) was unintentionally recomputing and rewriting position for records that resolve to updates (when the payload omitted position). The change aligns upsert behavior with existing update runners by only backfilling position for newly inserted rows.

Changes:

  • Disable position backfill during argument processing when upsert: true (create-one and create-many paths).
  • Backfill "first" positions only for recordsToInsert after upsert categorization, via RecordPositionService.
  • Add integration tests covering “upsert update without position does not change position” and “upsert insert assigns a position”.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
packages/twenty-server/test/integration/graphql/suites/upsert/upsert.integration-spec.ts Adds integration coverage for position behavior on upsert update vs insert.
packages/twenty-server/src/engine/api/common/common-query-runners/common-create-one-query-runner.service.ts Prevents position backfill during create-one arg processing when upsert: true.
packages/twenty-server/src/engine/api/common/common-query-runners/common-create-many-query-runner/common-create-many-query-runner.service.ts Moves position backfill to post-categorization and applies it only to insert candidates during upsert.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@twenty-ci-bot-public

twenty-ci-bot-public Bot commented Jun 10, 2026

Copy link
Copy Markdown

🔍 Automated Pre-Review

No issues detected - This PR is ready for human review.


View details

Automated pre-review — human approval still required.

Comment on lines +314 to +333
it('should assign a position to a record inserted via upsert', async () => {
const upsertResponse = await makeGraphqlAPIRequest({
query: createRecordsWithPositionQuery,
variables: {
data: [
{
firstUniqueTestField: 'insertedViaUpsertField',
secondUniqueTestField: 'insertedViaUpsertSecondField',
name: 'insertedRecord',
},
],
upsert: true,
},
});

const insertedRecord = upsertResponse.body.data.createTestRecordObjects[0];

expect(insertedRecord.id).toEqual(expect.any(String));
expect(typeof insertedRecord.position).toBe('number');
});

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.

i don't understand this test, can we provide newPosition: initialPosition+1 for example and check position is properly updated?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants