Repository: yorkie-team/yorkie
Pull request #458 (2023-02-07 08:04:05)

Remove duplicated backslash in string escaping
<!--  Thanks for sending a pull request! -->

**What this PR does / why we need it**:
At PR #439, I escaped RHT key and value strings to fix issue #437.
When checking `EscapeString()` function at that time, I thought it should put two backslashes in front of special characters, because `JSON.stringify()` in JavaScript added two backslashes. For example, the return value of `JSON.stringify({"key": "value\n"})` is `'{"key":"value\\n"}'`.
However, the additional backslash in the example above is just to represent a single backslash in JS string. Valid JSON string uses only a single backslash for control characters. 
Since the updated `EscapeString()` makes invalid JSON strings, it should be reverted.

**Which issue(s) this PR fixes**:
<!--
*Automatically closes linked issue when PR is merged.
Usage: `Fixes #<issue number>`, or `Fixes (paste link of issue)`.
-->
Fixes #439 and #443

**Special notes for your reviewer**:

**Does this PR introduce a user-facing change?**:
<!--
If no, just write "NONE" in the release-note block below.
If yes, a release note is required:
Enter your extended release note in the block below. If the PR requires additional action from users switching to the new release, include the string "action required".
-->
```release-note

```

**Additional documentation**:

<!--
This section can be blank if this pull request does not require a release note.

Please use the following format for linking documentation:
- [Usage]: <link>
- [Other doc]: <link>
-->
```docs

```

**Checklist**:
- [x] Added relevant tests or not required
- [x] Didn't break anything


