From 371f4d9927e0e0849646ddc7189608df31544f55 Mon Sep 17 00:00:00 2001 From: Nicolas Alexandre Date: Fri, 21 Feb 2025 14:57:27 +0100 Subject: [PATCH 1/3] fix: has_one through when through collection has 2 belongs_to --- .../utils/query.rb | 29 ++++++++++++------- .../rename_field_collection_decorator.rb | 12 ++++---- 2 files changed, 23 insertions(+), 18 deletions(-) diff --git a/packages/forest_admin_datasource_active_record/lib/forest_admin_datasource_active_record/utils/query.rb b/packages/forest_admin_datasource_active_record/lib/forest_admin_datasource_active_record/utils/query.rb index 865937257..566462710 100644 --- a/packages/forest_admin_datasource_active_record/lib/forest_admin_datasource_active_record/utils/query.rb +++ b/packages/forest_admin_datasource_active_record/lib/forest_admin_datasource_active_record/utils/query.rb @@ -15,7 +15,7 @@ def initialize(collection, projection, filter) end def build - build_select + build_select(@collection, @projection) apply_filter apply_select end @@ -92,20 +92,26 @@ def compute_main_operator(condition_tree, aggregator) @query end - def build_select - unless @projection.nil? - @select += @projection.columns.map { |field| "#{@collection.model.table_name}.#{field}" } - @projection.relations.each_key do |relation| - relation_schema = @collection.schema[:fields][relation] - @select << if ['OneToOne', 'PolymorphicOneToOne'].include?(relation_schema.type) - "#{@collection.model.table_name}.#{relation_schema.origin_key_target}" + def build_select(collection, projection) + return if projection.nil? + + if collection.model.table_name == @collection.model.table_name + @select += projection.columns.map { |field| "#{collection.model.table_name}.#{field}" } + end + + one_to_one_relations = %w[OneToOne PolymorphicOneToOne] + projection.relations.each do |relation_name, sub_projection| + relation_schema = collection.schema[:fields][relation_name] + if collection.model.table_name == @collection.model.table_name + @select << if one_to_one_relations.include?(relation_schema.type) + "#{collection.model.table_name}.#{relation_schema.origin_key_target}" else - "#{@collection.model.table_name}.#{relation_schema.foreign_key}" + "#{collection.model.table_name}.#{relation_schema.foreign_key}" end end - end - @query + build_select(collection.datasource.get_collection(relation_schema.foreign_collection), sub_projection) + end end def apply_select @@ -149,6 +155,7 @@ def format_relation_projection(projection) result[relation.to_sym] = formatted_relations end + result end end diff --git a/packages/forest_admin_datasource_customizer/lib/forest_admin_datasource_customizer/decorators/rename_field/rename_field_collection_decorator.rb b/packages/forest_admin_datasource_customizer/lib/forest_admin_datasource_customizer/decorators/rename_field/rename_field_collection_decorator.rb index 968d5462d..022632fda 100644 --- a/packages/forest_admin_datasource_customizer/lib/forest_admin_datasource_customizer/decorators/rename_field/rename_field_collection_decorator.rb +++ b/packages/forest_admin_datasource_customizer/lib/forest_admin_datasource_customizer/decorators/rename_field/rename_field_collection_decorator.rb @@ -152,14 +152,13 @@ def mark_all_schema_as_dirty # Convert field path from child collection to this collection def path_from_child_collection(path) if path.include?(':') - paths = path.split(':') - child_field = paths[0] + child_field = path[0..path.index(':') - 1] relation_name = from_child_collection[child_field] || child_field relation_schema = schema[:fields][relation_name] if relation_schema.type != 'PolymorphicManyToOne' relation = datasource.get_collection(relation_schema.foreign_collection) - return "#{relation_name}:#{relation.path_from_child_collection(paths[1])}" + return "#{relation_name}:#{relation.path_from_child_collection(path[path.index(":") + 1..])}" end end @@ -169,18 +168,17 @@ def path_from_child_collection(path) # Convert field path from this collection to child collection def path_to_child_collection(path) if path.include?(':') - paths = path.split(':') - relation_name = paths[0] + relation_name = path[0..path.index(':') - 1] relation_schema = schema[:fields][relation_name] if relation_schema.type == 'PolymorphicManyToOne' relation_name = to_child_collection[relation_name] || relation_name - return "#{relation_name}:#{paths[1]}" + return "#{relation_name}:#{path[path.index(":") + 1..]}" else relation = datasource.get_collection(relation_schema.foreign_collection) child_field = to_child_collection[relation_name] || relation_name - return "#{child_field}:#{relation.path_to_child_collection(paths[1])}" + return "#{child_field}:#{relation.path_to_child_collection(path[path.index(":") + 1..])}" end end From d0faff0aed89f10321707fddad5bcbc6de249ccd Mon Sep 17 00:00:00 2001 From: Nicolas Alexandre Date: Mon, 24 Feb 2025 14:36:39 +0100 Subject: [PATCH 2/3] test: add test on introspect has_one_through relation --- .../spec/dummy/app/models/account.rb | 4 ++++ .../spec/dummy/app/models/account_history.rb | 3 +++ .../spec/dummy/app/models/supplier.rb | 4 ++++ .../db/migrate/20250121150630_create_suppliers.rb | 11 +++++++++++ .../20250121150710_create_account_histories.rb | 11 +++++++++++ .../db/migrate/20250121150713_create_accounts.rb | 12 ++++++++++++ .../collection_spec.rb | 9 +++++++++ .../datasource_spec.rb | 2 +- 8 files changed, 55 insertions(+), 1 deletion(-) create mode 100644 packages/forest_admin_datasource_active_record/spec/dummy/app/models/account.rb create mode 100644 packages/forest_admin_datasource_active_record/spec/dummy/app/models/account_history.rb create mode 100644 packages/forest_admin_datasource_active_record/spec/dummy/app/models/supplier.rb create mode 100644 packages/forest_admin_datasource_active_record/spec/dummy/db/migrate/20250121150630_create_suppliers.rb create mode 100644 packages/forest_admin_datasource_active_record/spec/dummy/db/migrate/20250121150710_create_account_histories.rb create mode 100644 packages/forest_admin_datasource_active_record/spec/dummy/db/migrate/20250121150713_create_accounts.rb diff --git a/packages/forest_admin_datasource_active_record/spec/dummy/app/models/account.rb b/packages/forest_admin_datasource_active_record/spec/dummy/app/models/account.rb new file mode 100644 index 000000000..5315e32e6 --- /dev/null +++ b/packages/forest_admin_datasource_active_record/spec/dummy/app/models/account.rb @@ -0,0 +1,4 @@ +class Account < ApplicationRecord + belongs_to :supplier + belongs_to :account_history +end diff --git a/packages/forest_admin_datasource_active_record/spec/dummy/app/models/account_history.rb b/packages/forest_admin_datasource_active_record/spec/dummy/app/models/account_history.rb new file mode 100644 index 000000000..337842259 --- /dev/null +++ b/packages/forest_admin_datasource_active_record/spec/dummy/app/models/account_history.rb @@ -0,0 +1,3 @@ +class AccountHistory < ApplicationRecord + has_one :account +end diff --git a/packages/forest_admin_datasource_active_record/spec/dummy/app/models/supplier.rb b/packages/forest_admin_datasource_active_record/spec/dummy/app/models/supplier.rb new file mode 100644 index 000000000..0ff4b174a --- /dev/null +++ b/packages/forest_admin_datasource_active_record/spec/dummy/app/models/supplier.rb @@ -0,0 +1,4 @@ +class Supplier < ApplicationRecord + has_one :account + has_one :account_history, through: :account +end diff --git a/packages/forest_admin_datasource_active_record/spec/dummy/db/migrate/20250121150630_create_suppliers.rb b/packages/forest_admin_datasource_active_record/spec/dummy/db/migrate/20250121150630_create_suppliers.rb new file mode 100644 index 000000000..528f4e502 --- /dev/null +++ b/packages/forest_admin_datasource_active_record/spec/dummy/db/migrate/20250121150630_create_suppliers.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +class CreateSuppliers < ActiveRecord::Migration[7.1] + def change + create_table :suppliers do |t| + t.string :name + + t.timestamps + end + end +end diff --git a/packages/forest_admin_datasource_active_record/spec/dummy/db/migrate/20250121150710_create_account_histories.rb b/packages/forest_admin_datasource_active_record/spec/dummy/db/migrate/20250121150710_create_account_histories.rb new file mode 100644 index 000000000..49c0f9b71 --- /dev/null +++ b/packages/forest_admin_datasource_active_record/spec/dummy/db/migrate/20250121150710_create_account_histories.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +class CreateAccountHistories < ActiveRecord::Migration[7.1] + def change + create_table :account_histories do |t| + # t.references :account, null: false, foreign_key: true + + t.timestamps + end + end +end diff --git a/packages/forest_admin_datasource_active_record/spec/dummy/db/migrate/20250121150713_create_accounts.rb b/packages/forest_admin_datasource_active_record/spec/dummy/db/migrate/20250121150713_create_accounts.rb new file mode 100644 index 000000000..14ecf2569 --- /dev/null +++ b/packages/forest_admin_datasource_active_record/spec/dummy/db/migrate/20250121150713_create_accounts.rb @@ -0,0 +1,12 @@ +# frozen_string_literal: true + +class CreateAccounts < ActiveRecord::Migration[7.1] + def change + create_table :accounts do |t| + t.references :supplier, null: false, foreign_key: true + t.references :account_history, null: false, foreign_key: true + + t.timestamps + end + end +end 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 fd7c82283..8ec6354cd 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 @@ -1,6 +1,7 @@ require 'spec_helper' module ForestAdminDatasourceActiveRecord + include ForestAdminDatasourceToolkit::Schema describe Collection do context 'without polymorphic support' do let(:datasource) { Datasource.new({ adapter: 'sqlite3', database: 'db/database.db' }) } @@ -41,6 +42,14 @@ module ForestAdminDatasourceActiveRecord expect(collection.schema[:fields].keys).to include('users') end + + it 'add has_one_through relation' do + collection = described_class.new(datasource, Supplier) + + expect(collection.schema[:fields].keys).to include('account_history') + + expect(collection.schema[:fields]['account_history'].class).to eq(Relations::ManyToManySchema) + end end end diff --git a/packages/forest_admin_datasource_active_record/spec/lib/forest_admin_datasource_active_record/datasource_spec.rb b/packages/forest_admin_datasource_active_record/spec/lib/forest_admin_datasource_active_record/datasource_spec.rb index 2e496a012..cde66e644 100644 --- a/packages/forest_admin_datasource_active_record/spec/lib/forest_admin_datasource_active_record/datasource_spec.rb +++ b/packages/forest_admin_datasource_active_record/spec/lib/forest_admin_datasource_active_record/datasource_spec.rb @@ -4,7 +4,7 @@ module ForestAdminDatasourceActiveRecord describe Datasource do it 'fetch all models' do datasource = described_class.new({ adapter: 'sqlite3', database: 'db/database.db' }) - expected = %w[User Order Check Category CarCheck Car Address Company CompaniesUser] + expected = %w[Account AccountHistory User Order Check Category CarCheck Car Address Company CompaniesUser Supplier] expect(datasource.collections.keys).to match_array(expected) end From a1035e74737c828e616e13daff64a96f66e082e1 Mon Sep 17 00:00:00 2001 From: Nicolas Alexandre Date: Mon, 24 Feb 2025 15:44:41 +0100 Subject: [PATCH 3/3] test: add test on build query with has_one_through relation --- .../utils/query.rb | 2 +- .../utils/query_spec.rb | 13 +++++++++++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/packages/forest_admin_datasource_active_record/lib/forest_admin_datasource_active_record/utils/query.rb b/packages/forest_admin_datasource_active_record/lib/forest_admin_datasource_active_record/utils/query.rb index 566462710..76aa5f050 100644 --- a/packages/forest_admin_datasource_active_record/lib/forest_admin_datasource_active_record/utils/query.rb +++ b/packages/forest_admin_datasource_active_record/lib/forest_admin_datasource_active_record/utils/query.rb @@ -3,7 +3,7 @@ module Utils class Query include ForestAdminDatasourceToolkit::Components::Query::ConditionTree - attr_reader :query + attr_reader :query, :select def initialize(collection, projection, filter) @collection = collection diff --git a/packages/forest_admin_datasource_active_record/spec/lib/forest_admin_datasource_active_record/utils/query_spec.rb b/packages/forest_admin_datasource_active_record/spec/lib/forest_admin_datasource_active_record/utils/query_spec.rb index 2c112d01a..843b3e639 100644 --- a/packages/forest_admin_datasource_active_record/spec/lib/forest_admin_datasource_active_record/utils/query_spec.rb +++ b/packages/forest_admin_datasource_active_record/spec/lib/forest_admin_datasource_active_record/utils/query_spec.rb @@ -15,6 +15,19 @@ module Utils expect(query_builder.query.to_sql).not_to eq(Car.where('id > ?', 10).to_sql) expect(query_builder.query.to_sql).to eq('SELECT "cars".* FROM "cars"') end + + describe 'build select' do + context 'when projection have nested relation' do + it 'build select with all requested fields related to the current collection' do + projection = Projection.new(%w[account_history:account:id account_history:account_id account_history:id]) + collection = Collection.new(datasource, Account) + query_builder = described_class.new(collection, projection, Filter.new) + query_builder.build + + expect(query_builder.select).to eq(%w[accounts.account_history_id accounts.id]) + end + end + end end end end