diff --git a/CHANGELOG.md b/CHANGELOG.md index 92023c3..479d5e2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,45 @@ ## [Unreleased] +### Fixed + +- **`:required` is now enforced on serialise as well as parse.** Previously + the flag raised `MissingAttributeError` only when an API response omitted + the field; it was silently ignored when sending (`save`), letting + incomplete payloads reach the backend and surface as a generic + `RequestError`. The flag now also fires at `serialise` time (and therefore + on `save`) when a required attribute is `nil`, before any HTTP request, + with the attribute name on the exception (`#attribute_name`). Inbound + behavior is unchanged — `parse` still raises on missing required fields. + A field is treated as missing whether the API response omits the key + entirely or includes it as explicit `null`; both are rejected as + incomplete data. +- **`:required` is now enforced on synthetic attributes (merge / combine / + split patterns).** Previously the flag was silently a no-op for any + attribute with a multi-parameter parse or serialise block. It now + enforces presence on all underlying API or model fields: a merge + attribute requires every source API field on parse; a combine attribute + requires every named model attribute on serialise; a split attribute + requires the underlying API field on parse. `:read_only` continues to + skip the serialise-time check. + +### Changed + +- **The DSL auto-applies `:synthetic` to multi-parameter serialise blocks** + (and mapper `serialise` methods), mirroring the existing behavior for + multi-parameter parse. Previously only the parse side was tagged, + leaving the flag inconsistent with its intent of marking attributes whose + storage shape diverges from the standard one-slot layout. +- **Combine attributes no longer read from their `api_name` on parse.** + A combine attribute's API name does not correspond to a real inbound + field by design — the value is built from `target_fields` at serialise + time. The previous code looked up `api_data[api_name]`, stored it at + the model slot, and ran the standard `:required` check, raising + spuriously when the field was absent (the documented case). Inbound + values for the shadowed API key are now ignored and the model slot is + set to `nil`, avoiding the contradiction where `instance.address` could + return one value while serialise would overwrite it with another. + ## [1.3.1] - 2026-05-27 ### Fixed diff --git a/README.md b/README.md index 17cf3aa..34bf734 100644 --- a/README.md +++ b/README.md @@ -326,12 +326,12 @@ attr [:tax_url, '@urlTaxReductionList'], String, :read_only ### Flags -| Flag | Effect | -|--------------|----------------------------------------------------------| -| `:required` | Raises `MissingAttributeError` if absent in API response | -| `:optional` | Documents that the field may be absent (default) | -| `:read_only` | Excluded from serialisation (not sent back to the API) | -| `:key` | Marks the unique identifier for CRUD operations | +| Flag | Effect | +|--------------|------------------------------------------------------------------------| +| `:required` | Raises `MissingAttributeError` if missing or explicitly `null` in the API response (on parse) or `nil` at serialise time (`save`, `to_api`). Omitted keys and explicit `null` are treated identically — a required attribute must carry a value. See [Merge](#merge-pattern--many-api-fields-into-one-model-attribute), [Combine](#combine-pattern--many-model-attributes-into-one-api-field), and [Split](#split-pattern--one-api-field-into-many-model-attributes) patterns for how this applies to synthetic attributes. | +| `:optional` | Documents that the field may be absent (default) | +| `:read_only` | Excluded from serialisation (not sent back to the API) | +| `:key` | Marks the unique identifier for CRUD operations | ```ruby key :id, Integer, :read_only @@ -430,6 +430,31 @@ end attr :full_name, String, FullNameMapper ``` +Marking a merge attribute `:required` enforces that **all** underlying API fields are present in the response on parse. If the attribute is also round-trippable (not `:read_only`), the merged model value must additionally be non-`nil` at serialise time. + +Note that string-concatenation merges like the `full_name` example above are inherently lossy on the round-trip — splitting `"Hans Erik Nilsson"` back into `FirstName`/`LastName` is ambiguous. Such attributes should normally be `:read_only`. Round-trippable merges work when the model representation preserves the structure (e.g. a tuple, a `Money` value object, or a `Date` built from year/month/day components). + +### Combine pattern — many model attributes into one API field + +The dual of merge: when the serialise method takes multiple parameters, RestEasy gathers the values from the corresponding model attributes and passes them in. The block returns the single combined API value: + +```ruby +attr :street, String +attr :city, String + +attr :address, String do + serialise { |street, city| "#{street}, #{city}" } +end +``` + +The parameter names (`street`, `city`) name model attributes; the framework reads them from the instance and splats them into the block. The block's return value is written under the attribute's API name (`Address`). + +This also works with mapper objects whose `serialise` method takes multiple parameters. + +Marking a combine attribute `:required` enforces that **all** named model attributes are non-`nil` at serialise time. The parse side does not apply — combine attributes don't read from a single API field on the way in, so users typically populate the underlying model attributes directly. + +Combine attributes do not run a `parse` block. If you declare one alongside a multi-parameter `serialise`, RestEasy emits a warning at load time — the parse block would be silently ignored otherwise. If you need to read from an API field on parse, declare a separate attribute for it (or restructure as a merge pattern). + ### Split pattern — one API field into many model attributes Use a bare block with a parameter to extract from a single API field: @@ -446,6 +471,10 @@ end The parameter name (`address`) determines which API field to read from. +Marking a split attribute `:required` enforces that the underlying API field is present on parse — in the example above, the API response must include `Address`. + +On serialise, split attributes are emitted under their own api_names (here, `Street` and `City`) rather than being recombined into the original source field. If the API does not accept the parts as independent top-level fields, mark them `:read_only` to keep them out of the outbound payload entirely; alternatively, reconstruct the source field in an `after_serialise` hook. + ### Ignoring fields Tell RestEasy to silently skip API fields you don't need: diff --git a/lib/rest_easy/attribute.rb b/lib/rest_easy/attribute.rb index c1a93ac..ef9cc2b 100644 --- a/lib/rest_easy/attribute.rb +++ b/lib/rest_easy/attribute.rb @@ -36,6 +36,20 @@ def synthetic? @flags.include?(:synthetic) end + # A combine attribute is built from multiple model fields on serialise + # (target_fields), with no inbound source from its own api_name on parse. + # See the "Combine pattern" section of the README. + def combine? + @target_fields.any? && @source_fields.empty? + end + + def validate_required!(*values) + return unless required? + return if values.none?(&:nil?) + + raise RestEasy::MissingAttributeError.new(model_name) + end + def coerce(value) @type[value] rescue Dry::Types::ConstraintError, Dry::Types::CoercionError => e diff --git a/lib/rest_easy/resource.rb b/lib/rest_easy/resource.rb index eb44e60..ddf8c6f 100644 --- a/lib/rest_easy/resource.rb +++ b/lib/rest_easy/resource.rb @@ -250,6 +250,7 @@ def attr(name_or_mapping, *args, &block) serialise_params = serialise_block.parameters.select { |ptype, _| ptype == :opt || ptype == :req } if serialise_params.length > 1 + flags << :synthetic unless flags.include?(:synthetic) target_fields = serialise_params.map { |_, pname| pname } end elsif block @@ -284,9 +285,22 @@ def attr(name_or_mapping, *args, &block) if serialise_block params = serialise_block.parameters.select { |ptype, _| ptype == :opt || ptype == :req } if params.length > 1 + flags << :synthetic unless flags.include?(:synthetic) target_fields = params.map { |_, pname| pname } end end + + # Combine pattern (multi-param serialise, no multi-param parse) + # has no inbound api_name on parse. If the user also wrote an + # explicit `parse` block, it will be silently ignored — warn so + # the inconsistency is visible at load time. + if target_fields.any? && source_fields.empty? && parse_block + warn "RestEasy: :#{attribute_model_name} declares a combine pattern " \ + "(serialise from #{target_fields.inspect}) and also defines a parse block. " \ + "Combine attributes have no inbound API field to read, so the parse block " \ + "will not run. Remove the parse block, or restructure the declaration if you " \ + "intended to read from the API." + end end end @@ -591,13 +605,15 @@ def serialise # Serialise all attributes klass.all_attribute_definitions.each do |_model_name, attr_def| next if attr_def.read_only? - value = @model_attributes[attr_def.model_name] if attr_def.target_fields.any? # Multi-param serialise: gather model values by param names, splat into block model_values = attr_def.target_fields.map { |fn| @model_attributes[fn] } + attr_def.validate_required!(*model_values) result[attr_def.api_name] = attr_def.serialise_value(*model_values) elsif attr_def.source_fields.any? + value = @model_attributes[attr_def.model_name] + attr_def.validate_required!(value) serialised = attr_def.serialise_value(value) if serialised.is_a?(::Array) # Array return: zip with source field API names @@ -613,6 +629,8 @@ def serialise result[attr_def.api_name] = serialised end else + value = @model_attributes[attr_def.model_name] + attr_def.validate_required!(value) result[attr_def.api_name] = attr_def.serialise_value(value) end end @@ -683,13 +701,19 @@ def init_from_api(api_data, extra_meta = {}) api_key = convention.serialise(field_name) api_data[api_key] end + + attr_def.validate_required!(*raw_values) + @model_attributes[model_name] = attr_def.parse_value(*raw_values) + elsif attr_def.combine? + # Combine pattern: the attribute's api_name does not exist on the + # API side by design — the value is built from target_fields at + # serialise time. Nothing inbound to read or validate. + @model_attributes[model_name] = nil else raw_value = api_data[attr_def.api_name] - if raw_value.nil? && attr_def.required? - raise MissingAttributeError.new(model_name) - end + attr_def.validate_required!(raw_value) if raw_value.nil? @model_attributes[model_name] = nil @@ -717,9 +741,12 @@ def init_from_api(api_data, extra_meta = {}) end end - # Warn about declared attributes missing from the API response + # Warn about declared attributes missing from the API response. + # Combine attrs have no inbound api_name by design; non-combine + # required attrs already raised in the parse loop above. klass.all_attribute_definitions.each do |model_name, attr_def| - next if attr_def.required? # already raises + next if attr_def.combine? + next if attr_def.required? api_keys_to_check = if attr_def.source_fields.any? attr_def.source_fields.map { |sf| convention.serialise(sf) } diff --git a/spec/rest_easy/attribute_spec.rb b/spec/rest_easy/attribute_spec.rb new file mode 100644 index 0000000..57b18e6 --- /dev/null +++ b/spec/rest_easy/attribute_spec.rb @@ -0,0 +1,87 @@ +# frozen_string_literal: true + +RSpec.describe RestEasy::Attribute do + describe "#validate_required!" do + let(:type) { RestEasy::Types::Coercible::String } + + context "when the attribute is not :required" do + let(:attr) { described_class.new(model_name: :name, api_name: "Name", type:) } + + it "does not raise even when the value is nil" do + expect { attr.validate_required!(nil) }.not_to raise_error + end + + it "does not raise with multiple nil values" do + expect { attr.validate_required!(nil, nil) }.not_to raise_error + end + end + + context "when the attribute is :required" do + let(:attr) { described_class.new(model_name: :name, api_name: "Name", type:, flags: [:required]) } + + it "does not raise when the single value is non-nil" do + expect { attr.validate_required!("Acme") }.not_to raise_error + end + + it "raises when the single value is nil" do + expect { attr.validate_required!(nil) }.to raise_error(RestEasy::MissingAttributeError) { |e| + expect(e.attribute_name).to eq(:name) + } + end + + it "does not raise when all of multiple values are non-nil" do + expect { attr.validate_required!("Alice", "Smith") }.not_to raise_error + end + + it "raises when any of multiple values is nil" do + expect { attr.validate_required!("Alice", nil) }.to raise_error(RestEasy::MissingAttributeError) + expect { attr.validate_required!(nil, "Smith") }.to raise_error(RestEasy::MissingAttributeError) + end + + it "does not raise when called with no values (treats empty splat as nothing-to-check)" do + # Documented behavior: callers must pass the values they intend to + # validate. An empty splat is treated as "nothing to check" rather + # than as "missing" so that future call sites with conditionally + # empty arrays don't spuriously raise. + expect { attr.validate_required! }.not_to raise_error + end + end + end + + describe "#combine?" do + let(:type) { RestEasy::Types::Coercible::String } + + it "is true when target_fields is populated and source_fields is empty" do + attr = described_class.new( + model_name: :address, api_name: "Address", type:, + flags: [:synthetic], target_fields: [:street, :city] + ) + expect(attr.combine?).to be true + end + + it "is false when source_fields is also populated (merge / split)" do + attr = described_class.new( + model_name: :full_name, api_name: "FullName", type:, + flags: [:synthetic], source_fields: [:first_name, :last_name] + ) + expect(attr.combine?).to be false + end + + it "is false for a non-synthetic standard attribute" do + attr = described_class.new(model_name: :name, api_name: "Name", type:) + expect(attr.combine?).to be false + end + + it "is false when :synthetic is set but no target_fields are present" do + # Edge case: user explicitly passes :synthetic as a flag without a + # multi-param block. The attribute is not actually combine-shaped — + # treating it as such would incorrectly skip the api_name lookup + # on parse and silently set the model slot to nil. + attr = described_class.new( + model_name: :foo, api_name: "Foo", type:, + flags: [:synthetic] + ) + expect(attr.combine?).to be false + end + end +end diff --git a/spec/rest_easy/resource_spec.rb b/spec/rest_easy/resource_spec.rb index 44ac7e9..4ea73cf 100644 --- a/spec/rest_easy/resource_spec.rb +++ b/spec/rest_easy/resource_spec.rb @@ -320,14 +320,303 @@ def serialise(model_name) end describe "attribute flags" do - it "supports :required flag" do - resource_class = Class.new(described_class) do - attr :name, String, :required + describe ":required flag" do + let(:resource_class) do + Class.new(described_class) do + attr :name, String, :required + attr :amount, Float + end end - expect { - resource_class.parse({}) - }.to raise_error(RestEasy::MissingAttributeError) + context "on parse (inbound)" do + it "raises MissingAttributeError when a :required attribute is absent from the API response" do + expect { + resource_class.parse({}) + }.to raise_error(RestEasy::MissingAttributeError) + end + + it "names the missing attribute on the error" do + expect { + resource_class.parse({}) + }.to raise_error(RestEasy::MissingAttributeError) { |e| + expect(e.attribute_name).to eq(:name) + } + end + + it "raises when a :required attribute is explicitly null in the API response" do + expect { + resource_class.parse({ "Name" => nil, "Amount" => 100.0 }) + }.to raise_error(RestEasy::MissingAttributeError) { |e| + expect(e.attribute_name).to eq(:name) + } + end + end + + context "on stub (build)" do + it "does not raise when a :required attribute is omitted" do + expect { + resource_class.stub(amount: 100.0) + }.not_to raise_error + end + + it "does not raise when a :required attribute is set to nil" do + expect { + resource_class.stub(name: nil, amount: 100.0) + }.not_to raise_error + end + end + + context "on serialise (outbound)" do + it "raises MissingAttributeError when a :required attribute is nil" do + instance = resource_class.stub(amount: 100.0) + + expect { + instance.serialise + }.to raise_error(RestEasy::MissingAttributeError) + end + + it "names the missing attribute on the error" do + instance = resource_class.stub(amount: 100.0) + + expect { + instance.serialise + }.to raise_error(RestEasy::MissingAttributeError) { |e| + expect(e.attribute_name).to eq(:name) + } + end + + it "does not raise when all :required attributes are present" do + instance = resource_class.stub(name: "Acme", amount: 100.0) + + expect { instance.serialise }.not_to raise_error + end + + it "raises when a parsed instance has been mutated to clear a :required attribute" do + instance = resource_class.parse({ "Name" => "Acme", "Amount" => 100.0 }) + mutated = instance.update(name: nil) + + expect(mutated.name).to be_nil + + expect { + mutated.serialise + }.to raise_error(RestEasy::MissingAttributeError) + end + + it "does not enforce :required on :read_only attributes (never sent)" do + read_only_required = Class.new(described_class) do + key :id, Integer, :read_only, :required + attr :name, String + end + + # No :id supplied. If :read_only didn't short-circuit the + # :required check, the stub instance would raise on serialise. + instance = read_only_required.stub(name: "Acme") + + expect { instance.serialise }.not_to raise_error + end + end + + context "on a synthetic merge attribute (many API → one model)" do + let(:merge_class) do + Class.new(described_class) do + attr :first_name, String + attr :last_name, String + + attr :full_name, String, :required do + parse { |first_name, last_name| "#{first_name} #{last_name}" } + serialise { |full_name| full_name.split(' ', 2) } + end + end + end + + it "raises on parse when any source field is absent" do + expect { + merge_class.parse({ "FirstName" => "Alice" }) + }.to raise_error(RestEasy::MissingAttributeError) { |e| + expect(e.attribute_name).to eq(:full_name) + } + end + + it "does not raise on parse when all source fields are present" do + expect { + merge_class.parse({ "FirstName" => "Alice", "LastName" => "Smith" }) + }.not_to raise_error + end + + it "raises on serialise when the merged value is nil" do + instance = merge_class.stub(first_name: "Alice", last_name: "Smith") + + expect { + instance.serialise + }.to raise_error(RestEasy::MissingAttributeError) { |e| + expect(e.attribute_name).to eq(:full_name) + } + end + + it "does not raise on serialise when the merged value is present" do + instance = merge_class.stub(full_name: "Alice Smith") + + expect { instance.serialise }.not_to raise_error + end + end + + context "on a synthetic combine attribute (many model → one API)" do + let(:combine_class) do + Class.new(described_class) do + attr :street, String + attr :city, String + + attr :address, String, :required do + serialise { |street, city| "#{street}, #{city}" } + end + end + end + + it "raises on serialise when any target field is nil" do + instance = combine_class.stub(street: "Main St") + + expect { + instance.serialise + }.to raise_error(RestEasy::MissingAttributeError) { |e| + expect(e.attribute_name).to eq(:address) + } + end + + it "does not raise on serialise when all target fields are present" do + instance = combine_class.stub(street: "Main St", city: "Stockholm") + + expect { instance.serialise }.not_to raise_error + end + + it "does not raise on parse when the combined API field is absent" do + expect { + combine_class.parse({ "Street" => "Main St", "City" => "Stockholm" }) + }.not_to raise_error + end + + it "ignores any inbound value for the combined API field (no shadowing)" do + instance = combine_class.parse({ + "Street" => "Main St", + "City" => "Stockholm", + "Address" => "Should be ignored" + }) + + expect(instance.address).to be_nil + end + end + + context "on a :read_only combine attribute" do + let(:read_only_combine) do + Class.new(described_class) do + attr :street, String + attr :city, String + + attr :address, String, :read_only, :required do + serialise { |street, city| "#{street}, #{city}" } + end + end + end + + it "does not enforce :required at serialise time (never sent)" do + instance = read_only_combine.stub + expect { instance.serialise }.not_to raise_error + end + + it "leaves the model slot nil on parse regardless of inbound value" do + instance = read_only_combine.parse({ "Address" => "ignored" }) + expect(instance.address).to be_nil + end + end + + context "on a :read_only synthetic attribute (derivational)" do + let(:derivational_class) do + Class.new(described_class) do + attr :first_name, String + attr :last_name, String + + attr :full_name, String, :read_only, :required do |first_name, last_name| + "#{first_name} #{last_name}" + end + end + end + + it "still enforces :required at parse time when a source field is absent" do + expect { + derivational_class.parse({ "FirstName" => "Alice" }) + }.to raise_error(RestEasy::MissingAttributeError) { |e| + expect(e.attribute_name).to eq(:full_name) + } + end + + it "does not enforce :required at serialise time (never sent)" do + instance = derivational_class.parse({ "FirstName" => "Alice", "LastName" => "Smith" }) + + expect { instance.serialise }.not_to raise_error + end + end + + context "on save (no HTTP traffic on incomplete payload)" do + before(:all) do + module RequiredFlagTestApi + extend RestEasy + + configure do |config| + config.base_url = "https://api.example.com/v1" + end + end + + class RequiredFlagTestApi::Invoice < RestEasy::Resource + configure do + path "invoices" + end + + key :document_number, Integer, :read_only + attr :customer_number, String, :required + attr :amount, Float + end + end + + after(:all) do + Object.send(:remove_const, :RequiredFlagTestApi) + end + + it "raises before POSTing a new record" do + instance = RequiredFlagTestApi::Invoice.stub(amount: 300.0) + + allow(RequiredFlagTestApi::Invoice).to receive(:post) + allow(RequiredFlagTestApi::Invoice).to receive(:put) + + expect { + RequiredFlagTestApi::Invoice.save(instance) + }.to raise_error(RestEasy::MissingAttributeError) { |e| + expect(e.attribute_name).to eq(:customer_number) + } + + expect(RequiredFlagTestApi::Invoice).not_to have_received(:post) + expect(RequiredFlagTestApi::Invoice).not_to have_received(:put) + end + + it "raises before PUTing an updated record" do + instance = RequiredFlagTestApi::Invoice.parse({ + "DocumentNumber" => 1, + "CustomerNumber" => "CUST-001", + "Amount" => 300.0 + }) + mutated = instance.update(customer_number: nil) + + allow(RequiredFlagTestApi::Invoice).to receive(:post) + allow(RequiredFlagTestApi::Invoice).to receive(:put) + + expect { + RequiredFlagTestApi::Invoice.save(mutated) + }.to raise_error(RestEasy::MissingAttributeError) { |e| + expect(e.attribute_name).to eq(:customer_number) + } + + expect(RequiredFlagTestApi::Invoice).not_to have_received(:post) + expect(RequiredFlagTestApi::Invoice).not_to have_received(:put) + end + end end it "supports :optional flag" do @@ -484,6 +773,11 @@ def self.serialise(street, city) expect(attr_def.target_fields).to eq([:street, :city]) end + it "auto-detects synthetic from mapper serialise parameter count" do + attr_def = @resource_class.all_attribute_definitions[:address] + expect(attr_def.synthetic?).to be true + end + it "gathers model values by param names for serialise" do instance = @resource_class.stub(street: "Main St", city: "Stockholm", address: "ignored") serialised = instance.serialise @@ -749,6 +1043,64 @@ def self.serialise(street, city) attr_def = resource_class.all_attribute_definitions[:address] expect(attr_def.target_fields).to eq([:street, :city]) end + + it "auto-detects synthetic from serialise block parameter count" do + resource_class = Class.new(described_class) do + attr :street, String + attr :city, String + + attr :address, String do + serialise { |street, city| "#{street}, #{city}" } + end + end + + attr_def = resource_class.all_attribute_definitions[:address] + expect(attr_def.synthetic?).to be true + end + + it "warns when a combine attribute also defines an explicit parse block" do + expect { + Class.new(described_class) do + attr :street, String + attr :city, String + + attr :address, String do + parse { |val| val.to_s.upcase } + serialise { |street, city| "#{street}, #{city}" } + end + end + }.to output( + /:address declares a combine pattern.*parse block.*will not run/m + ).to_stderr + end + + it "does not warn when the combine attribute has no explicit parse block" do + expect { + Class.new(described_class) do + attr :street, String + attr :city, String + + attr :address, String do + serialise { |street, city| "#{street}, #{city}" } + end + end + }.not_to output(/declares a combine pattern/).to_stderr + end + + it "does not warn for mapper-form combine attributes" do + mapper = Module.new do + def self.parse(raw_value) = raw_value + def self.serialise(street, city) = "#{street}, #{city}" + end + + expect { + Class.new(described_class) do + attr :street, String + attr :city, String + attr :address, String, mapper + end + }.not_to output(/declares a combine pattern/).to_stderr + end end # ── Resource-level hooks ───────────────────────────────────────────────