internal/ini: Fix ini parser to handle empty values (#406)

The ini parser incorrectly decided whether a statement should be skipped. As a result, valid statements in the ini files were being squashed. The PR fixes incorrect modifications to the previous token value of the skipper. We also add checks for cases where a skipped statement should be marked as complete and not be ignored.

Adds test cases for cases for statements that need to be skipped. Also adds suggested tests from aws/aws-sdk-go#2801 .

Author: skotambkar

CHANGELOG_PENDING.md

@@ -40,3 +40,6 @@ SDK Bugs
 ---
 * `service/s3/s3manager`: Fix index out of range when a streaming reader returns -1 ([#378](https://github.com/aws/aws-sdk-go-v2/pull/378))
   * Fixes the S3 Upload Manager's handling of an unbounded streaming reader that returns negative bytes read.
+* `internal/ini`: Fix ini parser to handle empty values [#406](https://github.com/aws/aws-sdk-go-v2/pull/406)
+  * Fixes incorrect modifications to the previous token value of the skipper. Adds checks for cases where a skipped statement should be marked as complete and not be ignored.
+  * Adds tests for nested and empty field value parsing, along with tests suggested in [#2801](https://github.com/aws/aws-sdk-go/pull/2801)

internal/ini/ini_parser.go

@@ -162,7 +162,7 @@ loop:
 			if len(tokens) == 0 {
 				break loop
 			}
-
+			// if should skip is true, we skip the tokens until should skip is set to false.
 			step = SkipTokenState
 		}
 
@@ -218,7 +218,7 @@ loop:
 			// S -> equal_expr' expr_stmt'
 			switch k.Kind {
 			case ASTKindEqualExpr:
-				// assiging a value to some key
+				// assigning a value to some key
 				k.AppendChild(newExpression(tok))
 				stack.Push(newExprStatement(k))
 			case ASTKindExpr:
@@ -250,6 +250,13 @@ loop:
 			if !runeCompare(tok.Raw(), openBrace) {
 				return nil, NewParseError("expected '['")
 			}
+			// If OpenScopeState is not at the start, we must mark the previous ast as complete
+			//
+			// for example: if previous ast was a skip statement;
+			// we should mark it as complete before we create a new statement
+			if k.Kind != ASTKindStart {
+				stack.MarkComplete(k)
+			}
 
 			stmt := newStatement()
 			stack.Push(stmt)

internal/ini/ini_parser_test.go

@@ -268,6 +268,25 @@ region = us-west-2
 				newExprStatement(noQuotesRegionEQRegion),
 			},
 		},
+		{
+			name: "missing section statement",
+			r: bytes.NewBuffer([]byte(
+				`[default]
+s3 =
+[assumerole]
+output = json
+				`)),
+			expectedStack: []AST{
+				newCompletedSectionStatement(
+					defaultProfileStmt,
+				),
+				newSkipStatement(newEqualExpr(newExpression(s3ID), equalOp)),
+				newCompletedSectionStatement(
+					assumeProfileStmt,
+				),
+				newExprStatement(outputEQExpr),
+			},
+		},
 	}
 
 	for i, c := range cases {

internal/ini/skipper.go

@@ -22,21 +22,21 @@ func newSkipper() skipper {
 }
 
 func (s *skipper) ShouldSkip(tok Token) bool {
+	// should skip state will be modified only if previous token was new line (NL);
+	// and the current token is not WhiteSpace (WS).
 	if s.shouldSkip &&
 		s.prevTok.Type() == TokenNL &&
 		tok.Type() != TokenWS {
-
 		s.Continue()
 		return false
 	}
-	s.prevTok = tok
 
+	s.prevTok = tok
 	return s.shouldSkip
 }
 
 func (s *skipper) Skip() {
 	s.shouldSkip = true
-	s.prevTok = emptyToken
 }
 
 func (s *skipper) Continue() {

internal/ini/testdata/valid/empty_values

@@ -0,0 +1,12 @@
+[foo]
+aws_access_key_id =
+aws_secret_access_key =
+aws_session_token =
+[bar]
+aws_access_key_id = valid
+aws_secret_access_key = valid
+aws_session_token = valid
+[baz]
+aws_access_key_id = valid
+aws_secret_access_key = valid
+aws_session_token = valid

internal/ini/testdata/valid/empty_values_expected

@@ -0,0 +1,14 @@
+{
+	"foo": {
+	},
+	"bar": {
+		"aws_access_key_id": "valid",
+		"aws_secret_access_key": "valid",
+		"aws_session_token": "valid"
+	},
+	"baz": {
+		"aws_access_key_id": "valid",
+		"aws_secret_access_key": "valid",
+		"aws_session_token": "valid"
+	}
+}

internal/ini/testdata/valid/nested_fields

@@ -0,0 +1,12 @@
+[foo]
+aws_access_key_id =
+	aws_secret_access_key = valid
+aws_session_token = valid
+[bar]
+aws_access_key_id = valid
+aws_secret_access_key = valid
+aws_session_token = valid
+[baz]
+aws_access_key_id = valid
+aws_secret_access_key = valid
+aws_session_token = valid

internal/ini/testdata/valid/nested_fields_expected

@@ -0,0 +1,15 @@
+{
+	"foo": {
+		"aws_session_token": "valid"
+	},
+	"bar": {
+		"aws_access_key_id": "valid",
+		"aws_secret_access_key": "valid",
+		"aws_session_token": "valid"
+	},
+	"baz": {
+		"aws_access_key_id": "valid",
+		"aws_secret_access_key": "valid",
+		"aws_session_token": "valid"
+	}
+}

internal/ini/walker_test.go

@@ -63,24 +63,24 @@ func TestValidDataFiles(t *testing.T) {
 				case string:
 					a := p.String(k)
 					if e != a {
-						t.Errorf("%s: expected %v, but received %v", path, e, a)
+						t.Errorf("%s: expected %v, but received %v for profile %v", path, e, a, profile)
 					}
 				case int:
 					a := p.Int(k)
 					if int64(e) != a {
-						t.Errorf("%s: expected %v, but received %v", path, e, a)
+						t.Errorf("%s: expected %v, but received %v for profile %v", path, e, a, profile)
 					}
 				case float64:
 					v := p.values[k]
 					if v.Type == IntegerType {
 						a := p.Int(k)
 						if int64(e) != a {
-							t.Errorf("%s: expected %v, but received %v", path, e, a)
+							t.Errorf("%s: expected %v, but received %v for profile %v", path, e, a, profile)
 						}
 					} else {
 						a := p.Float64(k)
 						if e != a {
-							t.Errorf("%s: expected %v, but received %v", path, e, a)
+							t.Errorf("%s: expected %v, but received %v for profile %v", path, e, a, profile)
 						}
 					}
 				default: