Bug 24966 - Fix calls to maybe_add where method call does not return a value
Summary: Fix calls to maybe_add where method call does not return a value
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: SIP2 (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Kyle M Hall
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-03-24 12:51 UTC by Kyle M Hall
Modified: 2020-05-13 21:40 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
20.05.00, 19.11.06, 19.05.11


Attachments
Bug 24966: Fix calls to maybe_add where method call does not return a value (2.31 KB, patch)
2020-03-24 12:54 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 24966: Fix calls to maybe_add where method call does not return a value (2.42 KB, patch)
2020-03-24 16:37 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 24966: [Alt] Fix calls to maybe_add where method call does not return a value (35.22 KB, patch)
2020-04-08 17:44 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 24966: [Alt] Fix calls to maybe_add where method call does not return a value (35.97 KB, patch)
2020-04-08 18:05 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 24966: (QA follow-up) Fix return value of hold_patron_bcode (1.12 KB, patch)
2020-04-10 09:52 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 24966: (QA follow-up) Fix return value of hold_patron_bcode (1.15 KB, patch)
2020-04-10 10:44 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 24966: Add unit tests (2.27 KB, patch)
2020-04-10 10:44 UTC, Kyle M Hall
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 2020-03-24 12:51:18 UTC
For reasons unknown to me, a call like:
--
$resp .= maybe_add( FID_CALL_NUMBER, $item->call_number, $server );
--
will not work as expected if the item has no callnumber.

One would expect the parameters to the subroutine to be: 'CY', under, and a SIPServer object.

What is actually received is: 'CY', and a SIPServer object.

We ingest the parameters like so:
--
sub maybe_add {
    my ($fid, $value, $server) = @_;
--
So, what happens is $value is populated with the server object!

This can cause bad output like this:
--
OUTPUT MSG: '101YNN20200324    063701AOBPL|AB32503201584185|AQBPL|AJCat /|CK001|CRn|CSJ 636.8 CLU|CYC4::SIP::SIPServer=HASH(0x1ea0e58)|DAC4::SIP::SIPServer=HASH(0x1ea0e58)|'---

I do not know why this is happening, but there are a couple solutions:
1) Move the $server parameter to be the first parameter of the subroutine call
2) Check the value of $value and swap $value and $server if it is the server object.

I will write a patch implementing the latter solution, as it is a two line change, versus a many line change for the former.
Comment 1 Kyle M Hall 2020-03-24 12:54:14 UTC
Created attachment 101559 [details] [review]
Bug 24966: Fix calls to maybe_add where method call does not return a value

For reasons unknown to me, a call like:
--
$resp .= maybe_add( FID_CALL_NUMBER, $item->call_number, $server );
--
will not work as expected if the item has no callnumber.

One would expect the parameters to the subroutine to be: 'CY', under, and a SIPServer object.

What is actually received is: 'CY', and a SIPServer object.

We ingest the parameters like so:
--
sub maybe_add {
    my ($fid, $value, $server) = @_;
--
So, what happens is $value is populated with the server object!

This can cause bad output like this:
--
OUTPUT MSG: '101YNN20200324    063701AOBPL|AB32503201584185|AQBPL|AJCat /|CK001|CRn|CSJ 636.8 CLU|CYC4::SIP::SIPServer=HASH(0x1ea0e58)|DAC4::SIP::SIPServer=HASH(0x1ea0e58)|'
--

Test Plan:
1) On master, perform a checkin of an item not on hold using the sip cli tester
2) Note some fields contain something like 'C4::SIP::SIPServer=HASH(0x1ea0e58)'
3) Apply this patch
4) Restart the SIP server
5) Perform the SIP checkin again
6) Those fields from step 2 should be gone!
Comment 2 Kyle M Hall 2020-03-24 16:37:30 UTC
Created attachment 101632 [details] [review]
Bug 24966: Fix calls to maybe_add where method call does not return a value

For reasons unknown to me, a call like:
--
$resp .= maybe_add( FID_CALL_NUMBER, $item->call_number, $server );
--
will not work as expected if the item has no callnumber.

One would expect the parameters to the subroutine to be: 'CY', under, and a SIPServer object.

What is actually received is: 'CY', and a SIPServer object.

