Restore soft-deleted junction record when re-adding a junction relation (#23371)
Fixes #23305. Removing a junction relation soft-deletes the intermediate junction record. Re-adding the same relation created a brand new record, which the composite unique index on the junction object (e.g. `personId` + `companyId`) rejected with "This record already exists". `useUpdateJunctionRelationFromCell` now creates the junction record through `useCreateManyRecords` with `upsert: true`. The server matches on the unique index with `withDeleted()` and clears `deletedAt` on the matched row instead of inserting. ## This revives, it does not create Worth being explicit, because it is a deliberate trade and not obvious from the diff. When a soft-deleted row exists for the pair, the user gets that row back. Same id, same `createdAt`, same `createdBy`, and anything attached to it (notes, files, and any fields the app added to the junction object). Verified on a dev instance: after re-adding a link through the picker, the row still reported a creation date from days earlier. That is fine for a pure link. It is a lie for a junction that carries data, for instance a `PersonCompanyRelationship` with a role and dates. The alternative designs are hard-deleting on detach (needs `canDestroyObjectRecords` on the junction object, which most roles do not grant) and partial unique indexes (needs `indexWhereClause` exposed to app-declared indexes, which the SDK does not support today). Both are larger changes. This one unblocks affected apps without requiring anything from them. Note that upsert conflict detection ignores `indexWhereClause`, so shipping partial indexes later would not change this behaviour on its own. ## Why the id handling changed The hook no longer sends a client-generated id. Under upsert the server decides which row you get, so the id in the input would be discarded. The optimistic store entry still uses a local id so the chip appears immediately, then adopts the persisted id once the mutation resolves. Without that, a revive left an id in the store that exists nowhere on the server, and the next removal failed with "This record does not exist or has been deleted". Supersedes #23365, which took the same approach but added `$upsert` to the shared `createOne` mutation. That capability already exists per call on `createMany`, and widening the shared document broke the `useCreateOneRecordMutation` and `useCreateOneRecord` tests. ## Testing Verified end to end on a dev instance with a junction object carrying a non-partial composite unique index, since the dev seed does not create one and the bug cannot reproduce without it: - before: re-adding after a removal fails with "This record already exists" - after: the original row is restored, `deletedAt` cleared, one row throughout, and removing again in the same session works, with no GraphQL errors Added an integration test for the path this depends on: a soft-deleted record matched on its unique fields alone, with no id in the input. The existing coverage only exercised upsert by explicit id. ## Known gaps, deliberately not addressed here **Toggling a link off while its creation is still in flight.** The store id is provisional until the mutation returns, so a removal issued inside that window deletes an id the server never received. Reproduced by stalling the create and clicking remove during it: `CombinedGraphQLErrors: Record not found`, and the link stays active despite the user removing it. The window is one mutation round trip. Left for a follow-up; the likely fix is a per-record operation queue so adds and removes on a field run in click order. **Junction objects with no unique index.** Remove then re-add still creates a duplicate row there, on this branch as on main, because conflict detection is driven by unique indexes. --------- Co-authored-by: Shinu Cherian <129690295+Shinu-Cherian@users.noreply.github.com>
This commit is contained in:
+88
-15
@@ -6,7 +6,8 @@ import { v4 } from 'uuid';
|
||||
import { useObjectMetadataItems } from '@/object-metadata/hooks/useObjectMetadataItems';
|
||||
import { type FieldMetadataItem } from '@/object-metadata/types/FieldMetadataItem';
|
||||
import { getObjectTypename } from '@/object-record/cache/utils/getObjectTypename';
|
||||
import { useCreateOneRecord } from '@/object-record/hooks/useCreateOneRecord';
|
||||
import { getRecordFromRecordNode } from '@/object-record/cache/utils/getRecordFromRecordNode';
|
||||
import { useCreateManyRecords } from '@/object-record/hooks/useCreateManyRecords';
|
||||
import { useDeleteOneRecord } from '@/object-record/hooks/useDeleteOneRecord';
|
||||
import { type FieldDefinition } from '@/object-record/record-field/ui/types/FieldDefinition';
|
||||
import {
|
||||
@@ -56,11 +57,8 @@ export const useUpdateJunctionRelationFromCell = ({
|
||||
junctionObjectMetadata?.nameSingular ??
|
||||
fieldDefinition.metadata.relationObjectMetadataNameSingular;
|
||||
|
||||
// Skip the post-optimistic effect since we handle optimistic updates manually
|
||||
// Otherwise Apollo would also add the record, resulting in duplicates
|
||||
const { createOneRecord: createJunctionRecord } = useCreateOneRecord({
|
||||
const { createManyRecords: createJunctionRecords } = useCreateManyRecords({
|
||||
objectNameSingular: junctionObjectNameSingular,
|
||||
skipPostOptimisticEffect: true,
|
||||
});
|
||||
|
||||
const { deleteOneRecord: deleteJunctionRecord } = useDeleteOneRecord({
|
||||
@@ -175,11 +173,11 @@ export const useUpdateJunctionRelationFromCell = ({
|
||||
}
|
||||
|
||||
const targetRecord = searchRecord.record;
|
||||
const newJunctionId = v4();
|
||||
const optimisticJunctionId = v4();
|
||||
const now = new Date().toISOString();
|
||||
|
||||
const junctionRecordForStore = {
|
||||
id: newJunctionId,
|
||||
id: optimisticJunctionId,
|
||||
createdAt: now,
|
||||
updatedAt: now,
|
||||
__typename: getObjectTypename(junctionObjectName),
|
||||
@@ -188,12 +186,6 @@ export const useUpdateJunctionRelationFromCell = ({
|
||||
[targetFieldName]: targetRecord,
|
||||
};
|
||||
|
||||
const newJunctionRecordForApi = {
|
||||
id: newJunctionId,
|
||||
[sourceJoinColumnName]: recordId,
|
||||
[targetJoinColumnName]: morphItem.recordId,
|
||||
};
|
||||
|
||||
store.set(
|
||||
recordStoreFamilyState.atomFamily(recordId),
|
||||
(currentRecord: Record<string, unknown> | null | undefined) => {
|
||||
@@ -213,12 +205,93 @@ export const useUpdateJunctionRelationFromCell = ({
|
||||
},
|
||||
);
|
||||
|
||||
await createJunctionRecord(newJunctionRecordForApi);
|
||||
const removeOptimisticJunctionRecord = () =>
|
||||
store.set(
|
||||
recordStoreFamilyState.atomFamily(recordId),
|
||||
(currentRecord: Record<string, unknown> | null | undefined) => {
|
||||
if (!isDefined(currentRecord)) {
|
||||
return currentRecord;
|
||||
}
|
||||
|
||||
const currentFieldValue = currentRecord[fieldName];
|
||||
|
||||
return {
|
||||
...currentRecord,
|
||||
[fieldName]: Array.isArray(currentFieldValue)
|
||||
? currentFieldValue.filter(
|
||||
(junctionRecord) =>
|
||||
junctionRecord.id !== optimisticJunctionId,
|
||||
)
|
||||
: currentFieldValue,
|
||||
} as ObjectRecord;
|
||||
},
|
||||
);
|
||||
|
||||
const persistedJunctionRecordNode = await createJunctionRecords({
|
||||
recordsToCreate: [
|
||||
{
|
||||
[sourceJoinColumnName]: recordId,
|
||||
[targetJoinColumnName]: morphItem.recordId,
|
||||
},
|
||||
],
|
||||
upsert: true,
|
||||
})
|
||||
.then(([createdJunctionRecordNode]) => createdJunctionRecordNode)
|
||||
.catch((error: Error) => {
|
||||
removeOptimisticJunctionRecord();
|
||||
|
||||
throw error;
|
||||
});
|
||||
|
||||
if (!isDefined(persistedJunctionRecordNode)) {
|
||||
removeOptimisticJunctionRecord();
|
||||
|
||||
return;
|
||||
}
|
||||
|
||||
const persistedJunctionRecord = getRecordFromRecordNode({
|
||||
recordNode: persistedJunctionRecordNode,
|
||||
});
|
||||
|
||||
store.set(
|
||||
recordStoreFamilyState.atomFamily(recordId),
|
||||
(currentRecord: Record<string, unknown> | null | undefined) => {
|
||||
if (!isDefined(currentRecord)) {
|
||||
return currentRecord;
|
||||
}
|
||||
|
||||
const currentFieldValue = currentRecord[fieldName];
|
||||
|
||||
if (!Array.isArray(currentFieldValue)) {
|
||||
return currentRecord as ObjectRecord;
|
||||
}
|
||||
|
||||
const junctionRecordsWithoutOptimistic = currentFieldValue.filter(
|
||||
(junctionRecord) => junctionRecord.id !== optimisticJunctionId,
|
||||
);
|
||||
|
||||
const isPersistedJunctionRecordAlreadyInStore =
|
||||
junctionRecordsWithoutOptimistic.some(
|
||||
(junctionRecord) =>
|
||||
junctionRecord.id === persistedJunctionRecord.id,
|
||||
);
|
||||
|
||||
return {
|
||||
...currentRecord,
|
||||
[fieldName]: isPersistedJunctionRecordAlreadyInStore
|
||||
? junctionRecordsWithoutOptimistic
|
||||
: [
|
||||
...junctionRecordsWithoutOptimistic,
|
||||
{ ...junctionRecordForStore, ...persistedJunctionRecord },
|
||||
],
|
||||
} as ObjectRecord;
|
||||
},
|
||||
);
|
||||
}
|
||||
},
|
||||
[
|
||||
store,
|
||||
createJunctionRecord,
|
||||
createJunctionRecords,
|
||||
deleteJunctionRecord,
|
||||
fieldDefinition.metadata.fieldName,
|
||||
junctionConfig,
|
||||
|
||||
+44
@@ -228,6 +228,50 @@ describe('upsert (createMany with upsert:true)', () => {
|
||||
);
|
||||
});
|
||||
|
||||
it('should restore a soft-deleted record matched on its unique fields only', async () => {
|
||||
const createResponse = await makeGraphqlAPIRequest({
|
||||
query: createRecordsQuery,
|
||||
variables: {
|
||||
data: [
|
||||
{
|
||||
firstUniqueTestField: 'softDeletedByUniqueFields',
|
||||
secondUniqueTestField: 'softDeletedByUniqueFieldsSecond',
|
||||
name: 'originalRecord',
|
||||
},
|
||||
],
|
||||
upsert: false,
|
||||
},
|
||||
});
|
||||
|
||||
const createdRecord = createResponse.body.data.createTestRecordObjects[0];
|
||||
|
||||
await makeGraphqlAPIRequest({
|
||||
query: deleteRecordsQuery,
|
||||
variables: {
|
||||
filter: { id: { eq: createdRecord.id } },
|
||||
},
|
||||
});
|
||||
|
||||
const upsertResponse = await makeGraphqlAPIRequest({
|
||||
query: createRecordsQuery,
|
||||
variables: {
|
||||
data: [
|
||||
{
|
||||
firstUniqueTestField: 'softDeletedByUniqueFields',
|
||||
secondUniqueTestField: 'softDeletedByUniqueFieldsSecond',
|
||||
},
|
||||
],
|
||||
upsert: true,
|
||||
},
|
||||
});
|
||||
|
||||
const upsertedRecord = upsertResponse.body.data.createTestRecordObjects[0];
|
||||
|
||||
expect(upsertedRecord.id).toEqual(createdRecord.id);
|
||||
expect(upsertedRecord.deletedAt).toBeNull();
|
||||
expect(upsertedRecord.name).toEqual('originalRecord');
|
||||
});
|
||||
|
||||
it('should update and restore updated soft-deleted record', async () => {
|
||||
const createResponse = await makeGraphqlAPIRequest({
|
||||
query: createRecordsQuery,
|
||||
|
||||
Reference in New Issue
Block a user