Take another approach to dealing with conflicting flattened keys

The approach in #2542 unfortunately introduced a regression, as whilst
it ensured that duplicate keys are removed when downgrading, it no longer
prevented the duplicates from being removed when converting to a legacy
Schema11 object. This resulted in things like backend.get_notetype_legacy()
returning duplicate keys, and could break syncing:

https://forums.ankiweb.net/t/windows-desktop-sync-error/33128

As syncing and schema11 object usage is quite common compared to downgrading,
the extra Value deserialization seemed a bit expensive, so I've switched
back to explicitly removing the problem keys. To ensure we don't forget to
add new keys in the future, I've added some new tests that should alert us
whenever a newly-added key is missing from the reserved list.

Cargo.lock

@@ -129,6 +129,7 @@ dependencies = [
  "num_enum",
  "once_cell",
  "percent-encoding-iri",
+ "phf 0.11.2",
  "pin-project",
  "prettyplease 0.2.9",
  "prost",

rslib/Cargo.toml

@@ -72,6 +72,7 @@ num_cpus.workspace = true
 num_enum.workspace = true
 once_cell.workspace = true
 percent-encoding-iri.workspace = true
+phf.workspace = true
 pin-project.workspace = true
 prost.workspace = true
 pulldown-cmark.workspace = true

rslib/src/deckconfig/schema11.rs

@@ -3,6 +3,8 @@
 
 use std::collections::HashMap;
 
+use phf::phf_set;
+use phf::Set;
 use serde::Deserialize as DeTrait;
 use serde::Deserialize;
 use serde::Deserializer;
@@ -337,15 +339,19 @@ impl From<DeckConfig> for DeckConfSchema11 {
             if let Some(new) = top_other.remove("new") {
                 let val: HashMap<String, Value> = serde_json::from_value(new).unwrap_or_default();
                 new_other = val;
+                new_other.retain(|k, _v| !RESERVED_DECKCONF_NEW_KEYS.contains(k))
             }
             if let Some(rev) = top_other.remove("rev") {
                 let val: HashMap<String, Value> = serde_json::from_value(rev).unwrap_or_default();
                 rev_other = val;
+                rev_other.retain(|k, _v| !RESERVED_DECKCONF_REV_KEYS.contains(k))
             }
             if let Some(lapse) = top_other.remove("lapse") {
                 let val: HashMap<String, Value> = serde_json::from_value(lapse).unwrap_or_default();
                 lapse_other = val;
+                lapse_other.retain(|k, _v| !RESERVED_DECKCONF_LAPSE_KEYS.contains(k))
             }
+            top_other.retain(|k, _v| !RESERVED_DECKCONF_KEYS.contains(k));
         }
         let i = c.inner;
         let new_order = i.new_card_insert_order();
@@ -407,13 +413,62 @@ impl From<DeckConfig> for DeckConfSchema11 {
     }
 }
 
+static RESERVED_DECKCONF_KEYS: Set<&'static str> = phf_set! {
+    "id",
+    "newSortOrder",
+    "replayq",
+    "newPerDayMinimum",
+    "usn",
+    "autoplay",
+    "dyn",
+    "maxTaken",
+    "reviewOrder",
+    "buryInterdayLearning",
+    "newMix",
+    "mod",
+    "timer",
+    "name",
+    "interdayLearningMix",
+    "newGatherPriority"
+};
+
+static RESERVED_DECKCONF_NEW_KEYS: Set<&'static str> = phf_set! {
+    "order", "delays", "bury", "perDay", "initialFactor", "ints"
+};
+
+static RESERVED_DECKCONF_REV_KEYS: Set<&'static str> = phf_set! {
+    "maxIvl", "hardFactor", "ease4", "ivlFct", "perDay", "bury"
+};
+
+static RESERVED_DECKCONF_LAPSE_KEYS: Set<&'static str> = phf_set! {
+    "leechFails", "mult", "leechAction", "delays", "minInt"
+};
+
 #[cfg(test)]
 mod test {
+    use itertools::Itertools;
     use serde::de::IntoDeserializer;
     use serde_json::json;
     use serde_json::Value;
 
     use super::*;
+    use crate::prelude::*;
+
+    #[test]
+    fn all_reserved_fields_are_removed() -> Result<()> {
+        let key_source = DeckConfSchema11::default();
+        let mut config = DeckConfig::default();
+        let empty: &[&String] = &[];
+
+        config.inner.other = serde_json::to_vec(&key_source)?;
+        let s11 = DeckConfSchema11::from(config);
+        assert_eq!(&s11.other.keys().collect_vec(), empty);
+        assert_eq!(&s11.new.other.keys().collect_vec(), empty);
+        assert_eq!(&s11.rev.other.keys().collect_vec(), empty);
+        assert_eq!(&s11.lapse.other.keys().collect_vec(), empty);
+
+        Ok(())
+    }
 
     #[test]
     fn new_intervals() {

rslib/src/decks/schema11.rs

@@ -4,6 +4,8 @@
 use std::collections::HashMap;
 
 use anki_proto::decks::deck::normal::DayLimit;
+use phf::phf_set;
+use phf::Set;
 use serde::Deserialize;
 use serde::Serialize;
 use serde_json::Value;
@@ -13,6 +15,7 @@ use super::DeckCommon;
 use super::FilteredDeck;
 use super::FilteredSearchTerm;
 use super::NormalDeck;
+use crate::notetype::schema11::parse_other_fields;
 use crate::prelude::*;
 use crate::serde::default_on_invalid;
 use crate::serde::deserialize_bool_from_anything;
@@ -201,13 +204,6 @@ impl DeckSchema11 {
         }
     }
 
-    // pub(crate) fn common_mut(&mut self) -> &mut DeckCommon {
-    //     match self {
-    //         Deck::Normal(d) => &mut d.common,
-    //         Deck::Filtered(d) => &mut d.common,
-    //     }
-    // }
-
     pub fn id(&self) -> DeckId {
         self.common().id
     }
@@ -397,11 +393,33 @@ impl From<Deck> for DeckCommonSchema11 {
                 DeckKind::Normal(n) => n.description,
                 DeckKind::Filtered(_) => String::new(),
             },
-            other: serde_json::from_slice(&deck.common.other).unwrap_or_default(),
+            other: parse_other_fields(&deck.common.other, &RESERVED_DECK_KEYS),
         }
     }
 }
 
+static RESERVED_DECK_KEYS: Set<&'static str> = phf_set! {
+    "usn",
+    "revToday",
+    "newLimit",
+    "dyn",
+    "reviewLimit",
+    "newToday",
+    "timeToday",
+    "reviewLimitToday",
+    "extendNew",
+    "mod",
+    "newLimitToday",
+    "desc",
+    "name",
+    "lrnToday",
+    "conf",
+    "browserCollapsed",
+    "extendRev",
+    "id",
+    "collapsed"
+};
+
 impl From<&Deck> for DeckTodaySchema11 {
     fn from(deck: &Deck) -> Self {
         let day = deck.common.last_day_studied as i32;
@@ -436,3 +454,23 @@ impl From<FilteredSearchTerm> for FilteredSearchTermSchema11 {
         }
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use itertools::Itertools;
+
+    use super::*;
+
+    #[test]
+    fn all_reserved_fields_are_removed() -> Result<()> {
+        let key_source = DeckSchema11::default();
+        let mut deck = Deck::new_normal();
+        deck.common.other = serde_json::to_vec(&key_source)?;
+        let DeckSchema11::Normal(s11) = DeckSchema11::from(deck) else { panic!() };
+
+        let empty: &[&String] = &[];
+        assert_eq!(&s11.common.other.keys().collect_vec(), empty);
+
+        Ok(())
+    }
+}

rslib/src/notetype/mod.rs

@@ -8,7 +8,7 @@ mod fields;
 mod notetypechange;
 mod render;
 mod restore;
-mod schema11;
+pub(crate) mod schema11;
 mod schemachange;
 mod service;
 pub(crate) mod stock;

rslib/src/notetype/schema11.rs

@@ -3,6 +3,8 @@
 
 use std::collections::HashMap;
 
+use phf::phf_set;
+use phf::Set;
 use serde::Deserialize;
 use serde::Serialize;
 use serde_json::Value;
@@ -127,14 +129,19 @@ fn other_to_bytes(other: &HashMap<String, Value>) -> Vec<u8> {
     }
 }
 
