Simplify Contact.all to parse everything or die
A bad file 500s the request and lands in Sentry with its name, which
beats quietly serving a partial address book.
Assisted-by: GLM-5.3 via pi
diff --git a/lib/pro_tacts/contact.rb b/lib/pro_tacts/contact.rb
index 0c65823..8b816f7 100644
--- a/lib/pro_tacts/contact.rb
+++ b/lib/pro_tacts/contact.rb
@@ -1,6 +1,5 @@
require "kdl"
require "pathname"
-require "sentry-ruby"
require "pro_tacts"
require "pro_tacts/vcard"
@@ -11,42 +10,29 @@ module ProTacts
# to the vCard UID (see docs/plans/2026-01-12-carddav-architecture.md).
class Contact < Data.define(:id, :vcard)
# Ids end up in paths and arrive from client-supplied hrefs, so a
- # filename outside this charset is skipped at load; everything
- # listed is then fetchable by its id.
+ # filename outside this charset cannot be served.
ID_FORMAT = /\A[\w-]+\z/
# Every contact in the directory, one file per contact. An empty
- # directory is a valid empty address book, but any non-hidden
- # non-.kdl file raises — a misplaced file or a wrong
- # PRO_TACTS_DATA_DIR should not quietly serve a partial address
- # book. Files that fail to parse or render are reported and skipped;
- # Sentry is a no-op while uninitialized, so tests need no DSN.
+ # directory is a valid empty address book, and dotfiles are skipped
+ # (Finder drops .DS_Store into any directory it opens). Anything
+ # else raises: a bad file 500s the request and lands in Sentry
+ # rather than quietly serving a partial address book.
def self.all(directory = ProTacts.config.contacts_dir)
directory = Pathname.new(directory)
raise ArgumentError, "contacts directory not found: #{directory}" unless directory.directory?
- unexpected = directory.children
+ directory.children
.reject { it.basename.to_s.start_with?(".") }
- .reject { it.extname == ".kdl" }
- unless unexpected.empty?
- raise ArgumentError, "unexpected non-KDL file in contacts directory: #{unexpected.first}"
- end
-
- directory.glob("*.kdl").filter_map do |path|
- parse(path)
- rescue KDL::Error, ArgumentError, SystemCallError => e
- Sentry.capture_message("skipping contact file #{path}: #{e.class}: #{e.message}")
- end
+ .map { parse(it) }
end
- # File to Contact; raises on anything that would make the contact
- # unloadable. `all` decides what to do about it.
+ # File to Contact; raises on any filename or content that would make
+ # the contact unloadable.
def self.parse(path)
path = Pathname.new(path)
id = path.basename(".kdl").to_s
- unless id.match?(ID_FORMAT)
- raise ArgumentError, "invalid contact id: #{id}"
- end
+ raise ArgumentError, "invalid contact id: #{id}" unless id.match?(ID_FORMAT)
new(id:, vcard: VCard.render(KDL.parse(path.read), uid: id))
end
diff --git a/test/pro_tacts/test_contact.rb b/test/pro_tacts/test_contact.rb
index f4af361..f9c10b2 100644
--- a/test/pro_tacts/test_contact.rb
+++ b/test/pro_tacts/test_contact.rb
@@ -10,7 +10,7 @@ class ContactTest < Minitest::Test
Dir.mktmpdir do |dir|
directory = Pathname.new(dir)
files.each { |name, content| File.write(directory / name, content) }
- yield ProTacts::Contact.all(directory)
+ yield directory
end
end
@@ -18,8 +18,8 @@ class ContactTest < Minitest::Test
with_contacts({
"znorth.kdl" => "name \"Zed\"",
"aiden.kdl" => "name \"Aiden\"",
- }) do |contacts|
- ids = contacts.map { it.id }
+ }) do |directory|
+ ids = ProTacts::Contact.all(directory).map { it.id }
assert_includes ids, "aiden"
assert_includes ids, "znorth"
@@ -27,14 +27,23 @@ class ContactTest < Minitest::Test
end
def test_the_uid_comes_from_the_filename
- with_contacts({"kqmtnwpxlrvszoyp.kdl" => "name \"Aiden\""}) do |contacts|
- assert_includes contacts.first.vcard, "UID:kqmtnwpxlrvszoyp"
+ with_contacts({"kqmtnwpxlrvszoyp.kdl" => "name \"Aiden\""}) do |directory|
+ assert_includes ProTacts::Contact.all(directory).first.vcard, "UID:kqmtnwpxlrvszoyp"
end
end
def test_an_empty_directory_lists_no_contacts
- with_contacts({}) do |contacts|
- assert_empty contacts
+ with_contacts({}) do |directory|
+ assert_empty ProTacts::Contact.all(directory)
+ end
+ end
+
+ def test_dotfiles_are_ignored
+ with_contacts({
+ ".DS_Store" => "junk",
+ "aiden.kdl" => "name \"Aiden\"",
+ }) do |directory|
+ assert_equal %w[aiden], ProTacts::Contact.all(directory).map { it.id }
end
end
@@ -47,59 +56,48 @@ class ContactTest < Minitest::Test
end
def test_non_kdl_files_raise
- Dir.mktmpdir do |dir|
- File.write(Pathname.new(dir) / "notes.txt", "hello")
-
- error = assert_raises(ArgumentError) { ProTacts::Contact.all(dir) }
+ with_contacts({"notes.txt" => "hello"}) do |directory|
+ error = assert_raises(ArgumentError) { ProTacts::Contact.all(directory) }
- assert_match(/non-KDL file/, error.message)
- assert_match(/notes\.txt/, error.message)
+ assert_equal "invalid contact id: notes.txt", error.message
end
end
- def test_dotfiles_are_ignored
- with_contacts({
- ".DS_Store" => "junk",
- "aiden.kdl" => "name \"Aiden\"",
- }) do |contacts|
- assert_equal %w[aiden], contacts.map { it.id }
+ def test_filenames_that_are_not_ids_raise
+ with_contacts({"John Smith.kdl" => "name \"John\""}) do |directory|
+ error = assert_raises(ArgumentError) { ProTacts::Contact.all(directory) }
+
+ assert_equal "invalid contact id: John Smith", error.message
end
end
- def test_files_whose_id_cannot_be_fetched_are_skipped
- with_contacts({
- "John Smith.kdl" => "name \"John\"",
- "aiden.kdl" => "name \"Aiden\"",
- }) do |contacts|
- assert_equal %w[aiden], contacts.map { it.id }
+ def test_unparseable_files_raise
+ with_contacts({"broken.kdl" => "contact {"}) do |directory|
+ assert_raises(KDL::ParseError) { ProTacts::Contact.all(directory) }
end
end
- def test_unparseable_files_are_skipped
- with_contacts({
- "broken.kdl" => "contact {",
- "aiden.kdl" => "name \"Aiden\"",
- }) do |contacts|
- assert_equal %w[aiden], contacts.map { it.id }
+ def test_files_whose_keys_are_not_contact_fields_raise
+ with_contacts({"person.kdl" => "person { name \"A\" }"}) do |directory|
+ error = assert_raises(ArgumentError) { ProTacts::Contact.all(directory) }
+
+ assert_equal "unknown key in contact: person", error.message
end
end
- def test_files_whose_keys_are_not_contact_fields_are_skipped
- with_contacts({
- "person.kdl" => "person { name \"A\" }",
- "empty.kdl" => "",
- "aiden.kdl" => "name \"Aiden\"",
- }) do |contacts|
- assert_equal %w[aiden], contacts.map { it.id }
+ def test_empty_files_raise
+ with_contacts({"empty.kdl" => ""}) do |directory|
+ error = assert_raises(ArgumentError) { ProTacts::Contact.all(directory) }
+
+ assert_equal "contact requires a name", error.message
end
end
- def test_files_whose_card_cannot_render_are_skipped
- with_contacts({
- "nameless.kdl" => "phone \"+1-555-1234\"",
- "aiden.kdl" => "name \"Aiden\"",
- }) do |contacts|
- assert_equal %w[aiden], contacts.map { it.id }
+ def test_files_whose_card_cannot_render_raise
+ with_contacts({"nameless.kdl" => "phone \"+1-555-1234\""}) do |directory|
+ error = assert_raises(ArgumentError) { ProTacts::Contact.all(directory) }
+
+ assert_equal "contact requires a name", error.message
end
end
end