From 75daf995ce92ca6f68ca82d45d8ca4adb8931b75 Mon Sep 17 00:00:00 2001 From: Neil Shweky Date: Wed, 31 Aug 2022 14:45:39 -0400 Subject: [PATCH] MONGOID-5226 Allow setting "store_in collection" on document subclass (#5449) * MONGOID-5226 Allow setting "store_in collection" on document subclass * remove comment * MONGOID-5226 add docs and testing * Apply suggestions from code review Co-authored-by: Oleg Pudeyev <39304720+p-mongo@users.noreply.github.com> * MONGOID-5226 answer comments Co-authored-by: Oleg Pudeyev <39304720+p-mongo@users.noreply.github.com> --- docs/reference/inheritance.txt | 53 ++++++++ lib/config/locales/en.yml | 6 - lib/mongoid/clients/storage_options.rb | 7 +- lib/mongoid/clients/validators/storage.rb | 12 -- lib/mongoid/errors.rb | 1 - lib/mongoid/errors/invalid_storage_parent.rb | 2 + spec/mongoid/clients_spec.rb | 135 +++++++++++++++++-- 7 files changed, 179 insertions(+), 37 deletions(-) diff --git a/docs/reference/inheritance.txt b/docs/reference/inheritance.txt index f1fde10526..0ef4bd1f07 100644 --- a/docs/reference/inheritance.txt +++ b/docs/reference/inheritance.txt @@ -247,3 +247,56 @@ either normal setting or through the build and create methods on the association rect = Rectangle.new(width: 100, height: 200) firefox.shapes + + +.. _inheritance-persistence-context: + +Persistence Contexts +==================== + +Mongoid allows the persistence context of a subclass to be changed from the +persistence context of its parent. This means that, using the ``store_in`` +method, we can store the documents of the subclasses in different collections +(as well as different databases, clients) than their parents: + +.. code:: ruby + + class Shape + include Mongoid::Document + store_in collection: :shapes + end + + class Circle < Shape + store_in collection: :circles + end + + class Square < Shape + store_in collection: :squares + end + + Shape.create! + Circle.create! + Square.create! + +Setting the collection on the children causes the documents for those children +to be stored in the set collection, instead of in the parent's collection: + +.. code:: javascript + + > db.shapes.find() + { "_id" : ObjectId("62fe9a493282a43d6b725e10"), "_type" : "Shape" } + > db.circles.find() + { "_id" : ObjectId("62fe9a493282a43d6b725e11"), "_type" : "Circle" } + > db.squares.find() + { "_id" : ObjectId("62fe9a493282a43d6b725e12"), "_type" : "Square" } + +If the collection is set on some of the subclasses and not others, the subclasses +with set collections will store documents in those collections, and the +subclasses without set collections will be store documents in the parent's +collection. + +.. note:: + + Note that changing the collection that a subclass is stored in will cause + documents of that subclass to no longer be found in the results of querying + its parent class. diff --git a/lib/config/locales/en.yml b/lib/config/locales/en.yml index 4942a5431d..fe1f445d80 100644 --- a/lib/config/locales/en.yml +++ b/lib/config/locales/en.yml @@ -300,12 +300,6 @@ en: \_\_\_\_include Mongoid::Document\n \_\_\_\_store_in collection: 'artists', database: 'music'\n \_\_end\n\n" - invalid_storage_parent: - message: "Invalid store_in call on class %{klass}." - summary: "The :store_in macro can only be called on a base Mongoid Document" - resolution: "Remove the store_in call on class %{klass}, as it will use its - parent store configuration. Or remove the hierarchy extension for this - class." invalid_time: message: "'%{value}' is not a valid Time." summary: "Mongoid tries to serialize the values for Date, DateTime, and diff --git a/lib/mongoid/clients/storage_options.rb b/lib/mongoid/clients/storage_options.rb index dd2ddbc755..a54657dbb9 100644 --- a/lib/mongoid/clients/storage_options.rb +++ b/lib/mongoid/clients/storage_options.rb @@ -6,10 +6,7 @@ module StorageOptions extend ActiveSupport::Concern included do - - cattr_accessor :storage_options, instance_writer: false do - storage_options_defaults - end + class_attribute :storage_options, instance_writer: false, default: storage_options_defaults end module ClassMethods @@ -49,7 +46,7 @@ module ClassMethods # @return [ Class ] The model class. def store_in(options) Validators::Storage.validate(self, options) - storage_options.merge!(options) + self.storage_options = self.storage_options.merge(options) end # Reset the store_in options diff --git a/lib/mongoid/clients/validators/storage.rb b/lib/mongoid/clients/validators/storage.rb index 2b16f3d863..ba8ea11de6 100644 --- a/lib/mongoid/clients/validators/storage.rb +++ b/lib/mongoid/clients/validators/storage.rb @@ -20,21 +20,9 @@ module Storage # @param [ Hash, String, Symbol ] options The provided options. def validate(klass, options) valid_keys?(options) or raise Errors::InvalidStorageOptions.new(klass, options) - valid_parent?(klass) or raise Errors::InvalidStorageParent.new(klass) end private - # Determine if the current klass is valid to change store_in - # options - # - # @api private - # - # @param [ Class ] klass - # - # @return [ true, false ] If the class is valid - def valid_parent?(klass) - !klass.superclass.include?(Mongoid::Document) - end # Determine if all keys in the options hash are valid. # diff --git a/lib/mongoid/errors.rb b/lib/mongoid/errors.rb index e530c54f88..cfa2fadd79 100644 --- a/lib/mongoid/errors.rb +++ b/lib/mongoid/errors.rb @@ -34,7 +34,6 @@ require "mongoid/errors/invalid_session_use" require "mongoid/errors/invalid_set_polymorphic_relation" require "mongoid/errors/invalid_storage_options" -require "mongoid/errors/invalid_storage_parent" require "mongoid/errors/invalid_time" require "mongoid/errors/invalid_value" require "mongoid/errors/inverse_not_found" diff --git a/lib/mongoid/errors/invalid_storage_parent.rb b/lib/mongoid/errors/invalid_storage_parent.rb index da235e56bd..5aa0253329 100644 --- a/lib/mongoid/errors/invalid_storage_parent.rb +++ b/lib/mongoid/errors/invalid_storage_parent.rb @@ -4,6 +4,8 @@ module Mongoid module Errors # Raised when calling store_in in a sub-class of Mongoid::Document + # + # @deprecated class InvalidStorageParent < MongoidError # Create the new error. diff --git a/spec/mongoid/clients_spec.rb b/spec/mongoid/clients_spec.rb index 19bda0dce5..d9cdfa5766 100644 --- a/spec/mongoid/clients_spec.rb +++ b/spec/mongoid/clients_spec.rb @@ -738,19 +738,6 @@ end end - context "when provided a class that extends another document" do - - let(:klass) do - Class.new(Band) - end - - it "raises an error" do - expect { - klass.store_in(database: :artists) - }.to raise_error(Mongoid::Errors::InvalidStorageParent) - end - end - context "when provided a hash" do context "when the hash is not valid" do @@ -762,6 +749,128 @@ end end end + + context "when it is called on a subclass" do + + let(:client) { StoreParent.collection.client } + let(:parent) { StoreParent.create! } + let(:child1) { StoreChild1.create! } + let(:child2) { StoreChild2.create! } + + before do + class StoreParent + include Mongoid::Document + end + + class StoreChild1 < StoreParent + end + + class StoreChild2 < StoreParent + end + end + + after do + Object.send(:remove_const, :StoreParent) + Object.send(:remove_const, :StoreChild1) + Object.send(:remove_const, :StoreChild2) + end + + context "when it is not called on the parent" do + + context "when it is called on all subclasses" do + + before do + StoreChild1.store_in collection: :store_ones + StoreChild2.store_in collection: :store_twos + [ parent, child1, child2 ] + end + + let(:db_parent) { client['store_parents'].find.first } + let(:db_child1) { client['store_ones'].find.first } + let(:db_child2) { client['store_twos'].find.first } + + it "stores the documents in the correct collections" do + expect(db_parent).to eq({ "_id" => parent.id, "_type" => "StoreParent" }) + expect(db_child1).to eq({ "_id" => child1.id, "_type" => "StoreChild1" }) + expect(db_child2).to eq({ "_id" => child2.id, "_type" => "StoreChild2" }) + end + + it "only queries from its own collections" do + expect(StoreParent.count).to eq(1) + expect(StoreChild1.count).to eq(1) + expect(StoreChild2.count).to eq(1) + end + end + + context "when it is called on one of the subclasses" do + + before do + StoreChild1.store_in collection: :store_ones + [ parent, child1, child2 ] + end + + let(:db_parent) { client['store_parents'].find.first } + let(:db_child1) { client['store_ones'].find.first } + let(:db_child2) { client['store_parents'].find.to_a.last } + + it "stores the documents in the correct collections" do + expect(db_parent).to eq({ "_id" => parent.id, "_type" => "StoreParent" }) + expect(db_child1).to eq({ "_id" => child1.id, "_type" => "StoreChild1" }) + expect(db_child2).to eq({ "_id" => child2.id, "_type" => "StoreChild2" }) + end + + it "queries from its own collections" do + expect(StoreParent.count).to eq(2) + expect(StoreChild1.count).to eq(1) + expect(StoreChild2.count).to eq(1) + end + end + end + + context "when it is called on the parent" do + + before do + StoreParent.store_in collection: :st_parents + end + + context "when it is called on all subclasses" do + + before do + StoreChild1.store_in collection: :store_ones + StoreChild2.store_in collection: :store_twos + [ parent, child1, child2 ] + end + + let(:db_parent) { client['st_parents'].find.first } + let(:db_child1) { client['store_ones'].find.first } + let(:db_child2) { client['store_twos'].find.first } + + it "stores the documents in the correct collections" do + expect(db_parent).to eq({ "_id" => parent.id, "_type" => "StoreParent" }) + expect(db_child1).to eq({ "_id" => child1.id, "_type" => "StoreChild1" }) + expect(db_child2).to eq({ "_id" => child2.id, "_type" => "StoreChild2" }) + end + end + + context "when it is called on one of the subclasses" do + + before do + StoreChild1.store_in collection: :store_ones + [ parent, child1, child2 ] + end + + let(:db_parent) { client['st_parents'].find.first } + let(:db_child1) { client['store_ones'].find.first } + let(:db_child2) { client['st_parents'].find.to_a.last } + + it "stores the documents in the correct collections" do + expect(db_parent).to eq({ "_id" => parent.id, "_type" => "StoreParent" }) + expect(db_child1).to eq({ "_id" => child1.id, "_type" => "StoreChild1" }) + expect(db_child2).to eq({ "_id" => child2.id, "_type" => "StoreChild2" }) + end + end + end + end end describe ".with" do