Repository: DevoInc/python-sdk

Issue:
Lookup API is unable to send records with a double quote character in their content
Right now, the `Lookup.clean_field` method wraps the field's content in double quotes, unless it is a number:
https://github.com/DevoInc/python-sdk/blob/84a4e2c9da8d3013cd8f012e4399994d64817035/devo/sender/lookup.py#L400-L414

However, it does not check the existence of double quotes present in the field's value itself. This leads to a problem in which a code like this:

```python
lookup.send_headers(headers=['KEY', 'VALUE'], key='KEY', event='START', action="INC")
lookup.send_data_line(key="11", fields=["11", 'double quote: "'])
lookup.send_headers(headers=['KEY', 'VALUE'], key='KEY', event='END', action="INC")
```

Makes the update process fail silently, without any notice from the client's perspective.

I think we need to ensure that double quotes are escaped by replacing them like this:

```python
field.replace('"', '""')
```

Or at least inform in the documentation that double quotes need to be escaped with two double quotes.

Discussion:
pep8speaks: Hello @ErickLares! Thanks for opening this PR. We checked the lines you've touched for [PEP 8](https://www.python.org/dev/peps/pep-0008) issues, and found:

* In the file [`devo/sender/lookup.py`](https://github.com/DevoInc/python-sdk/blob/7e2d3ebaad985a2a6685936cccae053f67518d4c/devo/sender/lookup.py):

> [Line 96:80](https://github.com/DevoInc/python-sdk/blob/7e2d3ebaad985a2a6685936cccae053f67518d4c/devo/sender/lookup.py#L96): [E501](https://duckduckgo.com/?q=pep8%20E501) line too long (95 > 79 characters)


CLAassistant: [![CLA assistant check](https://cla-assistant.io/pull/badge/not_signed)](https://cla-assistant.io/DevoInc/python-sdk?pullRequest=113) <br/>Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our [Contributor License Agreement](https://cla-assistant.io/DevoInc/python-sdk?pullRequest=113) before we can accept your contribution.<br/><sub>You have signed the CLA already but the status is still pending? Let us [recheck](https://cla-assistant.io/check/DevoInc/python-sdk?pullRequest=113) it.</sub>

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 28e86d5..6686fd3 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -10,7 +10,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 * `Lookup.send_headers` method not working as expected with key_index parameter
 * Add `type_of_key` parameter to `Lookup.send_headers` method
 * Avoid lookups' key element to be deleted when `Lookup.send_data_line` is invoked.
-
+### Added
+ * Double quotes on lookups can be escaped by adding `"escape_quotes": true` to the config file.
 
 ## [3.4.2] - 2021-05-18
 ### Added
diff --git a/devo/sender/lookup.py b/devo/sender/lookup.py
index d92908e..db089e3 100644
--- a/devo/sender/lookup.py
+++ b/devo/sender/lookup.py
@@ -6,6 +6,7 @@ import sys
 import time
 
 
+
 def find_key_index(value=None, headers=None):
     """Find index of key value in a list"""
     for index, val in enumerate(headers):
@@ -43,7 +44,7 @@ class Lookup:
     CONTROL_TABLE = "my.lookup.control"
 
     def __init__(self, name="example", historic_tag=None,
-                 con=None, delay=5):
+                 con=None, delay=5, escape_quotes=False):
 
         if not re.match(r"^[A-Za-z0-9_]+$", name):
             raise Exception('Devo Lookup: The name of the lookup is incorrect,'
@@ -58,6 +59,7 @@ class Lookup:
         self.historic_tag = historic_tag
         self.name = name.replace(" ", "_")
         self.delay = delay
+        self.escape_quotes = escape_quotes
 
     # Helper methods
     # --------------------------------------------------------------------------
@@ -92,9 +94,11 @@ class Lookup:
         :return:
         """
         # TODO: Deprecate this if with list_to_fields in v4
-        p_fields = Lookup.list_to_fields(fields=fields, key=key) \
+        p_fields = Lookup.list_to_fields(fields=fields, key=key,
+                                         escape_quotes=self.escape_quotes) \
             if key_index is None \
-            else Lookup.process_fields(fields=fields, key_index=key_index)
+            else Lookup.process_fields(fields=fields, key_index=key_index,
+                                       escape_quotes=self.escape_quotes)
         self.send_data(row=p_fields, delete=delete)
 
     @staticmethod
@@ -206,8 +210,10 @@ class Lookup:
                 if delete_index is not None:
                     for fields in spam_reader:
                         field_action = fields.pop(delete_index)
-                        p_fields = Lookup.process_fields(fields=fields,
-                                                         key_index=key_index)
+                        p_fields = Lookup.process_fields(
+                            fields=fields,
+                            key_index=key_index,
+                            escape_quotes=self.escape_quotes)
                         self.send_data(row=p_fields,
                                        delete=field_action == "delete"
                                        or field_action == "DELETE")
@@ -218,8 +224,10 @@ class Lookup:
                         counter += 1
                 else:
                     for fields in spam_reader:
-                        p_fields = Lookup.process_fields(fields=fields,
-                                                         key_index=key_index)
+                        p_fields = Lookup.process_fields(
+                            fields=fields,
+                            key_index=key_index,
+                            escape_quotes=self.escape_quotes)
                         self.send_data(row=p_fields)
 
                         # Send full log for historic
@@ -351,16 +359,16 @@ class Lookup:
         return out
 
     @staticmethod
-    def field_to_str(field):
+    def field_to_str(field, escape_quotes=False):
         """
         Convert one value to STR, cleaning it
         :param field: field to clean
         :return:
         """
-        return ',%s' % Lookup.clean_field(field)
+        return ',%s' % Lookup.clean_field(field, escape_quotes)
 
     @staticmethod
-    def process_fields(fields=None, key_index=None):
+    def process_fields(fields=None, key_index=None, escape_quotes=False):
         """
         Method to convert list with one row/fields to STR to send
         :param fields: fields list
@@ -368,23 +376,23 @@ class Lookup:
         :return:
         """
         # First the key
-        out = '%s' % Lookup.clean_field(fields[key_index])
+        out = '%s' % Lookup.clean_field(fields[key_index], escape_quotes)
 
         # The rest of the fields
         for item in fields[:key_index] + fields[key_index + 1:]:
-            out += Lookup.field_to_str(item)
+            out += Lookup.field_to_str(item, escape_quotes)
         return out
 
     # TODO: Deprecated
     @staticmethod
-    def list_to_fields(fields=None, key="key"):
+    def list_to_fields(fields=None, key="key", escape_quotes=False):
         """
         Transform list item to the object we need send to Devo for each row
         :param list fields: list of field names
         :param str key: key name, optional
         :result str
         """
-        key = Lookup.clean_field(key)
+        key = Lookup.clean_field(key, escape_quotes)
         # First the key
         out = '%s' % key
 
@@ -393,7 +401,7 @@ class Lookup:
 
         # The rest of the fields
         for item in fields:
-            item = Lookup.clean_field(item)
+            item = Lookup.clean_field(item, escape_quotes)
             # If file is the key don't add
             if item == key:
                 continue
@@ -401,7 +409,7 @@ class Lookup:
         return out
 
     @staticmethod
-    def clean_field(field=None):
+    def clean_field(field=None, escape_quotes=False):
         """
         Strip and quotechar the fields
         :param str field: field for clean
@@ -414,6 +422,9 @@ class Lookup:
         if Lookup.is_number(field):
             return field
 
+        if escape_quotes:
+            field = field.replace('"', '""')
+
         return '"%s"' % field
 
     @staticmethod
diff --git a/docs/sender/lookup.md b/docs/sender/lookup.md
index 3cde900..04105a4 100644
--- a/docs/sender/lookup.md
+++ b/docs/sender/lookup.md
@@ -18,13 +18,15 @@ object into the _lookup_ variable with the new parameters or add in the CLI flag
 Example:
 
 ```
-{   sender": {
+{   
+    sender": {
         ...
     },
     "lookup": {
         "name": "Test_Lookup_of_180306_02",
         "file": "test_lookup.csv",
         "lkey": "KEY"
+        "escape_quotes": true
     }
 }
 ```
@@ -225,3 +227,24 @@ You can use config file to send types list:
     ....
  }}
 ```
+##### Double quotes in the field value
+
+Any double quotes inside the field value can cause trouble when sending the lookup if this is not escaped.
+An example of this case can be seen below:
+
+`lookup.send_data_line(key="11", fields=["11", 'double quotes must escaped"'])`
+
+That lookup creation will fail because that double quote will be interpreted as a field termination and 
+the number of fields for that row will unmatch the corresponding number of columns. To avoid this, 
+add `"escape_quotes": true` to the lookup configuration file and `escape_quotes=True` to the `Lookup` constructor. 
+Below, an example for the constructor is shown:
+
+```
+`lookup = Lookup(name=self.lookup_name, historic_tag=None, con=con, escape_quotes=True)`
+```
+
+To see an example for the lookup configuration please refer to the one shown at the start of the document.
+
+This will escape ALL double quotes in the field value by adding a second double quote to it.
+
+The default behavior is to NOT escape any double quotes. 
\ No newline at end of file


Test patch:
diff --git a/tests/sender/send_lookup.py b/tests/sender/send_lookup.py
index 4e7e416..a0937a8 100644
--- a/tests/sender/send_lookup.py
+++ b/tests/sender/send_lookup.py
@@ -243,6 +243,96 @@ class TestLookup(unittest.TestCase):
         self.assertEqual(fields, ["a", "b", "c"])
         self.assertEqual(processed_fields, '"b","a","c"')
 
+    # Clean field
+    def test_clean_field_parametrized(self):
+        test_params = [
+            ("No double quotes", False, '"No double quotes"'),
+            ("No double quotes", True, '"No double quotes"'),
+            ('Double quotes"', False, '"Double quotes""'),
+            ('Double quotes"', True, '"Double quotes"""')
+        ]
+        for field, escape_quotes, expected_result in test_params:
+            with self.subTest(
+                field=field,
+                escape_quotes=escape_quotes,
+                expected_result=expected_result
+            ):
+                result = Lookup.clean_field(field, escape_quotes)
+                self.assertEqual(result, expected_result)
+
+    # Test to make sure escape_quotes is propagated correctly
+    def test_escape_quotes_in_send_data_line_key(self):
+        engine_config = SenderConfigSSL(
+            address=(self.server, self.port),
+            key=self.key,
+            cert=self.cert,
+        )
+        con = Sender(engine_config)
+
+        lookup = Lookup(name=self.lookup_name, historic_tag=None, con=con,
+                        escape_quotes=True)
+
+        with mock.patch.object(Lookup, 'clean_field',
+                               wraps=Lookup.clean_field) as clean_field:
+            lookup.send_data_line(key="11", fields=["11", 'Double quotes"'])
+            clean_field.assert_called_with('Double quotes"', True)
+
+    # Test to make sure escape_quotes is propagated correctly
+    def test_escape_quotes_in_send_data_line(self):
+        engine_config = SenderConfigSSL(
+            address=(self.server, self.port),
+            key=self.key,
+            cert=self.cert,
+        )
+        con = Sender(engine_config)
+
+        lookup = Lookup(name=self.lookup_name, historic_tag=None, con=con,
+                        escape_quotes=True)
+
+        with mock.patch.object(Lookup, 'clean_field',
+                               wraps=Lookup.clean_field) as clean_field:
+            lookup.send_data_line(fields=["11", 'Double quotes"'])
+            clean_field.assert_called_with('Double quotes"', True)
+
+            # Test to make sure escape_quotes is propagated correctly
+
+    def test_escape_quotes_in_send_csv(self):
+        engine_config = SenderConfigSSL(
+            address=(self.server, self.port),
+            key=self.key,
+            cert=self.cert,
+        )
+        con = Sender(engine_config)
+
+        lookup = Lookup(name=self.lookup_name, historic_tag=None, con=con,
+                        escape_quotes=True)
+
+        with mock.patch.object(Lookup, 'clean_field',
+                               wraps=Lookup.clean_field) as clean_field:
+            lookup.send_csv(path=self.lookup_file,
+                            has_header=True,
+                            key=self.lookup_key)
+            clean_field.assert_called_with('ffffff', True)
+
+            # Test to make sure escape_quotes is propagated correctly
+
+    def test_escape_quotes_in_send_csv_delete_index(self):
+        engine_config = SenderConfigSSL(
+            address=(self.server, self.port),
+            key=self.key,
+            cert=self.cert,
+        )
+        con = Sender(engine_config)
+
+        lookup = Lookup(name=self.lookup_name, historic_tag=None, con=con,
+                        escape_quotes=True)
+
+        with mock.patch.object(Lookup, 'clean_field',
+                               wraps=Lookup.clean_field) as clean_field:
+            lookup.send_csv(path=self.lookup_file,
+                            has_header=True,
+                            key=self.lookup_key, delete_field="Green")
+            clean_field.assert_called_with('ffffff', True)
 
 if __name__ == "__main__":
     unittest.main()