-fn bytes_to_other(bytes: &[u8]) -> HashMap<String, Value> {
+pub(crate) fn parse_other_fields(
+    bytes: &[u8],
+    reserved: &Set<&'static str>,
+) -> HashMap<String, Value> {
     if bytes.is_empty() {
         Default::default()
     } else {
-        serde_json::from_slice(bytes).unwrap_or_else(|e| {
+        let mut map: HashMap<String, Value> = serde_json::from_slice(bytes).unwrap_or_else(|e| {
             println!("deserialization failed for other: {}", e);
             Default::default()
-        })
+        });
+        map.retain(|k, _v| !reserved.contains(k));
+        map
     }
 }
 
@@ -165,11 +172,29 @@ impl From<Notetype> for NotetypeSchema11 {
             latexsvg: c.latex_svg,
             req: CardRequirementsSchema11(c.reqs.into_iter().map(Into::into).collect()),
             original_stock_kind: c.original_stock_kind,
-            other: bytes_to_other(&c.other),
+            other: parse_other_fields(&c.other, &RESERVED_NOTETYPE_KEYS),
         }
     }
 }
 
+static RESERVED_NOTETYPE_KEYS: Set<&'static str> = phf_set! {
+    "latexPost",
+    "flds",
+    "css",
+    "originalStockKind",
+    "id",
+    "usn",
+    "mod",
+    "req",
+    "latexPre",
+    "name",
+    "did",
+    "tmpls",
+    "type",
+    "sortf",
+    "latexsvg"
+};
+
 impl From<CardRequirementSchema11> for CardRequirement {
     fn from(r: CardRequirementSchema11) -> Self {
         CardRequirement {
@@ -266,8 +291,6 @@ impl From<NoteFieldSchema11> for NoteField {
     }
 }
 
-// fixme: must make sure calling code doesn't break the assumption ord is set
-
 impl From<NoteField> for NoteFieldSchema11 {
     fn from(p: NoteField) -> Self {
         let conf = p.config;
@@ -282,11 +305,24 @@ impl From<NoteField> for NoteFieldSchema11 {
             description: conf.description,
             collapsed: conf.collapsed,
             exclude_from_search: conf.exclude_from_search,
-            other: bytes_to_other(&conf.other),
+            other: parse_other_fields(&conf.other, &RESERVED_FIELD_KEYS),
         }
     }
 }
 
+static RESERVED_FIELD_KEYS: Set<&'static str> = phf_set! {
+    "name",
+    "ord",
+    "sticky",
+    "rtl",
+    "plainText",
+    "font",
+    "size",
+    "collapsed",
+    "description",
+    "excludeFromSearch",
+};
+
 #[derive(Serialize, Deserialize, Debug, Default, Clone)]
 pub struct CardTemplateSchema11 {
     pub(crate) name: String,
@@ -329,8 +365,6 @@ impl From<CardTemplateSchema11> for CardTemplate {
     }
 }
 
-// fixme: make sure we don't call this when ord not set
-
 impl From<CardTemplate> for CardTemplateSchema11 {
     fn from(p: CardTemplate) -> Self {
         let conf = p.config;
@@ -348,7 +382,46 @@ impl From<CardTemplate> for CardTemplateSchema11 {
             },
             bfont: conf.browser_font_name,
             bsize: conf.browser_font_size as u8,
-            other: bytes_to_other(&conf.other),
+            other: parse_other_fields(&conf.other, &RESERVED_TEMPLATE_KEYS),
         }
     }
 }
+
+static RESERVED_TEMPLATE_KEYS: Set<&'static str> = phf_set! {
+    "name",
+    "ord",
+    "did",
+    "afmt",
+    "bafmt",
+    "qfmt",
+    "bqfmt",
+    "bfont",
+    "bsize",
+};
+
+#[cfg(test)]
+mod tests {
+    use itertools::Itertools;
+
+    use super::*;
+    use crate::notetype::stock::basic;
+    use crate::prelude::*;
+
+    #[test]
+    fn all_reserved_fields_are_removed() -> Result<()> {
+        let mut nt = basic(&I18n::template_only());
+
+        let key_source = NotetypeSchema11::from(nt.clone());
+        nt.config.other = serde_json::to_vec(&key_source)?;
+        nt.fields[0].config.other = serde_json::to_vec(&key_source.flds[0])?;
+        nt.templates[0].config.other = serde_json::to_vec(&key_source.tmpls[0])?;
+        let s11 = NotetypeSchema11::from(nt);
+
+        let empty: &[&String] = &[];
+        assert_eq!(&s11.other.keys().collect_vec(), empty);
+        assert_eq!(&s11.flds[0].other.keys().collect_vec(), empty);
+        assert_eq!(&s11.tmpls[0].other.keys().collect_vec(), empty);
+
+        Ok(())
+    }
+}

