Translation of error pages #1032

veloman-yunkan · 2023-12-05T15:00:19Z

Will fix #1019

codecov · 2023-12-05T15:04:08Z

Codecov Report

Attention: Patch coverage is 49.34211% with 77 lines in your changes missing coverage. Please review.

Project coverage is 39.34%. Comparing base (9c5f5c7) to head (dc3960c).
Report is 165 commits behind head on main.

Files with missing lines	Patch %	Lines
src/server/response.cpp	50.44%	4 Missing and 52 partials ⚠️
src/tools/otherTools.cpp	15.38%	1 Missing and 10 partials ⚠️
src/server/internalServer.cpp	44.44%	0 Missing and 5 partials ⚠️
src/server/request_context.cpp	70.00%	0 Missing and 3 partials ⚠️
src/server/i18n.cpp	50.00%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1032      +/-   ##
==========================================
+ Coverage   38.95%   39.34%   +0.39%     
==========================================
  Files          58       58              
  Lines        3958     4061     +103     
  Branches     2181     2237      +56     
==========================================
+ Hits         1542     1598      +56     
- Misses       1086     1091       +5     
- Partials     1330     1372      +42

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

veloman-yunkan · 2023-12-06T14:34:03Z

@mgautierfr This PR is still work in progress, however I would like to learn your feedback regarding the small enhancement that I made in mustache.hpp in order to reduce the amount of extra coding that I would need to do without it.

Conceptually, the backend part is complete (I only have to mark those error pages that should be dynamically-translatable and have KIWIX_RESPONSE_DATA in them). The frontend will only have to take advantage of the response parameter data included in the error pages and update the error page text accordingly.

kelson42 · 2023-12-19T04:03:14Z

@mgautierfr We need a feedback here

mgautierfr · 2023-12-19T09:56:34Z

I'm a bit puzzled with the idea to add new specific api feature to mustache.
It would need that we ask packagers to use a patched mustache library and it would be probably (and legitimately) refused.
We should at least propose a patch upstream (and make it accepted) before we go with this.

Why not using lambda (#751 (comment)) ?
It would make us:

adapt a bit the templates (to add the "call" to the translation lambda)
Create the lambda code but it would be pretty close from what you have done in evaluate function (maybe even simply has you would not have to traverse data objects)

Is there any traps I haven't seen ?

But I really like the separation of the response from the connection. It is a step in the direction of #740 (even if this may not be the purpose)

kelson42 · 2023-12-29T13:12:23Z

We definitly should rely on unmodified versions of external dependencies... but indeed, we can do upstream PR.

veloman-yunkan · 2024-01-05T15:30:44Z

Why not using lambda (#751 (comment)) ?

@mgautierfr

Generation of the error page content is a two step process.

There is a shared template for error pages with parameters such as the page title & heading. This is the mandatory minimum needed to build an error page. However, optionally, a list of additional detail lines can be provided.

The error details are generated in C++ code by using parameterized message facilities. Currently (outside this PR) the translation of this additional info happens eagerly in the backend and the translated/instantiated string goes into step 2.
The HTML of the error page is produced by instantiating a mustache template with values for the page title, heading and details text.

Consider it on an example:

    return HTTP404Response(request)
           + noSuchBookErrorMsg(bookName);

First, noSuchBookErrorMsg(bookName) is executed and instantiates a i18n message template for the given book name. Then the resulting std::string (with any explicit information about the book name erased from it) participates in another level of mustache template instantiation. The idea of this PR was to preserve all the data used to generate the response HTML and embed that data in the response so that it can be rerendered in the front-end.

This two-step nature of error response generation was missing from the HTTPErrorResponse unit tests but now I added a commit illustrating it.

When I started writing this reply I didn't at all see how lambdas could be used to address that problem. Now I can sense some solution relying on lambdas, however it seems artificial and limited (it won't handle additional levels involved in making up the final text; suppose that the bookName parameter in the example above is not a terminal string, but some text obtained as a result of translating a parameterized message: bookName = getBookNameMsg(book, name) ).

I will think further, but will appreciate if you hint at the solution that you had in mind if it really is capable of addressing the described challenge.

veloman-yunkan · 2024-01-06T15:09:29Z

I also implemented the translation of the error page in the front-end. In the worst case (if the modification to mustache.hpp is not accepted) I can switch our ParameterizedMessage from kainjow::mustache::data to a relatively small class implemented in kiwix code.

veloman-yunkan · 2024-01-08T16:00:02Z

I completed the draft version of the solution that doesn't need mustache.hpp to be patched. I will clean up the PR tomorrow.

This should have been done in PR#997 in order to better guarantee a lasting solution to issue#995.

This will simplify testing of Response utilities.

... whereupon `ContentResponseBlueprint::generateResponseObject()` (and `ContentResponseBlueprint` as a whole) no longer needs to be polymorphic.

ContentResponseBlueprint::m_data is now an opaque data member implemented in the .cpp and ready to be switched from kainjow::mustache::data to a different implementation.

Now when parameterized messages are added to an error response, they are not immediately instantiated (translated). Instead the message id and the parameters of the message are recorded. The instantiation of the messages happens right before generating the final content of the response.

Note that it is declared in stringTools.h but its definition remains in otherTools.cpp (to minimize the diff).

- More familiar escape sequences for tab, newline and carriage return symbols. - Quote symbol is escaped by default too, however that behaviour can be disabled for uses in HTML-related contexts where quotes should then be replaced with the character entity "

Now the data used to generate an error response can be made to be embedded in the response as a JS object KIWIX_RESPONSE_DATA.

This is a shortcut change since it doesn't make sense to send the error page template with every error response (the viewer can fetch it from the server once but that's slightly more work).

