Ensure url parsing errors in noteObservation are exposed as such. Fixes #571

CHANGELOG.md

@@ -19,6 +19,7 @@
 ### What's Fixed
 
 - PlacesConnection.getVisited will now return that invalid URLs have not been visited, instead of throwing. ([#552](https://github.com/mozilla/application-services/issues/552))
+- PlacesConnection.noteObservation will correctly identify url parse failures as such. ([#571](https://github.com/mozilla/application-services/issues/571))
 
 # 0.13.3 (_2019-01-11_)
 

components/places/android/src/test/java/org/mozilla/places/PlacesConnectionTest.kt

@@ -106,6 +106,14 @@ class PlacesConnectionTest {
     }
 
 
+    @Test
+    fun testNoteObservationBadUrl() {
+        try {
+            db.noteObservation(VisitObservation(url = "http://www.[].com", visitType = VisitType.LINK))
+        } catch (e: PlacesException) {
+            assert(e is UrlParseFailed)
+        }
+    }
 
 }
 

components/places/src/observation.rs

@@ -21,8 +21,10 @@ use url::Url;
 /// no observation.
 #[derive(Debug, Serialize, Deserialize)]
 pub struct VisitObservation {
-    #[serde(with = "url_serde")]
-    pub url: Url,
+    /// Ideally, we'd use url::Url here with `serde_url`, but we really would
+    /// like to expose these errors over the FFI as UrlParseErrors and not json
+    /// errors, and we also would like to do so without parsing strings.
+    pub url: String,
 
     #[serde(skip_serializing_if = "Option::is_none")]
     #[serde(default)]
@@ -48,10 +50,10 @@ pub struct VisitObservation {
     #[serde(default)]
     pub at: Option<Timestamp>,
 
-    #[serde(with = "url_serde")]
+    /// Semantically also a url::Url, See the comment about the `url` property.
     #[serde(skip_serializing_if = "Option::is_none")]
     #[serde(default)]
-    pub referrer: Option<Url>,
+    pub referrer: Option<String>,
 
     #[serde(skip_serializing_if = "Option::is_none")]
     #[serde(default)]
@@ -61,7 +63,7 @@ pub struct VisitObservation {
 impl VisitObservation {
     pub fn new(url: Url) -> Self {
         VisitObservation {
-            url,
+            url: url.into_string(),
             title: None,
             visit_type: None,
             is_error: None,
@@ -112,7 +114,7 @@ impl VisitObservation {
     }
 
     pub fn with_referrer(mut self, v: impl Into<Option<Url>>) -> Self {
-        self.referrer = v.into();
+        self.referrer = v.into().map(|v| v.into_string());
         self
     }
 

components/places/src/storage/history.rs

@@ -28,9 +28,10 @@ pub fn apply_observation_direct(
     db: &Connection,
     visit_ob: VisitObservation,
 ) -> Result<Option<RowId>> {
-    let mut page_info = match fetch_page_info(db, &visit_ob.url)? {
+    let url = Url::parse(&visit_ob.url)?;
+    let mut page_info = match fetch_page_info(db, &url)? {
         Some(info) => info.page,
-        None => new_page_info(db, &visit_ob.url, None)?,
+        None => new_page_info(db, &url, None)?,
     };
     let mut update_change_counter = false;
     let mut update_frec = false;