Bug 37675

Summary: MARC basic editor loses data for records with thousands of the same field
Product: Koha Reporter: Alejandra Tenorio <a.tenorio>
Component: CatalogingAssignee: Bugs List <koha-bugs>
Status: In Discussion --- QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: alexander.wagner, dcook, jonathan.druart, m.de.rooy, oleonard, phil
Version: unspecified   
Hardware: Other   
OS: Linux   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:
Attachments: Marc File
Unmangled but still linked MARC file
Bug 37675: Make random generates the same numbers less probable
Bug 37675: Ensure unique random number server-side
Bug 37675: Remove need for random numbers in MARC subfields
Bug 37675: Use UUID

Description Alejandra Tenorio 2024-08-19 16:16:39 UTC
Created attachment 170470 [details]
Marc File

Dear support,

We have an article with more than 2000 authors, these must be associated with their authorities. However, starting with author 1600, every time this record is edited, Koha loses data from it.

We also checked the table in the database and this is its structure:

mysql> describe biblio_metadata;
+--------------+-------------+------+-----+-------------------+-----------------------------------------------+
| Field        | Type        | Null | Key | Default           | Extra                                         |
+--------------+-------------+------+-----+-------------------+-----------------------------------------------+
| id           | int         | NO   | PRI | NULL              | auto_increment                                |
| biblionumber | int         | NO   | MUL | NULL              |                                               |
| format       | varchar(16) | NO   |     | NULL              |                                               |
| schema       | varchar(16) | YES  |     | NULL              |                                               |
| metadata     | longtext    | NO   |     | NULL              |                                               |
| timestamp    | timestamp   | NO   | MUL | CURRENT_TIMESTAMP | DEFAULT_GENERATED on update CURRENT_TIMESTAMP |
+--------------+-------------+------+-----+-------------------+-----------------------------------------------+
6 rows in set (0.01 sec)


I share with you the MARC file, has this error happened to you?
Comment 1 Katrin Fischer 2024-08-19 17:05:11 UTC
You say it loses data after being edited. Did you alter your MARC frameworks? 

What data is lost? Are the fields/subfields lost being present in the framework? Are they set to be "ignored" or have been deleted?
Comment 2 Alejandra Tenorio 2024-08-19 19:08:58 UTC
----
You say it loses data after being edited. Did you alter your MARC frameworks? 

There came a time when, at each edit/save, authors are lost (700 field). This does not happen with articles with few authors.

We have not edited our MARC frameworks, we have the 700 field as repeatable and not mandatory.

----
What data is lost? Are the fields/subfields lost being present in the framework? 
A whole 700 field is lost, with all its subfields.

----
Are they set to be "ignored" or have been deleted?
How can this be configured?


Regards,
Comment 3 Katrin Fischer 2024-08-20 07:11:15 UTC
If only some of many are lost, it's probably not the frameworks. It might be some kind of length limit at play here. Like maybe the number of fields to display?
Adding David as he has a good idea about the inner workings of our MARC conversions I think.
Comment 4 David Cook 2024-08-21 00:48:52 UTC
Nothing comes to mind off the top of my head.

