Merge pull request #592 from EdouardVanbelle/fix/carddav-parser-tel-adr

fix(carddav): fix tel uri

src/application/services/contact_service.rs

@@ -103,7 +103,11 @@ impl ContactService {
         Ok(book)
     }
 
-    fn parse_vcard(&self, vcard_data: &str) -> Result<Contact, DomainError> {
+    // Associated function (no `&self`) so tests in this module
+    // can call `ContactService::parse_vcard(&body)` directly
+    // without instantiating a full service (which needs an
+    // Arc<ContactStorageAdapter> and an Arc<PgAclEngine>).
+    fn parse_vcard(vcard_data: &str) -> Result<Contact, DomainError> {
         // This is a simplified vCard parser - a real implementation would use a proper vCard library
         // For now, we'll create a basic contact with minimal data
 
@@ -123,11 +127,15 @@ impl ContactService {
                     contact.set_first_name(Some(parts[1].to_string()));
                 }
             } else if line.starts_with("EMAIL") {
-                let value = line.split(':').nth(1).unwrap_or("");
+                // Split on the FIRST colon — same rationale as the
+                // TEL branch below; keeps parameter parsing separate
+                // from value parsing.
+                let value = line.split_once(':').map(|(_, v)| v.trim()).unwrap_or("");
                 if !value.is_empty() {
-                    let email_type = if line.contains("TYPE=HOME") {
+                    let params_upper = line.to_ascii_uppercase();
+                    let email_type = if params_upper.contains("TYPE=HOME") {
                         "home"
-                    } else if line.contains("TYPE=WORK") {
+                    } else if params_upper.contains("TYPE=WORK") {
                         "work"
                     } else {
                         "other"
@@ -140,15 +148,35 @@ impl ContactService {
                     });
                 }
             } else if line.starts_with("TEL") {
-                let value = line.split(':').nth(1).unwrap_or("");
+                // Split on the FIRST colon so URI-form values survive.
+                // Apple Contacts / DAVx⁵ send:
+                //   TEL;TYPE=cell;VALUE=uri:tel:+15551234567
+                // The pre-fix `split(':').nth(1)` picked up "tel"
+                // (the middle segment), silently losing the actual
+                // phone number. `split_once(':')` splits ONCE at the
+                // property-name/value boundary; we then strip the
+                // `tel:` URI scheme if present.
+                let value = line.split_once(':').map(|(_, v)| v.trim()).unwrap_or("");
+                let value = value.strip_prefix("tel:").unwrap_or(value);
                 if !value.is_empty() {
-                    let phone_type = if line.contains("TYPE=CELL") || line.contains("TYPE=MOBILE") {
+                    // RFC 6350 §5.3: parameter values are
+                    // case-insensitive. Match on the uppercase
+                    // form of the whole property line so
+                    // `TYPE=cell` and `TYPE=CELL` both route
+                    // correctly. Pre-fix this was case-sensitive
+                    // and dropped lowercase to "other" — matches
+                    // the shape python-caldav / Apple Contacts
+                    // emit.
+                    let params_upper = line.to_ascii_uppercase();
+                    let phone_type = if params_upper.contains("TYPE=CELL")
+                        || params_upper.contains("TYPE=MOBILE")
+                    {
                         "mobile"
-                    } else if line.contains("TYPE=HOME") {
+                    } else if params_upper.contains("TYPE=HOME") {
                         "home"
-                    } else if line.contains("TYPE=WORK") {
+                    } else if params_upper.contains("TYPE=WORK") {
                         "work"
-                    } else if line.contains("TYPE=FAX") {
+                    } else if params_upper.contains("TYPE=FAX") {
                         "fax"
                     } else {
                         "other"
@@ -160,6 +188,62 @@ impl ContactService {
                         is_primary: contact.phone_is_empty(), // First one is primary
                     });
                 }
+            } else if line.starts_with("ADR") {
+                // ADR (RFC 6350 §6.3.1). Structured value: 7 components
+                // separated by `;` — (pobox, extended, street, city,
+                // region, postal, country). Positions 0/1 are legacy
+                // and typically empty; we preserve positions 2–6 as
+                // (street, city, state, postal_code, country) which
+                // matches the emitter format at
+                // `carddav_adapter.rs::contact_to_vcard`.
+                //
+                // Pre-fix parse_vcard had NO ADR handler at all —
+                // every ADR line sent by a client was silently dropped
+                // at PUT time, so no address ever survived a
+                // round-trip. See bug_carddav_parser_gaps.md.
+                let value = line.split_once(':').map(|(_, v)| v).unwrap_or("");
+                let parts: Vec<&str> = value.split(';').collect();
+                let field = |i: usize| {
+                    parts
+                        .get(i)
+                        .map(|s| s.trim().to_string())
+                        .filter(|s| !s.is_empty())
+                };
+                let params_upper = line.to_ascii_uppercase();
+                let addr_type = if params_upper.contains("TYPE=HOME") {
+                    "home"
+                } else if params_upper.contains("TYPE=WORK") {
+                    "work"
+                } else {
+                    "other"
+                };
+                // Only push if AT LEAST one of the useful fields
+                // is populated — an all-empty ADR line is a
+                // no-op sent by some clients that "clear" the
+                // address; storing an empty row would confuse
+                // downstream UIs.
+                let street = field(2);
+                let city = field(3);
+                let state = field(4);
+                let postal_code = field(5);
+                let country = field(6);
+                if street.is_some()
+                    || city.is_some()
+                    || state.is_some()
+                    || postal_code.is_some()
+                    || country.is_some()
+                {
+                    let is_primary = contact.address_is_empty();
+                    contact.push_address(Address {
+                        street,
+                        city,
+                        state,
+                        postal_code,
+                        country,
+                        r#type: addr_type.to_string(),
+                        is_primary,
+                    });
+                }
             } else if let Some(stripped) = line.strip_prefix("ORG:") {
                 contact.set_organization(Some(stripped.to_string()));
             } else if let Some(stripped) = line.strip_prefix("TITLE:") {
@@ -535,7 +619,7 @@ impl ContactUseCase for ContactService {
             .await?;
 
         // Parse vCard data
-        let mut contact = self.parse_vcard(&dto.vcard)?;
+        let mut contact = Self::parse_vcard(&dto.vcard)?;
 
         // Set address book ID
         contact.set_address_book_id(address_book_id);
@@ -1224,3 +1308,136 @@ impl UserLifecycleHook for DefaultAddressBookLifecycleHook {
         Ok(())
     }
 }
+
+// ─────────────────────────────────────────────────────────────
+// Tests — parse_vcard property surface
+// ─────────────────────────────────────────────────────────────
+
+#[cfg(test)]
+mod parse_vcard_tests {
+    use super::*;
+
+    /// Wrap minimal vCard 3.0 header/footer around one or more
+    /// property lines. CRLF-normalise, matching wire format.
+    fn vcard(lines: &[&str]) -> String {
+        let mut body =
+            String::from("BEGIN:VCARD\r\nVERSION:3.0\r\nUID:parse-test\r\nFN:Parse Test\r\n");
+        for l in lines {
+            body.push_str(l);
+            body.push_str("\r\n");
+        }
+        body.push_str("END:VCARD\r\n");
+        body
+    }
+
+    // ── TEL ───────────────────────────────────────────────────
+
+    #[test]
+    fn tel_plain_form_still_parses() {
+        // Regression pin: pre-existing shape `TEL;TYPE=CELL:+1...`
+        // (no VALUE=uri) must continue to parse cleanly.
+        let body = vcard(&["TEL;TYPE=CELL:+15551234567"]);
+        let c = ContactService::parse_vcard(&body).expect("valid vcard");
+        assert_eq!(c.phone().len(), 1);
+        assert_eq!(c.phone()[0].number, "+15551234567");
+        assert_eq!(c.phone()[0].r#type, "mobile");
+    }
+
+    #[test]
+    fn tel_uri_form_survives_first_colon_split() {
+        // The #528-adjacent bug the fix targets. Pre-fix the
+        // parser `split(':').nth(1)` would return "tel", losing
+        // the actual number.
+        let body = vcard(&["TEL;TYPE=cell;VALUE=uri:tel:+15551234567"]);
+        let c = ContactService::parse_vcard(&body).expect("valid vcard");
+        assert_eq!(c.phone().len(), 1);
+        assert_eq!(
+            c.phone()[0].number,
+            "+15551234567",
+            "URI-scheme prefix must be stripped so downstream UIs \
+             show a clickable number, not `tel:+15551234567`."
+        );
+        assert_eq!(c.phone()[0].r#type, "mobile");
+    }
+
+    #[test]
+    fn tel_uri_form_without_scheme_prefix_survives() {
+        // Some clients emit VALUE=uri but no explicit `tel:` in
+        // the value. Handle gracefully — we take everything after
+        // the first colon and only strip `tel:` if present.
+        let body = vcard(&["TEL;VALUE=uri:+15551234567"]);
+        let c = ContactService::parse_vcard(&body).expect("valid vcard");
+        assert_eq!(c.phone()[0].number, "+15551234567");
+    }
+
+    // ── ADR ───────────────────────────────────────────────────
+
+    #[test]
+    fn adr_full_structured_value_populates_all_fields() {
+        // The reference shape from RFC 6350 §6.3.1:
+        //   ADR;TYPE=HOME:pobox;ext;street;city;region;postal;country
+        // Positions 0/1 (pobox, ext) are legacy and typically
+        // empty on real client output; we skip them by design.
+        let body = vcard(&["ADR;TYPE=HOME:;;42 Rue de Rivoli;Paris;Île-de-France;75001;France"]);
+        let c = ContactService::parse_vcard(&body).expect("valid vcard");
+        assert_eq!(c.address().len(), 1);
+        let a = &c.address()[0];
+        assert_eq!(a.street.as_deref(), Some("42 Rue de Rivoli"));
+        assert_eq!(a.city.as_deref(), Some("Paris"));
+        assert_eq!(a.state.as_deref(), Some("Île-de-France"));
+        assert_eq!(a.postal_code.as_deref(), Some("75001"));
+        assert_eq!(a.country.as_deref(), Some("France"));
+        assert_eq!(a.r#type, "home");
+        assert!(a.is_primary, "first ADR should be primary");
+    }
+
+    #[test]
+    fn adr_partial_value_only_populates_present_fields() {
+        // Client sends street + city only — the other structured
+        // components stay None (not "" — that would confuse the
+        // Address DTO's Option<String>-based null semantics).
+        let body = vcard(&["ADR:;;42 Rue de Rivoli;Paris;;;"]);
+        let c = ContactService::parse_vcard(&body).expect("valid vcard");
+        assert_eq!(c.address().len(), 1);
+        let a = &c.address()[0];
+        assert_eq!(a.street.as_deref(), Some("42 Rue de Rivoli"));
+        assert_eq!(a.city.as_deref(), Some("Paris"));
+        assert!(a.state.is_none());
+        assert!(a.postal_code.is_none());
+        assert!(a.country.is_none());
+        assert_eq!(a.r#type, "other", "no TYPE param → 'other'");
+    }
+
+    #[test]
+    fn adr_all_empty_is_dropped() {
+        // Some clients emit `ADR:;;;;;;` as a "clear this
+        // address" operation. Storing an empty row would show as
+        // a blank address slot in UIs. Skip it.
+        let body = vcard(&["ADR:;;;;;;"]);
+        let c = ContactService::parse_vcard(&body).expect("valid vcard");
+        assert_eq!(c.address().len(), 0);
+    }
+
+    #[test]
+    fn adr_type_work_recognized() {
+        let body = vcard(&["ADR;TYPE=WORK:;;5 Wall St;NYC;NY;10005;USA"]);
+        let c = ContactService::parse_vcard(&body).expect("valid vcard");
+        assert_eq!(c.address()[0].r#type, "work");
+    }
+
+    #[test]
+    fn adr_and_tel_coexist() {
+        // Two independent fixes on the same PUT should both
+        // populate — proves neither branch consumes lines meant
+        // for the other via prefix ambiguity.
+        let body = vcard(&[
+            "TEL;TYPE=CELL:+15551234567",
+            "ADR;TYPE=HOME:;;42 Rue de Rivoli;Paris;;75001;France",
+        ]);
+        let c = ContactService::parse_vcard(&body).expect("valid vcard");
+        assert_eq!(c.phone().len(), 1);
+        assert_eq!(c.phone()[0].number, "+15551234567");
+        assert_eq!(c.address().len(), 1);
+        assert_eq!(c.address()[0].city.as_deref(), Some("Paris"));
+    }
+}

src/domain/entities/contact.rs

@@ -402,6 +402,9 @@ impl Contact {
     pub fn push_phone(&mut self, p: Phone) {
         self.phone.push(p);
     }
+    pub fn push_address(&mut self, a: Address) {
+        self.address.push(a);
+    }
     pub fn set_email(&mut self, email: Vec<Email>) {
         self.email = email;
     }
@@ -417,6 +420,9 @@ impl Contact {
     pub fn phone_is_empty(&self) -> bool {
         self.phone.is_empty()
     }
+    pub fn address_is_empty(&self) -> bool {
+        self.address.is_empty()
+    }
 
     // --- Consuming methods for ownership transfer ---
     pub fn into_email(self) -> Vec<Email> {

src/infrastructure/services/nextcloud_chunked_upload_service.rs

@@ -1,6 +1,5 @@
 use std::path::PathBuf;
 use tokio::fs;
-use tokio::io::AsyncWriteExt;
 
 use crate::common::errors::{DomainError, Result};
 
@@ -76,6 +75,20 @@ impl NextcloudChunkedUploadService {
     /// `interfaces/upload_ingest::stream_body_to_path` helper to stream the
     /// HTTP body directly to disk and avoid materialising the whole chunk
     /// in RAM.
+    ///
+    /// Uses `tokio::fs::write` (single `spawn_blocking` around
+    /// `std::fs::write`) rather than manually driving
+    /// `create + write_all` and letting the tokio handle drop close the
+    /// fd. The manual shape leaked a race: `tokio::fs::File::drop`
+    /// dispatches `close(2)` to the blocking pool without awaiting it,
+    /// and until close completes the dirent update may not be visible
+    /// to a subsequent `read_dir` — on macOS APFS routinely, on Linux
+    /// under I/O contention. In practice that turned into
+    /// `ordered_chunk_paths` silently missing a just-uploaded chunk;
+    /// the NC assembly path (`handle_assemble` → `ordered_chunk_paths`)
+    /// would then produce a truncated file with no error to the client.
+    /// `std::fs::write` opens, writes, and synchronously closes before
+    /// returning, so the dirent is guaranteed visible on `.await`.
     pub async fn store_chunk(
         &self,
         user: &str,
@@ -84,13 +97,9 @@ impl NextcloudChunkedUploadService {
         data: &[u8],
     ) -> Result<()> {
         let chunk_path = self.safe_chunk_path(user, upload_id, chunk_name)?;
-        let mut file = fs::File::create(&chunk_path)
+        fs::write(&chunk_path, data)
             .await
-            .map_err(|e| DomainError::internal_error("ChunkedUpload", e.to_string()))?;
+            .map_err(|e| DomainError::internal_error("ChunkedUpload", e.to_string()))
-        file.write_all(data)
-            .await
-            .map_err(|e| DomainError::internal_error("ChunkedUpload", e.to_string()))?;
-        Ok(())
     }
 
     /// List the session's chunk files in assembly (numeric) order.

tests/api/carddav_vcard_properties.hurl

@@ -0,0 +1,134 @@
+# =============================================================
+# OxiCloud — CardDAV vCard property round-trip regression
+# =============================================================
+# Regression pin for fix/carddav-parser-tel-adr.
+#
+# `contact_service.rs::parse_vcard` had two independent gaps
+# and one case-sensitivity issue on the TYPE parameter:
+#
+#   1. TEL used `split(':').nth(1)` — a URI-form value like
+#      `TEL;TYPE=cell;VALUE=uri:tel:+15551234567` was sliced
+#      down to `"tel"`, losing the phone number entirely.
+#   2. ADR had no parser branch at all — every address was
+#      silently dropped at PUT time.
+#   3. TYPE param matching was case-sensitive; real clients
+#      (Apple Contacts, DAVx⁵, python-caldav) mix cases so
+#      `TYPE=cell` fell through to "other" instead of "mobile".
+#
+# Post-fix: `splitn(2, ':')` + `tel:` scheme strip, an ADR
+# branch parsing the 7-part structured value into (street,
+# city, state, postal_code, country), and case-insensitive
+# TYPE matching (uppercased once, checked against upper).
+#
+# Test shape: PUT a vCard exercising all three fixes; GET it
+# back; assert the emitter surfaces the parsed fields.
+# =============================================================
+
+
+# ─────────────────────────────────────────────────────────────
+# Step 1 — Admin login.
+# ─────────────────────────────────────────────────────────────
+POST {{base_url}}/api/auth/login
+Content-Type: application/json
+{ "username": "{{username}}", "password": "{{password}}" }
+
+HTTP 200
+[Captures]
+admin_token: jsonpath "$.access_token"
+
+
+# ─────────────────────────────────────────────────────────────
+# Step 2 — Discover admin's default address book.
+# ─────────────────────────────────────────────────────────────
+GET {{base_url}}/api/address-books
+Authorization: Bearer {{admin_token}}
+
+HTTP 200
+[Captures]
+# See dav_error_mapping.hurl for why body regex vs jsonpath
+# filter — same rationale (Hurl's scalar-vs-list handling on
+# single-match jsonpath filters is brittle).
+default_book_id: body regex "\"id\":\"([a-f0-9-]{36})\",\"name\":\"Contacts\""
+
+
+# ─────────────────────────────────────────────────────────────
+# Step 3 — PUT a vCard exercising all three fixes:
+#   * TEL URI form with lowercase TYPE=cell (URI-scheme + case
+#     insensitivity).
+#   * ADR with a full 7-field structured value and TYPE=HOME
+#     (parser branch existence + type detection).
+#   * EMAIL as a sanity anchor — the pre-existing path we did
+#     NOT change; must still round-trip cleanly.
+#
+# `dav-err-` UID prefix so a re-run inside the same DB (this
+# file runs BEFORE contacts.hurl in run.sh, so its state
+# doesn't collide with that suite).
+# ─────────────────────────────────────────────────────────────
+PUT {{base_url}}/carddav/{{default_book_id}}/dav-err-vcard-props.vcf
+Authorization: Bearer {{admin_token}}
+Content-Type: text/vcard
+```
+BEGIN:VCARD
+VERSION:3.0
+UID:dav-err-vcard-props
+FN:Regression VCard
+N:VCard;Regression;;;
+EMAIL;TYPE=work:regression@example.com
+TEL;TYPE=cell;VALUE=uri:tel:+15551234567
+ADR;TYPE=HOME:;;42 Rue de Rivoli;Paris;Île-de-France;75001;France
+END:VCARD
+```
+
+HTTP *
+[Asserts]
+status >= 200
+status < 300
+
+
+# ─────────────────────────────────────────────────────────────
+# Step 4 — GET the vCard back and assert the parser+emitter
+# preserved each property. The emitter regenerates the body
+# from DTO fields, so a value surfacing in the response body
+# is proof it made it through parser → DB → emitter intact.
+#
+# NOTE: emitter uses vCard 3.0 uppercase TYPE values
+# (`TEL;TYPE=MOBILE:`, `ADR;TYPE=HOME:`), so the response
+# body's casing is normalised regardless of what the client
+# sent.
+# ─────────────────────────────────────────────────────────────
+GET {{base_url}}/carddav/{{default_book_id}}/dav-err-vcard-props.vcf
+Authorization: Bearer {{admin_token}}
+
+HTTP 200
+[Asserts]
+# The bare number, with URI scheme stripped and unchanged
+# through the parser's first-colon split. Pre-fix this would
+# have been `+15551234567` in the input but the DB would
+# store `"tel"` and the emitter would output that instead.
+body contains "+15551234567"
+# Case-insensitive TYPE detection: lowercase `TYPE=cell` on
+# input → mapped to "mobile" internally → emitter writes
+# uppercase `TYPE=MOBILE`. Pre-fix (case-sensitive) this fell
+# through to "other" and emitted `TYPE=OTHER`.
+body contains "TYPE=MOBILE"
+# Address components — proves the ADR parser branch runs.
+body contains "42 Rue de Rivoli"
+body contains "Paris"
+body contains "Île-de-France"
+body contains "75001"
+body contains "France"
+# TYPE=HOME preserved from the input (uppercase in both
+# directions).
+body contains "TYPE=HOME"
+# Sanity: pre-existing EMAIL path still round-trips.
+body contains "regression@example.com"
+
+
+# ─────────────────────────────────────────────────────────────
+# Step 5 — Cleanup: delete the vCard so downstream files
+# (contacts.hurl in particular) don't inherit the fixture.
+# ─────────────────────────────────────────────────────────────
+DELETE {{base_url}}/carddav/{{default_book_id}}/dav-err-vcard-props.vcf
+Authorization: Bearer {{admin_token}}
+
+HTTP 204

tests/api/run.sh

@@ -166,6 +166,7 @@ hurl --variables-file "$API_DIR/test.env" --file-root "$REPO_ROOT/tests" --test
   "$API_DIR/dedup_blob_cleanup.hurl" \
   "$API_DIR/default_caldav_carddav.hurl" \
   "$API_DIR/dav_error_mapping.hurl" \
+  "$API_DIR/carddav_vcard_properties.hurl" \
   "$API_DIR/contacts.hurl" \
   "$API_DIR/calendar.hurl" \
   "$API_DIR/caldav_recurring.hurl" \