fix: static entitlement deserialization #3701
Conversation
📝 WalkthroughWalkthroughThese changes standardize how entitlement Config is encoded and decoded across the API and database layers. Config is now consistently handled as a JSON-encoded string at the API/DB boundary—avoiding double-encoding and ensuring consistent representation across V1, V2, and V3 API versions. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 0
🧹 Nitpick comments (2)
openmeter/entitlement/adapter/entitlement.go (1)
511-522: Nice fix for the double-encoding issue! The backward compatibility approach looks solid.The logic here unwraps one layer of JSON encoding when reading from the DB, which handles the legacy double-encoded values. The silent fallback when unmarshaling fails is intentional to support both old and new data formats, which makes sense given there's no migration.
One thought: It might be worth adding a metric or debug log when the fallback path is taken. This could help you track how much legacy double-encoded data is still in the wild and eventually validate when it's safe to remove this compatibility layer. Something like:
if err := json.Unmarshal(e.Config, &configStr); err == nil { ent.Config = []byte(configStr) } else { + // TODO: Add metric/log to track fallback usage for legacy data ent.Config = e.Config }But this is totally optional—the current implementation handles the requirements well!
e2e/entitlement_test.go (1)
646-914: Excellent test coverage! Really thorough validation across all API versions.The test covers:
- V1, V2, and V3 API paths
- Both Go client and raw HTTP (smart!)
- Create and retrieve operations
- Proper JSON encoding verification
One optional enhancement to consider: It might be worth adding a test case that explicitly verifies backward compatibility with existing double-encoded data. You could:
- Manually insert a double-encoded Config value into the DB (simulating legacy data)
- Read it back through the API
- Verify it's properly unwrapped
This would explicitly validate the fallback path in the adapter (lines 511-522 in entitlement.go) that handles legacy data. But honestly, the current coverage is already really solid for the happy path!
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
api/v3/handlers/customers/entitlementaccess/mapping.go(1 hunks)e2e/entitlement_test.go(2 hunks)openmeter/entitlement/adapter/entitlement.go(2 hunks)openmeter/entitlement/driver/parser.go(5 hunks)openmeter/entitlement/driver/v2/mapping.go(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
⚙️ CodeRabbit configuration file
**/*.go: In general when reviewing the Golang code make readability and maintainability a priority, even potentially suggest restructuring the code to improve them.Performance should be a priority in critical code paths. Anything related to event ingestion, message processing, database operations (regardless of database) should be vetted for potential performance bottlenecks.
Files:
api/v3/handlers/customers/entitlementaccess/mapping.goopenmeter/entitlement/adapter/entitlement.goe2e/entitlement_test.goopenmeter/entitlement/driver/parser.goopenmeter/entitlement/driver/v2/mapping.go
**/*_test.go
⚙️ CodeRabbit configuration file
**/*_test.go: Make sure the tests are comprehensive and cover the changes. Keep a strong focus on unit tests and in-code integration tests.
When appropriate, recommend e2e tests for critical changes.
Files:
e2e/entitlement_test.go
🧠 Learnings (1)
📚 Learning: 2025-03-07T12:17:43.129Z
Learnt from: GAlexIHU
Repo: openmeterio/openmeter PR: 2383
File: openmeter/entitlement/metered/lateevents_test.go:37-45
Timestamp: 2025-03-07T12:17:43.129Z
Learning: In the OpenMeter codebase, test files like `openmeter/entitlement/metered/lateevents_test.go` may use variables like `meterSlug` and `namespace` without explicit declarations visible in the same file. This appears to be an accepted pattern in their test structure.
Applied to files:
e2e/entitlement_test.go
🧬 Code graph analysis (3)
api/v3/handlers/customers/entitlementaccess/mapping.go (2)
api/v3/server/server.go (1)
Config(32-43)openmeter/server/server.go (1)
Config(65-69)
e2e/entitlement_test.go (2)
e2e/helpers.go (1)
CreateCustomerWithSubject(16-48)api/api.gen.go (4)
CreateFeatureJSONRequestBody(9369-9369)EntitlementStaticCreateInputs(3330-3348)CreateEntitlementJSONRequestBody(9426-9426)CreateCustomerEntitlementV2JSONRequestBody(9459-9459)
openmeter/entitlement/driver/v2/mapping.go (1)
api/api.gen.go (2)
EntitlementStaticV2(3354-3400)Annotations(1049-1049)
⏰ 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). (9)
- GitHub Check: E2E
- GitHub Check: Quickstart
- GitHub Check: Test
- GitHub Check: Lint
- GitHub Check: Build
- GitHub Check: Migration Checks
- GitHub Check: Code Generators
- GitHub Check: Repository Scan
- GitHub Check: Analyze (go)
🔇 Additional comments (6)
openmeter/entitlement/driver/parser.go (2)
88-94: LGTM! The encoding at the API boundary looks correct.After the adapter unwraps the double-encoded data, this properly marshals it back to a JSON string for the API response. The flow makes sense: DB → unwrap in adapter → re-encode for API consumers.
Also applies to: 108-108
201-203: Clean simplification here!Now that the adapter handles the unwrapping, this can just do a straightforward conversion to string. Much cleaner than the previous approach.
openmeter/entitlement/driver/v2/mapping.go (2)
125-131: Good consistency across V1 and V2 APIs.The V2 mapping follows the same pattern as V1, encoding Config as a JSON string for the API response. Nice to see consistent handling across API versions!
Also applies to: 136-136
352-352: Good catch on the redundant type conversion!Since
v.Configisjson.RawMessage(which is already[]byte), the old[]byte(v.Config)was unnecessary and may have been adding an extra encoding layer. Usingv.Configdirectly lets the properly-encoded input pass through to storage correctly.api/v3/handlers/customers/entitlementaccess/mapping.go (1)
29-33: Nice cleanup! This is much simpler now.Since the adapter handles the unwrapping, this can just do a straightforward bytes-to-string conversion. The comment makes it clear what's happening too.
e2e/entitlement_test.go (1)
917-924: LGTM! Clean helper function.Standard pattern for e2e tests - gets the address from env and skips gracefully if not available.
This reverts commit 9f5a6df.
Overview
The issue
field.JSON()in ent treats the[]byteas a value to be JSON-marshaled for storage. But we're passing it[]byte("\"day\"")which already has quotes, so it marshals it again, creating double-encoding.The Fix
We should fix the persisted values in the database (e.g. like
IlwiZGF5XCIi-> decodes to"\"day\"") but that would require a data migration.This PR implements the double decoding in the adapter layer so the http drivers don't have to manage it.
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.