Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes critical bug in collision-resolver code, adds prefix and suffix options. #24

Closed
wants to merge 11 commits into from

Conversation

dprater
Copy link

@dprater dprater commented Feb 28, 2013

This commit includes a fix for a bug I came across in the "resolve_token_collisions" method. In the current code, if a field other than the one being used by mongoid_token encounters a duplicate key error, the gem crashes because it attempts to "continue", which is not valid ruby. This update properly checks to see if the mongoid_token field is the one with the duplicate key - if it is, it handles it as before by attempting to regenerate the token. If the duplicate key error is not being caused by the field used by mongoid_token, it simply re-raises the error as it should, whereas before it was not taking any further action.

It also adds a prefix and suffix options that allow the user to specify a prefix and/or suffix to be added to each generated token. Added documentation and specs for updated code.

@thetron
Copy link
Owner

thetron commented Feb 28, 2013

Thanks very much for this @dprater. I'm really not a huge fan of that collision retrying nonsense at all - and am still on the fence as to whether it should just be pulled out of the gem all together. I appreciate you sorting this out though, it's always been a thorn in my side.

Do you think it would be possible for you to extract the collision fix into a separate pull request? The reason I ask is that I am currently working on a huge refactor for the gem. The update includes an overhaul to the token generator which allows the ability to define a token's structure using a pattern, which naturally allows for things like prefixes and suffixes (and others) - as well as support for the existing simpler structure definition.

If you're interested, you can check it out here: feature/token-patterns.

Thanks again though, if you can make those changes to the pull request, i'd love to push our a release as soon as possible.

@dprater
Copy link
Author

dprater commented Mar 1, 2013

I pulled out the prefix/suffix code, as requested. Out of curiosity, any indication on when you might have your rewrite ready? I ask because we're currently using a forked version of mongoid_token that I created that has the prefix/suffix code (as well as the collision fix, obviously). I'd prefer to use yours, but it's not a big deal for now. No worries, just wondering.

@thetron
Copy link
Owner

thetron commented Mar 6, 2013

Thanks @dprater - sorry I totally missed the notification of your response.

I think it's about 90% there? I'm just trying to get the last of my refactoring done, and then it will be good-to-go. I might see if I can wrap it up this week, but I wouldn't pin my hopes on it! I have a deadline in the coming weeks which i'm sure will swamp me.

@thetron
Copy link
Owner

thetron commented Apr 12, 2013

Thanks again @dprater - this was really helpful. This was no longer compatible with the 2.0.0 branch (because the codebase has been refactored into modules), but I've integrated your changes into the new structure.

@thetron thetron closed this Apr 12, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants