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/c2719470...@github.com>_______________________________________________
rails-dev mailing list
rails-dev@openstreetmap.org
https://lists.openstreetmap.org/listinfo/rails-dev
