Collision and performance issue

[hari]

Hello,

First and foremost, a big thank you for your dedication to maintaining this library. We genuinely appreciate your hard work.

We've been using this library in our production-grade application and came across a couple of issues that we'd like to discuss and propose solutions for.

Background:

1. Collision: The url_key column is currently case-insensitive, which has led to it treating two different keys like aLlen and allen as identical. To address this, we believe it would be beneficial to resolve this at the package level. This way, users won't need to be concerned about case sensitivity when utilizing the library.

2. Performance: The library currently fetches the latest ID using ShortUrl::latest(), which, while functional, is not the most performant approach. It sorts the rows by created_at date, which can be resource-intensive. A more efficient method would be to use ShortUrl::max('id'), as the id column is indexed by default. This change could significantly boost performance for users.

We'd love to hear your thoughts on these proposed changes. If you're open to it, we can prepare a pull request to implement these improvements.

Alternatively, for the collision issue, we could consider adding a note to the documentation section, explaining why users might encounter this situation and how to handle it.

Once again, thank you sincerely for your time and effort in maintaining this package.

Best regards, Hari Lamichhane

[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 🙂

[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.

[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]

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? 😄