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

made the url_key unique and changed destination_url into TEXT #80

Merged
merged 2 commits into from
Sep 21, 2021
Merged

made the url_key unique and changed destination_url into TEXT #80

merged 2 commits into from
Sep 21, 2021

Conversation

Victor-emil
Copy link
Contributor

@Victor-emil Victor-emil commented Sep 20, 2021

This PR makes the url_key column unique in the database. Currently the code tries to take care of duplicate entries, but when you have a couple of jobs running you might run into situations where two concurrent jobs considers the same key to be available (race condition). To mitigate this I would appreciate if the database would throw an error by using a unique index. This was I can catch the exception in a try catch. This will also make the searches faster when retrieving the key in the ShortURLController.

The PR also changes the destination_url column into TEXT which allows for urls that takes up more space than the allowed current varchar(255) column. We have a lot of urls that contains UTM parameters which can take up more space.

@ash-jc-allen
Copy link
Owner

Hey @Victor-emil! Thanks for the PR (and don't worry, I've not forgotten about the other one) 😄

I totally agree with the idea behind these changes. Would you mind maybe making these to the original migration that creates the short_urls table instead though (https://github.com/ash-jc-allen/short-url/blob/master/database/migrations/2019_12_22_015115_create_short_urls_table.php)?

My main reasoning behind it is that we don't know if anyone's already added the unique index themselves. So, I don't feel too comfortable adding migrations that might cause people any issues. But by putting it in the original migration, it means that any new projects using this package will get to make use of the indexes and column change.

I'm guessing you probably already have, but of course, if you need to make these changes yourself, feel free to publish the migrations and make your own changes there.

@Victor-emil
Copy link
Contributor Author

Victor-emil commented Sep 20, 2021

@ash-jc-allen. Makes sense. I will do that asap. 👍

Yes I already adjusted the table in my own code. 😀

@ash-jc-allen
Copy link
Owner

@Victor-emil Perfect, thanks! 😄

@Victor-emil
Copy link
Contributor Author

@ash-jc-allen. Done!😎

@ash-jc-allen ash-jc-allen merged commit d679124 into ash-jc-allen:master Sep 21, 2021
@ash-jc-allen
Copy link
Owner

Hey @Victor-emil! I'm just dropping you a quick message on this PR too, to let you know that I've just tagged these changes in the v.5.2.0 release. Thanks again for the PR on this one! 😄

https://github.com/ash-jc-allen/short-url/releases/tag/v5.2.0

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.

None yet

2 participants