Key out of sync with short url's ID

[fhusquinet]

Hi!

Yesterday evening, I've noticed a lot of my queue emails were failing in Horizon after a 60s timeout. Usually, they don't take more than 3-4s to process, sometimes a bit more if a few things had to be processed first.
After digging in, I've noticed that the KeyGenerator@generateRandom() method had to do its do while loop about 17 thousands time to find a unique key. This caused a lot of database queries, and unnecessary strain on my MySQL instance, and ultimately caused the timeout issue. I've fixed it by increasing the key length for 10 to 12, basically "resetting" this check, but I'm curious if anyone encountered this?

The method's comment state that it should be pretty rare for one key to collide with an existing ID, but after checking my database the model with the ID of 161089 had a key matching the ID 179014...

Obviously once an ID collides, all future checks will always collides as well forcing the app to do one extra loop at the very least, but in my case it's a lot more...
Note that my app contains about 175k rows now, since September 2019.

This might be a bit out of the scope of the package, but I was thinking about maybe logging if this issue occurs. Like throwing an exception that doesn't stop the execution of the loop but simply warns the user that the data might be falling out of sync if it has to execute more than X times?
It might also be less performant, but it would also be possible to decode the last key found and increment its value instead of the ID. Obviously I could do this inside a custom key generator class, but if anyone had this issue it might be worth considering adding a few checks in the package directly, it wasn't a fun issue to debug I have to say... 😅

[ash-jc-allen]

Hey @fhusquinet!

First off, I'm really sorry that you ran into that problem because of the package! 😞

Am I right in saying that your short_urls table has over 175k rows? If that's the case, that's really cool. When I first built this package, I never imagined it'd be used this much haha 😄

I'd definitely be open to adding some kind of option or method (e.g. - onCollision()) that's called whenever there's a collision when trying to generate a random key. Off the top of my head, I think this method would allow you to pass in a closure that's invoked when a collision occurs. In this closure, you could maybe choose to report/log the collision, or maybe throw an exception entirely.

Do you think something like this would probably help you in this case?

[fhusquinet]

Don't apologise! The package is great and issues can always creep up after a while.

Yes that's right, the table has about 175k rows. It's expected to grow in the future as well, at the rate of 1k/day at the moment but hopefully 3-4x that before the end of the year.

They are all created into a queue, which can handle multiple jobs at the same time and probably created this issue.
For example, a user can send an email to their client and request a copy. We decided on sending two separate emails to track them independently but they are both sent to the queue in the same request, which might very well be a cause of this.

Having a log or a way to hook into it would certainly help though I'm not sure it should be included in the package itself. Maybe an extra KeyGenerator class that would be considered a bit slower but safer that we can switch to via the config?
I'm not sure what the "key" attribute holds as a value, except for it being a random key. If the goal is simply that and never to decode it to retrieve it, then maybe retrieving the last key created, decoding it then incrementing it would be better? While still using a do while loop just in case another model is created at the same time?