sequel: keep the rescues, and drop the sole seam with them
`sole_or_nil`, and the sole/sole! pair after it, bought a `row &&` or a
`row ?` at every call site where a method-level `rescue
Sequel::NoMatchingRow` had kept the happy path to one expression. The
rescue reads better, so #contact, #book_name, #card_id_with_uid,
#group and #everyone_group_id go back to it and Sequel::Sole is one
method again.
#delete_group is the exception and keeps its early return, because
rescuing around the whole method swallowed a NoMatchingRow raised from
inside the transaction below. It reads with `first` instead: id is the
primary key, so there is no ambiguity for `sole` to catch —
#stored_card's own rule.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WWFQaLD58uLFedh3Dvfb2C
diff --git a/AGENTS.md b/AGENTS.md
index d974def..09eff06 100644
--- a/AGENTS.md
+++ b/AGENTS.md
@@ -210,13 +210,13 @@ not a fixture; edit a `.vcf` to change what the replay serves.
so no repair is ever needed for it. A write that touches a card must
land its change-log entry in the same transaction, because a client's
sync token silently skips whatever the log missed.
-- A read whose filter is meant to identify one row uses `sole` or
- `sole!`, not `first` — `first` answers with one of several rather
- than saying the filter was too loose. Both raise
- `Sequel::Sole::TooManyRows` for more than one row. They differ on
- none: `sole!` raises `Sequel::NoMatchingRow`, and `sole` answers nil
- for a read where no row is ordinary, which is what `Store#contact`
- does for the 404 path. The store loads the extension on every open.
+- A read whose filter is meant to identify one row uses `sole`, not
+ `first` — `first` answers with one of several rather than saying the
+ filter was too loose. `sole` raises either way it is wrong:
+ `Sequel::NoMatchingRow` for none, `Sequel::Sole::TooManyRows` for
+ more. A caller for whom no row is ordinary rescues the first, which is
+ what `Store#contact` does for the 404 path. The store loads the
+ extension on every open.
- An etag is derived from the card, never stored beside it: `Contact`
hashes what it serves, and its constructor is the only way to make
one — no second path takes an etag on trust. The etag in `changes` is
diff --git a/lib/pro_tacts/store.rb b/lib/pro_tacts/store.rb
index f03401b..c1f2f9f 100644
--- a/lib/pro_tacts/store.rb
+++ b/lib/pro_tacts/store.rb
@@ -338,12 +338,15 @@ module ProTacts
end
# `sole` rather than `first`: the id is the primary key, so a second
- # row is a corruption and not a choice to make quietly. No row is
- # the ordinary answer for an href nobody has — the 404 path.
+ # row is a corruption and not a choice to make quietly. It raises on
+ # no row too, which here is the ordinary answer for an href nobody
+ # has — the 404 path — so that one is caught and turned back into
+ # nil.
#: (String id) -> Contact?
def contact(id)
- row = cards.where(id:).sole
- row && contact_from(row, birthday_of(id), inherited_of(id))
+ contact_from(cards.where(id:).sole, birthday_of(id), inherited_of(id))
+ rescue Sequel::NoMatchingRow
+ nil
end
# The groups a contact belongs to, whole — a tag names one and
@@ -388,8 +391,9 @@ module ProTacts
# is set either way.
#: (String login) -> String
def book_name(login)
- row = books.where(login:).sole
- row ? row.fetch(:name).to_s : login
+ books.where(login:).sole.fetch(:name).to_s
+ rescue Sequel::NoMatchingRow
+ login
end
# Sets the name a login's book goes by, or with nil or a blank goes
@@ -422,8 +426,9 @@ module ProTacts
# cards sharing a UID is a corruption to raise on, not a choice.
#: (String uid) -> String?
def card_id_with_uid(uid)
- row = card_properties.where(name: "UID", value: uid).sole
- row && row.fetch(:card_id).to_s
+ card_properties.where(name: "UID", value: uid).sole.fetch(:card_id).to_s
+ rescue Sequel::NoMatchingRow
+ nil
end
# Stores a card and everything that has to move with it: the
@@ -570,10 +575,10 @@ module ProTacts
# keep serving the dead href out of its cache forever.
#: (String id) -> String
def reid(id)
- # `sole!` rather than #contact's nil: a read whose filter
+ # `sole` rather than #contact's nil-for-404: a read whose filter
# means one row, and no row here is the caller's mistake to hear
# about as Sequel::NoMatchingRow, not a request to answer.
- before = contact_from(cards.where(id:).sole!, birthday_of(id), inherited_of(id))
+ before = contact_from(cards.where(id:).sole, birthday_of(id), inherited_of(id))
CONTACT_ID_ATTEMPTS.times do
new_id = ChangeId.mint(ChangeId::CONTACT_LENGTH)
@@ -601,7 +606,7 @@ module ProTacts
# rather than moved — the deal #reindex always gives it. The
# parameters follow their properties away on the cascade.
card_properties.where(card_id: id).delete
- after = contact_from(cards.where(id: new_id).sole!, birthday_of(new_id), inherited_of(new_id))
+ after = contact_from(cards.where(id: new_id).sole, birthday_of(new_id), inherited_of(new_id))
record(id, action: Action::DELETE, etag: nil, diff: CardDiff.between(before.vcard, nil))
record(
new_id,
@@ -662,8 +667,9 @@ module ProTacts
# #contact's own shape.
#: (String id) -> Group?
def group(id)
- row = groups.select(*GROUP_COLUMNS, group_label.as(:label)).where(id:).sole
- row && load_groups([row]).fetch(0)
+ load_groups([groups.select(*GROUP_COLUMNS, group_label.as(:label)).where(id:).sole]).fetch(0)
+ rescue Sequel::NoMatchingRow
+ nil
end
# A group's name, or none — NULL being the one spelling of
@@ -676,7 +682,7 @@ module ProTacts
def rename_group(id, name:)
name = nil if name.to_s.strip.empty?
@database.transaction do
- was = groups.where(id:).sole!.fetch(:name)&.to_s
+ was = groups.where(id:).sole.fetch(:name)&.to_s
if was != name && (sync_name?(was) || sync_name?(name))
members = member_ids([id])
fan_out(members, moved: members) { groups.where(id:).update(name:) }
@@ -757,7 +763,12 @@ module ProTacts
# the ordinary miss, #delete's shape.
#: (String id) -> bool
def delete_group(id)
- row = groups.where(id:).sole
+ # A plain `first` read, #stored_card's reason: id is the primary
+ # key, so there is no ambiguity for `sole` to catch. Read here
+ # rather than rescued around the whole method, which would have
+ # swallowed a NoMatchingRow raised from inside the transaction
+ # below and answered it as an ordinary miss.
+ row = groups.where(id:).first
return false if row.nil?
name = row.fetch(:name).to_s
@@ -961,8 +972,9 @@ module ProTacts
# (db/migrations/008_group_names.rb).
#: () -> String
def everyone_group_id
- row = groups.where(name: EVERYONE).sole
- row ? row.fetch(:id).to_s : create_group(name: EVERYONE)
+ groups.where(name: EVERYONE).sole.fetch(:id).to_s
+ rescue Sequel::NoMatchingRow
+ create_group(name: EVERYONE)
end
# The group name that puts cards in this login's book alone.
@@ -978,7 +990,7 @@ module ProTacts
#: (String group_id) -> bool
def sync_group?(group_id)
- sync_name?(groups.where(id: group_id).sole!.fetch(:name)&.to_s)
+ sync_name?(groups.where(id: group_id).sole.fetch(:name)&.to_s)
end
#: (String group_id, String card_id) -> bool
@@ -999,11 +1011,11 @@ module ProTacts
end
# A group_members row names a card by foreign key, so a missing
- # row here is the corruption `sole!` exists to raise on rather than
+ # row here is the corruption `sole` exists to raise on rather than
# the ordinary miss an href off the wire is.
#: (String id) -> Contact
def contact!(id)
- contact_from(cards.where(id:).sole!, birthday_of(id), inherited_of(id))
+ contact_from(cards.where(id:).sole, birthday_of(id), inherited_of(id))
end
# Replaces a card's rows in the index. Rebuilt wholesale rather than
diff --git a/lib/pro_tacts/web/contacts.rb b/lib/pro_tacts/web/contacts.rb
index 399ff6c..36ede4b 100644
--- a/lib/pro_tacts/web/contacts.rb
+++ b/lib/pro_tacts/web/contacts.rb
@@ -190,7 +190,7 @@ module ProTacts
# `was` is the boxes checked when the page loaded, so a group joined
# or left elsewhere since stays as it is, and a stale page has
# nothing to revert and no snapshot to refuse. Only ids naming a
- # group: Store#add_member reads the group with `sole!`, so a doctored
+ # group: Store#add_member reads the group with `sole`, so a doctored
# one would be a 500 rather than the bad input it is. A `new` name
# is a group the filter box offered to create, joined in the same
# save; a blank one is none.
diff --git a/lib/sequel/extensions/sole.rb b/lib/sequel/extensions/sole.rb
index a489d13..0fc9066 100644
--- a/lib/sequel/extensions/sole.rb
+++ b/lib/sequel/extensions/sole.rb
@@ -2,20 +2,19 @@
require "sequel"
module Sequel
- # The row a filter identifies, and what to do when there is not one.
+ # The row a filter identifies, or a raise.
#
# A filter meant to identify one row — a lookup by primary key, later
# the group that contributed a property to a card — reads naturally as
# `first`, and `first` answers with one of several without a word when
# the filter turns out not to identify anything. That is the failure
- # worth catching: the query claimed one row, so more than one is a
- # broken assumption rather than a result to pick from, and both of
- # these raise on it.
+ # worth catching: the query claimed one row, so anything else is a
+ # broken assumption rather than a result to pick from.
#
- # Where the two differ is no row at all, which is the ordinary answer
- # for some reads — an href nobody has, a login whose book was never
- # named — and a corruption for others. `sole` answers nil there and
- # `sole!` raises, which is what the bang says everywhere else in Ruby.
+ # Both ways of being wrong raise, so the assertion is in one method
+ # rather than split across a pair. A caller for whom no row is
+ # ordinary says so by rescuing Sequel::NoMatchingRow, which is the
+ # error Sequel's own `first!` raises for it.
#
# Load it with `DB.extension(:sole)`, which reaches every dataset the
# database makes.
@@ -26,7 +25,7 @@ module Sequel
class TooManyRows < Sequel::Error; end
#: () -> Hash[Symbol, untyped]
- def sole!
+ def sole
# Two is enough to know, and a LIMIT keeps a filter that turned out
# to match the whole table from dragging it back to prove the
# point. It replaces any limit already set, which is why this is
@@ -41,16 +40,6 @@ module Sequel
rows.fetch(0)
end
-
- # Sequel::NoMatchingRow is what Sequel's own `first!` raises for an
- # empty dataset, so the rescue names one error rather than this
- # extension's own.
- #: () -> Hash[Symbol, untyped]?
- def sole
- sole!
- rescue Sequel::NoMatchingRow
- nil
- end
end
Dataset.register_extension(:sole, Sole)
diff --git a/sig/gems/sequel.rbs b/sig/gems/sequel.rbs
index d5955f8..32b10bc 100644
--- a/sig/gems/sequel.rbs
+++ b/sig/gems/sequel.rbs
@@ -154,8 +154,7 @@ module Sequel
class Error < StandardError
end
- # What `first!` raises on an empty dataset, `sole!` with it, and
- # `sole` rescues to answer nil.
+ # What `first!` raises on an empty dataset, and `sole` with it.
class NoMatchingRow < Error
def initialize: (?untyped message_or_dataset) -> void
end
diff --git a/test/pro_tacts/test_store.rb b/test/pro_tacts/test_store.rb
index a544beb..3bbdaee 100644
--- a/test/pro_tacts/test_store.rb
+++ b/test/pro_tacts/test_store.rb
@@ -1302,7 +1302,7 @@ class StoreTest < Minitest::Test
with_store do |store|
id = store.create_group
- assert_nil database(store)[:groups].where(id:).sole!.fetch(:name)
+ assert_nil database(store)[:groups].where(id:).sole.fetch(:name)
assert_raises(Sequel::ConstraintViolation) { store.create_group(name: "") }
end
end
@@ -2167,7 +2167,7 @@ class StoreTest < Minitest::Test
end
def card_row(store, id)
- database(store)[:cards].where(id:).sole!
+ database(store)[:cards].where(id:).sole
end
def indexed_names(store, card_id)
diff --git a/test/sequel/extensions/test_sole.rb b/test/sequel/extensions/test_sole.rb
index 17b55b1..16c852a 100644
--- a/test/sequel/extensions/test_sole.rb
+++ b/test/sequel/extensions/test_sole.rb
@@ -23,7 +23,7 @@ class SoleTest < Minitest::Test
def test_the_one_row_comes_back
with_rows(%w[a b]) do |things|
- assert_equal "body of a", things.where(id: "a").sole!.fetch(:body)
+ assert_equal "body of a", things.where(id: "a").sole.fetch(:body)
end
end
@@ -31,33 +31,12 @@ class SoleTest < Minitest::Test
# for whom no row is ordinary has one error to name.
def test_no_rows_raises
with_rows(%w[a]) do |things|
- assert_raises(Sequel::NoMatchingRow) { things.where(id: "nobody").sole! }
+ assert_raises(Sequel::NoMatchingRow) { things.where(id: "nobody").sole }
end
end
# The half `first` gets wrong: it would answer with one of the two.
def test_more_than_one_row_raises
- with_rows(%w[a a]) do |things|
- assert_raises(Sequel::Sole::TooManyRows) { things.where(id: "a").sole! }
- end
- end
-
- def test_sole_answers_the_one_row
- with_rows(%w[a b]) do |things|
- assert_equal "body of a", things.where(id: "a").sole.fetch(:body)
- end
- end
-
- # The half the pair exists for: a read where no row is the answer
- # rather than a corruption.
- def test_sole_answers_nil_for_no_rows
- with_rows(%w[a]) do |things|
- assert_nil things.where(id: "nobody").sole
- end
- end
-
- # More than one is the broken assumption either way it is asked.
- def test_sole_still_raises_for_more_than_one_row
with_rows(%w[a a]) do |things|
assert_raises(Sequel::Sole::TooManyRows) { things.where(id: "a").sole }
end
@@ -67,7 +46,7 @@ class SoleTest < Minitest::Test
# filter can carry card content, so the message names the table only.
def test_the_message_names_the_table_and_not_the_query
with_rows(%w[a a]) do |things|
- error = assert_raises(Sequel::Sole::TooManyRows) { things.where(body: "body of a").sole! }
+ error = assert_raises(Sequel::Sole::TooManyRows) { things.where(body: "body of a").sole }
assert_equal "more than one row in things", error.message
refute_includes error.message, "body of a"
@@ -99,7 +78,7 @@ class SoleTest < Minitest::Test
log = QueryLog.new
things.db.loggers << log
- assert_raises(Sequel::Sole::TooManyRows) { things.sole! }
+ assert_raises(Sequel::Sole::TooManyRows) { things.sole }
assert(log.queries.any? { it.include?("LIMIT 2") }, log.queries.inspect)
end
end