With zotero 6.0 it breaks the database

Question

With zotero 6.0 it breaks the database

rvlobato opened this issue 3 years ago · 30 comments

With Zotero 6.0 this extension breaks my database. It starts to populate the Title field with 0.
After restarting the application, I get the following error:

There was an error starting Zotero.

You can report this problem in the Zotero Forums.

Error: cleanDOI: argument must be a string
cleanDOI@chrome://zotero/content/xpcom/utilities/utilities.js:407:15
_cacheKeyMappings@chrome://zotero/content/xpcom/retractions.js:773:21
From previous event:
ZoteroService@file:///usr/lib/zotero/components/zotero-service.js:346:7
@chrome://zotero/content/include.js:4:14

And the app does not work any more. The only way to solve is deleting the sqlite database and disabling the extension.

Answer 1 · 2022-03-22T02:00:01.000Z

Hey @rvlobato -- hmm.. I'm taking a look now, I upgraded my zotero to 6 and it worked, so that's a bit unusual

Answer 2 · 2022-03-22T02:01:31.000Z

Just to confirm so I can reproduce, did you already the plugin installed with zotero 6, then the plugin updated caused it to break?

Can you let me know which other plugins you have installed?

When I released I tried the following to make sure so this is a little surprising:

Started with Zotero 5 on v1.11.2 of our plugin -> upgraded zotero to 6, then upgraded our plugin
Started with Zotero 5 on v1.11.3 of our plugin -> upgraded zotero to 6
Fresh install of Zotero 6 with our latest plugin
Fresh install of Zotero 6 with old plugin -> upgraded plugin

All of them worked, so trying to figure out what's going on here

Answer 3 · 2022-03-22T02:18:25.000Z

I have been using the plugin for some time with the zotero. Today I updated zotero and seems that the plugin also had an update, when I have attempted to add a new reference, the Title field was populated with 0. Subsequently, the database was corrupted.

I have:

Better BibTex
DOI Manager
Storage Scanner
Zotfile

I tried a fresh install and set out to use just your plugin, when zotero starts to sync my database, the Title file come as 0. Maybe it has a problem with my database, due DOI manager, since in the error it says Error: cleanDOI: argument must be a string

Answer 4 · 2022-03-22T02:19:44.000Z

Let me try it with those plugins and get back to you! I tried it in isolation.

Answer 5 · 2022-03-22T02:41:16.000Z

OK -- this is super weird, sometimes if I sync my library with a fresh install and the plugin, it does what you described. And other times, it works. Seems like a race condition with the patched getField, so looking into it. Thanks for reporting this... sorry it caused issues in the first place

Answer 6 · 2022-03-22T02:44:30.000Z

No problem, glad that you were able to reproduce, thanks for looking into it.

Answer 7 · 2022-03-22T03:07:09.000Z

same question

Answer 8 · 2022-03-22T03:22:44.000Z

This is fixed in the latest Zotero beta (6.0.2-beta.1, available in a few minutes if not yet), and the fix will be in 1.0.2 within the next day, but I'm a bit surprised the Scite plugin would cause this.

Basically this is due to a 0 integer being set for a DOI field in the database itself — which should only contain strings — but I'm not seeing any plugin code that modifies items. Is the plugin doing something that would affect DOI fields? Or cause Zotero itself to save a 0 integer to the database?

Answer 9 · 2022-03-22T03:24:21.000Z

Oh, though I see the OP was reporting a problem with the Title field. So then seems like definitely this plugin, and maybe fairly widespread breakage that won't be fixed by our patch.

Answer 10 · 2022-03-22T03:25:14.000Z

That's what I noticed as well (doi being 0, same with title in some cases) when it broke for mine. But it only happens sometimes during syncing. For most of today when I did a normal upgrade it worked fine on my library, which is why it's a little worrying to me...

My best guess at the moment is because I'm patching getField and might be some interaction there -- really it should not be a problem, because I only do "my stuff" if it's a scite specific column, so it seems odd... 👀

Answer 11 · 2022-03-22T03:34:22.000Z

OK, this is wiping out people's database:

https://forums.zotero.org/discussion/comment/402416/#Comment_402416

We're not going to be able to recover from these changes — people are going to have to revert to the last automatic backup or delete the local DB and pull from the online library.

Please push a fix ASAP that disables all functionality immediately while you debug this.

Answer 12 · 2022-03-22T03:37:26.000Z

Or we can do that — we'll need to do something breaks Scite from loading, but we'd need guidance on what that would be. We're not set up to block plugins by id.

Answer 13 · 2022-03-22T03:39:44.000Z

Once this is disabled, we can possibly add a schema update step that clears invalid DB fields containing 0.

Answer 14 · 2022-03-22T03:42:37.000Z

@dstillman I can push a change that makes our plugin do nothing (just shows our columns with an empty loading animation) -- but even if I push that, people will need to upgrade won't they? And if they load their zotero before, the bug will cause it the issue anyway before the upgrade happens?

