Id<P> type + ObjectId confinement (#156)#174
Merged
Merged
Conversation
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
The actual problem with #156 wasn't the hex string format — it's that `mongodb::bson::oid::ObjectId` (a type from the mongo crate) was being passed through service-layer models, transport handlers, webhook payloads, and MCP tool inputs. Every layer that mentioned an ID transitively depended on MongoDB. This PR fixes the architectural smell, not the format: - `core::public_id::Id<P>` wraps a 24-char ObjectId hex string. Wire format is `<prefix>_<hex>` (e.g. `aff_665a1b2c3d4e5f6a7b8c9d0e`), added on serialize, validated on deserialize. Marker `P` makes per-resource aliases (`AffiliateId`, `TenantId`, …) distinct types at compile time. - `Id::from_object_id(oid)` / `id.to_object_id()?` form the bridge. Repos own these calls; everyone else just passes the typed value. - No new ID format, no data migration, no backfill, no schema change. The underlying value is still the raw ObjectId hex — just with a prefix at the API boundary. Enforcement is structural, not advisory: - `architecture_tests::object_id_confined_to_storage_layer` walks `src/` and bans the word `ObjectId` everywhere except an allowlist: `**/repo.rs`, `migrations/**`, `core/db.rs`, `core/public_id/mod.rs`, `app.rs`, `main.rs`, `*_tests.rs`. New files inherit the rule. - `OBJECT_ID_BACKLOG` lists existing violator files (45 entries); follow-up commits remove them one at a time. - `object_id_backlog_entries_still_have_violations` is the symmetric test: a file on the backlog must still contain `ObjectId`, or the test fails. The backlog can only shrink. Files added/modified: - `core/public_id/mod.rs` — type, trait, all 10 marker types, impls (Serialize/Deserialize, Display, FromStr, Hash, Ord, utoipa `ToSchema` + `PartialSchema`, schemars `JsonSchema` under `mcp`) - `core/public_id/models.rs` — `ParseIdError`, 10 type aliases - `core/public_id/public_id_tests.rs` — 18 unit tests - `core/mod.rs` — register `pub mod public_id` - `architecture_tests.rs` — new test + backlog + 6 parser tests - `CLAUDE.md` — new "Public identifiers" section Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
saltyskip
force-pushed
the
feat/public-id-foundation
branch
from
dd9a6e6 to
343522d
Compare
saltyskip
changed the title
The whole reason we were planning a Doc/Response split per resource: BSON
ObjectId can't deserialize into a string-backed type, and our `Id<P>` is
string-backed. Without this commit, every `models.rs` would need a private
storage struct in repo.rs plus a conversion helper.
With this commit, `Id<P>` branches on `Serializer::is_human_readable()`:
- JSON / OpenAPI / MCP wire format: prefixed string `aff_<hex>`
- BSON (raw, non-human-readable — the path the mongodb driver uses):
native ObjectId binary
So a struct `Affiliate { id: AffiliateId, tenant_id: TenantId, name: String }`
serializes correctly to both MongoDB (BSON ObjectIds for `_id` and `tenant_id`)
and to HTTP responses (prefixed strings). No Doc/Response split, no
conversion helpers, no per-resource refactor beyond changing field types.
Per-resource migration now collapses to mechanical search/replace:
- `id: ObjectId` → `id: AffiliateId` (per type)
- `&ObjectId` parameters → `&AffiliateId`
- `Path<String>` + manual parse → `Path<AffiliateId>`
- `ObjectId::new()` → `AffiliateId::new()`
Note on `bson::to_bson` vs the driver path: bson 2.x's `to_bson` defaults
to human_readable=true (returns a string for our type). The actual mongodb
driver uses `to_raw_document_buf` internally, which is non-human-readable
and produces ObjectId binary. The tests exercise the raw path.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Add `Id::new()` (generates a fresh ObjectId hex) and `Default` impl.
- Add `From<Id<P>> for Bson` so `doc! { "_id": id }` produces a native
BSON ObjectId without round-tripping through serde.
Affiliates resource migrated end-to-end:
- `services/affiliates/models.rs` — `Affiliate { id: AffiliateId, tenant_id: TenantId, .. }`,
`AffiliateDetail.id` typed, `MintedCredential.affiliate_id` typed.
File no longer imports `mongodb::bson::oid::ObjectId`.
- `services/affiliates/repo.rs` — trait signatures take `&TenantId` /
`&AffiliateId` (the `From<Id<P>> for Bson` impl lets the existing
`doc!` queries work unchanged).
- `services/affiliates/service.rs` — CRUD methods take `AffiliateId`;
bridges to `ctx.tenant_id` (still `ObjectId` in AuthContext, 136
call sites — separate migration) via `.into()` at the boundary.
Credential methods (`mint_credential`, `list_credentials`,
`revoke_credential`) still take `ObjectId` until secret_keys migrates.
- `api/affiliates/routes.rs` — CRUD route handlers use
`Path<AffiliateId>` directly; no more manual `ObjectId::parse_str`.
`to_detail` just clones the typed id.
- `services/links/service.rs` and `services/billing/repos/resource_counts_adapter.rs`
bridge their `&ObjectId` to the new typed signature with `.into()`.
- `tests/common/mocks/affiliates.rs` updated to match the new trait.
Removed `src/services/affiliates/models.rs` from `OBJECT_ID_BACKLOG`.
`service.rs` and `routes.rs` stay on the backlog (credential paths
still use ObjectId pending the secret_keys migration).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…xt → services
The marker-type safety story now actually delivers: there is one typed `TenantId`,
one typed `UserId`, one typed `AffiliateId` across the entire codebase. They flow
unchanged from the auth middleware through services down to repo boundaries.
Restructured `Id<P>`:
- Stores `ObjectId` directly instead of a `String` hex (fixes the agent's
perf + reachable-`expect` concerns). `to_object_id()` is now infallible.
- `Copy` derived — eliminates `clone()` noise at every plumbing call site.
- Removed the blanket `impl<P: IdPrefix> From<ObjectId> for Id<P>` —
conversions must name the target type via `TenantId::from_object_id(oid)`,
so cross-resource mixups are caught at review.
- Dropped `Default` — defaults must be cheap/deterministic; `new()` mints
a fresh ObjectId (clock + counter) and is explicit.
- `SecretKeyIdMarker::PREFIX` changed from `"sk"` to `"skid"` to avoid
collision with Stripe's `sk_live_` / `sk_test_` muscle memory.
Unification:
- `api/auth/models.rs` re-exports `core::public_id::{TenantId, UserId}` —
the old `TenantId(pub ObjectId)` / `UserId(pub ObjectId)` newtypes are
gone. `Extension<TenantId>` is the typed value end-to-end.
- `auth/middleware.rs` constructs `TenantId::from_object_id(oid)` /
`UserId::from_object_id(oid)` once at the auth boundary.
- `AuthContext.tenant_id: TenantId`, `Principal::User { user_id: UserId, .. }`,
`ResourceScope::Affiliate { affiliate_id: AffiliateId }`.
- ~28 axum extension call sites flow the typed value through (no `.0` access).
- Affiliate credentials (`mint_credential` / `list_credentials` /
`revoke_credential`) take `AffiliateId` — affiliates is now fully migrated.
Bridges to un-migrated layers:
- `ctx.tenant_id.as_object_id()` for repos that still take `&ObjectId`
(most repos; per-resource migration is follow-up work).
- `ctx.tenant_id.to_object_id()` for owned ObjectId needs (BSON metadata,
Sentry tags via `.to_string()`).
- `affiliate_id.to_object_id()` when calling secret_keys repo (still ObjectId).
Architecture test:
- Tightened `*_tests.rs` allowlist: a `*_tests.rs` file now only counts as
a sibling-test if there's another non-test `.rs` file in the same
directory. Closes the "name any file `_tests.rs` to bypass" escape.
- `architecture_tests.rs` itself added to the explicit allowlist (mentions
the word in parser tests and error messages).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
3 tasks
* Migrate domains, apps, conversions to typed Id<P>
- domains: `Domain { id: DomainId, tenant_id: TenantId }`. Routes use
`Path<DomainId>` flow. `services/domains/models.rs` off the backlog.
- apps: `App { id: AppId, tenant_id: TenantId }`. `AppDetail.id: AppId`.
Routes use `Path<AppId>`. `resolve_tenant_from_host` returns
`Option<TenantId>`. `apps/models.rs` and `api/apps/routes.rs` off
the backlog.
- conversions: `Source { id: SourceId, tenant_id: TenantId }`,
`ConversionEvent.id: Option<ConversionEventId>`,
`ConversionMeta { tenant_id: TenantId, source_id: SourceId }`,
`SourceDetail.id: SourceId`, `CreateSourceResponse.id: SourceId`.
MCP `CreateSourceOutput` / `SourceSummary` typed too.
`ConversionDedup` stays on backlog (internal-only docu).
Cross-resource bridges:
- `links/routes.rs` calls `domain.tenant_id.as_object_id()` when
passing to repos still on backlog.
- `apps/routes.rs` passes `tenant_id.as_object_id()` to apps repo.
- `conversions/service.rs` bridges to ObjectId at the ingest boundary
(tenant_id, source_id) since the time-series repo still uses ObjectId.
Tests:
- `links/service_tests.rs` Domain mock uses typed Id<P>.
- `conversions/parsers_tests.rs` source factory uses typed Id<P>.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* Migrate webhooks, app_users, install_events to typed Id<P>
- webhooks: `Webhook { id: WebhookId, tenant_id: TenantId }`,
`WebhookDetail.id: WebhookId`, `CreateWebhookResponse.id: WebhookId`.
Service takes `WebhookId` for `create_webhook`. Off the backlog.
- app_users: added `AppUserIdMarker` (prefix `appusr`). `AppUserDoc`
uses `Option<AppUserId>` / `TenantId`. Off the backlog.
- install_events: added `InstallEventIdMarker` (prefix `iev`).
`InstallEvent` uses `Option<InstallEventId>` / `TenantId`. Off the backlog.
Also added `LinkInternalIdMarker` (prefix `lnk`) for the upcoming links
model migration — distinct from the public `link_id` vanity slug.
Bridges:
- `webhooks/routes.rs`: comparisons use `w.id.to_object_id()`.
- `install_events/repo.rs`: bridges `*tenant_id` to `TenantId::from_object_id`.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* Migrate auth/{sessions,tenants,users,oauth}/models.rs + add session markers
New markers:
- `AuthSessionIdMarker` (`sess_`) — user auth sessions
- `OAuthSessionIdMarker` (`osess_`) — OAuth flow state
Models migrated to typed Id<P>:
- `SessionDoc { id: AuthSessionId, user_id: UserId, tenant_id: TenantId }`
- `ResolvedSession`, `SignInOutcome` use typed IDs
- `TenantDoc.id: Option<TenantId>`
- `UserDoc { id: Option<UserId>, tenant_id: TenantId }`
- `UserDetail`, `InviteResult`, `VerifyResult` use typed IDs
- `OauthCallbackOutcome { user_id: UserId, tenant_id: TenantId }`
Axum extension unification:
- `api::auth::models::SessionId` is now `pub use crate::core::public_id::AuthSessionId as SessionId` — same trick as Tenant/User: one type, end-to-end.
AuthContext + Principal:
- `Principal::User { user_id: UserId, session_id: AuthSessionId }`
- `AuthContext::for_session` takes typed args
Bridges:
- `tenants/service.rs::create_blank` mints `TenantId::new()` internally, returns `ObjectId` to existing callers (one-line bridge until call sites migrate).
- `users_service::create_tenant_with_verified_owner` still returns ObjectIds; sessions and OAuth services bridge with `from_object_id` at the call site.
- `stripe_webhook.rs::try_resolve_tenant` `.map(|t| id.to_object_id())` bridges.
- Test mocks updated to mirror typed IDs.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* Cleanup unused imports + remove sessions/oauth services from backlog
Lib + lib tests pass (205 tests, including arch tests). Integration tests
under `tests/` need follow-up updates to common mocks for the typed IDs —
they reference patterns that changed in the auth/sessions migration.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* Migrate affiliate credential routes + link CreateRequest affiliate_id
- `api/affiliates/routes.rs`: credential route handlers use `Path<AffiliateId>`
instead of `Path<String>` + manual `ObjectId::parse_str`.
- `api/webhooks/routes.rs`: `delete_webhook` and `patch_webhook` use
`Path<WebhookId>`.
- `services/links/models.rs`: `CreateLinkRequest.affiliate_id`,
`BulkLinkTemplate.affiliate_id`, `LinkDetail.affiliate_id` typed as
`Option<AffiliateId>`. Drops the custom `serialize_opt_object_id_as_hex`
helper for affiliate_id — `Id<P>::serialize` is format-conditional.
- `services/links/service.rs`: bridges request `Option<AffiliateId>` to
storage `Option<ObjectId>` via `.map(|a| a.to_object_id())`. The
response builder maps the other direction.
- Integration test fixtures updated to use the prefixed format
(`aff_<hex>`, `wh_<hex>`) via `Id<P>::new().to_string()` /
`Id<P>::parse(&s)`.
Tests: 205 lib + 179 doc + 147 integration all pass.
Backlog now 30 (was 44 at start of #175).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* Migrate secret_keys + publishable_keys models + their delete routes
- secret_keys/models.rs: `SecretKeyDoc { id: SecretKeyId, tenant_id: TenantId,
created_by: UserId }`. `CreatedKey.id`, `KeyDetail.{id,created_by}` typed.
- publishable_keys/models.rs: `SdkKeyDoc { id: PublishableKeyId, tenant_id: TenantId }`.
- secret_keys delete + sdk_keys revoke + affiliate credential revoke routes all
use `Path<SecretKeyId>` / `Path<PublishableKeyId>` (no more manual parse_str).
- mint_scoped (secret_keys::service) constructs via `SecretKeyId::new()`.
- Middleware `find_secret_key_doc` helper returns ObjectIds via `to_object_id()`
at the boundary.
- Test fixtures + mocks updated.
205 lib + 179 doc + 147 integration tests pass.
Backlog now 28 (was 30).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* Migrate links.Link, conversions.ConversionDedup, billing/auth/usage models
- `services/links/models.rs::Link`: `id: LinkInternalId`, `tenant_id: TenantId`,
`affiliate_id: Option<AffiliateId>` typed end-to-end.
- `services/auth/usage/models.rs::UsageDoc`: typed `Option<UsageRowId>` /
`Option<SecretKeyId>` (new `UsageRowIdMarker` with prefix `usage`).
- `services/conversions/models.rs::ConversionDedup`: `id: ConversionDedupId`,
`tenant_id: TenantId` (new `ConversionDedupIdMarker` with prefix `cdedup`).
- `services/auth/publishable_keys/models.rs::SdkKeyDoc` typed too.
- `services/billing/models.rs::EventCounterDoc.tenant_id` typed.
- Link list cursor parses via `LinkInternalId::parse` then `.to_object_id()`.
- `api/lifecycle/routes.rs` builds `IdentifyEventPayload.tenant_id` as
`TenantId::from_object_id(*tenant_id).to_string()` so the webhook wire
format is the prefixed string.
- Many test fixtures + repo internals updated with `from_object_id` bridges.
- 205 lib + 179 doc + 147 integration tests all pass.
Backlog: 24 (was 28).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* Drop analytics + publishable_keys routes + auth/secret_keys/lifecycle/webhooks routes from backlog
* Migrate KeyScope::Affiliate.affiliate_id to AffiliateId
* Migrate AuthKeyId to wrap SecretKeyId + cached webhook lookup uses TenantId
* Migrate Principal::SecretKey.key_id + tenants::create_blank to typed
* Migrate links/routes lookup_tenant_domain + auth/users/routes + permissions/models
* Migrate sessions/service.revoke + conversions/routes path types
* Migrate affiliates credential service methods to typed Ids
* Migrate auth/users service: delete + create_tenant_with_verified_owner typed
* Middleware validate_api_key returns typed (TenantId, SecretKeyId)
* Migrate stripe_webhook helpers to TenantId
* Migrate secret_keys service to typed TenantId/UserId/SecretKeyId
* Migrate QuotaChecker trait to typed TenantId
* Migrate conversions service to typed TenantId/SourceId
* Migrate TierResolver trait + BillingService to typed TenantId
* Migrate links models (ClickMeta, AttributionEventMeta, CreateLinkInput) to typed IDs
* Migrate links service to typed TenantId/AffiliateId — backlog drained to zero
* Docs: update prefixed-ID examples in OpenAPI + CLAUDE.md/AGENTS.md
---------
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary
Phase 1 of #156 — introduces
core::public_id::PublicId<P: IdPrefix>, a typed wrapper around<prefix>_<22-char-base62>IDs (~131 bits entropy). No DB migration, no per-resource cutover, no breaking changes — this PR ships the type, schema integrations, enforcement scaffolding, and docs. Phase 2 adds the storage column; Phase 3 migrates resources one at a time; Phase 4 locks in.AffiliateIdandWebhookIdare distinct — passing one where the other is expected fails to build.utoipa::ToSchema+PartialSchemaimpls produce string schemas with regexpattern.schemars::JsonSchemamirrored under themcpfeature.architecture_tests::public_response_types_must_not_expose_object_idsscans everymodels.rsforpub _: ObjectIdfields. 17 existing violator files seedPUBLIC_ID_CLEANUP_BACKLOG; new files inherit enforcement.Deferred from the issue's Phase 1 list
clippy.tomldisallowed-methodsforObjectId::to_hexmoves to Phase 4 (lock-in). Adding it now would force#[allow(clippy::disallowed_methods)]on every existing call site — conflicts with CLAUDE.md's "never suppress warnings" rule. Once the backlog is empty in Phase 4, clippy bites without exceptions.What's NOT in this PR
public_id: Stringstorage column + eager backfill — Phase 2TenantId/UserId/SecretKeyId/PublishableKeyIdaliases — naming/collision review deferred to their Phase 3 PRs (reserved-prefix list incore/public_id/mod.rs)idfields, audit Sentry tags — Phase 4Test plan
cargo fmt -- --checkcargo clippy --all-targets -- -D warningscargo test --lib— 205 passing (23 new public_id tests, 2 new arch tests, 7 new parser tests)cargo build --no-default-features --features apicargo build --no-default-features --features mcpCloses part of #156. Next PR: Phase 2 storage migration.
🤖 Generated with Claude Code