Stops Meta claiming methods it has no value for - #8
Merged
Conversation
Meta#respond_to_missing? returned true for every name, on the reasoning that method_missing answers to everything so respond_to? should agree. That is internally consistent but wrong: the set of names Ruby *asks* about is far larger than the set anyone will ever call. Every implicit probe got a false yes and then took the nil that method_missing returns for an unknown key as a real answer. Marshal made it expensive. dump asked for marshal_dump, got nil, and wrote that as the object's entire serialised state — 20 bytes, no error. load then called marshal_load on an allocated instance with no @DaTa yet, raising NoMethodError: undefined method `[]' for nil:NilClass from inside method_missing. Consumers caching resources in a store that marshals its entries wrote a poisoned entry on the first request and raised on every cache hit after that. Found via fortnox-api on a Rails :memory_store, where every resource carries a Meta and the second request inside the cache window blew up. Bare getters are now whitelisted against the keys @DaTa actually holds — the same shape ModelProxy already uses for model attributes. Setters and predicates stay unconditional: writing is how a key comes into existence and an unset predicate is meaningfully false. A blacklist of protocol names was considered and rejected; it cannot be enumerated, which is the tell that it was the wrong shape. Three details the first cut of this fix got wrong: - @DaTa is guarded, because respond_to? must never raise and Ruby allocates before initialising. Psych's revive probes init_with on a bare allocation, and so does Marshal.load on a payload written by an older version. Without the guard the whitelist merely moved the NoMethodError out of method_missing and into respond_to_missing?. With it, YAML round-trips — which it never did before — and a stale payload fails as a legible TypeError. - The name check is anchored to /\A[a-zA-Z_]\w*[=?]\z/ rather than end_with?("="), so operators stay out. Unanchored, respond_to?(:<=) was true and `meta <= 5` read as a setter writing @DaTa[:<]. Only half the hole: Ruby dispatches operators without consulting respond_to?, so method_missing still writes that key. Filed as #6. - The guarantee is narrower than "nothing probes for = or ?". No core Ruby probe uses those shapes, but ActiveSupport's acts_like?(:date) is respond_to?(:acts_like_date?), so Meta still answers yes. Filed as #7 rather than papered over. Resource::MetaCollector had the identical defect and gets the identical treatment, guard and anchoring included. method_missing is deliberately unchanged. A bare getter for an unset key still returns nil rather than raising, because the gem author extensions example in docs/model-architecture.md sets a meta key conditionally and reads it unconditionally; raising would break it on every full resource. respond_to? therefore under-reports for never-set keys, which is the safe direction — Ruby's implicit probes consult respond_to? and so never reach method_missing. Reconciling the two halves means requiring meta keys to be declared up front, which closes the acts_like? and operator holes with it; filed as #7 for 2.0. Payloads written before this fix are not recoverable, so the CHANGELOG carries an upgrade note for consumers on a cache that survives deploys. Adds spec/rest_easy/meta_spec.rb plus MetaCollector coverage in resource_spec.rb — 26 examples, 16 of which fail against the unfixed code. Covers both constructor paths for meta (class-level metadata defaults and before_parse hooks), collections, stubs, YAML, and a whole-hash assertion, since the original failure nulled @DaTa entirely and spot-checking single keys would have been weak. The implicit coercion examples assert on the TypeError message rather than its class: both versions raise, and only the broken one reports "#to_hash gives NilClass".
There was a problem hiding this comment.
Pull request overview
Prevents Meta and MetaCollector from claiming unsupported methods, fixing serialization and coercion failures.
Changes:
- Restricts dynamic method introspection to known getters and valid accessors.
- Adds Marshal, YAML, coercion, and metadata regression coverage.
- Documents behavior and cache migration requirements.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
lib/rest_easy/meta.rb |
Refines dynamic method introspection. |
lib/rest_easy/resource.rb |
Applies equivalent handling to MetaCollector. |
spec/rest_easy/meta_spec.rb |
Adds serialization and coercion regression tests. |
spec/rest_easy/resource_spec.rb |
Adds MetaCollector coverage. |
CHANGELOG.md |
Documents the fix and upgrade guidance. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Two follow-ups, one from review and one from CI. Ruby identifiers are not ASCII-only, so /\A[a-zA-Z_]\w*[=?]\z/ under-reported: `meta.företag = x` is a valid setter that method_missing accepts and stores, yet respond_to?(:företag=) was false. That contradicts the "setters and predicates are claimed unconditionally" rule the fix itself documents, and it matters here — Swedish meta keys are not hypothetical. POSIX classes ([[:alpha:]_] and [[:word:]]) claim those names while still excluding operators and digit-leading names. Same change in MetaCollector::SETTER_PATTERN. The stale-payload spec pinned Ruby's TypeError message with a backtick-apostrophe quote pair. Ruby 3.4 changed it to matching straight quotes, so the example passed on 3.2 and 3.3 and failed on 3.4. The matcher now accepts either form. Adds a non-ASCII accessor example to both spec files.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Meta#respond_to_missing? returned true for every name, on the reasoning that method_missing answers to everything so respond_to? should agree. That is internally consistent but wrong: the set of names Ruby asks about is far larger than the set anyone will ever call. Every implicit probe got a false yes and then took the nil that method_missing returns for an unknown key as a real answer.
Marshal made it expensive. dump asked for marshal_dump, got nil, and wrote that as the object's entire serialised state — 20 bytes, no error. load then called marshal_load on an allocated instance with no @DaTa yet, raising
NoMethodError: undefined method `[]' for nil:NilClass
from inside method_missing. Consumers caching resources in a store that marshals its entries wrote a poisoned entry on the first request and raised on every cache hit after that. Found via fortnox-api on a Rails :memory_store, where every resource carries a Meta and the second request inside the cache window blew up.
Bare getters are now whitelisted against the keys @DaTa actually holds — the same shape ModelProxy already uses for model attributes. Setters and predicates stay unconditional: writing is how a key comes into existence and an unset predicate is meaningfully false. A blacklist of protocol names was considered and rejected; it cannot be enumerated, which is the tell that it was the wrong shape.
Three details the first cut of this fix got wrong:
@DaTa is guarded, because respond_to? must never raise and Ruby allocates before initialising. Psych's revive probes init_with on a bare allocation, and so does Marshal.load on a payload written by an older version. Without the guard the whitelist merely moved the NoMethodError out of method_missing and into respond_to_missing?. With it, YAML round-trips — which it never did before — and a stale payload fails as a legible TypeError.
The name check is anchored to /\A[a-zA-Z_]\w*[=?]\z/ rather than end_with?("="), so operators stay out. Unanchored, respond_to?(:<=) was true and
meta <= 5read as a setter writing @DaTa[:<]. Only half the hole: Ruby dispatches operators without consulting respond_to?, so method_missing still writes that key. Filed as Meta#method_missing treats operators as setters #6.The guarantee is narrower than "nothing probes for = or ?". No core Ruby probe uses those shapes, but ActiveSupport's acts_like?(:date) is respond_to?(:acts_like_date?), so Meta still answers yes. Filed as 2.0: Reconcile Meta's two halves by declaring meta keys up front #7 rather than papered over.
Resource::MetaCollector had the identical defect and gets the identical treatment, guard and anchoring included.
method_missing is deliberately unchanged. A bare getter for an unset key still returns nil rather than raising, because the gem author extensions example in docs/model-architecture.md sets a meta key conditionally and reads it unconditionally; raising would break it on every full resource. respond_to? therefore under-reports for never-set keys, which is the safe direction — Ruby's implicit probes consult respond_to? and so never reach method_missing. Reconciling the two halves means requiring meta keys to be declared up front, which closes the acts_like? and operator holes with it; filed as #7 for 2.0.
Payloads written before this fix are not recoverable, so the CHANGELOG carries an upgrade note for consumers on a cache that survives deploys.
Adds spec/rest_easy/meta_spec.rb plus MetaCollector coverage in resource_spec.rb — 26 examples, 16 of which fail against the unfixed code. Covers both constructor paths for meta (class-level metadata defaults and before_parse hooks), collections, stubs, YAML, and a whole-hash assertion, since the original failure nulled @DaTa entirely and spot-checking single keys would have been weak. The implicit coercion examples assert on the TypeError message rather than its class: both versions raise, and only the broken one reports "#to_hash gives NilClass".