search

mongoid / mongoid-slug

Generates a URL slug/permalink based on fields in a Mongoid-based model.
https://github.com/mongoid/mongoid-slug
MIT License
492 stars 164 forks source link

Use BSON::Regexp::Raw over Ruby's Regexp when querying the server #238

Closed mzikherman closed 7 years ago

mzikherman commented 7 years ago

This closes https://github.com/mongoid/mongoid-slug/issues/235

As discussed in that issue, and then continued in the discussion https://github.com/mongodb/bson-ruby/pull/71#issuecomment-290067567, it's better to use Regexp::Raw over Ruby's Regexp when querying the server. This allows prefixed expressions to use an index.

However, when querying embedded documents that are already loaded in-memory, we should just be using Ruby's.

This isn't new functionality, so I didn't add any specs besides ensuring the current suite is passing.

However, as mentioned in https://github.com/mongoid/mongoid-slug/issues/235:

Then, is there a way to write a test for this in mongoid-slug (eg. examining the query results and making sure it hit an index or the number of documents scanned was 1 when there're 2 records in the DB for example), and then way to modify the query and remove that option in mongoid-slug?

This seems like an awesome test! And one that is appropriate for this PR, as that's the crux of the change. I'll give that a shot, but wanted to open this PR in the meantime for feedback.

mongoid-bot commented 7 years ago
1 Warning
:warning: There’re library changes, but not tests. That’s OK as long as you’re refactoring existing code.

Generated by :no_entry_sign: danger

dblock commented 7 years ago

So does it not work for embedded? or the older versions of Mongoid with Regexp::Raw and why? The case implies that it doesn't, otherwise it would make things much simpler in code, no?

mzikherman commented 7 years ago

Yea, embedded? docs are already loaded in memory and can just use Ruby's Regexp. There's an open PR on Mongoid to bring that in to avoid the special handling for embedded docs.

The older versions of Mongoid bring in earlier versions of the driver (+ BSON gems) that don't include Regexp::Raw, but also don't include the update that makes it not performant to use Regexp on the server.

dblock commented 7 years ago

Thanks @mzikherman. Would you like to make a release? Happy to add you to maintainers + what's your rubygems email?

mzikherman commented 7 years ago

Yea, I'd love that! I am mzikherman@gmail.com on Rubygems

dblock commented 7 years ago

Added you here and Rubygems @mzikherman. Maybe you can also contribute a RELEASING doc ala https://github.com/mongoid/mongoid-compatibility/blob/master/RELEASING.md?

Btw, you're matt[at]artsymail.com on rubygems...