Non-HTML-encoded HTML-template data causes problems in HTML even when it appears inside JS string (resulting in the <script> tag being closed by a </script> appearing inside a JS string). Besides, the KIWIX_RESPONSE_DATA and KIWIX_RESPONSE_TEMPLATE variables are set on the window object so that they can be accessed from the top context. This commit eliminates the need for the `escapeQuote` parameter in `escapeForJSON()` (that was introduced earlier in this PR) since now it is set to false in all call contexts. However from the consistency point of view, the default and intuitive behaviour of `escapeForJSON()` should be to escape the quote symbols, which justifies the existence of that parameter.

veloman-yunkan · 2024-01-09T20:38:50Z

Clean-up is complete, yet there is still some work to be done on this PR. I will move forward once @mgautierfr approves the current solution. Note that to test it, you must use the last-but-one commit ("Demo of error page translation") - the extra commit was added to prove that this PR doesn't utterly break the CI.

mgautierfr

Seems good to me, but I'm not sure that Variant is fully supported on our compilers.

We will check that and if we are good... we are good.
Else we will have to find another solution (or discuss about our compilers toolchain)

mgautierfr · 2024-01-11T17:12:22Z

src/server/response.cpp

+  typedef std::map<std::string, Data> Object;
+
+private:
+  std::variant<std::string, bool, List, Object> data;


Are we sure that variant is correctly supported ?

From https://gcc.gnu.org/onlinedocs/libstdc++/manual/status.html#status.iso.2017 it is in gcc 7.1 and we are using gcc 6.3 for aarch64 (because of kiwix/kiwix-build#634)

I will setup a build with this branch on kiwix-build to check that.

And, sadly, this doesn't compile on aarch64 focal (https://github.com/kiwix/kiwix-build/actions/runs/7492497587/job/20396122209)

I provided a work-around.

aarch64 focal build passed https://github.com/kiwix/kiwix-build/actions/runs/7492497587/job/20454495746

Good. So now I'm happy with your proposal. Have you finish it or you still have things to do ? (In #1032 (comment) you said that it is no)

I still have to enable front-end side translation of error pages and fix the unit tests that will become broken by that change.

veloman-yunkan · 2024-01-17T15:05:57Z

The PR is now functionally ready, however some changes may need to be made depending on the answers to the following questions:

Is sending the template data with every response that may need to be translated in the frontend acceptable? That was a shortcut intended to achieve the desired functionality with the least effort and will also simplify fixing Search results page is not translated #1028.
Is it OK that translation data is also embedded into error responses that are not currently expected to be displayed by the viewer (e.g. responses to invalid requests to /catalog, /raw, /suggest, etc, endpoints)?

kelson42 · 2024-01-20T08:01:38Z

@mgautierfr Review please?

mgautierfr · 2024-01-22T13:27:20Z

Is sending the template data with every response that may need to be translated in the frontend acceptable? That was a shortcut intended to achieve the desired functionality with the least effort and will also simplify fixing
#1028.

I think we are good on this. We may in the future introduce a endpoint error_template to get the template from an error code (and giving this error code in the response) but for now, we can go without it. Template is a not of data, and it send only for error.

