fix: consider value type when escaping and unescaping

this fixes #104 (introduced in 0.16.2) where URL properties got unnecessarily escaped
hoodie · Aug 13, 2024 · fb07391 · fb07391
1 parent a57ce3e
commit fb07391
Show file tree

Hide file tree

Showing 7 changed files with 291 additions and 100 deletions.
diff --git a/src/components/alarm.rs b/src/components/alarm.rs
@@ -25,7 +25,7 @@ use super::*;
 /// When it is time for the Alarm to occur we have to define what is actually supposed to happen.
 /// The RFC5545 know three different actions, two of which are currently implemented.
 ///
-/// 1. Display  
+/// 1. Display
 /// 2. Audio
 /// 3. Email (not yet implemented)
 ///

diff --git a/src/components/date_time.rs b/src/components/date_time.rs
@@ -1,7 +1,6 @@
-#![allow(dead_code, unused)]
 use std::str::FromStr;
 
-use chrono::{DateTime, Duration, NaiveDate, NaiveDateTime, Offset, TimeZone as _, Utc};
+use chrono::{DateTime, Duration, NaiveDate, NaiveDateTime, TimeZone as _, Utc};
 
 use crate::{Property, ValueType};
 

diff --git a/src/lib.rs b/src/lib.rs
@@ -100,6 +100,7 @@ mod components;
 #[cfg(feature = "parser")]
 pub mod parser;
 mod properties;
+mod value_types;
 
 pub use crate::{
     calendar::{Calendar, CalendarComponent},
@@ -108,7 +109,8 @@ pub use crate::{
         date_time::{CalendarDateTime, DatePerhapsTime},
         Component, Event, EventLike, Todo, Venue,
     },
-    properties::{Class, EventStatus, Parameter, Property, TodoStatus, ValueType},
+    properties::{Class, EventStatus, Parameter, Property, TodoStatus},
+    value_types::ValueType,
 };
 
 #[cfg(feature = "chrono-tz")]

diff --git a/src/parser/parsed_string.rs b/src/parser/parsed_string.rs
@@ -1,5 +1,7 @@
 use std::borrow::Cow;
 
+use crate::ValueType;
+
 /// A zero-copy string parsed from an iCal input.
 #[derive(Debug, Eq, Clone)]
 #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))]
@@ -26,6 +28,13 @@ impl ParseString<'_> {
 }
 
 impl<'a> ParseString<'a> {
+    pub fn unescape_by_value_type(self, value_type: ValueType) -> ParseString<'a> {
+        match value_type {
+            ValueType::Text => self.unescape_text(),
+            _ => self,
+        }
+    }
+
     pub fn unescape_text(self) -> ParseString<'a> {
         if self.0.contains(r#"\\"#)
             || self.0.contains(r#"\,"#)

diff --git a/src/parser/properties.rs b/src/parser/properties.rs
@@ -3,7 +3,7 @@ use std::{
     str::FromStr,
 };
 
-use crate::{parser::utils::valid_key_sequence_cow, properties::fold_line};
+use crate::{parser::utils::valid_key_sequence_cow, properties::fold_line, value_types::ValueType};
 
 use super::{
     parameters::{parameters, Parameter},
@@ -124,7 +124,7 @@ impl FromStr for crate::Property {
 }
 
 #[test]
-fn test_property() {
+fn properties() {
     assert_parser!(
         property,
         "KEY:VALUE\n",
@@ -164,31 +164,97 @@ fn test_property() {
             params: vec![Parameter::new_ref("foo", Some("bar"))]
         }
     );
+}
+
+#[test]
+fn unescape_text() {
+    assert_eq!(
+        "DESCRIPTION:https\\://example.com\n"
+            .parse::<crate::Property>()
+            .map(|p| p.val)
+            .unwrap(),
+        "https://example.com"
+    );
+
+    assert_parser!(
+        property,
+        "DESCRIPTION:https\\://example.com\n",
+        Property {
+            name: "DESCRIPTION".into(),
+            val: "https://example.com".into(),
+            params: Default::default()
+        }
+    );
+}
+
+#[test]
+fn property_escape_url_as_url() {
+    assert_eq!(
+        "URL:https://example.com\n"
+            .parse::<crate::Property>()
+            .map(|p| p.val)
+            .unwrap(),
+        "https://example.com"
+    );
+
+    assert_parser!(
+        property,
+        "URL:https://example.com\n",
+        Property {
+            name: "URL".into(),
+            val: "https://example.com".into(),
+            params: Default::default()
+        }
+    );
+}
 
-    // TODO: newlines followed by spaces must be ignored
+#[test]
+fn property_escape_description_as_text() {
     assert_parser!(
         property,
-        "KEY4;foo=bar:VALUE\\n newline separated\n",
+        "DESCRIPTION;foo=bar:VALUE\\n newline separated\n",
         Property {
-            name: "KEY4".into(),
+            name: "DESCRIPTION".into(),
             val: "VALUE\n newline separated".into(),
             params: vec![Parameter::new_ref("foo", Some("bar"))]
         }
     );
+}
 
+#[test]
+fn property_escape_by_value_param() {
     assert_parser!(
         property,
-        "KEY3;foo=bar:VALUE\\n newline separated\n",
+        "FOO;VALUE=TEXT:VALUE\\n newline separated\n",
         Property {
-            name: "KEY3".into(),
+            name: "FOO".into(),
             val: "VALUE\n newline separated".into(),
-            params: vec![Parameter::new_ref("foo", Some("bar"))]
+            params: vec![Parameter::new_ref("VALUE", Some("TEXT"))]
+        }
+    );
+
+    assert_parser!(
+        property,
+        "FOO_AS_TEXT;VALUE=TEXT:https\\://example.com\n",
+        Property {
+            name: "FOO_AS_TEXT".into(),
+            val: "https://example.com".into(),
+            params: vec![Parameter::new_ref("VALUE", Some("TEXT"))]
+        }
+    );
+    assert_parser!(
+        property,
+        "FOO_AS_URL;VALUE=URL:https\\://example.com\n",
+        Property {
+            name: "FOO_AS_URL".into(),
+            val: "https\\://example.com".into(),
+            params: vec![Parameter::new_ref("VALUE", Some("URL"))]
         }
     );
 }
 
 #[test]
-fn test_property_with_dash() {
+fn property_with_dash() {
     assert_parser!(
         property,
         "X-HOODIE-KEY:VALUE\n",
@@ -318,8 +384,7 @@ pub fn property<'a, E: ParseError<&'a str> + ContextError<&'a str>>(
                             // this is for single line prop parsing, just so I can leave off the '\n'
                             take_while(|_| true),
                         ))
-                        .map(ParseString::from)
-                        .map(ParseString::unescape_text),
+                        .map(ParseString::from),
                     ),
                 ),
                 context(
@@ -329,10 +394,25 @@ pub fn property<'a, E: ParseError<&'a str> + ContextError<&'a str>>(
             )),
             opt(line_ending),
         ))
