getlago / lago

Open Source Metering and Usage Based Billing API ⭐️ Consumption tracking, Subscription management, Pricing iterations, Payment orchestration & Revenue analytics
https://www.getlago.com
GNU Affero General Public License v3.0
6.41k stars 272 forks source link

[BUG]: billable-metric separator code ':' #345

Open aelttil opened 2 months ago

aelttil commented 2 months ago

The billable metric code appears not to support the ":" separator, despite its acceptance by the user interface. The issue does not occur with any Aggregation type. For instance, it works fine with the count type.

Steps to reproduce the behavior:

  1. Go to 'billable-metric'
  2. Create new billable-metric with code test:test
  3. Select Aggregation type : Weighted sum
  4. Add a new billable-metric into plan
  5. Push new event
  6. Go to customer details and show Analytics Current usage report.

Error in UI Screenshot 2024-04-29 at 10 40 19

Grafhql Query is POST http://localhost:3000/graphql

"query customerUsage($customerId: ID!, $subscriptionId: ID!) {\n customerUsage(customerId: $customerId, subscriptionId: $subscriptionId) {\n amountCents\n currency\n fromDatetime\n toDatetime\n chargesUsage {\n units\n amountCents\n charge {\n id\n invoiceDisplayName\n __typename\n }\n billableMetric {\n id\n code\n name\n __typename\n }\n filters {\n id\n __typename\n }\n groupedUsage…oiceDisplayName\n __typename\n }\n billableMetric {\n name\n __typename\n }\n filters {\n id\n amountCents\n units\n values\n invoiceDisplayName\n __typename\n }\n groupedUsage {\n amountCents\n groupedBy\n eventsCount\n units\n filters {\n id\n amountCents\n units\n values\n invoiceDisplayName\n __typename\n }\n __typename\n }\n __typename\n }\n __typename\n}"

Regards, Alex.

julienbourdeau commented 3 weeks ago

Thanks for the report!

I had a look yesterday and I did reproduce it. I initially thought it had to do with GraphQL, because I know GQL cannot contains colons in some stuff.

But it's not, it's because of ActiveRecord variable replacement in SQL. When we prepare the query, variables like :to_datetime, :from_datetime, :initial_value are replace by their value.

ActiveRecord will look to replace :test in WHERE billable_metrics.code = "test:test" 🤨

I'm not sure how to fix that. An easy way would be to disallow the : char in the billable metric code but I'd rather fix it as the ActiveRecord model so it doesn't happen with other variable that are not supposed to be interpolated.

https://github.com/getlago/lago-api/blob/bdc020f668f038e483d98287bb75c3beef65543c/app/services/events/stores/postgres_store.rb#L261-L270

vincent-pochet commented 3 weeks ago

The fix requires us to update Rails to 7.1 (https://github.com/rails/rails/blob/main/activerecord/lib/active_record/sanitization.rb#L219) To do so we need to upgrade clickhouse-activerecord gem (Rebasing https://github.com/PNixx/clickhouse-activerecord into https://github.com/getlago/clickhouse-activerecord)

We are taking it into account in our roadmap @aelttil and we will let you know when it will be fixed. In the meantime, the workaround is to avoid using : in a billable metric code