feat: Person UUID, related OIDC claims, and apis - #11415
Merged
jennifer-richards merged 18 commits intoAug 14, 2026
Merged
Conversation
rjsparks
marked this pull request as ready for review
August 10, 2026 15:36
jennifer-richards
requested changes
Aug 11, 2026
jennifer-richards
left a comment
jennifer-richards
left a comment
Member
There was a problem hiding this comment.
A few high level things I think we should fix (details are inline):
- fix / simplify the DRF serializers to get rid of polymorphic complications and @extend_schema` misuse
- the oidc claims should just be in a single scope
- guard person creation with transactions
- avoid registering the custom path converter on import
- consider allowing retries on the uuid push task
We should manually test that the schema that comes out of this by running spectacular is correct (if you haven't done that already) and can be used to generate a client.
There are a bunch of other nit-level comments. The ones related to using the Person uuid @property accessors in tests could be ignored if it's a lot of manual work, but I'm assuming it's simple to generate code incorporating that comment (if you agree).
Reformatting the import into a parenthesized block moved the ignore comment to
the closing paren. mypy reports the simple_history HistoricalPerson and
HistoricalEmail attribute errors against the 'from ... import (' line, so the
comment has to sit there to suppress them.
Registering it in ietf/utils/converters.py made importing that module a side effect, and Django refuses to register a converter twice, so naming the converter from a second URLconf was a latent error. Define it there, register it once in ietf/urls.py before urlpatterns names it.
A Person with no primary UUID cannot be named to any external system, so the create and the assign_primary_uuid() that follows it have to succeed or fail together. Covers all three production creation sites, including the draft submission one, and wraps the surrounding aliases and nominee email so a failure part way leaves nothing half-built.
The resolved/unknown split needed a PolymorphicProxySerializer, which is an annotation helper rather than a real serializer, so the endpoints hand-built dicts and told consumers not to infer the outcome from which fields were present. Use one entry serializer per endpoint instead, discriminated on status, with the identifier fields nullable and always present, and actually serialize responses through it so the schema cannot drift from what is returned. Drops the ResolvedStatusEnum/UnknownStatusEnum overrides that existed only to keep the two single-valued status enums apart - there is now one StatusEnum. The entry fields are not read_only because read_only implies required=False, which left a generated client treating even status as optional. Also annotates retrieve with @extend_schema_view rather than overriding it just to call super().
Routing this lookup through a GenericViewSet forced the handler to be named create, because that is what SimpleRouter maps POST to on a collection route. Nothing is created: the view returned 200 while drf-spectacular inferred 201 from the action name, so the schema advertised a status code the endpoint never sends and a generated client would treat the real response as unexpected. An APIView.post() returns 200 with no annotation gymnastics. The viewset was buying nothing else - no retrieve, no mixins, and an empty queryset. api_key auth is unaffected, since HasApiKey just reads api_key_endpoint off the view. The URL is unchanged. Its name loses the router's -list suffix, and the schema test now checks the declared success codes so this cannot drift again.
Splitting the current identifier and the superseded ones across two scopes was finer-grained than any consumer needs - there is no case for granting one and not the other, and the prior list is far too short for response size to matter. Also corrects the scope description, which claimed the prior list included the identifier in use now. It does not, and datatracker_uuid is where that lives.
The job logged that every Person has exactly one primary while only looking for Persons with none. The partial unique constraint should make more than one impossible, so finding one means the data is grossly inconsistent and worth reporting - and ensure_primary_uuid() cannot repair that case, since it would be picking a survivor arbitrarily, so it is reported and skipped rather than silently 'fixed'. Same change in the base-test-data check.
Celery's default is three attempts over well under a second, which is cheap enough on the request path that changed the UUID set and is the difference between riding out a broker blip or failover and dropping the push on the floor. The broker-error catch still keeps an outright outage from failing the datatracker operation.
Neither endpoint had an APP_API_TOKENS entry in the container config, so every call to them from a dev environment got a 403.
… accessors PersonFactory now makes its Person's primary UUID with PersonUUIDFactory instead of calling assign_primary_uuid() itself, so all UUID handling in tests goes through the factories. PersonFactory(primary_uuid=False) covers the no-UUIDs-at-all case, which no production path can reach, replacing the tests that created a Person and then deleted its UUID rows. Tests now assert through Person.primary_uuid and Person.prior_uuids rather than querying uuids directly, so the accessors are the example to copy. Direct queries remain only where they are the point: the test proving the accessors agree with the rows, and setup that deliberately builds inconsistent state. Also drops the retry kwarg assertion that went with the old retry=False.
A merge stamps every UUID it moves with the same time, so ordering the prior list on time alone left the order undefined in exactly the case where there is more than one prior. Break ties on the UUID, which also makes the claim that uuid_sets_for() matches this accessor true - it was already ordering on both.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feat/uuid #11415 +/- ##
============================================
Coverage ? 88.67%
============================================
Files ? 335
Lines ? 44994
Branches ? 0
============================================
Hits ? 39897
Misses ? 5097
Partials ? 0 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
jennifer-richards
left a comment
jennifer-richards
left a comment
Member
There was a problem hiding this comment.
Thanks for the updates. One question/suggestion to clarify a comment, plus a nit just to point at (in case it bothers you more than it bothers me and you want to adjust the logging, I think it's ok as-is)
The previous comment justified the tie-break by claiming a merge gives every UUID it moves the same timestamp. It does not: merge_persons() moves them with a queryset update that names only person and primary, and PersonUUID.time is a per-row default with no auto_now, so each keeps its original timestamp. The tie-break stands on narrower ground - it makes the order total instead of leaving equal timestamps to the database, and matches the ordering uuid_sets_for() already used - so only the comment changes.
jennifer-richards
approved these changes
Aug 13, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.