Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Mongoid Support #46

Merged
merged 5 commits into from Jul 19, 2012

Conversation

Projects
None yet
3 participants
Contributor

jgwmaxwell commented Jul 11, 2012

Hi there,

I've written a patch for Mongoid support for Impressionist, selectable through the same setup as MongoMapper and AR, just choose

config.orm = :mongoid

It doesn't change any of the existing API, although I've removed the legacy code marked for removal in v0.5 since there will be no legacy Mongoid applications - imho it didn't make any sense to include it. Also, since it's essentially a new feature, it is only compatible with Mongoid 3.0+, those who haven't moved over yet from older versions of Mongoid should really be looking to anyway - it's a big step forward.

I wasn't 100% sure on how best to integrate the tests, so haven't included my own specs in the repo as I didn't want to clash with your style - I'll study yours a little more and include appropriate versions tomorrow.

Hope this helps and is of interest to you, feel free to tear it apart!

Looking ahead, changing some of the counting code so it runs MapReduce might offer some advantages - especially when the collection gets very large, I'd be happy to write relevant code if wanted.

John.

EDIT: The latest couple of commits (should have been one, but realised a new initializer wasn't needed too late...) remove the Mongoid 3 limitation and make it work on Mongoid 2.x as well.

The default setting of pre-selecting ActiveRecord causes generators to fail on the

ActiveRecord::Base,send(:include, Impressionist::Impressionable)

line. Checking for it being defined beforehand allows apps generated with -O (skip AR) to still run the generators.

Just for sake of completeness, not at all Mongoid propaganda ;)

Mongoid needs the Id of the Polymorphic record to be supplied as a Moped::BSON::ObjectId. In order to do this without rewriting the whole of the controller module, redefining this method seemed to be the best method. I've only required this file if :mongoid is selected, so for MongoMapper and AR the original code remains unchanged.

This file fixes the #direct_create_statement method with one that Mongoid can cope with.

Use Model#inc atomic update here as it is fast and accurate

Mongoid doesn't run #count the same way as AR, so defining the filter to check for existence, and the time check to be a simple Range

If there is a counter_cache, declare the field for it

Contributor

jgwmaxwell commented Jul 11, 2012

By checking if Moped is defined, it now supports Mongoid 2 as well as Mongoid 3, tested under 2.4.11 as well as 3.0.0

Contributor

jgwmaxwell commented Jul 11, 2012

Removed another file which should never have been committed (a lesson to go to bed earlier!)

@jgwmaxwell jgwmaxwell referenced this pull request Jul 11, 2012

Closed

Compatible with Mongoid #34

dalibor commented Jul 19, 2012

Hi @jgwmaxwell

I've tested your code for the mongoid support and I've found an issue with the counter cache. It does not update when impression is being created with direct create statement:

Impression.create(direct_create_statement)

It updates when impression is created with association statement:

obj.impressions.create(associative_create_statement({:message => message}))
Contributor

jgwmaxwell commented Jul 19, 2012

@dalibor what situation are you using this within, called from a plain call to impressionist inside a Controller? If you want me to look into it I need more than some pseudocode, the Mongoid version works like-for-like with the AR one for me.

dalibor commented Jul 19, 2012

@jgwmaxwell Yes, when only using impressionist in a controller - impression is being created with direct create statement.

Owner

johnmcaliley commented Jul 19, 2012

@jgwmaxwell Thanks for this! For some reason I didn't get a notice about the pull request. I would have merged this a week ago if I had seen it. I am merging, but I will wait until I take a closer look before I put a new release on rubygems.

@johnmcaliley johnmcaliley added a commit that referenced this pull request Jul 19, 2012

@johnmcaliley johnmcaliley Merge pull request #46 from jgwmaxwell/mongoid
Mongoid Support
7a93fff

@johnmcaliley johnmcaliley merged commit 7a93fff into charlotte-ruby:master Jul 19, 2012

Contributor

jgwmaxwell commented Jul 19, 2012

@dalibor Ok, I have to say I can't recreate this with either Mongoid 2.4 or 3, feel free to send me a Gist if you want me to look into it, but as far as I can see it works - counter caches are updating for me. Happy to try and help diagnose if you want though. The code that I have that works a treat is:

class Post
  include Mongoid::Document
  is_impressionable :counter_cache => true
  field :title, type: String
  field :content, type: String
end

and

class PostsController < ApplicationController
  impressionist
  ......

For me that updates as expected, and tests pass. Let me know if you want help.

Owner

johnmcaliley commented Jul 19, 2012

@jgwmaxwell I added you to the impressionist repo team. you have push access now.

Contributor

jgwmaxwell commented Jul 19, 2012

@johnmcaliley Many thanks - do you have a roadmap of fixes/changed you want added? I liked the idea of adding Geolocation to the features from #43, I've got a working version of that ready to rock, if you are interested in adding that?

dalibor commented Jul 19, 2012

@jgwmaxwell Thanks for your time! I've tested it on errbit and it wasn't working because of identity setting in App model there:

  # Some legacy apps may have string as key instead of BSON::ObjectID
  identity :type => String

In a new application (or when identity removed from this App model) - it works.

I'm not quite familiar with mongoid, but I guess this might be a potential bug in there.

Contributor

jgwmaxwell commented Jul 19, 2012

Ahhhh right, let me work on this a bit - I don't see why it shouldn't work like this, but disabling BSON::ObjectId or (now) Moped::BSON::ObjectId as the identifier of the model does cause some interesting quirks with Mongoid as far as I recall. Not sure where the issue is yet, but I'll get to the bottom of it and get back to you.

As a matter of design, I've always found it better to let Mongoid set ObjectIds as it wants, then override #to_param and use custom finders to get the functionality that you need from other fields - it tends to make Mongoid faster, more reliable and more predictable IMHO.

dalibor commented Jul 19, 2012

Thanks for the explanation @jgwmaxwell

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment