silverstripe / silverstripe-graphql

Serves Silverstripe data as GraphQL representations
BSD 3-Clause "New" or "Revised" License
52 stars 61 forks source link

QueryCachingMiddleware vars fix #584

Closed theTigerDuck closed 2 months ago

theTigerDuck commented 3 months ago

Module version(s) affected

lastest

Description

Shouldn't it be: $vars = $vars['vars'] ?? $vars; instead of $vars = $vars['vars'] ?? [];

I use it like this:

---
Name: 'graphql-middlewares-caching'
After: '#graphql-middlewares'
---
SilverStripe\Core\Injector\Injector:
  # default implementation
  SilverStripe\GraphQL\QueryHandler\QueryHandlerInterface:
    properties:
      Middlewares:
        cache: '%$SilverStripe\GraphQL\Middleware\QueryCachingMiddleware'
SilverStripe\ORM\DataObject:
  extensions:
    - SilverStripe\GraphQL\Extensions\QueryRecorderExtension

So it caches the Filelistings for Files on S3, because unpublished Files make it superslow. I cache it on a redis like:

  Psr\SimpleCache\CacheInterface.graphql:
    factory: RedisCacheFactory

Should I make a pullRequest or why not?

How to reproduce

---
Name: 'graphql-middlewares-caching'
After: '#graphql-middlewares'
---
SilverStripe\Core\Injector\Injector:
  # default implementation
  SilverStripe\GraphQL\QueryHandler\QueryHandlerInterface:
    properties:
      Middlewares:
        cache: '%$SilverStripe\GraphQL\Middleware\QueryCachingMiddleware'
SilverStripe\ORM\DataObject:
  extensions:
    - SilverStripe\GraphQL\Extensions\QueryRecorderExtension

So it caches the Filelistings for Files on S3, because unpublished Files make it superslow. I cache it on a redis like:

  Psr\SimpleCache\CacheInterface.graphql:
    factory: RedisCacheFactory

open AssetAdmin

Possible Solution

$vars = $vars['vars'] ?? $vars; instead of $vars = $vars['vars'] ?? [];

Additional Context

No response

Validations

GuySartorelli commented 3 months ago

What is the actual problem you're reporting? You've got a lot of code samples there, but you haven't said what the bug is. Are you getting an error? If so, what is the error, and what do you to that triggers the error? Is there something that isn't working the way you expect it to? If so, what are you doing, what happens, and what do you expect to happen instead?

theTigerDuck commented 3 months ago

The $vars that come in are just fine. There are no $vars['vars'], so when I open AssetAdmin it looks like this: Screenshot 2024-05-14 at 11-25-31 Silverstripe - Files

theTigerDuck commented 3 months ago

Added a pullRequest so you can see what I intend to do here. https://github.com/theTigerDuck/silverstripe-graphql/pull/1

theTigerDuck commented 3 months ago

Another issue. It proves for LastEdited, if you delete anything it won't refresh the cache.

GuySartorelli commented 3 months ago

You've mentioned redis in the issue description - does the error only get hit when you're using redis? If so, it seems likely there's something wrong with the way redis is being used.

If not, please explain how to trigger this error (step by step - so far your "how to reproduce" lacks any steps I can actually follow to see the error happen locally) in a setup that doesn't use redis.

GuySartorelli commented 2 months ago

Since I can't reproduce this, and no further information has been provided in the last month, I'm going to close it.

If someone can provide clear steps to reproduce this I will reopen it.