Answer 15 · 2022-03-22T03:43:53.000Z

Yes. But we may be able to clean up invalid fields in the DB once this is out.

Answer 16 · 2022-03-22T03:45:02.000Z

Can you also set your update.rdf to say that it's not compatible with 6.0? Not sure if that will override a install.rdf that says it's compatible but it's worth a shot, to try to prevent people updating Zotero who haven't yet received the next Scite update from having the plugin enabled.

Answer 17 · 2022-03-22T03:46:26.000Z

Can you also set your update.rdf to say that it's not compatible with 6.0? Not sure if that will override a install.rdf that says it's compatible but it's worth a shot, to try to prevent people updating Zotero who haven't yet received the next Scite update from having the plugin enabled.

Hm... that's tricky because I use another lib to manage generating update.rdf and I did that a few weeks ago by upgrading its dependency -- https://github.com/scitedotai/scite-zotero-plugin/pull/36/files#diff-7ae45ad102eab3b6d7e7896acd08c427a9b25b346470d7bc6507b6481575d519R44

Let me try

Answer 18 · 2022-03-22T03:54:49.000Z

Oh, update.rdf is bundled with releases? That's unfortunate. It's better to be able to update that independently.

Most important is to push a fix that disables all functionality ASAP. People upgrading to Z6 will then hopefully get that version at the same time.

Answer 19 · 2022-03-22T04:02:36.000Z

OK. I released a new version (PR -- #39), which anyone can download here that will make the plugin do nothing (I verified this by doing a fresh Zotero 6 install with that plugin version and then syncing my lib multiple times and made sure it synced correctly each time).

Existing installations should generally auto update like most other plugins.

Answer 20 · 2022-03-22T04:03:46.000Z

I think what's happening is:

A patched function is raising an exception -- I think getField
Maybe it's bubbling up in a weird way and causing DB fields to be written weirdly?

Looking more closely now.

(I'm very sorry about this, to anyone affected)

Answer 21 · 2022-03-22T04:10:09.000Z

Great, thanks for the quick patch.

For others, once you update to plugin version 1.11.4, you can go to Tools → Developer → Run JavaScript and run this:

await Zotero.DB.queryAsync("UPDATE itemDataValues SET value='INVALID_SCITE_VALUE' WHERE value=0")

This should replace all invalid values with the string 'INVALID_SCITE_VALUE', which should prevent further errors and allow you to find and correct them.

Answer 22 · 2022-03-22T04:23:30.000Z

Does this make sense / ring a bell for you --

In the earlier version, this block of code was likely raising an exception if the value for field being passed in wasn't a string (e.g. sometimes it passes an int, which won't have the .includes() method).
Since that's one of the first things that gets run, a non scite specific field could have been passed in, it raises the exception, goes into the catch block which does a return 0

And then something else is taking that and storing the values as 0 (not in the plugin itself, but upstream).

Really that return 0 was only supposed to be called in the catch block for a scite specific column, but if the exception was raised earlier before it verified that the field was actually one of ours, it could have caused non scite fields to be come 0 instead, which would line up with what we're seeing.

In that case, adding a check for

    if (typeof field !== 'string') {
      return original.apply(this, arguments)
    }

before the .includes(...) lines should do the trick, I think? Let me verify with a bigger library...

EDIT: It is a little janky because it was also supporting the old XUL tree version in older clients, where I believe it was passing the id instead of the dataKey, which is the only reason I have to check.

Answer 23 · 2022-03-22T04:45:39.000Z

~~I don't think that's quite right. getField would only be passed a string value, and this would only mangle the DOI field if "DOI" is what was passed. Something else has to be erroring out there.~~

~~I'm not really understanding the XUL part, though. This is patching Zotero.Item.prototype.getField(), no? That shouldn't get passed anything other than a field name.~~

Answer 24 · 2022-03-22T04:47:16.000Z

Oh, I guess the column ids are only for your columns, where it's not a real field? Somehow getField() ends up getting called with the column id?

Answer 25 · 2022-03-22T05:00:06.000Z

(e.g. sometimes it passes an int, which won't have the .includes() method)

Ah, sorry, you're right — it's this. Yes, field can be a field name or fieldID. So that would cause this.

Answer 26 · 2022-03-22T05:00:26.000Z

Yeah I think this should do it -- #40 -- I did not know (or maybe... forgot...) that field could be id (specifically an int), which is the problem.

Answer 27 · 2022-03-22T05:01:19.000Z

I am not going to push this tonight, I know it's late for you too, and I would like to test on a few more libraries, but that seems to be working consistently for me.

Answer 28 · 2022-03-22T16:33:50.000Z

@rvlobato and @trotsky1997 this should be fixed now with v1.11.5 if you want to confirm.

Answer 29 · 2022-03-22T16:44:41.000Z

HI @u-ashish I can confirm that it is working. I think that this issue can be closed. Thanks.

Answer 30 · 2022-03-22T16:45:48.000Z

Super. Thanks again, I hope it didn't disrupt with your work too much.