Repository: garcia/simfile

Issue:
Multi-value properties aren't handled correctly in SSC
Two bugs in **simfile** 2.0.0–2.1.0's SSC implementation break multi-value properties, causing them to be truncated or mangled past the first value:

1. When opening an SSC file, the `DISPLAYBPM` and `ATTACKS` properties of both simfiles and charts stop parsing at the first `:`. For `DISPLAYBPM`, this means a BPM range of `min:max` will be incorrectly parsed as a static BPM of `min`. `ATTACKS` are completely broken as they use colon as a separator.
2. The aforementioned properties are incorrectly serialized with colons escaped from `SSCChart`. This has the same effects described above, but this only affects values with colons that were written to the chart object manually (e.g. `chart.displaybpm = "120:240"`) since the first bug shadows this bug during deserialization.

Discussion:

Patch:
diff --git a/simfile/__init__.py b/simfile/__init__.py
index 37dc0ea..431cd21 100644
--- a/simfile/__init__.py
+++ b/simfile/__init__.py
@@ -22,7 +22,7 @@ from .sm import SMSimfile
 from .types import Simfile
 
 
-__version__ = "2.1.0"
+__version__ = "2.1.1"
 __all__ = [
     "load",
     "loads",
diff --git a/simfile/ssc.py b/simfile/ssc.py
index 7b3d5e0..400e4e6 100644
--- a/simfile/ssc.py
+++ b/simfile/ssc.py
@@ -99,20 +99,28 @@ class SSCChart(BaseChart):
             raise ValueError("expected NOTEDATA property first")
 
         for param in iterator:
-            self[param.key] = param.value
+            if param.key in BaseSimfile.MULTI_VALUE_PROPERTIES:
+                self[param.key] = ":".join(param.components[1:])
+            else:
+                self[param.key] = param.value
             if param.value is self.notes:
                 break
 
     def serialize(self, file):
         file.write(f"{MSDParameter(('NOTEDATA', ''))}\n")
         notes_key = "NOTES"
+
         for (key, value) in self.items():
-            # notes must always be the last property in a chart
+            # Either NOTES or NOTES2 must be the last chart property
             if value is self.notes:
                 notes_key = key
                 continue
-            param = MSDParameter((key, value))
+            if key in BaseSimfile.MULTI_VALUE_PROPERTIES:
+                param = MSDParameter((key, *value.split(":")))
+            else:
+                param = MSDParameter((key, value))
             file.write(f"{param}\n")
+
         notes_param = MSDParameter((notes_key, self[notes_key]))
         file.write(f"{notes_param}\n\n")
 
@@ -208,7 +216,7 @@ class SSCSimfile(BaseSimfile):
         partial_chart: Optional[SSCChart] = None
         for param in parser:
             key = param.key.upper()
-            if key not in BaseSimfile.MULTI_VALUE_PROPERTIES:
+            if key in BaseSimfile.MULTI_VALUE_PROPERTIES:
                 value: Optional[str] = ":".join(param.components[1:])
             else:
                 value = param.value


Test patch:
diff --git a/simfile/tests/test_sm.py b/simfile/tests/test_sm.py
index 8461af0..87929e9 100644
--- a/simfile/tests/test_sm.py
+++ b/simfile/tests/test_sm.py
@@ -159,6 +159,20 @@ class TestSMSimfile(unittest.TestCase):
         self.assertEqual(with_bgchanges.bgchanges, with_animations.bgchanges)
         self.assertNotIn("BGCHANGES", with_animations)
         self.assertIn("ANIMATIONS", with_animations)
+    
+    def test_init_handles_multi_value_properties(self):
+        with_multi_value_properties = SMSimfile(string='''
+            #TITLE:Colons should be preserved below: but not here;
+            #DISPLAYBPM:60:240;
+            #ATTACKS:TIME=1.000:LEN=0.500:MODS=*5 -2.5 reverse;
+        ''')
+        self.assertEqual('Colons should be preserved below', with_multi_value_properties.title)
+        self.assertEqual('60:240', with_multi_value_properties.displaybpm)
+        self.assertEqual(
+            'TIME=1.000:LEN=0.500:MODS=*5 -2.5 reverse',
+            with_multi_value_properties.attacks,
+        )
+        
 
     def test_repr(self):
         unit = SMSimfile(string=testing_simfile())
@@ -198,3 +212,19 @@ class TestSMSimfile(unittest.TestCase):
 
         unit.charts = SMCharts()
         self.assertEqual(0, len(unit.charts))
+
+    def test_serialize_handles_multi_value_properties(self):
+        expected = SMSimfile(string='''
+            #TITLE:Colons should be preserved below;
+            #DISPLAYBPM:60:240;
+            #ATTACKS:TIME=1.000:LEN=0.500:MODS=*5 -2.5 reverse;
+        ''')
+        
+        # None of the colons should be escaped
+        serialized = str(expected)
+        self.assertNotIn('\\', serialized)
+        
+        deserialized = SMSimfile(string=serialized)
+        self.assertEqual(expected.title, deserialized.title)
+        self.assertEqual(expected.displaybpm, deserialized.displaybpm)
+        self.assertEqual(expected.attacks, deserialized.attacks)
\ No newline at end of file
diff --git a/simfile/tests/test_ssc.py b/simfile/tests/test_ssc.py
index bde7305..8402a54 100644
--- a/simfile/tests/test_ssc.py
+++ b/simfile/tests/test_ssc.py
@@ -59,6 +59,29 @@ class TestSSCChart(unittest.TestCase):
         self.assertEqual("0.793,1.205,0.500,0.298,0.961", unit.radarvalues)
         self.assertEqual("\n\n0000\n0000\n0000\n0000\n", unit.notes)
 
+    def test_init_handles_multi_value_properties(self):
+        with_multi_value_properties = SSCChart.from_str(
+            """
+            #NOTEDATA:;
+            #CHARTNAME:Colons should be preserved below: but not here;
+            #DISPLAYBPM:60:240;
+            #ATTACKS:TIME=1.000:LEN=0.500:MODS=*5 -2.5 reverse;
+            #NOTES:
+                0000
+                0000
+                0000
+                0000
+            ;"""
+        )
+        self.assertEqual(
+            "Colons should be preserved below", with_multi_value_properties.chartname
+        )
+        self.assertEqual("60:240", with_multi_value_properties.displaybpm)
+        self.assertEqual(
+            "TIME=1.000:LEN=0.500:MODS=*5 -2.5 reverse",
+            with_multi_value_properties.attacks,
+        )
+
     def test_serialize(self):
         unit = SSCChart.from_str(testing_chart())
         expected = (
@@ -81,6 +104,31 @@ class TestSSCChart(unittest.TestCase):
             "\n"
         )
 
+    def test_serialize_handles_multi_value_properties(self):
+        expected = SSCChart.from_str(
+            """
+            #NOTEDATA:;
+            #CHARTNAME:Colons in values below should be preserved;
+            #DISPLAYBPM:60:240;
+            #ATTACKS:TIME=1.000:LEN=0.500:MODS=*5 -2.5 reverse;
+            #NOTES:
+                0000
+                0000
+                0000
+                0000
+            ;"""
+        )
+        serialized = SSCChart.from_str(str(expected))
+
+        # None of the colons should be escaped
+        serialized = str(expected)
+        self.assertNotIn("\\", serialized)
+
+        deserialized = SSCChart.from_str(str(serialized))
+        self.assertEqual(expected.chartname, deserialized.chartname)
+        self.assertEqual(expected.displaybpm, deserialized.displaybpm)
+        self.assertEqual(expected.attacks, deserialized.attacks)
+
     def test_serialize_with_escapes(self):
         unit = SSCChart.from_str(testing_chart())
         unit.chartname = "A:B;C//D\\E"
@@ -194,6 +242,43 @@ class TestSSCSimfile(unittest.TestCase):
         self.assertNotIn("BGCHANGES", with_animations)
         self.assertIn("ANIMATIONS", with_animations)
 
+    def test_init_handles_multi_value_properties(self):
+        with_multi_value_properties = SSCSimfile(
+            string="""
+            #VERSION:0.83;
+            #TITLE:Colons should be preserved below: but not here;
+            #DISPLAYBPM:60:240;
+            #ATTACKS:TIME=1.000:LEN=0.500:MODS=*5 -2.5 reverse;
+        """
+        )
+        self.assertEqual(
+            "Colons should be preserved below", with_multi_value_properties.title
+        )
+        self.assertEqual("60:240", with_multi_value_properties.displaybpm)
+        self.assertEqual(
+            "TIME=1.000:LEN=0.500:MODS=*5 -2.5 reverse",
+            with_multi_value_properties.attacks,
+        )
+
+    def test_serialize_handles_multi_value_properties(self):
+        expected = SSCSimfile(
+            string="""
+            #VERSION:0.83;
+            #TITLE:Colons should be preserved below;
+            #DISPLAYBPM:60:240;
+            #ATTACKS:TIME=1.000:LEN=0.500:MODS=*5 -2.5 reverse;
+        """
+        )
+
+        # None of the colons should be escaped
+        serialized = str(expected)
+        self.assertNotIn("\\", serialized)
+
+        deserialized = SSCSimfile(string=serialized)
+        self.assertEqual(expected.title, deserialized.title)
+        self.assertEqual(expected.displaybpm, deserialized.displaybpm)
+        self.assertEqual(expected.attacks, deserialized.attacks)
+
     def test_repr(self):
         unit = SSCSimfile(string=testing_simfile())