From 01d1bc9679c536956b1d1e70eff45dbab1fbb2c1 Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Tue, 6 Aug 2024 00:49:13 +0300 Subject: [PATCH 1/3] fix(dataproducer): CurrentUser result is uncacheable --- .../GraphQL/DataProducer/User/CurrentUser.php | 10 +- .../Kernel/DataProducer/CurrentUserTest.php | 92 +++++++++++++++++++ 2 files changed, 97 insertions(+), 5 deletions(-) create mode 100644 tests/src/Kernel/DataProducer/CurrentUserTest.php diff --git a/src/Plugin/GraphQL/DataProducer/User/CurrentUser.php b/src/Plugin/GraphQL/DataProducer/User/CurrentUser.php index 8f4d6ab37..b3fb27986 100644 --- a/src/Plugin/GraphQL/DataProducer/User/CurrentUser.php +++ b/src/Plugin/GraphQL/DataProducer/User/CurrentUser.php @@ -42,7 +42,7 @@ public static function create(ContainerInterface $container, array $configuratio } /** - * CurrentUser constructor. + * Constructs a new CurrentUser data producer. * * @param array $configuration * A configuration array containing information about the plugin instance. @@ -59,7 +59,7 @@ public function __construct(array $configuration, string $plugin_id, array $plug } /** - * Returns current user. + * Returns the current user. * * @param \Drupal\graphql\GraphQL\Execution\FieldContext $field_context * Field context. @@ -68,9 +68,9 @@ public function __construct(array $configuration, string $plugin_id, array $plug * The current user. */ public function resolve(FieldContext $field_context): AccountInterface { - // Response must be cached based on current user as a cache context, - // otherwise a new user would became a previous user. - $field_context->addCacheableDependency($this->currentUser); + // Response must be cached per user so that information from previously + // logged in users will not leak to newly logged in users. + $field_context->addCacheContexts(['user']); return $this->currentUser; } diff --git a/tests/src/Kernel/DataProducer/CurrentUserTest.php b/tests/src/Kernel/DataProducer/CurrentUserTest.php new file mode 100644 index 000000000..872f2c6dd --- /dev/null +++ b/tests/src/Kernel/DataProducer/CurrentUserTest.php @@ -0,0 +1,92 @@ +users = [ + $this->createUser(), + $this->createUser(), + ]; + + // Log out initially. + $this->container->get('current_user')->setAccount(User::getAnonymousUser()); + } + + /** + * @covers \Drupal\graphql\Plugin\GraphQL\DataProducer\User\CurrentUser::resolve + */ + public function testCurrentUser(): void { + // Initially no user is logged in. + $result = $this->executeDataProducer('current_user'); + $this->assertInstanceOf(AccountInterface::class, $result); + $this->assertEquals(0, $result->id()); + + // Log in as the first user. + $this->container->get('current_user')->setAccount($this->users[0]); + $result = $this->executeDataProducer('current_user'); + $this->assertInstanceOf(AccountInterface::class, $result); + $this->assertEquals($this->users[0]->id(), $result->id()); + + // Log in as the second user. + $this->container->get('current_user')->setAccount($this->users[1]); + $result = $this->executeDataProducer('current_user'); + $this->assertInstanceOf(AccountInterface::class, $result); + $this->assertEquals($this->users[1]->id(), $result->id()); + + // Log out again. + $this->container->get('current_user')->setAccount(User::getAnonymousUser()); + $result = $this->executeDataProducer('current_user'); + $this->assertInstanceOf(AccountInterface::class, $result); + $this->assertEquals(0, $result->id()); + } + + /** + * {@inheritdoc} + */ + protected function executeDataProducer($id, array $contexts = []) { + /** @var \Drupal\graphql\Plugin\DataProducerPluginManager $manager */ + $manager = $this->container->get('plugin.manager.graphql.data_producer'); + + /** @var \Drupal\graphql\Plugin\DataProducerPluginInterface $plugin */ + $plugin = $manager->createInstance($id); + + // The 'user' cache context should be added so that the results will be + // cached per user. + $context = $this->prophesize(FieldContext::class); + $context->addCacheContexts(['user'])->willReturn($context->reveal())->shouldBeCalled(); + + return $plugin->resolveField($context->reveal()); + } + +} From 2eb82f02e9d0765f3766a28b3fed408cc5298e9b Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Tue, 6 Aug 2024 01:01:55 +0300 Subject: [PATCH 2/3] Reorder use statements. --- tests/src/Kernel/DataProducer/CurrentUserTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/src/Kernel/DataProducer/CurrentUserTest.php b/tests/src/Kernel/DataProducer/CurrentUserTest.php index 872f2c6dd..1fa13f3b8 100644 --- a/tests/src/Kernel/DataProducer/CurrentUserTest.php +++ b/tests/src/Kernel/DataProducer/CurrentUserTest.php @@ -5,9 +5,9 @@ namespace Drupal\Tests\graphql\Kernel\DataProducer; use Drupal\Core\Session\AccountInterface; +use Drupal\graphql\GraphQL\Execution\FieldContext; use Drupal\Tests\graphql\Kernel\GraphQLTestBase; use Drupal\Tests\user\Traits\UserCreationTrait; -use Drupal\graphql\GraphQL\Execution\FieldContext; use Drupal\user\Entity\User; /** From 5a6c3bfa8d9eab249aa8b6e29f83b993b2b0346c Mon Sep 17 00:00:00 2001 From: Pieter Frenssen Date: Tue, 6 Aug 2024 01:34:40 +0300 Subject: [PATCH 3/3] Introduce a B/C layer. --- config/install/graphql.settings.yml | 1 + config/schema/graphql.schema.yml | 5 ++++ graphql.install | 12 ++++++++ .../GraphQL/DataProducer/User/CurrentUser.php | 29 +++++++++++++++---- 4 files changed, 42 insertions(+), 5 deletions(-) diff --git a/config/install/graphql.settings.yml b/config/install/graphql.settings.yml index abdb9497a..666cff5b9 100644 --- a/config/install/graphql.settings.yml +++ b/config/install/graphql.settings.yml @@ -1 +1,2 @@ +dataproducer_allow_current_user_caching: true dataproducer_populate_default_values: true diff --git a/config/schema/graphql.schema.yml b/config/schema/graphql.schema.yml index cec9ca533..2de5a9259 100644 --- a/config/schema/graphql.schema.yml +++ b/config/schema/graphql.schema.yml @@ -72,6 +72,11 @@ graphql.settings: label: "GraphQL Settings" mapping: # @todo Remove in GraphQL 5. + dataproducer_allow_current_user_caching: + type: boolean + label: "Allow the result of the current_user data producer to be cached" + description: "Legacy setting: Allows the results of resolvers that invoke the current_user data producer to be cached. Set this to true to be future-proof. This setting is deprecated and will be removed in a future release." + # @todo Remove in GraphQL 5. dataproducer_populate_default_values: type: boolean label: "Populate dataproducer context default values" diff --git a/graphql.install b/graphql.install index bf79cf1cd..b54b87f99 100644 --- a/graphql.install +++ b/graphql.install @@ -81,3 +81,15 @@ function graphql_update_10400() :void { ->set('dataproducer_populate_default_values', FALSE) ->save(); } + +/** + * Preserve caching behavior of current_user data producer for existing sites. + * + * Set dataproducer_allow_current_user_caching to TRUE after you verified that + * your resolvers that use the current_user data producer can handle caching. + */ +function graphql_update_10401() :void { + \Drupal::configFactory()->getEditable('graphql.settings') + ->set('dataproducer_allow_current_user_caching', FALSE) + ->save(); +} diff --git a/src/Plugin/GraphQL/DataProducer/User/CurrentUser.php b/src/Plugin/GraphQL/DataProducer/User/CurrentUser.php index b3fb27986..1604faaa7 100644 --- a/src/Plugin/GraphQL/DataProducer/User/CurrentUser.php +++ b/src/Plugin/GraphQL/DataProducer/User/CurrentUser.php @@ -2,6 +2,7 @@ namespace Drupal\graphql\Plugin\GraphQL\DataProducer\User; +use Drupal\Core\Config\ConfigFactoryInterface; use Drupal\Core\Plugin\ContainerFactoryPluginInterface; use Drupal\Core\Session\AccountInterface; use Drupal\graphql\GraphQL\Execution\FieldContext; @@ -24,10 +25,13 @@ class CurrentUser extends DataProducerPluginBase implements ContainerFactoryPlug /** * The current user. - * - * @var \Drupal\Core\Session\AccountInterface */ - protected $currentUser; + protected AccountInterface $currentUser; + + /** + * The config factory. + */ + protected ConfigFactoryInterface $configFactory; /** * {@inheritdoc} @@ -37,7 +41,8 @@ public static function create(ContainerInterface $container, array $configuratio $configuration, $plugin_id, $plugin_definition, - $container->get('current_user') + $container->get('current_user'), + $container->get('config.factory') ); } @@ -52,10 +57,13 @@ public static function create(ContainerInterface $container, array $configuratio * The plugin implementation definition. * @param \Drupal\Core\Session\AccountInterface $current_user * The current user. + * @param \Drupal\Core\Config\ConfigFactoryInterface $config_factory + * The config factory. */ - public function __construct(array $configuration, string $plugin_id, array $plugin_definition, AccountInterface $current_user) { + public function __construct(array $configuration, string $plugin_id, array $plugin_definition, AccountInterface $current_user, ConfigFactoryInterface $config_factory) { parent::__construct($configuration, $plugin_id, $plugin_definition); $this->currentUser = $current_user; + $this->configFactory = $config_factory; } /** @@ -71,6 +79,17 @@ public function resolve(FieldContext $field_context): AccountInterface { // Response must be cached per user so that information from previously // logged in users will not leak to newly logged in users. $field_context->addCacheContexts(['user']); + + // Previous versions of this plugin were always returning an uncacheable + // result. In order to preserve backwards compatibility a temporary flag + // has been introduced to allow users to opt-in to caching after verifying + // that the result is safe to cache. + // @todo Remove in 5.x. + $allow_caching = $this->configFactory->get('graphql.settings')->get('dataproducer_allow_current_user_caching') ?? FALSE; + if (!$allow_caching) { + $field_context->mergeCacheMaxAge(0); + } + return $this->currentUser; }