search

twoixter / trackoid

Trackoid is an easy scalable analytics tracker using MongoDB and Mongoid
MIT License
88 stars 16 forks source link

ClassAlreadyDefined raised on all requests but first with Rails 3.1. #6

Closed grk closed 13 years ago

grk commented 13 years ago

It looks like Rails 3.1 introduced something that messed with the check for re-defining classes.

In development, the first request after the server starts goes through just fine, but all next requests have this error:

Started GET "/assets/restaurant.css" for 127.0.0.1 at 2011-07-07 13:20:02 +0200

Mongoid::Errors::ClassAlreadyDefined (RestaurantAggregates already defined, can't aggregate!):
  app/models/restaurant.rb:77:in `<class:Restaurant>'
  app/models/restaurant.rb:5:in `<top (required)>'

I suggest using https://github.com/thoughtbot/appraisal for testing with multiple Rails versions.

grk commented 13 years ago

Accidentally added the code to the same branch, so my pull request contains fixes for both issues.

twoixter commented 13 years ago

Thanks grk for your commits!

This exception is weird, it should not happen since the "define_aggregate_model" should be called only once. If it is being called twice of more then it must be something with the new "class_attribute" fix instead of the "class_inheritable_accessor".

Perhaps the fix does not work as expected, in the "aggregate" class method there is a call to:

define_aggregate_model if aggregate_klass.nil?

"aggregate_klass" should be the class variable and should NOT be nil the second time. I don't remember why I added the "aggregate_klass" variable, since later I check the definition using "Object.const_defined?". Perhaps it was kind of a optimization since Object.const_defined? I think it's quite slow.

grk commented 13 years ago

This happens regardless of the class_attribute fix.

As for const_defined?, it's not the best idea, as the reloading in rails makes it re-defined, resulting in a warning:

/Users/gk/.rvm/gems/ruby-1.9.2-p180-patched/bundler/gems/trackoid-906960b09fdc/lib/trackoid/aggregates.rb:136: warning: already initialized constant RestaurantAggregates
twoixter commented 13 years ago

I will try to review this since I think the check and exception is somewhat redundant. I mean, "RestaurantAggregates" must be unique to the "Restaurant" model so I think there would be no harm reusing it if already exists.

twoixter commented 13 years ago

Sorry, forgot to add... The check I think was originally meant to avoid name clashes in case you already had a model or class named "RestaurantAggregates".

twoixter commented 13 years ago

Done. Should be now compatible with Rails 3.1