From aa94e7e680fce2d289ed6601ce77febb57cea4bf Mon Sep 17 00:00:00 2001
From: Nadav Har'El <nyh@scylladb.com>
Date: Mon, 24 Jun 2019 14:54:21 +0300
Subject: [PATCH] alternator: clean up parsing of attribute-path components

Before this patch, we read either an attribute name like "name" or
a reference to one "#name", as one type of token - NAME.
However, while attribute paths indeed can use either one, in some other
contexts - such as a function name - only "name" is allowed, so we
need to distinguish between two types of tokens: NAME and NAMEREF.

While separating those, I noticed that we incorrectly allowed a "#"
followed by *zero* alphanumeric characters to be considered a NAMEREF,
which it shouldn't. In other words, NAMEREF should have ALNUM+, not ALNUM*.
Same for VALREF, which can't be just a ":" with nothing after it.
So this patch fixes these mistakes, and adds tests for them.

Signed-off-by: Nadav Har'El <nyh@scylladb.com>
---
 alternator-test/test_update_expression.py | 13 +++++++++++++
 alternator/expressions.g                  | 12 +++++++-----
 2 files changed, 20 insertions(+), 5 deletions(-)

diff --git a/alternator-test/test_update_expression.py b/alternator-test/test_update_expression.py
index 8315762d21..cf290417cd 100644
--- a/alternator-test/test_update_expression.py
+++ b/alternator-test/test_update_expression.py
@@ -140,6 +140,14 @@ def test_update_expression_name_token(test_table_s):
         test_table_s.update_item(Key={'p': p}, UpdateExpression='SET #hi-there = :val1', ExpressionAttributeValues={':val1': 7}, ExpressionAttributeNames={'#hi-there': silly_name})
     with pytest.raises(ClientError, match='ValidationException'):
         test_table_s.update_item(Key={'p': p}, UpdateExpression='SET #!hi = :val1', ExpressionAttributeValues={':val1': 7}, ExpressionAttributeNames={'#!hi': silly_name})
+    # Just a "#" is not enough as a token. Interestingly, DynamoDB will
+    # find the bad name in ExpressionAttributeNames before it actually tries
+    # to parse UpdateExpression, but we can verify the parse fails too by
+    # using a valid but irrelevant name in ExpressionAttributeNames:
+    with pytest.raises(ClientError, match='ValidationException'):
+        test_table_s.update_item(Key={'p': p}, UpdateExpression='SET # = :val1', ExpressionAttributeValues={':val1': 7}, ExpressionAttributeNames={'#': silly_name})
+    with pytest.raises(ClientError, match='ValidationException'):
+        test_table_s.update_item(Key={'p': p}, UpdateExpression='SET # = :val1', ExpressionAttributeValues={':val1': 7}, ExpressionAttributeNames={'#a': silly_name})
 
     # There is also the value references, ":reference", for the right-hand
     # side of an assignment. These have similar naming rules like "#reference".
@@ -153,6 +161,11 @@ def test_update_expression_name_token(test_table_s):
     assert test_table_s.get_item(Key={'p': p}, ConsistentRead=True)['Item']['a'] == 11
     with pytest.raises(ClientError, match='ValidationException'):
         test_table_s.update_item(Key={'p': p}, UpdateExpression='SET a = :hi!there', ExpressionAttributeValues={':hi!there': 12})
+    # Just a ":" is not enough as a token.
+    with pytest.raises(ClientError, match='ValidationException'):
+        test_table_s.update_item(Key={'p': p}, UpdateExpression='SET a = :', ExpressionAttributeValues={':': 7})
+    with pytest.raises(ClientError, match='ValidationException'):
+        test_table_s.update_item(Key={'p': p}, UpdateExpression='SET a = :', ExpressionAttributeValues={':a': 7})
     # Trying to use a :reference on the left-hand side of an assignment will
     # not work. In DynamoDB, it's a different type of token (and generates
     # syntax error).
diff --git a/alternator/expressions.g b/alternator/expressions.g
index 2c79c7d235..e9e65e824d 100644
--- a/alternator/expressions.g
+++ b/alternator/expressions.g
@@ -132,18 +132,20 @@ fragment ALPHA: 'A'..'Z' | 'a'..'z';
 fragment DIGIT: '0'..'9';
 fragment ALNUM: ALPHA | DIGIT | '_';
 INTEGER: DIGIT+;
-NAME: (ALPHA | '#') ALNUM*;
-VALREF: ':' ALNUM*;
+NAME: ALPHA ALNUM*;
+NAMEREF: '#' ALNUM+;
+VALREF: ':' ALNUM+;
 
 /*
  * Parsing phase - parsing the string of tokens generated by the lexical
  * analyzer defined above.
  */
 
+path_component: NAME | NAMEREF;
 path returns [parsed::path p]:
-    root=NAME { $p.set_root($root.text); }
-    (   '.' name=NAME     { $p.add_dot($name.text); }
-      | '[' INTEGER ']'   { $p.add_index(std::stoi($INTEGER.text)); }
+    root=path_component           { $p.set_root($root.text); }
+    (   '.' name=path_component   { $p.add_dot($name.text); }
+      | '[' INTEGER ']'           { $p.add_index(std::stoi($INTEGER.text)); }
     )*;
 
 // FIXME: set action supports additional types of rhs besides VALREF.