feat(identity): publish the identity-resolution OpenAPI contract and gate it - #2349
Conversation
The service had no way to publish its own contract, so the committed
document under docs/components/backend/ stayed the retired .NET one: it
declared routes the service answers 404 for, omitted the ones it serves,
and knew nothing of the operator correction surface.
Add the `openapi` subcommand and `api::openapi_document`, mirroring
analytics and authenticator — same offline emit, reusing the very
`build_operations` route table the live gear serves, so the document and
the router cannot diverge.
Declare the path and query parameters while here. Every templated route
named its parameters in the path and described none of them, which is not
a valid document: a generated client has nothing to fill `{source_id}`
from. A test now holds that invariant for the whole table.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Sergei Mozhaev <mozhaev.dev@gmail.com>
Regenerate the document from the service and add the drift gate beside the
analytics and authenticator ones, so it cannot go stale again — which is
the only reason the previous document survived the port.
What the refresh corrects: the seven operator correction routes and both
persons-sync operations appear for the first time; `/v1/persons/{email}`
and `POST /v1/persons-seed` are gone, the service having retired them; the
subchart parameter is spelled `{person_id}`, as the route actually is. The
internal S2S resolvers stay out, registered raw exactly so.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Sergei Mozhaev <mozhaev.dev@gmail.com>
The generator carried a tripwire for exactly this moment: identity was listed as Untrusted because its committed document was the .NET one, and `--check` fails the entry the moment that stops being true. It now does, so promote it to Generated. The models stop being a transcription of the Rust DTOs and become a contract test — a validation failure now says the service and its published document disagree, which is the whole point of generating them. Two consequences handled here. The `/internal/persons/*` resolvers are excluded from the document by design, so `IdentityValue` has no generated counterpart and moves to `identity_internal.py`, still hand-written. And the two operation journals, field-identical when one hand-written model served both, now have a model each — the admin listing test names the right one per path. The names are the contract's; the package re-exports them under the ones the suite already uses, so the rename stops at the schemas package. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Sergei Mozhaev <mozhaev.dev@gmail.com>
The hand-written identity models carried more than fields: `walk()` and `emails()` over a subchart, and `in_force` on the two temporal journals. Generated models carry the contract and nothing else, so re-exporting them under the old names left four suites calling methods that no longer exist — an AttributeError at run time, invisible to collection. Move that behaviour to free functions beside the tests that use it. Deliberately not wrapper subclasses: those would have to re-declare the fields they keep, and a hand-maintained field list beside a generated one is the drift the generator exists to prevent. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Sergei Mozhaev <mozhaev.dev@gmail.com>
Declaring only the path parameters left every list route's filters undocumented, so a generated client could reach the routes but not narrow them: `status`/`limit` on both operation journals, `person`/`role`/ `active`/`limit` on role assignments, `viewer`/`viewed`/`active`/`limit` on visibility grants, `depth`/`valid_at` on both subchart reads. Both revoke routes also accept an optional body carrying the reason, which the document did not mention at all. `RevokeReasonRequest` gains the schema derives the builder needs to describe it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Sergei Mozhaev <mozhaev.dev@gmail.com>
The subcommand is documented as needing no config, and builds the document from the route table alone — but the config was loaded before the command was chosen, so `--config <missing> openapi` failed on a file it never reads. Behaviour and documentation disagreed. Load the config per command instead. The commands that do need one keep validating the path exactly as before; only the offline emit stops depending on it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Sergei Mozhaev <mozhaev.dev@gmail.com>
|
Warning Review limit reached
Next review available in: 48 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe identity-resolution service now generates an offline OpenAPI document. CI detects specification drift. Stand schemas are generated from the contract, and identity tests use the generated models and shared view helpers. ChangesIdentity OpenAPI contract
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CI as openapi-specs workflow
participant CLI as identity-resolution Openapi CLI
participant API as openapi_document
participant Schemas as schema generator
participant Tests as stand identity tests
CI->>CLI: Generate OpenAPI JSON
CLI->>API: Build offline document
API-->>CLI: Return documented routes
CLI-->>CI: Write specification output
CI->>CI: Compare committed specification
CI->>Schemas: Generate identity.py
Schemas-->>Tests: Provide generated response models
Tests->>Tests: Validate identity API responses
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
src/backend/services/identity-resolution/src/api/mod.rs (1)
543-554: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider asserting the whole path set, not one path.
the_document_builds_without_state_or_backendspasses if every path except/v1/resolution/binddisappears. The CI drift gate catches such a loss, but the unit test states a weaker rule than its name suggests. Asserting the expected path count, or the full sorted key list, turns an accidental route removal into a failing unit test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/backend/services/identity-resolution/src/api/mod.rs` around lines 543 - 554, The test the_document_builds_without_state_or_backends should validate the complete expected OpenAPI path set rather than only checking /v1/resolution/bind. Assert the expected path count or, preferably, compare the full sorted path-key list so any accidental route removal fails the unit test.tests/stand/api/schemas/__init__.py (1)
73-112: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExport the sync single-operation model
PersonsSeedOperationResponseandPersonsSeedListResponseare exported asOperationandSeedOperationList;PersonsSyncOperationResponseandPersonsSyncListResponseare only exported under the sync-list name. Add an explicit export forPersonsSyncOperationResponseso tests do not mix seed and sync single-operation models. Consider renamingOperationtoSeedOperationwhen you add the sync export.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/stand/api/schemas/__init__.py` around lines 73 - 112, Update the schema exports in __init__.py to explicitly expose PersonsSyncOperationResponse as a sync single-operation model, alongside PersonsSyncListResponse. Rename the existing PersonsSeedOperationResponse alias from Operation to SeedOperation so seed and sync operation models remain distinct, and preserve the existing list aliases.tests/generate_schemas.py (1)
258-263: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConfirm identity datetime expectations, then remove the orphaned
Untrustedsupport.
identity-resolutiondoes not setunzoned_datetime, whileanalyticsdoes. KeepCreatePersonRoleRequest.valid_fromandCreateVisibilityRequest.valid_fromasAwareDatetimeonly if the suite does not construct those identities request models with naive or date-only values; otherwise identity and analytics should use the same timezone handling.
TARGETSno longer has anUntrustedentry, butcheck_untrusted()and theisinstance(target, Untrusted)branch still run fromTARGETS. RemoveUntrusted,Untrusted,check_untrusted(), the branch, and the stale instruction to retirestand/api/schemas/identity.py, or add a real service that still needs that path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/generate_schemas.py` around lines 258 - 263, Confirm how identity request models construct valid_from values, especially CreatePersonRoleRequest and CreateVisibilityRequest; preserve AwareDatetime only when inputs are timezone-aware, otherwise align identity’s unzoned_datetime handling with analytics. Remove the obsolete Untrusted type, its TARGETS entry, check_untrusted(), related isinstance branch, and stale instruction to retire identity.py unless an actual service still requires them.src/backend/services/identity-resolution/src/main.rs (1)
76-79: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove documentation comments from this service binary.
src/backend/services/identity-resolution/src/main.rsis under the backend rule that forbids Rust///docs in service binaries. Remove the docs onprint_openapi()at lines 158-160, and keep user-visible command help through the clap#[command(about = "...")]metadata rather than enum comments.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/backend/services/identity-resolution/src/main.rs` around lines 76 - 79, Remove the Rust documentation comments associated with the Openapi command and the print_openapi() function in the identity-resolution service binary. Preserve user-visible command help through the existing clap #[command(about = "...")] metadata, without replacing the removed comments with additional /// documentation.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/backend/services/identity-resolution/src/api/mod.rs`:
- Around line 380-393: Update the limit query-parameter descriptions in
src/backend/services/identity-resolution/src/api/mod.rs#L380-L393 and `#L437-L445`
to document both bounds and the default, using the established form “(1..=500,
default 100)” for assignments and grants. Also review the persons-seed and
persons-sync limit descriptions at `#L267-L273` and `#L306-L312` and update them to
state their verified lower bound, upper cap, and default; do not assume values
without confirming their clamp behavior.
In `@src/backend/services/identity-resolution/src/api/person_roles.rs`:
- Around line 91-96: The two RevokeReasonRequest structs collide on the
generated OpenAPI schema name. In
src/backend/services/identity-resolution/src/api/person_roles.rs:91-96, assign
the type a distinct schema name such as PersonRoleRevokeReasonRequest or replace
it with a shared type; apply the corresponding distinct-name or shared-type
change in src/backend/services/identity-resolution/src/api/visibility.rs:90-95,
preserving the existing wire fields.
In `@tests/stand/api/schemas/identity_internal.py`:
- Around line 1-11: Align the module documentation and models with both internal
routes: inspect the `by-external-id` and `by-email-override` resolver responses,
then either document both routes in `IdentityValue` when their shapes match, add
a distinct model for the override response when they differ, or explicitly state
that its body is not validated when no model is intended.
---
Nitpick comments:
In `@src/backend/services/identity-resolution/src/api/mod.rs`:
- Around line 543-554: The test the_document_builds_without_state_or_backends
should validate the complete expected OpenAPI path set rather than only checking
/v1/resolution/bind. Assert the expected path count or, preferably, compare the
full sorted path-key list so any accidental route removal fails the unit test.
In `@src/backend/services/identity-resolution/src/main.rs`:
- Around line 76-79: Remove the Rust documentation comments associated with the
Openapi command and the print_openapi() function in the identity-resolution
service binary. Preserve user-visible command help through the existing clap
#[command(about = "...")] metadata, without replacing the removed comments with
additional /// documentation.
In `@tests/generate_schemas.py`:
- Around line 258-263: Confirm how identity request models construct valid_from
values, especially CreatePersonRoleRequest and CreateVisibilityRequest; preserve
AwareDatetime only when inputs are timezone-aware, otherwise align identity’s
unzoned_datetime handling with analytics. Remove the obsolete Untrusted type,
its TARGETS entry, check_untrusted(), related isinstance branch, and stale
instruction to retire identity.py unless an actual service still requires them.
In `@tests/stand/api/schemas/__init__.py`:
- Around line 73-112: Update the schema exports in __init__.py to explicitly
expose PersonsSyncOperationResponse as a sync single-operation model, alongside
PersonsSyncListResponse. Rename the existing PersonsSeedOperationResponse alias
from Operation to SeedOperation so seed and sync operation models remain
distinct, and preserve the existing list aliases.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: eb19e72e-0c46-4795-b8a3-c7bc87f18560
📒 Files selected for processing (17)
.github/workflows/openapi-specs.ymldocs/components/backend/identity-resolution/openapi.jsonsrc/backend/services/identity-resolution/src/api/mod.rssrc/backend/services/identity-resolution/src/api/person_roles.rssrc/backend/services/identity-resolution/src/api/visibility.rssrc/backend/services/identity-resolution/src/main.rstests/generate_schemas.pytests/lib/insight_stand/coverage.pytests/pyproject.tomltests/stand/api/identity/test_admin.pytests/stand/api/identity/test_conflicts.pytests/stand/api/identity/test_query_contracts.pytests/stand/api/identity/test_subchart.pytests/stand/api/identity/views.pytests/stand/api/schemas/__init__.pytests/stand/api/schemas/identity.pytests/stand/api/schemas/identity_internal.py
| #[derive(Debug, Deserialize, ToSchema)] | ||
| pub struct RevokeReasonRequest { | ||
| #[serde(default)] | ||
| pub reason: Option<String>, | ||
| } | ||
| impl toolkit::api::api_dto::RequestApiDto for RevokeReasonRequest {} |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Two RevokeReasonRequest types collide on one generated schema component. Both files define a distinct type named RevokeReasonRequest and both now derive ToSchema. utoipa derives the component name from the bare type name, so both claim components.schemas.RevokeReasonRequest. The generated tests/stand/api/schemas/identity.py holds exactly one such class at Line 362, which confirms the collapse. The structs are field-identical today, so the wire contract is still correct; the contract becomes wrong the moment one side changes.
src/backend/services/identity-resolution/src/api/person_roles.rs#L91-L96: give the type a distinct component name with#[schema(as = PersonRoleRevokeReasonRequest)], or import a single shared type.src/backend/services/identity-resolution/src/api/visibility.rs#L90-L95: apply the matching change, either a distinct component name or a reference to the shared type.
📍 Affects 2 files
src/backend/services/identity-resolution/src/api/person_roles.rs#L91-L96(this comment)src/backend/services/identity-resolution/src/api/visibility.rs#L90-L95
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/backend/services/identity-resolution/src/api/person_roles.rs` around
lines 91 - 96, The two RevokeReasonRequest structs collide on the generated
OpenAPI schema name. In
src/backend/services/identity-resolution/src/api/person_roles.rs:91-96, assign
the type a distinct schema name such as PersonRoleRevokeReasonRequest or replace
it with a shared type; apply the corresponding distinct-name or shared-type
change in src/backend/services/identity-resolution/src/api/visibility.rs:90-95,
preserving the existing wire fields.
| """Identity shapes that no OpenAPI document describes — hand-written. | ||
|
|
||
| The service registers its two `/internal/persons/*` S2S resolvers as raw routes, | ||
| deliberately kept out of the generated document (the .NET contract excluded them | ||
| the same way). They therefore cannot be generated into `identity.py`, and a model | ||
| for them has to be written from the Rust DTO by hand. | ||
|
|
||
| `extra` stays at its default here, unlike the generated models: nothing | ||
| regenerates this file when the DTO gains a field, so forbidding the unknown would | ||
| turn a benign addition into a failing suite. | ||
| """ |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
The docstring names two internal routes, and the module supplies one model.
Lines 3-6 state that both /internal/persons/* resolvers stay out of the generated document and need hand-written models. Only by-external-id gets one. by-email-override has no model here.
If the override route returns the same shape, say so in the IdentityValue docstring and reference both routes. If it returns a different shape, add the second model, or state that the suite does not validate that body.
Also applies to: 20-34
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/stand/api/schemas/identity_internal.py` around lines 1 - 11, Align the
module documentation and models with both internal routes: inspect the
`by-external-id` and `by-email-override` resolver responses, then either
document both routes in `IdentityValue` when their shapes match, add a distinct
model for the override response when they differ, or explicitly state that its
body is not validated when no model is intended.
The `limit` descriptions stated the lower clamp and stopped there, so a generated client could not learn the page size it would actually get. All four list routes clamp to 1..=500 and default to 50; say so, in the form the review queue's own description already uses. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Sergei Mozhaev <mozhaev.dev@gmail.com>
|
Addressed in |
2 participants
Why
The committed
docs/components/backend/identity-resolution/openapi.jsonwas still the retired .NET contract: it declared routes the Rust service answers 404 for, omitted ones it serves, described none of the operator correction surface, and nothing in CI noticed. The service had no way to publish its own contract, so there was no path back to accuracy.What
The service emits its own document. New
openapisubcommand andapi::openapi_document(), mirroring analytics and authenticator — offline, no backends, built from the verybuild_operationsroute table the live gear serves, so the document and the router cannot diverge. It is genuinely offline: the config is now loaded per command, so the emit does not depend on a file it never reads.The contract is accurate again. 15 paths became 21. The seven operator correction routes and both persons-sync operations appear for the first time;
/v1/persons/{email}andPOST /v1/persons-seedare gone, the service having retired them; the subchart parameter is spelled{person_id}, as the route actually is. The internal S2S resolvers stay out, registered raw exactly so — a test holds that.Parameters are described, not just paths. Every templated route named its parameters in the path and described none of them, which is not a valid document: a generated client has nothing to fill
{source_id}from. All path parameters are declared now, along with the filters the list handlers accept (status/limiton both journals,person/role/active/limiton role assignments,viewer/viewed/active/limiton visibility grants,depth/valid_aton both subchart reads,limiton the review queue) and the optional revoke-reason body both DELETE routes accept. A test holds the templated-path invariant for the whole table.It cannot go stale again. Drift gate added to
openapi-specs.ymlbeside the analytics and authenticator ones.The stand models are generated from it.
generate_schemas.pycarried a tripwire for exactly this moment — identity was listed asUntrustedbecause its document was the .NET one, and--checkfails that entry the moment it stops being true. It now does, so the entry is promoted toGenerated. The models stop being a transcription of the Rust DTOs and become a contract test: a validation failure now says the service and its published document disagree.Two consequences handled here:
/internal/persons/*resolvers are excluded from the document by design, soIdentityValuehas no generated counterpart and moves toidentity_internal.py, still hand-written.walk(),emails(),in_force). Generated models carry the contract and nothing else, so that behaviour moves to free functions instand/api/identity/views.py, beside the tests that use it. Deliberately not wrapper subclasses: those must re-declare the fields they keep, and a hand-maintained field list beside a generated one is the drift the generator exists to prevent.The generated names are the contract's (
SubchartResponse, notSubchart); the schemas package re-exports them under the names the suite already uses, so the rename stops at that package.Verification
tests/--config <missing> openapinow succeeds and emits the document; commands that do need a config still validate the path exactly as before.Not verified locally: the four affected stand suites need a running stand, so they did not execute here — the layer that changed (models plus helpers) was verified by instantiating them. CI covers the rest.
Notes for review
Untrustedingenerate_schemas.pynow has no entries. Kept rather than deleted: it just did its job, and the next service arriving with a foreign contract is its user. Happy to remove it if reviewers prefer..standard_errorsstamps the full error set onto every operation, so gating would demand codes nothing observes. The stale justification incoverage.pyis corrected; turning the gate on is a change to the gate (backend(analytics): OpenAPI spec over-declares boilerplate error codes and under-declares real ones (.standard_errors) #1669), not to this PR.main.rsnow loads config per command, which diverges from analytics and authenticator. The divergence is deliberate — they share the same wart, and pulling them along belongs in its own change.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests