Bug 11701 - Fix C4::Context::Zconn
Summary: Fix C4::Context::Zconn
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Z39.50 / SRU / OpenSearch Servers (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Julian Maurice
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 10344
  Show dependency treegraph
 
Reported: 2014-02-06 16:15 UTC by Julian Maurice
Modified: 2015-06-04 23:23 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 11701: Unit tests for C4::Context::Zconn (1.14 KB, patch)
2014-02-06 16:15 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 11701: Fix C4::Context::Zconn (1.92 KB, patch)
2014-02-06 16:16 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 11701: Fix C4::Context::Zconn (2.26 KB, patch)
2014-02-19 11:09 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 11701: Unit tests for C4::Context::Zconn (1.14 KB, patch)
2014-03-14 09:18 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 11701: Fix C4::Context::Zconn (2.19 KB, patch)
2014-03-14 09:18 UTC, Julian Maurice
Details | Diff | Splinter Review
[SIGNED OFF] Bug 11701: Unit tests for C4::Context::Zconn (1.31 KB, patch)
2014-03-14 18:17 UTC, Mark Tompsett
Details | Diff | Splinter Review
[SIGNED OFF] Bug 11701: Fix C4::Context::Zconn (2.76 KB, patch)
2014-03-14 18:18 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 11701: Unit tests for C4::Context::Zconn (1.36 KB, patch)
2014-03-21 09:49 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11701: Fix C4::Context::Zconn (2.81 KB, patch)
2014-03-21 09:49 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Julian Maurice 2014-02-06 16:15:06 UTC
When calling C4::Context::Zconn once with async=0, then once again with async=1, the same ZOOM::Connection object is returned twice (with option async=0)
Comment 1 Julian Maurice 2014-02-06 16:15:58 UTC Comment hidden (obsolete)
Comment 2 Julian Maurice 2014-02-06 16:16:01 UTC Comment hidden (obsolete)
Comment 3 Marcel de Rooy 2014-02-12 13:18:03 UTC
I wonder if this report should not cover the further parameters of Zconn too.
What about $auth, $piggyback and $syntax?
These could (theoretically) have different values too. In the same way the routine should check if it returns the correct object.

If you argue that the current codebase does not use these parameters, I probably agree glancing through some grep results. But then we should better remove these parameters and give them default values instead of pretending to support them and at the same time ignoring them just as we do for async until now..

Could you please have a look at that?
Comment 4 Julian Maurice 2014-02-19 11:09:31 UTC Comment hidden (obsolete)
Comment 5 Julian Maurice 2014-02-19 11:10:49 UTC
A quick grep shows me that other parameters are not used, but it isn't harder to support all parameters.
Comment 6 Dobrica Pavlinusic 2014-03-12 11:47:27 UTC
I get merge conflict when trying to apply this patch:

Applying: Bug 11701: Unit tests for C4::Context::Zconn
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging t/Context.t
CONFLICT (content): Merge conflict in t/Context.t
Failed to merge in the changes.
Patch failed at 0001 Bug 11701: Unit tests for C4::Context::Zconn

Would you rebase it on top of master please?
Comment 7 Julian Maurice 2014-03-14 09:18:21 UTC Comment hidden (obsolete)
Comment 8 Julian Maurice 2014-03-14 09:18:33 UTC Comment hidden (obsolete)
Comment 9 Mark Tompsett 2014-03-14 18:17:38 UTC Comment hidden (obsolete)
Comment 10 Mark Tompsett 2014-03-14 18:18:24 UTC Comment hidden (obsolete)
Comment 11 Jonathan Druart 2014-03-21 09:49:11 UTC
Created attachment 26506 [details] [review]
Bug 11701: Unit tests for C4::Context::Zconn

TEST PLAN
---------
1) apply patch
2) run koha qa test tool
3) prove -v t/Context.t

Unit tests ran as expected.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 12 Jonathan Druart 2014-03-21 09:49:16 UTC
Created attachment 26507 [details] [review]
Bug 11701: Fix C4::Context::Zconn

When calling C4::Context::Zconn twice with different parameters,
the same ZOOM::Connection object is returned twice (parameters
of 2nd call are not used) This patch fixes that.

This is in part, because the connection cache is keyed on server
name only. This patch corrects this by keying on all parameters.

TEST PLAN
---------
1) apply patch
2) run koha qa test tools
3) prove -v t/Context.t

The unit tests properly triggers the modified routine for
testing. Additionally, in hunting for ways it could break,
no nested synchronous or asynchronous Zconn's were found.
And even if they were, the keying on all parameters should allow
it to function properly without messing up the other connection.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 13 Galen Charlton 2014-03-25 14:48:39 UTC
Pushed to master.  Thanks, Julian!
Comment 14 Fridolin Somers 2014-06-12 09:29:34 UTC
Patch does not apply on 3.14.x.
Since its a test, it's not a big deal.
Comment 15 Fridolin Somers 2014-06-12 09:59:18 UTC
It was easy to rebase so :
Pushed to 3.14.x, will be in 3.14.08