ankane / ahoy

Simple, powerful, first-party analytics for Rails
MIT License
4.22k stars 376 forks source link

Fail to upgrade to 5.0.2 #549

Closed leonardow-unep-wcmc closed 9 months ago

leonardow-unep-wcmc commented 9 months ago

Hi,

I upgrading an application which was using ahoy_matey 1.0.1. I follow the links below, to upgrade from 1.6.1 > 2.2.1 > 3.3.0 > 4.2.1 > 5.0.2

https://github.com/ankane/ahoy/tree/v1.6.1?tab=readme-ov-file#upgrading https://github.com/ankane/ahoy/blob/v2.2.1/docs/Ahoy-2-Upgrade.md https://github.com/ankane/ahoy/tree/v2.2.1?tab=readme-ov-file#upgrading https://github.com/ankane/ahoy/tree/v3.3.0?tab=readme-ov-file#upgrading https://github.com/ankane/ahoy/tree/v4.2.1?tab=readme-ov-file#upgrading https://github.com/ankane/ahoy/tree/v5.0.2?tab=readme-ov-file#upgrading

All good to 4.2.1, however when I upgrade to 5.0.2, it raise SystemStackError: stack level too deep

     # ./config/initializers/ahoy.rb:19:in `visit'
     # /usr/local/bundle/gems/ahoy_matey-5.0.2/lib/ahoy/tracker.rb:94:in `visit'
     # /usr/local/bundle/gems/ahoy_matey-5.0.2/lib/ahoy/tracker.rb:210:in `visit_token_helper'
     # /usr/local/bundle/gems/ahoy_matey-5.0.2/lib/ahoy/tracker.rb:128:in `visit_token'
     # ./config/initializers/ahoy.rb:19:in `visit'
     # /usr/local/bundle/gems/ahoy_matey-5.0.2/lib/ahoy/tracker.rb:94:in `visit'
     # /usr/local/bundle/gems/ahoy_matey-5.0.2/lib/ahoy/tracker.rb:210:in `visit_token_helper'
     # /usr/local/bundle/gems/ahoy_matey-5.0.2/lib/ahoy/tracker.rb:128:in `visit_token'
     # /usr/local/bundle/gems/ahoy_matey-5.0.2/lib/ahoy/tracker.rb:83:in `authenticate'
     # /usr/local/bundle/gems/ahoy_matey-5.0.2/lib/ahoy/warden.rb:4:in `block in <top (required)>'

I traced the code, and found that a method in config/initializers/ahoy.rb, which 2.0 upgrade guide asked me to add, break.

def visit
    @visit ||= visit_model.find_by(id: ensure_uuid(ahoy.visit_token)) if ahoy.visit_token
end

I also changed the index from 5.0 upgrade, which ask me to create an index:

add_index :ahoy_visits, [:visitor_token, :started_at]

However my database column is named visitor_id not visitor_token, so I changed it to:

add_index :ahoy_visits, [:visitor_id, :started_at]

But I don't think this is related to the error.

Should I just remove the visit method in initializers? What is the right way to fix this? Any input will be appreciated, thanks.

ankane commented 9 months ago

Hi @leonardow-unep-wcmc, you'll need to copy the latest version of that method and adapt it to your model. I believe you'll want to replace:

  1. where(visit_token: ahoy.visit_token) with where(id: ensure_uuid(ahoy.visit_token))
  2. where(visitor_token: ahoy.visitor_token) with where(visitor_id: ensure_uuid(ahoy.visitor_token))
leonardow-unep-wcmc commented 9 months ago

Thanks for your reply. Now I understand that the reason for the overriding is due to column names changed in v1.4.0.

After apply your suggestion, ahoy.track "something", {} in controller raise error:

NoMethodError (undefined method `visit_token' for #<Ahoy::Visit ...>

I solve it by using the following:

# config/initializers/ahoy.rb

class Ahoy::Store < Ahoy::DatabaseStore
  def authenticate(data)
    # https://github.com/ankane/ahoy/tree/v5.0.2?tab=readme-ov-file#gdpr-compliance-1
    # disables automatic linking of visits and users (for GDPR compliance)
  end

  def track_visit(data)
    # Map the new column names (since 1.4.0), to old column name (< 1.4.0).
    data[:id] = ensure_uuid(data.delete(:visit_token))
    data[:visitor_id] = ensure_uuid(data.delete(:visitor_token))
    super(data)
  end

  def track_event(data)
    # Map the new column names (since 1.4.0), to old column name (< 1.4.0).
    data[:id] = ensure_uuid(data.delete(:event_id))
    super(data)
  end

  def ensure_uuid(id)
    UUIDTools::UUID.parse(id).to_s
  rescue
    UUIDTools::UUID.sha1_create(UUIDTools::UUID.parse(Ahoy::Tracker::UUID_NAMESPACE), id).to_s
  end
end
# app/models/ahoy/visit.rb
module Ahoy
  class Visit < ApplicationRecord
    # Map the new column names (since 1.4.0), to old column name (< 1.4.0).
    alias_attribute :visit_token, :id
    alias_attribute :visitor_token, :visitor_id
  end
end

Could you tell me if you think the above approach is feasible? Would it raise any other issues? Thanks.

ankane commented 9 months ago

That looks reasonable to me (along with the visit method change), but you'll need to try it out to see if there are other issues.