From f914027b854960ead8ed012c5a9fedb5e066fd0a Mon Sep 17 00:00:00 2001 From: Artyom Savchenko Date: Wed, 21 May 2025 19:10:10 +0700 Subject: [PATCH] UBERF-10672: Fix person duplicates (#9004) --- pods/server/src/rpc.ts | 15 +- .../src/__tests__/parseEmailHeader.test.ts | 96 ++++++------- .../parseNameFromEmailHeader.test.ts | 47 +++--- .../mail-common/src/__tests__/person.test.ts | 134 ++++++++++++++++++ services/mail/mail-common/src/message.ts | 2 +- services/mail/mail-common/src/utils.ts | 31 +--- 6 files changed, 233 insertions(+), 92 deletions(-) diff --git a/pods/server/src/rpc.ts b/pods/server/src/rpc.ts index e5f047af46..3d0d6d2c06 100644 --- a/pods/server/src/rpc.ts +++ b/pods/server/src/rpc.ts @@ -344,8 +344,21 @@ export function registerRPC (app: Express, sessions: SessionManager, ctx: Measur name: combineName(firstName, lastName), personUuid: uuid }) + const createUniquePersonTx = txFactory.createTxApplyIf( + core.space.Workspace, + socialId, + [], + [ + { + _class: contact.class.Person, + query: { personUuid: uuid } + } + ], + [createPersonTx], + 'createLocalPerson' + ) - await session.txRaw(ctx, createPersonTx) + await session.txRaw(ctx, createUniquePersonTx) personRef = createPersonTx.objectId } diff --git a/services/mail/mail-common/src/__tests__/parseEmailHeader.test.ts b/services/mail/mail-common/src/__tests__/parseEmailHeader.test.ts index cde80c4ea3..ee83393b97 100644 --- a/services/mail/mail-common/src/__tests__/parseEmailHeader.test.ts +++ b/services/mail/mail-common/src/__tests__/parseEmailHeader.test.ts @@ -36,8 +36,8 @@ describe('parseEmailHeader', () => { expect(result).toEqual([ { email: 'test@example.com', - firstName: 'test', - lastName: 'example.com' + firstName: 'test@example.com', + lastName: '' } ]) }) @@ -47,8 +47,8 @@ describe('parseEmailHeader', () => { expect(result).toEqual([ { email: 'john.doe@example.com', - firstName: 'John', - lastName: 'Doe' + firstName: '', + lastName: 'John Doe' } ]) }) @@ -58,8 +58,8 @@ describe('parseEmailHeader', () => { expect(result).toEqual([ { email: 'john.doe@example.com', - firstName: 'John', - lastName: 'Doe' + firstName: '', + lastName: 'John Doe' } ]) }) @@ -69,8 +69,8 @@ describe('parseEmailHeader', () => { expect(result).toEqual([ { email: 'john.doe@example.com', - firstName: 'John', - lastName: 'Doe Smith' + firstName: '', + lastName: 'John Doe Smith' } ]) }) @@ -80,13 +80,13 @@ describe('parseEmailHeader', () => { expect(result).toEqual([ { email: 'test1@example.com', - firstName: 'test1', - lastName: 'example.com' + firstName: 'test1@example.com', + lastName: '' }, { email: 'test2@example.com', - firstName: 'test2', - lastName: 'example.com' + firstName: 'test2@example.com', + lastName: '' } ]) }) @@ -96,13 +96,13 @@ describe('parseEmailHeader', () => { expect(result).toEqual([ { email: 'john@example.com', - firstName: 'John', - lastName: 'example.com' + firstName: '', + lastName: 'John' }, { email: 'jane@example.com', - firstName: 'Jane', - lastName: 'Doe' + firstName: '', + lastName: 'Jane Doe' } ]) }) @@ -112,13 +112,13 @@ describe('parseEmailHeader', () => { expect(result).toEqual([ { email: 'john@example.com', - firstName: 'Doe,', - lastName: 'John' + firstName: '', + lastName: 'Doe, John' }, { email: 'jane@example.com', - firstName: 'Jane', - lastName: 'Doe' + firstName: '', + lastName: 'Jane Doe' } ]) }) @@ -128,13 +128,13 @@ describe('parseEmailHeader', () => { expect(result).toEqual([ { email: 'john@example.com', - firstName: 'john', - lastName: 'example.com' + firstName: 'john@example.com', + lastName: '' }, { email: 'jane@example.com', - firstName: 'Jane', - lastName: 'Doe' + firstName: '', + lastName: 'Jane Doe' } ]) }) @@ -144,13 +144,13 @@ describe('parseEmailHeader', () => { expect(result).toEqual([ { email: 'john@example.com', - firstName: 'john', - lastName: 'example.com' + firstName: 'john@example.com', + lastName: '' }, { email: 'jane@example.com', - firstName: 'jane', - lastName: 'example.com' + firstName: 'jane@example.com', + lastName: '' } ]) }) @@ -160,13 +160,13 @@ describe('parseEmailHeader', () => { expect(result).toEqual([ { email: 'john@example.com', - firstName: 'john', - lastName: 'example.com' + firstName: 'john@example.com', + lastName: '' }, { email: 'jane@example.com', - firstName: 'Jane', - lastName: 'Doe' + firstName: '', + lastName: 'Jane Doe' } ]) }) @@ -176,13 +176,13 @@ describe('parseEmailHeader', () => { expect(result).toEqual([ { email: 'example-staff@example.com', - firstName: 'example', - lastName: 'staff' + firstName: '', + lastName: 'example staff' }, { email: 'personnel@example.com', - firstName: 'personnel', - lastName: 'example.com' + firstName: '', + lastName: 'personnel' } ]) }) @@ -192,13 +192,13 @@ describe('parseEmailHeader', () => { expect(result).toEqual([ { email: 'abc@test.com', - firstName: 'abc', - lastName: 'test.com' + firstName: 'abc@test.com', + lastName: '' }, { email: '123@test.com', - firstName: '123', - lastName: 'test.com' + firstName: '123@test.com', + lastName: '' } ]) }) @@ -208,13 +208,13 @@ describe('parseEmailHeader', () => { expect(result).toEqual([ { email: 'john@example.com', - firstName: 'John', - lastName: 'example.com' + firstName: '', + lastName: 'John' }, { email: 'jane@example.com', - firstName: 'Jane', - lastName: 'example.com' + firstName: '', + lastName: 'Jane' } ]) }) @@ -224,13 +224,13 @@ describe('parseEmailHeader', () => { expect(result).toEqual([ { email: 'john@example.com', - firstName: 'John', - lastName: 'example.com' + firstName: '', + lastName: 'John' }, { email: 'jane@example.com', - firstName: 'Jane', - lastName: 'example.com' + firstName: '', + lastName: 'Jane' } ]) }) diff --git a/services/mail/mail-common/src/__tests__/parseNameFromEmailHeader.test.ts b/services/mail/mail-common/src/__tests__/parseNameFromEmailHeader.test.ts index 594a5285f1..ee2c81ad8b 100644 --- a/services/mail/mail-common/src/__tests__/parseNameFromEmailHeader.test.ts +++ b/services/mail/mail-common/src/__tests__/parseNameFromEmailHeader.test.ts @@ -1,3 +1,18 @@ +// +// Copyright © 2025 Hardcore Engineering Inc. +// +// Licensed under the Eclipse Public License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. You may +// obtain a copy of the License at https://www.eclipse.org/legal/epl-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// +// See the License for the specific language governing permissions and +// limitations under the License. +// + import { parseNameFromEmailHeader } from '../utils' import { EmailContact } from '../types' @@ -6,8 +21,8 @@ describe('parseNameFromEmailHeader', () => { const input = '"John Doe" ' const expected: EmailContact = { email: 'john.doe@example.com', - firstName: 'John', - lastName: 'Doe' + firstName: '', + lastName: 'John Doe' } expect(parseNameFromEmailHeader(input)).toEqual(expected) @@ -17,8 +32,8 @@ describe('parseNameFromEmailHeader', () => { const input = 'Jane Smith ' const expected: EmailContact = { email: 'jane.smith@example.com', - firstName: 'Jane', - lastName: 'Smith' + firstName: '', + lastName: 'Jane Smith' } expect(parseNameFromEmailHeader(input)).toEqual(expected) @@ -28,8 +43,8 @@ describe('parseNameFromEmailHeader', () => { const input = 'no-reply@example.com' const expected: EmailContact = { email: 'no-reply@example.com', - firstName: 'no-reply', - lastName: 'example.com' + firstName: 'no-reply@example.com', + lastName: '' } expect(parseNameFromEmailHeader(input)).toEqual(expected) @@ -39,8 +54,8 @@ describe('parseNameFromEmailHeader', () => { const input = '' const expected: EmailContact = { email: 'support@example.com', - firstName: 'support', - lastName: 'example.com' + firstName: 'support@example.com', + lastName: '' } expect(parseNameFromEmailHeader(input)).toEqual(expected) @@ -50,8 +65,8 @@ describe('parseNameFromEmailHeader', () => { const input = 'Maria Van Der Berg ' const expected: EmailContact = { email: 'maria@example.com', - firstName: 'Maria', - lastName: 'Van Der Berg' + firstName: '', + lastName: 'Maria Van Der Berg' } expect(parseNameFromEmailHeader(input)).toEqual(expected) @@ -82,8 +97,8 @@ describe('parseNameFromEmailHeader', () => { const input = 'John Doe john.doe@example.com' const expected: EmailContact = { email: 'John Doe john.doe@example.com', - firstName: 'John Doe john.doe', - lastName: 'example.com' + firstName: 'John Doe john.doe@example.com', + lastName: '' } expect(parseNameFromEmailHeader(input)).toEqual(expected) @@ -93,8 +108,8 @@ describe('parseNameFromEmailHeader', () => { const input = 'Support ' const expected: EmailContact = { email: 'help@example.com', - firstName: 'Support', - lastName: 'example.com' + firstName: '', + lastName: 'Support' } expect(parseNameFromEmailHeader(input)).toEqual(expected) @@ -104,8 +119,8 @@ describe('parseNameFromEmailHeader', () => { const input = '"O\'Neill, James" ' const expected: EmailContact = { email: 'james.oneill@example.com', - firstName: "O'Neill,", - lastName: 'James' + firstName: '', + lastName: "O'Neill, James" } expect(parseNameFromEmailHeader(input)).toEqual(expected) diff --git a/services/mail/mail-common/src/__tests__/person.test.ts b/services/mail/mail-common/src/__tests__/person.test.ts index f70dade268..6d03a588d2 100644 --- a/services/mail/mail-common/src/__tests__/person.test.ts +++ b/services/mail/mail-common/src/__tests__/person.test.ts @@ -146,6 +146,140 @@ describe('PersonCache', () => { expect(result2).toEqual(mockPerson2) expect(mockRestClient.ensurePerson).toHaveBeenCalledTimes(2) }) + + it('should normalize email with whitespace and different case', async () => { + // Arrange + const email1 = ' MiXeD@ExAmPlE.com ' // With whitespace and mixed case + const email2 = 'mixed@example.com' // Normal lowercase + + const contact1: EmailContact = { + email: email1, + firstName: 'Mixed', + lastName: 'Case' + } + + const contact2: EmailContact = { + email: email2, + firstName: 'Mixed', + lastName: 'Case' + } + + const mockPerson = { + socialId: 'person-789' as PersonId, + uuid: 'uuid-789' as PersonUuid, + localPerson: 'local-789' + } + + mockRestClient.ensurePerson.mockResolvedValue(mockPerson) + + // Act + await personCache.ensurePerson(contact1) + await personCache.ensurePerson(contact2) + + // Assert + expect(mockRestClient.ensurePerson).toHaveBeenCalledTimes(1) + expect(mockRestClient.ensurePerson).toHaveBeenCalledWith( + SocialIdType.EMAIL, + 'mixed@example.com', // normalized email + 'Mixed', + 'Case' + ) + }) + + it('should create different person entries for different emails', async () => { + // Arrange + const email1 = 'first@example.com' + const email2 = 'second@example.com' + + const contact1: EmailContact = { + email: email1, + firstName: 'First', + lastName: 'User' + } + + const contact2: EmailContact = { + email: email2, + firstName: 'Second', + lastName: 'User' + } + + const mockPerson1 = { + socialId: 'person-1' as PersonId, + uuid: 'uuid-1' as PersonUuid, + localPerson: 'local-1' + } + + const mockPerson2 = { + socialId: 'person-2' as PersonId, + uuid: 'uuid-2' as PersonUuid, + localPerson: 'local-2' + } + + mockRestClient.ensurePerson.mockResolvedValueOnce(mockPerson1).mockResolvedValueOnce(mockPerson2) + + // Act + const result1 = await personCache.ensurePerson(contact1) + const result2 = await personCache.ensurePerson(contact2) + + // Assert + expect(mockRestClient.ensurePerson).toHaveBeenCalledTimes(2) + expect(result1).toEqual(mockPerson1) + expect(result2).toEqual(mockPerson2) + expect(personCache.size()).toBe(2) + }) + + it('should handle concurrent requests for the same email', async () => { + // Arrange + const email = 'concurrent@example.com' + const contact: EmailContact = { + email, + firstName: 'Concurrent', + lastName: 'User' + } + + // Create a delayed mock response to simulate server latency + const mockPerson = { + socialId: 'person-123' as PersonId, + uuid: 'uuid-123' as PersonUuid, + localPerson: 'local-123' + } + + // Create a delayed promise that resolves after 50ms + mockRestClient.ensurePerson.mockImplementation( + () => + new Promise((resolve) => { + setTimeout(() => { + resolve(mockPerson) + }, 50) + }) + ) + + // Act - Create multiple concurrent requests + const requests = [] + for (let i = 0; i < 10; i++) { + requests.push(personCache.ensurePerson(contact)) + } + + // Wait for all requests to complete + const results = await Promise.all(requests) + + // Assert + expect(mockRestClient.ensurePerson).toHaveBeenCalledTimes(1) + expect(mockRestClient.ensurePerson).toHaveBeenCalledWith( + SocialIdType.EMAIL, + 'concurrent@example.com', + 'Concurrent', + 'User' + ) + + // All results should reference the same person + results.forEach((result) => { + expect(result).toEqual(mockPerson) + }) + + // Cache size should be 1 + expect(personCache.size()).toBe(1) + }) }) describe('clearCache', () => { diff --git a/services/mail/mail-common/src/message.ts b/services/mail/mail-common/src/message.ts index f88ecbb884..ae5b1d98e2 100644 --- a/services/mail/mail-common/src/message.ts +++ b/services/mail/mail-common/src/message.ts @@ -243,7 +243,7 @@ async function saveMessageToSpaces ( space._id, mail.tag.MailThread, {}, - Date.now(), + createdDate, owner ) threadId = newThreadId as Ref diff --git a/services/mail/mail-common/src/utils.ts b/services/mail/mail-common/src/utils.ts index 6639c8973c..58c851a328 100644 --- a/services/mail/mail-common/src/utils.ts +++ b/services/mail/mail-common/src/utils.ts @@ -72,41 +72,20 @@ export function parseNameFromEmailHeader (headerValue: string | undefined): Emai if (match == null) { const address = headerValue.trim() - const parts = address.split('@') return { email: address, - firstName: parts[0], - lastName: parts[1] + firstName: address, + lastName: '' } } const displayName = match[1]?.trim() const email = match[2].trim() - if (displayName == null || displayName === '') { - const parts = email.split('@') - return { - email, - firstName: parts[0], - lastName: parts[1] - } - } - - const nameParts = displayName.split(/\s+/) - let firstName: string | undefined - let lastName: string | undefined - - if (nameParts.length === 1) { - firstName = nameParts[0] - } else if (nameParts.length > 1) { - firstName = nameParts[0] - lastName = nameParts.slice(1).join(' ') - } - - const parts = email.split('@') + const wrappedEmail = displayName != null && displayName.length > 0 ? `<${email}>` : email return { email, - firstName: firstName ?? parts[0], - lastName: lastName ?? parts[1] + firstName: wrappedEmail, + lastName: displayName ?? '' } }