rslib/src/serde.rs

@@ -7,24 +7,7 @@ pub(crate) use serde_aux::field_attributes::deserialize_bool_from_anything;
 pub(crate) use serde_aux::field_attributes::deserialize_number_from_string;
 use serde_json::Value;
 
-use crate::prelude::*;
-
-/// Serializes the value to JSON, removing any duplicate keys in the process.
-///
-/// This function solves a very specific problem when (de)serializing structs on
-/// up-/downgrade:
-/// Older clients may have received keys from a newer client when
-/// syncing, which get bundled into `other`. If they then upgrade, then
-/// downgrade their collection to schema11, serde will serialize the
-/// new default keys, but then add them again from `other`, leading
-/// to the keys being duplicated in the resulting json - which older
-/// clients then can't read. So we need to strip out any new keys we
-/// add.
-pub(crate) fn schema11_to_string(value: impl serde::Serialize) -> Result<String> {
-    serde_json::to_value(value)
-        .and_then(|val| serde_json::to_string(&val))
-        .map_err(Into::into)
-}
+use crate::timestamp::TimestampSecs;
 
 /// Note: if you wish to cover the case where a field is missing, make sure you
 /// also use the `serde(default)` flag.

rslib/src/storage/deck/mod.rs

@@ -404,7 +404,7 @@ impl SqliteStorage {
 
     pub(crate) fn downgrade_decks_from_schema15(&self) -> Result<()> {
         let decks = self.get_all_decks_as_schema11()?;
-        self.set_schema11_decks(&decks)
+        self.set_schema11_decks(decks)
     }
 
     fn get_schema11_decks(&self) -> Result<HashMap<DeckId, DeckSchema11>> {
@@ -420,8 +420,8 @@ impl SqliteStorage {
         Ok(decks)
     }
 
-    pub(crate) fn set_schema11_decks(&self, decks: &HashMap<DeckId, DeckSchema11>) -> Result<()> {
-        let json = crate::serde::schema11_to_string(decks)?;
+    pub(crate) fn set_schema11_decks(&self, decks: HashMap<DeckId, DeckSchema11>) -> Result<()> {
+        let json = serde_json::to_string(&decks)?;
         self.db.execute("update col set decks = ?", [json])?;
         Ok(())
     }

rslib/src/storage/deckconfig/mod.rs

@@ -255,7 +255,7 @@ impl SqliteStorage {
             .collect();
         self.db.execute(
             "update col set dconf=?",
-            params![crate::serde::schema11_to_string(confmap)?],
+            params![serde_json::to_string(&confmap)?],
         )?;
         Ok(())
     }

rslib/src/storage/notetype/mod.rs

@@ -361,7 +361,7 @@ impl SqliteStorage {
 
     pub(crate) fn downgrade_notetypes_from_schema15(&self) -> Result<()> {
         let nts = self.get_all_notetypes_as_schema11()?;
-        self.set_schema11_notetypes(&nts)
+        self.set_schema11_notetypes(nts)
     }
 
     fn get_schema11_notetypes(&self) -> Result<HashMap<NotetypeId, NotetypeSchema11>> {
@@ -379,9 +379,9 @@ impl SqliteStorage {
 
     pub(crate) fn set_schema11_notetypes(
         &self,
-        notetypes: &HashMap<NotetypeId, NotetypeSchema11>,
+        notetypes: HashMap<NotetypeId, NotetypeSchema11>,
     ) -> Result<()> {
-        let json = crate::serde::schema11_to_string(notetypes)?;
+        let json = serde_json::to_string(&notetypes)?;
         self.db.execute("update col set models = ?", [json])?;
         Ok(())
     }