-        .map(|(((key, params), val), _)| Property {
-            name: key,
-            val,
-            params,
+        .map(|(((name, params), val), _)| {
+            let value_type = params
+                .iter()
+                .find(|param| param.key == "VALUE")
+                .and_then(|Parameter { val, .. }| val.as_ref())
+                .and_then(|typ| ValueType::from_str(typ.as_str()).ok())
+                .or_else(|| ValueType::by_name(name.as_str()));
+
+            if let Some(value_type) = value_type {
+                eprintln!("escaping for {value_type:?}");
+                Property {
+                    name,
+                    val: val.unescape_by_value_type(value_type),
+                    params,
+                }
+            } else {
+                eprintln!("NOT escaping");
+                Property { name, val, params }
+            }
         })),
     )
     .parse(input)

diff --git a/src/properties.rs b/src/properties.rs
@@ -2,8 +2,11 @@ use std::{
     collections::HashMap,
     fmt::{self, Write},
     mem,
+    str::FromStr,
 };
 
+use crate::value_types::ValueType;
+
 #[derive(Clone, Debug, PartialEq, Eq)]
 /// key-value pairs inside of `Property`s
 pub struct Parameter {
@@ -89,7 +92,10 @@ impl Property {
 
     /// Returns the `VALUE` parameter, if any is specified.
     pub fn value_type(&self) -> Option<ValueType> {
-        ValueType::from_str(&self.params.get("VALUE")?.val)
+        self.params
+            .get("VALUE")
+            .and_then(|p| ValueType::from_str(&p.val).ok())
+            .or_else(|| ValueType::by_name(self.key()))
     }
 
     // /// Returns the value as a certain type
@@ -171,7 +177,17 @@ impl Property {
         for Parameter { key, val } in self.params.values() {
             write!(line, ";{}={}", key, Self::quote_if_contains_colon(val))?;
         }
-        write!(line, ":{}", Self::escape_text(&self.val))?;
+        let value_type = self.value_type();
+        eprintln!(
+            "{}:{} value type of {key} = {value_type:?}",
+            file!(),
+            line!(),
+            key = self.key
+        );
+        match value_type {
+            Some(ValueType::Text) => write!(line, ":{}", Self::escape_text(&self.val))?,
+            _ => write!(line, ":{}", self.val)?,
+        }
         write_crlf!(out, "{}", fold_line(&line))?;
         Ok(())
     }
@@ -224,85 +240,6 @@ impl From<Class> for Property {
     }
 }
 
-/// see 8.3.4. [Value Data Types Registry](https://tools.ietf.org/html/rfc5545#section-8.3.4)
-#[derive(Copy, Clone, Debug, Eq, PartialEq)]
-pub enum ValueType {
-    /// [`Binary`](https://datatracker.ietf.org/doc/html/rfc5545#section-3.3.1)
-    Binary,
-    /// [`Boolean`](https://datatracker.ietf.org/doc/html/rfc5545#section-3.3.2)
-    Boolean,
-    /// [`CalAddress`](https://datatracker.ietf.org/doc/html/rfc5545#section-3.3.3)
-    CalAddress,
-    /// [`Date`](https://datatracker.ietf.org/doc/html/rfc5545#section-3.3.4)
-    Date,
-    /// [`DateTime`](https://datatracker.ietf.org/doc/html/rfc5545#section-3.3.5)
-    DateTime,
-    /// [`Duration`](https://datatracker.ietf.org/doc/html/rfc5545#section-3.3.6)
-    Duration,
-    /// [`Float`](https://datatracker.ietf.org/doc/html/rfc5545#section-3.3.7)
-    Float,
-    /// [`Integer`](https://datatracker.ietf.org/doc/html/rfc5545#section-3.3.8)
-    Integer,
-    /// [`Period`](https://datatracker.ietf.org/doc/html/rfc5545#section-3.3.9)
-    Period,
-    /// [`Recur`](https://datatracker.ietf.org/doc/html/rfc5545#section-3.3.10)
-    Recur,
-    /// [`Text`](https://datatracker.ietf.org/doc/html/rfc5545#section-3.3.11)
-    Text,
-    /// [`Time`](https://datatracker.ietf.org/doc/html/rfc5545#section-3.3.12)
-    Time,
-    /// [`Uri`](https://datatracker.ietf.org/doc/html/rfc5545#section-3.3.13)
-    Uri,
-    /// [`UtcOffset`](https://datatracker.ietf.org/doc/html/rfc5545#section-3.3.14)
-    UtcOffset,
-}
-
-impl ValueType {
-    pub(crate) fn from_str(s: &str) -> Option<Self> {
-        match s {
-            "BINARY" => Some(Self::Binary),
-            "BOOLEAN" => Some(Self::Boolean),
-            "CAL-ADDRESS" => Some(Self::CalAddress),
-            "DATE" => Some(Self::Date),
-            "DATE-TIME" => Some(Self::DateTime),
-            "DURATION" => Some(Self::Duration),
-            "FLOAT" => Some(Self::Float),
-            "INTEGER" => Some(Self::Integer),
-            "PERIOD" => Some(Self::Period),
-            "RECUR" => Some(Self::Recur),
-            "TEXT" => Some(Self::Text),
-            "TIME" => Some(Self::Time),
-            "URI" => Some(Self::Uri),
-            "UTC-OFFSET" => Some(Self::UtcOffset),
-            _ => None,
-        }
-    }
-}
-
-impl From<ValueType> for Parameter {
-    fn from(val: ValueType) -> Self {
-        Parameter {
-            key: String::from("VALUE"),
-            val: String::from(match val {
-                ValueType::Binary => "BINARY",
-                ValueType::Boolean => "BOOLEAN",
-                ValueType::CalAddress => "CAL-ADDRESS",
-                ValueType::Date => "DATE",
-                ValueType::DateTime => "DATE-TIME",
-                ValueType::Duration => "DURATION",
-                ValueType::Float => "FLOAT",
-                ValueType::Integer => "INTEGER",
-                ValueType::Period => "PERIOD",
-                ValueType::Recur => "RECUR",
-                ValueType::Text => "TEXT",
-                ValueType::Time => "TIME",
-                ValueType::Uri => "URI",
-                ValueType::UtcOffset => "UTC-OFFSET",
-            }),
-        }
-    }
-}
-
 #[derive(Copy, Clone, Debug, Eq, PartialEq)]
 /// Encodes the status of an `Event`
 /// [RFC 5545, Section 3.8.1.11](https://datatracker.ietf.org/doc/html/rfc5545#section-3.8.1.11)
@@ -375,6 +312,31 @@ impl From<EventStatus> for Property {
     }
 }
 
+// TODO: why do we add this?
+impl From<ValueType> for Parameter {
+    fn from(val: ValueType) -> Self {
+        Parameter {
+            key: String::from("VALUE"),
+            val: String::from(match val {
+                ValueType::Binary => "BINARY",
+                ValueType::Boolean => "BOOLEAN",
+                ValueType::CalAddress => "CAL-ADDRESS",
+                ValueType::Date => "DATE",
+                ValueType::DateTime => "DATE-TIME",
+                ValueType::Duration => "DURATION",
+                ValueType::Float => "FLOAT",
+                ValueType::Integer => "INTEGER",
+                ValueType::Period => "PERIOD",
+                ValueType::Recur => "RECUR",
+                ValueType::Text => "TEXT",
+                ValueType::Time => "TIME",
+                ValueType::Uri => "URI",
+                ValueType::UtcOffset => "UTC-OFFSET",
+            }),
+        }
+    }
+}
+
 impl From<TodoStatus> for Property {
     fn from(val: TodoStatus) -> Self {
         Property::new_pre_alloc(