Add helper function to fix JS subkey encoding

crates/core/src/error.rs

@@ -1,6 +1,9 @@
-use alloc::string::{String, ToString};
+use alloc::{
+    format,
+    string::{String, ToString},
+};
 use core::error::Error;
-use sqlite_nostd::{sqlite3, Connection, ResultCode};
+use sqlite_nostd::{context, sqlite3, Connection, Context, ResultCode};
 
 #[derive(Debug)]
 pub struct SQLiteError(pub ResultCode, pub Option<String>);
@@ -11,6 +14,24 @@ impl core::fmt::Display for SQLiteError {
     }
 }
 
+impl SQLiteError {
+    pub fn apply_to_ctx(self, description: &str, ctx: *mut context) {
+        let SQLiteError(code, message) = self;
+
+        if message.is_some() {
+            ctx.result_error(&format!("{:} {:}", description, message.unwrap()));
+        } else {
+            let error = ctx.db_handle().errmsg().unwrap();
+            if error == "not an error" {
+                ctx.result_error(&format!("{:}", description));
+            } else {
+                ctx.result_error(&format!("{:} {:}", description, error));
+            }
+        }
+        ctx.result_error_code(code);
+    }
+}
+
 impl Error for SQLiteError {}
 
 pub trait PSResult<T> {

crates/core/src/fix_data.rs

@@ -0,0 +1,200 @@
+use core::ffi::c_int;
+
+use alloc::format;
+use alloc::string::String;
+
+use crate::create_sqlite_optional_text_fn;
+use crate::error::{PSResult, SQLiteError};
+use sqlite_nostd::{self as sqlite, ColumnType, Value};
+use sqlite_nostd::{Connection, Context, ResultCode};
+
+use crate::ext::SafeManagedStmt;
+use crate::util::quote_identifier;
+
+// Apply a data migration to fix any existing data affected by the issue
+// fixed in v0.3.5.
+//
+// The issue was that the `ps_updated_rows` table was not being populated
+// with remove operations in some cases. This causes the rows to be removed
+// from ps_oplog, but not from the ps_data__tables, resulting in dangling rows.
+//
+// The fix here is to find these dangling rows, and add them to ps_updated_rows.
+// The next time the sync_local operation is run, these rows will be removed.
+pub fn apply_v035_fix(db: *mut sqlite::sqlite3) -> Result<i64, SQLiteError> {
+    // language=SQLite
+    let statement = db
+      .prepare_v2("SELECT name, powersync_external_table_name(name) FROM sqlite_master WHERE type='table' AND name GLOB 'ps_data__*'")
+      .into_db_result(db)?;
+
+    while statement.step()? == ResultCode::ROW {
+        let full_name = statement.column_text(0)?;
+        let short_name = statement.column_text(1)?;
+        let quoted = quote_identifier(full_name);
+
+        // language=SQLite
+        let statement = db.prepare_v2(&format!(
+            "
+INSERT OR IGNORE INTO ps_updated_rows(row_type, row_id)
+SELECT ?1, id FROM {}
+  WHERE NOT EXISTS (
+      SELECT 1 FROM ps_oplog
+      WHERE row_type = ?1 AND row_id = {}.id
+  );",
+            quoted, quoted
+        ))?;
+        statement.bind_text(1, short_name, sqlite::Destructor::STATIC)?;
+
+        statement.exec()?;
+    }
+
+    Ok(1)
+}
+
+/// Older versions of the JavaScript SDK for PowerSync used to encode the subkey in oplog data
+/// entries as JSON.
+///
+/// It wasn't supposed to do that, since the keys are regular strings already. To make databases
+/// created with those SDKs compatible with other SDKs or the sync client implemented in the core
+/// extensions, a migration is necessary. Since this migration is only relevant for the JS SDK, it
+/// is mostly implemented there. However, the helper function to remove the key encoding is
+/// implemented here because user-defined functions are expensive on JavaScript.
+fn remove_duplicate_key_encoding(key: &str) -> Option<String> {
+    // Acceptable format: <type>/<id>/<subkey>
+    // Inacceptable format: <type>/<id>/"<subkey>"
+    // This is a bit of a tricky conversion because both type and id can contain slashes and quotes.
+    // However, the subkey is either a UUID value or a `<table>/UUID` value - so we know it can't
+    // end in a quote unless the improper encoding was used.
+    if !key.ends_with('"') {
+        return None;
+    }
+
+    // Since the subkey is JSON-encoded, find the start quote by going backwards.
+    let mut chars = key.char_indices();
+    chars.next_back()?; // Skip the quote ending the string
+
+    enum FoundStartingQuote {
+        HasQuote { index: usize },
+        HasBackslachThenQuote { quote_index: usize },
+    }
+    let mut state: Option<FoundStartingQuote> = None;
+    let found_starting_quote = loop {
+        if let Some((i, char)) = chars.next_back() {
+            state = match state {
+                Some(FoundStartingQuote::HasQuote { index }) => {
+                    if char == '\\' {
+                        // We've seen a \" pattern, not the start of the string
+                        Some(FoundStartingQuote::HasBackslachThenQuote { quote_index: index })
+                    } else {
+                        break Some(index);
+                    }
+                }
+                Some(FoundStartingQuote::HasBackslachThenQuote { quote_index }) => {
+                    if char == '\\' {
+                        // \\" pattern, the quote is unescaped
+                        break Some(quote_index);
+                    } else {
+                        None
+                    }
+                }
+                None => {
+                    if char == '"' {
+                        Some(FoundStartingQuote::HasQuote { index: i })
+                    } else {
+                        None
+                    }
+                }
+            }
+        } else {
+            break None;
+        }
+    }?;
+
+    let before_json = &key[..found_starting_quote];
+    let mut result: String = serde_json::from_str(&key[found_starting_quote..]).ok()?;
+
+    result.insert_str(0, before_json);
+    Some(result)
+}
+
+fn powersync_remove_duplicate_key_encoding_impl(
+    ctx: *mut sqlite::context,
+    args: &[*mut sqlite::value],
+) -> Result<Option<String>, SQLiteError> {
+    let arg = args.get(0).ok_or(ResultCode::MISUSE)?;
+
+    if arg.value_type() != ColumnType::Text {
+        return Err(ResultCode::MISMATCH.into());
+    }
+
+    return Ok(remove_duplicate_key_encoding(arg.text()));
+}
+
+create_sqlite_optional_text_fn!(
+    powersync_remote_duplicate_key_encoding,
+    powersync_remove_duplicate_key_encoding_impl,
+    "powersync_remote_duplicate_key_encoding"
+);
+
+pub fn register(db: *mut sqlite::sqlite3) -> Result<(), ResultCode> {
+    db.create_function_v2(
+        "powersync_remote_duplicate_key_encoding",
+        1,
+        sqlite::UTF8 | sqlite::DETERMINISTIC,
+        None,
+        Some(powersync_remote_duplicate_key_encoding),
+        None,
+        None,
+        None,
+    )?;
+    Ok(())
+}
+
+#[cfg(test)]
+mod test {
+    use core::assert_matches::assert_matches;
+
+    use super::remove_duplicate_key_encoding;
+
+    fn assert_unaffected(source: &str) {
+        assert_matches!(remove_duplicate_key_encoding(source), None);
+    }
+
+    #[test]
+    fn does_not_change_unaffected_keys() {
+        assert_unaffected("object_type/object_id/subkey");
+        assert_unaffected("object_type/object_id/null");
+
+        // Object type and ID could technically contain quotes and forward slashes
+        assert_unaffected(r#""object"/"type"/subkey"#);
+        assert_unaffected("object\"/type/object\"/id/subkey");
+
+        // Invalid key, but we shouldn't crash
+        assert_unaffected("\"key\"");
+    }
+
+    #[test]
+    fn removes_quotes() {
+        assert_eq!(
+            remove_duplicate_key_encoding("foo/bar/\"baz\"").unwrap(),
+            "foo/bar/baz",
+        );
+
+        assert_eq!(
+            remove_duplicate_key_encoding(r#"foo/bar/"nested/subkey""#).unwrap(),
+            "foo/bar/nested/subkey"
+        );
+
+        assert_eq!(
+            remove_duplicate_key_encoding(r#"foo/bar/"escaped\"key""#).unwrap(),
+            "foo/bar/escaped\"key"
+        );
+        assert_eq!(
+            remove_duplicate_key_encoding(r#"foo/bar/"escaped\\key""#).unwrap(),
+            "foo/bar/escaped\\key"
+        );
+        assert_eq!(
+            remove_duplicate_key_encoding(r#"foo/bar/"/\\"subkey""#).unwrap(),
+            "foo/bar/\"/\\\\subkey"
+        );
+    }
+}

crates/core/src/lib.rs

@@ -18,7 +18,7 @@ mod crud_vtab;
 mod diff;
 mod error;
 mod ext;
-mod fix035;
+mod fix_data;
 mod json_merge;
 mod kv;
 mod macros;
@@ -57,6 +57,7 @@ fn init_extension(db: *mut sqlite::sqlite3) -> Result<(), ResultCode> {
     crate::views::register(db)?;
     crate::uuid::register(db)?;
     crate::diff::register(db)?;
+    crate::fix_data::register(db)?;
     crate::json_merge::register(db)?;
     crate::view_admin::register(db)?;
     crate::checkpoint::register(db)?;

crates/core/src/macros.rs

@@ -11,18 +11,7 @@ macro_rules! create_sqlite_text_fn {
             let result = $fn_impl_name(ctx, args);
 
             if let Err(err) = result {
-                let SQLiteError(code, message) = SQLiteError::from(err);
-                if message.is_some() {
-                    ctx.result_error(&format!("{:} {:}", $description, message.unwrap()));
-                } else {
-                    let error = ctx.db_handle().errmsg().unwrap();
-                    if error == "not an error" {
-                        ctx.result_error(&format!("{:}", $description));
-                    } else {
-                        ctx.result_error(&format!("{:} {:}", $description, error));
-                    }
-                }
-                ctx.result_error_code(code);
+                SQLiteError::from(err).apply_to_ctx($description, ctx);
             } else if let Ok(r) = result {
                 ctx.result_text_transient(&r);
             }
@@ -43,18 +32,7 @@ macro_rules! create_sqlite_optional_text_fn {
             let result = $fn_impl_name(ctx, args);
 
             if let Err(err) = result {
-                let SQLiteError(code, message) = SQLiteError::from(err);
-                if message.is_some() {
-                    ctx.result_error(&format!("{:} {:}", $description, message.unwrap()));
-                } else {
-                    let error = ctx.db_handle().errmsg().unwrap();
-                    if error == "not an error" {
-                        ctx.result_error(&format!("{:}", $description));
-                    } else {
-                        ctx.result_error(&format!("{:} {:}", $description, error));
-                    }
-                }
-                ctx.result_error_code(code);
+                SQLiteError::from(err).apply_to_ctx($description, ctx);
             } else if let Ok(r) = result {
                 if let Some(s) = r {
                     ctx.result_text_transient(&s);

crates/core/src/migrations.rs

@@ -10,7 +10,7 @@ use sqlite_nostd::{Connection, Context};
 
 use crate::bucket_priority::BucketPriority;
 use crate::error::{PSResult, SQLiteError};
-use crate::fix035::apply_v035_fix;
+use crate::fix_data::apply_v035_fix;
 
 pub const LATEST_VERSION: i32 = 9;