From 067b8d6df6729493aaaaf2f0893b6f7e10688fa8 Mon Sep 17 00:00:00 2001 From: Matt Date: Thu, 27 Aug 2026 16:24:15 +0200 Subject: [PATCH 1/3] fix(datasource-active-record): mark non-polymorphic has_one :through identity joins read-only A non-polymorphic has_one :through (e.g. Supplier -> Account -> AccountHistory) falls into a OneToOneSchema branch that hardcodes origin_key: association.klass. primary_key, origin_key_target: @model.primary_key -- an identity join, since OneToOneSchema has no through_collection to route through the real FK. This isn't just misleading display data: update_related.rb's PUT /relationships route and store.rb's create-with-relationships path both write through relation.origin_key, which for this shape is the FOREIGN COLLECTION'S OWN PRIMARY KEY. Trying to (re)associate the relation via the standard admin UI would attempt to overwrite (or null out) that primary key. Read/filter/sort was already unaffected and stays that way: Utils::Query resolves has_one :through joins via direct ActiveRecord reflection, not via origin_key/origin_key_target, confirmed by the existing join_to_one_optimization suite exercising this exact shape successfully. Dropping the field outright was rejected -- it would break that already-working, already-tested functionality and trip FieldValidator's "Relation not found" on any caller referencing it. Fix, across the three touched packages: - RelationSchema (toolkit) gains is_read_only: (default false, attr_reader -- nothing needs to flip it after construction). Every relation type inherits it uniformly, so no consumer has to duck-type-probe for its presence. - collection.rb sets is_read_only: true on the identity-join branch and logs a diagnostic (warn_readonly_identity_join, no removal promised -- the relation stays published on purpose). A composite primary key on either endpoint can't even be a single origin_key/origin_key_target column, so that shape is skipped entirely instead (unrepresentable_one_to_one? / warn_unrepresentable_one_to_one). - Utils::Collection.assert_writable_relation! (toolkit) is the actual enforcement: both update_related.rb (PUT) and store.rb (POST-with- relationships) call it before writing through a to-one relation's origin_key, raising ForestException when is_read_only. The published isReadOnly flag (generator_field.rb) only hides the UI control -- a direct API call bypasses it entirely without this server-side check. 197/197 -> 203/203 (active_record), 481/481 (toolkit), 1175/1175 (agent). Rubocop clean on all three. Fixes #379 Co-Authored-By: Claude Sonnet 5 --- .../resources/related/update_related.rb | 2 + .../routes/resources/store.rb | 2 + .../utils/schema/generator_field.rb | 2 +- .../resources/related/update_related_spec.rb | 20 ++++ .../routes/resources/store_spec.rb | 28 +++++ .../schema/generator_field_one_to_one_spec.rb | 30 ++++++ .../collection.rb | 39 ++++++- .../collection_spec.rb | 101 +++++++++++++++--- .../schema/relation_schema.rb | 5 +- .../schema/relations/one_to_one_schema.rb | 4 +- .../utils/collection.rb | 10 ++ .../relations/one_to_one_schema_spec.rb | 15 +++ 12 files changed, 238 insertions(+), 20 deletions(-) diff --git a/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/related/update_related.rb b/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/related/update_related.rb index 884fca495..ba48061b0 100644 --- a/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/related/update_related.rb +++ b/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/related/update_related.rb @@ -109,6 +109,8 @@ def update_polymorphic_one_to_one(relation, parent_primary_key_values, linked_pr end def update_one_to_one(relation, parent_primary_key_values, linked_primary_key_values, context) + Collection.assert_writable_relation!(relation) + origin_value = Collection.get_value(context.collection, context.caller, parent_primary_key_values, relation.origin_key_target) diff --git a/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/store.rb b/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/store.rb index 1984a82a1..c621ef377 100644 --- a/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/store.rb +++ b/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/store.rb @@ -64,6 +64,8 @@ def linked_one_to_one_relation(field, value, context) schema = context.collection.schema[:fields][field] return unless %w[OneToOne PolymorphicOneToOne].include?(schema.type) + ForestAdminDatasourceToolkit::Utils::Collection.assert_writable_relation!(schema) + id = value.dig('data', 'id') return if id.nil? diff --git a/packages/forest_admin_agent/lib/forest_admin_agent/utils/schema/generator_field.rb b/packages/forest_admin_agent/lib/forest_admin_agent/utils/schema/generator_field.rb index 0f8940649..f48624dea 100644 --- a/packages/forest_admin_agent/lib/forest_admin_agent/utils/schema/generator_field.rb +++ b/packages/forest_admin_agent/lib/forest_admin_agent/utils/schema/generator_field.rb @@ -127,7 +127,7 @@ def build_one_to_one_schema(relation, collection, foreign_collection, base_schem isFilterable: foreign_collection_filterable?(foreign_collection), isPrimaryKey: false, isRequired: false, - isReadOnly: key_field.is_read_only, + isReadOnly: relation.is_read_only || key_field.is_read_only, isSortable: target_field.is_sortable, validations: [], reference: "#{foreign_collection.name}.#{relation.origin_key_target}" diff --git a/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/related/update_related_spec.rb b/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/related/update_related_spec.rb index 029088e61..bf98e394e 100644 --- a/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/related/update_related_spec.rb +++ b/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/related/update_related_spec.rb @@ -27,6 +27,12 @@ module Related origin_key_target: 'id', foreign_collection: 'book' ), + 'locked_book' => Relations::OneToOneSchema.new( + origin_key: 'author_id', + origin_key_target: 'id', + foreign_collection: 'book', + is_read_only: true + ), 'address' => Relations::PolymorphicOneToOneSchema.new( origin_key: 'addressable_id', foreign_collection: 'address', @@ -277,6 +283,20 @@ module Related expect(result).to eq({ content: nil, status: 204 }) end + it 'refuses to write a read-only one_to_one relation (#379: it would overwrite ' \ + "the foreign collection's own primary key)" do + allow(@datasource.get_collection('book')).to receive_messages(aggregate: [{ 'value' => 1 }], update: true) + + args[:params]['collection_name'] = 'user' + args[:params]['relation_name'] = 'locked_book' + args[:params]['data'] = { 'id' => 1 } + args[:params]['id'] = 1 + + expect { update.handle_request(args) } + .to raise_error(ForestAdminDatasourceToolkit::Exceptions::ForestException, /not editable/) + expect(@datasource.get_collection('book')).not_to have_received(:update) + end + it 'call handle_request on a polymorphic_one_to_one relation' do allow(permissions).to receive(:get_scope) .and_return(Nodes::ConditionTreeLeaf.new('location', Operators::EQUAL, 'paris')) diff --git a/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/store_spec.rb b/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/store_spec.rb index 8bb9571d2..d53e80d2c 100644 --- a/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/store_spec.rb +++ b/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/store_spec.rb @@ -408,6 +408,34 @@ def respond_to?(arg) end end + describe 'with a read-only one to one relation' do + before do + @datasource.get_collection('person').schema[:fields]['locked_passport'] = + Relations::OneToOneSchema.new( + origin_key: 'person_id', + origin_key_target: 'id', + foreign_collection: 'passport', + is_read_only: true + ) + end + + it 'refuses to link it (#379: it would overwrite the foreign collection\'s own primary key)' do + args[:params][:data] = { + attributes: { 'name' => 'john' }, + relationships: { 'locked_passport' => { 'data' => { 'type' => 'passports', 'id' => 1 } } }, + type: 'persons' + } + args[:params]['collection_name'] = 'person' + allow(@datasource.get_collection('person')).to receive(:create) + allow(@datasource.get_collection('passport')).to receive(:update) + + expect { store.handle_request(args) } + .to raise_error(ForestAdminDatasourceToolkit::Exceptions::ForestException, /not editable/) + expect(@datasource.get_collection('person')).not_to have_received(:create) + expect(@datasource.get_collection('passport')).not_to have_received(:update) + end + end + describe 'with polymorphic one to one relation' do before do collection_address = build_collection( diff --git a/packages/forest_admin_agent/spec/lib/forest_admin_agent/utils/schema/generator_field_one_to_one_spec.rb b/packages/forest_admin_agent/spec/lib/forest_admin_agent/utils/schema/generator_field_one_to_one_spec.rb index 30590965d..7e3b56b87 100644 --- a/packages/forest_admin_agent/spec/lib/forest_admin_agent/utils/schema/generator_field_one_to_one_spec.rb +++ b/packages/forest_admin_agent/spec/lib/forest_admin_agent/utils/schema/generator_field_one_to_one_spec.rb @@ -63,6 +63,36 @@ module Schema ) end + it 'marks the field read-only when the relation itself is read-only, even if the ' \ + 'underlying key column is writable' do + collection_note = Collection.new(@datasource, 'Note') + collection_note.add_fields( + { + 'id' => ColumnSchema.new(column_type: 'Number', is_primary_key: true, is_read_only: false), + 'author_id' => ColumnSchema.new(column_type: 'String', is_read_only: false, is_sortable: true) + } + ) + + collection_person = Collection.new(@datasource, 'PersonReadonly') + collection_person.add_fields( + { + 'id' => ColumnSchema.new(column_type: 'Number', is_primary_key: true), + 'note' => Relations::OneToOneSchema.new( + origin_key: 'author_id', + origin_key_target: 'id', + foreign_collection: 'Note', + is_read_only: true + ) + } + ) + @datasource.add_collection(collection_note) + @datasource.add_collection(collection_person) + + schema = described_class.build_schema(@datasource.get_collection('PersonReadonly'), 'note') + + expect(schema[:isReadOnly]).to be true + end + it 'generate inverse relation' do schema = described_class.build_schema(@datasource.get_collection('Book'), 'author') diff --git a/packages/forest_admin_datasource_active_record/lib/forest_admin_datasource_active_record/collection.rb b/packages/forest_admin_datasource_active_record/lib/forest_admin_datasource_active_record/collection.rb index 674a9058d..61e3cbdda 100644 --- a/packages/forest_admin_datasource_active_record/lib/forest_admin_datasource_active_record/collection.rb +++ b/packages/forest_admin_datasource_active_record/lib/forest_admin_datasource_active_record/collection.rb @@ -114,6 +114,12 @@ def valid_many_to_many_source?(association) association.source_reflection&.belongs_to? end + # A composite primary key on either endpoint can't be used as a single OneToOneSchema + # origin_key/origin_key_target column (#379). + def unrepresentable_one_to_one?(association) + Array(association.klass.primary_key).size > 1 || Array(@model.primary_key).size > 1 + end + def build_many_to_many_field(association, through_reflection, is_polymorphic, source_polymorphic) ForestAdminDatasourceToolkit::Schema::Relations::ManyToManySchema.new( foreign_collection: format_model_name(association.klass.name), @@ -157,15 +163,19 @@ def fetch_associations if many_to_many_shape add_many_to_many_field(association, through_reflection, is_polymorphic, source_polymorphic) + elsif unrepresentable_one_to_one?(association) + warn_unrepresentable_one_to_one(association, through_reflection) else add_field( association.name.to_s, ForestAdminDatasourceToolkit::Schema::Relations::OneToOneSchema.new( foreign_collection: format_model_name(association.klass.name), origin_key: association.klass.primary_key, - origin_key_target: @model.primary_key + origin_key_target: @model.primary_key, + is_read_only: true ) ) + warn_readonly_identity_join(association, through_reflection) end elsif association.inverse_of&.polymorphic? add_field( @@ -362,6 +372,33 @@ def warn_identity_join(association, through_reflection) ) end + def warn_unrepresentable_one_to_one(association, through_reflection) + logger = ActiveSupport::Logger.new($stdout) + logger.warn( + "[ForestAdmin] ⚠️ Skipping association '#{association.name}' in model '#{@model.name}': " \ + "this has_one :through (via '#{format_model_name(through_reflection.klass.name)}') can't be " \ + 'represented as a Forest Admin one-to-one -- one of its endpoints has a composite primary key.' + ) + end + + # OneToOneSchema has no through_collection, so this publishes each side's own primary key + # as a placeholder join -- the columns exist on both sides, so GeneratorField's field + # lookups don't raise, but the join isn't meaningful data (it matches two unrelated + # sequences), which is exactly why it's marked read-only rather than left writable through + # what's really the foreign collection's own primary key (#379). + def warn_readonly_identity_join(association, through_reflection) + logger = ActiveSupport::Logger.new($stdout) + foreign_key = association.klass.primary_key + origin_key = @model.primary_key + logger.warn( + "[ForestAdmin] ⚠️ Association '#{association.name}' in model '#{@model.name}' is published " \ + "as a read-only one-to-one joining '#{format_model_name(association.klass.name)}'.'#{foreign_key}' " \ + "to this model's own '#{origin_key}': it's a has_one chained through " \ + "'#{format_model_name(through_reflection.klass.name)}', which OneToOneSchema can't express as a " \ + 'real two-hop join -- see #379 for background.' + ) + end + def warn_missing_polymorphic_columns(association) missing_columns = [] missing_columns << association.foreign_key unless schema[:fields][association.foreign_key] diff --git a/packages/forest_admin_datasource_active_record/spec/lib/forest_admin_datasource_active_record/collection_spec.rb b/packages/forest_admin_datasource_active_record/spec/lib/forest_admin_datasource_active_record/collection_spec.rb index ce7b97c70..af6ca2e22 100644 --- a/packages/forest_admin_datasource_active_record/spec/lib/forest_admin_datasource_active_record/collection_spec.rb +++ b/packages/forest_admin_datasource_active_record/spec/lib/forest_admin_datasource_active_record/collection_spec.rb @@ -62,24 +62,97 @@ module ForestAdminDatasourceActiveRecord expect(collection.schema[:fields].keys).to include('users') end - # Supplier -> Account -> AccountHistory is a has_one :through that isn't polymorphic, - # so it never reaches the many_to_many_shape guard above -- it falls into this plain - # OneToOneSchema branch instead, which hardcodes {AccountHistory,Supplier}'s own - # primary keys unconditionally, regardless of the real FK or of whether the source is - # a belongs_to (here it actually is one; account_history_id is a real column on - # accounts, just never consulted by this branch). Unrelated to #370's guard, tracked - # separately as #379; this test pins current (arguably wrong) behavior, not correct - # behavior. - it 'add has_one_through relation as a to-one (OneToOne)' do + # Supplier -> Account -> AccountHistory is a has_one :through that isn't polymorphic, so + # it never reaches the many_to_many_shape guard above -- it falls into the read-only + # identity-join branch instead (see warn_readonly_identity_join for why). The real FK + # this ignores is accounts.account_history_id. + it 'adds a has_one :through relation as a read-only to-one (OneToOne) identity join' do collection = described_class.new(datasource, Supplier) expect(collection.schema[:fields].keys).to include('account_history') field = collection.schema[:fields]['account_history'] - expect(field.class).to eq(Relations::OneToOneSchema) - expect(field.foreign_collection).to eq('AccountHistory') - expect(field.origin_key).to eq(AccountHistory.primary_key) - expect(field.origin_key_target).to eq(Supplier.primary_key) + expect(field).to have_attributes( + class: Relations::OneToOneSchema, + foreign_collection: 'AccountHistory', + origin_key: AccountHistory.primary_key, + origin_key_target: Supplier.primary_key, + is_read_only: true + ) + end + + it 'adds a second, belongs_to-chained has_one :through relation as a read-only identity ' \ + 'join too (Account -> AccountHistory -> Order)' do + collection = described_class.new(datasource, Account) + + field = collection.schema[:fields]['order'] + expect(field).to have_attributes(class: Relations::OneToOneSchema, is_read_only: true) + end + + it 'leaves a plain, non-through has_one writable' do + field = collection.schema[:fields]['user'] + + expect(field).to have_attributes(class: Relations::OneToOneSchema, is_read_only: false) + end + + it 'skips a non-polymorphic has_one :through when the root has a composite primary key' do + stub_const('CompPkLeaf', Class.new(ApplicationRecord) do + self.table_name = 'users' + self.abstract_class = true + end) + + stub_const('CompPkMiddle', Class.new(ApplicationRecord) do + self.table_name = 'cars' + self.abstract_class = true + + has_one :leaf, class_name: 'CompPkLeaf', foreign_key: :id + end) + + stub_const('CompPkRoot', Class.new(ApplicationRecord) do + self.table_name = 'categories' + self.primary_key = %w[id label] + self.abstract_class = true + + has_one :middle, class_name: 'CompPkMiddle', foreign_key: :category_id + has_one :leaf, through: :middle + end) + + collection = nil + expect do + collection = described_class.new(datasource, CompPkRoot) + end.to output(/can't be represented as a Forest Admin one-to-one/).to_stdout + + expect(collection.schema[:fields].keys).not_to include('leaf') + end + + it 'skips a non-polymorphic has_one :through when the foreign collection has a composite primary key' do + stub_const('CompPkLeaf2', Class.new(ApplicationRecord) do + self.table_name = 'cars' + self.primary_key = %w[category_id reference] + self.abstract_class = true + end) + + stub_const('CompPkMiddle2', Class.new(ApplicationRecord) do + self.table_name = 'users' + self.abstract_class = true + + has_one :leaf, class_name: 'CompPkLeaf2', foreign_key: :id + end) + + stub_const('CompPkRoot2', Class.new(ApplicationRecord) do + self.table_name = 'categories' + self.abstract_class = true + + has_one :middle, class_name: 'CompPkMiddle2', foreign_key: :category_id + has_one :leaf, through: :middle + end) + + collection = nil + expect do + collection = described_class.new(datasource, CompPkRoot2) + end.to output(/can't be represented as a Forest Admin one-to-one/).to_stdout + + expect(collection.schema[:fields].keys).not_to include('leaf') end it 'skips association when foreign_key raises an error' do @@ -278,7 +351,7 @@ module ForestAdminDatasourceActiveRecord it 'still publishes, with a diagnostic warning, a polymorphic has_one :through with the same shape' do # Solo -> Kid (polymorphic has_one) -> Detail: same bucket-B shape as above, through the # has_one branch. A non-polymorphic has_one :through isn't reached by this guard at all -- - # it falls into the pre-existing OneToOneSchema path, unguarded (tracked as #379). + # it falls into the separately-guarded read-only identity-join branch instead (#379). expect { datasource.get_collection('Solo') }.to output(/is published as a many-to-many/).to_stdout field = datasource.get_collection('Solo').schema[:fields]['detail'] diff --git a/packages/forest_admin_datasource_toolkit/lib/forest_admin_datasource_toolkit/schema/relation_schema.rb b/packages/forest_admin_datasource_toolkit/lib/forest_admin_datasource_toolkit/schema/relation_schema.rb index 18377ad74..843fe7790 100644 --- a/packages/forest_admin_datasource_toolkit/lib/forest_admin_datasource_toolkit/schema/relation_schema.rb +++ b/packages/forest_admin_datasource_toolkit/lib/forest_admin_datasource_toolkit/schema/relation_schema.rb @@ -2,11 +2,12 @@ module ForestAdminDatasourceToolkit module Schema class RelationSchema attr_accessor :foreign_collection - attr_reader :type + attr_reader :type, :is_read_only - def initialize(foreign_collection, type) + def initialize(foreign_collection, type, is_read_only: false) @foreign_collection = foreign_collection @type = type + @is_read_only = is_read_only end end end diff --git a/packages/forest_admin_datasource_toolkit/lib/forest_admin_datasource_toolkit/schema/relations/one_to_one_schema.rb b/packages/forest_admin_datasource_toolkit/lib/forest_admin_datasource_toolkit/schema/relations/one_to_one_schema.rb index a02fafe98..c68716e1e 100644 --- a/packages/forest_admin_datasource_toolkit/lib/forest_admin_datasource_toolkit/schema/relations/one_to_one_schema.rb +++ b/packages/forest_admin_datasource_toolkit/lib/forest_admin_datasource_toolkit/schema/relations/one_to_one_schema.rb @@ -4,8 +4,8 @@ module Relations class OneToOneSchema < RelationSchema attr_accessor :origin_key, :origin_key_target - def initialize(origin_key:, origin_key_target:, foreign_collection:) - super(foreign_collection, 'OneToOne') + def initialize(origin_key:, origin_key_target:, foreign_collection:, is_read_only: false) + super(foreign_collection, 'OneToOne', is_read_only: is_read_only) @origin_key = origin_key @origin_key_target = origin_key_target end diff --git a/packages/forest_admin_datasource_toolkit/lib/forest_admin_datasource_toolkit/utils/collection.rb b/packages/forest_admin_datasource_toolkit/lib/forest_admin_datasource_toolkit/utils/collection.rb index 3e5e84c1a..00b546f68 100644 --- a/packages/forest_admin_datasource_toolkit/lib/forest_admin_datasource_toolkit/utils/collection.rb +++ b/packages/forest_admin_datasource_toolkit/lib/forest_admin_datasource_toolkit/utils/collection.rb @@ -78,6 +78,16 @@ def self.get_field_schema(collection, field_name) ) end + # A read-only relation (e.g. #379's has_one :through identity join, which has no real + # join column and would corrupt the foreign collection's own primary key if written to) + # only hides its edit control in the UI via isReadOnly -- a direct write still has to be + # blocked here, at every route that can write a to-one relation's origin_key. + def self.assert_writable_relation!(relation) + return unless relation.is_read_only + + raise ForestException, "Field #{relation.foreign_collection} is not editable" + end + def self.get_value(collection, caller, primary_key_values, field) if primary_key_values.is_a? Array index = Schema.primary_keys(collection).index(field) diff --git a/packages/forest_admin_datasource_toolkit/spec/lib/forest_admin_datasource_toolkit/schema/relations/one_to_one_schema_spec.rb b/packages/forest_admin_datasource_toolkit/spec/lib/forest_admin_datasource_toolkit/schema/relations/one_to_one_schema_spec.rb index 6cf05653b..57762b62b 100644 --- a/packages/forest_admin_datasource_toolkit/spec/lib/forest_admin_datasource_toolkit/schema/relations/one_to_one_schema_spec.rb +++ b/packages/forest_admin_datasource_toolkit/spec/lib/forest_admin_datasource_toolkit/schema/relations/one_to_one_schema_spec.rb @@ -17,6 +17,21 @@ module Relations it { expect(relation.origin_key).to eq 'origin_key' } it { expect(relation.origin_key_target).to eq 'origin_key_target' } it { expect(relation.foreign_collection).to eq 'foreign_collection' } + it { expect(relation.is_read_only).to be false } + end + + describe 'is_read_only' do + it 'defaults to false when not given' do + expect(described_class.new(origin_key: 'a', origin_key_target: 'b', foreign_collection: 'c').is_read_only) + .to be false + end + + it 'can be set explicitly' do + relation = described_class.new(origin_key: 'a', origin_key_target: 'b', foreign_collection: 'c', + is_read_only: true) + + expect(relation.is_read_only).to be true + end end describe 'setters' do From 17df92556d23a1b31ea2e13f8f98a3ba3cff17d8 Mon Sep 17 00:00:00 2001 From: Matt Date: Fri, 28 Aug 2026 09:55:06 +0200 Subject: [PATCH 2/3] fix(datasource-active-record): fix RPC hydration, guard ordering, error status and message accuracy for the #379 read-only guard Addresses christophebrun-forest's review on PR #381, 5 findings: 1. Blocker: RelationSchema's base initializer now sets is_read_only on every subclass instance, so RPC serialization (instance_values/as_json) put the key on the wire for ALL relation types -- but only OneToOneSchema's constructor forwarded it. Any other type crashed RPC hydration with "unknown keyword: :is_read_only". Fixed both ends: every RelationSchema subclass now accepts and forwards is_read_only:, and forest_admin_datasource_rpc/collection.rb no longer deletes specific keys by name -- it filters the payload down to whatever the target class's constructor actually declares, so a future attribute added to one side doesn't need a matching manual delete on the other. Locked in by a round-trip test asserting is_read_only=true survives hydration for OneToMany and ManyToOne, not just OneToOne. 2. Blocker: store.rb's assert_writable_relation! ran before `return if id.nil?`, so a read-only relation carrying `"data": null` (an explicitly supported, already-tested no-op payload) now raised instead of being ignored like a writable one. Moved below the nil check -- only actual link attempts are blocked. 3. assert_writable_relation! raised a bare ForestException, which ErrorTranslator maps to HTTP 500. Rejecting a write to a non-editable field is a client error: switched to ValidationError (400), which IS-A ForestException so existing call sites are unaffected. 4. The rejection message named the foreign collection ("Field passport is not editable") instead of the actual field the caller tried to write ("locked_passport"). Both call sites now pass the field/relation name through. 5. unrepresentable_one_to_one? used Array(pk).size > 1, only catching a literal composite Array. Its many_to_many sibling already established the right criterion: check column membership instead, which also catches a primary_key flattened into a mangled string and a custom primary_key pointing at a nonexistent column -- both of which would otherwise hit nil.column_type in build_one_to_one_schema. Also closes a coverage gap qlty flagged (assert_writable_relation! had no direct unit test, only indirect coverage through the two routes). 483/483 (toolkit), 203/203 (active_record), 1176/1176 (agent), 169/169 (rpc). Rubocop clean on all four packages. Every fix re-verified non-vacuous by hand-reverting and confirming the right failure. Co-Authored-By: Claude Sonnet 5 --- .../resources/related/update_related.rb | 7 +-- .../routes/resources/store.rb | 4 +- .../resources/related/update_related_spec.rb | 3 +- .../routes/resources/store_spec.rb | 19 ++++++- .../collection.rb | 9 ++-- .../forest_admin_datasource_rpc/collection.rb | 54 ++++++++++--------- .../collection_spec.rb | 19 +++++++ .../spec/shared/schema.rb | 6 ++- .../schema/relations/many_to_many_schema.rb | 5 +- .../schema/relations/many_to_one_schema.rb | 4 +- .../schema/relations/one_to_many_schema.rb | 4 +- .../polymorphic_one_to_many_schema.rb | 4 +- .../polymorphic_one_to_one_schema.rb | 4 +- .../utils/collection.rb | 8 +-- .../utils/collection_spec.rb | 16 ++++++ 15 files changed, 116 insertions(+), 50 deletions(-) diff --git a/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/related/update_related.rb b/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/related/update_related.rb index ba48061b0..d58739804 100644 --- a/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/related/update_related.rb +++ b/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/related/update_related.rb @@ -40,7 +40,8 @@ def handle_request(args = {}) when 'PolymorphicManyToOne' update_polymorphic_many_to_one(relation, parent_primary_key_values, linked_primary_key_values, context) when 'OneToOne' - update_one_to_one(relation, parent_primary_key_values, linked_primary_key_values, context) + update_one_to_one(args[:params]['relation_name'], relation, parent_primary_key_values, + linked_primary_key_values, context) when 'PolymorphicOneToOne' update_polymorphic_one_to_one(relation, parent_primary_key_values, linked_primary_key_values, context) end @@ -108,8 +109,8 @@ def update_polymorphic_one_to_one(relation, parent_primary_key_values, linked_pr create_new_polymorphic_one_to_one_relationship(relation, origin_value, linked_primary_key_values, context) end - def update_one_to_one(relation, parent_primary_key_values, linked_primary_key_values, context) - Collection.assert_writable_relation!(relation) + def update_one_to_one(field_name, relation, parent_primary_key_values, linked_primary_key_values, context) + Collection.assert_writable_relation!(field_name, relation) origin_value = Collection.get_value(context.collection, context.caller, parent_primary_key_values, relation.origin_key_target) diff --git a/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/store.rb b/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/store.rb index c621ef377..9bbe33b61 100644 --- a/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/store.rb +++ b/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/store.rb @@ -64,11 +64,11 @@ def linked_one_to_one_relation(field, value, context) schema = context.collection.schema[:fields][field] return unless %w[OneToOne PolymorphicOneToOne].include?(schema.type) - ForestAdminDatasourceToolkit::Utils::Collection.assert_writable_relation!(schema) - id = value.dig('data', 'id') return if id.nil? + ForestAdminDatasourceToolkit::Utils::Collection.assert_writable_relation!(field, schema) + { schema: schema, foreign_collection: context.datasource.get_collection(schema.foreign_collection), diff --git a/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/related/update_related_spec.rb b/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/related/update_related_spec.rb index bf98e394e..668e4f64e 100644 --- a/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/related/update_related_spec.rb +++ b/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/related/update_related_spec.rb @@ -293,7 +293,8 @@ module Related args[:params]['id'] = 1 expect { update.handle_request(args) } - .to raise_error(ForestAdminDatasourceToolkit::Exceptions::ForestException, /not editable/) + .to raise_error(ForestAdminDatasourceToolkit::Exceptions::ValidationError, + 'Field locked_book is not editable') expect(@datasource.get_collection('book')).not_to have_received(:update) end diff --git a/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/store_spec.rb b/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/store_spec.rb index d53e80d2c..bdfe997f1 100644 --- a/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/store_spec.rb +++ b/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/store_spec.rb @@ -430,10 +430,27 @@ def respond_to?(arg) allow(@datasource.get_collection('passport')).to receive(:update) expect { store.handle_request(args) } - .to raise_error(ForestAdminDatasourceToolkit::Exceptions::ForestException, /not editable/) + .to raise_error(ForestAdminDatasourceToolkit::Exceptions::ValidationError, + 'Field locked_passport is not editable') expect(@datasource.get_collection('person')).not_to have_received(:create) expect(@datasource.get_collection('passport')).not_to have_received(:update) end + + it 'ignores a read-only one to one relationship carrying no data, same as a writable one' do + args[:params][:data] = { + attributes: { 'name' => 'john' }, + relationships: { 'locked_passport' => { 'data' => nil } }, + type: 'persons' + } + args[:params]['collection_name'] = 'person' + allow(@datasource.get_collection('person')).to receive_messages( + create: { 'id' => 1, 'name' => 'john' }, + list: [{ 'id' => 1, 'name' => 'john' }] + ) + + expect { store.handle_request(args) }.not_to raise_error + expect(@datasource.get_collection('passport')).not_to have_received(:update) + end end describe 'with polymorphic one to one relation' do diff --git a/packages/forest_admin_datasource_active_record/lib/forest_admin_datasource_active_record/collection.rb b/packages/forest_admin_datasource_active_record/lib/forest_admin_datasource_active_record/collection.rb index 61e3cbdda..66a88ffb7 100644 --- a/packages/forest_admin_datasource_active_record/lib/forest_admin_datasource_active_record/collection.rb +++ b/packages/forest_admin_datasource_active_record/lib/forest_admin_datasource_active_record/collection.rb @@ -114,10 +114,13 @@ def valid_many_to_many_source?(association) association.source_reflection&.belongs_to? end - # A composite primary key on either endpoint can't be used as a single OneToOneSchema - # origin_key/origin_key_target column (#379). + # Same criterion as unrepresentable_many_to_many?: a composite primary key can arrive as a + # real Array or already flattened by Rails into a mangled String -- checking column + # membership catches both forms, plus a custom primary_key pointing at a non-column, all of + # which would otherwise hit nil.column_type in build_one_to_one_schema (#379). def unrepresentable_one_to_one?(association) - Array(association.klass.primary_key).size > 1 || Array(@model.primary_key).size > 1 + !association.klass.column_names.include?(association.klass.primary_key) || + !@model.column_names.include?(@model.primary_key) end def build_many_to_many_field(association, through_reflection, is_polymorphic, source_polymorphic) diff --git a/packages/forest_admin_datasource_rpc/lib/forest_admin_datasource_rpc/collection.rb b/packages/forest_admin_datasource_rpc/lib/forest_admin_datasource_rpc/collection.rb index acf0bbb86..1041fd740 100644 --- a/packages/forest_admin_datasource_rpc/lib/forest_admin_datasource_rpc/collection.rb +++ b/packages/forest_admin_datasource_rpc/lib/forest_admin_datasource_rpc/collection.rb @@ -28,34 +28,26 @@ def initialize(datasource, name, schema) @schema[:aggregation_capabilities] = schema[:aggregation_capabilities] if schema[:aggregation_capabilities] end + FIELD_CLASSES = { + 'Column' => ForestAdminDatasourceToolkit::Schema::ColumnSchema, + 'ManyToMany' => ForestAdminDatasourceToolkit::Schema::Relations::ManyToManySchema, + 'OneToMany' => ForestAdminDatasourceToolkit::Schema::Relations::OneToManySchema, + 'ManyToOne' => ForestAdminDatasourceToolkit::Schema::Relations::ManyToOneSchema, + 'OneToOne' => ForestAdminDatasourceToolkit::Schema::Relations::OneToOneSchema, + 'PolymorphicManyToOne' => ForestAdminDatasourceToolkit::Schema::Relations::PolymorphicManyToOneSchema, + 'PolymorphicOneToMany' => ForestAdminDatasourceToolkit::Schema::Relations::PolymorphicOneToManySchema, + 'PolymorphicOneToOne' => ForestAdminDatasourceToolkit::Schema::Relations::PolymorphicOneToOneSchema + }.freeze + + KEYWORD_PARAM_KINDS = %i[key keyreq].freeze + def add_fields(fields) fields.each do |field_name, schema| field_name = field_name.to_s - type = schema[:type] - schema.delete(:type) - # remove these - schema.delete(:allow_null) - case type - when 'Column' - add_field(field_name, ForestAdminDatasourceToolkit::Schema::ColumnSchema.new(**schema)) - when 'ManyToMany' - add_field(field_name, ForestAdminDatasourceToolkit::Schema::Relations::ManyToManySchema.new(**schema)) - when 'OneToMany' - add_field(field_name, ForestAdminDatasourceToolkit::Schema::Relations::OneToManySchema.new(**schema)) - when 'ManyToOne' - add_field(field_name, ForestAdminDatasourceToolkit::Schema::Relations::ManyToOneSchema.new(**schema)) - when 'OneToOne' - add_field(field_name, ForestAdminDatasourceToolkit::Schema::Relations::OneToOneSchema.new(**schema)) - when 'PolymorphicManyToOne' - add_field(field_name, - ForestAdminDatasourceToolkit::Schema::Relations::PolymorphicManyToOneSchema.new(**schema)) - when 'PolymorphicOneToMany' - add_field(field_name, - ForestAdminDatasourceToolkit::Schema::Relations::PolymorphicOneToManySchema.new(**schema)) - when 'PolymorphicOneToOne' - add_field(field_name, - ForestAdminDatasourceToolkit::Schema::Relations::PolymorphicOneToOneSchema.new(**schema)) - end + klass = FIELD_CLASSES[schema[:type]] + next unless klass + + add_field(field_name, klass.new(**accepted_keywords(klass, schema))) end end @@ -175,6 +167,18 @@ def build_params(extra_params = {}) @base_params.merge(extra_params) end + # Filters a deserialized field payload down to whatever klass#initialize actually accepts, + # instead of deleting specific keys by name -- an attribute the RPC agent's schema + # serialization adds later (or a version mismatch between agents) shouldn't crash + # hydration on this side. + def accepted_keywords(klass, schema) + accepted = klass.instance_method(:initialize).parameters.filter_map do |kind, name| + name if KEYWORD_PARAM_KINDS.include?(kind) + end + + schema.slice(*accepted) + end + def encode_form_data(data) data.to_h do |key, value| if value.is_a?(Hash) && value.key?('buffer') diff --git a/packages/forest_admin_datasource_rpc/spec/lib/forest_admin_datasource_rpc/collection_spec.rb b/packages/forest_admin_datasource_rpc/spec/lib/forest_admin_datasource_rpc/collection_spec.rb index fff27c825..e0824d5af 100644 --- a/packages/forest_admin_datasource_rpc/spec/lib/forest_admin_datasource_rpc/collection_spec.rb +++ b/packages/forest_admin_datasource_rpc/spec/lib/forest_admin_datasource_rpc/collection_spec.rb @@ -28,6 +28,25 @@ module ForestAdminDatasourceRpc expect(collection.instance_variable_get(:@client)).to eq(datasource.shared_rpc_client) end + it 'hydrates every relation type from a payload carrying is_read_only=true, not just OneToOne' do + # A schema attribute added to RelationSchema (the shared base) gets serialized for + # every relation type, but only reaches a subclass's own attr_accessor if that + # subclass's initializer also accepts it -- fixtures pin true (not the false default) + # so a silently-dropped attribute (accepted_keywords filtering it out because the + # subclass doesn't declare it) shows up as a wrong value, not just a lucky match. + manufacturer_collection = datasource.get_collection('Manufacturer') + product_collection = datasource.get_collection('Product') + + expect(manufacturer_collection.schema[:fields]['products']).to have_attributes( + class: ForestAdminDatasourceToolkit::Schema::Relations::OneToManySchema, + is_read_only: true + ) + expect(product_collection.schema[:fields]['manufacturer']).to have_attributes( + class: ForestAdminDatasourceToolkit::Schema::Relations::ManyToOneSchema, + is_read_only: true + ) + end + context 'when the schema carries action static_form values' do let(:actions_introspection) do { diff --git a/packages/forest_admin_datasource_rpc/spec/shared/schema.rb b/packages/forest_admin_datasource_rpc/spec/shared/schema.rb index d9073c862..ed71799da 100644 --- a/packages/forest_admin_datasource_rpc/spec/shared/schema.rb +++ b/packages/forest_admin_datasource_rpc/spec/shared/schema.rb @@ -74,7 +74,8 @@ foreign_collection: 'Product', type: 'OneToMany', origin_key: 'manufacturer_id', - origin_key_target: 'id' + origin_key_target: 'id', + is_read_only: true } }, countable: true, @@ -180,7 +181,8 @@ foreign_collection: 'Manufacturer', type: 'ManyToOne', foreign_key: 'manufacturer_id', - foreign_key_target: 'id' + foreign_key_target: 'id', + is_read_only: true } }, countable: true, diff --git a/packages/forest_admin_datasource_toolkit/lib/forest_admin_datasource_toolkit/schema/relations/many_to_many_schema.rb b/packages/forest_admin_datasource_toolkit/lib/forest_admin_datasource_toolkit/schema/relations/many_to_many_schema.rb index 957f88cfb..539b60936 100644 --- a/packages/forest_admin_datasource_toolkit/lib/forest_admin_datasource_toolkit/schema/relations/many_to_many_schema.rb +++ b/packages/forest_admin_datasource_toolkit/lib/forest_admin_datasource_toolkit/schema/relations/many_to_many_schema.rb @@ -15,9 +15,10 @@ def initialize( origin_type_field: nil, origin_type_value: nil, foreign_type_field: nil, - foreign_type_value: nil + foreign_type_value: nil, + is_read_only: false ) - super(foreign_collection, 'ManyToMany') + super(foreign_collection, 'ManyToMany', is_read_only: is_read_only) @origin_key = origin_key @origin_key_target = origin_key_target @through_collection = through_collection diff --git a/packages/forest_admin_datasource_toolkit/lib/forest_admin_datasource_toolkit/schema/relations/many_to_one_schema.rb b/packages/forest_admin_datasource_toolkit/lib/forest_admin_datasource_toolkit/schema/relations/many_to_one_schema.rb index 03d5f3371..7dd64fba1 100644 --- a/packages/forest_admin_datasource_toolkit/lib/forest_admin_datasource_toolkit/schema/relations/many_to_one_schema.rb +++ b/packages/forest_admin_datasource_toolkit/lib/forest_admin_datasource_toolkit/schema/relations/many_to_one_schema.rb @@ -4,8 +4,8 @@ module Relations class ManyToOneSchema < RelationSchema attr_accessor :foreign_key, :foreign_key_target - def initialize(foreign_key:, foreign_key_target:, foreign_collection:) - super(foreign_collection, 'ManyToOne') + def initialize(foreign_key:, foreign_key_target:, foreign_collection:, is_read_only: false) + super(foreign_collection, 'ManyToOne', is_read_only: is_read_only) @foreign_key = foreign_key @foreign_key_target = foreign_key_target end diff --git a/packages/forest_admin_datasource_toolkit/lib/forest_admin_datasource_toolkit/schema/relations/one_to_many_schema.rb b/packages/forest_admin_datasource_toolkit/lib/forest_admin_datasource_toolkit/schema/relations/one_to_many_schema.rb index f7c761a6b..78adf8d5f 100644 --- a/packages/forest_admin_datasource_toolkit/lib/forest_admin_datasource_toolkit/schema/relations/one_to_many_schema.rb +++ b/packages/forest_admin_datasource_toolkit/lib/forest_admin_datasource_toolkit/schema/relations/one_to_many_schema.rb @@ -4,8 +4,8 @@ module Relations class OneToManySchema < RelationSchema attr_accessor :origin_key, :origin_key_target - def initialize(origin_key:, origin_key_target:, foreign_collection:) - super(foreign_collection, 'OneToMany') + def initialize(origin_key:, origin_key_target:, foreign_collection:, is_read_only: false) + super(foreign_collection, 'OneToMany', is_read_only: is_read_only) @origin_key = origin_key @origin_key_target = origin_key_target end diff --git a/packages/forest_admin_datasource_toolkit/lib/forest_admin_datasource_toolkit/schema/relations/polymorphic_one_to_many_schema.rb b/packages/forest_admin_datasource_toolkit/lib/forest_admin_datasource_toolkit/schema/relations/polymorphic_one_to_many_schema.rb index 6681c5208..464636e77 100644 --- a/packages/forest_admin_datasource_toolkit/lib/forest_admin_datasource_toolkit/schema/relations/polymorphic_one_to_many_schema.rb +++ b/packages/forest_admin_datasource_toolkit/lib/forest_admin_datasource_toolkit/schema/relations/polymorphic_one_to_many_schema.rb @@ -6,8 +6,8 @@ class PolymorphicOneToManySchema < RelationSchema attr_reader :origin_key_target, :origin_type_field, :cascade_on_delete def initialize(origin_key:, origin_key_target:, foreign_collection:, origin_type_field:, origin_type_value:, - cascade_on_delete: false) - super(foreign_collection, 'PolymorphicOneToMany') + cascade_on_delete: false, is_read_only: false) + super(foreign_collection, 'PolymorphicOneToMany', is_read_only: is_read_only) @origin_key = origin_key @origin_key_target = origin_key_target @origin_type_field = origin_type_field diff --git a/packages/forest_admin_datasource_toolkit/lib/forest_admin_datasource_toolkit/schema/relations/polymorphic_one_to_one_schema.rb b/packages/forest_admin_datasource_toolkit/lib/forest_admin_datasource_toolkit/schema/relations/polymorphic_one_to_one_schema.rb index 8d939402e..499a04039 100644 --- a/packages/forest_admin_datasource_toolkit/lib/forest_admin_datasource_toolkit/schema/relations/polymorphic_one_to_one_schema.rb +++ b/packages/forest_admin_datasource_toolkit/lib/forest_admin_datasource_toolkit/schema/relations/polymorphic_one_to_one_schema.rb @@ -6,8 +6,8 @@ class PolymorphicOneToOneSchema < RelationSchema attr_reader :origin_key_target, :origin_type_field, :cascade_on_delete def initialize(origin_key:, origin_key_target:, foreign_collection:, origin_type_field:, origin_type_value:, - cascade_on_delete: false) - super(foreign_collection, 'PolymorphicOneToOne') + cascade_on_delete: false, is_read_only: false) + super(foreign_collection, 'PolymorphicOneToOne', is_read_only: is_read_only) @origin_key = origin_key @origin_key_target = origin_key_target @origin_type_field = origin_type_field diff --git a/packages/forest_admin_datasource_toolkit/lib/forest_admin_datasource_toolkit/utils/collection.rb b/packages/forest_admin_datasource_toolkit/lib/forest_admin_datasource_toolkit/utils/collection.rb index 00b546f68..7963ac7f8 100644 --- a/packages/forest_admin_datasource_toolkit/lib/forest_admin_datasource_toolkit/utils/collection.rb +++ b/packages/forest_admin_datasource_toolkit/lib/forest_admin_datasource_toolkit/utils/collection.rb @@ -81,11 +81,13 @@ def self.get_field_schema(collection, field_name) # A read-only relation (e.g. #379's has_one :through identity join, which has no real # join column and would corrupt the foreign collection's own primary key if written to) # only hides its edit control in the UI via isReadOnly -- a direct write still has to be - # blocked here, at every route that can write a to-one relation's origin_key. - def self.assert_writable_relation!(relation) + # blocked here, at every route that can write a to-one relation's origin_key. ValidationError + # (not a bare ForestException) so this maps to a 400, not a 500 -- rejecting a write to a + # non-editable field is a client error, not a server failure. + def self.assert_writable_relation!(field_name, relation) return unless relation.is_read_only - raise ForestException, "Field #{relation.foreign_collection} is not editable" + raise ValidationError, "Field #{field_name} is not editable" end def self.get_value(collection, caller, primary_key_values, field) diff --git a/packages/forest_admin_datasource_toolkit/spec/lib/forest_admin_datasource_toolkit/utils/collection_spec.rb b/packages/forest_admin_datasource_toolkit/spec/lib/forest_admin_datasource_toolkit/utils/collection_spec.rb index 6fa476765..fb7d88535 100644 --- a/packages/forest_admin_datasource_toolkit/spec/lib/forest_admin_datasource_toolkit/utils/collection_spec.rb +++ b/packages/forest_admin_datasource_toolkit/spec/lib/forest_admin_datasource_toolkit/utils/collection_spec.rb @@ -326,6 +326,22 @@ module Utils ForestAdminDatasourceToolkit::Components::Query::Aggregation.new(operation: 'Count'))).to eq(1) end end + + describe 'assert_writable_relation!' do + it 'does nothing for a writable relation' do + relation = Relations::OneToOneSchema.new(origin_key: 'a', origin_key_target: 'id', foreign_collection: 'b') + + expect { described_class.assert_writable_relation!('field_name', relation) }.not_to raise_error + end + + it 'raises a ValidationError naming the field for a read-only relation' do + relation = Relations::OneToOneSchema.new(origin_key: 'a', origin_key_target: 'id', foreign_collection: 'b', + is_read_only: true) + + expect { described_class.assert_writable_relation!('locked_field', relation) } + .to raise_error(ValidationError, 'Field locked_field is not editable') + end + end end end end From c5c51f2ccb4f6aacce24a0706d1ae956fff2c7ca Mon Sep 17 00:00:00 2001 From: Matt Date: Fri, 28 Aug 2026 10:06:22 +0200 Subject: [PATCH 3/3] fix(datasource-active-record): enforce is_read_only on every relation mutation route, not just OneToOne Addresses a Macroscope finding on PR #381: is_read_only now lives on the shared RelationSchema base, so any relation type can be marked read-only -- but only update_one_to_one actually enforced it. ManyToOne, PolymorphicManyToOne, PolymorphicOneToOne (update_related.rb), and every to-many type via AssociateRelated/DissociateRelated remained writable through direct API calls regardless of the flag. - update_related.rb: moved the check to handle_request, once, before dispatch -- covers every branch instead of just OneToOne. - associate_related.rb / dissociate_related.rb: added the same check right after the relation is resolved, before any read or write. - Collection.assert_writable_relation! guards with relation.respond_to?(:is_read_only) again, but for a different, narrower reason than before: PolymorphicManyToOneSchema doesn't inherit RelationSchema at all (unlike every other relation type), so it genuinely has no such concept -- not a version-skew gap this time. Added regression tests for ManyToOne (update_related), and ManyToMany (associate_related, dissociate_related) -- the three previously-unguarded paths -- each verified to fail without the fix. 483/483 (toolkit), 203/203 (active_record), 1179/1179 (agent), 169/169 (rpc). Rubocop clean on all four. Co-Authored-By: Claude Sonnet 5 --- .../resources/related/associate_related.rb | 1 + .../resources/related/dissociate_related.rb | 1 + .../resources/related/update_related.rb | 12 ++++++---- .../related/associate_related_spec.rb | 22 ++++++++++++++++++ .../related/dissociate_related_spec.rb | 23 +++++++++++++++++++ .../resources/related/update_related_spec.rb | 21 +++++++++++++++++ .../utils/collection.rb | 12 ++++++---- 7 files changed, 83 insertions(+), 9 deletions(-) diff --git a/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/related/associate_related.rb b/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/related/associate_related.rb index f122518c9..299d4e150 100644 --- a/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/related/associate_related.rb +++ b/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/related/associate_related.rb @@ -28,6 +28,7 @@ def handle_request(args = {}) target_primary_key_values = Utils::Id.unpack_id(context.child_collection, args[:params]['data'][0]['id'], with_key: true) relation = Schema.get_to_many_relation(context.collection, args[:params]['relation_name']) + Collection.assert_writable_relation!(args[:params]['relation_name'], relation) case relation.type when 'OneToMany' diff --git a/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/related/dissociate_related.rb b/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/related/dissociate_related.rb index 31867aa8b..fe003eaff 100644 --- a/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/related/dissociate_related.rb +++ b/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/related/dissociate_related.rb @@ -34,6 +34,7 @@ def handle_request(args = {}) filter = get_base_foreign_filter(args, context) relation = Schema.get_to_many_relation(context.collection, args[:params]['relation_name']) + Collection.assert_writable_relation!(args[:params]['relation_name'], relation) relation_name = args[:params]['relation_name'] options = { diff --git a/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/related/update_related.rb b/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/related/update_related.rb index d58739804..32c8e3b8d 100644 --- a/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/related/update_related.rb +++ b/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/related/update_related.rb @@ -28,6 +28,11 @@ def handle_request(args = {}) # Must run before unpack_id: unauthorized callers get a 403, not a validation error context.permissions.can?(:edit, mutated_collection(relation, context)) + # Every relation type this route writes through can be read-only (#379's + # OneToOne identity join is the reason this exists, but the flag itself sits on + # the shared RelationSchema, so enforce it once here rather than per-branch). + Collection.assert_writable_relation!(args[:params]['relation_name'], relation) + parent_primary_key_values = Utils::Id.unpack_id(context.collection, args[:params]['id']) linked_primary_key_values = if (id = args.dig(:params, 'data', 'id')) @@ -40,8 +45,7 @@ def handle_request(args = {}) when 'PolymorphicManyToOne' update_polymorphic_many_to_one(relation, parent_primary_key_values, linked_primary_key_values, context) when 'OneToOne' - update_one_to_one(args[:params]['relation_name'], relation, parent_primary_key_values, - linked_primary_key_values, context) + update_one_to_one(relation, parent_primary_key_values, linked_primary_key_values, context) when 'PolymorphicOneToOne' update_polymorphic_one_to_one(relation, parent_primary_key_values, linked_primary_key_values, context) end @@ -109,9 +113,7 @@ def update_polymorphic_one_to_one(relation, parent_primary_key_values, linked_pr create_new_polymorphic_one_to_one_relationship(relation, origin_value, linked_primary_key_values, context) end - def update_one_to_one(field_name, relation, parent_primary_key_values, linked_primary_key_values, context) - Collection.assert_writable_relation!(field_name, relation) - + def update_one_to_one(relation, parent_primary_key_values, linked_primary_key_values, context) origin_value = Collection.get_value(context.collection, context.caller, parent_primary_key_values, relation.origin_key_target) diff --git a/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/related/associate_related_spec.rb b/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/related/associate_related_spec.rb index 1a0ebe732..b1f13d9b0 100644 --- a/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/related/associate_related_spec.rb +++ b/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/related/associate_related_spec.rb @@ -81,6 +81,15 @@ module Related origin_key_target: 'id', origin_type_field: 'addressable_type', origin_type_value: 'user' + ), + 'locked_addresses' => Relations::ManyToManySchema.new( + foreign_key: 'address_id', + foreign_collection: 'address', + foreign_key_target: 'id', + through_collection: 'address_user', + origin_key: 'user_id', + origin_key_target: 'id', + is_read_only: true ) } } @@ -191,6 +200,19 @@ module Related end end + it 'refuses to associate a read-only relation (any type, not just OneToOne, can be ' \ + 'marked is_read_only now that it lives on the shared RelationSchema)' do + args[:params]['relation_name'] = 'locked_addresses' + args[:params]['data'] = [{ 'id' => 1 }] + args[:params]['id'] = 1 + allow(@datasource.get_collection('address_user')).to receive(:create) + + expect { associate.handle_request(args) } + .to raise_error(ForestAdminDatasourceToolkit::Exceptions::ValidationError, + 'Field locked_addresses is not editable') + expect(@datasource.get_collection('address_user')).not_to have_received(:create) + end + context 'when call on one to many relation' do before do args[:params]['relation_name'] = 'address_users' diff --git a/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/related/dissociate_related_spec.rb b/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/related/dissociate_related_spec.rb index bfe2f3113..bcf48f52b 100644 --- a/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/related/dissociate_related_spec.rb +++ b/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/related/dissociate_related_spec.rb @@ -41,6 +41,15 @@ module Related origin_key_target: 'id', origin_type_field: 'addressable_type', origin_type_value: 'user' + ), + 'locked_addresses' => Relations::ManyToManySchema.new( + foreign_key: 'address_id', + foreign_collection: 'address', + foreign_key_target: 'id', + through_collection: 'address_user', + origin_key: 'user_id', + origin_key_target: 'id', + is_read_only: true ) } ) @@ -240,6 +249,20 @@ module Related ) end + it 'refuses to dissociate a read-only relation (any type, not just OneToOne, can be ' \ + 'marked is_read_only now that it lives on the shared RelationSchema)' do + allow(@datasource.get_collection('address_user')).to receive(:delete) + + args[:params]['relation_name'] = 'locked_addresses' + args[:params][:data] = [{ 'id' => 1 }] + args[:params]['id'] = 1 + + expect { dissociate.handle_request(args) } + .to raise_error(ForestAdminDatasourceToolkit::Exceptions::ValidationError, + 'Field locked_addresses is not editable') + expect(@datasource.get_collection('address_user')).not_to have_received(:delete) + end + it 'call dissociate_or_delete_many_to_many without deletion' do allow(@datasource.get_collection('address_user')) .to receive_messages(list: [AddressUser.new(1, 1, 1)], delete: true) diff --git a/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/related/update_related_spec.rb b/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/related/update_related_spec.rb index 668e4f64e..0f238076a 100644 --- a/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/related/update_related_spec.rb +++ b/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/related/update_related_spec.rb @@ -53,6 +53,12 @@ module Related foreign_key: 'author_id', foreign_key_target: 'id', foreign_collection: 'user' + ), + 'locked_author' => Relations::ManyToOneSchema.new( + foreign_key: 'author_id', + foreign_key_target: 'id', + foreign_collection: 'user', + is_read_only: true ) } ) @@ -163,6 +169,21 @@ module Related expect(result).to eq({ content: nil, status: 204 }) end + it 'refuses to write a read-only many_to_one relation (not just OneToOne can be ' \ + 'marked is_read_only now that it lives on the shared RelationSchema)' do + allow(@datasource.get_collection('book')).to receive(:update) + + args[:params]['collection_name'] = 'book' + args[:params]['relation_name'] = 'locked_author' + args[:params]['data'] = { 'id' => 1 } + args[:params]['id'] = 1 + + expect { update.handle_request(args) } + .to raise_error(ForestAdminDatasourceToolkit::Exceptions::ValidationError, + 'Field locked_author is not editable') + expect(@datasource.get_collection('book')).not_to have_received(:update) + end + it 'call handle_request on a polymorphic_many_to_one relation' do allow(permissions).to receive(:get_scope) .and_return(Nodes::ConditionTreeLeaf.new('location', Operators::EQUAL, 'paris')) diff --git a/packages/forest_admin_datasource_toolkit/lib/forest_admin_datasource_toolkit/utils/collection.rb b/packages/forest_admin_datasource_toolkit/lib/forest_admin_datasource_toolkit/utils/collection.rb index 7963ac7f8..537cdf97c 100644 --- a/packages/forest_admin_datasource_toolkit/lib/forest_admin_datasource_toolkit/utils/collection.rb +++ b/packages/forest_admin_datasource_toolkit/lib/forest_admin_datasource_toolkit/utils/collection.rb @@ -81,11 +81,15 @@ def self.get_field_schema(collection, field_name) # A read-only relation (e.g. #379's has_one :through identity join, which has no real # join column and would corrupt the foreign collection's own primary key if written to) # only hides its edit control in the UI via isReadOnly -- a direct write still has to be - # blocked here, at every route that can write a to-one relation's origin_key. ValidationError - # (not a bare ForestException) so this maps to a 400, not a 500 -- rejecting a write to a - # non-editable field is a client error, not a server failure. + # blocked here, at every route that can write to a relation's origin_key/through_collection. + # ValidationError (not a bare ForestException) so this maps to a 400, not a 500 -- rejecting + # a write to a non-editable field is a client error, not a server failure. + # + # respond_to? guards PolymorphicManyToOneSchema specifically: it doesn't inherit + # RelationSchema (unlike every other relation type), so it has no is_read_only at all -- + # not a versioning gap, a genuinely different type with no such concept yet. def self.assert_writable_relation!(field_name, relation) - return unless relation.is_read_only + return unless relation.respond_to?(:is_read_only) && relation.is_read_only raise ValidationError, "Field #{field_name} is not editable" end