thetron / mongoid_token

A little random, unique token generator for Mongoid documents.
MIT License
182 stars 91 forks source link

What happens if it generates duplicate token? #1

Closed cielo closed 12 years ago

cielo commented 13 years ago

I like this gem, but what happens if it generates duplicate token??? Also, if all possible combinations are already in db, what happens then? Do user manually have to increase the length of the token?

thetron commented 13 years ago

Hi cielo,

The tokens are always unique and are checked for collisions before saving, however, yes - the length of the tokens must be manually increased. As the number of records approaches the total number of tokens, there is a noticeable performance dropoff.

I'm planning on doing some benchmarks to quantify an appropriate token length, given the expected size of a database.

cielo commented 13 years ago

Thank you for your response. I think I do not have to worry about the case that all possible combinations gets taken. For example, if I use alphanumeric with length of 4, then

(26 + 26 + 10)^4 = 14 776 336 possible combinations,

and if I set length=5

(26 + 26 + 10)^5 = 916 132 832 possible combinations.

That is just enough for me. :) Thank you so much.

thetron commented 13 years ago

Not a problem at all.

I've been running some benchmarks this afternoon, trying to work out the best token length to choose, and it would seem that you should expect to only use 1 - 1.5% of the total number of available tokens. Anything more than that and performance starts to drop off noticeably.

So in your case of using a length of 5, you should only expect to have a maximum of around 14,000,000 documents.

eagleas commented 13 years ago

Imho, correct alghorithm may be:

  1. Generate token
  2. Save object without checks unique
  3. select(:conditions => {:token => self.token}).count
  4. if found >1 objects - repeat 1-3 steps (but only ONE time!, if more - raise exception), else done. This sequence of steps guarantees unique tokens, even if dublicates arose at the moment between create and save steps. Nicholas, what do you think about?
thetron commented 13 years ago

I definitely agree with modifying the algorithm to reduce chances of collisions between create and save - this was something I hadn't really considered.

I do have concerns with raising an exception after the first failure though. I think the gem probably needs a configuration value that you can set for the number of retries before throwing the exception - so that developers can tailor it, depending on their application's demands.

eagleas commented 13 years ago

Yep, configurable number of retries - good idea.

eagleas commented 13 years ago

Another path: building on Mongo unique indexes http://www.mongodb.org/display/DOCS/Indexes#Indexes-UniqueIndexes, and throw exception on save. Do not check doing before save.

thetron commented 13 years ago

I already do have an unique index setup by default when you add a token to a model, the index is optional though, which means we'd still need something to for when the index isn't in place (unless I make the index mandatory?).

thetron commented 13 years ago

The problem seems to be that either mongodb isn't throwing the exception, or mongoid is ignoring it. Because I seem to have no trouble adding multiple records with the same unique index value.

Or i'm doing something really wrong.

eagleas commented 13 years ago

Forgotten Model.create_indexes ?

thetron commented 13 years ago

I've tested it with and without. Weird.

Might do this in stages instead of working out why it's not throwing the exception. I'm going to implement the retry system, which will throw a Mongoid::Token::CollisionRetriesExceeded exception. And then i'll look at this indexing issue again.

thetron commented 13 years ago

Alexander,

I've just pushed up some changes, which includes the new generate -> save -> validate -> rollback algorithm for token generation. I've also updated the spec to test all these new methods, which appears to be ok - I'd certainly appreciate a look over it to make sure i've not made any silly mistakes.

I noticed there seems to be a bug in Mongoid too, it would seem that raising an exception in the after_save callback does not trigger a rollback on the database record (like it does in active record) - so i've manually added a call to self.destroy. I don't like it that way, but it was the only way I could ensure the record would get properly cleaned up.

skyeagle commented 13 years ago

I think it's wrong way. How about a to rescue native mongodb unique index validation error? We don't need additional queries to check uniqness of a token. Having unique index and saving instance in safety mode more than enough. It will raise duplicate index error in safe mode. I suppose one of the other ways is overlay saving methods in mongoid with catching this error, changing token and retry saving. Loop the cicle a few times until we have no a success result.

thetron commented 13 years ago

I had tried that - but couldn't get mongoid to raise the unique index validation error. I'm sure I'm doing something wrong, but for the life of me can't work out what's causing it.

I'd much prefer to rescue from the mongodb exceptions - but when with the extra query in the interim, till we work out a proper solution.

eagleas commented 13 years ago

Nicholas, please see safe mode MongoDB, and non-safe mode with getLastError. Exactly safely saving

skyeagle commented 13 years ago

thetron, Had you created indexes with Model.create_indexes before you tried to rescue exception?

eagleas commented 13 years ago

Why MongoDB can create unique index but Mongoid cannot?

thetron commented 13 years ago

Ah! Thanks....currently doing a rewrite, so we'll see how it looks after that.

skyeagle commented 13 years ago

How about this implementation:

  before_create :set_token

  def upsert_with_safety(args = {})
    retryable(:tries => 2, :on => Mongo::OperationFailure) do
      safely.upsert_without_safety(args)
    end
  end
  alias_method_chain :upsert, :safety

  private

  def set_token
    self.token = self.class.generate_token
  end

  def self.generate_token
    (1..7).collect { (i = Kernel.rand(62); i += ((i < 10) ? 48 : ((i < 36) ? 55 : 61 ))).chr }.join
  end

It replaces only #save(aka upsert) but it could be overwritten for save! too. retryable is described here It's very simple and clean. We are trying to re-save if saving is failure due to duplicate index error.