codecov[bot]: # [Codecov](https://codecov.io/gh/yorkie-team/yorkie/pull/458?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=yorkie-team) Report
> Merging [#458](https://codecov.io/gh/yorkie-team/yorkie/pull/458?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=yorkie-team) (923e8d5) into [main](https://codecov.io/gh/yorkie-team/yorkie/commit/7635b73a236d86133ca6bfdf2e687e3dae9877e2?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=yorkie-team) (7635b73) will **increase** coverage by `0.13%`.
> The diff coverage is `100.00%`.

```diff
@@            Coverage Diff             @@
##             main     #458      +/-   ##
==========================================
+ Coverage   46.47%   46.60%   +0.13%     
==========================================
  Files          70       70              
  Lines        5730     5714      -16     
==========================================
  Hits         2663     2663              
+ Misses       2773     2757      -16     
  Partials      294      294              
```


| [Impacted Files](https://codecov.io/gh/yorkie-team/yorkie/pull/458?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=yorkie-team) | Coverage Δ | |
|---|---|---|
| [pkg/document/crdt/strings.go](https://codecov.io/gh/yorkie-team/yorkie/pull/458?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=yorkie-team#diff-cGtnL2RvY3VtZW50L2NyZHQvc3RyaW5ncy5nbw==) | `100.00% <100.00%> (+36.36%)` | :arrow_up: |

Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=yorkie-team). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=yorkie-team)


Patch:
diff --git a/Makefile b/Makefile
index 71ab3dd8..b354870a 100644
--- a/Makefile
+++ b/Makefile
@@ -59,7 +59,9 @@ test: ## runs integration tests that require local applications such as MongoDB
 	go test -tags integration -race ./...
 
 bench: ## runs benchmark tests
-	go test -tags bench -benchmem -bench=. ./test/bench -memprofile=mem.prof -cpuprofile=cpu.prof | tee output.txt
+	mkfifo pipe
+	tee output.txt < pipe &
+	go test -tags bench -benchmem -bench=. ./test/bench -memprofile=mem.prof -cpuprofile=cpu.prof > pipe
 
 docker: ## builds docker images with the current version and latest tag
 	docker buildx build --push --platform linux/amd64,linux/arm64,linux/386 -t yorkieteam/yorkie:$(YORKIE_VERSION) -t yorkieteam/yorkie:latest .
diff --git a/pkg/document/crdt/strings.go b/pkg/document/crdt/strings.go
index ca9fb251..707677a1 100644
--- a/pkg/document/crdt/strings.go
+++ b/pkg/document/crdt/strings.go
@@ -36,31 +36,24 @@ func EscapeString(s string) string {
 			buf.WriteByte('\\')
 			buf.WriteByte('\\')
 		case '"':
-			buf.WriteByte('\\')
 			buf.WriteByte('\\')
 			buf.WriteByte('"')
 		case '\n':
-			buf.WriteByte('\\')
 			buf.WriteByte('\\')
 			buf.WriteByte('n')
 		case '\f':
-			buf.WriteByte('\\')
 			buf.WriteByte('\\')
 			buf.WriteByte('f')
 		case '\b':
-			buf.WriteByte('\\')
 			buf.WriteByte('\\')
 			buf.WriteByte('b')
 		case '\r':
-			buf.WriteByte('\\')
 			buf.WriteByte('\\')
 			buf.WriteByte('r')
 		case '\t':
-			buf.WriteByte('\\')
 			buf.WriteByte('\\')
 			buf.WriteByte('t')
 		default:
-			buf.WriteByte('\\')
 			buf.WriteByte('\\')
 			buf.WriteByte('u')
 			buf.WriteByte('0')


Test patch:
diff --git a/pkg/document/crdt/rht_test.go b/pkg/document/crdt/rht_test.go
index c422c447..39d20da0 100644
--- a/pkg/document/crdt/rht_test.go
+++ b/pkg/document/crdt/rht_test.go
@@ -12,7 +12,7 @@ func TestMarshal(t *testing.T) {
 		value1 := "world\"\f\b"
 		key2 := "hi"
 		value2 := `test\r`
-		expected := `{"hello\\\\\\t":"world\\"\\f\\b","hi":"test\\r"}`
+		expected := `{"hello\\\\\\t":"world\"\f\b","hi":"test\\r"}`
 
 		rht := NewRHT()
 		rht.Set(key1, value1, nil)
diff --git a/pkg/document/crdt/strings_test.go b/pkg/document/crdt/strings_test.go
index 395ac2cf..2e11b4d5 100644
--- a/pkg/document/crdt/strings_test.go
+++ b/pkg/document/crdt/strings_test.go
@@ -30,30 +30,16 @@ func TestEscapeString(t *testing.T) {
 		assert.Equal(t, expected, actual)
 	})
 
-	t.Run("escape string with doublequote", func(t *testing.T) {
-		str := `hello world"`
-		expected := `hello world\\"`
+	t.Run("escape string with backslash, doublequote and control characters", func(t *testing.T) {
+		str := "hello world\"\\\n\f\b\r\t"
+		expected := `hello world\"\\\n\f\b\r\t`
 		actual := EscapeString(str)
 		assert.Equal(t, expected, actual)
 	})
 
-	t.Run("escape string with raw control character", func(t *testing.T) {
-		str := "hello world\n"
-		expected := `hello world\\n`
-		actual := EscapeString(str)
-		assert.Equal(t, expected, actual)
-	})
-
-	t.Run("escape string with escaped control character", func(t *testing.T) {
-		str := `hello world\n`
-		expected := `hello world\\n`
-		actual := EscapeString(str)
-		assert.Equal(t, expected, actual)
-	})
-
-	t.Run("escape string with raw unicode character", func(t *testing.T) {
-		str := "hello world\u1234"
-		expected := "hello world\u1234"
+	t.Run("escape string with unicode characters", func(t *testing.T) {
+		str := "hello world\u1234\u6645"
+		expected := "hello world\u1234\u6645"
 		actual := EscapeString(str)
 		assert.Equal(t, expected, actual)
 	})
diff --git a/pkg/document/document_test.go b/pkg/document/document_test.go
index 4158b05c..d5d7dd46 100644
--- a/pkg/document/document_test.go
+++ b/pkg/document/document_test.go
@@ -336,7 +336,7 @@ func TestDocument(t *testing.T) {
 			assert.Equal(
 				t,
 				`[0:0:00:0 {} ""][1:2:00:0 {"b":"1"} "Hel"][1:2:00:3 {"b":"1","i":"1"} "lo"]`+
-					`[5:1:00:0 {"list":"true"} "\\n"][4:1:00:0 {} " Yorkie"]{1:2:00:5 {} " world"}`,
+					`[5:1:00:0 {"list":"true"} "\n"][4:1:00:0 {} " Yorkie"]{1:2:00:5 {} " world"}`,
 				text.StructureAsString(),
 			)
 			return nil
@@ -345,7 +345,7 @@ func TestDocument(t *testing.T) {
 		assert.Equal(
 			t,
 			`{"k1":[{"attrs":{"b":"1"},"val":"Hel"},{"attrs":{"b":"1","i":"1"},"val":"lo"},`+
-				`{"attrs":{"list":"true"},"val":"\\n"},{"val":" Yorkie"}]}`,
+				`{"attrs":{"list":"true"},"val":"\n"},{"val":" Yorkie"}]}`,
 			doc.Marshal(),
 		)
 	})
diff --git a/test/bench/document_bench_test.go b/test/bench/document_bench_test.go
index c84be5f2..20dae0af 100644
--- a/test/bench/document_bench_test.go
+++ b/test/bench/document_bench_test.go
@@ -317,7 +317,7 @@ func BenchmarkDocument(b *testing.B) {
 				text.Edit(5, 5, "\n", map[string]string{"list": "true"})
 				assert.Equal(
 					b,
-					`[0:0:00:0 {} ""][1:2:00:0 {"b":"1"} "Hel"][1:2:00:3 {"b":"1","i":"1"} "lo"][5:1:00:0 {"list":"true"} "\\n"][4:1:00:0 {} " Yorkie"]{1:2:00:5 {} " world"}`,
+					`[0:0:00:0 {} ""][1:2:00:0 {"b":"1"} "Hel"][1:2:00:3 {"b":"1","i":"1"} "lo"][5:1:00:0 {"list":"true"} "\n"][4:1:00:0 {} " Yorkie"]{1:2:00:5 {} " world"}`,
 					text.StructureAsString(),
 				)
 				return nil
@@ -325,7 +325,7 @@ func BenchmarkDocument(b *testing.B) {
 			assert.NoError(b, err)
 			assert.Equal(
 				b,
-				`{"k1":[{"attrs":{"b":"1"},"val":"Hel"},{"attrs":{"b":"1","i":"1"},"val":"lo"},{"attrs":{"list":"true"},"val":"\\n"},{"val":" Yorkie"}]}`,
+				`{"k1":[{"attrs":{"b":"1"},"val":"Hel"},{"attrs":{"b":"1","i":"1"},"val":"lo"},{"attrs":{"list":"true"},"val":"\n"},{"val":" Yorkie"}]}`,
 				doc.Marshal(),
 			)
 		}