MONGOID-5028 demongoize on pluck (#5176)

* MONGOID-5028 demongoize on pluck

* MONGOID-5028 add demongoize to distinct

* MONGOID-5028 add feature flag and tests. Revamped config tests

* MONGOID-5028 remove byebug

* MONGOID-5028 update docs and release notes

* MONGOID-5028 add back broken scoping

* MONGOID-5028 change language

* MONGOID-5028 update tests

* MONGOID-5028 sort test results

* MONGOID-5028 revert config spec

* MONGOID-5028 make tests more explicit

* MONGOID-5028 expand more tests

Author: neilshweky

docs/reference/configuration.txt

@@ -182,7 +182,7 @@ for details on driver options.
           # determined by the driver will be used. Please refer to:
           # https://docs.mongodb.com/ruby-driver/current/reference/authentication/#auth-source
           auth_source: admin
-          
+
           # Connect directly to and perform all operations on the specified
           # server, bypassing replica set node discovery and monitoring.
           # Exactly one host address must be specified. (default: false)
@@ -367,6 +367,12 @@ for details on driver options.
       # to parent contexts by default. (default: false)
       join_contexts: false
 
+      # Maintain legacy behavior of pluck and distinct, which does not demongoize
+      # values on returning them. Setting this option to false will cause
+      # pluck and distinct to return demongoized values.
+      # (default: true in Mongoid 7, will change to false in Mongoid 8)
+      legacy_pluck_distinct: true
+
       # Maintain legacy behavior of === on Mongoid document classes, which
       # returns true in a number of cases where Ruby's === implementation would
       # return false. Note that the behavior of === on Mongoid document

docs/release-notes/mongoid-7.4.txt

@@ -179,7 +179,7 @@ in Mongoid 7.4 for backwards compatibility. In Mongoid 8 the default value
 will change to ``true``.
 
 
-Respect Field Aliases In Embedded Documents When Using ``distinct`` And ``pluck`` 
+Respect Field Aliases In Embedded Documents When Using ``distinct`` And ``pluck``
 ---------------------------------------------------------------------------------
 
 When ``distinct`` and ``pluck`` are used with aliased fields in embedded
@@ -273,10 +273,10 @@ and then to a non-nil value, for example:
 
   class Canvas
     include Mongoid::Document
-    
+
     embeds_one :palette
   end
-  
+
   canvas.palette = palette
   canvas.palette = nil
   canvas.palette = palette
@@ -345,7 +345,7 @@ For example:
     Band.with_scope(active: true) do
       # ...
     end
-    
+
     # {year: 2020} condition is applied here
   end
 
@@ -359,11 +359,51 @@ all scopes are cleared:
     Band.with_scope(active: true) do
       # ...
     end
-    
+
     # No scope is applied here
   end
 
 .. note::
 
   The value of ``Mongoid.broken_scoping`` option will change to ``false``
   by default in Mongoid 8.0.
+
+
+Demongoize Values Returned from Pluck and Distinct
+--------------------------------------------------
+
+Mongoid 7.4 with the ``Mongoid.legacy_pluck_distinct`` option set to ``false``
+will demongoize values returned from the pluck and distinct methods.
+For example:
+
+.. code-block:: ruby
+
+  class Band
+    include Mongoid::Document
+
+    field :sales, type: BigDecimal
+  end
+
+  Band.create!(sales: "1E2")
+  Band.create!(sales: "2E2")
+  Band.pluck(:sales)
+  # => [0.1e3, 0.2e3]
+  Band.distinct(:sales)
+  # => [0.1e3, 0.2e3]
+
+
+In Mongoid 7.3 and earlier, and in 7.4 with the ``Mongoid.legacy_pluck_distinct``
+option set to ``true`` (the default), the value returned from the pluck and
+distinct methods will not be demongoized. For example:
+
+.. code-block:: ruby
+
+  Band.pluck(:sales)
+  # => ["1E2", "2E2"]
+  Band.distinct(:sales)
+  # => ["1E2", "2E2"]
+
+.. note::
+
+  The value of ``Mongoid.legacy_pluck_distinct`` option will change to ``false``
+  by default in Mongoid 8.0.

lib/mongoid/config.rb

@@ -110,6 +110,10 @@ module Config
     # the one monkey-patched into Mongoid.
     option :object_id_as_json_oid, default: true
 
+    # Maintain legacy behavior of pluck and distinct, which does not
+    # demongoize the values on returning them.
+    option :legacy_pluck_distinct, default: true
+
     # Has Mongoid been configured? This is checking that at least a valid
     # client config exists.
     #

lib/mongoid/contextual/mongo.rb

@@ -123,8 +123,13 @@ def destroy
       #
       # @return [ Array<Object> ] The distinct values for the field.
       def distinct(field)
-        view.distinct(klass.database_field_name(field)).map do |value|
-          value.class.demongoize(value)
+        field_name = klass.database_field_name(field)
+        view.distinct(field_name).map do |value|
+          if !Mongoid.legacy_pluck_distinct && field = klass.fields[field_name]
+            field.demongoize(value)
+          else
+            value.class.demongoize(value)
+          end
         end
       end
 
@@ -422,7 +427,14 @@ def pluck(*fields)
 
         view.projection(normalized_select).reduce([]) do |plucked, doc|
           values = normalized_select.keys.map do |n|
-            n =~ /\./ ? doc[n.partition('.')[0]] : doc[n]
+            res = n =~ /\./ ? doc[n.partition('.')[0]] : doc[n]
+            if Mongoid.legacy_pluck_distinct
+              res
+            elsif field = klass.fields[n]
+              field.demongoize(res)
+            else
+              res.class.demongoize(res)
+            end
           end
           plucked << (values.size == 1 ? values.first : values)
         end

spec/mongoid/config_spec.rb

@@ -328,6 +328,14 @@
     it_behaves_like "a config option"
   end
 
+  context 'when setting the legacy_pluck_distinct option in the config' do
+    let(:option) { :legacy_pluck_distinct }
+    let(:default) { true }
+
+    it_behaves_like "a config option"
+  end
+
+
   describe "#load!" do
 
     before(:all) do

spec/mongoid/contextual/mongo_spec.rb

@@ -473,8 +473,9 @@
   describe "#distinct" do
 
     before do
-      Band.create!(name: "Depeche Mode", years: 30)
-      Band.create!(name: "New Order", years: 25)
+      Band.create!(name: "Depeche Mode", years: 30, sales: "1E2")
+      Band.create!(name: "New Order", years: 25, sales: "2E3")
+      Band.create!(name: "10,000 Maniacs", years: 20, sales: "1E2")
     end
 
     context "when limiting the result set" do
@@ -503,7 +504,7 @@
       end
 
       it "returns the distinct field values" do
-        expect(context.distinct(:name)).to eq([ "Depeche Mode", "New Order" ])
+        expect(context.distinct(:name).sort).to eq([ "10,000 Maniacs", "Depeche Mode", "New Order" ].sort)
       end
     end
 
@@ -518,7 +519,7 @@
       end
 
       it "returns the distinct field values" do
-        expect(context.distinct(:years).sort).to eq([ 25, 30 ])
+        expect(context.distinct(:years).sort).to eq([ 20, 25, 30 ])
       end
     end
 
@@ -534,15 +535,41 @@
       end
 
       let(:expected_results) do
-        ["Depeche Mode", "New Order"]
+        ["10,000 Maniacs", "Depeche Mode", "New Order"]
       end
 
       let(:criteria) do
         Band.where({}).collation(locale: 'en_US', strength: 2)
       end
 
       it 'applies the collation' do
-        expect(context.distinct(:name)).to eq(expected_results)
+        expect(context.distinct(:name).sort).to eq(expected_results.sort)
+      end
+    end
+
+    context "when providing a demongoizable field" do
+      let(:criteria) do
+        Band.criteria
+      end
+
+      let(:context) do
+        described_class.new(criteria)
+      end
+
+      context "when legacy_pluck_distinct is set" do
+        config_override :legacy_pluck_distinct, true
+
+        it "returns the non-demongoized distinct field values" do
+          expect(context.distinct(:sales).sort).to eq([ "1E2", "2E3" ])
+        end
+      end
+
+      context "when legacy_pluck_distinct is not set" do
+        config_override :legacy_pluck_distinct, false
+
+        it "returns the non-demongoized distinct field values" do
+          expect(context.distinct(:sales).sort).to eq([ BigDecimal("1E2"), BigDecimal("2E3") ])
+        end
       end
     end
   end

spec/mongoid/criteria_spec.rb

@@ -2830,6 +2830,10 @@ class D
       Band.create!(name: "Photek", likes: 1)
     end
 
+    let(:maniacs) do
+      Band.create!(name: "10,000 Maniacs", likes: 1, sales: "1E2")
+    end
+
     context "when the field is aliased" do
 
       let!(:expensive) do
@@ -3026,13 +3030,24 @@ class D
       end
 
       context 'when plucking the entire field' do
-
         let(:plucked) do
           Dictionary.all.pluck(:description)
         end
 
-        it 'returns all translations' do
-          expect(plucked.first).to eq({'en' => 'english-text', 'de' => 'deutsch-text'})
+        context "when legacy_pluck_distinct is set" do
+          config_override :legacy_pluck_distinct, true
+
+          it 'returns the non-demongoized translations' do
+            expect(plucked.first).to eq({"de"=>"deutsch-text", "en"=>"english-text"})
+          end
+        end
+
+        context "when legacy_pluck_distinct is not set" do
+          config_override :legacy_pluck_distinct, false
+
+          it 'returns the demongoized translations' do
+            expect(plucked.first).to eq('deutsch-text')
+          end
         end
       end
 
@@ -3047,6 +3062,31 @@ class D
         end
       end
     end
+
+    context 'when plucking a field to be demongoized' do
+
+      let(:plucked) do
+        Band.where(name: maniacs.name).pluck(:sales)
+      end
+
+      context "when legacy_pluck_distinct is set" do
+        config_override :legacy_pluck_distinct, true
+
+        it "does not demongoize the field" do
+          expect(plucked.first).to be_a(String)
+          expect(plucked.first).to eq("1E2")
+        end
+      end
+
+      context "when legacy_pluck_distinct is not set" do
+        config_override :legacy_pluck_distinct, false
+
+        it "demongoizes the field" do
+          expect(plucked.first).to be_a(BigDecimal)
+          expect(plucked.first).to eq(BigDecimal("1E2"))
+        end
+      end
+    end
   end
 
   describe "#respond_to?" do
@@ -3648,7 +3688,7 @@ def self.ages; self; end
     # Used to test MONGOID-5251 where the find command was adding unnecessary
     # and clauses. Since the find command creates the criteria and executes it,
     # it is difficult to analyze the criteria used. For this reason, I have
-    # extracted the crux of the issue, adding an _id to the the criteria twice, 
+    # extracted the crux of the issue, adding an _id to the the criteria twice,
     # and used that for the test case.
     context "when searching by _id twice" do
       let(:_id) { BSON::ObjectId.new }

spec/support/macros.rb

@@ -29,12 +29,12 @@ def config_override(key, value)
       end
     end
 
-    def with_config_values(key, *values)
+    def with_config_values(key, *values, &block)
       values.each do |value|
         context "when #{key} is #{value}" do
           config_override key, value
 
-          yield
+          class_exec(value, &block)
         end
       end
     end