Bug 34339 - $c->validation should be avoided (part 2)
Summary: $c->validation should be avoided (part 2)
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 33556
Blocks:
  Show dependency treegraph
 
Reported: 2023-07-21 13:09 UTC by Jonathan Druart
Modified: 2023-09-15 09:55 UTC (History)
5 users (show)

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


Attachments
Bug 34339: (bug 33556 follow-up) Easy ERM-related changes (18.98 KB, patch)
2023-07-21 13:12 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 34339: Avoid relying on $c->validation - ERM (6.72 KB, patch)
2023-07-21 14:36 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 34339: (bug 33556 follow-up) Easy ERM-related changes (19.03 KB, patch)
2023-09-07 18:39 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 34339: Avoid relying on $c->validation - ERM (6.77 KB, patch)
2023-09-07 18:39 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 34339: (bug 33556 follow-up) Easy ERM-related changes (19.11 KB, patch)
2023-09-08 06:19 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 34339: Avoid relying on $c->validation - ERM (6.86 KB, patch)
2023-09-08 06:19 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 34339: (QA follow-up) Replace new by search in Licenses (965 bytes, patch)
2023-09-08 06:19 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2023-07-21 13:09:41 UTC

    
Comment 1 Jonathan Druart 2023-07-21 13:12:28 UTC
Created attachment 153786 [details] [review]
Bug 34339: (bug 33556 follow-up) Easy ERM-related changes

I left the (easy) ERM cases out because it throws this when running the
QA script:

Processing files before patches
|========================>| 51 / 51 (100.00%)

An error occurred : Inconsistent hierarchy during C3 merge of class 'Koha::REST::V1::ERM::EHoldings::Titles::Local':
	current merge results [
		Koha::REST::V1::ERM::EHoldings::Titles::Local,
	]
	merging failed on 'Mojolicious::Controller' at /kohadevbox/qa-test-tools/koha-qa.pl line 112.

and didn't want this to pollute the rest of the changes.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 2 Jonathan Druart 2023-07-21 13:24:23 UTC
The error can easily be recreated redoing what does the QA script

my @files = qw(
    Koha/REST/V1/ERM/Agreements.pm
    Koha/REST/V1/ERM/EHoldings/Titles/Local.pm
    Koha/REST/V1/ERM/EHoldings/Packages.pm
    Koha/REST/V1/ERM/Licenses.pm
    Koha/REST/V1/ERM/EHoldings/Titles/EBSCO.pm
    Koha/REST/V1/ERM/EHoldings/Packages/Local.pm
);

for my $file (@files) {
    next if $file !~ m|\.pm$|;
    my $module = $file;
    delete $INC{$module};
    $module =~ s|/|::|g;
    $module =~ s|\.pm$||;
    eval "require $module";
    $module->import;
}

This appears before and after the patch. Should not be blocker.
Comment 3 Jonathan Druart 2023-07-21 13:26:33 UTC
I have opened https://gitlab.com/koha-community/qa-test-tools/-/issues/72
Comment 4 Jonathan Druart 2023-07-21 14:36:07 UTC
Created attachment 153796 [details] [review]
Bug 34339: Avoid relying on $c->validation - ERM
Comment 5 Jonathan Druart 2023-07-21 14:37:17 UTC
Test plan:
All EBSCO related actions must be tested in the eRM module.
Comment 6 Marcel de Rooy 2023-07-24 06:54:55 UTC
From 33556:

(In reply to Tomás Cohen Arazi from comment #21)
> (In reply to Marcel de Rooy from comment #20)
> > Please clarify
> 
> I can't, and won't. It is an underlying problem with the modules design,
> which the author(s) should address. My feeling is it is related to this:
> 
> use Mojo::Base 'Mojolicious::Controller';
> 
> use Koha::ERM::EHoldings::Titles;


Just comparing Biblios with ERM/Agreements here:

=== REST/V1/Biblios
use Modern::Perl;
use Mojo::Base 'Mojolicious::Controller';
use Koha::Biblios;
use Koha::DateUtils;
use Koha::Ratings;
use Koha::RecordProcessor;
use C4::Biblio qw( DelBiblio AddBiblio ModBiblio );
use C4::Search qw( FindDuplicate );
use C4::Barcodes::ValueBuilder;
use C4::Context;
use Koha::Items;
use List::MoreUtils qw( any );
use MARC::Record::MiJ;
use Try::Tiny qw( catch try );

=== REST/V1/ERM/Agreements
use Modern::Perl;
use Mojo::Base 'Mojolicious::Controller';
use Koha::ERM::Agreements;
use Scalar::Util qw( blessed );
use Try::Tiny qw( catch try );

I do not really see a difference here. Can anyone help me why ERM/Agreements is doing something different?
Comment 7 Marcel de Rooy 2023-07-24 07:01:53 UTC
To provide a bit more context, the "Inconsistent hierarchy during C3 merge of class" error refers to method resolution order / C3 algorithm and might be caused by e.g. 'use parent' that adds a class twice in @ISA.
I did not see parent here, but base. So should be fine?
But looking at Mojo/Base, I do see some mangling of @ISA in there.
At perlmonks someone suggested to clear @ISA at some point.
Not sure how to proceed..
Comment 8 Jonathan Druart 2023-07-27 10:13:39 UTC
(In reply to Marcel de Rooy from comment #7)
> To provide a bit more context, the "Inconsistent hierarchy during C3 merge
> of class" error refers to method resolution order / C3 algorithm and might
> be caused by e.g. 'use parent' that adds a class twice in @ISA.
> I did not see parent here, but base. So should be fine?
> But looking at Mojo/Base, I do see some mangling of @ISA in there.
> At perlmonks someone suggested to clear @ISA at some point.
> Not sure how to proceed..

It's not failing on Koha/REST/V1/ERM/Agreements.pm but the packages or titles packages.

The error appears with only
    Koha/REST/V1/ERM/EHoldings/Packages.pm
    Koha/REST/V1/ERM/EHoldings/Packages/Local.pm


Koha/REST/V1/ERM/EHoldings/Packages.pm
use Mojo::Base 'Mojolicious::Controller';
use Koha::REST::V1::ERM::EHoldings::Packages::Local;
use Koha::REST::V1::ERM::EHoldings::Packages::EBSCO;

Koha/REST/V1/ERM/EHoldings/Packages/Local.pm
use Mojo::Base 'Mojolicious::Controller';
Comment 9 Jonathan Druart 2023-07-27 10:13:59 UTC
But really, this is for another bug, it's not related to this patch.
Comment 10 Tomás Cohen Arazi 2023-09-07 18:39:05 UTC
Created attachment 155340 [details] [review]
Bug 34339: (bug 33556 follow-up) Easy ERM-related changes

I left the (easy) ERM cases out because it throws this when running the
QA script:

Processing files before patches
|========================>| 51 / 51 (100.00%)

An error occurred : Inconsistent hierarchy during C3 merge of class 'Koha::REST::V1::ERM::EHoldings::Titles::Local':
	current merge results [
		Koha::REST::V1::ERM::EHoldings::Titles::Local,
	]
	merging failed on 'Mojolicious::Controller' at /kohadevbox/qa-test-tools/koha-qa.pl line 112.

and didn't want this to pollute the rest of the changes.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 11 Tomás Cohen Arazi 2023-09-07 18:39:08 UTC
Created attachment 155341 [details] [review]
Bug 34339: Avoid relying on $c->validation - ERM

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 12 Marcel de Rooy 2023-09-08 06:19:54 UTC
Created attachment 155351 [details] [review]
Bug 34339: (bug 33556 follow-up) Easy ERM-related changes

I left the (easy) ERM cases out because it throws this when running the
QA script:

Processing files before patches
|========================>| 51 / 51 (100.00%)

An error occurred : Inconsistent hierarchy during C3 merge of class 'Koha::REST::V1::ERM::EHoldings::Titles::Local':
	current merge results [
		Koha::REST::V1::ERM::EHoldings::Titles::Local,
	]
	merging failed on 'Mojolicious::Controller' at /kohadevbox/qa-test-tools/koha-qa.pl line 112.

and didn't want this to pollute the rest of the changes.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 13 Marcel de Rooy 2023-09-08 06:19:57 UTC
Created attachment 155352 [details] [review]
Bug 34339: Avoid relying on $c->validation - ERM

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 14 Marcel de Rooy 2023-09-08 06:19:59 UTC
Created attachment 155353 [details] [review]
Bug 34339: (QA follow-up) Replace new by search in Licenses

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 15 Marcel de Rooy 2023-09-08 06:22:47 UTC
 WARN   Koha/REST/V1/ERM/EHoldings/Packages/EBSCO.pm
   WARN   tidiness
                The file is less tidy than before (bad/messy lines before: 25, now: 27)
 WARN   Koha/REST/V1/ERM/EHoldings/Resources/EBSCO.pm
   WARN   tidiness
                The file is less tidy than before (bad/messy lines before: 54, now: 56)
 WARN   Koha/REST/V1/ERM/EHoldings/Titles/EBSCO.pm
   WARN   tidiness
                The file is less tidy than before (bad/messy lines before: 21, now: 24)

=> Just warnings. No reason to block pushing imo.

Corrected Koha/REST/V1/ERM/Licenses.pm
-        my $license    = $c->objects->find( Koha::ERM::Licenses->search, $license_id );
+        my $license = $c->objects->find( Koha::ERM::Licenses->new, $c->param('license_id') );

prove t/db_dependent/api/v1/erm*
t/db_dependent/api/v1/erm_agreements.t ........... ok
t/db_dependent/api/v1/erm_documents.t ............ ok
t/db_dependent/api/v1/erm_eholdings_packages.t ... ok
t/db_dependent/api/v1/erm_eholdings_resources.t .. ok
t/db_dependent/api/v1/erm_eholdings_titles.t ..... ok
t/db_dependent/api/v1/erm_licenses.t ............. ok
t/db_dependent/api/v1/erm_users.t ................ ok
All tests successful.
Files=7, Tests=24, 78 wallclock secs ( 0.15 usr  0.05 sys + 68.57 cusr  5.75 csys = 74.52 CPU)
Result: PASS
Comment 16 Marcel de Rooy 2023-09-08 06:23:18 UTC
Any further idea/knowledge about:

Processing files before patches
Error happened when reloading the module Koha::REST::V1::ERM::EHoldings::Packages::EBSCO between the two steps. Might not be a blocker for QA:
Inconsistent hierarchy during C3 merge of class 'Koha::REST::V1::ERM::EHoldings::Packages::EBSCO':
        current merge results [
                Koha::REST::V1::ERM::EHoldings::Packages::EBSCO,
        ]
        merging failed on 'Mojolicious::Controller' at /app/qatools/koha-qa.pl line 114.
Error happened when reloading the module Koha::REST::V1::ERM::EHoldings::Resources::EBSCO between the two steps. Might not be a blocker for QA:
Inconsistent hierarchy during C3 merge of class 'Koha::REST::V1::ERM::EHoldings::Resources::EBSCO':
        current merge results [
                Koha::REST::V1::ERM::EHoldings::Resources::EBSCO,
        ]
        merging failed on 'Mojolicious::Controller' at /app/qatools/koha-qa.pl line 114.
Error happened when reloading the module Koha::REST::V1::ERM::EHoldings::Packages::Local between the two steps. Might not be a blocker for QA:
Inconsistent hierarchy during C3 merge of class 'Koha::REST::V1::ERM::EHoldings::Packages::Local':
        current merge results [
                Koha::REST::V1::ERM::EHoldings::Packages::Local,
        ]
        merging failed on 'Mojolicious::Controller' at /app/qatools/koha-qa.pl line 114.
Comment 17 Tomás Cohen Arazi 2023-09-08 14:54:19 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 18 Fridolin Somers 2023-09-08 19:40:26 UTC
Pushed to 23.05.x for 23.05.04
Comment 19 Matt Blenkinsop 2023-09-15 09:55:03 UTC
Dependency missing for 22.11.x - not backporting

Nice work everyone!