Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Backslash symbol in suggestions results in broken JSON #842

Closed
veloman-yunkan opened this issue Nov 10, 2022 · 3 comments · Fixed by #843
Closed

Backslash symbol in suggestions results in broken JSON #842

veloman-yunkan opened this issue Nov 10, 2022 · 3 comments · Fixed by #843
Assignees
Milestone

Comments

@veloman-yunkan
Copy link
Collaborator

veloman-yunkan commented Nov 10, 2022

Originally posted by @kelson42 in #769 (comment)

I had a look to the problem with "pi" and the problem is related to wrong json escaping, see this:

$ curl -s "http://127.0.0.1:8080/suggest?content=wikipedia_en_all_nopic_2022-01&term=pi" | cat -n
     1	[
     2	  {
     3	    "value" : "PI",
     4	    "label" : "<b>PI</b>",
     5	    "kind" : "path"
     6	      , "path" : "A/PI"
     7	  },
     8	  {
     9	    "value" : "Pi",
    10	    "label" : "<b>Pi</b>",
    11	    "kind" : "path"
    12	      , "path" : "A/Pi"
    13	  },
    14	  {
    15	    "value" : "Pi.",
    16	    "label" : "<b>Pi</b>.",
    17	    "kind" : "path"
    18	      , "path" : "A/Pi."
    19	  },
    20	  {
    21	    "value" : "Pí",
    22	    "label" : "Pí",
    23	    "kind" : "path"
    24	      , "path" : "A/Pí"
    25	  },
    26	  {
    27	    "value" : "\pi",
    28	    "label" : "\<b>pi</b>",
    29	    "kind" : "path"
    30	      , "path" : "A/\pi"
    31	  },
    32	  {
    33	    "value" : "E^pi-pi",
    34	    "label" : "E^<b>pi</b>-<b>pi</b>",
    35	    "kind" : "path"
    36	      , "path" : "A/E^pi-pi"
    37	  },
    38	  {
    39	    "value" : "PI 88788",
    40	    "label" : "<b>PI</b> 88788",
    41	    "kind" : "path"
    42	      , "path" : "A/PI_88788"
    43	  },
    44	  {
    45	    "value" : "PI-21858",
    46	    "label" : "<b>PI</b>-21858",
    47	    "kind" : "path"
    48	      , "path" : "A/PI-21858"
    49	  },
    50	  {
    51	    "value" : "PI-3K",
    52	    "label" : "<b>PI</b>-3K",
    53	    "kind" : "path"
    54	      , "path" : "A/PI-3K"
    55	  },
    56	  {
    57	    "value" : "Pi (1998)",
    58	    "label" : "<b>Pi</b> (1998)",
    59	    "kind" : "path"
    60	      , "path" : "A/Pi_(1998)"
    61	  },
    62	  {
    63	    "value" : "pi ",
    64	    "label" : "containing 'pi'...",
    65	    "kind" : "pattern"
    66	    
    67	  }
    68	]

and here with json integrety check:

$ curl "http://127.0.0.1:8080/suggest?content=wikipedia_en_all_nopic_2022-01&term=pi" | jq
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  1317  100  1317    0     0  24687      0 --:--:-- --:--:-- --:--:-- 24849
parse error: Invalid escape at line 27, column 19

@veloman-yunkan Would you be able to quickly fit that (and adapt the test)? In general I wonder that we have this kind of bug, we don't use an external primitive to do the json escaping?!

@veloman-yunkan veloman-yunkan self-assigned this Nov 10, 2022
@kelson42
Copy link
Collaborator

@veloman-yunkan Discussing with @mgautierfr about the problem, it came back that a few months ago, there was the discussion about using a proper external json library. I wonder if this would be the right time to reassess this idea.

@veloman-yunkan
Copy link
Collaborator Author

@kelson42 The fix itself is simple enough to not justify utilizing a special library (as long as only libkiwix is concerned). The only other place in the Kiwix/OpenZIM ecosystem (that I am aware of) where we have to deal with JSON generation is in zimcheck. If we want to address that one too, then it makes a little more sense.

@kelson42
Copy link
Collaborator

@veloman-yunkan Yes, I'm supportive to address both. I'm just unsure about the timing. Probably we could go for your fix for now, but at next opportunity we should really put the topic on the topic on the table.

@kelson42 kelson42 added this to the 12.0.0 milestone Nov 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants