Bug 30818 - REST API: Why does a wrong column name still cause a general 500 while validating exhaustively?
Summary: REST API: Why does a wrong column name still cause a general 500 while valida...
Status: NEW
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Bugs List
QA Contact:
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-05-20 09:20 UTC by Marcel de Rooy
Modified: 2023-06-18 12:49 UTC (History)
4 users (show)

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


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Marcel de Rooy 2022-05-20 09:20:30 UTC
This might just be a dumb question:

While testing 30790 and tweaking the test a bit, I found this:

    my $api_filter = encode_json({'isbn' => '123', 'xx1' => 'a2' });
    $t->get_ok("//$userid:$password@/api/v1/biblios?q=$api_filter");

I am sending xx1 to Koha/REST/V1/Biblios, sub list.

To my surprise, this doesnt trigger an error that says xx1 is not a valid biblio column. It appears that this query is just sent to DBIx. It crashes and the Exceptions module returns 500 Something went wrong.

How is that possible? I stumble over all kinds of validation, extracting and merging parameters etc. etc.
Comment 1 Marcel de Rooy 2022-05-20 09:29:29 UTC
[2022/05/20 09:10:31] [ERROR] GET /api/v1/biblios: unhandled exception (DBIx::Class::Exception)<<DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st execute failed: Unknown column 'xx1' in 'where clause' at /usr/share/perl5/Data/Page.pm line 50>> Koha::REST::Plugin::Exceptions::__ANON__ /usr/share/koha/Koha/REST/Plugin/Exceptions.pm (73)
Comment 2 Marcel de Rooy 2022-05-20 10:06:38 UTC
Just for my reference. Going thru code

$filtered_params = $result_set->attributes_from_api($filtered_params);
NB my $columns_info = $self->_result->result_source->columns_info;

$filtered_params = $c->build_query_params( $filtered_params, $reserved_params );

merge_q_params

_parse_dbic_query
Hmm Recursive

_from_api_param

The extracted_reserved_params seems a bit suboptimal.
The attributes_from_api should be able to remove bad columns?

The REST API is a great thing. But I must admit that going thru all these modules, plugins, etc. the exact meaning of a lot of subroutines is unclear and the comments are sparse. Hoping that not only Tomas has a global detailed view of what is going on here.. ;)
Comment 3 Tomás Cohen Arazi 2022-05-20 10:52:46 UTC
(In reply to Marcel de Rooy from comment #2)
> The REST API is a great thing. But I must admit that going thru all these
> modules, plugins, etc. the exact meaning of a lot of subroutines is unclear
> and the comments are sparse. Hoping that not only Tomas has a global
> detailed view of what is going on here.. ;)

There are two main things that need to be considered when thinking about the API tooling:

1. We need to map attributes from the API to the actual DB names. This happens when creating/updating resources, as well as querying. The latter is the most complex in the code (i.e. the recursive _parse_dbic_query)

2. When we are using q= in conjunction with x-koha-embed, we need to prefetch/join the right tables. This is mainly handled by _parse_prefetch.

About (1). In the original implementations, the controllers had two methods: to_model and to_api. So the mappings were entirely handled there. The problem with that is the fact we wanted to do more complex things like embedding related objects, so the API representation of an object needed to be handled on the object itself. There's some redundancy, most probably in the attributes_from_api and friends stuff on the Koha::Object(s) method. That could be polished. But the main thing is it needs to be handled at that level and reused as much as possible. There are not many more things to optimize there IMHO.

About (2). This is the difficult part. Embeds can happen at an arbitrary depth. Like in 'x-koha-embed: biblio.items.checkouts.patron'. As the 'how I should be rendered on the API' question is answered at the Koha::Object level, we build a data structure ('embed' param for to_api) that contains a (probably enhanceable) data structure representing the 'rest of the tree' that each object needs to embed. Along with this data structure, there's the need to build the prefetches/joins so we optimize things. Embedding can happen on real 'relations' (thus actual prefetches take place) or just on 'methods output' in which case no prefetch takes place. This is handled by _parse_prefetch, which is also recursive.

Regarding to_api(), we are adding new parameters that are passed through to related objects like 'public'. So each object, needs to know how it should render on an unprivileged context.

So, TL;DR

Our helper methods take care of handling what the OpenAPI plugin generates out of the requests, and convert it into:
- Queries, objects, etc with mapped attribute names
- Prefetches are appended to resultsets with embeds
- Generate data structure to be passed to to_api()

And most of the magic takes place in Koha::Object->to_api(). The most complex bit is querying.

Regarding this bug, I believe the solution is to catch the DBIx::Class::Exception in the objects.search helper, and return a 400 with a meaningful message.
Comment 4 Tomás Cohen Arazi 2022-05-20 10:55:24 UTC
(In reply to Marcel de Rooy from comment #0)
> How is that possible? I stumble over all kinds of validation, extracting and
> merging parameters etc. etc.

As I mentioned on my earlier answer, we shouldn't validate any arbitrary query contains valid attributes. It is more sustainable to just catch the DBIx::Class exception and act better.
Comment 5 Katrin Fischer 2023-06-18 12:49:14 UTC
(In reply to Tomás Cohen Arazi from comment #4)
> (In reply to Marcel de Rooy from comment #0)
> > How is that possible? I stumble over all kinds of validation, extracting and
> > merging parameters etc. etc.
> 
> As I mentioned on my earlier answer, we shouldn't validate any arbitrary
> query contains valid attributes. It is more sustainable to just catch the
> DBIx::Class exception and act better.

What does it mean for this bug? Should it be rephrased/closed?