Closed colagrosso closed 2 months ago
The issue is that in this approach, we've denormalized the database - now we have the same author information stored in two places. This makes it easier to write joins, but data integrity is eventually going to become a problem - at some point someone, maybe not even you or me but someone years from now, is going to edit the DB directly to update an author and forget to change one or the other table, or mistype into one or the other table, and now we have a big problem that will be difficult to diagnose.
Also, we lose the ability to search by just one of the authors, or to search for the authors in an order that's different from our slug.
The approach in the other PR is just fine, and should be just as performant if we index both columns, especially since the case of multiple authors is pretty uncommon.
Fair enough, thanks for the feedback. I'll take your advice in #377 and polish that PR up.
This PR is longer because I changed my approach: In the
Ebook
class, I convertedAuthorsUrl
from an on-demand property to a field written to the database with an index.Most of the PR is that change:
config/sql/se/Contributors.sql
: Remove theUrlName
index onContributors
. More on that in a moment.config/sql/se/Ebooks.sql
: WriteAuthorsUrl
to the DBlib/Library.php
andwww/ebooks/author.php
: The purpose of the PR, i.e., a straightforward DB query on a column with an index to get ebooks by an author.My original plan was to use the
UrlName
index onContributors
and join that with theEbooks
table. That works fine for single authors, but it was hard to make it work for ebooks with multiple authors like:https://standardebooks.org/ebooks/william-wordsworth_samuel-taylor-coleridge/lyrical-ballads https://standardebooks.org/ebooks/karl-marx_friedrich-engels/the-communist-manifesto/samuel-moore
because the
Contributors
table has entries for them individually, not together:If you'd like to see that alternative, I created draft PR #377. I don't recommend that approach. The implementation in
Library::GetEbooksByAuthor()
would be harder to maintain. I'm showing that alternative because it was my initial intent, and I thought you might be interested.