[openstreetmap/openstreetmap-website] Add UserActivities module for structured user activity history (PR #5761)
Emin Kocan
notifications at github.com
Thu Mar 13 00:56:07 UTC 2025
kcne left a comment (openstreetmap/openstreetmap-website#5761)
> Did you try doing this via normal ActiveModel queries before resorting to raw SQL queries?
>
> I certainly don't like the idea of interpolating arguments into the queries - the fact that you called the argument `quoted_user_id` immediately hints at the risks of doing so. Is it not possible to use bound parameters with these queries?
>
> What analysis have you done of the execution plan(s) and the likely performance? Is everything using indexes or are there things which are doing table scans?
Thank you for the feedback Tom. I've completely refactored the code to address your concerns:
1. **ActiveRecord Usage**: Now using ActiveRecord for all activity fetching. Only kept raw SQL for the `activity_days` method since the UNION ALL across multiple tables is complex to express in pure ActiveRecord.
2. **SQL Injection Protection**: Eliminated all string interpolation and replaced `quoted_user_id` with proper parameter binding:
```ruby
ActiveRecord::Base.sanitize_sql([sql, { :user_id => user_id, :limit => limit, :offset => offset }])
```
3. **Performance Analysis**: Conducted thorough testing with multiple users:
- Execution time: 9-14ms for users with ~25 activity days
- Query planning: ~0.15-0.20ms, execution: ~0.15-0.19ms
- All tables have appropriate indexes except `gpx_files` which has `[user_id,id]` index as agreed in #5747
This was just the initial implementation. Let me know if further adjustments are needed. Thank you.
--
Reply to this email directly or view it on GitHub:
https://github.com/openstreetmap/openstreetmap-website/pull/5761#issuecomment-2719470105
You are receiving this because you are subscribed to this thread.
Message ID: <openstreetmap/openstreetmap-website/pull/5761/c2719470105 at github.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstreetmap.org/pipermail/rails-dev/attachments/20250312/d4cb87cd/attachment-0001.htm>
More information about the rails-dev
mailing list