Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 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 |
borbelyr-kong
added
the
release-note/ignore
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
openmeter/taxcode/adapter/mapping.go (1)
33-33: Consider copying annotations defensively before returning the domain object.Line 33 assigns the map by reference, which can create accidental shared mutations. Copying it (like metadata handling does) is safer.
♻️ Suggested refactor
func MapTaxCodeFromEntity(entity *db.TaxCode) (taxcode.TaxCode, error) { if entity == nil { return taxcode.TaxCode{}, errors.New("entity is required") } + var annotations models.Annotations + if entity.Annotations != nil { + annotations = make(models.Annotations, len(entity.Annotations)) + for k, v := range entity.Annotations { + annotations[k] = v + } + } + return taxcode.TaxCode{ NamespacedID: models.NamespacedID{ Namespace: entity.Namespace, ID: entity.ID, }, @@ AppMappings: lo.FromPtr(entity.AppMappings), Metadata: models.NewMetadata(entity.Metadata), - Annotations: entity.Annotations, + Annotations: annotations, }, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/taxcode/adapter/mapping.go` at line 33, The Annotations map is being assigned by reference (Annotations: entity.Annotations) which risks shared mutations; change the mapper (in mapping.go) to defensively copy entity.Annotations into a new map before assigning to the domain object's Annotations field (similar to how metadata is handled): create an empty map when entity.Annotations is non-nil, copy each key/value, and assign that new map (handle nil case by leaving domain Annotations nil or empty consistently).openmeter/taxcode/service.go (1)
39-39: Recommend guarding system-reserved annotations at the service boundary.Since this input now accepts annotations, adding validation for reserved/system-managed keys would help prevent accidental externalization of internal control flags later.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/taxcode/service.go` at line 39, The service now accepts Annotations (models.Annotations) but doesn't guard system-reserved keys; add validation in the service boundary (inside the methods in service.go that accept models.Annotations, e.g., Create/Update handlers or the service struct methods) to either reject requests containing reserved keys or strip them before persisting; implement a small reservedKeys set (e.g., "_system", "managed_by", "internal_*") and check incoming annotations against it, returning a clear validation error (bad request) or removing those keys before calling lower layers.openmeter/taxcode/service/taxcode_test.go (1)
33-54: Assert the persisted state after each operation.These subtests mostly stop at the returned error/value. A refetch after blocked update/delete, a refetch after successful update, and a not-found check after successful delete would lock down the real contract here: the DB row stayed unchanged when blocked and actually changed when allowed.
As per coding guidelines,
**/*_test.go: Make sure the tests are comprehensive and cover the changes. Keep a strong focus on unit tests and in-code integration tests.Also applies to: 67-90
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/taxcode/service/taxcode_test.go` around lines 33 - 54, After asserting errors for UpdateTaxCode and DeleteTaxCode, refetch the persisted taxcode via the service (e.g., env.Service.GetTaxCode or the equivalent read method) and assert the DB state: for the blocked UpdateIsBlocked and DeleteIsBlocked subtests confirm the taxcode row is unchanged (same Name and other fields) and still present; for the successful update subtest refetch and assert the Name changed to "updated name"; for the successful delete subtest refetch and assert the taxcode is not found (expect a not-found error). Use the same NamespacedID (models.NamespacedID{Namespace: ns, ID: tc.ID}) when refetching to verify persistence after calling UpdateTaxCode and DeleteTaxCode.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/v3/handlers/taxcodes/convert.go`:
- Around line 23-24: Reject any incoming label keys that start with the reserved
prefix "openmeter.io/" when mapping client Labels into Metadata (the code around
the goverter mapping comment "goverter:map Labels Metadata" and input handlers
such as CreateTaxCodeHandler / UpsertTaxCodeHandler or the converter function
that maps Labels->Metadata); implement a validation check on create/upsert input
that returns a ValidationIssue (per API handler conventions) listing the
offending keys rather than copying them into Metadata, and add the same check
wherever Labels are accepted so responses' openmeter.io/ annotations cannot be
spoofed by client input.
In `@openmeter/taxcode/service/taxcode.go`:
- Around line 47-49: Existing legacy tax_code rows have NULL annotations so
UpdateTaxCode and DeleteTaxCode guards (which call existing.IsManagedBySystem())
incorrectly allow edits/deletes; fix by either adding a migration backfill that
UPDATEs annotation=managed_by=system for rows that were created via
GetOrCreateByAppMapping, or modify IsManagedBySystem (or add a helper used by
UpdateTaxCode/DeleteTaxCode) to treat NULL annotation as system-managed for
records whose creation context matches GetOrCreateByAppMapping; ensure the
change references the existing functions GetOrCreateByAppMapping, UpdateTaxCode,
DeleteTaxCode and the annotations column so legacy rows are protected.
---
Nitpick comments:
In `@openmeter/taxcode/adapter/mapping.go`:
- Line 33: The Annotations map is being assigned by reference (Annotations:
entity.Annotations) which risks shared mutations; change the mapper (in
mapping.go) to defensively copy entity.Annotations into a new map before
assigning to the domain object's Annotations field (similar to how metadata is
handled): create an empty map when entity.Annotations is non-nil, copy each
key/value, and assign that new map (handle nil case by leaving domain
Annotations nil or empty consistently).
In `@openmeter/taxcode/service.go`:
- Line 39: The service now accepts Annotations (models.Annotations) but doesn't
guard system-reserved keys; add validation in the service boundary (inside the
methods in service.go that accept models.Annotations, e.g., Create/Update
handlers or the service struct methods) to either reject requests containing
reserved keys or strip them before persisting; implement a small reservedKeys
set (e.g., "_system", "managed_by", "internal_*") and check incoming annotations
against it, returning a clear validation error (bad request) or removing those
keys before calling lower layers.
In `@openmeter/taxcode/service/taxcode_test.go`:
- Around line 33-54: After asserting errors for UpdateTaxCode and DeleteTaxCode,
refetch the persisted taxcode via the service (e.g., env.Service.GetTaxCode or
the equivalent read method) and assert the DB state: for the blocked
UpdateIsBlocked and DeleteIsBlocked subtests confirm the taxcode row is
unchanged (same Name and other fields) and still present; for the successful
update subtest refetch and assert the Name changed to "updated name"; for the
successful delete subtest refetch and assert the taxcode is not found (expect a
not-found error). Use the same NamespacedID (models.NamespacedID{Namespace: ns,
ID: tc.ID}) when refetching to verify persistence after calling UpdateTaxCode
and DeleteTaxCode.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b48a85f0-8c70-4967-b971-8fefb0b35b51
⛔ Files ignored due to path filters (10)
openmeter/ent/db/entmixinaccessor.gois excluded by!**/ent/db/**openmeter/ent/db/migrate/schema.gois excluded by!**/ent/db/**openmeter/ent/db/mutation.gois excluded by!**/ent/db/**openmeter/ent/db/setorclear.gois excluded by!**/ent/db/**openmeter/ent/db/taxcode.gois excluded by!**/ent/db/**openmeter/ent/db/taxcode/taxcode.gois excluded by!**/ent/db/**openmeter/ent/db/taxcode/where.gois excluded by!**/ent/db/**openmeter/ent/db/taxcode_create.gois excluded by!**/ent/db/**openmeter/ent/db/taxcode_update.gois excluded by!**/ent/db/**tools/migrate/migrations/atlas.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (14)
api/v3/handlers/taxcodes/convert.gen.goapi/v3/handlers/taxcodes/convert.goopenmeter/ent/schema/taxcode.goopenmeter/taxcode/adapter/mapping.goopenmeter/taxcode/adapter/taxcode.goopenmeter/taxcode/annotations.goopenmeter/taxcode/errors.goopenmeter/taxcode/service.goopenmeter/taxcode/service/taxcode.goopenmeter/taxcode/service/taxcode_test.goopenmeter/taxcode/taxcode.goopenmeter/taxcode/testutils/env.gotools/migrate/migrations/20260402082013_add_annotations_to_taxcode.down.sqltools/migrate/migrations/20260402082013_add_annotations_to_taxcode.up.sql
| // goverter:map Labels Metadata | ||
| // goverter:ignore Annotations |
There was a problem hiding this comment.
Reserve the openmeter.io/ label namespace on write.
Create/upsert still copy client Labels straight into Metadata, while responses now emit annotations under the same openmeter.io/ prefix. That lets a caller send openmeter.io/managed_by=system on a user-managed tax code and get back a label that looks identical to a real system annotation. Please reject that prefix with a ValidationIssue, or otherwise reserve it on input, so the annotation namespace stays trustworthy.
As per coding guidelines, api/**/*.go: Follow HTTP handler patterns and ValidationIssue conventions as documented in the /api skill.
Also applies to: 29-32, 86-109
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/v3/handlers/taxcodes/convert.go` around lines 23 - 24, Reject any
incoming label keys that start with the reserved prefix "openmeter.io/" when
mapping client Labels into Metadata (the code around the goverter mapping
comment "goverter:map Labels Metadata" and input handlers such as
CreateTaxCodeHandler / UpsertTaxCodeHandler or the converter function that maps
Labels->Metadata); implement a validation check on create/upsert input that
returns a ValidationIssue (per API handler conventions) listing the offending
keys rather than copying them into Metadata, and add the same check wherever
Labels are accepted so responses' openmeter.io/ annotations cannot be spoofed by
client input.
| if existing.IsManagedBySystem() { | ||
| return taxcode.TaxCode{}, taxcode.ErrTaxCodeManagedBySystem | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for migrations/backfills or compatibility logic related to legacy taxcode annotations.
rg -n -C3 'AnnotationKeyManagedBy|managed_by|GetOrCreateByAppMapping|SetAnnotations|UPDATE .*tax.?code|ALTER TABLE .*tax.?code|annotations' \
--glob '*.go' --glob '*.sql' --glob '*.yaml' --glob '*.yml'Repository: openmeterio/openmeter
Length of output: 50379
🏁 Script executed:
cat tools/migrate/migrations/20260402082013_add_annotations_to_taxcode.up.sqlRepository: openmeterio/openmeter
Length of output: 290
🏁 Script executed:
cat tools/migrate/migrations/20260402082013_add_annotations_to_taxcode.down.sqlRepository: openmeterio/openmeter
Length of output: 255
🏁 Script executed:
head -200 openmeter/taxcode/service/taxcode.goRepository: openmeterio/openmeter
Length of output: 4647
Legacy tax codes need a backfill to enforce the new system-managed protection.
The migration adds the annotations column but doesn't backfill existing rows with managed_by=system. Since GetOrCreateByAppMapping only stamps that annotation on new creates, any tax codes that were auto-created before this rollout will have a NULL annotation. The new guards in UpdateTaxCode (line 47) and DeleteTaxCode (line 141) check existing.IsManagedBySystem(), which will return false for NULL values, letting legacy rows slip through editable/deletable.
Either add an UPDATE statement to the migration to backfill existing rows created by GetOrCreateByAppMapping, or implement a compatibility check that treats missing annotations as system-managed based on creation context.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openmeter/taxcode/service/taxcode.go` around lines 47 - 49, Existing legacy
tax_code rows have NULL annotations so UpdateTaxCode and DeleteTaxCode guards
(which call existing.IsManagedBySystem()) incorrectly allow edits/deletes; fix
by either adding a migration backfill that UPDATEs annotation=managed_by=system
for rows that were created via GetOrCreateByAppMapping, or modify
IsManagedBySystem (or add a helper used by UpdateTaxCode/DeleteTaxCode) to treat
NULL annotation as system-managed for records whose creation context matches
GetOrCreateByAppMapping; ensure the change references the existing functions
GetOrCreateByAppMapping, UpdateTaxCode, DeleteTaxCode and the annotations column
so legacy rows are protected.
1 participant
Summary by CodeRabbit
Release Notes