Replace search and insert by insert and catch when creating short URLs

[marijnvandevoorde]

Today, Shlink will first find and then insert the shorturl in the db.
It would be interesting to compare the performance between this approach and trying to create and catch the cases where it fails and retry with a new short url. Given the odds are minimal that the short url is taken, I'd say the first query is useless in 99+% of the cases.

The unique index on short code + domain already exists, so an insert should fail when trying to introduce a duplicate.

Happy to explore this and benchmark, when I have some time that is :D

[acelaya]

There's a couple considerations.

The first one is that Shlink does not check if the short URL exists based on the short code when the short code is auto-generated. In that case, the purpose of findIfExists is avoiding to accidentally create two different short URLs that point to the same place. Therefore, the existing short-code+domain unique index would not prevent another short URL to be created.

The other considerations is that, for an upsert approach to work, there needs to be unique indexes for all the fields that findIfExists takes into consideration (valid_until, valid_since, max_visits, tags, et., etc.), which is not practical, because you may want to to create "duplicate" URLs.

Ignore this. See the comments below

[acelaya]

Oh! Sorry, my mistake. I think I jumped into conclusions because we were discussing this other issue in parallel. I'll edit the title again.

Yeah, in that case I guess it could make sense, but I would need to check if there was some reason to implement it this way.

One thing I can think of is the fact that exceptions thrown all the way down from the database are not easy to reliably identify, and might require introspecting a message which could be different between database engines and versions.

But that's just a blind guess.

[marijnvandevoorde]

Fair point for sure. I'm not sure how to what extent doctrine will handle that for you.

[acelaya]

I was poking a bit with this, trying to see what kind of exception is thrown when inserting data that "breaks" a unique index, and I think I know why I implemented this with a SELECT right before the INSERT.

Since the domain_id is nullable, MySQL/Maria do not properly trigger an error when inserting two short URLs with the same short code, as long as they belong to the default domain. I haven't checked if other engines behave the same, but it's a bit unintuitive. See https://stackoverflow.com/questions/23155190/unique-index-allows-duplicate-entries-in-case-that-one-of-entries-is-null

If I set a domain, then it does throw a Doctrine\DBAL\Exception\UniqueConstraintViolationException.

The domain_id is nullable for backwards compatibility reasons. It's not a great way to handle it, and it should be approached differently probably, but it could break many scenarios, so it needs to be done carefully.

[marijnvandevoorde]

This one makes sense, I didn't know that was the behaviour MySQL & Maria had. There's no way around it then, so feel free to close this.

[acelaya]

It still feels wrong, and a side effect of a technical decision made at some point. The fact is, domain_id should probably not be nullable, and the default domain should be represented somehow in the database.

I'll try to investigate a bit, if not to fully solve the problem, at least to have a better understanding of the implications, so that it is eventually possible to avoid the extra SELECT.

But that will have to wait until I start focusing on Shlink 4.2, for now my focus is on shipping a pre-release of shlink-dashboard.