Bug 30677 - Unknown column 'biblioitem.title' in 'where clause' 500 error in API /api/v1/acquisitions/orders
Summary: Unknown column 'biblioitem.title' in 'where clause' 500 error in API /api/v1/...
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: master
Hardware: All All
: P5 - low critical (vote)
Assignee: Andrew Nugged
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 30165
Blocks:
  Show dependency treegraph
 
Reported: 2022-05-04 14:05 UTC by Andrew Nugged
Modified: 2022-06-28 12:41 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.11.00


Attachments
Bug 30677: Add tests (2.01 KB, patch)
2022-05-06 11:56 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30677: Add tests (2.07 KB, patch)
2022-05-11 15:50 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30677: Use lookahead in regex for biblioitem replacement (1.05 KB, patch)
2022-05-11 15:50 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30677: Cleanup (912 bytes, patch)
2022-06-28 11:31 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30677: Add tests (2.16 KB, patch)
2022-06-28 11:56 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 30677: Use lookahead in regex for biblioitem replacement (1.15 KB, patch)
2022-06-28 11:56 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 30677: Cleanup (1010 bytes, patch)
2022-06-28 11:56 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Nugged 2022-05-04 14:05:53 UTC
The exception:

[ERROR] GET /api/v1/acquisitions/orders: unhandled exception (DBIx::Class::Exception)<<DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st execute failed: Unknown column 'biblioitem.title' in 'where clause' at /usr/share/perl5/Data/Page.pm line 49

Because here is the experimental dump of what sub table_name_fixer replaces:

in:

  'q' => '{"-and":[{"basket.vendor_id":2},[[{"basket.name":{"like":"%f%"}}],[{"basket.basket_group.name":{"like":"%f%"}}],[{"me.order_id":{"like":"%f%"}}],[{"biblio.title":{"like":"%f%"}},{"biblio.author":{"like":"%f%"}},{"biblio.isbn":{"like":"%f%"}},{"biblio.publisher":{"like":"%f%"}},{"me.internal_note":{"like":"%f%"}},{"me.vendor_note":{"like":"%f%"}}],[{"me.replacement_price":{"like":"%f%"}}],[{"me.quantity":{"like":"%f%"}}],[{"me.ecost":{"like":"%f%"}}],[{"fund.name":{"like":"%f%"}}]]]}'
};

out:

  'q' => '{"-and":[{"basket.vendor_id":2},[[{"basket.name":{"like":"%f%"}}],[{"basket.basket_group.name":{"like":"%f%"}}],[{"me.order_id":{"like":"%f%"}}],[{"biblio.biblioitem.title":{"like":"%f%"}},{"biblio.biblioitem.author":{"like":"%f%"}},{"biblio.biblioitem.isbn":{"like":"%f%"}},{"biblio.biblioitem.publisher":{"like":"%f%"}},{"me.internal_note":{"like":"%f%"}},{"me.vendor_note":{"like":"%f%"}}],[{"me.replacement_price":{"like":"%f%"}}],[{"me.quantity":{"like":"%f%"}}],[{"me.ecost":{"like":"%f%"}}],[{"fund.name":{"like":"%f%"}}]]]}'
};


That because this:

    $q =~ s|biblio\.|biblio\.biblioitem\.|g
      if $q =~ m/.*(isbn|ean|publisher).*/;

should be with lookahead to exclude false matches:

    $q =~ s{biblio\.(?=isbn|ean|publisher)}{biblio\.biblioitem\.}g;

Making a fix.
Comment 1 Andrew Nugged 2022-05-04 14:31:11 UTC Comment hidden (obsolete)
Comment 2 Andrew Nugged 2022-05-04 15:27:41 UTC
we need to add Tests, and some reproduction stuck: some settings, TBU...
Comment 3 Tomás Cohen Arazi 2022-05-04 16:37:44 UTC
There are some regression tests here:

https://git.koha-community.org/Koha-community/Koha/src/branch/master/t/db_dependent/api/v1/acquisitions_orders.t#L115

you should extend them with your cases if you confirm them. Thanks!
Comment 4 Jonathan Druart 2022-05-06 11:56:20 UTC
Created attachment 134724 [details] [review]
Bug 30677: Add tests
Comment 5 Martin Renvoize 2022-05-11 15:50:51 UTC
Created attachment 134900 [details] [review]
Bug 30677: Add tests

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 6 Martin Renvoize 2022-05-11 15:50:54 UTC
Created attachment 134901 [details] [review]
Bug 30677: Use lookahead in regex for biblioitem replacement

This patch takes Andrew's suggested fix using a lookahead regex to
correct our biblio vs biblioitem table name replacements.