We ingest the parameters like so:
--
sub maybe_add {
    my ($fid, $value, $server) = @_;
--
So, what happens is $value is populated with the server object!

This can cause bad output like this:
--
OUTPUT MSG: '101YNN20200324    063701AOBPL|AB32503201584185|AQBPL|AJCat /|CK001|CRn|CSJ 636.8 CLU|CYC4::SIP::SIPServer=HASH(0x1ea0e58)|DAC4::SIP::SIPServer=HASH(0x1ea0e58)|'
--

Test Plan:
1) On master, perform a checkin of an item not on hold using the sip cli tester
2) Note some fields contain something like 'C4::SIP::SIPServer=HASH(0x1ea0e58)'
3) Apply this patch
4) Restart the SIP server
5) Perform the SIP checkin again
6) Those fields from step 2 should be gone!

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Barbara Johnson <Barbara.Johnson@bedfordtx.gov>
Comment 3 Jonathan Druart 2020-03-26 10:28:13 UTC
Could you provide a test? I did not manage to write one that fails without your patch.
I suspect a place where we return () instead of undef
Comment 4 Kyle M Hall 2020-04-02 16:06:47 UTC
(In reply to Jonathan Druart from comment #3)
> Could you provide a test? I did not manage to write one that fails without
> your patch.
> I suspect a place where we return () instead of undef

Try as I might, I cannot replicate the error outside of production. Instead of this patch, would a patch reordering the parameters by more acceptable?
Comment 5 Jonathan Druart 2020-04-03 10:00:30 UTC
I am not sure I understand, how would you do that?

My fair is that the real bug is hidden somewhere else, and fixing it here will keep it hidden.
Comment 6 Kyle M Hall 2020-04-08 16:48:41 UTC
(In reply to Jonathan Druart from comment #5)
> I am not sure I understand, how would you do that?
> 
> My fair is that the real bug is hidden somewhere else, and fixing it here
> will keep it hidden.

I agree, bug for the life of me I cannot even understand how this is possible. I think we might be even getting into perl internals finding the bug!

I will write a version of the alternate I proposed. It's a more 'correct' fix anyway imo.
Comment 7 Kyle M Hall 2020-04-08 17:44:40 UTC
Created attachment 102584 [details] [review]
Bug 24966: [Alt] Fix calls to maybe_add where method call does not return a value

For reasons unknown to me, a call like:
--
$resp .= maybe_add( FID_CALL_NUMBER, $item->call_number, $server );
--
will not work as expected if the item has no callnumber.

One would expect the parameters to the subroutine to be: 'CY', under, and a SIPServer object.

What is actually received is: 'CY', and a SIPServer object.

We ingest the parameters like so:
--
sub maybe_add {
    my ($fid, $value, $server) = @_;
--
So, what happens is $value is populated with the server object!

This can cause bad output like this:
--
OUTPUT MSG: '101YNN20200324    063701AOBPL|AB32503201584185|AQBPL|AJCat /|CK001|CRn|CSJ 636.8 CLU|CYC4::SIP::SIPServer=HASH(0x1ea0e58)|DAC4::SIP::SIPServer=HASH(0x1ea0e58)|'
--

Test Plan:
1) On master, perform a checkin of an item not on hold using the sip cli tester
2) Note some fields contain something like 'C4::SIP::SIPServer=HASH(0x1ea0e58)'
3) Apply this patch
4) Restart the SIP server
5) Perform the SIP checkin again
6) Those fields from step 2 should be gone!

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Barbara Johnson <Barbara.Johnson@bedfordtx.gov>
Comment 8 Kyle M Hall 2020-04-08 18:05:34 UTC
Created attachment 102585 [details] [review]
Bug 24966: [Alt] Fix calls to maybe_add where method call does not return a value

For reasons unknown to me, a call like:
--
$resp .= maybe_add( FID_CALL_NUMBER, $item->call_number, $server );
--
will not work as expected if the item has no callnumber.

One would expect the parameters to the subroutine to be: 'CY', under, and a SIPServer object.

What is actually received is: 'CY', and a SIPServer object.

