From 874bfc8bd1f42434369acb4ab75b325a4505ac59 Mon Sep 17 00:00:00 2001
From: Jeremy Mikola <jmikola@gmail.com>
Date: Tue, 26 Sep 2023 14:25:42 -0400
Subject: [PATCH] PHPLIB-476: Consider transaction readPreference in
 select_server

This also refactors the conditionals in extract_session_from_options and extract_read_preference_from_options to improve readability.

select_server() previously did not consider the read preference of an active transaction. This isn't very significant, as transactions require a primary read preference, but it is correct to do so.
---
 src/functions.php                             | 37 +++++++++-----
 .../Functions/SelectServerFunctionalTest.php  | 51 +++++++++++++++++++
 2 files changed, 74 insertions(+), 14 deletions(-)
 create mode 100644 tests/Functions/SelectServerFunctionalTest.php

diff --git a/src/functions.php b/src/functions.php
index 95c85380b..9a7228531 100644
--- a/src/functions.php
+++ b/src/functions.php
@@ -544,11 +544,11 @@ function with_transaction(Session $session, callable $callback, array $transacti
  */
 function extract_session_from_options(array $options): ?Session
 {
-    if (! isset($options['session']) || ! $options['session'] instanceof Session) {
-        return null;
+    if (isset($options['session']) && $options['session'] instanceof Session) {
+        $options['session'];
     }
 
-    return $options['session'];
+    return null;
 }
 
 /**
@@ -558,33 +558,42 @@ function extract_session_from_options(array $options): ?Session
  */
 function extract_read_preference_from_options(array $options): ?ReadPreference
 {
-    if (! isset($options['readPreference']) || ! $options['readPreference'] instanceof ReadPreference) {
-        return null;
+    if (isset($options['readPreference']) && $options['readPreference'] instanceof ReadPreference) {
+        return $options['readPreference'];
     }
 
-    return $options['readPreference'];
+    return null;
 }
 
 /**
- * Performs server selection, respecting the readPreference and session options
- * (if given)
+ * Performs server selection, respecting the readPreference and session options.
+ *
+ * The pinned server for an active transaction takes priority, followed by an
+ * operation-level read preference, followed by an active transaction's read
+ * preference, followed by a primary read preference.
  *
  * @internal
  */
 function select_server(Manager $manager, array $options): Server
 {
     $session = extract_session_from_options($options);
-    $server = $session instanceof Session ? $session->getServer() : null;
-    if ($server !== null) {
-        return $server;
+
+    // Pinned server for an active transaction takes priority
+    if ($session instanceof Session && $session->getServer() instanceof Server) {
+        return $session->getServer();
     }
 
+    // Operation read preference takes priority
     $readPreference = extract_read_preference_from_options($options);
-    if (! $readPreference instanceof ReadPreference) {
-        // TODO: PHPLIB-476: Read transaction read preference once PHPC-1439 is implemented
-        $readPreference = new ReadPreference(ReadPreference::PRIMARY);
+
+    // Read preference for an active transaction takes priority
+    if ($readPreference === null && $session instanceof Session && $session->isInTransaction()) {
+        /* Session::getTransactionOptions() should always return an array if the
+         * session is in a transaction, but we can be defensive. */
+        $readPreference = extract_read_preference_from_options($session->getTransactionOptions() ?? []);
     }
 
+    // Manager::selectServer() defaults to a primary read preference
     return $manager->selectServer($readPreference);
 }
 
diff --git a/tests/Functions/SelectServerFunctionalTest.php b/tests/Functions/SelectServerFunctionalTest.php
new file mode 100644
index 000000000..79d20dea4
--- /dev/null
+++ b/tests/Functions/SelectServerFunctionalTest.php
@@ -0,0 +1,51 @@
+<?php
+
+namespace MongoDB\Tests\Functions;
+
+use MongoDB\Driver\ReadPreference;
+use MongoDB\Driver\Server;
+use MongoDB\Tests\FunctionalTestCase;
+
+use function MongoDB\select_server;
+
+class SelectServerFunctionalTest extends FunctionalTestCase
+{
+    /** @dataProvider providePinnedOptions */
+    public function testSelectServerPrefersPinnedServer(array $options): void
+    {
+        $this->skipIfTransactionsAreNotSupported();
+
+        if (! $this->isShardedCluster()) {
+            $this->markTestSkipped('Pinning requires a sharded cluster');
+        }
+
+        if (! $this->isLoadBalanced()) {
+            $this->markTestSkipped('libmongoc does not pin for load-balanced topology');
+        }
+
+        /* By default, the Manager under test is created with a single-mongos
+         * URI. Explicitly create a Client with multiple mongoses. */
+        $client = static::createTestClient(static::getUri(true));
+
+        // Collection must be created before the transaction starts
+        $this->createCollection($this->getDatabaseName(), $this->getCollectionName());
+
+        $session = $client->startSession();
+        $session->startTransaction();
+
+        $collection = $client->selectCollection($this->getDatabaseName(), $this->getCollectionName());
+        $collection->find([], ['session' => $session]);
+
+        $this->assertTrue($session->isInTransaction());
+        $this->assertInstanceOf(Server::class, $session->getServer(), 'Session is pinned');
+        $this->assertEquals($session->getServer(), select_server($client->getManager(), ['session' => $session]));
+    }
+
+    public static function providePinnedOptions(): array
+    {
+        return [
+            [['readPreference' => new ReadPreference(ReadPreference::PRIMARY_PREFERRED)]],
+            [[]],
+        ];
+    }
+}