Bug 13265 - Still too many search cursor cookies
Summary: Still too many search cursor cookies
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
: 12510 (view as bug list)
Depends on: 12481
Blocks:
  Show dependency treegraph
 
Reported: 2014-11-17 14:46 UTC by Kyle M Hall
Modified: 2016-12-05 21:22 UTC (History)
12 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 13265 - Still too many search cursor cookies (12.05 KB, patch)
2014-11-17 15:55 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13265 - Still too many search cursor cookies (11.72 KB, patch)
2015-04-16 16:01 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13265 - Still too many search cursor cookies (12.07 KB, patch)
2015-05-18 13:14 UTC, Marcel de Rooy
Details | Diff | Splinter Review
[COUNTER-PATCH] Bug 13265: Use sessionStorage to store searches instead of cookies (8.44 KB, patch)
2015-05-22 12:00 UTC, Jonathan Druart
Details | Diff | Splinter Review
[Signed-off] Bug 13265: Use sessionStorage to store searches instead of cookies (8.69 KB, patch)
2015-05-23 09:57 UTC, Marc Véron
Details | Diff | Splinter Review
[PASSED QA] [PASSED QA] Bug 13265: Use sessionStorage to store searches instead of cookies (8.81 KB, patch)
2015-06-06 17:54 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2014-11-17 14:46:23 UTC
We have multiple sites that are still exhibiting bug 11369. For whatever reason, the cookies on these sites are larger and thus the maximum of 10 is still too large. This setting should be controlled by a system preference to the number can be tailored on a per-site basis.
Comment 1 Kyle M Hall 2014-11-17 15:55:36 UTC Comment hidden (obsolete)
Comment 2 David Cook 2014-12-01 05:31:44 UTC
I have an idea to fix this that would use the sessions table (like the OPAC browser but without its current limitations).

The fix I have in mind will allow the result browser to work in multiple tabs without bloating the request header which leads to the fatal error.

It's more work, but I think it'll be a more robust fix. 

That said, I wouldn't be opposed to this patch getting pushed in the mean-time...
Comment 3 Abby 2015-01-14 21:21:03 UTC
The search cookie is no longer used in master, search history is stored in the database.

