Bug 12343 - TransformKohaToMarc() is adding MARC subfields in random order
Summary: TransformKohaToMarc() is adding MARC subfields in random order
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Jacek Ablewicz
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-06-02 10:56 UTC by Jacek Ablewicz
Modified: 2015-06-04 23:30 UTC (History)
4 users (show)

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


Attachments
Bug 12343 - TransformKohaToMarc() is adding MARC subfields in random order (3.15 KB, patch)
2014-06-03 07:53 UTC, Jacek Ablewicz
Details | Diff | Splinter Review
Bug 12343 - TransformKohaToMarc() is adding MARC subfields in random order (3.20 KB, patch)
2014-06-08 09:18 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 12343 - TransformKohaToMarc() is adding MARC subfields in random order (3.27 KB, patch)
2014-06-11 11:37 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12343: Add regression test for TransformKohaToMarc (1.44 KB, patch)
2014-06-11 11:37 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12343: QA follow-up (1013 bytes, patch)
2014-06-11 11:37 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 Jacek Ablewicz 2014-06-02 10:56:18 UTC
This function iterates through it's argument - which is a hashref - using 'each'. Perl is not guaranteed to return hash keys in any particular order (not to mention that in more recent perl versions, explicit hash key order randomization is to be expected).

As a consequence:

1) For biblio records added via acquisition (order from a new/empty record, order 
from a suggestion), freshly created MARC biblio records doesn't always have 260 $b and 260 $c stored in the proper order

2) Holdings data exported for zebra indexing as 952 fields may have subfields generated in more-or-less random order. While it probably (?) does not affect zebra indexing/searching in any significant way, end result is prone to be somehow ugly (which can be a potential issue e.g. for people running Z39.50 server) and is not guaranteed to be consistent; different records - or even different items in the same record, can have 952 subfields generated in indiscriminate order.
Comment 1 Marcel de Rooy 2014-06-02 11:03:42 UTC
Vaguely remember some earlier discussion about this.. You did not find any references to this in older reports? :)
Comment 2 Jacek Ablewicz 2014-06-02 20:32:13 UTC
(In reply to M. de Rooy from comment #1)
> Vaguely remember some earlier discussion about this.. You did not find any
> references to this in older reports? :)

I performed several bugzilla searches, but no luck - they do not seem to yield anything relevant. Also, according to http://splitter.koha-community.org/, there are no C4/Biblio.pm patches for this issue currently awaiting in the queue (?).

I'm wondering which approach might be best for fixing this:
- just sort (or reverse sort) the darn hash keys - that's a simple one; results would be perhaps not particularly well-sorted, but at least they will be sorted consistently ;)
- sort by field tags & subfield codes (but this function is apparently pretty sensitive performance-wise ?)
- make it to accept arrayref instead of (or in addition to) hashref as an argument, so the subfield order would be preserved - but that would not be all that helpfull for item/holding data subfields which come from the database in the hashref form.
Comment 3 Jacek Ablewicz 2014-06-03 07:53:26 UTC Comment hidden (obsolete)
Comment 4 Chris Cormack 2014-06-08 09:18:22 UTC Comment hidden (obsolete)
Comment 5 Jonathan Druart 2014-06-11 11:35:36 UTC
Comment on attachment 28719 [details] [review]
Bug 12343 - TransformKohaToMarc() is adding MARC subfields in random order

Review of attachment 28719 [details] [review]:
-----------------------------------------------------------------

::: C4/Biblio.pm
@@ +2194,4 @@
>      while ( my ($name, $value) = each %$hash ) {
>          next unless my $dtm = $db_to_marc->{''}->{$name};
>          next unless ( scalar( @$dtm ) );
> +        my ($tag, $letter) = @$dtm; $tag .= '';

Please prefer 1 instruction by line.

@@ +2198,2 @@
>          foreach my $value ( split(/\s?\|\s?/, $value, -1) ) {
> +            $value eq '' && next;

Please prefer
  next if $value eq '';
of
  if ( $value eq '' ) {
    next;
  }
Comment 6 Jonathan Druart 2014-06-11 11:37:38 UTC
Created attachment 28776 [details] [review]
Bug 12343 - TransformKohaToMarc() is adding MARC subfields in random order

TransformKohaToMarc() function in C4/Biblio.pm iterates through it's
argument - which is a hashref - using 'each'. Perl is not guaranteed
to return hash keys in any particular order (not to mention that
in more recent perl versions, explicit hash key order randomization
is to be expected).

As a consequence:
1) For biblio records added via acquisition (order from a new/empty
record, order from a suggestion), freshly created MARC biblio records
doesn't always have 260 $b and 260 $c stored in the proper order
2) Holdings data exported for zebra indexing as 952 fields may have
subfields generated in more-or-less random order. While it probably (?)
does not affect zebra indexing/searching in any significant way,
end result is prone to be somehow ugly (which can be a potential
issue e.g. for people running Z39.50 server) and is not guaranteed
to be consistent; different records - or even different items in the
same record, can have 952 subfields generated in indiscriminate order.

This patch fixes abovementioned issues via introducting explicit
sorting (by subfiled code/letter) for subfield pairs before they
are added to the MARC record.

To test:
1/ Try to confirm and reproduce both issues (use perl 5.18.1 if possible
for more randomly ordered results).
2/ Apply patch.
3/ Redo the tests; ensure that both issues are now fixed and that there
are no apparent regressions of any kind (especially regarding to 952 fields
generated for zebra [re]indexing).

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 7 Jonathan Druart 2014-06-11 11:37:42 UTC
Created attachment 28777 [details] [review]
Bug 12343: Add regression test for TransformKohaToMarc

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 8 Jonathan Druart 2014-06-11 11:37:47 UTC
Created attachment 28778 [details] [review]
Bug 12343: QA follow-up

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 9 Galen Charlton 2014-06-11 17:25:01 UTC
One thing to note regarding subfield order: in the general case, one cannot count on alphabetical order being the correct order for subfields in a field.  For example, the MARC21 246 field puts $i before $a:

246 0# $iAdded title page title on some issues:$aAnnual report

However, in the specific case, since TransformKohaToMarc is used only for creating brief records in acquisitions and embedding item fields in the bib record, this isn't a blocker IMO, but should be noted if TransformKohaToMarc gets used more generally in the future.
Comment 10 Tomás Cohen Arazi 2014-06-16 18:05:52 UTC
Patches pushed to master.

Thanks Jacek!
Comment 11 Galen Charlton 2014-06-18 21:12:56 UTC
Pushed to 3.16.x for inclusion in 3.16.1.