From 72852d72aab80461571c26add1c6a62979227a14 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=B2=20Rebughini?= Date: Thu, 26 Mar 2026 08:14:10 +0100 Subject: [PATCH 1/8] Simplify scope/relation --- lib/active_shopify_graphql/loader.rb | 2 +- lib/active_shopify_graphql/query/relation.rb | 15 +- lib/active_shopify_graphql/query/scope.rb | 219 ----------- .../query/scope_spec.rb | 372 ------------------ .../response/paginated_result_spec.rb | 4 +- 5 files changed, 5 insertions(+), 607 deletions(-) delete mode 100644 lib/active_shopify_graphql/query/scope.rb delete mode 100644 spec/active_shopify_graphql/query/scope_spec.rb diff --git a/lib/active_shopify_graphql/loader.rb b/lib/active_shopify_graphql/loader.rb index 34406b4..7756eb1 100644 --- a/lib/active_shopify_graphql/loader.rb +++ b/lib/active_shopify_graphql/loader.rb @@ -88,7 +88,7 @@ def load_attributes(id) # @param before [String, nil] Cursor to fetch records before # @param sort_key [String, nil] The Shopify sort key (e.g., "CREATED_AT") # @param reverse [Boolean, nil] Whether to reverse the sort order - # @param query_scope [Query::Scope] The query scope for navigation + # @param query_scope [Query::Relation] The query scope for navigation # @return [PaginatedResult] A paginated result with attribute hashes and page info def load_paginated_collection(conditions:, per_page:, query_scope:, after: nil, before: nil, sort_key: nil, reverse: nil) collection_query_name = context.query_name.pluralize diff --git a/lib/active_shopify_graphql/query/relation.rb b/lib/active_shopify_graphql/query/relation.rb index 5fc21f1..c065407 100644 --- a/lib/active_shopify_graphql/query/relation.rb +++ b/lib/active_shopify_graphql/query/relation.rb @@ -192,7 +192,7 @@ def each_page current_page = PaginatedResult.new( records: trimmed_records, page_info: PageInfo.new, - query_scope: build_query_scope_for_pagination + query_scope: self ) end end @@ -324,7 +324,7 @@ def fetch_page(after: nil, before: nil) before: before, sort_key: @sort_key, reverse: @reverse, - query_scope: build_query_scope_for_pagination + query_scope: self ) end @@ -409,17 +409,6 @@ def build_conditions(conditions_or_first_condition, args, options) end end - # Build a Query::Scope for backward compatibility with PaginatedResult - def build_query_scope_for_pagination - Query::Scope.new( - @model_class, - conditions: @conditions, - loader: loader, - total_limit: @total_limit, - per_page: @per_page - ) - end - def validate_includes_connections!(connection_names) return unless @model_class.respond_to?(:connections) diff --git a/lib/active_shopify_graphql/query/scope.rb b/lib/active_shopify_graphql/query/scope.rb deleted file mode 100644 index 387c42e..0000000 --- a/lib/active_shopify_graphql/query/scope.rb +++ /dev/null @@ -1,219 +0,0 @@ -# frozen_string_literal: true - -module ActiveShopifyGraphQL - module Query - # A chainable query builder that accumulates query configuration - # and executes the query when records are accessed. - # - # @example Basic usage - # ProductVariant.where(sku: "*").limit(100).to_a - # - # @example Pagination with block - # ProductVariant.where(sku: "*").in_pages(of: 50) do |page| - # process_batch(page) - # end - # - # @example Manual pagination - # page = ProductVariant.where(sku: "*").in_pages(of: 50) - # page.each { |record| process(record) } - # page = page.next_page while page.has_next_page? - class Scope - include Enumerable - - DEFAULT_PER_PAGE = 250 - - attr_reader :model_class, :conditions, :total_limit, :per_page - - def initialize(model_class, conditions: {}, loader: nil, total_limit: nil, per_page: DEFAULT_PER_PAGE) - @model_class = model_class - @conditions = conditions - @loader = loader - @total_limit = total_limit - @per_page = [per_page, ActiveShopifyGraphQL.configuration.max_objects_per_paginated_query].min - @loaded = false - @records = nil - end - - # Set a limit on total records to return - # @param count [Integer] Maximum number of records to fetch across all pages - # @return [Scope] A new scope with the limit applied - def limit(count) - dup_with(total_limit: count) - end - - # Configure pagination and optionally iterate through pages - # @param of [Integer] Number of records per page (default: 250, max: 250) - # @yield [PaginatedResult] Each page of results - # @return [PaginatedResult, self] Returns PaginatedResult if no block given - def in_pages(of: DEFAULT_PER_PAGE, &block) - page_size = [of, ActiveShopifyGraphQL.configuration.max_objects_per_paginated_query].min - scoped = dup_with(per_page: page_size) - - if block_given? - scoped.each_page(&block) - self - else - scoped.fetch_first_page - end - end - - # Iterate through all pages, yielding each page - # @yield [PaginatedResult] Each page of results - def each_page - current_page = fetch_first_page - records_yielded = 0 - - loop do - break if current_page.empty? - - # Apply total limit if set - if @total_limit - remaining = @total_limit - records_yielded - break if remaining <= 0 - - if current_page.size > remaining - # Trim the page to fit the limit - trimmed_records = current_page.records.first(remaining) - current_page = Response::PaginatedResult.new( - records: trimmed_records, - page_info: Response::PageInfo.new, # Empty page info to stop pagination - query_scope: self - ) - end - end - - yield current_page - records_yielded += current_page.size - - break unless current_page.has_next_page? - break if @total_limit && records_yielded >= @total_limit - - current_page = current_page.next_page - end - end - - # Iterate through all records across all pages - # @yield [Object] Each record - def each(&block) - return to_enum(:each) unless block_given? - - each_page do |page| - page.each(&block) - end - end - - # Load all records respecting total_limit - # @return [Array] All records - def to_a - return @records if @loaded - - all_records = [] - each_page do |page| - all_records.concat(page.to_a) - end - @records = all_records - @loaded = true - @records - end - alias load to_a - - # Get first record - # @param count [Integer, nil] Number of records to return - # @return [Object, Array, nil] First record(s) or nil - def first(count = nil) - if count - scoped = dup_with(total_limit: count, per_page: [count, max_objects_per_paginated_query].min) - scoped.to_a - else - scoped = dup_with(total_limit: 1, per_page: 1) - scoped.to_a.first - end - end - - # Check if any records exist - # @return [Boolean] - def exists? - first(1).any? - end - - # Check if no records exist (Array compatibility) - # @return [Boolean] - def empty? - first(1).empty? - end - - # Size/length of records (loads all pages, use with caution) - # @return [Integer] - def size - to_a.size - end - alias length size - - # Count records (loads all pages, use with caution) - # @return [Integer] - def count - to_a.count - end - - # Array-like access - def [](index) - to_a[index] - end - - # Map over records (Array compatibility) - def map(&block) - to_a.map(&block) - end - - # Select/filter records (Array compatibility) - def select_records(&block) - to_a.select(&block) - end - - # Fetch a specific page by cursor - # @param after [String, nil] Cursor to fetch records after - # @param before [String, nil] Cursor to fetch records before - # @return [PaginatedResult] - def fetch_page(after: nil, before: nil) - loader.load_paginated_collection( - conditions: @conditions, - per_page: effective_per_page, - after: after, - before: before, - query_scope: self - ) - end - - # Fetch the first page of results - # @return [PaginatedResult] - def fetch_first_page - fetch_page - end - - private - - # Calculate effective per_page considering total_limit - def effective_per_page - if @total_limit && @total_limit < @per_page - @total_limit - else - @per_page - end - end - - def loader - @loader ||= @model_class.default_loader - end - - def dup_with(**changes) - Scope.new( - @model_class, - conditions: changes.fetch(:conditions, @conditions), - loader: changes.fetch(:loader, @loader), - total_limit: changes.fetch(:total_limit, @total_limit), - per_page: changes.fetch(:per_page, @per_page) - ) - end - end - end -end diff --git a/spec/active_shopify_graphql/query/scope_spec.rb b/spec/active_shopify_graphql/query/scope_spec.rb deleted file mode 100644 index 9dfb38d..0000000 --- a/spec/active_shopify_graphql/query/scope_spec.rb +++ /dev/null @@ -1,372 +0,0 @@ -# frozen_string_literal: true - -require "spec_helper" - -RSpec.describe ActiveShopifyGraphQL::Query::Scope do - describe "#initialize" do - it "stores model class and conditions" do - mock_executor = ->(_query, **_variables) { { "data" => {} } } - ActiveShopifyGraphQL.configure { |c| c.admin_api_executor = mock_executor } - customer_class = build_customer_class - stub_const("Customer", customer_class) - scope = ActiveShopifyGraphQL::Query::Scope.new(customer_class, conditions: { email: "test@example.com" }) - - expect(scope.model_class).to eq(customer_class) - expect(scope.conditions).to eq({ email: "test@example.com" }) - end - - it "defaults per_page to 250" do - mock_executor = ->(_query, **_variables) { { "data" => {} } } - ActiveShopifyGraphQL.configure { |c| c.admin_api_executor = mock_executor } - customer_class = build_customer_class - stub_const("Customer", customer_class) - scope = described_class.new(customer_class, conditions: {}) - - expect(scope.per_page).to eq(250) - end - - it "accepts custom per_page" do - mock_executor = ->(_query, **_variables) { { "data" => {} } } - ActiveShopifyGraphQL.configure { |c| c.admin_api_executor = mock_executor } - customer_class = build_customer_class - stub_const("Customer", customer_class) - scope = described_class.new(customer_class, conditions: {}, per_page: 50) - - expect(scope.per_page).to eq(50) - end - - it "caps per_page at 250" do - mock_executor = ->(_query, **_variables) { { "data" => {} } } - ActiveShopifyGraphQL.configure { |c| c.admin_api_executor = mock_executor } - customer_class = build_customer_class - stub_const("Customer", customer_class) - scope = described_class.new(customer_class, conditions: {}, per_page: 500) - - expect(scope.per_page).to eq(250) - end - end - - describe "#limit" do - it "returns a new scope with total_limit set" do - mock_executor = ->(_query, **_variables) { { "data" => {} } } - ActiveShopifyGraphQL.configure { |c| c.admin_api_executor = mock_executor } - customer_class = build_customer_class - stub_const("Customer", customer_class) - scope = described_class.new(customer_class, conditions: {}) - - limited_scope = scope.limit(100) - - expect(limited_scope).to be_a(described_class) - expect(limited_scope).not_to eq(scope) - expect(limited_scope.total_limit).to eq(100) - end - - it "is chainable" do - mock_executor = lambda { |_query, **_variables| - { "data" => { "customers" => { "pageInfo" => { "hasNextPage" => false }, "nodes" => [] } } } - } - ActiveShopifyGraphQL.configure { |c| c.admin_api_executor = mock_executor } - customer_class = build_customer_class - stub_const("Customer", customer_class) - scope = described_class.new(customer_class, conditions: { email: "test" }) - - limited_scope = scope.limit(50) - result = limited_scope.to_a - - expect(result).to be_an(Array) - end - end - - describe "#in_pages" do - it "returns a PaginatedResult without a block" do - mock_executor = lambda { |_query, **_variables| - { "data" => { "customers" => { "pageInfo" => { "hasNextPage" => true, "endCursor" => "abc" }, "nodes" => [] } } } - } - ActiveShopifyGraphQL.configure { |c| c.admin_api_executor = mock_executor } - customer_class = build_customer_class - stub_const("Customer", customer_class) - scope = customer_class.where(email: "test") - - result = scope.in_pages(of: 10) - - expect(result).to be_a(ActiveShopifyGraphQL::Response::PaginatedResult) - end - - it "yields each page with a block" do - call_count = 0 - mock_executor = lambda { |_query, **_variables| - call_count += 1 - if call_count == 1 - { "data" => { "customers" => { - "pageInfo" => { "hasNextPage" => true, "endCursor" => "cursor1" }, - "nodes" => [{ "id" => "gid://shopify/Customer/1", "email" => "a@test.com" }] - } } } - else - { "data" => { "customers" => { - "pageInfo" => { "hasNextPage" => false }, - "nodes" => [{ "id" => "gid://shopify/Customer/2", "email" => "b@test.com" }] - } } } - end - } - ActiveShopifyGraphQL.configure { |c| c.admin_api_executor = mock_executor } - customer_class = build_customer_class - stub_const("Customer", customer_class) - scope = customer_class.where(email: "*") - pages_yielded = [] - - scope.in_pages(of: 1) { |page| pages_yielded << page } - - expect(pages_yielded.size).to eq(2) - expect(pages_yielded[0]).to be_a(ActiveShopifyGraphQL::Response::PaginatedResult) - expect(pages_yielded[1]).to be_a(ActiveShopifyGraphQL::Response::PaginatedResult) - end - - it "respects per_page size" do - received_variables = nil - mock_executor = lambda { |_query, **variables| - received_variables = variables - { "data" => { "customers" => { "pageInfo" => { "hasNextPage" => false }, "nodes" => [] } } } - } - ActiveShopifyGraphQL.configure { |c| c.admin_api_executor = mock_executor } - customer_class = build_customer_class - stub_const("Customer", customer_class) - scope = customer_class.where(email: "test") - - scope.in_pages(of: 25).to_a - - expect(received_variables[:first]).to eq(25) - end - - it "caps per_page at 250" do - received_variables = nil - mock_executor = lambda { |_query, **variables| - received_variables = variables - { "data" => { "customers" => { "pageInfo" => { "hasNextPage" => false }, "nodes" => [] } } } - } - ActiveShopifyGraphQL.configure { |c| c.admin_api_executor = mock_executor } - customer_class = build_customer_class - stub_const("Customer", customer_class) - scope = customer_class.where(email: "test") - - scope.in_pages(of: 500).to_a - - expect(received_variables[:first]).to eq(250) - end - end - - describe "#to_a" do - it "loads all records across multiple pages" do - call_count = 0 - mock_executor = lambda { |_query, **_variables| - call_count += 1 - if call_count == 1 - { "data" => { "customers" => { - "pageInfo" => { "hasNextPage" => true, "endCursor" => "cursor1" }, - "nodes" => [{ "id" => "gid://shopify/Customer/1", "email" => "a@test.com" }] - } } } - else - { "data" => { "customers" => { - "pageInfo" => { "hasNextPage" => false }, - "nodes" => [{ "id" => "gid://shopify/Customer/2", "email" => "b@test.com" }] - } } } - end - } - ActiveShopifyGraphQL.configure { |c| c.admin_api_executor = mock_executor } - customer_class = build_customer_class - stub_const("Customer", customer_class) - scope = customer_class.where(email: "*").in_pages(of: 1) - - result = scope.to_a - - expect(result.size).to eq(1) # PaginatedResult#to_a returns current page records only - end - - it "respects total_limit across pages" do - call_count = 0 - mock_executor = lambda { |_query, **_variables| - call_count += 1 - { "data" => { "customers" => { - "pageInfo" => { "hasNextPage" => call_count < 5, "endCursor" => "cursor#{call_count}" }, - "nodes" => [{ "id" => "gid://shopify/Customer/#{call_count}", "email" => "#{call_count}@test.com" }] - } } } - } - ActiveShopifyGraphQL.configure { |c| c.admin_api_executor = mock_executor } - customer_class = build_customer_class - stub_const("Customer", customer_class) - scope = customer_class.where(email: "*").limit(3) - - records = [] - scope.in_pages(of: 1) { |page| records.concat(page.to_a) } - - expect(records.size).to eq(3) - end - end - - describe "#first" do - it "returns the first record" do - mock_executor = lambda { |_query, **_variables| - { "data" => { "customers" => { - "pageInfo" => { "hasNextPage" => false }, - "nodes" => [{ "id" => "gid://shopify/Customer/1", "email" => "first@test.com" }] - } } } - } - ActiveShopifyGraphQL.configure { |c| c.admin_api_executor = mock_executor } - customer_class = build_customer_class - stub_const("Customer", customer_class) - scope = customer_class.where(email: "*") - - result = scope.first - - expect(result).to be_a(customer_class) - expect(result.email).to eq("first@test.com") - end - - it "returns nil when no results" do - mock_executor = lambda { |_query, **_variables| - { "data" => { "customers" => { "pageInfo" => { "hasNextPage" => false }, "nodes" => [] } } } - } - ActiveShopifyGraphQL.configure { |c| c.admin_api_executor = mock_executor } - customer_class = build_customer_class - stub_const("Customer", customer_class) - scope = customer_class.where(email: "nonexistent") - - result = scope.first - - expect(result).to be_nil - end - - it "returns n records when count is given" do - mock_executor = lambda { |_query, **_variables| - { "data" => { "customers" => { - "pageInfo" => { "hasNextPage" => false }, - "nodes" => [ - { "id" => "gid://shopify/Customer/1", "email" => "a@test.com" }, - { "id" => "gid://shopify/Customer/2", "email" => "b@test.com" } - ] - } } } - } - ActiveShopifyGraphQL.configure { |c| c.admin_api_executor = mock_executor } - customer_class = build_customer_class - stub_const("Customer", customer_class) - scope = customer_class.where(email: "*") - - result = scope.first(2) - - expect(result).to be_an(Array) - expect(result.size).to eq(2) - end - end - - describe "#each" do - it "iterates over all records across pages" do - call_count = 0 - mock_executor = lambda { |_query, **_variables| - call_count += 1 - if call_count == 1 - { "data" => { "customers" => { - "pageInfo" => { "hasNextPage" => true, "endCursor" => "cursor1" }, - "nodes" => [{ "id" => "gid://shopify/Customer/1", "email" => "a@test.com" }] - } } } - else - { "data" => { "customers" => { - "pageInfo" => { "hasNextPage" => false }, - "nodes" => [{ "id" => "gid://shopify/Customer/2", "email" => "b@test.com" }] - } } } - end - } - ActiveShopifyGraphQL.configure { |c| c.admin_api_executor = mock_executor } - customer_class = build_customer_class - stub_const("Customer", customer_class) - scope = customer_class.where(email: "*") - emails = [] - - scope.in_pages(of: 1) { |page| page.each { |c| emails << c.email } } - - expect(emails).to eq(["a@test.com", "b@test.com"]) - end - - it "returns an enumerator when no block given" do - mock_executor = ->(_query, **_variables) { { "data" => {} } } - ActiveShopifyGraphQL.configure { |c| c.admin_api_executor = mock_executor } - customer_class = build_customer_class - stub_const("Customer", customer_class) - scope = customer_class.where(email: "*") - - result = scope.each - - expect(result).to be_an(Enumerator) - end - end - - describe "Enumerable compatibility" do - it "supports map" do - mock_executor = lambda { |_query, **_variables| - { "data" => { "customers" => { - "pageInfo" => { "hasNextPage" => false }, - "nodes" => [ - { "id" => "gid://shopify/Customer/1", "email" => "a@test.com" }, - { "id" => "gid://shopify/Customer/2", "email" => "b@test.com" } - ] - } } } - } - ActiveShopifyGraphQL.configure { |c| c.admin_api_executor = mock_executor } - customer_class = build_customer_class - stub_const("Customer", customer_class) - scope = customer_class.where(email: "*") - - emails = scope.map(&:email) - - expect(emails).to eq(["a@test.com", "b@test.com"]) - end - - it "supports empty?" do - mock_executor = lambda { |_query, **_variables| - { "data" => { "customers" => { "pageInfo" => { "hasNextPage" => false }, "nodes" => [] } } } - } - ActiveShopifyGraphQL.configure { |c| c.admin_api_executor = mock_executor } - customer_class = build_customer_class - stub_const("Customer", customer_class) - scope = customer_class.where(email: "*") - - expect(scope.empty?).to be true - end - - it "supports size/length" do - mock_executor = lambda { |_query, **_variables| - { "data" => { "customers" => { - "pageInfo" => { "hasNextPage" => false }, - "nodes" => [ - { "id" => "gid://shopify/Customer/1", "email" => "a@test.com" }, - { "id" => "gid://shopify/Customer/2", "email" => "b@test.com" } - ] - } } } - } - ActiveShopifyGraphQL.configure { |c| c.admin_api_executor = mock_executor } - customer_class = build_customer_class - stub_const("Customer", customer_class) - scope = customer_class.where(email: "*") - - expect(scope.size).to eq(2) - expect(scope.length).to eq(2) - end - - it "supports array indexing" do - mock_executor = lambda { |_query, **_variables| - { "data" => { "customers" => { - "pageInfo" => { "hasNextPage" => false }, - "nodes" => [ - { "id" => "gid://shopify/Customer/1", "email" => "a@test.com" }, - { "id" => "gid://shopify/Customer/2", "email" => "b@test.com" } - ] - } } } - } - ActiveShopifyGraphQL.configure { |c| c.admin_api_executor = mock_executor } - customer_class = build_customer_class - stub_const("Customer", customer_class) - scope = customer_class.where(email: "*") - - expect(scope[0].email).to eq("a@test.com") - expect(scope[1].email).to eq("b@test.com") - end - end -end diff --git a/spec/active_shopify_graphql/response/paginated_result_spec.rb b/spec/active_shopify_graphql/response/paginated_result_spec.rb index f2e2113..4743430 100644 --- a/spec/active_shopify_graphql/response/paginated_result_spec.rb +++ b/spec/active_shopify_graphql/response/paginated_result_spec.rb @@ -191,7 +191,7 @@ "hasNextPage" => true, "endCursor" => "cursor123" ) - query_scope = instance_double(ActiveShopifyGraphQL::Query::Scope) + query_scope = instance_double(ActiveShopifyGraphQL::Query::Relation) next_page_result = double("next_page") paginated_result = ActiveShopifyGraphQL::Response::PaginatedResult.new( attributes: [], @@ -227,7 +227,7 @@ "hasPreviousPage" => true, "startCursor" => "cursor456" ) - query_scope = instance_double(ActiveShopifyGraphQL::Query::Scope) + query_scope = instance_double(ActiveShopifyGraphQL::Query::Relation) previous_page_result = double("previous_page") paginated_result = ActiveShopifyGraphQL::Response::PaginatedResult.new( attributes: [], From b5ff77c0b40bf7e4d681fd27ab4593a6d72ee7a7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=B2=20Rebughini?= Date: Thu, 26 Mar 2026 08:34:43 +0100 Subject: [PATCH 2/8] Remove model builder This wasn't really useful, as models already know how to build themselves --- .../connections/connection_loader.rb | 8 +-- lib/active_shopify_graphql/model_builder.rb | 53 ------------------- lib/active_shopify_graphql/query/relation.rb | 4 +- .../response/paginated_result.rb | 2 +- .../testing/test_loader.rb | 12 ++--- 5 files changed, 13 insertions(+), 66 deletions(-) delete mode 100644 lib/active_shopify_graphql/model_builder.rb diff --git a/lib/active_shopify_graphql/connections/connection_loader.rb b/lib/active_shopify_graphql/connections/connection_loader.rb index 0bb96b9..dbba60d 100644 --- a/lib/active_shopify_graphql/connections/connection_loader.rb +++ b/lib/active_shopify_graphql/connections/connection_loader.rb @@ -56,12 +56,12 @@ def load_nested_connection(query_name, variables, parent, connection_config) return nil if attributes.nil? wire_inverse_of(parent, attributes, connection_config) - ModelBuilder.build(@context.model_class, attributes) + @context.model_class.new(attributes) else return [] if attributes.nil? || attributes.empty? attributes.each { |attrs| wire_inverse_of(parent, attrs, connection_config) } - ModelBuilder.build_many(@context.model_class, attributes) + attributes.filter_map { |attrs| @context.model_class.new(attrs) if attrs } end end @@ -87,11 +87,11 @@ def load_root_connection(query_name, variables, connection_config) if singular return nil if attributes.nil? - ModelBuilder.build(@context.model_class, attributes) + @context.model_class.new(attributes) else return [] if attributes.nil? || attributes.empty? - ModelBuilder.build_many(@context.model_class, attributes) + attributes.filter_map { |attrs| @context.model_class.new(attrs) if attrs } end end diff --git a/lib/active_shopify_graphql/model_builder.rb b/lib/active_shopify_graphql/model_builder.rb deleted file mode 100644 index 4e4a823..0000000 --- a/lib/active_shopify_graphql/model_builder.rb +++ /dev/null @@ -1,53 +0,0 @@ -# frozen_string_literal: true - -module ActiveShopifyGraphQL - # Pure factory for building model instances from attribute hashes. - # Separates data mapping (Loader/ResponseMapper) from object construction. - # - # Responsibilities: - # - Build single model instances from attributes - # - Apply connection caches to instances - # - Handle batch building with connection caching - # - # @example Single instance - # attributes = loader.load_attributes(gid) - # customer = ModelBuilder.build(Customer, attributes) - # - # @example Batch building - # attributes_list = loader.load_paginated_attributes(...) - # customers = ModelBuilder.build_many(Customer, attributes_list) - class ModelBuilder - class << self - # Build a single model instance from attributes - # @param model_class [Class] The model class to instantiate - # @param attributes [Hash] Attribute hash (may contain :_connection_cache) - # @return [Object] The instantiated model with cached connections - def build(model_class, attributes) - return nil if attributes.nil? - - instance = model_class.new(attributes) - apply_connection_cache(instance, attributes) - instance - end - - # Build multiple model instances from an array of attribute hashes - # @param model_class [Class] The model class to instantiate - # @param attributes_array [Array] Array of attribute hashes - # @return [Array] Array of instantiated models - def build_many(model_class, attributes_array) - return [] if attributes_array.nil? || attributes_array.empty? - - attributes_array.filter_map { |attrs| build(model_class, attrs) } - end - - # Apply connection cache from attributes to an already-instantiated model - # @param instance [Object] The model instance - # @param attributes [Hash] Attribute hash that may contain :_connection_cache - def apply_connection_cache(instance, attributes) - return unless attributes.is_a?(Hash) && attributes.key?(:_connection_cache) - - instance.instance_variable_set(:@_connection_cache, attributes[:_connection_cache]) - end - end - end -end diff --git a/lib/active_shopify_graphql/query/relation.rb b/lib/active_shopify_graphql/query/relation.rb index c065407..c864a9b 100644 --- a/lib/active_shopify_graphql/query/relation.rb +++ b/lib/active_shopify_graphql/query/relation.rb @@ -108,7 +108,7 @@ def find(id = nil) attributes = loader.load_attributes raise ObjectNotFoundError, "Couldn't find current customer" if attributes.nil? - return ModelBuilder.build(@model_class, attributes) + return @model_class.new(attributes) end # Standard case: find by ID @@ -117,7 +117,7 @@ def find(id = nil) raise ObjectNotFoundError, "Couldn't find #{@model_class.name} with id=#{id}" if attributes.nil? - ModelBuilder.build(@model_class, attributes) + @model_class.new(attributes) end # Include connections for eager loading diff --git a/lib/active_shopify_graphql/response/paginated_result.rb b/lib/active_shopify_graphql/response/paginated_result.rb index 4bed15b..1757866 100644 --- a/lib/active_shopify_graphql/response/paginated_result.rb +++ b/lib/active_shopify_graphql/response/paginated_result.rb @@ -30,7 +30,7 @@ def initialize(attributes:, model_class:, page_info:, query_scope:) # Get the records for this page (builds instances on first access) def records - @records ||= ModelBuilder.build_many(@model_class, @attributes) + @records ||= @attributes.filter_map { |attrs| @model_class.new(attrs) if attrs } end # Iterate over records in this page diff --git a/lib/active_shopify_graphql/testing/test_loader.rb b/lib/active_shopify_graphql/testing/test_loader.rb index 558184f..2fff36e 100644 --- a/lib/active_shopify_graphql/testing/test_loader.rb +++ b/lib/active_shopify_graphql/testing/test_loader.rb @@ -75,13 +75,13 @@ def load_connection_records(_query_name, _variables, parent = nil, connection_co attrs = filter_to_model_attributes(data, target_class) wire_inverse_of(parent, attrs, connection_config) - ModelBuilder.build(target_class, attrs) + target_class.new(attrs) else items = connection_data.is_a?(Array) ? connection_data : [connection_data] items.filter_map do |item| attrs = filter_to_model_attributes(item, target_class) wire_inverse_of(parent, attrs, connection_config) - ModelBuilder.build(target_class, attrs) + target_class.new(attrs) end end end @@ -159,14 +159,14 @@ def populate_connection_cache(record, attrs) if data child_attrs = filter_to_model_attributes(data, target_class) populate_nested_connections(data, child_attrs, target_class, nested_includes) - cache[connection_name] = ModelBuilder.build(target_class, child_attrs) + cache[connection_name] = target_class.new(child_attrs) end else items = connection_data.is_a?(Array) ? connection_data : [connection_data] cache[connection_name] = items.filter_map do |item| child_attrs = filter_to_model_attributes(item, target_class) populate_nested_connections(item, child_attrs, target_class, nested_includes) - ModelBuilder.build(target_class, child_attrs) + target_class.new(child_attrs) end end end @@ -196,14 +196,14 @@ def populate_nested_connections(record, attrs, model_class, nested_includes) if data child_attrs = filter_to_model_attributes(data, target_class) populate_nested_connections(data, child_attrs, target_class, deeper_includes) - cache[connection_name] = ModelBuilder.build(target_class, child_attrs) + cache[connection_name] = target_class.new(child_attrs) end else items = connection_data.is_a?(Array) ? connection_data : [connection_data] cache[connection_name] = items.filter_map do |item| child_attrs = filter_to_model_attributes(item, target_class) populate_nested_connections(item, child_attrs, target_class, deeper_includes) - ModelBuilder.build(target_class, child_attrs) + target_class.new(child_attrs) end end end From 50a4dc2808b51028132522e4e9b7d9a088ca36c4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=B2=20Rebughini?= Date: Thu, 26 Mar 2026 08:39:53 +0100 Subject: [PATCH 3/8] Simplify inverse cache wiring --- .../connections/connection_loader.rb | 18 +----- .../connections/connection_proxy.rb | 24 +------- .../connections/inverse_cache_wiring.rb | 56 +++++++++++++++++++ lib/active_shopify_graphql/model.rb | 22 +------- .../model/connections.rb | 24 +------- .../response/response_mapper.rb | 18 +----- .../testing/test_loader.rb | 15 +---- 7 files changed, 65 insertions(+), 112 deletions(-) create mode 100644 lib/active_shopify_graphql/connections/inverse_cache_wiring.rb diff --git a/lib/active_shopify_graphql/connections/connection_loader.rb b/lib/active_shopify_graphql/connections/connection_loader.rb index dbba60d..f38b4fd 100644 --- a/lib/active_shopify_graphql/connections/connection_loader.rb +++ b/lib/active_shopify_graphql/connections/connection_loader.rb @@ -97,23 +97,7 @@ def load_root_connection(query_name, variables, connection_config) # Wire up inverse_of associations by adding parent to attributes cache def wire_inverse_of(parent, attributes, connection_config) - return unless attributes.is_a?(Hash) && connection_config&.dig(:inverse_of) - - inverse_name = connection_config[:inverse_of] - attributes[:_connection_cache] ||= {} - - # Check the type of the inverse connection - target_class = @context.model_class - return unless target_class.respond_to?(:connections) && target_class.connections[inverse_name] - - inverse_type = target_class.connections[inverse_name][:type] - attributes[:_connection_cache][inverse_name] = - if inverse_type == :singular - parent - else - # For collection inverses, wrap parent in an array - [parent] - end + InverseCacheWiring.wire_attributes(attributes, connection_config, parent) end def extract_gid(parent) diff --git a/lib/active_shopify_graphql/connections/connection_proxy.rb b/lib/active_shopify_graphql/connections/connection_proxy.rb index c916f1c..5a1e61a 100644 --- a/lib/active_shopify_graphql/connections/connection_proxy.rb +++ b/lib/active_shopify_graphql/connections/connection_proxy.rb @@ -115,28 +115,8 @@ def populate_inverse_cache(records, connection_config, parent) return unless connection_config[:inverse_of] return if records.nil? || (records.is_a?(Array) && records.empty?) - inverse_name = connection_config[:inverse_of] - target_class = connection_config[:class_name].constantize - - # Ensure target class has the inverse connection defined - return unless target_class.respond_to?(:connections) && target_class.connections[inverse_name] - - inverse_type = target_class.connections[inverse_name][:type] - records_array = records.is_a?(Array) ? records : [records] - - records_array.each do |record| - next unless record - - record.instance_variable_set(:@_connection_cache, {}) unless record.instance_variable_get(:@_connection_cache) - cache = record.instance_variable_get(:@_connection_cache) - - cache[inverse_name] = - if inverse_type == :singular - parent - else - # For collection inverses, wrap parent in an array - [parent] - end + Array(records).compact.each do |record| + InverseCacheWiring.wire_instance(record, connection_config, parent) end end end diff --git a/lib/active_shopify_graphql/connections/inverse_cache_wiring.rb b/lib/active_shopify_graphql/connections/inverse_cache_wiring.rb new file mode 100644 index 0000000..dbdb1a9 --- /dev/null +++ b/lib/active_shopify_graphql/connections/inverse_cache_wiring.rb @@ -0,0 +1,56 @@ +# frozen_string_literal: true + +module ActiveShopifyGraphQL + module Connections + # Single home for the inverse cache wiring logic. + # + # When a connection has an +inverse_of+ option, loading records on one side + # should automatically populate the cache on the other side so the inverse + # association doesn't trigger an extra round-trip. + # + # Two entry points cover the two moments this happens: + # + # * +wire_instance+ — the child record is already a model instance (ivar-based cache). + # * +wire_attributes+ — the child record is still an attributes Hash, about to be + # instantiated (Hash-based cache, keyed +:_connection_cache+). + module InverseCacheWiring + # Wire the inverse cache on an already-built model instance. + # + # @param record [Object] a model instance that is the child side of the connection + # @param connection_config [Hash] the connection config hash that includes +:inverse_of+ + # @param parent [Object] the parent model instance to store as the cached value + def self.wire_instance(record, connection_config, parent) + return unless record && connection_config[:inverse_of] + + inverse_name = connection_config[:inverse_of] + target_class = connection_config[:class_name].constantize + + return unless target_class.respond_to?(:connections) && target_class.connections[inverse_name] + + inverse_type = target_class.connections[inverse_name][:type] + + record.instance_variable_set(:@_connection_cache, {}) unless record.instance_variable_get(:@_connection_cache) + cache = record.instance_variable_get(:@_connection_cache) + cache[inverse_name] = inverse_type == :singular ? parent : [parent] + end + + # Wire the inverse cache into an attributes Hash before model instantiation. + # + # @param attributes [Hash] the attributes hash that will be passed to +model_class.new+ + # @param connection_config [Hash] the connection config hash that includes +:inverse_of+ + # @param parent [Object] the parent model instance to store as the cached value + def self.wire_attributes(attributes, connection_config, parent) + return unless attributes.is_a?(Hash) && connection_config[:inverse_of] + + inverse_name = connection_config[:inverse_of] + target_class = connection_config[:class_name].constantize + + return unless target_class.respond_to?(:connections) && target_class.connections[inverse_name] + + inverse_type = target_class.connections[inverse_name][:type] + attributes[:_connection_cache] ||= {} + attributes[:_connection_cache][inverse_name] = inverse_type == :singular ? parent : [parent] + end + end + end +end diff --git a/lib/active_shopify_graphql/model.rb b/lib/active_shopify_graphql/model.rb index 86402a7..4ee1582 100644 --- a/lib/active_shopify_graphql/model.rb +++ b/lib/active_shopify_graphql/model.rb @@ -52,26 +52,8 @@ def populate_inverse_caches_on_initialization connection_config = self.class.connections[connection_name] next unless connection_config && connection_config[:inverse_of] - inverse_name = connection_config[:inverse_of] - records_array = Array(records).compact - - records_array.each do |record| - next unless record.respond_to?(:instance_variable_set) - - record.instance_variable_set(:@_connection_cache, {}) unless record.instance_variable_get(:@_connection_cache) - cache = record.instance_variable_get(:@_connection_cache) - - # Determine the type of the inverse connection - next unless record.class.respond_to?(:connections) && record.class.connections[inverse_name] - - inverse_type = record.class.connections[inverse_name][:type] - cache[inverse_name] = - if inverse_type == :singular - self - else - # For collection inverses, wrap parent in an array - [self] - end + Array(records).compact.each do |record| + ActiveShopifyGraphQL::Connections::InverseCacheWiring.wire_instance(record, connection_config, self) end end end diff --git a/lib/active_shopify_graphql/model/connections.rb b/lib/active_shopify_graphql/model/connections.rb index 7939138..7ab8d20 100644 --- a/lib/active_shopify_graphql/model/connections.rb +++ b/lib/active_shopify_graphql/model/connections.rb @@ -153,28 +153,8 @@ def populate_inverse_cache_for_connection(records, connection_config, parent) return unless connection_config[:inverse_of] return if records.nil? || (records.is_a?(Array) && records.empty?) - inverse_name = connection_config[:inverse_of] - target_class = connection_config[:class_name].constantize - - # Ensure target class has the inverse connection defined - return unless target_class.respond_to?(:connections) && target_class.connections[inverse_name] - - inverse_type = target_class.connections[inverse_name][:type] - records_array = records.is_a?(Array) ? records : [records] - - records_array.each do |record| - next unless record - - record.instance_variable_set(:@_connection_cache, {}) unless record.instance_variable_get(:@_connection_cache) - cache = record.instance_variable_get(:@_connection_cache) - - cache[inverse_name] = - if inverse_type == :singular - parent - else - # For collection inverses, wrap parent in an array - [parent] - end + Array(records).compact.each do |record| + ActiveShopifyGraphQL::Connections::InverseCacheWiring.wire_instance(record, connection_config, parent) end end end diff --git a/lib/active_shopify_graphql/response/response_mapper.rb b/lib/active_shopify_graphql/response/response_mapper.rb index c2dd790..17da245 100644 --- a/lib/active_shopify_graphql/response/response_mapper.rb +++ b/lib/active_shopify_graphql/response/response_mapper.rb @@ -209,23 +209,7 @@ def build_nested_model_instance(node_data, target_class, nested_includes, parent instance = target_class.new(attributes) # Populate inverse cache if inverse_of is specified - if parent_instance && connection_config && connection_config[:inverse_of] - inverse_name = connection_config[:inverse_of] - instance.instance_variable_set(:@_connection_cache, {}) unless instance.instance_variable_get(:@_connection_cache) - cache = instance.instance_variable_get(:@_connection_cache) - - # Check the type of the inverse connection to determine how to cache - if target_class.respond_to?(:connections) && target_class.connections[inverse_name] - inverse_type = target_class.connections[inverse_name][:type] - cache[inverse_name] = - if inverse_type == :singular - parent_instance - else - # For collection inverses, wrap parent in an array - [parent_instance] - end - end - end + Connections::InverseCacheWiring.wire_instance(instance, connection_config, parent_instance) if parent_instance && connection_config # Handle nested connections recursively (instance becomes parent for its children) if nested_includes.any? diff --git a/lib/active_shopify_graphql/testing/test_loader.rb b/lib/active_shopify_graphql/testing/test_loader.rb index 2fff36e..6d49333 100644 --- a/lib/active_shopify_graphql/testing/test_loader.rb +++ b/lib/active_shopify_graphql/testing/test_loader.rb @@ -213,20 +213,7 @@ def populate_nested_connections(record, attrs, model_class, nested_includes) # Wire inverse_of associations into the connection cache. def wire_inverse_of(parent, attributes, connection_config) - return unless attributes.is_a?(Hash) && connection_config&.dig(:inverse_of) - - inverse_name = connection_config[:inverse_of] - target_class = connection_config[:class_name].constantize - return unless target_class.respond_to?(:connections) && target_class.connections[inverse_name] - - attributes[:_connection_cache] ||= {} - inverse_type = target_class.connections[inverse_name][:type] - attributes[:_connection_cache][inverse_name] = - if inverse_type == :singular - parent - else - [parent] - end + Connections::InverseCacheWiring.wire_attributes(attributes, connection_config, parent) end end end From 41e11a02054d147ce4e3c63c614910ef1786473c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=B2=20Rebughini?= Date: Thu, 26 Mar 2026 08:42:13 +0100 Subject: [PATCH 4/8] Avoid allocating type casters constantly --- .../response/response_mapper.rb | 16 ++++++++-------- .../testing/test_loader.rb | 16 ++++++++-------- 2 files changed, 16 insertions(+), 16 deletions(-) diff --git a/lib/active_shopify_graphql/response/response_mapper.rb b/lib/active_shopify_graphql/response/response_mapper.rb index 17da245..fe9e61e 100644 --- a/lib/active_shopify_graphql/response/response_mapper.rb +++ b/lib/active_shopify_graphql/response/response_mapper.rb @@ -5,6 +5,13 @@ module Response # Handles mapping GraphQL responses to model attributes. # Refactored to use LoaderContext and unified mapping methods. class ResponseMapper + TYPE_CASTERS = { + string: ActiveModel::Type::String.new, + integer: ActiveModel::Type::Integer.new, + float: ActiveModel::Type::Float.new, + boolean: ActiveModel::Type::Boolean.new, + datetime: ActiveModel::Type::DateTime.new + }.freeze attr_reader :context def initialize(context) @@ -162,14 +169,7 @@ def coerce_value(value, type) end def type_caster(type) - case type - when :string then ActiveModel::Type::String.new - when :integer then ActiveModel::Type::Integer.new - when :float then ActiveModel::Type::Float.new - when :boolean then ActiveModel::Type::Boolean.new - when :datetime then ActiveModel::Type::DateTime.new - else ActiveModel::Type::Value.new - end + TYPE_CASTERS.fetch(type, ActiveModel::Type::Value.new) end def extract_connection_records(node_data, connection_config, nested_includes, parent_instance: nil) diff --git a/lib/active_shopify_graphql/testing/test_loader.rb b/lib/active_shopify_graphql/testing/test_loader.rb index 6d49333..234c771 100644 --- a/lib/active_shopify_graphql/testing/test_loader.rb +++ b/lib/active_shopify_graphql/testing/test_loader.rb @@ -6,6 +6,13 @@ module Testing # executing GraphQL queries. Overrides the three main loading methods # and raises on any attempt to perform a real GraphQL query. class TestLoader < Loader + TYPE_CASTERS = { + string: ActiveModel::Type::String.new, + integer: ActiveModel::Type::Integer.new, + float: ActiveModel::Type::Float.new, + boolean: ActiveModel::Type::Boolean.new, + datetime: ActiveModel::Type::DateTime.new + }.freeze # Load attributes for a single record by ID. # # @param id [String] The GID of the record @@ -129,14 +136,7 @@ def coerce_value(value, type) return nil if value.nil? return value if value.is_a?(Array) - case type - when :string then ActiveModel::Type::String.new - when :integer then ActiveModel::Type::Integer.new - when :float then ActiveModel::Type::Float.new - when :boolean then ActiveModel::Type::Boolean.new - when :datetime then ActiveModel::Type::DateTime.new - else ActiveModel::Type::Value.new - end.cast(value) + TYPE_CASTERS.fetch(type, ActiveModel::Type::Value.new).cast(value) end # Build connection cache from inline connection data in the stored record. From 52205e02a226b0a5d7745cffce434bf441d2482d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=B2=20Rebughini?= Date: Thu, 26 Mar 2026 12:36:22 +0100 Subject: [PATCH 5/8] Extract type coercion --- .../response/response_mapper.rb | 20 ++--------------- .../testing/test_loader.rb | 16 ++------------ lib/active_shopify_graphql/type_coercion.rb | 22 +++++++++++++++++++ 3 files changed, 26 insertions(+), 32 deletions(-) create mode 100644 lib/active_shopify_graphql/type_coercion.rb diff --git a/lib/active_shopify_graphql/response/response_mapper.rb b/lib/active_shopify_graphql/response/response_mapper.rb index fe9e61e..80d56fd 100644 --- a/lib/active_shopify_graphql/response/response_mapper.rb +++ b/lib/active_shopify_graphql/response/response_mapper.rb @@ -5,13 +5,8 @@ module Response # Handles mapping GraphQL responses to model attributes. # Refactored to use LoaderContext and unified mapping methods. class ResponseMapper - TYPE_CASTERS = { - string: ActiveModel::Type::String.new, - integer: ActiveModel::Type::Integer.new, - float: ActiveModel::Type::Float.new, - boolean: ActiveModel::Type::Boolean.new, - datetime: ActiveModel::Type::DateTime.new - }.freeze + include TypeCoercion + attr_reader :context def initialize(context) @@ -161,17 +156,6 @@ def validate_null_constraint!(value, config, attr_name) raise ArgumentError, "Attribute '#{attr_name}' (GraphQL path: '#{config[:path]}') cannot be null but received nil" end - def coerce_value(value, type) - return nil if value.nil? - return value if value.is_a?(Array) # Preserve arrays - - type_caster(type).cast(value) - end - - def type_caster(type) - TYPE_CASTERS.fetch(type, ActiveModel::Type::Value.new) - end - def extract_connection_records(node_data, connection_config, nested_includes, parent_instance: nil) # Use original_name (Ruby attr name) as the response key since we alias connections response_key = connection_config[:original_name].to_s diff --git a/lib/active_shopify_graphql/testing/test_loader.rb b/lib/active_shopify_graphql/testing/test_loader.rb index 234c771..95d8873 100644 --- a/lib/active_shopify_graphql/testing/test_loader.rb +++ b/lib/active_shopify_graphql/testing/test_loader.rb @@ -6,13 +6,8 @@ module Testing # executing GraphQL queries. Overrides the three main loading methods # and raises on any attempt to perform a real GraphQL query. class TestLoader < Loader - TYPE_CASTERS = { - string: ActiveModel::Type::String.new, - integer: ActiveModel::Type::Integer.new, - float: ActiveModel::Type::Float.new, - boolean: ActiveModel::Type::Boolean.new, - datetime: ActiveModel::Type::DateTime.new - }.freeze + include TypeCoercion + # Load attributes for a single record by ID. # # @param id [String] The GID of the record @@ -132,13 +127,6 @@ def apply_defaults_and_transforms(value, config) config[:transform] ? config[:transform].call(value) : value end - def coerce_value(value, type) - return nil if value.nil? - return value if value.is_a?(Array) - - TYPE_CASTERS.fetch(type, ActiveModel::Type::Value.new).cast(value) - end - # Build connection cache from inline connection data in the stored record. def populate_connection_cache(record, attrs) normalized = Query::QueryBuilder.normalize_includes(@included_connections) diff --git a/lib/active_shopify_graphql/type_coercion.rb b/lib/active_shopify_graphql/type_coercion.rb new file mode 100644 index 0000000..0a4ce4a --- /dev/null +++ b/lib/active_shopify_graphql/type_coercion.rb @@ -0,0 +1,22 @@ +# frozen_string_literal: true + +module ActiveShopifyGraphQL + module TypeCoercion + TYPE_CASTERS = { + string: ActiveModel::Type::String.new, + integer: ActiveModel::Type::Integer.new, + float: ActiveModel::Type::Float.new, + boolean: ActiveModel::Type::Boolean.new, + datetime: ActiveModel::Type::DateTime.new + }.freeze + + private + + def coerce_value(value, type) + return nil if value.nil? + return value if value.is_a?(Array) + + TYPE_CASTERS.fetch(type, ActiveModel::Type::Value.new).cast(value) + end + end +end From 12bcd5a7dbf488f4c74c0974e1945040cc0aae98 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=B2=20Rebughini?= Date: Thu, 26 Mar 2026 13:01:53 +0100 Subject: [PATCH 6/8] Don't leak Customer Account API concerns to main loader class --- lib/active_shopify_graphql/loader.rb | 8 ++++++++ lib/active_shopify_graphql/loader_proxy.rb | 14 +++++++------- .../loaders/customer_account_api_loader.rb | 4 ++++ lib/active_shopify_graphql/query/relation.rb | 9 ++++----- .../model/finder_methods_spec.rb | 2 +- spec/active_shopify_graphql/query/relation_spec.rb | 4 ++-- 6 files changed, 26 insertions(+), 15 deletions(-) diff --git a/lib/active_shopify_graphql/loader.rb b/lib/active_shopify_graphql/loader.rb index 7756eb1..e6662ff 100644 --- a/lib/active_shopify_graphql/loader.rb +++ b/lib/active_shopify_graphql/loader.rb @@ -56,6 +56,14 @@ def initialization_args [] end + # Whether this loader can load a record without an explicit ID. + # Subclasses that support "current entity" semantics (e.g. Customer Account API) + # should override this to return true. + # @return [Boolean] + def supports_nil_id? + false + end + # Map the GraphQL response to model attributes def map_response_to_attributes(response_data, parent_instance: nil) mapper = Response::ResponseMapper.new(context) diff --git a/lib/active_shopify_graphql/loader_proxy.rb b/lib/active_shopify_graphql/loader_proxy.rb index af0cbb0..412a1b9 100644 --- a/lib/active_shopify_graphql/loader_proxy.rb +++ b/lib/active_shopify_graphql/loader_proxy.rb @@ -33,17 +33,17 @@ def find_by(conditions = {}, **options) end def find(id = nil) - # For Customer Account API, if no ID is provided, load the current customer - if id.nil? && @loader.is_a?(ActiveShopifyGraphQL::Loaders::CustomerAccountApiLoader) + # For loaders that support nil ID (e.g. Customer Account API current customer), + # delegate directly; otherwise require an ID via the standard flow. + if id.nil? + return nil unless @loader.supports_nil_id? + attributes = @loader.load_attributes - return nil if attributes.nil? + return @model_class.new(attributes) unless attributes.nil? - return @model_class.new(attributes) + return nil end - # For other cases, require ID and use standard flow - return nil if id.nil? - build_relation.find(id) end diff --git a/lib/active_shopify_graphql/loaders/customer_account_api_loader.rb b/lib/active_shopify_graphql/loaders/customer_account_api_loader.rb index 035850a..83e0004 100644 --- a/lib/active_shopify_graphql/loaders/customer_account_api_loader.rb +++ b/lib/active_shopify_graphql/loaders/customer_account_api_loader.rb @@ -33,6 +33,10 @@ def perform_graphql_query(query, **variables) def initialization_args [@token] end + + def supports_nil_id? + context.graphql_type == "Customer" + end end end end diff --git a/lib/active_shopify_graphql/query/relation.rb b/lib/active_shopify_graphql/query/relation.rb index c864a9b..2683ba3 100644 --- a/lib/active_shopify_graphql/query/relation.rb +++ b/lib/active_shopify_graphql/query/relation.rb @@ -95,18 +95,17 @@ def find_by(conditions = {}, **options) end # Find a single record by ID - # For Customer Account API, if no ID is provided, fetches the current customer # @param id [String, Integer, nil] The record ID (will be converted to GID automatically) # @return [Object] The model instance # @raise [ObjectNotFoundError] If the record is not found - # @raise [ArgumentError] If id is nil and not using Customer Account API loader + # @raise [ArgumentError] If id is nil and the loader does not support it def find(id = nil) - # Handle Customer Account API case where no ID means "current customer" + # Handle "current entity" case where no ID is supported by the loader if id.nil? - raise ArgumentError, "find requires an ID argument unless using Customer Account API" unless loader.is_a?(ActiveShopifyGraphQL::Loaders::CustomerAccountApiLoader) + raise ArgumentError, "find requires an ID argument" unless loader.supports_nil_id? attributes = loader.load_attributes - raise ObjectNotFoundError, "Couldn't find current customer" if attributes.nil? + raise ObjectNotFoundError, "Couldn't find #{@model_class.name}" if attributes.nil? return @model_class.new(attributes) end diff --git a/spec/active_shopify_graphql/model/finder_methods_spec.rb b/spec/active_shopify_graphql/model/finder_methods_spec.rb index 63a1b9f..43f19ee 100644 --- a/spec/active_shopify_graphql/model/finder_methods_spec.rb +++ b/spec/active_shopify_graphql/model/finder_methods_spec.rb @@ -69,7 +69,7 @@ customer_class = build_customer_class stub_const("Customer", customer_class) - expect { customer_class.find }.to raise_error(ArgumentError, "find requires an ID argument unless using Customer Account API") + expect { customer_class.find }.to raise_error(ArgumentError, "find requires an ID argument") end it "fetches current customer when called without id using Customer Account API" do diff --git a/spec/active_shopify_graphql/query/relation_spec.rb b/spec/active_shopify_graphql/query/relation_spec.rb index a989e82..6669453 100644 --- a/spec/active_shopify_graphql/query/relation_spec.rb +++ b/spec/active_shopify_graphql/query/relation_spec.rb @@ -198,7 +198,7 @@ relation = described_class.new(customer_class) - expect { relation.find }.to raise_error(ArgumentError, "find requires an ID argument unless using Customer Account API") + expect { relation.find }.to raise_error(ArgumentError, "find requires an ID argument") end it "raises ObjectNotFoundError when current customer is not found" do @@ -213,7 +213,7 @@ loader_extra_args: ["test_token"] ) - expect { relation.find }.to raise_error(ActiveShopifyGraphQL::ObjectNotFoundError, "Couldn't find current customer") + expect { relation.find }.to raise_error(ActiveShopifyGraphQL::ObjectNotFoundError, "Couldn't find Customer") end end From 26dce87c88cb61e02f6df24515aefc81ca0b55d2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=B2=20Rebughini?= Date: Thu, 26 Mar 2026 13:09:17 +0100 Subject: [PATCH 7/8] Remove unneeded respond_to? call --- lib/active_shopify_graphql/model/graphql_type_resolver.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/active_shopify_graphql/model/graphql_type_resolver.rb b/lib/active_shopify_graphql/model/graphql_type_resolver.rb index c4404cc..c9cb1be 100644 --- a/lib/active_shopify_graphql/model/graphql_type_resolver.rb +++ b/lib/active_shopify_graphql/model/graphql_type_resolver.rb @@ -44,7 +44,7 @@ def graphql_type_for_loader(loader_class) return loader_type if loader_type # 4. Infer from model name - return name.demodulize if respond_to?(:name) && name + return name.demodulize if name raise NotImplementedError, "#{self} must define graphql_type or #{loader_class} must define graphql_type" From b383bc2c1664ad4c5c505df6c2c7c2be72d3bcbd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=B2=20Rebughini?= Date: Thu, 26 Mar 2026 13:16:33 +0100 Subject: [PATCH 8/8] Deduplicate graphql type resolver --- .../model/graphql_type_resolver.rb | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-) diff --git a/lib/active_shopify_graphql/model/graphql_type_resolver.rb b/lib/active_shopify_graphql/model/graphql_type_resolver.rb index c9cb1be..008eb1f 100644 --- a/lib/active_shopify_graphql/model/graphql_type_resolver.rb +++ b/lib/active_shopify_graphql/model/graphql_type_resolver.rb @@ -57,17 +57,7 @@ def graphql_type_for_loader(loader_class) # @param loader_class [Class, nil] The loader class # @return [String] The resolved GraphQL type def resolve_graphql_type(model_class: nil, loader_class: nil) - if model_class.respond_to?(:graphql_type_for_loader) && loader_class - model_class.graphql_type_for_loader(loader_class) - elsif model_class.respond_to?(:graphql_type) - model_class.graphql_type - elsif loader_class.respond_to?(:graphql_type) - loader_class.graphql_type - elsif model_class.respond_to?(:name) && model_class.name - model_class.name.demodulize - else - raise ArgumentError, "Cannot resolve graphql_type from provided arguments" - end + ActiveShopifyGraphQL::Model::GraphqlTypeResolver.resolve(model_class: model_class, loader_class: loader_class) end end