refactor: use helpers for converting labels#4097
Merged
chrisgacsal merged 2 commits intomainfrom Apr 8, 2026
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughCentralizes metadata↔labels conversion by adding a new Changes
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 |
chrisgacsal
force-pushed
the
refactor/api-labels
branch
from
ab53d24 to
ed9f06b
Compare
chrisgacsal
requested review from
borbelyr-kong,
gergely-kurucz-konghq and
tothandras
![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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
api/v3/handlers/meters/convert.gen.go (1)
15-29:⚠️ Potential issue | 🟠 MajorThe meter label round-trip is still asymmetric.
Line 53 now includes annotation-backed labels in the response, but Line 28 still collapses request labels into
Metadataonly. Re-posting a returned meter will lose the annotation semantics on write. Please update the source converter to split labels back into metadata + annotations and regenerate.As per coding guidelines: "Never edit generated files with '// Code generated by X, DO NOT EDIT.' headers — regenerate them instead using the specified make targets" and "Define converter interfaces in convert.go and regenerate convert.gen.go using 'make generate' via Goverter".
Also applies to: 42-53
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/handlers/meters/convert.gen.go` around lines 15 - 29, The Create->Model converter ConvertFromCreateMeterRequestToCreateMeterInput currently collapses all request labels into Metadata via pV3LabelsToModelsMetadata, losing annotation-backed labels; update the source converter definition (the non-generated convert.go converter interface/impl) to split v3 labels into Metadata and Annotations (preserve annotation semantics when a label is annotation-backed) so that ConvertFromCreateMeterRequestToCreateMeterInput will set both meter.CreateMeterInput.Metadata and meter.CreateMeterInput.Annotations appropriately (you may add a helper like pV3LabelsToModelsMetadataAndAnnotations), then regenerate the generated file instead of editing it manually by running the project codegen (make generate / Goverter) so convert.gen.go is rebuilt with the corrected logic.api/v3/handlers/subscriptions/convert.go (1)
17-24:⚠️ Potential issue | 🟠 MajorSubscriptions now read annotations, but create still drops them.
Line 23 builds
Labelsfromsubscription.Metadata+subscription.Annotations, while Lines 81-83 still cast request labels straight intomodels.Metadata. If a client round-trips a subscription payload, the annotation-backed labels won't come back as annotations. Please use the inverse labels helper on the request path too.Also applies to: 70-84
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/handlers/subscriptions/convert.go` around lines 17 - 24, ConvertSubscriptionToAPISubscription reads labels via labels.FromMetadataAnnotations but the create/update request path still casts request Labels straight into models.Metadata, losing annotation-backed labels; when building the incoming subscription model (the code around lines 70-84 that currently assigns request.Labels -> models.Metadata), use the inverse helper (e.g., labels.ToMetadataAnnotations) to split api.BillingSubscription.Labels back into Metadata and Annotations so annotation-backed labels survive a round-trip; update the request-to-model conversion to populate both subscription.Metadata and subscription.Annotations via that helper and keep ConvertSubscriptionToAPISubscription unchanged.api/v3/handlers/customers/convert.gen.go (1)
16-30:⚠️ Potential issue | 🟠 MajorThis only fixes half of the customer labels round-trip.
Line 57 now emits annotation-backed labels, but Lines 29 and 81 still send request labels through
pV3LabelsToPModelsMetadata(...)only. A client that reads a customer and writes the same labels back will flatten the annotation keys into metadata instead of preserving the annotation side. Please wire the inverse split inapi/v3/handlers/customers/convert.goand regenerate this file.As per coding guidelines: "Never edit generated files with '// Code generated by X, DO NOT EDIT.' headers — regenerate them instead using the specified make targets" and "Define converter interfaces in convert.go and regenerate convert.gen.go using 'make generate' via Goverter".
Also applies to: 43-57, 70-82
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/handlers/customers/convert.gen.go` around lines 16 - 30, The generated converters are only mapping labels into Metadata via pV3LabelsToPModelsMetadata, losing annotation-backed keys on round-trip; update the inverse mapping logic in the non-generated converter definitions (api/v3/handlers/customers/convert.go) so the input v3 labels are split into annotation-backed labels and metadata (the inverse of the split used when reading customers) and ensure the Create/Update customer converters (which generate ConvertCreateCustomerRequestToCustomerMutate and the other affected converters that currently call pV3LabelsToPModelsMetadata) use that inverse split function instead of a plain pV3LabelsToPModelsMetadata call; after implementing the converter interface changes, regenerate convert.gen.go with the project generator (run make generate via Goverter) so ConvertCreateCustomerRequestToCustomerMutate and the other generated functions are updated accordingly.
🤖 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/customers/convert.go`:
- Around line 20-26: The write-path converters (e.g.,
ConvertFromCreateCustomerRequestToCreateCustomerInput) are currently mapping
incoming request Labels only into Metadata and dropping annotations; update
these converters to call
labels.ToMetadataAnnotations(createCustomerRequest.Labels) (or equivalent for
the Update converter referenced at lines 53–55) and assign both returned values:
set the first result to CustomerMutate.Metadata and the second to
CustomerMutate.Annotation (i.e., populate customer.CustomerMutate.Annotation as
well as Metadata). After making this change for all affected converters, run
make generate to regenerate convert.gen.go.
In `@api/v3/handlers/features/convert_test.go`:
- Around line 534-536: The test case is calling the wrong helper: it asserts nil
behavior for the reverse conversion; update the test so it exercises
labels.FromMetadata(nil) instead of labels.ToMetadata(nil) to match the case
name "metadata to labels", i.e. replace the call to labels.ToMetadata(nil) with
labels.FromMetadata(nil) and assert the returned labels value is nil; keep the
test name and assertion unchanged so the new behavior for FromMetadata(nil) is
covered.
In `@api/v3/handlers/meters/convert.go`:
- Around line 24-25: The CREATE conversion currently maps incoming request
Labels only into Meter.Metadata and ignores Annotations causing asymmetry with
the READ converter which uses ConvertMetadataAnnotationsToLabels; update the
CREATE converter (the conversion registered around the Create path) to call
labels.ToMetadataAnnotations() and assign the returned Metadata and Annotations
into meter.Meter.Metadata and meter.Meter.Annotations respectively, then
regenerate convert.gen.go so the generated converters include this custom
behavior.
In `@api/v3/labels/convert.go`:
- Around line 67-81: The switch over v in convert.go currently only handles
fmt.Stringer, string, and encoding.TextMarshaler, causing primitive annotation
values from models.Annotations (map[string]interface{}) to be dropped and later
fail ValidateLabel; update the type switch on v (the same switch that assigns
val) to add explicit cases for bool, int, int64, float32 and float64 (and other
integer sizes you expect) and convert them to strings using strconv (e.g.,
strconv.FormatBool, strconv.FormatInt/FormatUint, strconv.FormatFloat) so val is
populated for those primitives before ValidateLabel is called; ensure you
reference the same variable names (v, vv, val) and keep the existing
TextMarshaler/Stringer branches intact.
- Around line 47-50: The helper FromMetadataAnnotations currently returns nil
when both metadata and annotations are nil, which changes the wire shape from an
empty labels object to absent; update FromMetadataAnnotations to return a
non-nil *api.Labels with an initialized empty labels map (preserving the prior
`{}` behavior) even when metadata==nil && annotations==nil, or alternatively
ensure all call sites wrap its result into a non-nil *api.Labels; locate the
function FromMetadataAnnotations and adjust its return to produce an initialized
api.Labels (and empty map) rather than nil.
---
Outside diff comments:
In `@api/v3/handlers/customers/convert.gen.go`:
- Around line 16-30: The generated converters are only mapping labels into
Metadata via pV3LabelsToPModelsMetadata, losing annotation-backed keys on
round-trip; update the inverse mapping logic in the non-generated converter
definitions (api/v3/handlers/customers/convert.go) so the input v3 labels are
split into annotation-backed labels and metadata (the inverse of the split used
when reading customers) and ensure the Create/Update customer converters (which
generate ConvertCreateCustomerRequestToCustomerMutate and the other affected
converters that currently call pV3LabelsToPModelsMetadata) use that inverse
split function instead of a plain pV3LabelsToPModelsMetadata call; after
implementing the converter interface changes, regenerate convert.gen.go with the
project generator (run make generate via Goverter) so
ConvertCreateCustomerRequestToCustomerMutate and the other generated functions
are updated accordingly.
In `@api/v3/handlers/meters/convert.gen.go`:
- Around line 15-29: The Create->Model converter
ConvertFromCreateMeterRequestToCreateMeterInput currently collapses all request
labels into Metadata via pV3LabelsToModelsMetadata, losing annotation-backed
labels; update the source converter definition (the non-generated convert.go
converter interface/impl) to split v3 labels into Metadata and Annotations
(preserve annotation semantics when a label is annotation-backed) so that
ConvertFromCreateMeterRequestToCreateMeterInput will set both
meter.CreateMeterInput.Metadata and meter.CreateMeterInput.Annotations
appropriately (you may add a helper like
pV3LabelsToModelsMetadataAndAnnotations), then regenerate the generated file
instead of editing it manually by running the project codegen (make generate /
Goverter) so convert.gen.go is rebuilt with the corrected logic.
In `@api/v3/handlers/subscriptions/convert.go`:
- Around line 17-24: ConvertSubscriptionToAPISubscription reads labels via
labels.FromMetadataAnnotations but the create/update request path still casts
request Labels straight into models.Metadata, losing annotation-backed labels;
when building the incoming subscription model (the code around lines 70-84 that
currently assigns request.Labels -> models.Metadata), use the inverse helper
(e.g., labels.ToMetadataAnnotations) to split api.BillingSubscription.Labels
back into Metadata and Annotations so annotation-backed labels survive a
round-trip; update the request-to-model conversion to populate both
subscription.Metadata and subscription.Annotations via that helper and keep
ConvertSubscriptionToAPISubscription unchanged.
🪄 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: a3097772-7aa1-4292-872b-bb2947a125b4
📒 Files selected for processing (15)
api/v3/handlers/apps/convert.goapi/v3/handlers/billingprofiles/convert.goapi/v3/handlers/customers/convert.gen.goapi/v3/handlers/customers/convert.goapi/v3/handlers/customers/credits/convert.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/subscriptions/convert.goapi/v3/handlers/taxcodes/convert.goapi/v3/labels/convert.goapi/v3/labels/convert_test.goapi/v3/labels/validate.goapi/v3/labels/validate_test.go
gergely-kurucz-konghq
previously approved these changes
Apr 8, 2026
chrisgacsal
force-pushed
the
refactor/api-labels
branch
from
ed9f06b to
f553985
Compare
gergely-kurucz-konghq
previously approved these changes
Apr 8, 2026
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
api/v3/handlers/features/convert.go (1)
56-64: Consider adding input validation for labels.The
labels.ToMetadatacall silently skips invalid labels (viaValidateLabelinternally). If a user passes labels with invalid keys like"_bad": "value", they'll be quietly dropped without any error feedback.You could call
labels.ValidateLabels(body.Labels)at the start of the handler (or in this function) and return a 400 error if validation fails. This would give users explicit feedback about invalid input.That said, the current behavior is consistent with how other converters in this PR handle it, so feel free to defer this if silent dropping is the intended design choice.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/handlers/features/convert.go` around lines 56 - 64, convertCreateRequestToDomain currently uses labels.ToMetadata which silently drops invalid labels; add explicit validation by calling labels.ValidateLabels(body.Labels) at the start of convertCreateRequestToDomain and if it returns an error return a descriptive validation error to the caller (translate into a 400/invalid input response upstream). Keep the rest of the function building the feature.CreateFeatureInputs (Namespace, Name, Description, Key, MeterID, Metadata via labels.ToMetadata) but only proceed when ValidateLabels succeeds so invalid keys like "_bad" produce an explicit error instead of being silently ignored.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@api/v3/handlers/features/convert.go`:
- Around line 56-64: convertCreateRequestToDomain currently uses
labels.ToMetadata which silently drops invalid labels; add explicit validation
by calling labels.ValidateLabels(body.Labels) at the start of
convertCreateRequestToDomain and if it returns an error return a descriptive
validation error to the caller (translate into a 400/invalid input response
upstream). Keep the rest of the function building the
feature.CreateFeatureInputs (Namespace, Name, Description, Key, MeterID,
Metadata via labels.ToMetadata) but only proceed when ValidateLabels succeeds so
invalid keys like "_bad" produce an explicit error instead of being silently
ignored.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 916d2708-801b-42ef-99fb-fefcf66e8389
📒 Files selected for processing (15)
api/v3/handlers/apps/convert.goapi/v3/handlers/billingprofiles/convert.goapi/v3/handlers/customers/convert.gen.goapi/v3/handlers/customers/convert.goapi/v3/handlers/customers/credits/convert.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/subscriptions/convert.goapi/v3/handlers/taxcodes/convert.goapi/v3/labels/convert.goapi/v3/labels/convert_test.goapi/v3/labels/validate.goapi/v3/labels/validate_test.go
✅ Files skipped from review due to trivial changes (1)
- api/v3/handlers/features/convert_test.go
🚧 Files skipped from review as they are similar to previous changes (7)
- api/v3/handlers/meters/convert.gen.go
- api/v3/handlers/subscriptions/convert.go
- api/v3/handlers/apps/convert.go
- api/v3/handlers/meters/convert.go
- api/v3/handlers/billingprofiles/convert.go
- api/v3/labels/validate.go
- api/v3/handlers/taxcodes/convert.go
chrisgacsal
force-pushed
the
refactor/api-labels
branch
from
f553985 to
cc4396d
Compare
gergely-kurucz-konghq
previously approved these changes
Apr 8, 2026
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/handlers/features/convert_test.go`:
- Around line 535-536: The test incorrectly expects a nil result from
labels.FromMetadata(nil); update the assertion in convert_test.go to reflect the
helper's behavior by asserting the returned pointer is non-nil (use
assert.NotNil or require.NotNil on labels.FromMetadata(nil)) and optionally
assert that the resulting labels map is empty (e.g., assert.Equal/Empty on the
dereferenced/map contents) so the test matches FromMetadata's contract.
🪄 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: 0a7d1710-52b2-4817-96b3-29fd569d4e3c
📒 Files selected for processing (11)
api/v3/handlers/apps/convert.goapi/v3/handlers/billingprofiles/convert.goapi/v3/handlers/customers/convert.gen.goapi/v3/handlers/customers/convert.goapi/v3/handlers/customers/credits/convert.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/subscriptions/convert.goapi/v3/handlers/taxcodes/convert.go
✅ Files skipped from review due to trivial changes (2)
- api/v3/handlers/features/convert.go
- api/v3/handlers/billingprofiles/convert.go
🚧 Files skipped from review as they are similar to previous changes (5)
- api/v3/handlers/meters/convert.gen.go
- api/v3/handlers/subscriptions/convert.go
- api/v3/handlers/apps/convert.go
- api/v3/handlers/meters/convert.go
- api/v3/handlers/customers/convert.go
chrisgacsal
force-pushed
the
refactor/api-labels
branch
from
cc4396d to
0d1ddf2
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
api/v3/handlers/features/convert_test.go (1)
534-537:⚠️ Potential issue | 🟡 MinorTighten the nil-metadata test to assert the actual contract.
At Line 536,
assert.Empty(l)is a bit too loose—it doesn’t explicitly lock in the expected non-nil pointer behavior forlabels.FromMetadata(nil), and the subtest name currently says “returns nil labels”.Suggested tweak
- t.Run("nil metadata returns nil labels", func(t *testing.T) { + t.Run("nil metadata returns empty labels", func(t *testing.T) { l := labels.FromMetadata((map[string]string)(nil)) - assert.Empty(t, l) + require.NotNil(t, l) + assert.Empty(t, *l) })As per coding guidelines: "Make sure the tests are comprehensive and cover the changes. Keep a strong focus on unit tests and in-code integration tests."
🤖 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 534 - 537, The test "nil metadata returns nil labels" is too loose using assert.Empty; update the assertion to explicitly check the contract for labels.FromMetadata when passed nil by replacing assert.Empty(t, l) with assert.Nil(t, l) (referencing labels.FromMetadata and the local l variable) so the test guarantees a nil return rather than just an empty value.
🧹 Nitpick comments (1)
api/v3/handlers/meters/convert.go (1)
103-111: Remove unusedConvertMetadataToLabelsfunction.The function isn't referenced by goverter in the meters handler (no
goverter:extenddirective, and it's missing fromconvert.gen.go). It's just dead code that can be cleaned up.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/handlers/meters/convert.go` around lines 103 - 111, Remove the dead helper ConvertMetadataToLabels: delete the ConvertMetadataToLabels function definition (which converts models.Metadata to *api.Labels) since it's unused by goverter and not referenced in convert.gen.go; ensure no other code references ConvertMetadataToLabels before removal and run tests/build to confirm no references remain.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@api/v3/handlers/features/convert_test.go`:
- Around line 534-537: The test "nil metadata returns nil labels" is too loose
using assert.Empty; update the assertion to explicitly check the contract for
labels.FromMetadata when passed nil by replacing assert.Empty(t, l) with
assert.Nil(t, l) (referencing labels.FromMetadata and the local l variable) so
the test guarantees a nil return rather than just an empty value.
---
Nitpick comments:
In `@api/v3/handlers/meters/convert.go`:
- Around line 103-111: Remove the dead helper ConvertMetadataToLabels: delete
the ConvertMetadataToLabels function definition (which converts models.Metadata
to *api.Labels) since it's unused by goverter and not referenced in
convert.gen.go; ensure no other code references ConvertMetadataToLabels before
removal and run tests/build to confirm no references remain.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3c7629df-767e-4fcf-b6b3-a7bb87537301
📒 Files selected for processing (11)
api/v3/handlers/apps/convert.goapi/v3/handlers/billingprofiles/convert.goapi/v3/handlers/customers/convert.gen.goapi/v3/handlers/customers/convert.goapi/v3/handlers/customers/credits/convert.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/subscriptions/convert.goapi/v3/handlers/taxcodes/convert.go
✅ Files skipped from review due to trivial changes (2)
- api/v3/handlers/customers/convert.gen.go
- api/v3/handlers/features/convert.go
🚧 Files skipped from review as they are similar to previous changes (2)
- api/v3/handlers/meters/convert.gen.go
- api/v3/handlers/apps/convert.go
gergely-kurucz-konghq
approved these changes
Apr 8, 2026
This was referenced Apr 8, 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
Add
labelspackage with helpers used for convertingmodels.Metadataandmodels.Annotationsto helpers and other way around. Removes code duplication and ensures that labels are populated with annotations.Summary by CodeRabbit
New Features
Refactor
Tests