Is it OK that translation data is also embedded into error responses that are not currently expected to be displayed by the viewer (e.g. responses to invalid requests to /catalog, /raw, /suggest, etc, endpoints)?

Yes also here. This double the error response size, but error response are pretty small and it is only for error, so pretty rare.

Also, the test cases against vulnerabilities in kiwix-serve seem to suggest
that KIWIX_RESPONSE_DATA should be HTML-encoded too.

I totally agree.

test/server.cpp

veloman-yunkan · 2024-01-25T11:53:42Z

Also, the test cases against vulnerabilities in kiwix-serve seem to suggest
that KIWIX_RESPONSE_DATA should be HTML-encoded too.

I totally agree.

@mgautierfr You better had objected in this case 😃 . I figured out that the issue is rather an edge case and provided a simpler fix.

This commit demonstrates front-end-side translation of an error page for a URL like /viewer#INVALIDBOOK/whatever (where INVALIDBOOK should be a book name NOT present in the library). Known issues: - This change breaks a couple of subtests in the ServerTest.Http404HtmlError unit test. - Changing the UI language while an error page is displayed in the viewer doesn't retranslate it.

This undoes frontend-side translation of the demo case with the purpose of having "clean" unit tests to support further work on this PR.

std::variant is not supported by the old version of gcc used under aarch64.

But the question is do we need all of them to be translatable in the frontend? Maybe only responses to /random, /content and /search endpoints (that are displayed in the viewer) should be translatable? Also, the test cases against vulnerabilities in kiwix-serve seem to suggest that KIWIX_RESPONSE_DATA should be HTML-encoded too.

Added a test case demonstrating how a bad error response could be generated if </script> appears inside KIWIX_RESPONSE_DATA. That seems to be the only problematic interaction between HTML-like syntax inside javascript code (hence the deleted XXX comments on the other two test cases).

mgautierfr · 2024-01-29T09:54:20Z

@mgautierfr You better had objected in this case 😃 . I figured out that the issue is rather an edge case and provided a simpler fix.

I had missed that, your comment opens my eyes.

