-
-
Notifications
You must be signed in to change notification settings - Fork 44
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Avoid storing deck_config_uuid
in Anki db and deal with fallout
#127
Conversation
35775ed
to
e8dca2f
Compare
e8dca2f
to
107e5a8
Compare
@@ -82,6 +84,12 @@ def _load_deck_config(self): | |||
new_config = DeckConfig.from_collection(self.collection, self.anki_dict["conf"]) | |||
self.deck_config_uuid = new_config.get_uuid() | |||
|
|||
# TODO Remove this once enough time has passed that #106/#116 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
create a separate issue to track this and mention it in the comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also - does this actually help? would we write an updated version of the dict to db?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
create a separate issue to track this and mention it in the comment?
Makes sense! Will do!
also - does this actually help?
Yeah, it helps!
- It ensures that the correct
deck_config_uuid
is set during that given export.
would we write an updated version of the dict to db?
- Yes, we do write an updated version of the deck dict to the db (here), so we're safe going forward (even if we remove this code).
To double check 2:
(steps 1-4 are from adapted from above, but without the changing of the deck config, since we're only concerned with whether deck_config_uuid
is present at all)
- Use the current, published version of the add-on (or
master
fromgit
). - Start with a fresh profile.
- Create a deck (say
foo
) with a couple of notes. - Export
foo
with CrowdAnki. - Re-import
foo
with CrowdAnki. - Using the Anki console (accessed with CtrlShift;) check that
deck_config_uuid
is (undesirably) present in the Anki dict:
print('deck_config_uuid' in mw.col.decks.byName('foo'))
True
- Quit Anki, switch to the version of the add-on from this branch/PR and start Anki again.
- (Optionally, do 5 again, getting
True
, again, to make sure that it's step 8 that removesdeck_config_uuid
, not restarting Anki etc.) - Export
foo
with CrowdAnki, again. - Like in 5 (and 7), check whether
deck_config_uuid
is present in the Anki dict forfoo
:
print('deck_config_uuid' in mw.col.decks.byName('foo'))
False
41099c4
to
146cedc
Compare
We don't want the deck_config_uuid to be imported into the database, since it duplicates the information stored in the deck's `conf` and in the relevant deck config's `crowdanki_uuid`, but isn't updated when `conf` changes, resulting in potentially broken `deck.json`s on export. See Stvad#106, Stvad#116.
The previous commit prevented `deck_config_uuid` from being added to decks. Here we're actively (permanently) removing it from decks which already have it, to prevent future broken exported decks.
(Or at least provide some background on the likely cause of the error.) Note that the error trace is still displayed afterwards anyway!
146cedc
to
123d7d6
Compare
Fix #106, fix #116, fix #61.
Due to the fact that the
deck_config_uuid
is imported byCrowdAnki
into the Anki database as a property of the deck (in thedecks
table, at least in later versions of Anki), but not updated when the options group (Deck config) is changed, and due to the order of keys incrowd_anki/representation/json_serializable.py
(deck.anki_dict["deck_config_uuid"]
(with the stale/hardcodeddeck_config_uuid
) overridesdeck.deck_config_uuid
(which is fetched on demand, usingDeck._load_deck_config
) during export), adeck.json
file with a deck whosedeck_config_uuid
points towards a Deck config that's not actually present indeck_configurations
can be created. This obviously results in an error when a user tries to import the deck.The reproduction steps are:
Start with a fresh profile.
Create a deck (say
foo
) with a couple of notes.Create a new deck config (options group), say
new_options
, via the standard:Options
>Options group
>Manage...
>Add
and switch back toDefault
.Note that this has to be done at this stage, rather than later, otherwise we're confounded by Options group created from another options group inherits its uuid (minor issue) #118.
Export
foo
with CrowdAnki.Re-import
foo
with CrowdAnki.Change the options group of
foo
fromDefault
tonew_options
.Export
foo
with CrowdAnki.Try re-importing
foo
.Expected result: All the imports and exports work fine.
Actual result: The final import fails with a
KeyError
(due to the mismatchingdeck_config_uuid
).I discussed some possible solutions here.
After some thought, I think that the
deck_config_uuid
should simply not be stored in the Anki db (i.e. 2 from the linked comment). It duplicates the information present in theconf
key in the db. We already (sensibly) stripnote_models
orchildren
from the deck object before writing it into the Anki db and there's no reason fordeck_config_uuid
to be different. (That's the first commit.)Unfortunately, this will only prevent the issue from coming up in the future, for people with fresh Anki profiles and creating new decks. We'd still be left with:
deck_config_uuid
still stored in the Anki database, attached to the Anki deck object.This means that if they ever change the options group of a deck with such a stale
deck_config_uuid
, and export it, we'll again have a brokendeck.json
.deck.json
s out there, which will crash users' Ankis if imported.IMO this is far less pressing than 1., because it's easy to notice and diagnose.
The second commit deals with 1. — it (permanently) removes the
deck_config_uuid
from the given deck'sanki_dict
when exporting that deck. (I thought about removingdeck_config_uuid
s from all decks, onCrowdAnki
startup, but I think that that's too risky and invasive.) It can hopefully be removed in a year and so, once we're sure that we've cleared out all thedeck_config_uuid
s from (most) users' databases.The third commit tries to deal with 2., by providing a modal popup with some info. I'm not sure if it's helpful or necessary, though.
We've been pretty lucky that we also have issue #118, as it's considerably mitigated #106/#116. #118 causes different deck configs to have the same
crowdanki_uuid
, so even though thedeck_config_uuid
is not updated when the deck config is changed, it doesn't matter, because both the "right" and the "wrong"deck_config_uuid
s are the same. (In that it matters to the extent that the wrong deck config is probably exported, but at least import doesn't cause a crash.)Summary
Stale
deck_config_uuid
can cause brokendeck.json
s. (Thedeck_config_uuid
is not among thecrowdanki_uuid
s of the deck configs indeck_configurations
.)4ec62298
prevents thedeck_config_uuid
from being stored in the Anki database during import. (Thedeck_config_uuid
duplicates the deck'sconf
and the deck config'scrowdanki_uuid
, so it's not really useful, but as noted above, can be harmful.)e8fa915f
removes thedeck_config_uuid
where it's already present in the database, during export.e8dca2f7
provides the end-user with some info if they do encounter a brokendeck.json
during import. I'm not sure if it actually adds any value.1 resolves the initial bug, 2 and 3 deal with the "fallout".