We have several libraries that have problems with their batch imports/updates from external sources after upgrading to 22.05.06. The problem is that records that are imported and/or updated via bulkmarcimport.pl do not get a 999 field, with the biblionumber in $c and $d. No 999 field is added at all. To reproduce in koha-testing-docker: - Edit koha/t/db_dependent/Record/testrecords/marcxml_utf8.xml and remove the 952 field (otherwise Koha will complain about an undefined branchcode) - Import that file: $ sudo koha-shell -c "perl koha/misc/migration_tools/bulkmarcimport.pl -b -v -m=MARCXML -match=Control-number,001 -insert -l=/tmp/import.log -file koha/t/db_dependent/Record/testrecords/marcxml_utf8.xml" kohadev - Check the result: $ sudo koha-mysql kohadev MariaDB [koha_kohadev]> SELECT biblionumber, ExtractValue(metadata, 'count(//datafield[@tag="999"])') AS count999 FROM biblio_metadata HAVING count999 = 0; This should give output like this: +--------------+----------+ | biblionumber | count999 | +--------------+----------+ | 439 | 0 | +--------------+----------+ 1 row in set (0.013 sec) ...meaning biblionumber 439 does not have a 999 field. - Updating the same record: $ sudo koha-shell -c "perl koha/misc/migration_tools/bulkmarcimport.pl -b -v -m=MARCXML -match=Control-number,001 -update -l=/tmp/import.log -file koha/t/db_dependent/Record/testrecords/marcxml_utf8.xml" kohadev ...seems to add the 999 and make the problem go away: $ sudo koha-mysql kohadev MariaDB [koha_kohadev]> SELECT biblionumber, ExtractValue(metadata, 'count(//datafield[@tag="999"])') AS count999 FROM biblio_metadata HAVING count999 = 0; Empty set (0.009 sec) BUT on live servers (running 22.05.06, not master like in ktd) it looks like updating a record via bulkmarcimport.pl can cause an existing record to loose its 999 field.
This is a bit weird, but now I can't reproduce the exact behaviour described in the first comment. On the first run with -import, the 999 field is missing. But if I the exact same command once more, the 999 goes away. And then I can re-run the -import as many times as I want, and no 999 is added. So run bulkmarcimport.pl with -import twice to reproduce the problem.
Done some more digging, and I think I found the problem. The POD for bulkmarcimport.pl reads: -insert if set, only insert when possible -update if set, only updates (any biblio should have a matching record) -all if set, do whatever is required The way I interpret this, if -insert is provided, records that are not present in the database (based on the matching provided by -match) should be inserted, and records that are already present should be skipped and not updated. If the script is run with -match and -insert, it will look for an existing record. If one is found, then the following logic happens: if ($biblionumber) { eval{ $biblioitemnumber = Koha::Biblios->find( $biblionumber )->biblioitem->biblioitemnumber; }; if ($update) { # If run with -update, but this is not what we are looking for } else { # If run with -insert, nothing happens, except some logging printlog( { id => $id || $originalid || $biblionumber, op => "insert", status => "warning : already in database" } ) if ($logfile); } } else { # An existing record was not found } This looks OK, nothing happens if there is a match and -insert is provided. But then, further down in the script, ModBiblioMarc gets run for all records: ModBiblioMarc( $clone_record, $biblionumber ); This updates biblio_metadata.metadata, even if we did not want to update records that have a match! To check this behviour, run the "bulkmarcimport.pl -import" command I gave in the first comment, then log into the database and run this: MariaDB [koha_kohadev]> select timestamp from biblio_metadata where biblionumber = 439; (Replace 439 with the biblionumber you got, if you got another biblionumber.) Then run "bulkmarcimport.pl -import" again and check the timestamp again. It will be different from the first timestamp. This could have worked (sort of), if the 999 field was added in ModBiblioMarc, but since bug 29486 that has not been the case. I think the solution here is to not run ModBiblioMarc for records that have a match, when run with -insert?
I am not familiar with this script, but I am wondering: should not we skip to the next record way before we reach those lines? Maybe we need a "next" in the "search for a match" block? iff --git a/misc/migration_tools/bulkmarcimport.pl b/misc/migration_tools/bulkmarcimport.pl index d2dfe1ad56a..45a7c94e39c 100755 --- a/misc/migration_tools/bulkmarcimport.pl +++ b/misc/migration_tools/bulkmarcimport.pl @@ -349,6 +349,7 @@ RECORD: while ( ) { next; } } + next if $insert; } elsif ( $results && scalar(@$results) > 1 ) { $logger->debug("more than one match for $query"); } else { But not knowing much this script it's hard to tell if it won't introduce regressions. Or, another option, more inline with your diagnostic, could be to diff --git a/misc/migration_tools/bulkmarcimport.pl b/misc/migration_tools/bulkmarcimport.pl index d2dfe1ad56a..e95b1fa199e 100755 --- a/misc/migration_tools/bulkmarcimport.pl +++ b/misc/migration_tools/bulkmarcimport.pl @@ -480,7 +480,7 @@ RECORD: while ( ) { C4::Biblio::_strip_item_fields($clone_record, ''); # This sets the marc fields if there was an error, and also calls # defer_marc_save. - ModBiblioMarc( $clone_record, $biblionumber ); + ModBiblioMarc( $clone_record, $biblionumber ) if $insert; if ( $error_adding ) { warn "ERROR: Adding items to bib $biblionumber failed: $error_adding"; printlog({id=>$id||$originalid||$biblionumber, op=>"insertitem",status=>"ERROR"}) if ($logfile);
Thanks for looking at this! I find it kind of hard to get my head around the logic of this script too. > Maybe we need a "next" in the "search for a match" block? I *think* it makes sense to skip to the next record if we have -insert, and we have found a matching record. But if we skip as early as line ~351, we miss the logging that happens on line 457: printlog( { id => $id || $originalid || $biblionumber, op => "insert", status => "warning : already in database" } ) if ($logfile); So that should probably be moved to before the "next". > Or, another option, more inline with your diagnostic, could be to... > > - ModBiblioMarc( $clone_record, $biblionumber ); > + ModBiblioMarc( $clone_record, $biblionumber ) if $insert; Just before this we do this: my $clone_record = $record->clone(); This seems to be unnecessary if are not updating anything anyway.
Magnus, is there an easy way to get around this with a fix?
(In reply to Hans Pålsson from comment #5) > Magnus, is there an easy way to get around this with a fix? It's been a long time since I looked at this, but from the comments above it sounds like it should be possible to add a "next" and avoid the problem.
In our test environment I still have the same issue, on 23.05.04.
(In reply to Magnus Enger from comment #6) > (In reply to Hans Pålsson from comment #5) > > Magnus, is there an easy way to get around this with a fix? > > It's been a long time since I looked at this, but from the comments above it > sounds like it should be possible to add a "next" and avoid the problem. Magnus, we actually did find a strange thing in our installation. By removing the plugin Biblibre Dibspayment and the db-tables allocated to it posts actually get imported (!) with 999 correctly. So Insert seems to work. However Update removes 999 from posts which is not wanted behavior. We will try to protect 999 using MarcOverlayRules. I think this should be known since Dibs payment-plugin has nothing to do with import of posts.
(In reply to Hans Pålsson from comment #8) > Magnus, we actually did find a strange thing in our installation. By > removing the plugin Biblibre Dibspayment and the db-tables allocated to it > posts actually get imported (!) with 999 correctly. So Insert seems to work. Reminded me of bug 35930, but guess they are not related. > However Update removes 999 from posts which is not wanted behavior. We will > try to protect 999 using MarcOverlayRules. I think this should be known > since Dibs payment-plugin has nothing to do with import of posts. How did the MarcOverlayRules work?
(In reply to Magnus Enger from comment #9) > How did the MarcOverlayRules work? It did not. :)