We ingest the parameters like so:
--
sub maybe_add {
    my ($fid, $value, $server) = @_;
--
So, what happens is $value is populated with the server object!

This can cause bad output like this:
--
OUTPUT MSG: '101YNN20200324    063701AOBPL|AB32503201584185|AQBPL|AJCat /|CK001|CRn|CSJ 636.8 CLU|CYC4::SIP::SIPServer=HASH(0x1ea0e58)|DAC4::SIP::SIPServer=HASH(0x1ea0e58)|'
--

Test Plan:
1) On master, perform a checkin of an item not on hold using the sip cli tester
2) Note some fields contain something like 'C4::SIP::SIPServer=HASH(0x1ea0e58)'
3) Apply this patch
4) Restart the SIP server
5) Perform the SIP checkin again
6) Those fields from step 2 should be gone!

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Barbara Johnson <Barbara.Johnson@bedfordtx.gov>
Comment 9 Marcel de Rooy 2020-04-10 08:41:21 UTC
Looking here
Comment 10 Marcel de Rooy 2020-04-10 09:42:45 UTC
We're not on the right track here.
But hang on, found something..
Comment 11 Marcel de Rooy 2020-04-10 09:47:41 UTC
Since you refer to a wrong message with CY, I should have started looking for that in the first place ;)
Check CY in Constants
FID_HOLD_PATRON_ID         => 'CY',
Now git grep FID_HOLD_PATRON_ID
Sip/Constants.pm:        FID_HOLD_PATRON_ID
Sip/Constants.pm:    FID_HOLD_PATRON_ID         => 'CY',
Sip/MsgType.pm:$resp .= maybe_add( FID_HOLD_PATRON_ID,       $item->hold_patron_bcode,   $server );
Ah, this call should be the one. If hold_patron_bcode returns an implicit undef in list context (read: empty list), than we found the culprit.
Look for it:
sub hold_patron_bcode {
    my $self = shift;
    my $borrowernumber = (@_ ? shift: $self->hold_patron_id()) or return;
    my $holder = Koha::Patrons->find( $borrowernumber );
    if ($holder and $holder->cardnumber ) {
        return $holder->cardnumber;
    }
    return;
}
And yes! The last return in list context makes $server shift to the second position.
So how should we address that? It is used only once. We could fix the call with scalar or explicitly return empty string.
I choose for the latter here. Preventing a second bad call in future.
Comment 12 Marcel de Rooy 2020-04-10 09:52:50 UTC
Created attachment 102689 [details] [review]
Bug 24966: (QA follow-up) Fix return value of hold_patron_bcode

It is used in list context, but we need a scalar value.
Can be fixed by adding scalar's, or returning empty string as here.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 13 Marcel de Rooy 2020-04-10 09:54:05 UTC
(In reply to Kyle M Hall from comment #6)
> I agree, bug for the life of me I cannot even understand how this is
> possible. I think we might be even getting into perl internals finding the
> bug!

No perl internals here ;) Just the difference between scalar and list context..
Comment 14 Marcel de Rooy 2020-04-10 09:55:29 UTC
Another note when git grepping maybe_add:

Sip/MsgType.pm:        $resp .= maybe_add( FID_SCREEN_MSG, $msg, $server, $server );
two servers ? this occurs more than once, 15x ?
Comment 15 Jonathan Druart 2020-04-10 10:23:27 UTC
Should not we need another QA review on your patch Marcel? Should not we provide a test to cover the change?
Comment 16 Kyle M Hall 2020-04-10 10:27:00 UTC
(In reply to Marcel de Rooy from comment #14)
> Another note when git grepping maybe_add:
> 
> Sip/MsgType.pm:        $resp .= maybe_add( FID_SCREEN_MSG, $msg, $server,
> $server );
> two servers ? this occurs more than once, 15x ?

Thanks Marcel!

That is definitely a bug as well. I'll file a separate report for that!
Comment 17 Marcel de Rooy 2020-04-10 10:29:54 UTC
(In reply to Jonathan Druart from comment #15)
> Should not we need another QA review on your patch Marcel? Should not we
> provide a test to cover the change?

Leaving that to the RM. Thx for feedback.
Comment 18 Kyle M Hall 2020-04-10 10:44:22 UTC
Created attachment 102697 [details] [review]
Bug 24966: (QA follow-up) Fix return value of hold_patron_bcode

It is used in list context, but we need a scalar value.
Can be fixed by adding scalar's, or returning empty string as here.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 19 Kyle M Hall 2020-04-10 10:44:31 UTC
Created attachment 102698 [details] [review]
Bug 24966: Add unit tests

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 20 Martin Renvoize 2020-04-14 07:37:06 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 21 Joy Nelson 2020-05-04 21:40:06 UTC
backported to 19.11.x for 19.11.06
Comment 22 Lucas Gass 2020-05-13 21:40:15 UTC
backported to 19.05.x for 19.05.11