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 884fca495..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')) 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..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 @@ -67,6 +67,8 @@ def linked_one_to_one_relation(field, value, context) 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/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/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 029088e61..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 @@ -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', @@ -47,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 ) } ) @@ -157,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')) @@ -277,6 +304,21 @@ 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::ValidationError, + 'Field locked_book is 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..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 @@ -408,6 +408,51 @@ 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::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 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..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,6 +114,15 @@ def valid_many_to_many_source?(association) association.source_reflection&.belongs_to? end + # 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) + !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) ForestAdminDatasourceToolkit::Schema::Relations::ManyToManySchema.new( foreign_collection: format_model_name(association.klass.name), @@ -157,15 +166,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 +375,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_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/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/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/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/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 3e5e84c1a..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 @@ -78,6 +78,22 @@ 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 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.respond_to?(:is_read_only) && relation.is_read_only + + raise ValidationError, "Field #{field_name} 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 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