Kovah / LinkAce

LinkAce is a self-hosted archive to collect links of your favorite websites.
https://www.linkace.org
GNU General Public License v3.0
2.5k stars 159 forks source link

Match tag names case-insensitively #776

Closed chrissawyerfan4 closed 5 days ago

chrissawyerfan4 commented 3 months ago

True (user) story

When I add a link from mobile with a tag, the keyboard will automatically capitalize the first letter of the tag I am adding. My expectation was that the system would recognize the preexisting tag but, instead, an entirely new tag was added and I now have both book and Book in existence.

As a link adder, I want the system to recognize a preexisting tag despite case differences such that I do not have to fiddle with the keyboard autocapitalize. (I've looked into disabling that behavior before for another project, but found no reliable way to mark the input field for mobile web browsers.)

So that is my side of the story; now, whether you want to also incorporate this change upstream depends. If anyone is relying on case differences to work correctly, this will break their ability to add links to one of the tags: the database will match one of them first and that will be the one assigned to the link, losing the ability to assign to the other one. It would not be my expectation, though, that someone is maintaining both Name and name and carefully adding some links to one but not the other.

I fully understand if you do not like the change and don't want to merge it, no hard feelings; I could have discussed it first but it was a small change and a good excuse to mess with Podman containers :)

Compatibility

While searching for prior discussion, I noticed you wrote this:

There are indeed "fixes" for making case insensitive searches in Postgres, but I don't want to change any search logic to support special use cases for different databases.

Agree, that's ugly and should be avoided if possible. I've verified that the SQL function is supported by all databases mentioned in the LinkAce setup documentation, including the databases that are marked as "untested, might work":

I've run the test suite on the default database which I'm pretty sure is MariaDB, and the change runs on my own setup which uses SQLite. In the latter, I have been adding test links and doing things like removing the tag first or reassigning all links from it first, trying to create a duplicate tag name from the /tags/create page itself, clearing them from trash, etc., and it all works correctly.

Implementation

The first bit of the new function checks if we are looking for a record with a specific name. If so, it will search for it by lowercasing both the existing database records and the name being searched for. They both use the SQL lower() function, rather than using strtolower() on the search value, to avoid any discrepancies there.

If this does not return a result, or we were not doing a name search, the rest of the implementation mimics the original code and should thus work identically. Ideally, it would just call that code, but this method is not static and so cannot be directly called. This seemed simpler than getting that parent callable.

The tests are added to the LinkCreateTest.php because the tag find-or-create is called from the link creating/updating code. It has been a while since I worked on a project with unit tests, let me know if this is not how it's supposed to be done.

Future work

Kovah commented 2 months ago

Hey, thanks for taking so much time to dig into this and craft a possible solution. I would like to think about this and run some tests. I am not yet sure if I really want to go into this direction, as it may have serious impact on existing instances and how tags are used there. For example, using Emoji or other special characters in lists and tags is quite difficult and altering those values may break them completely.