We are good
(I've just rebase-fixup)

veloman-yunkan · 2024-01-29T11:53:14Z

(I've just rebase-fixup)

Thank you. Note, however, that one of the fixup commits implied (and that was stated in its commit message) that the description of the commit-to-be-amended had to be corrected too. I have now added that information as a commit comment.

veloman-yunkan requested a review from mgautierfr December 6, 2023 14:34

veloman-yunkan force-pushed the error_response_i18n branch from 9b607fd to fd35881 Compare January 7, 2024 17:33

veloman-yunkan added 20 commits January 9, 2024 20:20

Moved microhttpd_wrapper.h under server/

3f0ea08

Added i18n unit test

96b6f41

ParameterizedMessage is actually a class

8993f99

Introduced testing of HTTP response utils

b9323f1

Dropped cookies from RequestContext

af228bf

This should have been done in PR#997 in order to better guarantee a lasting solution to issue#995.

Decoupled RequestContext from MHD_Connection

aee6c23

This will simplify testing of Response utilities.

Testing of HTTPErrorResponse translation

c57b8a0

Testing of MIME-type of HTTP 500 response

797f4c4

Retired HTTP500Response::generateResponseObject()

54191bc

... whereupon `ContentResponseBlueprint::generateResponseObject()` (and `ContentResponseBlueprint` as a whole) no longer needs to be polymorphic.

Fixed an encapsulation breach

0b7cd61

Moved constructor into .cpp

d39e91f

Enter ContentResponseBlueprint::Data

e72fc23

ContentResponseBlueprint::m_data is now an opaque data member implemented in the .cpp and ready to be switched from kainjow::mustache::data to a different implementation.

New implementation of ContentResponseBlueprint::Data

0b542fe

Unmustached i18n::Parameters

f298acd

Exposed escapeForJSON() in kiwix namespace

f3d3ab1

Note that it is declared in stringTools.h but its definition remains in otherTools.cpp (to minimize the diff).

Added KIWIX_RESPONSE_DATA to error response

b151a2a

Now the data used to generate an error response can be made to be embedded in the response as a JS object KIWIX_RESPONSE_DATA.

Added error details in testing of error responses

d2fedf9

The page template is embedded in the error response

e14de69

This is a shortcut change since it doesn't make sense to send the error page template with every error response (the viewer can fetch it from the server once but that's slightly more work).

veloman-yunkan force-pushed the error_response_i18n branch from 43d90a7 to c349d73 Compare January 9, 2024 20:29

mgautierfr reviewed Jan 11, 2024

View reviewed changes

mgautierfr mentioned this pull request Jan 11, 2024

[WIP] Build libkiwix using error_response_i18n kiwix/kiwix-build#676

Closed

veloman-yunkan changed the title ~~[WIP] Translation of error pages~~ Translation of error pages Jan 17, 2024

veloman-yunkan marked this pull request as ready for review January 17, 2024 14:49

veloman-yunkan requested a review from mgautierfr January 17, 2024 15:06

mgautierfr reviewed Jan 22, 2024

View reviewed changes

test/server.cpp Outdated Show resolved Hide resolved

veloman-yunkan requested a review from mgautierfr January 27, 2024 11:44

veloman-yunkan added 7 commits January 29, 2024 10:53

Undid the demo of frontend-side error page translation

e1f067c

This undoes frontend-side translation of the demo case with the purpose of having "clean" unit tests to support further work on this PR.

Workaround for missing support for of std::variant

bb1a730

std::variant is not supported by the old version of gcc used under aarch64.

Enabled frontend-side translation of 500 error page

13a6863

Fix against a malicious "</script>" in KIWIX_RESPONSE_DATA

dc3960c

mgautierfr force-pushed the error_response_i18n branch from 5eafe5a to dc3960c Compare January 29, 2024 09:53

mgautierfr approved these changes Jan 29, 2024

View reviewed changes

mgautierfr merged commit d2f20db into main Jan 29, 2024
16 checks passed

mgautierfr deleted the error_response_i18n branch January 29, 2024 09:58

veloman-yunkan mentioned this pull request Jan 30, 2024

Translation of search results page #1046

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Translation of error pages #1032

Translation of error pages #1032

veloman-yunkan commented Dec 5, 2023

codecov bot commented Dec 5, 2023 •

edited

Loading

veloman-yunkan commented Dec 6, 2023 •

edited

Loading

kelson42 commented Dec 19, 2023

mgautierfr commented Dec 19, 2023 •

edited

Loading

kelson42 commented Dec 29, 2023

veloman-yunkan commented Jan 5, 2024

veloman-yunkan commented Jan 6, 2024

veloman-yunkan commented Jan 8, 2024

veloman-yunkan commented Jan 9, 2024

mgautierfr left a comment

mgautierfr Jan 11, 2024

mgautierfr Jan 12, 2024

veloman-yunkan Jan 13, 2024

veloman-yunkan Jan 13, 2024

mgautierfr Jan 15, 2024

veloman-yunkan Jan 17, 2024

veloman-yunkan commented Jan 17, 2024

kelson42 commented Jan 20, 2024

mgautierfr commented Jan 22, 2024

veloman-yunkan commented Jan 25, 2024

mgautierfr commented Jan 29, 2024

veloman-yunkan commented Jan 29, 2024

Translation of error pages #1032

Translation of error pages #1032

Conversation

veloman-yunkan commented Dec 5, 2023

codecov bot commented Dec 5, 2023 • edited Loading

Codecov Report

veloman-yunkan commented Dec 6, 2023 • edited Loading

kelson42 commented Dec 19, 2023

mgautierfr commented Dec 19, 2023 • edited Loading

kelson42 commented Dec 29, 2023

veloman-yunkan commented Jan 5, 2024

veloman-yunkan commented Jan 6, 2024

veloman-yunkan commented Jan 8, 2024

veloman-yunkan commented Jan 9, 2024

mgautierfr left a comment

Choose a reason for hiding this comment

mgautierfr Jan 11, 2024

Choose a reason for hiding this comment

mgautierfr Jan 12, 2024

Choose a reason for hiding this comment

veloman-yunkan Jan 13, 2024

Choose a reason for hiding this comment

veloman-yunkan Jan 13, 2024

Choose a reason for hiding this comment

mgautierfr Jan 15, 2024

Choose a reason for hiding this comment

veloman-yunkan Jan 17, 2024

Choose a reason for hiding this comment

veloman-yunkan commented Jan 17, 2024

kelson42 commented Jan 20, 2024

mgautierfr commented Jan 22, 2024

veloman-yunkan commented Jan 25, 2024

mgautierfr commented Jan 29, 2024

veloman-yunkan commented Jan 29, 2024

codecov bot commented Dec 5, 2023 •

edited

Loading

veloman-yunkan commented Dec 6, 2023 •

edited

Loading

mgautierfr commented Dec 19, 2023 •

edited

Loading