Sounds like this needs more troubleshooting/investigation.
Comment 5 David Cook 2024-08-21 01:12:27 UTC
(In reply to David Cook from comment #4)
> Nothing comes to mind off the top of my head.
> 
> Sounds like this needs more troubleshooting/investigation.

Usually I'd point you at https://koha-community.org/support/ for this one, but I decided to take a little look anyway.

--

I looked at the MARCXML file and it has 1685 700 fields.

Imported... still has 1685 fields.

Trying to edit... and the basic editor doesn't load properly. 

I can see in the developer tools in the browser that 26 fields have duplicate "id" and "name" attributes. So that would explain your data loss. 

I don't know why a person would even try to edit this record. It's fairly clear at a glance that the editor isn't loading properly. 

--

While 1600-2000 authors is a bit of an edge case for many Koha instances, it's not unheard of in research publications. 

It would be interesting to find the threshold where duplicate ID/name fields occur.
Comment 6 David Cook 2024-08-21 01:15:22 UTC
Adding Owen as he might have some thoughts on this one too.
Comment 7 Alexander Wagner 2024-08-21 09:45:23 UTC
> While 1600-2000 authors is a bit of an edge case for many Koha instances, it's not unheard of in research publications. 

Quite common in experimental particle physics ie. every Atlas, CMS, Alice paper these days. (Atlas is currently at ~2.9k, CMS at ~2.3k)

Just to add, that in join² we recently discovered some papers with ~35k "authors". They are not from HEP but from medicine, usually associated with COVID research. (These do cause a lot of headache at a number of other services downstream as well.)
Comment 8 Phil Ringnalda 2024-08-21 20:36:12 UTC
(In reply to David Cook from comment #5)
> It would be interesting to find the threshold where duplicate ID/name fields
> occur.

That's a statistics question: how often does /cataloguing/addbiblio.pl have to call int(rand(1000000) to get the same number twice?
Comment 9 Alejandra Tenorio 2024-08-21 21:05:42 UTC
(In reply to David Cook from comment #5)
> (In reply to David Cook from comment #4)
> > Nothing comes to mind off the top of my head.
> > 
> > Sounds like this needs more troubleshooting/investigation.
> 
> Usually I'd point you at https://koha-community.org/support/ for this one,
> but I decided to take a little look anyway.
> 
> --
> 
> I looked at the MARCXML file and it has 1685 700 fields.
> 
> Imported... still has 1685 fields.
> 
> Trying to edit... and the basic editor doesn't load properly. 
> 
> I can see in the developer tools in the browser that 26 fields have
> duplicate "id" and "name" attributes. So that would explain your data loss. 
> 
> I don't know why a person would even try to edit this record. It's fairly
> clear at a glance that the editor isn't loading properly. 
> 
> --
> 
> While 1600-2000 authors is a bit of an edge case for many Koha instances,
> it's not unheard of in research publications. 
> 
> It would be interesting to find the threshold where duplicate ID/name fields
> occur.


Hi David

---------
I looked at the MARCXML file and it has 1685 700 fields. 
We imported this record, it had 2001 authors, when we edited it to associate them to their authority it began to lose authors. First my colleagues didn't notice that.

---------
Imported... still has 1685 fields.
Yeah!

---------
Trying to edit... and the basic editor doesn't load properly. 
You can edit if the page is zoomed to 70%.

---------
I can see in the developer tools in the browser that 26 fields have duplicate "id" and "name" attributes. So that would explain your data loss. 
We didn't notice that. 


---------
I don't know why a person would even try to edit this record. It's fairly clear at a glance that the editor isn't loading properly. 
Because we have to associate each author with to their authority.


Regards,
Comment 10 David Cook 2024-08-22 02:25:00 UTC
(In reply to Alexander Wagner from comment #7)
> Quite common in experimental particle physics ie. every Atlas, CMS, Alice
> paper these days. (Atlas is currently at ~2.9k, CMS at ~2.3k)
> 
> Just to add, that in join² we recently discovered some papers with ~35k
> "authors". They are not from HEP but from medicine, usually associated with
> COVID research. (These do cause a lot of headache at a number of other
> services downstream as well.)

I suppose it depends on the type of library and how they use Koha. For instance, a circulating public library is probably not going to run into this. An academic library will probably have them in a digital repository and served via a discovery system. A special library hopefully has a digital repository but might be using their Koha for it. The smaller special libraries not using digital repositories are the ones where I'd usually see this scenario.
Comment 11 David Cook 2024-08-22 02:26:10 UTC
(In reply to Phil Ringnalda from comment #8)
> (In reply to David Cook from comment #5)
> > It would be interesting to find the threshold where duplicate ID/name fields
> > occur.
> 
> That's a statistics question: how often does /cataloguing/addbiblio.pl have
> to call int(rand(1000000) to get the same number twice?

I hadn't looked at the code yet. That's interesting! Also, *sighs*. Haha.
Comment 12 David Cook 2024-08-22 02:31:52 UTC
(In reply to Alejandra Tenorio from comment #9)
> Trying to edit... and the basic editor doesn't load properly. 
> You can edit if the page is zoomed to 70%.

Thanks for the tip. 

> ---------
> I don't know why a person would even try to edit this record. It's fairly
> clear at a glance that the editor isn't loading properly. 
> Because we have to associate each author with to their authority.

Sorry I meant that needing to zoom the page to 70% might be an indication that Koha isn't expecting to work this way, so it might not be surprising if it didn't work correctly. Just a miscommunication on my part. No worries.

--

The MARC editor rarely gets attention, so I think a lot of people will want to avoid this issue. I don't mind looking at it, but I probably won't have time for a little while. Even when someone does provide a patch, it might take some time to get it approved. 

So workarounds to help you for now... you could try the Advanced Editor but I don't know if it'll have similar problems. (Phil, you might have some insight here?)

Alternatively, you can do your cataloguing edits in an external problem like MarcEdit and upload (using a 999$c record matching rule to replace the existing record). 

Hopefully someone else can fix this before I can, but that's probably the best help I can provide right now.
Comment 13 Phil Ringnalda 2024-08-22 03:15:16 UTC
I was looking at whether the advanced editor would handle it better, but I got stuck on the fact that the basic editor is apparently not actually losing data, it's just mangling it. When it winds up with duplicate IDs, it then saves two or three 700 fields together in just one. That's invalid MARC, you can't have duplicate $a subfields, but the basic editor doesn't care if an existing record is invalid, it will still let you resave it. The advanced editor does care, so it's not possible to test how it behaves without someone unmangling all the fields that currently have duplicate subfields first.
Comment 14 David Cook 2024-08-22 03:23:39 UTC
(In reply to Phil Ringnalda from comment #13)
> I was looking at whether the advanced editor would handle it better, but I
> got stuck on the fact that the basic editor is apparently not actually
> losing data, it's just mangling it. When it winds up with duplicate IDs, it
> then saves two or three 700 fields together in just one. That's invalid
> MARC, you can't have duplicate $a subfields, but the basic editor doesn't
> care if an existing record is invalid, it will still let you resave it. The
> advanced editor does care, so it's not possible to test how it behaves
> without someone unmangling all the fields that currently have duplicate
> subfields first.

If you don't laugh you cry... haha. Thanks, Phil. I can take a look at that tomorrow.
Comment 15 Phil Ringnalda 2024-08-22 04:14:44 UTC
Created attachment 170580 [details]
Unmangled but still linked MARC file

This is the record exported as MARCXML after unmangling the 700s where two or three separate ones had already been jammed into a single field, but still with the original $9 subfields (which of course don't actually match any authorities we have).
Comment 16 Alexander Wagner 2024-08-22 07:22:18 UTC
> > Just to add, that in join² we recently discovered some papers with ~35k
> > "authors".
[...]
> I suppose it depends on the type of library and how they use Koha.

Of course.

I just added this comment to give an idea how far one would need to scale up the number of authors to be done with (for a while). Besides getting the ids right there might even be some issues rendering the display of whatever editor you use, btw.

> For instance, a circulating public library is probably not going to run into
> this.

Of course not.

[...]

> A special library hopefully has a digital
> repository but might be using their Koha for it.

We were pondering on using Koha as the successor system for our current repository, that is also based on Marc21 and we have this several k author lists use a lot of normalizations etc. Koha would fit a lot of bills. It would however require Koha to get a decent OA repository component. (join² was pondering if we could pull this off, but it was finally decided otherwise for other reasons.)

Still, I think it would be worthwhile to add a decent OA repository component to Koha, but that's another story and also quite a task to get it right. It is actually quite convenient if your repo is your catalogue. I'll miss that.

> The smaller special
> libraries not using digital repositories are the ones where I'd usually see
> this scenario.

It's probably not a matter of size of the library, but on the research area of your institution. I would definitely call us pretty small, while Atlas and CMS papers are regulars in our systems. (More on the repository end / article cataloguing of course.)
Comment 17 Alejandra Tenorio 2024-08-22 22:19:39 UTC
(In reply to David Cook from comment #12)
> (In reply to Alejandra Tenorio from comment #9)
> > Trying to edit... and the basic editor doesn't load properly. 
> > You can edit if the page is zoomed to 70%.
> 
> Thanks for the tip. 
> 
> > ---------
> > I don't know why a person would even try to edit this record. It's fairly
> > clear at a glance that the editor isn't loading properly. 
> > Because we have to associate each author with to their authority.
> 
> Sorry I meant that needing to zoom the page to 70% might be an indication
> that Koha isn't expecting to work this way, so it might not be surprising if
> it didn't work correctly. Just a miscommunication on my part. No worries.
> 
> --
> 
> The MARC editor rarely gets attention, so I think a lot of people will want
> to avoid this issue. I don't mind looking at it, but I probably won't have
> time for a little while. Even when someone does provide a patch, it might
> take some time to get it approved. 
> 
> So workarounds to help you for now... you could try the Advanced Editor but
> I don't know if it'll have similar problems. (Phil, you might have some
> insight here?)
> 
> Alternatively, you can do your cataloguing edits in an external problem like
> MarcEdit and upload (using a 999$c record matching rule to replace the
> existing record). 
> 
> Hopefully someone else can fix this before I can, but that's probably the
> best help I can provide right now.



Hi,

As you suggest, we edited the record using MarcEdit, to test we just added a few authors, we imported it to koha and the process was successful. We continue adding the missing authors.

Regards,
Comment 18 David Cook 2024-08-22 23:18:46 UTC
(In reply to Alexander Wagner from comment #16)
> I just added this comment to give an idea how far one would need to scale up
> the number of authors to be done with (for a while). Besides getting the ids
> right there might even be some issues rendering the display of whatever
> editor you use, btw.

Mmm I understand you now. 

Could also cause issues rendering on the detail page and search results. Could be a good reason to build in some "length handling" for some of the interfaces. 

> We were pondering on using Koha as the successor system for our current
> repository, that is also based on Marc21 and we have this several k author
> lists use a lot of normalizations etc. Koha would fit a lot of bills. It
> would however require Koha to get a decent OA repository component. (join²
> was pondering if we could pull this off, but it was finally decided
> otherwise for other reasons.)
> 
> Still, I think it would be worthwhile to add a decent OA repository
> component to Koha, but that's another story and also quite a task to get it
> right. It is actually quite convenient if your repo is your catalogue. I'll
> miss that.

I hope some day that Koha is more modular so that it would be easier to add in these components.

Although part of the problem is maintenance. It's not easy for a small number of volunteers to maintain such a large and growing codebase. 

Tough to be all things to all people...

But I'd be interested to hear more.

> It's probably not a matter of size of the library, but on the research area
> of your institution. I would definitely call us pretty small, while Atlas
> and CMS papers are regulars in our systems. (More on the repository end /
> article cataloguing of course.)

What I mean is larger institutions usually have more money which means they can pay for more systems. But that's a good point. I do support some library systems for small low budget parts of large wealthy organisations.
Comment 19 David Cook 2024-08-22 23:21:04 UTC
(In reply to Alejandra Tenorio from comment #17)
> As you suggest, we edited the record using MarcEdit, to test we just added a
> few authors, we imported it to koha and the process was successful. We
> continue adding the missing authors.

I'm glad that you've got something working for the short-term. 

Thanks for raising this issue. 

I'm going to keep it on my radar.
Comment 20 Jonathan Druart 2024-08-27 11:14:31 UTC
Created attachment 170764 [details] [review]
Bug 37675: Make random generates the same numbers less probable

This is really not the ideal patch but definitely the easiest to write
and with (hopefully) the least side-effects possible.

Ideally we would like to rethink how this "id" is generated and start
from ... 1

To improve this patch we could follow-up and store the list of the
generate numbers, then generate a new one if it exists already.
Comment 21 Jonathan Druart 2024-08-27 11:15:21 UTC
Terribly ugly patch, and I am not proud of it. But maybe will it fix the problem easily?
Comment 22 Phil Ringnalda 2024-08-27 20:32:40 UTC
Interesting thing about that patch: it doesn't help, nor does adding two more zeros, nor does adding five more zeros.
Comment 23 David Cook 2024-08-27 22:54:18 UTC
(In reply to Phil Ringnalda from comment #22)
> Interesting thing about that patch: it doesn't help, nor does adding two
> more zeros, nor does adding five more zeros.

In hindsight, maybe I re-named this bug prematurely. I assumed it was caused by the duplicate "name" inputs, but I didn't actually verify it...
Comment 24 David Cook 2024-08-27 22:55:09 UTC
(In reply to David Cook from comment #23)
> (In reply to Phil Ringnalda from comment #22)
> > Interesting thing about that patch: it doesn't help, nor does adding two
> > more zeros, nor does adding five more zeros.
> 
> In hindsight, maybe I re-named this bug prematurely. I assumed it was caused
> by the duplicate "name" inputs, but I didn't actually verify it...

Renaming 'Duplicate "id" and "name" inputs in MARC basic editor leads to data loss' to 'MARC basic editor loses data for records with thousands of the same field'
Comment 25 Phil Ringnalda 2024-08-28 02:28:53 UTC
Don't sell the duplicate ids short, just because they aren't caused by the size of the rand() call.

I just loaded the record with duplicate ids for Alalalmeh, S.O. and Pashaei, A., saved the record, reloaded it, and now I still have a field for Alalalmeh, S.O. and no field for Pashaei, A., which is exactly the reported problem.

It's a shame that we can't get rid of the duplicate ids by throwing zeroes at rand(), but it doesn't mean they aren't the problem.
Comment 26 David Cook 2024-08-28 02:49:10 UTC
(In reply to Phil Ringnalda from comment #25)
> Don't sell the duplicate ids short, just because they aren't caused by the
> size of the rand() call.
> 
> I just loaded the record with duplicate ids for Alalalmeh, S.O. and Pashaei,
> A., saved the record, reloaded it, and now I still have a field for
> Alalalmeh, S.O. and no field for Pashaei, A., which is exactly the reported
> problem.
> 
> It's a shame that we can't get rid of the duplicate ids by throwing zeroes
> at rand(), but it doesn't mean they aren't the problem.

Thanks, Phil. I really appreciate you chasing this up. I just meant that I haven't had a chance to investigate this, so I didn't want to say anything with absolute certainty heh.
Comment 27 David Cook 2024-08-28 03:28:31 UTC
(In reply to Phil Ringnalda from comment #25)
> It's a shame that we can't get rid of the duplicate ids by throwing zeroes
> at rand(), but it doesn't mean they aren't the problem.

I just updated cataloguing/addbiblio.pl to guarantee that CreateKey() would always return a unique value, and there are still duplicates. 

The interesting thing is that the duplicates always seem to be around the same number... always about 20-some fields with errors suggesting 10ish duplicate numbers.

Seems like there's another factor here...
Comment 28 David Cook 2024-08-28 03:36:58 UTC
(In reply to David Cook from comment #27)
> (In reply to Phil Ringnalda from comment #25)
> > It's a shame that we can't get rid of the duplicate ids by throwing zeroes
> > at rand(), but it doesn't mean they aren't the problem.
> 
> I just updated cataloguing/addbiblio.pl to guarantee that CreateKey() would
> always return a unique value, and there are still duplicates. 
> 
> The interesting thing is that the duplicates always seem to be around the
> same number... always about 20-some fields with errors suggesting 10ish
> duplicate numbers.
> 
> Seems like there's another factor here...

Aha... Koha/UI/Form/Builder/Biblio.pm has a $self->create_key() as well which also needs updating. Bleargh.
Comment 29 David Cook 2024-08-28 03:52:47 UTC
Created attachment 170795 [details] [review]
Bug 37675: Ensure unique random number server-side

This change ensures that a unique number is used
for the random numbers used in the MARC basic editor.

Note that there is a chance for a near infinite loop for records
with a large number of records. This could be improved by dynamically
increasing the random number max and throwing a fatal error if
too much time is spent generating numbers.

Or we could calculate how many numbers we need ahead of time, and use those.

Note that the Javascript used for creating random numbers does
not have knowledge of the already used set of numbers currently. This
should be improved as well.
Comment 30 David Cook 2024-08-28 03:55:15 UTC
I think Jonathan's patch didn't have much impact, because it didn't target the most necessary "create_key" function. 

With this added patch on top, I am able to prevent the problem at first glance.

But it has its own problems as well. I don't like while() loops in web apps.
Comment 31 David Cook 2024-08-28 04:38:42 UTC
Created attachment 170796 [details] [review]
Bug 37675: Remove need for random numbers in MARC subfields

This change removes the need for random numbers in the index_tag
and index_subfield variables. Instead, respectively, we use the
tag number and an incrementing integer.

This eliminates the possibility of duplicate ID/name values
in server-side generated content.

Note that there is still a risk of a collision when generating
these values via the Javascript at this time.
Comment 32 David Cook 2024-08-28 04:48:31 UTC
Technically, I think there's still some more work to do here.

The main thing I'm thinking is that we add a "data-subfield" attribute to the inputs in the template, we can have the Javascript look up the fields it needs, and then calculate the next key instead of generating a random number. (Alternatively, we could do a class like tag700a or something but a data attribute might look nicer.)

An alternative to that would be to have a variable for tracking the unique random numbers generated by the Javascript (variable scope is a bit of a problem in koha-tmpl/intranet-tmpl/prog/js/cataloging.js for that... we'd need to use something like window.Koha.marc_basic_editor_unique_subfield_keys or something), and we could make sure that the lowest number is a very high number which is unlikely to clash with the server-side generated numbers. But this would still have some risks.

Anyway... I've spent way more time on this than I should've, so I'm going to go have my late lunch and work on other things...
Comment 33 Jonathan Druart 2024-08-28 08:26:08 UTC
(In reply to David Cook from comment #30)
> I think Jonathan's patch didn't have much impact, because it didn't target
> the most necessary "create_key" function. 

Good catch, I missed that one.

> But it has its own problems as well. I don't like while() loops in web apps.

Yes, that's terrible. And the fact the a collision could still happen between server-side and client-side seems pretty bad as well.

What about using UUIDs instead? :)
Comment 34 Jonathan Druart 2024-08-28 08:31:08 UTC
Created attachment 170800 [details] [review]
Bug 37675: Use UUID
Comment 35 Jonathan Druart 2024-08-28 08:32:40 UTC
Generating UUIDs is obviously slower but at least we don't create mess in different places.

Patch for discussion, the editor will need to be tested carefully.
Comment 36 Alexander Wagner 2024-08-28 09:12:02 UTC
(In reply to David Cook from comment #18)

> (In reply to Alexander Wagner from comment #16)
> > I just added this comment to give an idea how far one would need to scale up
> > the number of authors to be done with (for a while). Besides getting the ids
> > right there might even be some issues rendering the display of whatever
> > editor you use, btw.
> 
> Mmm I understand you now. 
> 
> Could also cause issues rendering on the detail page and search results.
> Could be a good reason to build in some "length handling" for some of the
> interfaces. 

Our current system displays them like this:

Aad, G. ; Aakvaag, E. ; Abbott, B. K. ; Abdelhameed, S. ; Abeling, K. ; Abicht, N. J. ; Abidi, H. ; Aboelela, M. ; Aboulhorma, A. ; Abramowicz, H. ; Abreu, H. ; Abulaiti, Y. ; Acharya, B. S. ; Ackermann, A. ; Adam Bourdarios, C. ; Adamczyk, L. ; Addepalli, S. ; Addison, M. ; Adelman, J. ; Adiguzel, A. ; Adye, T. ; Affolder, A. A. ; Affolder, T. ; Afik, Y. ; Agaras, M. N. ; et al. 
Show all 2942 authors

(That we display 25 before the `et.al.` is a political thing for this use case. For Koha etc I'd shorten them as usual to one to three.)

Example: https://pubdb.desy.de/record/612581
 
[...]
> > Still, I think it would be worthwhile to add a decent OA repository
[...]
> But I'd be interested to hear more.

Maybe priv comm as it's a bit drifting from the issue at hand.
Comment 37 David Cook 2024-08-29 00:36:28 UTC
(In reply to Jonathan Druart from comment #35)
> Generating UUIDs is obviously slower but at least we don't create mess in
> different places.
> 
> Patch for discussion, the editor will need to be tested carefully.

I tried adding an alphabetic character to the start of the server-side generated integer, but I started getting some weird Javascript errors. My patch worked, but stopped and the error messages went away.

Haven't tried with the UUIDs yet, but I'd look for that when testing.

(I need to do some customer work now...)