OpdsCatalog::getSearchUrl()

[veloman-yunkan]

Fixes #480.

Must be merged after #526.

This is a preliminary implementation of OPDS search url generation targeting the new API introduced by the yet unmerged #492.

Note that #488 has enabled us to search by all fields via the query parameter. For example a search by category can be done in two ways:

1. (old) search_endpoint?category=whatever
2. (new) search_endpoint?q=category:whatever

Internally, the old approach is converted to an equivalent of the new one with the slight difference of not assuming partial query (which is the default mode for the q parameter).

Some fields (creator, publisher) are not exposed for searching in the old scheme but are available via the new scheme.

Now the question is which one of the two schemes should be utilized for the search URL generation?

Another question is what should be done about the filtering criteria Filter::local(), Filter::remote(), Filter::remote()? They have never been exposed through the OPDS search API.

TODOs:

• Don't ignore the Filter::maxSize() & Filter::rejectTags() filtering criteria.

[kelson42]

@veloman-yunkan Thx, do you have a kiwix-desktop related PR, so I can just test if it works (no regression) from user perspective?

[veloman-yunkan]

@veloman-yunkan Thx, do you have a kiwix-desktop related PR, so I can just test if it works (no regression) from user perspective?

No. It won't work until the new OPDS catalog API is merged and goes live on library.kiwix.org (or you will have to test with a custom-built kiwix-serve incorporating #492).

[kelson42]

@veloman-yunkan Really hard to me then to verify it works fine from a user persective. Will let @mgautierfr do the review.

[mgautierfr]

Now the question is which one of the two schemes should be utilized for the search URL generation?

I prefer the old one. Mainly because it is easier to read.

[mgautierfr]

Couldn't we have buildSearchString adding the ? itself if needed ?
Then we would simply have to always add opdsSearchEndpoint and searchString.

[veloman-yunkan]

I tried it but didn't like the result. getSearchUrl() starts looking like a plagiarist that delegates all work to a ghost writer but receives all the credit. The current division of work between getSearchUrl() and buildSearchString() is more balanced.

[kelson42]

what is the status here?

[veloman-yunkan]

@kelson42 I will finalize this PR after #492 is merged (which introduces new catalog API that is assumed here).

[stale]

This pull request has been automatically marked as stale because it has not had recent activity. It will be now be reviewed manually. Thank you for your contributions.

[codecov]

Codecov Report

Merging #527 (c3595aa) into master (4124ad3) will increase coverage by 0.19%.
The diff coverage is 100.00%.

❗ Current head c3595aa differs from pull request most recent head b5c1b26. Consider uploading reports for the commit b5c1b26 to get more accurate results

@@            Coverage Diff             @@
##           master     #527      +/-   ##
==========================================
+ Coverage   65.05%   65.25%   +0.19%     
==========================================
  Files          51       52       +1     
  Lines        3626     3649      +23     
  Branches     1825     1841      +16     
==========================================
+ Hits         2359     2381      +22     
- Misses       1265     1266       +1     
  Partials        2        2              

Impacted Files	Coverage Δ
src/opds_catalog.cpp	100.00% <100.00%> (ø)
src/reader.cpp	44.11% <0.00%> (-0.17%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4124ad3...b5c1b26. Read the comment docs.

[kelson42]

@veloman-yunkan #492 has been merge, I guess nothing stops us anymore to finish this PR?

[veloman-yunkan]

@kelson42 This PR is finished and pending review by @mgautierfr

[mgautierfr]

Nothing to say on the code.

I'm a bit less sure about the generated endpoint.
As the catalog V2 is not totally finalized, we may change the endpoints and we agree to not use them until we finish it (at least until we are confident enough to tell we can start us it).
If getSearchUrl use the v2 and we do a release with getSearchUrl then user will start to use it and start to use the v2 catalog. We have to be careful with this.

I will not block this PR for this, but I may block the next release however 🙂

[stale]

This pull request has been automatically marked as stale because it has not had recent activity. It will be now be reviewed manually. Thank you for your contributions.

[mgautierfr]

rebase/fixup on master.

This PR is open for two long now. As we agree on the code we can merge it.
We will discuss again this v2 endpoint when we will do the release.