Collision and performance issue

[hari]

Collision and performance issue

[ash-jc-allen]

Hey @hari!

Thank you for the kind words, I really appreciate it! 😄

The performance issues for larger apps is something that seems to be popping up quite a lot, so I feel like this is something I'm definitely going to have to look at.

In all honesty, when I first made this package, I didn't expect that anyone was going to use it. I definitely didn't expect it to be used in larger apps with 1k+/10k+/100k+ short URLs haha! So there are some decisions I made when building it that didn't really think too much about performance because it wasn't really on my mind at the time.

Case-sensitivity is something that I'd never considered and I think that'll explain a lot of other people's issues too! I'd definitely be open to a PR for that!

With regards to the using max(), can you see this causing any issues at all? I think I like the idea of switching over to this, but I'm just trying to think if it'd cause any problems for existing apps using the package? Either way, I think I'd be open to a PR for this 😄

In the near future, I'm intending on using an interface for the key generator class too. I'll be making this part of the package configurable so that you can swap out for your own implementation to generate the keys if the package's default implementation doesn't fit your apps use case anymore. I'm hoping this will make things a lot more flexible 🙂

[ash-jc-allen]

Sweet! I'm in the process of getting my new book released (release date is 3rd October), so I doubt I'll be able to look at the PRs before that date, so there's no rush to make them 🙂

[hari]

Thank you for the response @ash-jc-allen. Switching to max() shouldn't cause any issues as it is just making a native SQL call.

I will submit 2 PRs this week.

[hari]

Hi @ash-jc-allen

I was wondering how the migration should be added for this change. I read your comment in another PR regarding modifying the existing migration for the base table. Should we do that or add a new migration for changing the column's collation?

Thank you

[ash-jc-allen]

Oooh that's a good question! For similar reasons mentioned in the comment you linked to, I'd be tempted to only make the changes in existing migration. Or if there's any performant way to handle this in the package's code without touching the database (I don't know if that's possible), then I'd be open to that too 🙂

[avivsalman]

Hi @hari & @ash-jc-allen ,

if you are on performence issues, so i just want to give you one more thing that i saw, each generate of new key, there is 2 database query of the model exist function, i think that it need to be only once.

[ash-jc-allen]

Hey @avivsalman! I'm always open to reviewing any PRs that can help to improve the performance of the package, if you fancy making one? 😄