Repository navigation
fix(members): store any company name in the IDP member sync and retry on concurrent member creation - #652
fix(members): store any company name in the IDP member sync and retry on concurrent member creation#652romanetar wants to merge 3 commits into
Conversation
…n1 company and concurrent creation Company outside latin1 (full-width parentheses, Latin Extended) must be stored as sent by the IDP, and registerExternalUserById() must retry when another process creates the member between the lookups and the insert. All three fail on the current code.
…s any company name Company was latin1, so a company name with a character outside latin1 made the write fail with 1366 in strict mode and rolled back the whole member sync. latin1 is a subset of utf8mb4, so existing values convert unchanged. The ALTER copies the table and blocks writes to Member while it runs.
…reation When another process creates the same member between the lookups and the insert, the unique ExternalUserId or Email key rejects the insert and the message was lost. Run the sync again in a new transaction so it finds and updates that member, as the login path does.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 40.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 10 functions across 3 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes both main changes: supporting company names outside latin1 and retrying member synchronization after concurrent creation. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-652/ This page is automatically updated on each push to this PR. |
ref: https://app.clickup.com/t/9014802374/86bch3jgw
Problem
Member.Companyis latin1. A company name with a character outside latin1 (full-width parentheses,Ł) is rejected by production's strict MySQL with1366 Incorrect string value. The wholeregisterExternalUserById()transaction rolls back (name, email, verification, groups) and the attendee sync that follows never fires. The message jobs swallow the exception, so nothing retries and the same user fails on every later update.Change
Member.Companytoutf8mb4_general_ci(latin1 is a subset, existing values convert unchanged). MySQL cannot do it in place (ALGORITHM=COPY), so writes toMemberare blocked while it runs.MemberService::registerExternalUserById()retries once, in a new transaction, when it fails withUniqueConstraintViolationException(ExternalUserIdorEmailkey), and logs a warning. The second run finds the member the other process created and updates it.Tests
New in
tests/MemberServiceTest.php, all three failing onmain:（ACME）andŁódź IncthroughregisterExternalUserByPayload(): stored as sent and first name updated (onmainthey read back?ACME?and?ód? Inc, CI runs MySQL non-strict)registerExternalUserById()with the IDP API mocked and a repository double that misses the first lookups: onmainit throwsDuplicate entry ... for key 'Member.Email'phpunit tests/MemberServiceTest.php: 6 tests, 0 failures.Production check
Loki, last 30 days,
{service_name="summit-api", k8s_cluster_name="prod-fn", container="summit-api-worker-message-broker-php"} |~ "(?i)incorrect string":Company, no otherMembercolumn fails, so the migration converts onlyCompany.PublishUserCreated/PublishUserUpdated, for 10 IDP users. The same user fails again on later updates (user 100318 failed on 09-29 and again on 10-13).ExternalUserIdduplicate of 2026-10-06 (IDP user 102460,PublishUserCreated) comes from the insert of a new member (MemberService.php:371), so the retry covers it: the second run finds the member the other process created.Pending
registerExternalUserById()the IDP users whose sync failed: 96477, 100318, 104565, 97078, 91396, 91711, 100767, 100066, 99199, 98654 and 102460 (duplicate). Run it after the migration, otherwise they fail again with the 1366.Incorrect string valuereturns no lines over 7 days.🤖 Generated with Claude Code
Summary by CodeRabbit