Skip to content

Raise Booqable::MissingAttribute for reads of absent attributes - #46

Merged
shime merged 3 commits into
mainfrom
feature/strict-attribute-reads
Jul 14, 2026
Merged

Raise Booqable::MissingAttribute for reads of absent attributes#46
shime merged 3 commits into
mainfrom
feature/strict-attribute-reads

Conversation

@shime

@shime shime commented Jul 8, 2026

Copy link
Copy Markdown
Member

Sawyer::Resource#method_missing silently answers reads of absent attributes with nil, which has turned typos and renamed API fields (e.g. time_zone vs default_timezone, order.customer_name on a v4 order) into silent production data bugs in consuming apps.

Reading an attribute that is absent from the API payload raises Booqable::MissingAttribute instead of silently returning nil, so typos and renamed API fields fail loudly:

customer.name       # => "John Doe"
customer.full_name  # raises Booqable::MissingAttribute (key absent from payload)

An attribute that is present in the payload with a null value still returns nil — only absent keys raise:

order.customer      # => nil when the payload contains "customer": null

Booqable::MissingAttribute subclasses NoMethodError, so generic rescues keep working. To probe for an attribute that may be absent, use hash-style access or key?:

order[:customer]      # => nil when the key is absent (lenient probe)
order.key?(:customer) # => false when the key is absent

Open in Devin Review

Sawyer::Resource#method_missing silently answers reads of absent
attributes with nil, which has turned typos and renamed API fields
(e.g. time_zone vs default_timezone, order.customer_name on a v4 order)
into silent production data bugs in consuming apps.

Reading an attribute that is absent from the API payload raises
`Booqable::MissingAttribute` instead of silently returning nil, so typos and
renamed API fields fail loudly:

```ruby
customer.name       # => "John Doe"
customer.full_name  # raises Booqable::MissingAttribute (key absent from payload)
```

An attribute that is present in the payload with a null value still returns
nil — only absent keys raise:

```ruby
order.customer      # => nil when the payload contains "customer": null
```

`Booqable::MissingAttribute` subclasses `NoMethodError`, so generic rescues
keep working. To probe for an attribute that may be absent, use hash-style
access or `key?`:

```ruby
order[:customer]      # => nil when the key is absent (lenient probe)
order.key?(:customer) # => false when the key is absent
```
Copilot AI review requested due to automatic review settings July 8, 2026 13:40

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no bugs or issues to report.

Open in Devin Review

This comment was marked as resolved.

Review feedback: don't peek at Sawyer's ivars. Sawyer::Resource's
public surface for these is the _agent/_fields attr_readers, so the
check now goes through them (which also drops the defined? guard — an
uninitialized ivar reads as nil through the reader). The bare
agent/fields spellings the review suggested are SPECIAL_METHODS
resolved inside method_missing itself, so calling them from this hook
would recurse infinitely; a comment now records that.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings July 8, 2026 13:51

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.

@shime shime added review Please look at my code team-ow Operations & Workflow labels Jul 9, 2026
@booqbruno
booqbruno requested a review from pbalaban July 9, 2026 12:12
@booqbruno booqbruno removed the review Please look at my code label Jul 9, 2026

@pbalaban pbalaban left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch 👍

Copilot AI review requested due to automatic review settings July 14, 2026 08:00
@shime
shime merged commit 9b4faef into main Jul 14, 2026
5 of 6 checks passed
@shime
shime deleted the feature/strict-attribute-reads branch July 14, 2026 08:01

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.

@shime shime mentioned this pull request Jul 14, 2026

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 1 new potential issue.

Open in Devin Review


attr_name = match[1].to_sym
return nil if _fields.include?(attr_name)
return nil if match[2].nil? && Sawyer::Resource::SPECIAL_METHODS.include?(match[1])

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Predicate checks on built-in resource accessors incorrectly raise an error instead of returning a value

The predicate form of Sawyer's built-in accessors (e.g. .agent?, .fields?, .rels?) is not exempted from the strict-read guard (booqable_missing_attribute_read at lib/booqable/strict_attributes.rb:63) because the special-methods bypass only fires for non-predicate calls, so these valid queries raise an error instead of returning a boolean.

Impact: Calling .agent?, .fields?, or .rels? on any API resource crashes with MissingAttribute instead of returning true.

Mechanism: the SPECIAL_METHODS guard is gated on match[2].nil?

At lib/booqable/strict_attributes.rb:63:

return nil if match[2].nil? && Sawyer::Resource::SPECIAL_METHODS.include?(match[1])

When the method is agent?, match[2] is "?" (not nil), so match[2].nil? is false and the entire guard is skipped. The code then falls through to return attr_name (:agent), which triggers MissingAttribute. Sawyer's own method_missing never gets a chance to handle the predicate form of the special method.

The fix is to drop the match[2].nil? && condition so both plain and predicate forms of special methods are passed through to Sawyer:

return nil if Sawyer::Resource::SPECIAL_METHODS.include?(match[1])
Suggested change
return nil if match[2].nil? && Sawyer::Resource::SPECIAL_METHODS.include?(match[1])
return nil if Sawyer::Resource::SPECIAL_METHODS.include?(match[1])
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

@shime shime mentioned this pull request Jul 14, 2026
shime added a commit that referenced this pull request Jul 14, 2026
Supersedes #48. The `Booqable::MissingAttribute` change (#46) landed on
main after that PR was opened — it's breaking, so this release is 2.0.0
instead of 1.2.2.

Included since 1.2.1:

- **Breaking:** reading an attribute absent from an API payload raises
`Booqable::MissingAttribute` instead of silently returning nil (#46)
- Require `cgi` and declare it as a runtime dependency, fixing
`undefined method 'parse' for class CGI` when handling `invalid_grant`
OAuth errors under Rails 8.1+ (#47)
- Ruby 4.0 support (#47)

🤖 Generated with [Claude Code](https://claude.com/claude-code)

<!-- devin-review-badge-begin -->

---

<a href="https://app.devin.ai/review/booqable/booqable.rb/pull/49"
target="_blank">
  <picture>
<source media="(prefers-color-scheme: dark)"
srcset="https://static.devin.ai/assets/gh-open-in-devin-review-dark.svg?v=1">
<img
src="https://static.devin.ai/assets/gh-open-in-devin-review-light.svg?v=1"
alt="Open in Devin Review">
  </picture>
</a>
<!-- devin-review-badge-end -->

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

team-ow Operations & Workflow

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants