Skip to content

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

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.

Sign up for GitHub

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

Open
notartom wants to merge 1 commit into
base: master
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 23 additions & 17 deletions application/libraries/Librivox_API.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,22 @@ function _build_data_set($params)

*/

// Run subqueries first, while the query builder is clean. The
// builder is stateful: WHERE clauses accumulate until a ->get()
// consumes and resets them. If we added main query clauses (e.g.
// p.title) before running these subqueries, they would leak into
// the subqueries (which query different tables without a 'p'
// alias), producing invalid SQL.
if (!empty($params['author']))
{
$author_project_id_list = $this->_build_author_project_id_list($params['author']);
}

if (!empty($params['genre']))
{
$genre_project_id_list = $this->_build_genre_project_id_list($params['genre']);
}

if ($params['id']) $this->db->where('p.id', $params['id']);

if ($params['since']) $this->db->where('UNIX_TIMESTAMP(STR_TO_DATE(p.date_catalog, "%Y-%m-%d")) >= ', $params['since']);
Expand All @@ -116,17 +132,14 @@ function _build_data_set($params)
}
}


if (!empty($params['author']))
if (!empty($author_project_id_list))
{
$project_id_list = $this->_build_author_project_id_list($params['author']);
$this->db->where_in('p.id', $project_id_list);
$this->db->where_in('p.id', $author_project_id_list);
}
Comment on lines +135 to 138

@redrun45 redrun45 Feb 15, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 notartom Feb 15, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 redrun45 Feb 15, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 redrun45 Feb 15, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. 😓


if (!empty($params['genre']))
if (!empty($genre_project_id_list))
{
$project_id_list = $this->_build_genre_project_id_list($params['genre']);
$this->db->where_in('p.id', $project_id_list);
$this->db->where_in('p.id', $genre_project_id_list);
}


Expand Down Expand Up @@ -311,11 +324,8 @@ function _build_author_project_id_list($author)
->result_array();

require_once(APPPATH.'libraries/Underscore.php');

$project_ids = __()->pluck($result , 'project_id');

return (empty($project_ids)) ? 0 : $project_ids;
//return implode(', ', $project_ids);
return $project_ids;

}

Expand All @@ -336,13 +346,9 @@ function _build_genre_project_id_list($genre)
->get('genres g')
->result_array();

//return $result;

require_once(APPPATH.'libraries/Underscore.php');

return $project_ids = __()->pluck($result , 'project_id');
//return implode(', ', $project_ids);

$project_ids = __()->pluck($result , 'project_id');
return $project_ids;
}

}
24 changes: 24 additions & 0 deletions application/tests/controllers/api/Feed_audiobooks_test.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
<?php

/**
* Regression test for part of issue #268:
* https://github.com/LibriVox/librivox-catalog/issues/268
*
* When both title and author params are provided in _build_data_set(), the
* subqueries (author, genre) must run before any main query WHERE clauses are
* added to the shared query builder, otherwise clauses like 'p.title' leak
* into subqueries that don't have a 'p' table alias, producing invalid SQL.
*/
class Feed_audiobooks_test extends TestCase {

/**
* @link https://github.com/LibriVox/librivox-catalog/issues/268
*/
public function test_title_and_author_combined_does_not_error() {
$output = $this->request(
'GET',
'api/feed/audiobooks?format=json&title=test&author=NONEXISTENT'
);
$this->assertResponseCode(200);
}
}