Repository: amzn/ion-go
Pull request #58 (2020-06-06 00:12:36)

Fix for string values
This pull request contains fixes for 2 cases:
- Single quote in a long string: ```''' ion's fun! '''```
- Strings with control characters


codecov-commenter: # [Codecov](https://codecov.io/gh/amzn/ion-go/pull/58?src=pr&el=h1) Report
> Merging [#58](https://codecov.io/gh/amzn/ion-go/pull/58?src=pr&el=desc) into [master](https://codecov.io/gh/amzn/ion-go/commit/e3d4ecd1abfcd47ce7c5fecaefb799510e10b31c&el=desc) will **increase** coverage by `0.23%`.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/amzn/ion-go/pull/58/graphs/tree.svg?width=650&height=150&src=pr&token=OBDT1TNoNE)](https://codecov.io/gh/amzn/ion-go/pull/58?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master      #58      +/-   ##
==========================================
+ Coverage   75.96%   76.19%   +0.23%     
==========================================
  Files          22       22              
  Lines        4609     4629      +20     
==========================================
+ Hits         3501     3527      +26     
+ Misses        682      679       -3     
+ Partials      426      423       -3     
```


| [Impacted Files](https://codecov.io/gh/amzn/ion-go/pull/58?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [ion/tokenizer.go](https://codecov.io/gh/amzn/ion-go/pull/58/diff?src=pr&el=tree#diff-aW9uL3Rva2VuaXplci5nbw==) | `79.78% <100.00%> (+0.54%)` | :arrow_up: |
| [ion/binaryreader.go](https://codecov.io/gh/amzn/ion-go/pull/58/diff?src=pr&el=tree#diff-aW9uL2JpbmFyeXJlYWRlci5nbw==) | `80.95% <0.00%> (+0.68%)` | :arrow_up: |
| [ion/bitstream.go](https://codecov.io/gh/amzn/ion-go/pull/58/diff?src=pr&el=tree#diff-aW9uL2JpdHN0cmVhbS5nbw==) | `79.17% <0.00%> (+0.82%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/amzn/ion-go/pull/58?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/amzn/ion-go/pull/58?src=pr&el=footer). Last update [e3d4ecd...e53022a](https://codecov.io/gh/amzn/ion-go/pull/58?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

Armanbqt: Thanks @fernomac , I applied your suggestions and updated the PR.

I do not have a preference between merge versus  rebase (even though as you mentioned, rebase keeps the history cleaner).
Which ever we decide, I am fine with it. Any thoughts on this @zslayton ?
zslayton: > I do not have a preference between merge versus rebase (even though as you mentioned, rebase keeps the history cleaner).
> Which ever we decide, I am fine with it. Any thoughts on this @zslayton ?

My preference is to rebase while the PR is a draft and merge once the first review has happened. This means that reviewers will see a version of the code that was up-to-date with `master` when the (non-draft) PR was opened, but that the discussion around the code doesn't get blown away by rebasing after comments have been added.

In short, if you have merges in your commit history when you go to request your first review, rebase to clean it up. When the PR is approved, we'll merge from master and re-review if large changes were required. Finally, we'll squash+merge back into `master`.

Patch:
diff --git a/ion/tokenizer.go b/ion/tokenizer.go
index 3413de7..6b5888c 100644
--- a/ion/tokenizer.go
+++ b/ion/tokenizer.go
@@ -542,11 +542,12 @@ func (t *tokenizer) readString() (string, error) {
 		if err != nil {
 			return "", err
 		}
-
-		switch c {
-		case -1, '\n':
+		// -1 denotes EOF, and new lines are not allowed in short string
+		if c == -1 || c == '\n' || isProhibitedControlChar(c) {
 			return "", t.invalidChar(c)
+		}
 
+		switch c {
 		case '"':
 			return ret.String(), nil
 
@@ -582,12 +583,14 @@ func (t *tokenizer) readLongString() (string, error) {
 		if err != nil {
 			return "", err
 		}
-
-		switch c {
-		case -1:
+		// -1 denotes EOF
+		if c == -1 || isProhibitedControlChar(c) {
 			return "", t.invalidChar(c)
+		}
 
+		switch c {
 		case '\'':
+			startPosition := t.pos
 			ok, err := t.skipEndOfLongString(t.skipCommentsHandler)
 			if err != nil {
 				return "", err
@@ -595,7 +598,10 @@ func (t *tokenizer) readLongString() (string, error) {
 			if ok {
 				return ret.String(), nil
 			}
-
+			if startPosition == t.pos {
+				// No character has been consumed. It is single '.
+				ret.WriteByte(byte(c))
+			}
 		case '\\':
 			c, err = t.peek()
 			if err != nil {
@@ -1263,3 +1269,25 @@ func (t *tokenizer) unread(c int) {
 	t.pos--
 	t.buffer = append(t.buffer, c)
 }
+
+func isProhibitedControlChar(c int) bool {
+	// Values between 0 to 31 are non-displayable ASCII characters; except for new line and white space characters.
+	if c < 0x00 || c > 0x1F {
+		return false
+	}
+	if isStringWhitespace(c) || isNewLineChar(c) {
+		return false
+	}
+	return true
+}
+
+func isStringWhitespace(c int) bool {
+	return c == 0x09 || //horizontal tab
+		c == 0x0B || //vertical tab
+		c == 0x0C // form feed
+}
+
+func isNewLineChar(c int) bool {
+	return c == 0x0A || //new line
+		c == 0x0D //carriage return
+}


Test patch:
diff --git a/ion/integration_test.go b/ion/integration_test.go
index ceb7a6b..60f506d 100644
--- a/ion/integration_test.go
+++ b/ion/integration_test.go
@@ -194,7 +194,6 @@ var malformedIonsSkipList = []string{
 	"localSymbolTableWithMultipleSymbolsAndImportsFields.ion",
 	"localSymbolTableWithMultipleSymbolsFields.10n",
 	"localSymbolTableWithMultipleSymbolsFields.ion",
-	"longStringRawControlCharacter.ion",
 	"minLongWithLenTooLarge.10n",
 	"minLongWithLenTooSmall.10n",
 	"negativeIntZero.10n",
@@ -203,8 +202,6 @@ var malformedIonsSkipList = []string{
 	"nopPadWithAnnotations.10n",
 	"nullDotCommentInt.ion",
 	"sexpOperatorAnnotation.ion",
-	"stringLenTooLarge.10n",
-	"stringRawControlCharacter.ion",
 	"stringWithLatinEncoding.10n",
 	"structOrderedEmpty.10n",
 	"surrogate_1.ion",
@@ -225,7 +222,6 @@ var malformedIonsSkipList = []string{
 
 var equivsSkipList = []string{
 	"annotatedIvms.ion",
-	"clobs.ion",
 	"localSymbolTableAppend.ion",
 	"localSymbolTableNullSlots.ion",
 	"localSymbolTableWithAnnotations.ion",
@@ -234,7 +230,6 @@ var equivsSkipList = []string{
 	"nonIVMNoOps.ion",
 	"sexps.ion",
 	"stringUtf8.ion",
-	"strings.ion",
 	"structsFieldsDiffOrder.ion",
 	"structsFieldsRepeatedNames.ion",
 	"systemSymbols.ion",