Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR centralizes label→metadata conversion via a new ConvertLabelsToMetadata (backed by labels.ToMetadata), makes label conversion error-aware, updates generated and handwritten converters to return/propagate errors, removes local per-file label helpers, and adjusts validation and tests to surface conversion/validation errors. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/v3/labels/validate.go (1)
13-16:⚠️ Potential issue | 🟡 MinorRemove unused sentinel errors.
ErrInvalidLabelKeyandErrInvalidLabelValuearen't referenced anywhere in the codebase. SinceValidateLabelno longer returns these after the refactor, you can clean them up.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/labels/validate.go` around lines 13 - 16, Remove the unused sentinel errors ErrInvalidLabelKey and ErrInvalidLabelValue from the file since ValidateLabel no longer returns them; search for ErrInvalidLabelKey and ErrInvalidLabelValue to confirm they have no external references, then delete their declarations from validate.go and run tests to ensure nothing else depends on those symbols.
🧹 Nitpick comments (3)
api/v3/labels/validate_test.go (1)
129-146: Good coverage of reserved prefix validation, consider adding more prefixes.The tests cover
openmeter_,kong_, andkonnect_reserved prefixes nicely. For completeness, you might want to add test cases for the other reserved prefixes defined inreservedPrefixMatcher(mesh,kic,kuma,insomnia).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/labels/validate_test.go` around lines 129 - 146, Add unit test rows in api/v3/labels/validate_test.go to cover the remaining reserved prefixes used by reservedPrefixMatcher: add test cases for "mesh_", "kic_", "kuma_", and "insomnia_" (each with a key like "mesh_key", "kic_key", "kuma_key", "insomnia_key", a sample value and wantErr: true) so the test table includes these prefixes and asserts validation fails for them.api/v3/labels/convert.go (1)
31-53: Good error aggregation pattern - but annotations branch may be dead code.The error collection and aggregation via
errors.Join+NewNillableGenericValidationErroris well-implemented.However, since
ValidateLabelnow rejects keys with theopenmeter_prefix (viareservedPrefixMatcher), the annotations branch (lines 38-43) will never execute - any label key starting withopenmeter_fails validation at line 32 and gets skipped. Only non-prefixed keys reach line 38, and they all go to metadata.This is intentional per the design that clients cannot set
openmeter_*labels via API. You could consider removing the dead branch for clarity, or adding a comment explaining it's preserved for symmetry/future use.♻️ Optional: Remove unreachable annotations branch
for k, v := range *labels { if err := ValidateLabel(k, v); err != nil { errs = append(errs, err) continue } - if strings.HasPrefix(k, AnnotationsPrefix) { - if annotations == nil { - annotations = make(models.Annotations) - } - - annotations[strings.TrimPrefix(k, AnnotationsPrefix)] = v - } else { - if metadata == nil { - metadata = make(models.Metadata) - } + if metadata == nil { + metadata = make(models.Metadata) + } - metadata[k] = v - } + metadata[k] = v }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/labels/convert.go` around lines 31 - 53, The annotations branch in the loop that builds MetadataAnnotations is unreachable because ValidateLabel (which uses reservedPrefixMatcher to reject keys starting with the openmeter_ prefix) will always error for annotation-like keys; update the code in the loop that iterates over *labels (the block referencing AnnotationsPrefix, annotations, and metadata) by either removing the annotations branch entirely and returning only metadata for now, or leaving the branch but adding a clear comment above it explaining that keys with the AnnotationsPrefix are rejected by ValidateLabel (via reservedPrefixMatcher) so the branch is retained only for symmetry/future use; ensure references to AnnotationsPrefix, annotations, metadata, MetadataAnnotations, and ValidateLabel remain consistent.api/v3/handlers/customers/convert.go (1)
46-50: Consider returningnilwhen there's an error.When
labels.ToMetadatafails, the function still returns&m(a pointer to the zero-value metadata) alongside the error. While the generated code does check errors before using the value, returningnilon error is a more defensive pattern that prevents accidental misuse.💡 Suggested tweak
func ConvertLabelsToMetadata(l *api.Labels) (*models.Metadata, error) { m, err := labels.ToMetadata(l) - - return &m, err + if err != nil { + return nil, err + } + return &m, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/handlers/customers/convert.go` around lines 46 - 50, The ConvertLabelsToMetadata function currently returns a pointer to m even when labels.ToMetadata returns an error; update ConvertLabelsToMetadata to return nil, err when err != nil and only return &m, nil on success. Locate ConvertLabelsToMetadata and the call to labels.ToMetadata so you return (nil, err) on failure and (&m, nil) on success to avoid returning a pointer to a zero-value models.Metadata when an error occurred.
🤖 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/labels/validate.go`:
- Around line 47-55: The validation error created when
reservedPrefixMatcher.MatchString(k) is true uses a misleading message; update
the models.NewValidationIssue call for the "invalid_label_key_prefix" case (the
block that appends to errs) to use a specific message such as "label key uses a
reserved prefix" instead of "label key must be a valid DNS_SUBDOMAIN format" so
the error reflects the reserved-prefix violation while keeping the same error
code, severity, field attribute (models.WithFieldString("labels")), and HTTP
status attribute.
---
Outside diff comments:
In `@api/v3/labels/validate.go`:
- Around line 13-16: Remove the unused sentinel errors ErrInvalidLabelKey and
ErrInvalidLabelValue from the file since ValidateLabel no longer returns them;
search for ErrInvalidLabelKey and ErrInvalidLabelValue to confirm they have no
external references, then delete their declarations from validate.go and run
tests to ensure nothing else depends on those symbols.
---
Nitpick comments:
In `@api/v3/handlers/customers/convert.go`:
- Around line 46-50: The ConvertLabelsToMetadata function currently returns a
pointer to m even when labels.ToMetadata returns an error; update
ConvertLabelsToMetadata to return nil, err when err != nil and only return &m,
nil on success. Locate ConvertLabelsToMetadata and the call to labels.ToMetadata
so you return (nil, err) on failure and (&m, nil) on success to avoid returning
a pointer to a zero-value models.Metadata when an error occurred.
In `@api/v3/labels/convert.go`:
- Around line 31-53: The annotations branch in the loop that builds
MetadataAnnotations is unreachable because ValidateLabel (which uses
reservedPrefixMatcher to reject keys starting with the openmeter_ prefix) will
always error for annotation-like keys; update the code in the loop that iterates
over *labels (the block referencing AnnotationsPrefix, annotations, and
metadata) by either removing the annotations branch entirely and returning only
metadata for now, or leaving the branch but adding a clear comment above it
explaining that keys with the AnnotationsPrefix are rejected by ValidateLabel
(via reservedPrefixMatcher) so the branch is retained only for symmetry/future
use; ensure references to AnnotationsPrefix, annotations, metadata,
MetadataAnnotations, and ValidateLabel remain consistent.
In `@api/v3/labels/validate_test.go`:
- Around line 129-146: Add unit test rows in api/v3/labels/validate_test.go to
cover the remaining reserved prefixes used by reservedPrefixMatcher: add test
cases for "mesh_", "kic_", "kuma_", and "insomnia_" (each with a key like
"mesh_key", "kic_key", "kuma_key", "insomnia_key", a sample value and wantErr:
true) so the test table includes these prefixes and asserts validation fails for
them.
🪄 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: ef7166d0-7dd2-4204-9656-000366288e44
📒 Files selected for processing (18)
api/v3/handlers/billingprofiles/convert.gen.goapi/v3/handlers/billingprofiles/convert.goapi/v3/handlers/customers/convert.gen.goapi/v3/handlers/customers/convert.goapi/v3/handlers/customers/create.goapi/v3/handlers/customers/upsert.goapi/v3/handlers/features/convert.goapi/v3/handlers/features/convert_test.goapi/v3/handlers/meters/convert.gen.goapi/v3/handlers/meters/convert.goapi/v3/handlers/meters/create.goapi/v3/handlers/subscriptions/convert.goapi/v3/handlers/taxcodes/convert.gen.goapi/v3/handlers/taxcodes/convert.goapi/v3/labels/convert.goapi/v3/labels/convert_test.goapi/v3/labels/validate.goapi/v3/labels/validate_test.go
chrisgacsal
force-pushed
the
refactor/labels
branch
from
4ca8963 to
95673e9
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
api/v3/labels/validate.go (1)
21-22: Consider updating the regex doc link abovereservedPrefixMatcher.The comment URL looks like the key/value format example, not the reserved-prefix matcher. Updating it would make this block easier to maintain later.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/labels/validate.go` around lines 21 - 22, The comment's regex101 URL above reservedPrefixMatcher is pointing to a different regex (key/value format) — update that URL to a regex101 link (or equivalent documentation) that uses the actual pattern ^(_|kong|konnect|insomnia|mesh|kic|kuma|openmeter) and includes representative test cases (matching and non-matching prefixes) so the inline doc reflects reservedPrefixMatcher's intent and is easier to maintain; locate the variable reservedPrefixMatcher and replace the comment link immediately above it with the corrected URL.
🤖 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/features/convert_test.go`:
- Around line 539-550: Add a negative test that calls labels.ToMetadata with an
invalid labels value and asserts it returns a non-nil error and nil (or empty)
metadata: create a new t.Run (e.g., "invalid labels returns error") that passes
a deliberately malformed api.Labels (such as a label with an empty key or
another value that triggers validation in labels.ToMetadata), then assert error
is not nil and meta is nil/empty using assert.Error and assert.Nil/assert.Empty
to cover the failure path introduced by the new (meta, err) signature.
In `@api/v3/labels/validate.go`:
- Around line 49-50: The validation error message for the
"invalid_label_key_prefix" code contains a typo ("nopt" should be "not"); update
the message string in api/v3/labels/validate.go (the entry with
"invalid_label_key_prefix") to read "label key must not have reserved prefix" so
the API returns the correct, professional wording.
---
Nitpick comments:
In `@api/v3/labels/validate.go`:
- Around line 21-22: The comment's regex101 URL above reservedPrefixMatcher is
pointing to a different regex (key/value format) — update that URL to a regex101
link (or equivalent documentation) that uses the actual pattern
^(_|kong|konnect|insomnia|mesh|kic|kuma|openmeter) and includes representative
test cases (matching and non-matching prefixes) so the inline doc reflects
reservedPrefixMatcher's intent and is easier to maintain; locate the variable
reservedPrefixMatcher and replace the comment link immediately above it with the
corrected URL.
🪄 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: 0bd0edcb-14c9-4ae1-b601-5bec170e4830
📒 Files selected for processing (15)
api/v3/handlers/billingprofiles/convert.gen.goapi/v3/handlers/billingprofiles/convert.goapi/v3/handlers/customers/convert.gen.goapi/v3/handlers/customers/convert.goapi/v3/handlers/customers/create.goapi/v3/handlers/customers/upsert.goapi/v3/handlers/features/convert.goapi/v3/handlers/features/convert_test.goapi/v3/handlers/meters/convert.gen.goapi/v3/handlers/meters/convert.goapi/v3/handlers/meters/create.goapi/v3/handlers/subscriptions/convert.goapi/v3/handlers/taxcodes/convert.gen.goapi/v3/handlers/taxcodes/convert.goapi/v3/labels/validate.go
✅ Files skipped from review due to trivial changes (3)
- api/v3/handlers/meters/create.go
- api/v3/handlers/customers/create.go
- api/v3/handlers/billingprofiles/convert.gen.go
🚧 Files skipped from review as they are similar to previous changes (6)
- api/v3/handlers/features/convert.go
- api/v3/handlers/subscriptions/convert.go
- api/v3/handlers/customers/upsert.go
- api/v3/handlers/taxcodes/convert.go
- api/v3/handlers/customers/convert.gen.go
- api/v3/handlers/customers/convert.go
chrisgacsal
force-pushed
the
refactor/labels
branch
from
95673e9 to
717a11c
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
api/v3/handlers/features/convert_test.go (1)
539-550:⚠️ Potential issue | 🟡 MinorPlease add one invalid-label failure-path test here.
Nice update for the new
(meta, err)flow, but we still only cover happy/nil paths. Sincelabels.ToMetadatais now error-aware, add one case with invalid labels and assert error + nil/empty metadata.Suggested test addition
func TestConvertMetadataLabels(t *testing.T) { @@ t.Run("nil labels returns nil metadata", func(t *testing.T) { meta, err := labels.ToMetadata(nil) assert.NoError(t, err) assert.Nil(t, meta) }) + + t.Run("invalid labels returns error", func(t *testing.T) { + l := api.Labels{"bad key": "value"} + meta, err := labels.ToMetadata(&l) + require.Error(t, err) + assert.Nil(t, meta) + }) }As per coding guidelines:
**/*_test.go: "Make sure the tests are comprehensive and cover the changes."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/handlers/features/convert_test.go` around lines 539 - 550, Add a failing-path unit test for labels.ToMetadata that constructs an invalid api.Labels value (e.g., a key or value that violates validation rules used by ToMetadata), calls labels.ToMetadata(&invalidLabels), and asserts that an error is returned and the returned metadata is nil or empty; place it alongside the existing tests ("labels to metadata" and "nil labels returns nil metadata") and name the subtest something like "invalid labels returns error" to verify ToMetadata's new (meta, err) error-aware behavior.
🤖 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/billingprofiles/convert.gen.go`:
- Around line 191-195: The create path is still using pV3LabelsToMapStringString
to copy source.Labels directly (allowing invalid labels through), while the
upsert path uses ConvertLabelsToMetadata and validates; update the create
converter to call ConvertLabelsToMetadata(source.Labels) (same as the upsert
flow), propagate and handle the returned error exactly as done where
billingUpdateProfileInput.Metadata is set, replace uses of
pV3LabelsToMapStringString in the create converter, and then regenerate this
file from the Goverter source so both create and upsert use the same validation
helper.
---
Duplicate comments:
In `@api/v3/handlers/features/convert_test.go`:
- Around line 539-550: Add a failing-path unit test for labels.ToMetadata that
constructs an invalid api.Labels value (e.g., a key or value that violates
validation rules used by ToMetadata), calls labels.ToMetadata(&invalidLabels),
and asserts that an error is returned and the returned metadata is nil or empty;
place it alongside the existing tests ("labels to metadata" and "nil labels
returns nil metadata") and name the subtest something like "invalid labels
returns error" to verify ToMetadata's new (meta, err) error-aware behavior.
🪄 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: e81667bd-a772-49ba-9a5a-359113395a75
📒 Files selected for processing (15)
api/v3/handlers/billingprofiles/convert.gen.goapi/v3/handlers/billingprofiles/convert.goapi/v3/handlers/customers/convert.gen.goapi/v3/handlers/customers/convert.goapi/v3/handlers/customers/create.goapi/v3/handlers/customers/upsert.goapi/v3/handlers/features/convert.goapi/v3/handlers/features/convert_test.goapi/v3/handlers/meters/convert.gen.goapi/v3/handlers/meters/convert.goapi/v3/handlers/meters/create.goapi/v3/handlers/subscriptions/convert.goapi/v3/handlers/taxcodes/convert.gen.goapi/v3/handlers/taxcodes/convert.goapi/v3/labels/validate.go
✅ Files skipped from review due to trivial changes (4)
- api/v3/handlers/customers/create.go
- api/v3/handlers/customers/upsert.go
- api/v3/handlers/meters/create.go
- api/v3/handlers/features/convert.go
🚧 Files skipped from review as they are similar to previous changes (6)
- api/v3/handlers/subscriptions/convert.go
- api/v3/handlers/billingprofiles/convert.go
- api/v3/handlers/meters/convert.gen.go
- api/v3/labels/validate.go
- api/v3/handlers/customers/convert.go
- api/v3/handlers/customers/convert.gen.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/labels/validate.go`:
- Around line 23-29: The validation issues currently use a generic "labels"
field which hides which specific label failed; update ValidateLabels to include
the concrete failing key in each NewValidationIssue call (e.g. use
models.WithFieldString(fmt.Sprintf("labels.%s", key)) or similar) so callers can
identify the bad entry—apply the same change for the other NewValidationIssue
invocations referenced (the blocks around lines 33-39 and 43-48) and ensure the
variable used is the actual label key checked in that validation branch.
🪄 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: e2bdb468-58bb-4482-83fc-dc3f30783d96
📒 Files selected for processing (3)
api/v3/handlers/billingprofiles/convert.gen.goapi/v3/handlers/billingprofiles/convert.goapi/v3/labels/validate.go
🚧 Files skipped from review as they are similar to previous changes (2)
- api/v3/handlers/billingprofiles/convert.gen.go
- api/v3/handlers/billingprofiles/convert.go
Comment on lines
+23
to
+29
| errs = append(errs, models.NewValidationIssue( | ||
| "invalid_label_key_format", | ||
| "label key must be in valid format", | ||
| models.WithFieldString("labels"), | ||
| models.WithCriticalSeverity(), | ||
| commonhttp.WithHTTPStatusCodeAttribute(http.StatusBadRequest), | ||
| )) |
Contributor
There was a problem hiding this comment.
Keep the failing label key in each validation issue.
Nice cleanup overall, but this now reports every failure against the same "labels" field. Once ValidateLabels joins errors from multiple entries, callers can’t tell which label actually failed. That’s a pretty rough regression for bulk validation. Consider attaching the concrete key (for example labels.<key> or an equivalent attribute) before returning the issue.
Also applies to: 33-39, 43-48
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/v3/labels/validate.go` around lines 23 - 29, The validation issues
currently use a generic "labels" field which hides which specific label failed;
update ValidateLabels to include the concrete failing key in each
NewValidationIssue call (e.g. use
models.WithFieldString(fmt.Sprintf("labels.%s", key)) or similar) so callers can
identify the bad entry—apply the same change for the other NewValidationIssue
invocations referenced (the blocks around lines 33-39 and 43-48) and ensure the
variable used is the actual label key checked in that validation branch.
borbelyr-kong
approved these changes
Apr 10, 2026
This was referenced Apr 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 join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Overview
Refactor
labelssupport forv3API.Summary by CodeRabbit
Bug Fixes
New Features