Fix combined title+author API query producing invalid SQL (#268)

[notartom]

Previously, _build_author_project_id_list() and
_build_genre_project_id_list() ran their subqueries after the main
query's WHERE clauses (e.g. p.title) had already been added to
CodeIgniter's shared query builder. Because the builder is stateful
and accumulates WHERE clauses until a ->get() consumes them, clauses
referencing the 'p' alias leaked into the author/genre subqueries,
which query different tables without a 'p' alias, producing invalid
SQL. Additionally, _build_author_project_id_list() could return 0
instead of an empty array when no authors matched, which
where_in() would not handle correctly.

With this patch, both subqueries are moved to the top of
_build_data_set(), before any main query WHERE clauses are added to
the builder, so the builder is still clean when they execute. Both
helper methods now also always return a results array (never 0),
making the subsequent empty() guard and where_in() behave correctly
in all cases. Together, these changes fix both underlying root
causes: the WHERE clause leaking into the subqueries, and the
potential return of 0 being unhandled.

A regression test is included that requests the API with both title
and author params and asserts a 200 response.

Co-Authored-By: Claude Opus 4.6 noreply@anthropic.com

[notartom]

@redrun45 thoughts? Claude Code could be controversial here. I wanted to play around with it to see what it can do - and it's genuinely impressive as an accelerator if you know what you're doing in the first place, environmental and intellectual property concerns aside.

[redrun45]

Well, I can see the appeal... but I also have questions. Those other concerns you mentioned would be worth talking to more folks about, but here's what I see in this PR. I don't know whether I would have made the same mistake, but I do know there is at least one.

We have our if block, with two statements in it. We find out we need to move the _build_..._list() call so that it's earlier in the process of building the query. Is there a reason we can't move the where_in() call up there, too? Have we checked?

If we do need to have them in different places, then we don't get the benefit of seeing them all together, so we need to be very careful about the way their conditions overlap (or don't). Is there a reason we switched this original condition from checking whether the author search parameter is empty, to checking whether the list of authors matching the search is empty?

On live, if we search by author name, and that author doesn't exist, then we will get no results. With this PR as written, we would instead get results from any author, as if we were not searching by author name at all.

[notartom]

To be clear, I didn't just tell Claude "fix the bug" and push the PR. There were iterations and guidance along the way, with me trying to understand and double check what it did. So any mistakes are my responsibility, not AI's ;)

In terms of having them in separate places... I genuinely don't know if there's a better solution. This is such messy spaghetti code that it's hard to follow all the branches and the WHEREs and the LIKEs. Assuming we do need them in separate places, you raise a good point that the conditions have been subtly changed with the current proposal. If there's an author clause in the search but it comes up empty, the expected behaviour is to have the entire search turn up nothing, because there's no author match. With this proposal, we'd still run all the other clauses, and treat the search as if the author clause didn't exist.

I'll need some time to fix this, mostly because I want the tests to cover all the cases...

[redrun45]

Understood - and all the kudos for doing more test cases! Often, that takes far longer than the actual code change...

I suppose there's already enough difficulty in guessing an author's thought processes, that we really should assume nothing.
If it were me writing this, I'd have defaulted to keeping things together if I could. If someone sent me this PR without any explanation, I might assume they did too, but there was a reason to deviate from that. And maybe that wouldn't be a safe assumption, AI-assistance or no. 🤔

[redrun45]

A potentially different technical avenue: these searches are essentially sub-queries - they run first, and then the main query is restricted to only the project IDs they return.

Key word being return, as it looks like they are both built and executed before the main query begins execution. If we're "leaking" WHEREs from these into the main query, that would seem to be a bug too, even if it's not causing other problems now.

Like you said... spaghetti code. 😓