[Fix] Call #nil? on attribute values only when a nil handler is set#546
[Fix] Call #nil? on attribute values only when a nil handler is set#546Sevenfold777 wants to merge 1 commit into
Conversation
`fetch_attribute` evaluated `value.nil?` before checking whether a nil handler exists, so `#nil?` was called on every attribute value even when `on_nil` was not used. For plain objects this is harmless, but lazy-loading proxies such as batch-loader implement `#nil?` via `method_missing` with a side effect: it forces the load immediately. When serializing a collection, this forces each record's proxy one by one and defeats batching (N+1 queries). Swapping the operands short-circuits on `nil_handler` first: - With `on_nil` set, behavior is unchanged. - Without it, `#nil?` is no longer called, which preserves lazy proxies' batching and skips one method call per attribute. Note for lazy proxy users: proxies now reach the serialized hash unresolved, so they must be resolved before or during encoding. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughAlba now avoids invoking ChangesNil handling behavior
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #546 +/- ##
=======================================
Coverage 97.74% 97.74%
=======================================
Files 14 14
Lines 664 664
Branches 116 116
=======================================
Hits 649 649
Misses 15 15 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
What
A one-line operand swap in
Alba::Resource::InstanceMethods#fetch_attribute:#nil?is now called on attribute values only when the resource declares a nil handler (on_nil).Why
fetch_attributecalledvalue.nil?on every attribute value even when noon_nilhandler was set (the common case), and the result was discarded becausenil_handlerwas falsy.For plain objects this is just one redundant method call per attribute. For lazy-loading proxies such as batch-loader, it is worse: they implement
#nil?viamethod_missingwith a side effect that forces the load immediately. When serializing a collection, each record's proxy is forced one by one while the hash is being built, which turns "collect all proxies, then load them in one batch" into N+1 queries. We hit this in production after porting collection serializers from ActiveModelSerializers (which only touches values at encoding time) to Alba.As far as I can tell, this is the only place where Alba calls a method on every attribute value unconditionally (
TypedAttribute#display_value_foronly runs for typed attributes on theTypeErrorpath, andselect/conditional procs are opt-in). So with this change, lazy values pass through hash building untouched end-to-end; the only remaining consideration for lazy-proxy users is resolution at encoding time, which is backend-dependent and outsidefetch_attribute's scope (see Behavior notes below).Behavior
No documented behavior changes — the entire existing test suite passes unchanged.
on_nilset:#nil?is evaluated exactly as before.on_nil:#nil?is no longer called.#nil?on a plain Ruby object is a side-effect-free predicate, and its result was discarded here anyway, so there is no observable difference — it just saves one method call per attribute.The only way to observe a difference is an object whose
#nil?itself has side effects — i.e. lazy proxies, which is what this PR is about. Two notes for such users:Alba.encoderthat walks the hash).on_errordeclared but noon_nil, a proxy whose load fails used to raise while fetching the attribute (whereon_errorcould handle it), because#nil?forced the load there. The error now surfaces at encoding time instead.Both notes describe objects relying on side effects of an internal evaluation order, not documented Alba behavior, so I kept the change as a plain swap. Happy to target a minor release rather than a patch if you prefer.
Tests
Added two tests in
test/usecases/nil_handler_test.rbusing a value object that records whether#nil?was called:on_nil→#nil?is not called (fails without this change)on_nil→#nil?is still called (guards handler semantics)rake test(283 runs),rubocop, andrake typecheck(RBS + Steep) all pass. Nosig/changes needed — no signature change.Summary by CodeRabbit
Bug Fixes
nil?check when no nil-handling option is configured.Documentation