Repository: automerge/automerge (language: rust)

Issue:
Why can't keys be empty?
Per [1], keys can't be empty strings in Automerge maps. I've run into a situation in the wild where my JS data naturally contains the empty string as a key, so this will be a bit awkward to work around.

I'm curious if there's a reason Automerge excludes `""` as a key, or if this is just a provisional decision.

(Thanks!)

[1] https://github.com/automerge/automerge/blob/a0ac03edd4c73b78ee3e1bf3b4583bc16804232f/rust/automerge/src/transaction/inner.rs#L419


Discussion / hints:
FWIW, in JS "All String and Symbol values, including the empty string, are valid as property keys." (https://262.ecma-international.org/6.0/#sec-object-type). I know this isn't determinative for Automerge, but it seems like a nice model to follow if possible.
We ran into a similar issue and, while not a real fix, we used this fast-clean library to 'clean' null/undefined/empty string values from our objects before storing them in automerge.
https://github.com/Youssef93/fast-clean

I know that it may not be the most elegant solution, but it worked for us :).
It's been a long time since I added that check for nonempty keys, but IIRC I was worried about cross-language compatibility. Even if JS allows empty strings as property names in an object, other languages may not, and we want Automerge to smoothly interoperate across languages and platforms (this is also the reason why `undefined` is not allowed). A secondary reason was that it might be useful for future extensibility of Automerge if there is one key that applications may not use, because then it could be used for some Automerge-internal (as yet undetermined) purpose in the future.

Both of those arguments are somewhat weak – the might be other keys (say, strings containing spaces or even weird Unicode stuff) that are allowed by Automerge, but that are not valid object properties in some languages. And the extension mechanism may never be needed, as the Automerge binary data format also has several other extension points that will probably be sufficient. So I think it would be okay to relax this constraint and allow empty strings.
For what it's worth, all the existing Automerge implementations support empty strings as hashmap keys:

- Swift
- Golang
- Rust
- JS
- Python
- Java

I'm actually curious which languages forbid empty strings as keys 🤔 
Wouldn't that also imply the language wouldn't be able to decode from json seeing as json supports the empty string?

Fix patch:
diff --git a/rust/automerge/src/error.rs b/rust/automerge/src/error.rs
index a1023ec53..f026e4108 100644
--- a/rust/automerge/src/error.rs
+++ b/rust/automerge/src/error.rs
@@ -12,8 +12,6 @@ pub enum AutomergeError {
     Deflate(#[source] std::io::Error),
     #[error("duplicate seq {0} found for actor {1}")]
     DuplicateSeqNumber(u64, ActorId),
-    #[error("key must not be an empty string")]
-    EmptyStringKey,
     #[error("general failure")]
     Fail,
     #[error("invalid actor ID `{0}`")]
diff --git a/rust/automerge/src/transaction/inner.rs b/rust/automerge/src/transaction/inner.rs
index cbd746dbe..19cb05a66 100644
--- a/rust/automerge/src/transaction/inner.rs
+++ b/rust/automerge/src/transaction/inner.rs
@@ -404,10 +404,6 @@ impl TransactionInner {
         prop: String,
         action: OpType,
     ) -> Result<Option<OpIdx>, AutomergeError> {
-        if prop.is_empty() {
-            return Err(AutomergeError::EmptyStringKey);
-        }
-
         let id = self.next_id();
         let prop_index = doc.ops_mut().osd.props.cache(prop.clone());
         let key = Key::Map(prop_index);


Test patch:
diff --git a/javascript/test/error.ts b/javascript/test/error.ts
index ab23663f2..86d15b97e 100644
--- a/javascript/test/error.ts
+++ b/javascript/test/error.ts
@@ -15,15 +15,4 @@ describe("Automerge errors", () => {
 
     assert(error instanceof Error)
   })
-
-  it("Automerge.from throws an error, not a string", () => {
-    let error
-    try {
-      Automerge.from({ "": "bad key" })
-    } catch (err) {
-      error = err
-    }
-
-    assert(error instanceof Error)
-  })
 })
diff --git a/javascript/test/legacy_tests.ts b/javascript/test/legacy_tests.ts
index 2c8d8770b..7a5051fe1 100644
--- a/javascript/test/legacy_tests.ts
+++ b/javascript/test/legacy_tests.ts
@@ -490,10 +490,9 @@ describe("Automerge", () => {
         assert.strictEqual(s1.prop, true)
       })
 
-      it("should require property names to be valid", () => {
-        assert.throws(() => {
-          Automerge.change(s1, "foo", doc => (doc[""] = "x"))
-        }, /must not be an empty string/)
+      it("should not error on empty string keys", () => {
+        s2 = Automerge.change(s1, "set empty string key", doc => (doc[""] = "x"))
+        assert.strictEqual(s2[""], "x")
       })
 
       it("should not allow assignment of unsupported datatypes", () => {
@@ -681,16 +680,6 @@ describe("Automerge", () => {
         assert.strictEqual(s1.textStyle, undefined)
         assert.deepStrictEqual(s1, { title: "Hello" })
       })
-
-      it("should validate field names", () => {
-        s1 = Automerge.change(s1, doc => (doc.nested = {}))
-        assert.throws(() => {
-          Automerge.change(s1, doc => (doc.nested[""] = "x"))
-        }, /must not be an empty string/)
-        assert.throws(() => {
-          Automerge.change(s1, doc => (doc.nested = { "": "x" }))
-        }, /must not be an empty string/)
-      })
     })
 
     describe("lists", () => {
diff --git a/rust/automerge/tests/test.rs b/rust/automerge/tests/test.rs
index 393d402cf..0347606ba 100644
--- a/rust/automerge/tests/test.rs
+++ b/rust/automerge/tests/test.rs
@@ -2216,3 +2216,15 @@ fn missing_actors_when_docs_are_forked() {
     // error occurs here
     doc2.save_and_verify().unwrap();
 }
+
+#[test]
+fn allows_empty_keys_in_mappings() {
+    let mut doc = AutoCommit::new();
+    doc.put(&automerge::ROOT, "", 1).unwrap();
+    assert_doc!(
+        &doc,
+        map! {
+            "" => { 1 },
+        }
+    );
+}