Bug 25513 - Integer casting in Koha::Object->TO_JSON causes random test failures
Summary: Integer casting in Koha::Object->TO_JSON causes random test failures
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Test Suite (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Tomás Cohen Arazi
URL:
Keywords:
Depends on: 17932 18731
Blocks: 25551 25638
  Show dependency treegraph
 
Reported: 2020-05-15 13:23 UTC by Jonathan Druart
Modified: 2020-06-14 21:52 UTC (History)
4 users (show)

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


Attachments
Bug 25513: DO NOT PUSH - try recreating the failure (997 bytes, patch)
2020-05-21 09:13 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25513: DO NOT PUSH - try recreating the failure (1.06 KB, patch)
2020-05-21 10:36 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25513: DO NOT PUSH - try recreating the failure (1.78 KB, patch)
2020-05-21 10:54 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25513: Fix random failure from acquisitions_orders.t (1.16 KB, patch)
2020-05-22 12:37 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25513: Fix random failure from acquisitions_orders.t (1.21 KB, patch)
2020-05-24 18:50 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 25513: Regression tests (1.61 KB, patch)
2020-05-27 13:05 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25513: Fix random failure from acquisitions_orders.t (1.27 KB, patch)
2020-05-27 13:05 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25513: Remove unneeded integer casting in Koha::Object->TO_JSON (1.66 KB, patch)
2020-05-27 13:09 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2020-05-15 13:23:57 UTC
Master_D8 #869

14:12:37 koha_1       |     #   Failed test 'SWAGGER3.3.2'
14:12:37 koha_1       |     #   at t/db_dependent/api/v1/acquisitions_orders.t line 129.
14:12:37 koha_1       |     #     Structures begin differing at:
14:12:37 koha_1       |     #          $got->{ecost_tax_excluded} = '9963405519357589504'
14:12:37 koha_1       |     #     $expected->{ecost_tax_excluded} = '9.96340551935759e+18'
14:12:37 koha_1       |     # Looks like you failed 1 test of 6.
14:12:37 koha_1       | 
14:12:37 koha_1       | #   Failed test 'get() tests'
14:12:37 koha_1       | #   at t/db_dependent/api/v1/acquisitions_orders.t line 141.
Comment 1 Jonathan Druart 2020-05-18 13:13:26 UTC
Master_U18 #788

14:48:26 koha_1       |     #   Failed test 'SWAGGER3.3.2'
14:48:26 koha_1       |     #   at t/db_dependent/api/v1/acquisitions_orders.t line 129.
14:48:26 koha_1       |     #     Structures begin differing at:
14:48:26 koha_1       |     #          $got->{unit_price} = '10177559957753600000'
14:48:26 koha_1       |     #     $expected->{unit_price} = '1.01775599577536e+19'
14:48:26 koha_1       |     # Looks like you failed 1 test of 6.
14:48:26 koha_1       | 
14:48:26 koha_1       | #   Failed test 'get() tests'
14:48:26 koha_1       | #   at t/db_dependent/api/v1/acquisitions_orders.t line 141.
Comment 2 Jonathan Druart 2020-05-20 12:07:50 UTC
Those two runs were on Docker_2
Comment 3 Jonathan Druart 2020-05-20 12:12:46 UTC
Master_U18 #772

01:45:29 koha_1       |     #   Failed test 'SWAGGER3.3.2'
01:45:29 koha_1       |     #   at t/db_dependent/api/v1/acquisitions_orders.t line 129.
01:45:29 koha_1       |     #     Structures begin differing at:
01:45:29 koha_1       |     #          $got->{unit_price_tax_included} = '15313858494394601472'
01:45:29 koha_1       |     #     $expected->{unit_price_tax_included} = '1.53138584943946e+19'
01:45:29 koha_1       |     # Looks like you failed 1 test of 6.
01:45:29 koha_1       | 
01:45:29 koha_1       | #   Failed test 'get() tests'
01:45:29 koha_1       | #   at t/db_dependent/api/v1/acquisitions_orders.t 

Also on Docker_2
Comment 4 didier 2020-05-21 08:44:24 UTC
Hi,

Could be link to 'ecost_tax_excluded' being a decimal 28,6 in mysql and swagger type a double?

It wouldn't be the first time a buggy double to string function didn't roundtrip for some values.

It seems '$expected->{' is coming from /usr/share/perl5/Test/More.pm, but it doesn't print the full number.
Comment 5 Jonathan Druart 2020-05-21 09:11:47 UTC
It comes from a wrong formatting, and I think it depends on some locale settings (for instance it seems to only fail on Docker_2).

The thing people should try is to set 9963405519357589504 for ecost_tax_excluded (for instance).
And see if it fails.

I cannot make it fail locally.
Comment 6 Jonathan Druart 2020-05-21 09:13:33 UTC
Created attachment 105170 [details] [review]
Bug 25513: DO NOT PUSH - try recreating the failure
Comment 7 Jonathan Druart 2020-05-21 09:13:55 UTC
You could try this patch.
Comment 8 didier 2020-05-21 10:10:25 UTC
Hi
There's a thinko in the patch should be:
-            value => { orderstatus => 'new' }
+            value => { orderstatus => 'new', ecost_tax_excluded => 9963405519357589504, unitprice => 10177559957753600000  }

Not
-            value => { orderstatus => 'new' }
+            value => { orderstatus => 'new', value => { ecost_tax_excluded => 9963405519357589504, unitprice => 10177559957753600000 } }

And it fails for me
ok 1 - list() tests
# Subtest: get() tests
    1..6
[Thu May 21 10:08:23 2020] [debug] GET "/api/v1/acquisitions/orders/48"
[Thu May 21 10:08:23 2020] [debug] Routing to controller "Koha::REST::V1::Auth" and action "under"
[Thu May 21 10:08:23 2020] [debug] Routing to controller "Koha::REST::V1::Acquisitions::Orders" and action "get"
[Thu May 21 10:08:23 2020] [debug] 200 OK (0.039639s, 25.228/s)
    ok 1 - GET //fgMbl_VX3EQexGiV17S9yOb3KOVToMz0qyMP_TOrYUeMkzAppWplcQ5izgERU:thePassword123@/api/v1/acquisitions/orders/48
    ok 2 - SWAGGER3.2.2
    not ok 3 - SWAGGER3.3.2

    #   Failed test 'SWAGGER3.3.2'
    #   at ./t/db_dependent/api/v1/acquisitions_orders.t line 129.
    #     Structures begin differing at:
    #          $got->{ecost_tax_excluded} = '9963405519357589504'
    #     $expected->{ecost_tax_excluded} = '9.96340551935759e+18'
[Thu May 21 10:08:23 2020] [debug] GET "/api/v1/acquisitions/orders/48"
[Thu May 21 10:08:23 2020] [debug] Routing to controller "Koha::REST::V1::Auth" and action "under"
[Thu May 21 10:08:23 2020] [debug] Routing to controller "Koha::REST::V1::Acquisitions::Orders" and action "get"
[Thu May 21 10:08:23 2020] [debug] 404 Not Found (0.034099s, 29.326/s)
    ok 4 - GET //fgMbl_VX3EQexGiV17S9yOb3KOVToMz0qyMP_TOrYUeMkzAppWplcQ5izgERU:thePassword123@/api/v1/acquisitions/orders/48
    ok 5 - 404 Not Found
    ok 6 - exact match for JSON Pointer "/error"
    # Looks like you failed 1 test of 6.
not ok 2 - get() tests
Comment 9 Jonathan Druart 2020-05-21 10:18:10 UTC
Indeed!
But still not fail for me (I tried yesterday and wrote the patch this morning...)
Comment 10 Jonathan Druart 2020-05-21 10:22:13 UTC
Didier, ping me on IRC when you are available :)
Comment 11 Jonathan Druart 2020-05-21 10:36:44 UTC
Created attachment 105175 [details] [review]
Bug 25513: DO NOT PUSH - try recreating the failure

