Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions openmeter/billing/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -152,3 +152,18 @@ type AppError struct {
func (e AppError) Error() string {
return fmt.Sprintf("app %s type with id %s in namespace %s: %s", e.AppType, e.AppID.ID, e.AppID.Namespace, e.Err.Error())
}

// ErrSnapshotInvalidDatabaseState is returned when the database state is invalid for snapshotting the line quantity.
// This can happen if the feature or meter is not found. In such cases we should transition the invoice to
// draft.invalid state.
type ErrSnapshotInvalidDatabaseState struct {
Err error
}

func (e ErrSnapshotInvalidDatabaseState) Error() string {
return fmt.Sprintf("snapshotting line quantity: %s", e.Err.Error())
}

func (e ErrSnapshotInvalidDatabaseState) Unwrap() error {
return e.Err
}
32 changes: 32 additions & 0 deletions openmeter/billing/featuremeter.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
package billing

import (
"fmt"

"github.com/openmeterio/openmeter/openmeter/meter"
"github.com/openmeterio/openmeter/openmeter/productcatalog/feature"
)

type FeatureMeter struct {
Feature feature.Feature
Meter *meter.Meter
}

type FeatureMeters map[string]FeatureMeter

func (f FeatureMeters) Get(featureKey string, dependsOnMeteredQuantity bool) (FeatureMeter, error) {
featureMeter, exists := f[featureKey]
if !exists {
return FeatureMeter{}, &ErrSnapshotInvalidDatabaseState{
Err: fmt.Errorf("feature[%s] not found", featureKey),
}
}

if dependsOnMeteredQuantity && featureMeter.Meter == nil {
return FeatureMeter{}, &ErrSnapshotInvalidDatabaseState{
Err: fmt.Errorf("feature[%s] has no meter associated, but the line depends on metered quantity", featureMeter.Feature.Key),
}
}

return featureMeter, nil
}
177 changes: 177 additions & 0 deletions openmeter/billing/service/featuremeter.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,177 @@
package billingservice

import (
"context"
"fmt"
"time"

"github.com/samber/lo"

"github.com/openmeterio/openmeter/openmeter/billing"
"github.com/openmeterio/openmeter/openmeter/meter"
"github.com/openmeterio/openmeter/openmeter/productcatalog/feature"
)

func (s *Service) resolveFeatureMeters(ctx context.Context, lines billing.StandardLines) (billing.FeatureMeters, error) {
namespaces := lo.Uniq(lo.Map(lines, func(line *billing.StandardLine, _ int) string {
return line.Namespace
}))

if len(namespaces) != 1 {
return nil, fmt.Errorf("all lines must be in the same namespace")
}

namespace := namespaces[0]

featuresToResolve := lo.Uniq(
lo.Filter(
lo.Map(lines, func(line *billing.StandardLine, _ int) string {
// Never happens, as StandardLine is always a usage based line, but until we migrate to a new table let's keep it here
if line.UsageBased == nil {
return ""
}

return line.UsageBased.FeatureKey
}),
func(featureKey string, _ int) bool {
return featureKey != ""
},
),
)

// Let's resolve the features
features, err := s.featureService.ListFeatures(ctx, feature.ListFeaturesParams{
IDsOrKeys: featuresToResolve,
Namespace: namespace,
IncludeArchived: true,
})
if err != nil {
return nil, fmt.Errorf("listing features: %w", err)
}

featuresByKey := getLastFeatures(features.Items)

metersToResolve := lo.Uniq(
lo.Filter(
lo.Map(lo.Values(featuresByKey), func(feature feature.Feature, _ int) string {
if feature.MeterSlug == nil {
return ""
}

return *feature.MeterSlug
}),
func(meterSlug string, _ int) bool {
return meterSlug != ""
},
),
)

meters, err := s.meterService.ListMeters(ctx, meter.ListMetersParams{
SlugFilter: lo.ToPtr(metersToResolve),
Namespace: namespace,
IncludeDeleted: true,
})
if err != nil {
return nil, fmt.Errorf("listing meters: %w", err)
}

metersByKey := getLastMeters(meters.Items)

out := make(billing.FeatureMeters, len(featuresByKey))
for featureKey, feature := range featuresByKey {
if feature.MeterSlug == nil {
out[featureKey] = billing.FeatureMeter{
Feature: feature,
}

continue
}

meter, exists := metersByKey[*feature.MeterSlug]
if !exists {
out[featureKey] = billing.FeatureMeter{
Feature: feature,
}

continue
}

out[featureKey] = billing.FeatureMeter{
Feature: feature,
Meter: &meter,
}
}

return out, nil
Comment on lines +15 to +105
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Find feature service ListFeatures implementation
fd -t f -n "feature" --type go | head -20

Repository: openmeterio/openmeter

Length of output: 295


🏁 Script executed:

#!/bin/bash
# Search for ListFeatures and ListMeters implementations
rg -n "func.*ListFeatures\(" --type=go -A 5

Repository: openmeterio/openmeter

Length of output: 4089


🏁 Script executed:

#!/bin/bash
# Search for ListMeters implementation
rg -n "func.*ListMeters\(" --type=go -A 5

Repository: openmeterio/openmeter

Length of output: 6308


🏁 Script executed:

#!/bin/bash
# Read the feature adapter ListFeatures implementation
sed -n '172,230p' openmeter/productcatalog/adapter/feature.go

Repository: openmeterio/openmeter

Length of output: 1797


🏁 Script executed:

#!/bin/bash
# Read the meter adapter ListMeters implementation
sed -n '18,80p' openmeter/meter/adapter/meter.go

Repository: openmeterio/openmeter

Length of output: 1785


🏁 Script executed:

#!/bin/bash
# Find ListFeaturesParams structure definition
rg -n "type ListFeaturesParams struct" --type=go -A 10

Repository: openmeterio/openmeter

Length of output: 2288


🏁 Script executed:

#!/bin/bash
# Find ListMetersParams structure definition
rg -n "type ListMetersParams struct" --type=go -A 10

Repository: openmeterio/openmeter

Length of output: 2418


🏁 Script executed:

#!/bin/bash
# Check how KeyIn behaves with empty slice
rg -n "func.*KeyIn\(" --type=go -A 5 | head -30

Repository: openmeterio/openmeter

Length of output: 2432


🏁 Script executed:

#!/bin/bash
# Also check if there are any tests showing ListMeters behavior with empty SlugFilter
rg -n "ListMeters.*SlugFilter" --type=go -B2 -A8

Repository: openmeterio/openmeter

Length of output: 47


Add early returns to prevent unintended list-all queries when inputs are empty.

When lines is empty or contains items with no UsageBased data, featuresToResolve becomes empty. Calling ListFeatures with an empty IDsOrKeys slice doesn't apply a filter, so it returns all features in the namespace instead of none—a correctness bug in a billing path. Similarly, skipping the meter lookup when there's nothing to resolve avoids unnecessary database hits.

🔧 Suggested fix
 func (s *Service) resolveFeatureMeters(ctx context.Context, lines billing.StandardLines) (billing.FeatureMeters, error) {
+	if len(lines) == 0 {
+		return billing.FeatureMeters{}, nil
+	}
 	namespaces := lo.Uniq(lo.Map(lines, func(line *billing.StandardLine, _ int) string {
 		return line.Namespace
 	}))
@@
 	featuresToResolve := lo.Uniq(
@@
 	)
+	if len(featuresToResolve) == 0 {
+		return billing.FeatureMeters{}, nil
+	}
@@
-	meters, err := s.meterService.ListMeters(ctx, meter.ListMetersParams{
-		SlugFilter:     lo.ToPtr(metersToResolve),
-		Namespace:      namespace,
-		IncludeDeleted: true,
-	})
-	if err != nil {
-		return nil, fmt.Errorf("listing meters: %w", err)
-	}
-
-	metersByKey := getLastMeters(meters.Items)
+	metersByKey := map[string]meter.Meter{}
+	if len(metersToResolve) > 0 {
+		meters, err := s.meterService.ListMeters(ctx, meter.ListMetersParams{
+			SlugFilter:     lo.ToPtr(metersToResolve),
+			Namespace:      namespace,
+			IncludeDeleted: true,
+		})
+		if err != nil {
+			return nil, fmt.Errorf("listing meters: %w", err)
+		}
+		metersByKey = getLastMeters(meters.Items)
+	}
🤖 Prompt for AI Agents
In `@openmeter/billing/service/featuremeter.go` around lines 15 - 105, In
resolveFeatureMeters: avoid calling ListFeatures/ListMeters with empty filter
slices which causes unfiltered "list all" queries; check the computed
featuresToResolve after building it and if it's empty return an empty
billing.FeatureMeters (no error), and likewise after computing metersToResolve
skip the call to s.meterService.ListMeters and leave metersByKey empty (or
return early if appropriate) so you don't issue an unfiltered query; update the
logic around featuresToResolve, metersToResolve, ListFeatures and ListMeters to
perform these early returns/short-circuits.

}

type lastEntityAccessor[T any] interface {
GetKey(T) string
GetDeletedAt(T) *time.Time
}

func getLastEntity[T any](entities []T, accessor lastEntityAccessor[T]) map[string]T {
featuresByKey := lo.GroupBy(entities, func(entity T) string {
return accessor.GetKey(entity)
})

out := make(map[string]T, len(featuresByKey))
for key, features := range featuresByKey {
// Let's try to find an unarchived feature
out[key] = latestEntity(features, accessor)
}

return out
}

func latestEntity[T any](entities []T, accessor lastEntityAccessor[T]) T {
for _, entity := range entities {
if accessor.GetDeletedAt(entity) == nil {
return entity
}
}

// Otherwise, let's find the most recently archived feature:
// - all entities have non-nil deleted at (or we would have returned already)
// - and we have at least one entity due to the definition of the groupBy
mostRecentlyArchivedFeature := entities[0]
for _, entity := range entities {
if accessor.GetDeletedAt(entity).After(*accessor.GetDeletedAt(mostRecentlyArchivedFeature)) {
mostRecentlyArchivedFeature = entity
}
}

return mostRecentlyArchivedFeature
}

type featureAccessor struct{}

var _ lastEntityAccessor[feature.Feature] = (*featureAccessor)(nil)

func (a featureAccessor) GetKey(feature feature.Feature) string {
return feature.Key
}

func (a featureAccessor) GetDeletedAt(feature feature.Feature) *time.Time {
return feature.ArchivedAt
}

func getLastFeatures(features []feature.Feature) map[string]feature.Feature {
return getLastEntity(features, featureAccessor{})
}

type meterAccessor struct{}

var _ lastEntityAccessor[meter.Meter] = (*meterAccessor)(nil)

func (a meterAccessor) GetKey(meter meter.Meter) string {
return meter.Key
}

func (a meterAccessor) GetDeletedAt(meter meter.Meter) *time.Time {
return meter.DeletedAt
}

func getLastMeters(meters []meter.Meter) map[string]meter.Meter {
return getLastEntity(meters, meterAccessor{})
}
63 changes: 63 additions & 0 deletions openmeter/billing/service/featuremeter_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
package billingservice

import (
"testing"
"time"

"github.com/samber/lo"
"github.com/stretchr/testify/require"

"github.com/openmeterio/openmeter/openmeter/productcatalog/feature"
)

func TestGetLastFeatures(t *testing.T) {
tcs := []struct {
name string
features []feature.Feature
expected map[string]string
}{
{
name: "single-active",
features: []feature.Feature{
{ID: "id-active", ArchivedAt: nil, Key: "feature-1-active"},
},
expected: map[string]string{"feature-1-active": "id-active"},
},
{
name: "single-archived",
features: []feature.Feature{
{ID: "id-archived", ArchivedAt: lo.ToPtr(time.Now()), Key: "feature-1-archived"},
},
expected: map[string]string{"feature-1-archived": "id-archived"},
},
{
name: "multi-archived",
features: []feature.Feature{
{ID: "id-archived", ArchivedAt: lo.ToPtr(time.Now()), Key: "feature-1"},
{ID: "id-active", ArchivedAt: nil, Key: "feature-1"},
},
expected: map[string]string{"feature-1": "id-active"},
},
{
name: "archived-ordering",
features: []feature.Feature{
{ID: "id-archived-1", ArchivedAt: lo.ToPtr(time.Now()), Key: "feature-1"},
{ID: "id-archived-2", ArchivedAt: lo.ToPtr(time.Now().Add(5 * time.Second)), Key: "feature-1"},
},
expected: map[string]string{"feature-1": "id-archived-2"},
},
}

for _, tc := range tcs {
t.Run(tc.name, func(t *testing.T) {
out := getLastFeatures(tc.features)

featureKeyToID := map[string]string{}
for key, feature := range out {
featureKeyToID[key] = feature.ID
}

require.Equal(t, tc.expected, featureKeyToID)
})
}
}
Loading
Loading