PHPLIB-715: Use flexible numeric comparisons in DocumentMatchConstraint (#958)

* PHPLIB-715: Use flexible numeric comparisons in DocumentMatchConstraint

* Fix phpcs

Author: alcaeus

tests/SpecTests/DocumentsMatchConstraint.php

@@ -35,10 +35,10 @@
 use function array_values;
 use function get_class;
 use function get_debug_type;
-use function in_array;
 use function is_array;
+use function is_float;
+use function is_int;
 use function is_object;
-use function is_scalar;
 use function method_exists;
 use function sprintf;
 
@@ -59,9 +59,6 @@ class DocumentsMatchConstraint extends Constraint
     /** @var boolean */
     private $ignoreExtraKeysInEmbedded = false;
 
-    /** @var array */
-    private $placeholders = [];
-
     /**
      * TODO: This is not currently used, but was preserved from the design of
      * TestCase::assertMatchesDocument(), which would sort keys and then compare
@@ -88,14 +85,12 @@ class DocumentsMatchConstraint extends Constraint
      * @param array|object $value
      * @param boolean      $ignoreExtraKeysInRoot     If true, ignore extra keys within the root document
      * @param boolean      $ignoreExtraKeysInEmbedded If true, ignore extra keys within embedded documents
-     * @param array        $placeholders              Placeholders for any value
      */
-    public function __construct($value, bool $ignoreExtraKeysInRoot = false, bool $ignoreExtraKeysInEmbedded = false, array $placeholders = [])
+    public function __construct($value, bool $ignoreExtraKeysInRoot = false, bool $ignoreExtraKeysInEmbedded = false)
     {
         $this->value = $this->prepareBSON($value, true, $this->sortKeys);
         $this->ignoreExtraKeysInRoot = $ignoreExtraKeysInRoot;
         $this->ignoreExtraKeysInEmbedded = $ignoreExtraKeysInEmbedded;
-        $this->placeholders = $placeholders;
         $this->comparatorFactory = Factory::getInstance();
     }
 
@@ -303,10 +298,6 @@ private function assertEquals(ArrayObject $expected, ArrayObject $actual, bool $
                 throw new RuntimeException(sprintf('$actual is missing key: "%s"', $keyPrefix . $key));
             }
 
-            if (in_array($expectedValue, $this->placeholders, true)) {
-                continue;
-            }
-
             $actualValue = $actual[$key];
 
             if ($expectedValue instanceof BSONDocument && isset($expectedValue['$$type'])) {
@@ -322,26 +313,13 @@ private function assertEquals(ArrayObject $expected, ArrayObject $actual, bool $
                 continue;
             }
 
-            if (is_scalar($expectedValue) && is_scalar($actualValue)) {
-                if ($expectedValue !== $actualValue) {
-                    throw new ComparisonFailure(
-                        $expectedValue,
-                        $actualValue,
-                        '',
-                        '',
-                        false,
-                        sprintf('Field path "%s": %s', $keyPrefix . $key, 'Failed asserting that two values are equal.')
-                    );
-                }
-
-                continue;
-            }
-
             $expectedType = get_debug_type($expectedValue);
             $actualType = get_debug_type($actualValue);
 
-            // Workaround for ObjectComparator printing the whole actual object
-            if ($expectedType !== $actualType) {
+            /* Early check to work around ObjectComparator printing the entire value
+             * for a failed type comparison. Avoid doing this if either value is
+             * numeric to allow for flexible numeric comparisons (e.g. 1 == 1.0). */
+            if ($expectedType !== $actualType && ! (self::isNumeric($expectedValue) || self::isNumeric($actualValue))) {
                 throw new ComparisonFailure(
                     $expectedValue,
                     $actualValue,
@@ -421,6 +399,11 @@ private function doToString()
         return 'matches ' . $this->exporter()->export($this->value);
     }
 
+    private static function isNumeric($value): bool
+    {
+        return is_int($value) || is_float($value) || $value instanceof Int64;
+    }
+
     /**
      * Prepare a BSON document or array for comparison.
      *

tests/SpecTests/DocumentsMatchConstraintTest.php

@@ -43,6 +43,14 @@ public function testIgnoreExtraKeysInRoot(): void
         $this->assertResult(false, $c, [1, ['a' => 1, 'b' => 2]], 'Extra keys in embedded are not permitted');
     }
 
+    public function testFlexibleNumericComparison(): void
+    {
+        $c = new DocumentsMatchConstraint(['x' => 1, 'y' => 1.0]);
+        $this->assertResult(true, $c, ['x' => 1.0, 'y' => 1.0], 'Float instead of expected int matches');
+        $this->assertResult(true, $c, ['x' => 1, 'y' => 1], 'Int instead of expected float matches');
+        $this->assertResult(false, $c, ['x' => 'foo', 'y' => 1.0], 'Different type does not match');
+    }
+
     public function testIgnoreExtraKeysInEmbedded(): void
     {
         $c = new DocumentsMatchConstraint(['x' => 1, 'y' => ['a' => 1, 'b' => 2]], false, true);
@@ -64,15 +72,6 @@ public function testIgnoreExtraKeysInEmbedded(): void
         $this->assertResult(false, $c, [1, ['a' => 2]], 'Keys must have the correct value');
     }
 
-    public function testPlaceholders(): void
-    {
-        $c = new DocumentsMatchConstraint(['x' => '42', 'y' => 42, 'z' => ['a' => 24]], false, false, [24, 42]);
-
-        $this->assertResult(true, $c, ['x' => '42', 'y' => 'foo', 'z' => ['a' => 1]], 'Placeholders accept any value');
-        $this->assertResult(false, $c, ['x' => 42, 'y' => 'foo', 'z' => ['a' => 1]], 'Placeholder type must match');
-        $this->assertResult(true, $c, ['x' => '42', 'y' => 42, 'z' => ['a' => 24]], 'Exact match');
-    }
-
     /**
      * @dataProvider provideBSONTypes
      */
@@ -146,13 +145,13 @@ public function errorMessageProvider()
                 ['foo' => ['foo' => 'bar', 'bar' => 'baz']],
             ],
             'Scalar value not equal' => [
-                'Field path "foo": Failed asserting that two values are equal.',
+                'Field path "foo": Failed asserting that two strings are equal.',
                 new DocumentsMatchConstraint(['foo' => 'bar']),
                 ['foo' => 'baz'],
             ],
             'Scalar type mismatch' => [
-                'Field path "foo": Failed asserting that two values are equal.',
-                new DocumentsMatchConstraint(['foo' => 42]),
+                'Field path "foo": \'42\' is not instance of expected type "bool".',
+                new DocumentsMatchConstraint(['foo' => true]),
                 ['foo' => '42'],
             ],
             'Type mismatch' => [

tests/SpecTests/Operation.php

@@ -12,6 +12,7 @@
 use MongoDB\Driver\Server;
 use MongoDB\Driver\Session;
 use MongoDB\GridFS\Bucket;
+use MongoDB\MapReduceResult;
 use MongoDB\Model\IndexInfo;
 use MongoDB\Operation\FindOneAndReplace;
 use MongoDB\Operation\FindOneAndUpdate;
@@ -206,6 +207,10 @@ public function assert(FunctionalTestCase $test, Context $context, bool $bubbleE
              * is not used (e.g. Command Monitoring spec). */
             if ($result instanceof Cursor) {
                 $result = $result->toArray();
+            } elseif ($result instanceof MapReduceResult) {
+                /* For mapReduce operations, we ignore the mapReduce metadata
+                 * and only return the result iterator for evaluation. */
+                $result = iterator_to_array($result->getIterator());
             }
         } catch (Exception $e) {
             $exception = $e;
@@ -789,7 +794,7 @@ private function getResultAssertionTypeForCollection()
                 return ResultExpectation::ASSERT_SAME_DOCUMENTS;
 
             case 'mapReduce':
-                return ResultExpectation::ASSERT_SAME_DOCUMENTS;
+                return ResultExpectation::ASSERT_DOCUMENTS_MATCH;
 
             case 'replaceOne':
             case 'updateMany':

tests/SpecTests/RetryableReadsSpecTest.php

@@ -26,7 +26,7 @@ class RetryableReadsSpecTest extends FunctionalTestCase
     ];
 
     /** @var array */
-    private static $incompleteTests = ['mapReduce: MapReduce succeeds with retry on' => 'PHPLIB-715'];
+    private static $incompleteTests = [];
 
     /**
      * Assert that the expected and actual command documents match.

tests/UnifiedSpecTests/Constraint/MatchesTest.php

@@ -23,6 +23,14 @@ public function testMatchesDocument(): void
         $this->assertResult(true, $c, ['y' => ['b' => 2, 'a' => 1], 'x' => 1], 'Root and embedded key order is not significant');
     }
 
+    public function testFlexibleNumericComparison(): void
+    {
+        $c = new Matches(['x' => 1, 'y' => 1.0]);
+        $this->assertResult(true, $c, ['x' => 1.0, 'y' => 1.0], 'Float instead of expected int matches');
+        $this->assertResult(true, $c, ['x' => 1, 'y' => 1], 'Int instead of expected float matches');
+        $this->assertResult(false, $c, ['x' => 'foo', 'y' => 1.0], 'Different type does not match');
+    }
+
     public function testDoNotAllowExtraRootKeys(): void
     {
         $c = new Matches(['x' => 1], null, false);