This is failing in stretch
Comment 12 Jonathan Druart 2020-05-21 10:54:45 UTC
Created attachment 105177 [details] [review]
Bug 25513: DO NOT PUSH - try recreating the failure

This is failing in stretch:
Stretch with mysql 5.5
Stretch with 10.1.44-MariaDB-1~bionic (docker-compose.mariadb_d9.yml)

Not on Buster with 10.1.44-MariaDB-1~bionic (docker-compose.mariadb_d9.yml)
Comment 13 Jonathan Druart 2020-05-21 10:57:39 UTC
However, using Postman I see the same output (exponent format) with both stretch and buster:
  "unit_price": 1.01775599577536e+19,
  "ecost_tax_excluded": 9.96340551935759e+18,
Comment 14 Jonathan Druart 2020-05-21 11:12:47 UTC
buster has JSON=4.02
stretch has JSON=2.90

% more xxx.pl
use Modern::Perl;
use Mojo::JSON qw(decode_json encode_json);
my $bytes = encode_json {foo => 9963405519357589504, bar => 10177559957753600000};
say $bytes;

buster:
% perl xxx.pl
{"bar":10177559957753600000,"foo":9963405519357589504}

stretch:
% perl xxx.pl
{"bar":10177559957753600000,"foo":9963405519357589504}

