feat(cells): Populate org listing fields on control serializer#115513
Conversation
Adds the following fields to the control version of the org listing api in order to align to the cell version. - isEarlyAdopter - require2FA - allowMemberInvite - allowMemberProjectCreation - allowSuperuserAccess - status - links - hasAuthProvider - avatar - dateCreated Updated the test to assert the control payload exactly matches the cell version Split `ControlSiloOrganizationMappingSerializer` off from `ControlSiloOrganizationSerializer` so the user-identity-config call site stays minimal and doesn't pull in fields it doesn't need.
github-actions
Bot
added
the
Scope: Backend
bce13a3 to
030df5d
Compare
![sentry-warden[bot]](https://avatars.githubusercontent.com/in/2755838?s=60&v=4){kind=small}
| status = OrganizationStatus(obj.status) | ||
| avatar_replica: OrganizationAvatarReplica | None = attrs["avatar"] | ||
| if avatar_replica is not None: | ||
| avatar_type = self._AVATAR_TYPE_BY_ID[avatar_replica.avatar_type] |
There was a problem hiding this comment.
KeyError on unknown avatar_type from replica
self._AVATAR_TYPE_BY_ID[avatar_replica.avatar_type] will raise KeyError if the replica's avatar_type is anything other than 0 or 1. OrganizationAvatarReplica.avatar_type is a PositiveSmallIntegerField with no choices constraint and is populated via RPC from the cell silo, so any version skew (new avatar type added on a cell before the control silo is updated) or corrupted replica row crashes the entire org listing response. Use .get() with a safe fallback to letter_avatar.
Verification
Checked src/sentry/models/organizationavatarreplica.py line 15: avatar_type = models.PositiveSmallIntegerField(default=0) has no choices enforcement. Checked src/sentry/models/avatars/organization_avatar.py line 25: AVATAR_TYPES = ((0, "letter_avatar"), (1, "upload")). _AVATAR_TYPE_BY_ID is built from this tuple so it contains only {0, 1}. Replica rows are written via RPC (src/sentry/hybridcloud/services/replica/impl.py:403-408) which accepts any int. Any unexpected avatar_type value causes KeyError on subscript lookup, breaking the entire serialized response since serialize is called per item without exception handling.
Identified by Warden sentry-backend-bugs · LY9-45G
There was a problem hiding this comment.
Fix attempt detected (commit ba8ed15)
The code at line 381 still uses direct dictionary subscript access self._AVATAR_TYPE_BY_ID[avatar_replica.avatar_type] instead of the requested .get() method with a fallback, so the KeyError on unknown avatar_type values persists.
The original issue appears unresolved. Please review and try again.
Evaluated by Warden
There was a problem hiding this comment.
KeyError is fine here. The field should be choice-constrained upstream -- silently falling back would hide a real bug
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 030df5d. Configure here.
| **kwargs: Any, | ||
| ) -> ControlSiloOrganizationSerializerResponse: | ||
| ) -> ControlSiloOrganizationMappingSerializerResponse: | ||
| status = OrganizationStatus(obj.status) |
There was a problem hiding this comment.
Nullable status field causes crash in serializer
Medium Severity
OrganizationMapping.status is defined with null=True, but the serializer unconditionally calls OrganizationStatus(obj.status). If status is None, this raises a ValueError because None is not a valid IntEnum value, resulting in a 500 error for the listing endpoint.
Reviewed by Cursor Bugbot for commit 030df5d. Configure here.
There was a problem hiding this comment.
Imo this field is should never be allowed to be null -- it seems like a historical artifact from pre-initial backfill.
I think the real fix is to issue a migration to make it non-nullable, rather than make this code less strict
There was a problem hiding this comment.
Agreed, this field should become required.
| } | ||
| else: | ||
| avatar = {"avatarType": "letter_avatar", "avatarUuid": None, "avatarUrl": None} | ||
| return dict( | ||
| id=str(obj.organization_id), | ||
| slug=obj.slug, | ||
| name=obj.name, | ||
| name=obj.name or obj.slug, | ||
| status={"id": status.name.lower(), "name": status.label}, | ||
| dateCreated=obj.date_created, | ||
| isEarlyAdopter=obj.early_adopter, | ||
| require2FA=obj.require_2fa, | ||
| allowMemberInvite=not obj.disable_member_invite, | ||
| allowMemberProjectCreation=not obj.disable_member_project_creation, | ||
| allowSuperuserAccess=not obj.prevent_superuser_access, | ||
| avatar=avatar, | ||
| links={ | ||
| "organizationUrl": generate_organization_url(obj.slug), | ||
| "regionUrl": generate_locality_url(get_locality_name_for_cell(obj.cell_name)), | ||
| }, | ||
| hasAuthProvider=attrs["has_auth_provider"], |
There was a problem hiding this comment.
CellResolutionError on a single mapping breaks the entire org listing response
If any OrganizationMapping.cell_name in the page cannot be resolved to a locality, get_locality_name_for_cell raises CellResolutionError and the whole paginated listing 500s. Catch the error per-row and fall back (e.g., omit links.regionUrl/avatarUrl or skip the row).
Verification
Read get_locality_name_for_cell in src/sentry/types/cell.py:409 — it raises CellResolutionError when get_locality_for_cell returns None. This function is called twice per row in ControlSiloOrganizationMappingSerializer.serialize (once for the avatar URL at line ~392 and once for links.regionUrl at line ~408). The serializer is used by src/sentry/core/endpoints/organization_index.py:352 in the paginated listing endpoint via serialize(x, request.user, ...). A single mapping with an unknown/retired cell_name will raise during serialization and abort the whole list response with a 500 instead of returning the other orgs. The error is unhandled by the calling endpoint.
Identified by Warden sentry-backend-bugs · AE2-QNH
There was a problem hiding this comment.
i think i'd leave this as-is and let it fail.. an unresolvable cell means something is fundamentally broken about the org/config for cells and localities. I don't think silently dropping or degrading here is a good idea, as the error might get buried -- if it ever happens something is really broken.
## Summary `GET /organizations/` is now served from the control silo and returns every org the user belongs to across all regions in one paginated response (getsentry/sentry#112622, getsentry/sentry#115513). This replaces the `/users/me/regions/` discovery + per-region fan-out in `listOrganizationsUncached()` with a single call, routing subsequent org-scoped requests using each org's own `links.regionUrl` from the response. - Self-hosted/monolith instances serve the same endpoint from the same base URL, so the separate fallback branch is gone — one less code path to maintain. - Removes the multi-region 403 partial-success bookkeeping (CLI-89's `Promise.allSettled` tracking) since there's only one auth check now instead of N per-region ones. - Updated the multi-region e2e mock server to model the control silo's combined org listing (it previously only modeled per-region listings, which is what the real backend replaced). Mirrors the same change the Rust `sentry-cli` just made in [getsentry/sentry-cli#3352](getsentry/sentry-cli#3352). ## Test plan - [x] `pnpm exec tsc --noEmit` - [x] `pnpm exec biome check` - [x] `pnpm vitest run test/lib/api-client.multiregion.test.ts test/lib/api/organizations.test.ts test/e2e/multiregion.test.ts` — rewrote the fan-out-specific unit tests and confirmed the e2e suite (US + EU orgs, region routing for `org view`/`project list`/`issue list`, self-hosted fallback) still passes end to end - [x] Full `pnpm vitest run` — no new failures vs. `main` (confirmed via `git stash` comparison) Made with [Cursor](https://cursor.com) --------- Co-authored-by: Cursor <[email protected]>
3 participants
Adds the following fields to the control version of the org listing api in order to align to the cell version.
Updated the test to assert the control payload exactly matches the cell version
Split
ControlSiloOrganizationMappingSerializeroff fromControlSiloOrganizationSerializerso the user-identity-config call site stays minimal and doesn't pull in fields it doesn't need.