From 9372d5a68e62c81464773a2e32a98a989bed26b6 Mon Sep 17 00:00:00 2001 From: Sean Doyle Date: Sun, 26 Jan 2025 23:31:42 -0500 Subject: [PATCH 1/2] Introduce `ActiveResource::WhereClause` Closes [#408][] Introduce a simple chainable `WhereClause` class inspired by [Active Record][]. All methods (including those that integrate with [Enumerable][]) are delegated to `WhereClause#all`, which itself delegates to `Base.find`. By merging parameters through `.where`-chaining, this commit supports deferred fetching: ```ruby people = Person.where(id: 2).where(name: "david") # => GET /people.json?id=2&name=david people = Person.where(id: 2).all(params: { name: "david" }) # => GET /people.json?id=2&name=david people = Person.all(from: "/records.json").where(id: 2) # => GET /records.json?id=2 ``` [#408]: https://github.com/rails/activeresource/issues/408 [Active Record]: https://github.com/rails/rails/blob/main/activerecord/lib/active_record/relation/where_clause.rb [Enumerable]: https://ruby-doc.org/3.4.1/Enumerable.html --- lib/active_resource.rb | 3 ++ lib/active_resource/base.rb | 4 +- lib/active_resource/railtie.rb | 2 + lib/active_resource/where_clause.rb | 47 ++++++++++++++++++++++ test/cases/finder_test.rb | 60 +++++++++++++++++++++++++++++ test/cases/notifications_test.rb | 2 +- 6 files changed, 115 insertions(+), 3 deletions(-) create mode 100644 lib/active_resource/where_clause.rb diff --git a/lib/active_resource.rb b/lib/active_resource.rb index 9662e6978a..4379acd372 100644 --- a/lib/active_resource.rb +++ b/lib/active_resource.rb @@ -49,6 +49,9 @@ module ActiveResource autoload :InheritingHash autoload :Validations autoload :Collection + eager_autoload do + autoload :WhereClause + end if ActiveSupport::VERSION::STRING >= "7.1" def self.deprecator diff --git a/lib/active_resource/base.rb b/lib/active_resource/base.rb index e7e7708437..51b21a70ce 100644 --- a/lib/active_resource/base.rb +++ b/lib/active_resource/base.rb @@ -1062,13 +1062,13 @@ def last(*args) # This is an alias for find(:all). You can pass in all the same # arguments to this method as you can to find(:all) def all(*args) - find(:all, *args) + WhereClause.new(self, *args) end def where(clauses = {}) clauses = sanitize_forbidden_attributes(clauses) raise ArgumentError, "expected a clauses Hash, got #{clauses.inspect}" unless clauses.is_a? Hash - find(:all, params: clauses) + all(params: clauses) end diff --git a/lib/active_resource/railtie.rb b/lib/active_resource/railtie.rb index 604997cafe..800f1208e2 100644 --- a/lib/active_resource/railtie.rb +++ b/lib/active_resource/railtie.rb @@ -5,6 +5,8 @@ module ActiveResource class Railtie < Rails::Railtie + config.eager_load_namespaces << ActiveResource + config.active_resource = ActiveSupport::OrderedOptions.new initializer "active_resource.set_configs" do |app| diff --git a/lib/active_resource/where_clause.rb b/lib/active_resource/where_clause.rb new file mode 100644 index 0000000000..bd9d3ad5fb --- /dev/null +++ b/lib/active_resource/where_clause.rb @@ -0,0 +1,47 @@ +# frozen_string_literal: true + +module ActiveResource + class WhereClause < BasicObject # :nodoc: + delegate_missing_to :resources + + def initialize(resource_class, options = {}) + @resource_class = resource_class + @options = options + @resources = nil + @loaded = false + end + + def where(clauses = {}) + all(params: clauses) + end + + def all(options = {}) + WhereClause.new(@resource_class, @options.deep_merge(options)) + end + + def load + unless @loaded + @resources = @resource_class.find(:all, @options) + @loaded = true + end + + self + end + + def reload + reset + load + end + + private + def resources + load + @resources + end + + def reset + @resources = nil + @loaded = false + end + end +end diff --git a/test/cases/finder_test.rb b/test/cases/finder_test.rb index 2682fe2eb4..56eb95b9f3 100644 --- a/test/cases/finder_test.rb +++ b/test/cases/finder_test.rb @@ -63,6 +63,66 @@ def test_where_with_clauses assert_kind_of StreetAddress, addresses.first end + def test_where_with_multiple_where_clauses + ActiveResource::HttpMock.respond_to.get "/people.json?id=2&name=david", {}, @people_david + + people = Person.where(id: 2).where(name: "david") + assert_equal 1, people.size + assert_kind_of Person, people.first + assert_equal 2, people.first.id + assert_equal "David", people.first.name + end + + def test_where_chained_from_all + ActiveResource::HttpMock.respond_to.get "/records.json?id=2", {}, @people_david + + people = Person.all(from: "/records.json").where(id: 2) + assert_equal 1, people.size + assert_kind_of Person, people.first + assert_equal 2, people.first.id + assert_equal "David", people.first.name + end + + def test_where_with_chained_into_all + ActiveResource::HttpMock.respond_to.get "/records.json?id=2&name=david", {}, @people_david + + people = Person.where(id: 2).all(from: "/records.json", params: { name: "david" }) + assert_equal 1, people.size + assert_kind_of Person, people.first + assert_equal 2, people.first.id + assert_equal "David", people.first.name + end + + def test_where_loading + ActiveResource::HttpMock.respond_to.get "/people.json?id=2", {}, @people_david + people = Person.where(id: 2) + + assert_changes -> { ActiveResource::HttpMock.requests.count }, from: 0, to: 1 do + people.load + end + assert_no_changes -> { ActiveResource::HttpMock.requests.count }, from: 1 do + 10.times { people.load } + end + end + + def test_where_reloading + ActiveResource::HttpMock.respond_to.get "/people.json?id=2", {}, @people_david + people = Person.where(id: 2) + + assert_changes -> { ActiveResource::HttpMock.requests.count }, from: 0, to: 1 do + assert_equal 1, people.size + end + assert_no_changes -> { ActiveResource::HttpMock.requests.count }, from: 1 do + assert_equal 1, people.size + end + assert_changes -> { ActiveResource::HttpMock.requests.count }, from: 1, to: 2 do + people.reload + end + assert_no_changes -> { ActiveResource::HttpMock.requests.count }, from: 2 do + assert_equal 1, people.size + end + end + def test_where_clause_with_unpermitted_params params = StrongParameters.new(person_id: "1") assert_raises(ActiveModel::ForbiddenAttributesError) { StreetAddress.where(params) } diff --git a/test/cases/notifications_test.rb b/test/cases/notifications_test.rb index da4c62778f..1997a5c616 100644 --- a/test/cases/notifications_test.rb +++ b/test/cases/notifications_test.rb @@ -15,7 +15,7 @@ def setup end def test_get_request_with_params - payload = capture_notifications { Person.where(name: "Matz") } + payload = capture_notifications { Person.where(name: "Matz").load } assert_equal :get, payload[:method] assert_equal "http://37s.sunrise.i:3000/people.json?name=Matz", payload[:request_uri] From b1b93e039ca0244d01aeb720f3d738bd34282611 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafael=20Mendon=C3=A7a=20Fran=C3=A7a?= Date: Thu, 11 Sep 2025 16:21:36 +0000 Subject: [PATCH 2/2] Fix deprecation mocha message --- test/cases/connection_test.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/cases/connection_test.rb b/test/cases/connection_test.rb index 1ba009e532..63f254fabf 100644 --- a/test/cases/connection_test.rb +++ b/test/cases/connection_test.rb @@ -258,7 +258,7 @@ def test_accept_http_header @http = mock("new Net::HTTP") @conn.expects(:http).returns(@http) path = "/people/1.xml" - @http.expects(:get).with(path, "Accept" => "application/xhtml+xml").returns(ActiveResource::Response.new(@matz, 200, "Content-Type" => "text/xhtml")) + @http.expects(:get).with(path, { "Accept" => "application/xhtml+xml" }).returns(ActiveResource::Response.new(@matz, 200, "Content-Type" => "text/xhtml")) assert_nothing_raised { @conn.get(path, "Accept" => "application/xhtml+xml") } end