I am missing something obvious...
Comment 16 Jonathan Druart 2020-05-21 12:53:55 UTC
(In reply to didier from comment #15)
> Could be something like?
> https://dba.stackexchange.com/questions/172216/why-would-mysqls-roundn-dd-1-
> sometimes-show-no-digits-after-the-decimal-place

Yes, that seems related.
Stretch has 4.041-2 and Buster has 4.050-2
Comment 17 Jonathan Druart 2020-05-21 12:54:45 UTC
(In reply to Jonathan Druart from comment #14)
> buster has JSON=4.02
> stretch has JSON=2.90
> 
> % more xxx.pl
> use Modern::Perl;
> use Mojo::JSON qw(decode_json encode_json);
> my $bytes = encode_json {foo => 9963405519357589504, bar =>
> 10177559957753600000};
> say $bytes;
> 
> buster:
> % perl xxx.pl
> {"bar":10177559957753600000,"foo":9963405519357589504}
> 
> stretch:
> % perl xxx.pl
> {"bar":10177559957753600000,"foo":9963405519357589504}
> 
> I am missing something obvious...

Note that this was meaningless, we wanted to test:

use Mojo::JSON qw(decode_json encode_json);
my $bytes = encode_json {foo => 9963405519357589504.0000, bar => 10177559957753600000.0000};
say $bytes;

Both return 
{"bar":1.01775599577536e+19,"foo":9.96340551935759e+18}
Comment 18 Jonathan Druart 2020-05-22 12:37:18 UTC
Created attachment 105265 [details] [review]
Bug 25513: Fix random failure from acquisitions_orders.t

No idea if this is correct, but the tests pass without that condition

See bug 17932
Comment 19 Jonathan Druart 2020-05-22 12:45:07 UTC
Additional notes:
1. This may break Jessie, so we should have a patch for stables that will "fix" the tests by adding specific value to the order we create
2. There is a "format" parameter in the API spec, but we don't use it
https://swagger.io/docs/specification/data-models/data-types/
Maybe we should.

Another fix would be to extend the test in that area (TO_JSON) and add  _float_column_type. We could then use Math::BigFloat:
  use Math::BigFloat;
  my $x = Math::BigFloat->new(10177559957753600000);
  my $y = Math::BigFloat->new(1.01775599577536e+19);
  say $x;say $y;
will display twice 10177559957753600000
Comment 20 Tomás Cohen Arazi 2020-05-23 02:10:13 UTC
(In reply to Jonathan Druart from comment #19)
> Additional notes:
> 1. This may break Jessie, so we should have a patch for stables that will
> "fix" the tests by adding specific value to the order we create
> 2. There is a "format" parameter in the API spec, but we don't use it
> https://swagger.io/docs/specification/data-models/data-types/
> Maybe we should.
> 
> Another fix would be to extend the test in that area (TO_JSON) and add 
> _float_column_type. We could then use Math::BigFloat:
>   use Math::BigFloat;
>   my $x = Math::BigFloat->new(10177559957753600000);
>   my $y = Math::BigFloat->new(1.01775599577536e+19);
>   say $x;say $y;
> will display twice 10177559957753600000

I agree we should be identifying floats and using:

"type:" "number",
"format": "float"

I would like to mention that the like you provided is for OAS3 and we are using OAS2
https://swagger.io/specification/v2/
Comment 21 Victor Grousset/tuxayo 2020-05-24 18:50:02 UTC
Created attachment 105317 [details] [review]
Bug 25513: Fix random failure from acquisitions_orders.t

No idea if this is correct, but the tests pass without that condition

See bug 17932

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 22 Victor Grousset/tuxayo 2020-05-24 18:55:39 UTC
Tested on koha-testing-docker on Ubuntu 18.04

By running the test in a loop (on master) I could get 2 failures in less that 20 runs.
But after that it took 150 runs to get a failure.

Thanks to the patch "Bug 25513: DO NOT PUSH - try recreating the failure" , now it fails every time.
And the 2nd patch (fix) works! :D
Comment 23 Victor Grousset/tuxayo 2020-05-25 00:47:12 UTC
On master + only the fix: the tests passed 1350 times and counting. Looks pretty solid :)
Comment 24 Victor Grousset/tuxayo 2020-05-25 13:20:01 UTC
In total, overnight, the tests passed 4649 times. Unconceivable to reach without the patch.
Comment 25 Tomás Cohen Arazi 2020-05-27 13:02:35 UTC
This bug is related to an original bug we reported to DBD::mysql RT:
https://rt.cpan.org/Public/Bug/Display.html?id=119904
which is now migrated to:
https://github.com/perl5-dbi/DBD-mysql/issues/212

The basic idea is that under some circumstances DBD::mysql output failed the looks_like_number test because of wrong flags set on the scalar variable.

This bug is removing our workaround under the assumption current DBD::mysql versions on our supported OS are patched. This is the case in Stretch, Buster and Focal. But as the Github link suggests, there's still questions about this being really fixed for good.

I'm adding a regression test based on Jonathan's DO NOT PUSH patch, so our CI can catch any issues now and in the future.
Comment 26 Tomás Cohen Arazi 2020-05-27 13:05:17 UTC
Created attachment 105393 [details] [review]
Bug 25513: Regression tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 27 Tomás Cohen Arazi 2020-05-27 13:05:22 UTC Comment hidden (obsolete)
Comment 28 Tomás Cohen Arazi 2020-05-27 13:09:27 UTC
Created attachment 105395 [details] [review]
Bug 25513: Remove unneeded integer casting in Koha::Object->TO_JSON

This patch removes a problematic integer casting happening in TO_JSON.
The original need for this cast (a DBD::mysql bug) is now fixed. We
remove the cast as it is causing random failures.

To test:
1. Apply the regression tests
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/acquisitions_orders.t
=> FAIL: Tests fail with select high values (i.e. casting gives wrong
        results)
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests now pass!
5. Sign off :-D

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 29 Jonathan Druart 2020-05-29 09:49:13 UTC
Pushed to master for 20.05
Comment 31 Jonathan Druart 2020-06-08 05:51:10 UTC
I confirm I can make it fail 100% of the time on U18, but only occasionally on D10 (hum?)

On U18 it's failing on the tests we wrote for this specific problem ("Regression tests for bug 25513"), but on D10 it fails on another one:

t/db_dependent/api/v1/acquisitions_orders.t .. 1/5 
    #   Failed test 'SWAGGER3.3.2'
    #   at t/db_dependent/api/v1/acquisitions_orders.t line 129.
    #     Structures begin differing at:
    #          $got->{ecost_tax_included} = '2.7621662673219e+21'
    #     $expected->{ecost_tax_included} = '2762166267321895092224.000000'
Comment 32 Aleisha Amohia 2020-06-14 21:52:00 UTC
missing dependencies, not backported to 19.11.x