fix(members): store any company name in the IDP member sync and retry on concurrent member creation

app/Services/Model/Imp/MemberService.php

@@ -21,6 +21,7 @@
 use App\Services\Apis\IExternalUserApi;
 use App\Services\Model\dto\ExternalUserDTO;
 use DateTime;
+use Doctrine\DBAL\Exception\UniqueConstraintViolationException;
 use Illuminate\Support\Facades\Cache;
 use Illuminate\Support\Facades\Event;
 use Illuminate\Support\Facades\Log;
@@ -454,15 +455,35 @@ public function registerExternalUserByPayload(array $user_data):Member{
      */
     public function registerExternalUserById($user_external_id): Member
     {
-        $member = $this->tx_service->transaction(function () use ($user_external_id) {
-            // get external user from IDP
-            $user_data = $this->user_ext_api->getUserById($user_external_id);
-            if(is_null($user_data) || !isset($user_data['email'])){
-                Log::warning(sprintf("MemberService::registerExternalUserById user_external_id %s does not exists.", $user_external_id));
-                throw new EntityNotFoundException(sprintf("MemberService::registerExternalUserById user_external_id %s does not exists.", $user_external_id));
-            }
-            return $this->registerExternalUserByPayload($user_data);
-        });
+        $register = function () use ($user_external_id) {
+            return $this->tx_service->transaction(function () use ($user_external_id) {
+                // get external user from IDP
+                $user_data = $this->user_ext_api->getUserById($user_external_id);
+                if(is_null($user_data) || !isset($user_data['email'])){
+                    Log::warning(sprintf("MemberService::registerExternalUserById user_external_id %s does not exists.", $user_external_id));
+                    throw new EntityNotFoundException(sprintf("MemberService::registerExternalUserById user_external_id %s does not exists.", $user_external_id));
+                }
+                return $this->registerExternalUserByPayload($user_data);
+            });
+        };
+
+        try {
+            $member = $register();
+        } catch (UniqueConstraintViolationException $ex) {
+            // another process (login or a concurrent IDP message) created the same member between
+            // our lookups and our insert (READ COMMITTED takes no gap lock), the retry finds it and updates it.
+            // Both Email and ExternalUserId are unique keys, so either one can be the reported one
+            Log::warning
+            (
+                sprintf
+                (
+                    "MemberService::registerExternalUserById user_external_id %s concurrent member creation, retrying once: %s",
+                    $user_external_id,
+                    $ex->getMessage()
+                )
+            );
+            $member = $register();
+        }
 
         Event::dispatch(new MemberDataUpdatedExternally($member->getId()));
 

database/migrations/model/Version20261021170000.php

@@ -0,0 +1,28 @@
+<?php
+
+namespace Database\Migrations\Model;
+
+use Doctrine\Migrations\AbstractMigration;
+use Doctrine\DBAL\Schema\Schema as Schema;
+
+/**
+ * Member.Company was latin1: a company name with a character outside latin1 (full-width
+ * parentheses, Latin Extended) made the IDP member sync fail with 1366 in strict mode.
+ * latin1 is a subset of utf8mb4, so existing values convert unchanged.
+ * MySQL can not do it in place (ALGORITHM=COPY): writes to Member are blocked while it runs.
+ */
+class Version20261021170000 extends AbstractMigration
+{
+    public function up(Schema $schema): void
+    {
+        $sql = <<<SQL
+ALTER TABLE `Member` MODIFY Company VARCHAR(255) CHARACTER SET utf8mb4 COLLATE utf8mb4_general_ci NULL;
+SQL;
+        $this->addSql($sql);
+    }
+
+    public function down(Schema $schema): void
+    {
+
+    }
+}

tests/MemberServiceTest.php

@@ -11,14 +11,18 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  **/
+use App\Services\Apis\IExternalUserApi;
 use App\Services\Model\dto\ExternalUserDTO;
 use App\Services\Model\IMemberService;
+use Doctrine\DBAL\Exception\UniqueConstraintViolationException;
 use Illuminate\Support\Facades\App;
 use Illuminate\Support\Facades\DB;
 use LaravelDoctrine\ORM\Facades\EntityManager;
 use LaravelDoctrine\ORM\Facades\Registry;
 use libs\utils\ITransactionService;
+use Mockery;
 use models\main\Group;
+use models\main\IMemberRepository;
 use models\main\Member;
 use models\utils\SilverstripeBaseModel;
 
@@ -50,6 +54,25 @@ final class MemberServiceTest extends TestCase
     /** @var string */
     private $reassigned_email;
 
+    /** @var string */
+    private $new_email;
+
+    /** @var \Mockery\MockInterface */
+    private $external_user_api;
+
+    public function createApplication()
+    {
+        $app = parent::createApplication();
+
+        // mock the IDP user API before any service singleton is resolved
+        $this->external_user_api = Mockery::mock(IExternalUserApi::class)->shouldIgnoreMissing();
+        App::singleton(IExternalUserApi::class, function () {
+            return $this->external_user_api;
+        });
+
+        return $app;
+    }
+
     protected function setUp(): void
     {
         parent::setUp();
@@ -81,7 +104,8 @@ protected function setUp(): void
 
         // member already provisioned for the new external id, with a different email
         $new_member = new Member();
-        $new_member->setEmail(strtolower("new+{$prefix}@test.com"));
+        $this->new_email = strtolower("new+{$prefix}@test.com");
+        $new_member->setEmail($this->new_email);
         $new_member->setActive(true);
         $new_member->setEmailVerified(true);
         $new_member->setFirstName("New");
@@ -268,6 +292,90 @@ public function testSynchronizeGroupsAdditiveModePreservesOutOfBandGroups(): voi
         }
     }
 
+    /**
+     * Company is stored in a column that must hold any character the IDP sends, such as the
+     * full-width parentheses. Production runs MySQL in strict mode and rejects the write (1366),
+     * CI is non-strict and silently stores "?" in place of the characters latin1 lacks, so the
+     * test asserts the value read back from the database.
+     *
+     * @dataProvider companyOutsideLatin1Provider
+     */
+    public function testRegisterExternalUserByPayloadStoresCompanyOutsideLatin1(string $company): void
+    {
+        $payload               = $this->buildExternalProfilePayload($this->new_external_id, $this->new_email);
+        $payload['company']    = $company;
+        $payload['first_name'] = 'Renamed';
+
+        $tx_service = App::make(ITransactionService::class);
+        $tx_service->transaction(function () use ($payload) {
+            return App::make(IMemberService::class)->registerExternalUserByPayload($payload);
+        });
+
+        $this->em->clear();
+
+        $member = $this->member_repository->findOneBy(['user_external_id' => $this->new_external_id]);
+        $this->assertNotNull($member, "member must still exist");
+        $this->assertEquals($company, $member->getCompany(), "company must be stored as sent by the IDP");
+        $this->assertEquals('Renamed', $member->getFirstName(), "first name must be updated");
+    }
+
+    public static function companyOutsideLatin1Provider(): array
+    {
+        return [
+            'full-width parentheses' => ['（ACME）'],
+            'latin extended'         => ['Łódź Inc'],
+        ];
+    }
+
+    /**
+     * Race: another process created the member of this IDP user between our lookups and our
+     * insert, so both lookups miss (READ COMMITTED takes no gap lock) and the insert collides
+     * with the unique ExternalUserId / Email keys. The sync must retry once and update the
+     * member the other process created, as the login path does.
+     */
+    public function testRegisterExternalUserByIdRetriesWhenMemberWasCreatedConcurrently(): void
+    {
+        $payload               = $this->buildExternalProfilePayload($this->new_external_id, $this->new_email);
+        $payload['first_name'] = 'Raced';
+
+        $this->external_user_api->shouldReceive('getUserById')
+            ->with($this->new_external_id)
+            ->andReturn($payload);
+
+        // double of the member repository: the first lookups by external id and by email miss
+        // (the member is not visible yet), every other call is forwarded to the real repository
+        $real_repository = EntityManager::getRepository(Member::class);
+        $repository      = Mockery::mock(IMemberRepository::class);
+        $repository->shouldReceive('getByExternalIdExclusiveLock')->once()->andReturn(null);
+        $repository->shouldReceive('getByEmail')->once()->andReturn(null);
+        foreach ((new \ReflectionClass(IMemberRepository::class))->getMethods() as $method) {
+            $name = $method->getName();
+            $repository->shouldReceive($name)->andReturnUsing(
+                function (...$args) use ($real_repository, $name) {
+                    return $real_repository->$name(...$args);
+                }
+            );
+        }
+        App::singleton(IMemberRepository::class, function () use ($repository) {
+            return $repository;
+        });
+        // the service singleton may already be resolved with the real repository
+        App::forgetInstance(IMemberService::class);
+
+        try {
+            App::make(IMemberService::class)->registerExternalUserById($this->new_external_id);
+        } catch (UniqueConstraintViolationException $ex) {
+            $this->fail(sprintf("the sync must retry on a concurrent member creation: %s", $ex->getMessage()));
+        }
+
+        $this->em = Registry::resetManager(SilverstripeBaseModel::EntityManager);
+        $this->member_repository = EntityManager::getRepository(Member::class);
+
+        $members = $this->member_repository->findBy(['user_external_id' => $this->new_external_id]);
+        $this->assertCount(1, $members, "the existing member must be reused, not duplicated");
+        $this->assertEquals('Raced', $members[0]->getFirstName(), "the existing member must be updated");
+    }
+
     private function buildExternalProfilePayload(int $external_id, string $email): array
     {
         return [