Mongoid support #1031

Merged
merged 42 commits into from Mar 12, 2012

Conversation

Projects
None yet
8 participants
Collaborator

mshibuya commented Mar 12, 2012

(related to #105)

Most of the features seem to be working now!

I'm also planning major refactoring on tests(making ORM switchable, running integration tests on each ORM like the way Devise does), but that may take some time, so I'll make another pull request when I've done.

Thanks again for creating and maintaining this awesome gem!

Marius Seritan and others added some commits Jan 30, 2011

@mshibuya Marius Seritan Initial checking, identify models and lists them.
Editing, associations do not work.
99362a3
@mshibuya Marius Seritan Fix fetching by ID and remove debugging. e604e42
@mshibuya Marius Seritan Add ability to save simple mongoid objects (keys hardcoded to :_id). dbcd35d
@mshibuya Marius Seritan Small fix for destroy, enables delete functionality. 1df7ab6
@mshibuya Marius Seritan Fix search crashes, search not working yet. 7651d17
@mshibuya Marius Seritan Fix bulk get and destroy. 4a922c1
@mshibuya Marius Seritan Avoid crash on embedded array document. fb3b846
@mshibuya Marius Seritan Remove debugging. 1cea49e
@mshibuya Marius Seritan Hack search capabilities in.
1. detect LIKE condition generated by the main_controller
2. extract the search string and rebuild Mongoid query with it
789bcbb
@mshibuya Marius Seritan Fix bug introduced by search. bccb4db
@mshibuya Marius Seritan Port to latest head. 4e6ede2
@mshibuya fjg add missing mongoid types 238291b
@mshibuya mshibuya Preliminary support for Mongoid c2028cf
@mshibuya mshibuya Support for filters a22ff21
@mshibuya mshibuya Removed unused codes c773783
@mshibuya mshibuya Added some tests for Mongoid adapter a7ebbfa
@mshibuya mshibuya Mapped Mongoid String type to RA string/text type 82e65a6
@mshibuya mshibuya Fixed the issue with foo_ids= assignment of references_many association f196cfb
@mshibuya mshibuya Followed the changes of upstream f8d2602
@mshibuya mshibuya Fixed spec failure caused by ordering of result set e78a14c
@mshibuya mshibuya Record deletion was not working 68fed35
@mshibuya mshibuya Invoke SimpleCov with 'rake spec:coverage' 7749ecd
@mshibuya mshibuya Mongoid Hash type should be represented as text 1075d68
@mshibuya mshibuya Minimal support for Mongoid Array/Hash types dfe5b34
@mshibuya mshibuya Added specs for ActiveRecord adapter 82858c7
@mshibuya mshibuya Added specs for Mongoid adapter bacfe92
@mshibuya mshibuya Stop bypassing cancan 3536cdc
@bbenezech bbenezech Merge remote-tracking branch 'mshibuya/mongoid' into mongoid
Conflicts:
	app/controllers/rails_admin/main_controller.rb
	lib/rails_admin/adapters/active_record.rb
	lib/rails_admin/config/fields/factories/belongs_to_association.rb
0b6ec58
@bbenezech bbenezech merge latest master into mongoid branch, fix specs 662cb54

I did some changes in active_record.rb. To spare you any useless hassle, I merged master into your branch on sferik/mongoid.

Thanks for your hard work.

Owner

mshibuya replied Mar 6, 2012

OK, I'll take a look later on.
Thanks!

Collaborator

bbenezech commented Mar 12, 2012

Merged 🔨

@bbenezech bbenezech added a commit that referenced this pull request Mar 12, 2012

@bbenezech bbenezech Merge pull request #1031 from mshibuya/mongoid
Mongoid support
a69281d

@bbenezech bbenezech merged commit a69281d into sferik:master Mar 12, 2012

Collaborator

bbenezech commented Mar 12, 2012

Thanks, that's awesome. I'll update Readme and add some doc when I find the time.

Collaborator

bbenezech commented Mar 12, 2012

There are a few spec failures:
http://travis-ci.org/#!/sferik/rails_admin/builds/846265

Nothing big enough to revert. I'm just curious about that one: http://travis-ci.org/#!/sferik/rails_admin/jobs/846268

And disregard the rbx build, it's been failing with Rspec for a while.

Collaborator

mshibuya commented Mar 12, 2012

That's because I forgot to add mongoid gem to Gemfile31.. :/

I'll try to resolve those failures including that one. Please wait for a while.
Sorry for inconvenience.

Owner

sferik commented Mar 12, 2012

@mshibuya This is great! Thanks so much for your work on this.

mshibuya referenced this pull request Mar 12, 2012

Merged

Fixed CI failures #1032

Collaborator

bbenezech commented Mar 12, 2012

@mshibuya It was only spec failures, nothing harmful.
Thanks again for tackling such a big chunk as the Mongoid adapter (and the ORM abstractions that went with it). Your code is really clean, it pleases the eyes!
Unfortunately I haven't found any time to try it, but I'm really eager to.

Owner

sferik commented Mar 12, 2012

@mshibuya I would be happy to add as a committer on the project if you would like. Are there any other issues you're planning to work on?

Collaborator

mshibuya commented Mar 13, 2012

@bbenezech
Without your work on ActiveRecord abstraction, this couldn't be done.
Thanks so much!

@sferik
Really? I'm willing to be!
Currently I have only that test refactoring one, but I'm ready to do more when I come up with another idea.
Thanks for invitation!

Owner

sferik commented Mar 13, 2012

@mshibuya I've added you as a collaborator. Feel free to commit directly to this repo, however, if you need to commit something experimental or untested, please do it in a branch (not master).

Thanks again for your work on RailsAdmin!

Collaborator

mshibuya commented Mar 13, 2012

I understand that.
Many thanks!

khomco commented Mar 15, 2012

I am having trouble figuring out how to configure rails_admin with giving visibility to my embedded documents. Is this supported? I have installed rails_admin successfully and can view "top-level" models, but receive an error with models that relate to being embedded_in. I appreciate any feedback.

Collaborator

mshibuya commented Mar 15, 2012

Yes, Mongoid embedded document is currently not supported, and that's exactly what I'm working on now!
Giving access to embedded document through parent model's nested attribute seems to work.
I think I could push it to master by tomorrow if everything goes fine.

Collaborator

mshibuya commented Mar 16, 2012

OK, I just pushed.
Suppose Article embeds_many Note,

class Article
  include Mongoid::Document
  field :title, :type => String
  ...
  embeds_many :notes
  accepts_nested_attributes_for :notes, :allow_destroy => true
end

class Note
  include Mongoid::Document
  embedded_in :article
  field :subject, :type => String
  field :description, :type => String
end

and you'll get access to document of Note in Article's edit view.

Collaborator

bbenezech commented Mar 16, 2012

Cool!

Sorting doesn't work. It can be solved easily: we should sort on 'title', not 'article.title' (same for conditions).

Include in Mongoid only solves the n+1 pb, it's not nearly as smart as the AR one, which creates a join if you sort or add conditions on the join-tables. There is no join in Mongodb, in fact. Let's find a way to describe the possibilities of the adapter and notify the user about them. (No sorting/conditions on join tables for Mongoid).

kidoman commented Mar 16, 2012

This is awesome guys. I am ready to help you guys test this thoroughly. But I am new to Mongoid and Rails/Ruby though.

Collaborator

mshibuya commented Mar 17, 2012

@bbenezech
As for disability of sorting, I noticed that problem at some point, but I've forgotten to fix it...

Not having joins is really an issue.

For conditions, I've already made a workaround for that.
I didn't want to get rid of the feature to filter records by content of associated records, so I implemented a logic to convert associated model's conditions to target model's conditions, executing search query on associated model, retrieving an array of ids which qualify the conditions. But this logic only works with directly-associated model. not working in indirectly(more than 1 step) associated model.
This could be solved by executing query recursively(searching associated model's associated model's...), but this might affect performance negatively(array of ids can be huge, depending on datasets).
What do you think of this? My opinion is that lacking the ability to search on associated model is worse than performing slow.

And sorting by associated model column appears to be impossible without joins. We should have some way(probably in Adapter's #associations) to indicate that this association is not sortable, and set the field's sortable to false.

Now I'll work on both of that.

@kid0m4n
Don't worry. I'm also new to Mongoid :)
Let's make RA better together!

Collaborator

bbenezech commented Mar 18, 2012

@mshibuya If I were you, I'd stick to ORM's well tested possibilities. People that use MongoDB are aware that it doesn't support joins and shouldn't expect any support for it in RailsAdmin. Instead we might look at interesting MongoDB 'specificities' that can add awesome added-value for little work and is more in line with MongoDB's philosophy.

At the end, if you write too much magic code, users tend to think as RailsAdmin as a magic box, with all the associated pain. :-)

My 2 cents.

Collaborator

mshibuya commented Mar 19, 2012

@bbenezech
What you are saying make sense. Putting everything into ORM adapter layer might be pretty bad thing.

Alternatively, it might better to have configurable search option which enables users to write a ORM-dependent code, by which they make a query on associated model's columns, or any custom query they need.
Currently I have no idea of what that option will be like, but it's promising.

As for now, instead of adding magic feature to Mongoid adapter, I'll prioritize ORM-switchable testing capability.

Thanks for your opinion!

Collaborator

mshibuya commented Mar 27, 2012

I've just pushed my test refactoring works into master.

Now we have capability to run almost all integration tests on each of ORMs!

Collaborator

bbenezech commented Mar 27, 2012

You, sir, are and achiever 👏

This is great! Thanks for your work

Contributor

timkurvers commented Mar 27, 2012

Fantastic work!

mshibuya referenced this pull request Apr 20, 2012

Closed

Support for MongoId #157

Great work!

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