Require required attributes#5
Merged
Merged
Conversation
The DSL auto-applied :synthetic when a parse block (or mapper #parse) declared 2+ parameters, but did not do the same for the symmetric multi-param serialise case. Both shapes produce an attribute whose value does not live at @model_attributes[model_name]; only the merge side was being marked. Until now the flag had no readers in lib, so the asymmetry was invisible. It becomes visible the moment any code path branches on synthetic? — applying the flag now keeps that future code correct.
Previously the flag raised MissingAttributeError only when an API response omitted the field; it was silently ignored on save, letting incomplete payloads reach the backend. Serialise now raises before any HTTP request when a required attribute is nil, with the attribute name on the exception.
For attributes with multi-param parse or serialise, :required was a
silent no-op: the parse-side check sat inside the non-source_fields
branch, and the serialise-side check explicitly bypassed any attr
flagged :synthetic. The flag's intent ("don't accept partial data")
was unenforceable for the very shapes most likely to depend on it.
Semantic: marking a synthetic attribute :required means all of its
underlying fields must be present.
* Merge (multi-param parse): all source API fields must be in the
response on parse; if round-trippable, the merged model value
must additionally be non-nil at serialise time.
* Combine (multi-param serialise): all named model attributes must
be non-nil at serialise time. Parse-side check does not apply.
* Split (single-param bare block extracting from one API field):
the underlying API field must be present on parse.
:read_only continues to skip the serialise-time check, matching the
single-field semantics.
Documents the previously-undocumented combine pattern (multi-param
serialise) in the README, alongside merge and split.
There was a problem hiding this comment.
Pull request overview
This PR tightens :required attribute semantics in RestEasy::Resource by enforcing requiredness not only when parsing inbound API payloads, but also when serialising outbound payloads (including synthetic/derived attributes), and documents the updated behavior.
Changes:
- Enforce
:requiredonserialise(and thereforesave/to_api), raisingMissingAttributeErrorwith#attribute_name. - Enforce
:requiredfor synthetic attributes on parse/serialise, and auto-tag multi-parameterserialisedefinitions as:synthetic. - Expand specs, README documentation, and changelog entries to cover the new rules and synthetic patterns (merge/combine/split).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| spec/rest_easy/resource_spec.rb | Adds comprehensive coverage for :required across parse/stub/serialise/save and synthetic patterns, plus synthetic auto-detection. |
| lib/rest_easy/resource.rb | Implements serialise-time required checks and required enforcement for synthetic parse paths; auto-applies :synthetic for multi-parameter serialise. |
| README.md | Updates :required documentation and adds detailed explanations/examples for merge/combine/split behavior. |
| CHANGELOG.md | Records the behavioral change for :required and synthetic auto-detection. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ehannes
added a commit
that referenced
this pull request
Jun 26, 2026
A combine attribute's api_name (e.g. "Address" for a model that builds
the value from :street + :city on serialise) does not exist on the API
side by design. The previous code still looked up api_data[api_name],
stored the value at @model_attributes[model_name], and ran the standard
:required check — none of which make sense for a synthetic attribute.
If the API ever did return the shadowed name, the framework would store
it and then overwrite it during serialise from target_fields, leaving
instance.address and the outbound payload in contradiction.
Three consequences:
* init_from_api now branches on attr_def.synthetic? for non-source_fields
attrs and stores nil, skipping the lookup and the :required raise.
* The debug-mode "expected API field but missing" warning also skips
combine attrs, since their absence is the documented contract.
* The combine context gains a spec asserting that an inbound value for
the shadowed API field is ignored.
Caught by Copilot on PR #5.
ehannes
added a commit
that referenced
this pull request
Jun 26, 2026
A combine attribute's api_name (e.g. "Address" for a model that builds
the value from :street + :city on serialise) does not exist on the API
side by design. The previous code still looked up api_data[api_name],
stored the value at @model_attributes[model_name], and ran the standard
:required check — none of which make sense for a synthetic attribute.
If the API ever did return the shadowed name, the framework would store
it and then overwrite it during serialise from target_fields, leaving
instance.address and the outbound payload in contradiction.
Three consequences:
* init_from_api now branches on attr_def.synthetic? for non-source_fields
attrs and stores nil, skipping the lookup and the :required raise.
* The debug-mode "expected API field but missing" warning also skips
combine attrs, since their absence is the documented contract.
* The combine context gains a spec asserting that an inbound value for
the shadowed API field is ignored.
Caught by Copilot on PR #5.
A combine attribute's api_name (e.g. "Address" for a model that builds
the value from :street + :city on serialise) does not exist on the API
side by design. The previous code still looked up api_data[api_name],
stored the value at @model_attributes[model_name], and ran the standard
:required check — none of which make sense for a synthetic attribute.
If the API ever did return the shadowed name, the framework would store
it and then overwrite it during serialise from target_fields, leaving
instance.address and the outbound payload in contradiction.
Three consequences:
* init_from_api now branches on attr_def.synthetic? for non-source_fields
attrs and stores nil, skipping the lookup and the :required raise.
* The debug-mode "expected API field but missing" warning also skips
combine attrs, since their absence is the documented contract.
* The combine context gains a spec asserting that an inbound value for
the shadowed API field is ignored.
A :required attribute means "must carry a value", and explicit null in
the API response is treated as missing — the same as an omitted key.
Rationale:
* :required + null is semantically odd; if null is acceptable, the
attribute is :optional, not :required.
* Stricter behavior catches upstream API regressions more loudly
(a field that suddenly goes null is a signal worth raising on).
* Symmetric with the serialise-side check, which already raises on nil.
* Loosening later is easy; tightening after consumers depend on the
loose behavior would be a breaking change.
Adds a spec pinning the behavior so future refactors don't accidentally
loosen it. Clarifies the README flags table and the parse-time CHANGELOG
entry. No code change.
The :required check appeared inline in three places: both branches of
init_from_api and the serialise loop. Each restated the same logic
("if required and any value is nil, raise MissingAttributeError"),
making future refactors (e.g. tweaking the message, adding new flags
like :nullable, changing the predicate to use Hash#key?) require
edits in multiple sites with subtle drift opportunities.
Centralizes the check on the attribute itself:
attr_def.validate_required!(*values)
The method is a no-op for non-required attrs and raises with the
attribute name when any value is nil. Behavior-preserving — all 202
existing specs continue to pass.
Also moves the serialise-loop required check from a leading pre-check
to per-branch calls, eliminating the duplicated target_fields gather
(previously computed once for the required check and once for the
actual serialisation).
The existing save spec only exercised the create path (stubbed
instance → new? → POST). The accompanying assertion that PUT was
not called was vacuously true since save never dispatched to it.
A future regression that only broke the update path (parse → mutate
→ PUT) would have left the suite green. Adds a sibling spec that
parses an instance to non-new?, mutates a :required field to nil,
and asserts save raises before PUT is issued.
Renames the original spec for symmetry ("raises before POSTing a
new record" / "raises before PUTing an updated record").
Two improvements:
* The ":read_only attributes are not enforced" spec stubbed an
:id-less instance by parsing { "Id" => 1, ... }. Since :id was
always non-nil, the spec passed regardless of whether the
:read_only short-circuit ran — the :required check would never
have raised either way. Switches to stub(name: "Acme") with no
:id, so the spec actually exercises the read_only-takes-priority
invariant.
* The "mutated to clear" spec asserted that serialise raises but
not that update(name: nil) actually applied. A future regression
in update that silently dropped nil-valued writes would have
produced a passing test for the wrong reason. Adds
expect(mutated.name).to be_nil before the raise expectation.
Earlier work on this branch widened the Description column in the Available settings table to fit the longer Flags-table descriptions that landed at the same time. That re-padded every settings row, polluting git blame on rows whose content was untouched. Reverts the settings-table whitespace to match main exactly. The Flags-table widening stays — it's load-bearing for the longer :required description.
Two sites in init_from_api had inline copies of the same compound check
("synthetic? && source_fields.empty?") to detect combine attributes —
the parse-loop elsif branch and the debug-warn skip. A future change
to what "combine" means (e.g., the target_fields-based refactor we've
been discussing) would have to update both.
Adds Attribute#combine? returning synthetic? && source_fields.empty?
and switches both call sites to use it. The debug-warn loop's earlier
"already raises" comment was misleading for combine + required attrs
(they no longer raise, they're skipped via this predicate); rewrites
the comment to describe both skip categories accurately.
Also removes a dead `value = @model_attributes[attr_def.model_name]`
assignment at the top of the serialise loop that was unused by the
target_fields branch. Inlines it in the two branches that need it.
A combine pattern (multi-parameter `serialise`, no multi-parameter `parse`) has no inbound API field — its api_name does not exist on the API side. If the user also defines an explicit `parse` block in the DSL, that block was silently ignored: init_from_api skips the combine branch without ever calling parse_value. That silent ignore is a footgun. A user mis-wiring a combine attr (or experimenting with hybrid shapes) would never get feedback that their parse logic is dead code. Emits a warning at attribute-declaration time when target_fields is populated, source_fields is empty, and an explicit parse_block was supplied via the DSL block form. The message names the attribute, shows the target_fields it gathers from, and suggests removing the parse block or restructuring. Mapper-form combine is intentionally NOT covered by the warning — the mapper interface requires a `parse` method, so its presence is structural rather than user intent. The README documents the limitation for both forms.
Two gaps surfaced in self-review:
* Attribute had no unit specs at all. Adds spec/rest_easy/attribute_spec.rb
covering validate_required! edge cases: not-required is a no-op,
required raises on any nil, multi-value calls behave correctly,
and the empty-splat case is documented as "nothing to check"
rather than "missing" so future call sites with conditionally
empty arrays don't spuriously raise.
* No spec exercised combine + :read_only. The combination is
plausible (e.g. an audit-trail attribute the API returns
but we never want to send back). Adds two specs verifying
that serialise is skipped via the :read_only short-circuit
and that parse leaves the model slot nil regardless of any
inbound shadow value.
Also adds Attribute#combine? predicate specs alongside.
The earlier wording — "split attributes typically aren't sent back to the API as a single field, mark them :read_only to skip the serialise-time check" — collapsed two distinct behaviors into one ambiguous claim. Split attributes ARE serialised; they go out under their own model api_names (Street, City), not recombined into the inbound source field (Address). Rewrites to make the serialise direction explicit, then offers two recovery patterns for users whose API does not accept the parts as independent top-level fields: :read_only (skip serialisation) or an after_serialise hook (reconstruct the source field).
The previous definition (synthetic? && source_fields.empty?) would classify an attribute as combine if the user passed :synthetic as an explicit flag without any block — e.g. `attr :foo, String, :synthetic`. With no target_fields, treating that as combine wrongly skips the api_name lookup in init_from_api and silently sets the model slot to nil, hiding any inbound value. Combine is structurally defined by target_fields (the model fields the serialise block gathers from) plus the absence of source_fields (no inbound merge / split). Using the structural predicate instead of the flag avoids the edge case without relying on synthetic? being applied consistently. Adds a spec for the explicit-synthetic-flag edge case so the classification stays correct under future refactors.
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.
No description provided.