Have we missed anything?
Comment 4 David Cook 2015-01-14 23:54:20 UTC
(In reply to Abby from comment #3)
> The search cookie is no longer used in master, search history is stored in
> the database.
> 
> Have we missed anything?

Interesting! I haven't looked at master recently, so I'm not sure. I wonder which commit changed the use of the search cookie...
Comment 5 Fridolin SOMERS 2015-01-29 16:35:57 UTC
(In reply to David Cook from comment #4)
> (In reply to Abby from comment #3)
> > The search cookie is no longer used in master, search history is stored in
> > the database.
> > 
> > Have we missed anything?
> 
> Interesting! I haven't looked at master recently, so I'm not sure. I wonder
> which commit changed the use of the search cookie...

I think its not the same thing, user search history is the history of wich search query he has used.
Bug 11369 concerns the browser used after a search to go to previous or next result.
Comment 6 Kyle M Hall 2015-04-07 11:31:01 UTC
(In reply to Fridolin SOMERS from comment #5)
> (In reply to David Cook from comment #4)
> > (In reply to Abby from comment #3)
> > > The search cookie is no longer used in master, search history is stored in
> > > the database.
> > > 
> > > Have we missed anything?
> > 
> > Interesting! I haven't looked at master recently, so I'm not sure. I wonder
> > which commit changed the use of the search cookie...
> 
> I think its not the same thing, user search history is the history of wich
> search query he has used.
> Bug 11369 concerns the browser used after a search to go to previous or next
> result.

If that's true, shouldn't my test plan fail?
Comment 7 Marc Véron 2015-04-11 07:57:02 UTC
Version is 'unspecified', is this for master?

Wanted to test with patch, does not apply:

Applying: Bug 13265 - Still too many search cursor cookies
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/addbiblio.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/results.tt
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/results.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/detail.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/cataloguing.pref
Auto-merging installer/data/mysql/updatedatabase.pl
CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl
Auto-merging installer/data/mysql/sysprefs.sql
Failed to merge in the changes.
Patch failed at 0001 Bug 13265 - Still too many search cursor cookies
Comment 8 Magnus Enger 2015-04-13 08:53:54 UTC
$ git bz apply 13265
Bug 13265 - Still too many search cursor cookies

33616 - Bug 13265 - Still too many search cursor cookies

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 13265 - Still too many search cursor cookies
Using index info to reconstruct a base tree...
M	installer/data/mysql/sysprefs.sql
M	installer/data/mysql/updatedatabase.pl
M	koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/cataloguing.pref
M	koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/detail.tt
M	koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/results.tt
M	koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/addbiblio.tt
Falling back to patching base and 3-way merge...
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/addbiblio.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/results.tt
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/results.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/detail.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/cataloguing.pref
Auto-merging installer/data/mysql/updatedatabase.pl
CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl
Auto-merging installer/data/mysql/sysprefs.sql
Failed to merge in the changes.
Patch failed at 0001 Bug 13265 - Still too many search cursor cookies
The copy of the patch that failed is found in:
   /home/magnus/scripts/kohaclone/.git/rebase-apply/patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-13265---Still-too-many-search-cursor-cookies-bMkL_V.patch
Comment 9 Kyle M Hall 2015-04-16 16:01:06 UTC Comment hidden (obsolete)
Comment 10 Jonathan Druart 2015-04-29 10:48:03 UTC
Kyle,
Don't you think it would be better to correctly fix the issue and store the searches using the sessionStorage?
It will avoid YASP.
Comment 11 Kyle M Hall 2015-05-01 13:46:14 UTC
(In reply to Jonathan Druart from comment #10)
> Kyle,
> Don't you think it would be better to correctly fix the issue and store the
> searches using the sessionStorage?
> It will avoid YASP.

Yes, but as David said, this will work in the mean-time, unless someone is willing to write and submit a counter patch!
Comment 12 Marcel de Rooy 2015-05-18 13:14:29 UTC Comment hidden (obsolete)
Comment 13 Jonathan Druart 2015-05-22 12:00:28 UTC Comment hidden (obsolete)
Comment 14 Jonathan Druart 2015-05-22 12:00:47 UTC
(In reply to Kyle M Hall from comment #11)
> (In reply to Jonathan Druart from comment #10)
> > Kyle,
> > Don't you think it would be better to correctly fix the issue and store the
> > searches using the sessionStorage?
> > It will avoid YASP.
> 
> Yes, but as David said, this will work in the mean-time, unless someone is
> willing to write and submit a counter patch!

Here it is!
Comment 15 Marc Véron 2015-05-23 09:57:44 UTC Comment hidden (obsolete)
Comment 16 Marc Véron 2015-05-23 09:59:22 UTC
Signed off counter patch ( Bug 13265: Use sessionStorage to store searches instead of cookies )
Comment 17 Jonathan Druart 2015-05-24 11:33:47 UTC
Kyle, could you have a look at my counter patch please?
Comment 18 Katrin Fischer 2015-06-06 17:54:00 UTC
Created attachment 39948 [details] [review]
[PASSED QA] [PASSED QA] Bug 13265: Use sessionStorage to store searches instead of cookies

This is a counter patch.
The idea is to provide a permanent solution for the cookie length issue
we occurred on storing the searches (intranet side).

Test plan:
Launch as many searches as you can (don't forget to sleep).
You should not get any error.
Confirm there is no regression using the results browser.

Tested with 6 parralel searches in different tabs (with alternatively browising up and down). No problems found.
Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 19 Tomás Cohen Arazi 2015-06-11 13:18:24 UTC
Patch pushed to master.

Thanks Jonathan!
Comment 20 Chris Cormack 2015-06-11 20:20:41 UTC
Pushed to 3.20.x will be in 3.20.1
Comment 21 Liz Rea 2015-06-22 23:28:51 UTC
Patch pushed to 3.18.x will be in 3.18.08
Comment 22 Mirko Tietgen 2016-04-10 15:05:09 UTC
*** Bug 12510 has been marked as a duplicate of this bug. ***