Small search improvement. #724

mgautierfr · 2022-03-09T14:46:56Z

This PR fixes small issues found on search handling on the server.
It is somehow a preparatory work for openSearch feature coming soon.

Fixes #723

veloman-yunkan

The second commit (the one titled "Introduce SearchInfo.") is a little two big and a little too different to validate. It will benefit from being split into smaller changes.

src/server/internalServer.cpp

veloman-yunkan · 2022-03-09T15:14:37Z

static/templates/400.html

+    <h1>Invalid request</h1>
+    {{#url}}
+    <p>
+      The requested URL "{{.}}" is not a valid request.


The {.} mustache syntax is a revelation for me. Is it documented?

I cannot find it in the mustache manual (https://mustache.github.io/mustache.5.html)
But it is in kainjow mustache library : https://github.com/kainjow/Mustache/blob/master/tests/tests.cpp#L273-L281

test/server.cpp

src/server/internalServer.cpp

codecov · 2022-03-09T15:50:33Z

Codecov Report

Merging #724 (701a416) into master (574c1ad) will increase coverage by 0.66%.
The diff coverage is 70.87%.

❗ Current head 701a416 differs from pull request most recent head 311f783. Consider uploading reports for the commit 311f783 to get more accurate results

@@            Coverage Diff             @@
##           master     #724      +/-   ##
==========================================
+ Coverage   58.31%   58.98%   +0.66%     
==========================================
  Files          56       56              
  Lines        3697     3735      +38     
  Branches     2069     2083      +14     
==========================================
+ Hits         2156     2203      +47     
+ Misses       1540     1531       -9     
  Partials        1        1

Impacted Files	Coverage Δ
src/server/internalServer.cpp	`79.13% <59.72%> (-2.07%)`	⬇️
src/server/internalServer.h	`88.88% <93.75%> (+55.55%)`	⬆️
src/server/response.cpp	`88.65% <100.00%> (+0.70%)`	⬆️
src/server/response.h	`100.00% <100.00%> (ø)`
src/tools/stringTools.cpp	`50.00% <0.00%> (+1.00%)`	⬆️
src/search_renderer.cpp	`89.33% <0.00%> (+24.00%)`	⬆️

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 574c1ad...311f783. Read the comment docs.

veloman-yunkan · 2022-03-17T17:56:11Z

@mgautierfr I reviewed the PR again before realizing that you forgot to push your changes.

kelson42 · 2022-03-23T08:28:14Z

@mgautierfr Can we move one please with this PR?

veloman-yunkan · 2022-03-23T10:46:08Z

@mgautierfr Can we move one please with this PR?

@kelson42 I would like to bring to your attention the situation with this PR described in #727 (comment). If we postpone this PR until the preparatory work for internationalizing kiwix-serve is completed, then it can be rewritten in a slightly cleaner way (while at the same time minimizing the need for addressing conflicts in a larger WIP PR #679).

kelson42 · 2022-03-23T11:21:49Z

@veloman-yunkan Not against the principle, but concerned about the overall timelime and interdependences. Considering that we have an XSS issue in the field with kiwix-serve... I would like that we consider a minor release within the next 2-3 weeks.

mgautierfr · 2022-03-23T13:47:25Z

The "problem" is that this PR is somehow a preparatory work for other PRs introducing opensearch feature.
And I'm affraid #679 will not be merged soon. But we will have merge conflicts and we must minimize them. I can rebase this PR on #727 easyily.
@veloman-yunkan will it be enough ? If not, can you extract another preparatory PR from #679 (And rebase this PR on your newly created PR) ?

veloman-yunkan · 2022-03-23T14:41:42Z

I can rebase this PR on #727 easyily. @veloman-yunkan will it be enough ?

No

If not, can you extract another preparatory PR from #679 (And rebase this PR on your newly created PR) ?

I will do it tomorrow.

mgautierfr · 2022-03-23T15:43:56Z

Rebased on last master (with #727 merged)
(The interesting commit the review are the two "fixup!" commit and the last one)

veloman-yunkan · 2022-03-24T17:42:37Z

If not, can you extract another preparatory PR from #679 (And rebase this PR on your newly created PR) ?

I will do it tomorrow.

The new PR extracted from #679 is #732. Unfortunately, a few changes that I intended to include in #732 were too tightly coupled with internationalization of kiwix-serve so I left them for another smaller follow-up PR.

kelson42 · 2022-03-28T13:53:49Z

@mgautierfr I guess this is now ready to fix/merge.

HTTP400HtmlResponse is build on the same design than HTTP404HtmlResponse.

mgautierfr · 2022-03-28T15:35:46Z

Rebased on master (with #732 merged)

veloman-yunkan

LGTM. But I am also including couple of optional suggestions for minor improvements that could be made while moving the code around.

src/server/internalServer.cpp

src/server/internalServer.h

mgautierfr · 2022-03-29T09:54:30Z

I've fixed your comment in two fixup commits

veloman-yunkan · 2022-03-29T10:51:46Z

src/server/internalServer.h

    }

  public: //data
    std::string pattern;
-    bool has_geo_query;
-    GeoQuery geo_query;
+    std::pair<bool, GeoQuery> geoQuery;


I don't like this. A better solution is to take advantage of the fact that GeoQuery can have an invalid state (for example when distance < 0) without adding a new data field to it. Add a proper default constructor to GeoQuery and a bool isValid() const member function.

Done with a operator bool operator

src/server/internalServer.h

SearchInfo is a small helper structure to store information about the queried search. It regroup already existing information (`patternString`, geo query, ...) in one structure. It is also used as key in the cache instead of using a generated string.

It is better to directly try to get the `Search` from the cache instead of getting the `Searcher` first which could be useless in Search already exist.

If user request for a non existent book, we must return a 400 page. (This is done by throwing a `std::invalid_argument` and let the catch handle it)

The `searchPattern` is already "diples encoded". So we can simply using it without protecting us from script injection. Fix #723

There is no reason to not use the pattern if there is a geo_query. If both the pattern and the qeo_query are provided, we must use both.

mgautierfr requested review from veloman-yunkan and maneeshpm March 9, 2022 14:46

mgautierfr force-pushed the search_improvement branch from 8b50187 to a83e99d Compare March 9, 2022 15:04

veloman-yunkan requested changes Mar 9, 2022

View reviewed changes

juuz0 mentioned this pull request Mar 11, 2022

Wrong html entities in kiwix-serve ft result page #723

Closed

mgautierfr requested a review from veloman-yunkan March 17, 2022 15:31

veloman-yunkan mentioned this pull request Mar 18, 2022

Testing of "Fulltext search unavailable" page #727

Merged

mgautierfr force-pushed the search_improvement branch from 7a3e6fb to f2e4f25 Compare March 23, 2022 13:55

mgautierfr force-pushed the search_improvement branch from f2e4f25 to 34cb506 Compare March 23, 2022 15:41

mgautierfr mentioned this pull request Mar 23, 2022

Better handling of multi search #729

Merged

mgautierfr force-pushed the search_improvement branch from 34cb506 to 544bcb1 Compare March 28, 2022 15:34

Introduce HTTP400HtmlResponse.

b1643e4

HTTP400HtmlResponse is build on the same design than HTTP404HtmlResponse.

mgautierfr force-pushed the search_improvement branch from 544bcb1 to 3aca36f Compare March 28, 2022 15:35

Return http 400 error response when needed.

e729334

mgautierfr force-pushed the search_improvement branch from 3aca36f to ea4e9cb Compare March 28, 2022 15:37

veloman-yunkan approved these changes Mar 28, 2022

View reviewed changes

src/server/internalServer.cpp Outdated Show resolved Hide resolved

src/server/internalServer.cpp Outdated Show resolved Hide resolved

src/server/internalServer.h Outdated Show resolved Hide resolved

mgautierfr requested a review from veloman-yunkan March 29, 2022 09:54

veloman-yunkan requested changes Mar 29, 2022

View reviewed changes

mgautierfr requested a review from veloman-yunkan March 29, 2022 11:05

veloman-yunkan requested changes Mar 29, 2022

View reviewed changes

src/server/internalServer.h Outdated Show resolved Hide resolved

mgautierfr requested a review from veloman-yunkan March 29, 2022 11:23

veloman-yunkan approved these changes Mar 29, 2022

View reviewed changes

mgautierfr added 7 commits March 29, 2022 14:05

Introduce SearchInfo.

d740ffe

SearchInfo is a small helper structure to store information about the queried search. It regroup already existing information (`patternString`, geo query, ...) in one structure. It is also used as key in the cache instead of using a generated string.

Better cache usage.

7407f30

It is better to directly try to get the `Search` from the cache instead of getting the `Searcher` first which could be useless in Search already exist.

Correctly handle invalid book.

1962262

If user request for a non existent book, we must return a 400 page. (This is done by throwing a `std::invalid_argument` and let the catch handle it)

Handle book without xapian index.

3641dbf

Correctly display searchpattern in search result page.

2cc4bef

The `searchPattern` is already "diples encoded". So we can simply using it without protecting us from script injection. Fix #723

Add braces around for loop's body.

f2a1c0f

Always use the search pattern when searching in the server.

311f783

There is no reason to not use the pattern if there is a geo_query. If both the pattern and the qeo_query are provided, we must use both.

mgautierfr force-pushed the search_improvement branch from 701a416 to 311f783 Compare March 29, 2022 12:07

mgautierfr merged commit 95d4dd6 into master Mar 29, 2022

mgautierfr deleted the search_improvement branch March 29, 2022 12:42

mgautierfr mentioned this pull request Apr 4, 2022

[REGRESSION] Fulltext search partly broken (no result) with master branch #722

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Small search improvement. #724

Small search improvement. #724

mgautierfr commented Mar 9, 2022 •

edited by kelson42

Loading

veloman-yunkan left a comment

veloman-yunkan Mar 9, 2022

mgautierfr Mar 9, 2022

codecov bot commented Mar 9, 2022 •

edited

Loading

veloman-yunkan commented Mar 17, 2022

kelson42 commented Mar 23, 2022

veloman-yunkan commented Mar 23, 2022

kelson42 commented Mar 23, 2022 •

edited

Loading

mgautierfr commented Mar 23, 2022

veloman-yunkan commented Mar 23, 2022

mgautierfr commented Mar 23, 2022 •

edited

Loading

veloman-yunkan commented Mar 24, 2022

kelson42 commented Mar 28, 2022

mgautierfr commented Mar 28, 2022

veloman-yunkan left a comment

mgautierfr commented Mar 29, 2022

veloman-yunkan Mar 29, 2022

mgautierfr Mar 29, 2022

Small search improvement. #724

Small search improvement. #724

Conversation

mgautierfr commented Mar 9, 2022 • edited by kelson42 Loading

veloman-yunkan left a comment

Choose a reason for hiding this comment

veloman-yunkan Mar 9, 2022

Choose a reason for hiding this comment

mgautierfr Mar 9, 2022

Choose a reason for hiding this comment

codecov bot commented Mar 9, 2022 • edited Loading

Codecov Report

veloman-yunkan commented Mar 17, 2022

kelson42 commented Mar 23, 2022

veloman-yunkan commented Mar 23, 2022

kelson42 commented Mar 23, 2022 • edited Loading

mgautierfr commented Mar 23, 2022

veloman-yunkan commented Mar 23, 2022

mgautierfr commented Mar 23, 2022 • edited Loading

veloman-yunkan commented Mar 24, 2022

kelson42 commented Mar 28, 2022

mgautierfr commented Mar 28, 2022

veloman-yunkan left a comment

Choose a reason for hiding this comment

mgautierfr commented Mar 29, 2022

veloman-yunkan Mar 29, 2022

Choose a reason for hiding this comment

mgautierfr Mar 29, 2022

Choose a reason for hiding this comment

mgautierfr commented Mar 9, 2022 •

edited by kelson42

Loading

codecov bot commented Mar 9, 2022 •

edited

Loading

kelson42 commented Mar 23, 2022 •

edited

Loading

mgautierfr commented Mar 23, 2022 •

edited

Loading