From 95ba167979504772b2615f52268bb420b82183e5 Mon Sep 17 00:00:00 2001
From: Paolo Sottovia <paolo.sottovia@canonical.com>
Date: Mon, 7 Aug 2023 07:59:53 +0200
Subject: [PATCH] Fix parsing of empty lines in config files

---
 spark8t/domain.py             |  3 ++
 spark8t/services.py           |  9 ++++-
 tests/unittest/test_domain.py | 69 ++++++++++++++++++++++++++---------
 3 files changed, 63 insertions(+), 18 deletions(-)

diff --git a/spark8t/domain.py b/spark8t/domain.py
index cc4f5b7..050016e 100644
--- a/spark8t/domain.py
+++ b/spark8t/domain.py
@@ -53,6 +53,9 @@ def _read_property_file_unsafe(cls, name: str) -> Dict:
         defaults = dict()
         with open(name) as f:
             for line in f:
+                # skip empty line
+                if len(line.strip()) == 0:
+                    continue
                 key, value = cls.parse_property_line(line)
                 defaults[key] = os.path.expandvars(value)
         return defaults
diff --git a/spark8t/services.py b/spark8t/services.py
index 044554c..246fd36 100644
--- a/spark8t/services.py
+++ b/spark8t/services.py
@@ -359,6 +359,9 @@ def get_service_accounts(
         labels_to_pass = dict()
         if labels:
             for entry in labels:
+                # skip empty label
+                if len(entry.strip()) == 0:
+                    continue
                 k, v = PropertyFile.parse_property_line(entry)
                 labels_to_pass[k] = v
 
@@ -1459,7 +1462,11 @@ def _generate_properties_file_from_arguments(confs: List[str]):
             return PropertyFile({})
 
         return PropertyFile(
-            dict(PropertyFile.parse_property_line(line) for line in confs)
+            dict(
+                PropertyFile.parse_property_line(line)
+                for line in confs
+                if len(line.strip()) != 0
+            )
         )
 
     def spark_submit(
diff --git a/tests/unittest/test_domain.py b/tests/unittest/test_domain.py
index f248ee4..bd0387d 100644
--- a/tests/unittest/test_domain.py
+++ b/tests/unittest/test_domain.py
@@ -1,4 +1,5 @@
 import logging
+import tempfile
 import uuid
 
 from spark8t.domain import Defaults, PropertyFile, ServiceAccount
@@ -72,36 +73,70 @@ def test_service_account():
     assert sa.configurations.props.get("spark.dummy.property1") == spark_dummy_property1
     assert sa.configurations.props.get("spark.dummy.property2") == spark_dummy_property2
 
-    def test_property_removing_conf(self):
-        confs = ["key1=value1", "key2=value2", "key3=value3"]
 
-        prop = PropertyFile(
-            dict(PropertyFile.parse_property_line(line) for line in confs)
-        )
+def test_property_removing_conf():
+    """
+    Validates removal of configuration options.
+    """
+    confs = ["key1=value1", "key2=value2", "key3=value3"]
 
-        self.assertFalse("key1" in prop.remove(["key1"]).props)
+    prop = PropertyFile(dict(PropertyFile.parse_property_line(line) for line in confs))
 
-        self.assertTrue("key3" in prop.remove(["key1", "key2"]).props)
+    assert "key1" not in prop.remove(["key1"]).props
 
-        self.assertDictEqual(prop.props, prop.remove([]).props)
+    assert "key3" in prop.remove(["key1", "key2"]).props
 
-    def test_property_removing_conf_with_pairs(self):
-        confs = ["key1=value1", "key2=value2", "key3=value3"]
+    assert prop.props == prop.remove([]).props
 
-        prop = PropertyFile(
-            dict(PropertyFile.parse_property_line(line) for line in confs)
-        )
 
-        self.assertFalse("key1" in prop.remove(["key1=value1"]).props)
+def test_property_removing_conf_with_pairs():
+    """
+    Validates the correct removal of property pairs.
+    """
+    confs = ["key1=value1", "key2=value2", "key3=value3"]
+
+    prop = PropertyFile(dict(PropertyFile.parse_property_line(line) for line in confs))
 
-        self.assertTrue("key1" in prop.remove(["key1=value2"]).props)
+    assert "key1" not in prop.remove(["key1=value1"]).props
 
-        self.assertFalse("key1" in prop.remove(["key1=value2", "key1=value1"]).props)
+    assert "key1" in prop.remove(["key1=value2"]).props
 
-        self.assertFalse("key1" in prop.remove(["key1", "key1=value2"]).props)
+    assert "key1" not in prop.remove(["key1=value2", "key1=value1"]).props
+
+    assert "key1" not in prop.remove(["key1", "key1=value2"]).props
+
+
+def test_property_empty_lines():
+    """
+    Validates that empty lines are skipped and configuration is parsed correctly.
+    """
+    confs = ["key1=value1", "", "key2=value2", "key3=value3", ""]
+
+    with tempfile.NamedTemporaryFile(mode="w+t") as f:
+        # write conf file
+        for conf in confs:
+            f.write(f"{conf}\n")
+        f.flush()
+        
+        with open(f.name, 'r') as fp:
+            assert len(fp.readlines()) == 5
+        
+        # read property file from temporary file name
+        prop = PropertyFile.read(f.name)
+
+        assert "key1" not in prop.remove(["key1=value1"]).props
+
+        assert "key1" in prop.remove(["key1=value2"]).props
+
+        assert "key1" not in prop.remove(["key1=value2", "key1=value1"]).props
+
+        assert "key1" not in prop.remove(["key1", "key1=value2"]).props
 
 
 def test_property_file_parsing_from_confs():
+    """
+    Validates parsing of configuration from list.
+    """
     confs = ["key1=value1", "key2=value2"]
 
     prop = PropertyFile(dict(PropertyFile.parse_property_line(line) for line in confs))