Please use the preceeding unit test patch proposed by Jonathan to test.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 7 Martin Renvoize 2022-05-11 15:51:57 UTC
All looks good to me and both manual testing and the unit test appear happy.

Signing of.. over to QA
Comment 8 Jonathan Druart 2022-05-12 10:24:42 UTC
Regarding comment 2 this is not ready. Andrew, any news on this?
Comment 9 Andrew Nugged 2022-05-12 11:07:02 UTC
I was stuck in the reproduction of this issue, but an only day ago I realized that I provided wrong place where this should be reproduced

if team found (and I see it is) then it's all ok as for me,
Comment 10 Andrew Nugged 2022-05-12 11:10:35 UTC
I want to propose as QA-phase only this:

That "if" is _probably_ not needed:

      ... if $q =~ m/.*(isbn|ean|publisher).*/;

because this anyway matches only those places:

   $q =~ s{biblio\.(?=isbn|ean|publisher)}{biblio\.biblioitem\.}g;

so having this:

    $q =~ s{biblio\.(?=isbn|ean|publisher)}{biblio\.biblioitem\.}g
       if $q =~ m/.*(isbn|ean|publisher).*/;

is something like "double-check for the same thing",
But this is more like 50/50 proposal to agree with you JD/Martin.
Comment 11 Marcel de Rooy 2022-05-13 09:15:00 UTC
+      ->json_is( "/0/biblio/isbn", 'SOMETHING', 'Filtering by a biblioitem column works!' )
+      ->json_is( "/0/biblio/title", $biblio->title, 'Filtering by a biblioitem and biblio columns works!' );

Descriptions refer tot biblioitem. I thought that was the problem in the first place.
Comment 12 Marcel de Rooy 2022-05-13 09:15:52 UTC
Still looking
Comment 13 Marcel de Rooy 2022-05-13 09:27:34 UTC
(In reply to Andrew Nugged from comment #10)
> I want to propose as QA-phase only this:
> 
> That "if" is _probably_ not needed:
> 
>       ... if $q =~ m/.*(isbn|ean|publisher).*/;
> 
> because this anyway matches only those places:
> 
>    $q =~ s{biblio\.(?=isbn|ean|publisher)}{biblio\.biblioitem\.}g;
> 
> so having this:
> 
>     $q =~ s{biblio\.(?=isbn|ean|publisher)}{biblio\.biblioitem\.}g
>        if $q =~ m/.*(isbn|ean|publisher).*/;
> 
> is something like "double-check for the same thing",
> But this is more like 50/50 proposal to agree with you JD/Martin.

Yes that if is horrible for a purist.
And note that matching .* is an insult already.
We just wanted m/isbn|etc/ and that was not even very specific matching.

Also note the separator switching here:
Please use s/ a / b / and m/c/ for instance, but not do s| | |  and m/ / 
It hurts my eyes :)
Comment 14 Marcel de Rooy 2022-05-13 09:28:33 UTC
Please tidy both patches. FQA
Comment 15 Tomás Cohen Arazi 2022-06-08 13:57:02 UTC
(In reply to Marcel de Rooy from comment #13)
> Yes that if is horrible for a purist.
> And note that matching .* is an insult already.

Language
Comment 16 Marcel de Rooy 2022-06-08 13:58:57 UTC
(In reply to Tomás Cohen Arazi from comment #15)
> (In reply to Marcel de Rooy from comment #13)
> > Yes that if is horrible for a purist.
> > And note that matching .* is an insult already.
> 
> Language

?
Comment 17 Simon Hohl 2022-06-23 11:27:13 UTC
This also affects table filtering in /cgi-bin/koha/acqui/parcel.pl?invoiceid=<number>.
Comment 18 Martin Renvoize 2022-06-28 11:31:55 UTC
Created attachment 136677 [details] [review]
Bug 30677: Cleanup

This patch cleans up the regular expression to remove the superflous
double check and use standard delimiters
Comment 19 Marcel de Rooy 2022-06-28 11:37:30 UTC
Will have a look right now ;)
Comment 20 Marcel de Rooy 2022-06-28 11:56:40 UTC
Created attachment 136678 [details] [review]
Bug 30677: Add tests

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 21 Marcel de Rooy 2022-06-28 11:56:44 UTC
Created attachment 136679 [details] [review]
Bug 30677: Use lookahead in regex for biblioitem replacement

This patch takes Andrew's suggested fix using a lookahead regex to
correct our biblio vs biblioitem table name replacements.

Please use the preceeding unit test patch proposed by Jonathan to test.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 22 Marcel de Rooy 2022-06-28 11:56:48 UTC
Created attachment 136680 [details] [review]
Bug 30677: Cleanup

This patch cleans up the regular expression to remove the superflous
double check and use standard delimiters

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 23 Tomás Cohen Arazi 2022-06-28 12:41:22 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!