fix(grants): undo incorrect rename #3434
Conversation
📝 WalkthroughWalkthroughAdds and reworks metadata/annotations across API schemas, JS client zod descriptions, Go domain/repo/adapter types and mappings, Ent/DB schema columns, and migrations; updates entitlement V2 input shapes and client schemas to expose annotations/metadata consistently. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
openmeter/entitlement/driver/v2/mapping.go (1)
394-432: Map Metadata in the V2 grant‐create mapper
The API’s EntitlementGrantCreateInputV2 includes a Metadata field but MapAPIGrantV2ToCreateGrantInput never sets it. Add, immediately after the Annotations block:if g.Metadata != nil && len(lo.FromPtr(g.Metadata)) > 0 { grantInput.Metadata = make(models.Metadata) for k, v := range lo.FromPtr(g.Metadata) { grantInput.Metadata[k] = v } }openmeter/entitlement/driver/metered.go (1)
115-117: Populate CreateGrantInput.Metadata alongside Annotations
Write-path currently only maps API metadata intoAnnotations. To maintain round-trip symmetry, also setCreateGrantInput.MetadatawhenapiGrant.Metadata != nil:@@ openmeter/entitlement/driver/metered.go:115-117 - if apiGrant.Metadata != nil { - req.GrantInput.Annotations = AnnotationsFromMetadata(lo.FromPtr(apiGrant.Metadata)) - } + if apiGrant.Metadata != nil { + req.GrantInput.CreateGrantInput.Metadata = lo.FromPtr(apiGrant.Metadata) + req.GrantInput.Annotations = AnnotationsFromMetadata(lo.FromPtr(apiGrant.Metadata)) + }
🧹 Nitpick comments (1)
openmeter/credit/adapter/grant.go (1)
46-47: LGTM: persisting metadata on create.Storing
MetadataalongsideAnnotationsaligns with the migration path.If you want to avoid writing empty metadata, gate the setter:
- SetMetadata(grant.Metadata). + // Optional: only set when non-empty to avoid null/empty writes + SetMetadata(grant.Metadata).(Use a nillable setter if generated by ent:
SetNillableMetadata.)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumtools/migrate/migrations/atlas.sumis excluded by!**/*.sum
📒 Files selected for processing (17)
api/client/javascript/src/client/schemas.ts(4 hunks)api/client/javascript/src/zod/index.ts(3 hunks)api/openapi.cloud.yaml(4 hunks)api/openapi.yaml(4 hunks)api/spec/src/entitlements/v2/grant.tsp(2 hunks)openmeter/credit/adapter/grant.go(2 hunks)openmeter/credit/grant.go(2 hunks)openmeter/credit/grant/events_2.go(2 hunks)openmeter/credit/grant/grant.go(1 hunks)openmeter/credit/grant/repo.go(1 hunks)openmeter/ent/db/grant.go(0 hunks)openmeter/ent/db/grant/grant.go(1 hunks)openmeter/ent/schema/grant.go(0 hunks)openmeter/entitlement/driver/metered.go(1 hunks)openmeter/entitlement/driver/v2/mapping.go(1 hunks)tools/migrate/migrations/20250926145502_grant-annotations.up.sql(1 hunks)tools/migrate/migrations/20250926145930_null-grant-annotations.up.sql(1 hunks)
💤 Files with no reviewable changes (2)
- openmeter/ent/schema/grant.go
- openmeter/ent/db/grant.go
🧰 Additional context used
🧬 Code graph analysis (8)
openmeter/credit/grant/grant.go (3)
api/client/javascript/src/client/schemas.ts (1)
Metadata(11682-11682)api/api.gen.go (1)
Metadata(4943-4943)api/client/go/client.gen.go (1)
Metadata(4476-4476)
openmeter/credit/grant.go (3)
api/client/javascript/src/client/schemas.ts (1)
Metadata(11682-11682)api/api.gen.go (1)
Metadata(4943-4943)api/client/go/client.gen.go (1)
Metadata(4476-4476)
openmeter/entitlement/driver/metered.go (3)
api/client/javascript/src/client/schemas.ts (1)
Metadata(11682-11682)api/api.gen.go (1)
Metadata(4943-4943)api/client/go/client.gen.go (1)
Metadata(4476-4476)
openmeter/credit/adapter/grant.go (4)
api/client/javascript/src/client/schemas.ts (1)
Metadata(11682-11682)api/api.gen.go (1)
Metadata(4943-4943)openmeter/billing/profile.go (1)
Metadata(22-22)api/client/go/client.gen.go (1)
Metadata(4476-4476)
openmeter/ent/db/grant/grant.go (1)
openmeter/ent/db/entitlement/entitlement.go (1)
FieldMetadata(23-23)
openmeter/credit/grant/repo.go (4)
api/client/javascript/src/client/schemas.ts (1)
Metadata(11682-11682)api/api.gen.go (1)
Metadata(4943-4943)openmeter/billing/profile.go (1)
Metadata(22-22)api/client/go/client.gen.go (1)
Metadata(4476-4476)
openmeter/credit/grant/events_2.go (3)
api/client/javascript/src/client/schemas.ts (1)
Metadata(11682-11682)api/api.gen.go (1)
Metadata(4943-4943)api/client/go/client.gen.go (1)
Metadata(4476-4476)
openmeter/entitlement/driver/v2/mapping.go (3)
api/client/javascript/src/client/schemas.ts (1)
Metadata(11682-11682)api/api.gen.go (1)
Metadata(4943-4943)api/client/go/client.gen.go (1)
Metadata(4476-4476)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Artifacts / Container image
- GitHub Check: Lint
- GitHub Check: Test
- GitHub Check: Code Generators
- GitHub Check: Build
- GitHub Check: Analyze (go)
🔇 Additional comments (22)
openmeter/ent/db/grant/grant.go (1)
72-72: LGTM: metadata column included in ent Columns.Generated file looks consistent with the schema update exposing metadata.
openmeter/credit/grant/repo.go (1)
65-70: LGTM: RepoCreateInput now carries Metadata.Matches the domain/public and API surfaces.
openmeter/credit/grant/events_2.go (2)
56-57: LGTM: Domain event now uses Metadata.Consistent with removing annotation↔metadata shims.
83-84: LGTM: Event literal maps Metadata from domain.End-to-end propagation looks correct.
openmeter/credit/grant/grant.go (1)
45-46: LGTM: Metadata added to Grant.Public model aligns with API and repository updates.
openmeter/credit/grant.go (2)
30-31: LGTM: Metadata added to CreateGrantInput.Completes the input surface.
78-83: LGTM: Metadata propagated to RepoCreateInput.Persistence path wired correctly.
tools/migrate/migrations/20250926145930_null-grant-annotations.up.sql (1)
2-2: Scope UPDATE to only rows where annotations IS NOT NULL.-UPDATE "grants" SET "annotations" = NULL; +UPDATE "grants" SET "annotations" = NULL WHERE "annotations" IS NOT NULL;openmeter/credit/adapter/grant.go (1)
241-242: LGTM: mapping entity.Metadata back to domain.Keeps the read-path consistent with the write.
api/spec/src/entitlements/v2/grant.tsp (1)
61-64: Re-enabling metadata in V2 inputs is correct.Omitting only rollover/expiration now preserves
metadatafrom the base input. Matches the intent to undo the prior rename.api/client/javascript/src/zod/index.ts (2)
17576-17579: Same metadata description regression here
See the previous comment; this occurrence needs the same fix.
17824-17827: Same metadata description regression here
See the previous comment; this occurrence needs the same fix.api/openapi.cloud.yaml (4)
15806-15812: Schema metadata restored correctly.Thanks for reintroducing the dedicated metadata block here; this aligns the create input with the reverted terminology.
15828-15832: Annotation wording back in sync.The updated description/example accurately reflects the annotations field after the revert.
15897-15902: Grant metadata field reinstated.Good to see the response schema exposing metadata directly again; this keeps SDKs and backend models consistent.
15915-15919: Annotation docs reverted cleanly.The clarified annotation description/example matches the intended grant representation.
api/client/javascript/src/client/schemas.ts (4)
4782-4788: LGTM: Metadata field addition aligns with PR objectives.The addition of the
metadatafield with proper JSDoc documentation and example follows the PR's goal of undoing the incorrect rename. The metadata field is correctly typed ascomponents['schemas']['Metadata']and includes appropriate documentation.
4801-4801: LGTM: Annotations field documentation updated correctly.The annotations field documentation has been updated to use the correct terminology "Grant annotations" instead of the previous incorrect naming, and the example has been adjusted accordingly. This aligns with the PR's objective to fix the terminologically incorrect rename.
Also applies to: 4803-4803
4862-4868: LGTM: Consistent metadata field addition in second schema.The metadata field addition in this schema is consistent with the first occurrence, maintaining proper typing and documentation structure.
4879-4879: LGTM: Consistent annotations documentation update.The annotations field documentation has been consistently updated in this schema as well, maintaining the same corrected terminology and example format as in the first occurrence.
Also applies to: 4881-4881
api/openapi.yaml (2)
16147-16153: Reintroducingmetadatavia shared schema looks correct.Linking the grant create input back to the common
Metadataschema aligns with the revert plan and keeps payload expectations consistent across the spec.
16238-16243:EntitlementGrantV2now mirrors the metadata contract.Adding the same
Metadatareference on the read surface keeps the response and create/update inputs in sync, which is exactly what we need for the rollback.
tools/migrate/migrations/20250926145502_grant-annotations.up.sql
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
e2e/e2e_test.go (1)
899-902: Guard the annotations pointer before dereferencing.If this expectation ever fails and the field comes back empty, the test will panic with a nil dereference instead of giving a clear assertion failure. A quick
require.NotNilbefore the equality keeps the failure message readable.- require.Equal(t, metered.Id, (*grantListResp.JSON200)[0].EntitlementId) - require.Equal(t, api.Annotations{ - "issueAfterReset": true, - }, *(*grantListResp.JSON200)[0].Annotations) + require.Equal(t, metered.Id, (*grantListResp.JSON200)[0].EntitlementId) + require.NotNil(t, (*grantListResp.JSON200)[0].Annotations) + require.Equal(t, api.Annotations{ + "issueAfterReset": true, + }, *(*grantListResp.JSON200)[0].Annotations)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
api/client/javascript/src/client/schemas.ts(5 hunks)api/openapi.cloud.yaml(5 hunks)api/openapi.yaml(5 hunks)api/spec/src/entitlements/grant.tsp(1 hunks)e2e/e2e_test.go(1 hunks)openmeter/entitlement/driver/metered.go(1 hunks)openmeter/entitlement/driver/v2/mapping.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- openmeter/entitlement/driver/v2/mapping.go
🧰 Additional context used
🧬 Code graph analysis (2)
e2e/e2e_test.go (3)
api/client/javascript/src/client/schemas.ts (1)
Annotations(11347-11347)api/api.gen.go (1)
Annotations(1036-1036)api/client/go/client.gen.go (1)
Annotations(951-951)
openmeter/entitlement/driver/metered.go (2)
api/api.gen.go (2)
Metadata(4946-4946)Annotations(1036-1036)pkg/convert/ptr.go (1)
MapToPointer(16-21)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Quickstart
- GitHub Check: E2E
- GitHub Check: Build
- GitHub Check: Lint
- GitHub Check: Code Generators
- GitHub Check: Test
- GitHub Check: Analyze (go)
🔇 Additional comments (14)
api/spec/src/entitlements/grant.tsp (1)
122-127: Looks consistent with the revert.Adding
annotations?: Annotationson the read model keeps the grant surface aligned with the rest of the schema work around annotations vs. metadata, and the example payload matches the existing pattern. No issues spotted here.api/openapi.cloud.yaml (2)
15705-15710: Clarified annotations surface looks good.The Added
annotationsblock lines up with the reverted terminology and keeps the schema self-descriptive. Nice to see the example carried through for client guidance.
15812-15817: Metadata field reintroduction matches the revert goal.Re-exposing
metadataon the create payload keeps the contract consistent with the Go/domain structs from the revert—this looks correct.api/client/javascript/src/client/schemas.ts (5)
4695-4701: Annotations docstrings restored.Good to see the grant-level annotations description back in sync with the spec surface. Nothing further from my side.
4789-4795: Metadata create input back in place.Reintroducing the metadata bag here lines up with the revert and keeps the example aligned with what integrators expect.
4808-4813: Confirm whether create payload should still exposeannotations.Now that
metadatais back on the create input, do we still expect callers to send grant-levelannotationsas well? If the backend reverted to metadata-only, we should drop this field to avoid advertising a payload we no longer accept. Can you double-check the API contract?
4870-4875: Metadata patch surface restored.Thanks for wiring the metadata docs back into the patch input too; this keeps parity with the create call.
4886-4891: Same question about annotations as in Line 4808.If we end up removing
annotationsfrom the writable payloads, this block should follow suit.api/openapi.yaml (5)
16046-16051: Annotations field restored cleanly
Glad to see the grant surface now delegates to the sharedAnnotationsschema again—it keeps the response aligned with the domain model after undoing the rename, and the example clarifies expected keys.
16153-16158: Create input metadata back in place
Reintroducingmetadataon the create payload keeps parity with what callers actually persist and matches the revert strategy described in the PR summary.
16175-16179: Create-time annotations docs consistent
The description/example now mirrors the grant response wording, which should help API consumers distinguish annotations from metadata during writes.
16244-16249: V2 grant metadata coverage restored
The V2 representation now exposes metadata explicitly again, lining up with the Go/domain changes in this revert.
16264-16266: V2 annotations wording matches response
Maintaining the dedicated annotations field (with consistent example) confirms the revert didn’t regress the newer annotations support.openmeter/entitlement/driver/metered.go (1)
414-415: Conditional pointer restores omitempty behavior.Using
convert.MapToPointerensures both metadata and annotations staynilwhen empty, so theomitemptyJSON tags work again without forcingnullinto responses. Nice cleanup.
2 participants
Overview
Due to some confusion, metadata was moved under annotations which is terminologically incorrect. This patch simply undoes that change in a two step migration and renames. No functionality changes due to this.
Summary by CodeRabbit
New Features
Resource Changes
Documentation
Chores