-
Notifications
You must be signed in to change notification settings - Fork 201
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add APQ plugin #1189
feat: add APQ plugin #1189
Conversation
Cool idea! Some reviewer thoughts:
|
I think it is an opt-in feature because you have to enable the plugin on admin/config/graphql/servers/manage/[server]/persisted_queries. |
* 8.x-4.x: fix(dataproducer): Fix language definition to be single value in entity reference producers (drupal-graphql#1241) docs(server): Improved class property descriptions feat(server): Add configurable validation security rules for introspection and query complexity (drupal-graphql#1244) tests(buffer): Fix ArryObject usage to not depend on Zend (drupal-graphql#1247) test(phpstan): Add missing return type hint fix(file-upldoad): Validate files in the correct order fix(DataProducer): Fix entity reference loading by language (drupal-graphql#1232) tests(assertions): Implement leaked metadata detection for QueryResultAssertionTrait (drupal-graphql#1207) tests(github): Switch testing to Drupal core 9.2.x (drupal-graphql#1215) chore(voyager): yarn audit security updates (drupal-graphql#1214) test(phpstan): Enable PHPStan run on PHP 8.0 (drupal-graphql#1211) refactor(executor): Replace deprecated AST::getOperation() with AST::getOperationAST() (drupal-graphql#1210) fix(executor): Remove swapped errors compatibility mode
@Kingdutch @klausi I would like to hear more feedback on this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the improvements and for requesting feedback! I think we can improve the robustness of this feature by taking a closer look at what Apollo does for its own server implementation.
* 8.x-4.x: test(dataprovider): Rename dataprovider functions to not be accidentally tested (drupal-graphql#1266) fix(EntityLoad): Return NULL on NULL entity IDs when composing entity load (drupal-graphql#1174) fix(entity_reference): Return emtpy arrays instead of NULL (drupal-graphql#1265) chore(voyager): JS dependency updates with yarn audit chore(explorer): JS dependency updates with yarn audit test(coder): Update Coder to 8.3.14 (drupal-graphql#1264) test(phpstan): Enable PHPStan on PHP 8 by disabling PHP opcache (drupal-graphql#1262) feat(server): Log unsafe server errors for better error tracing (drupal-graphql#1258) test(phpstan): Ignore PHPStan warning directly in code with a comment (drupal-graphql#1263) test(phpstan): Ignore a false unreachable statement error (drupal-graphql#1261) test(phpstan): Update PHPStan and dependencies (drupal-graphql#1257) test(php): Add PHP 8.1 for testing (drupal-graphql#1256) test(core): Update Drupal core to 9.3 for testing (drupal-graphql#1255)
* 8.x-4.x: refactor(drupal): Drop Drupal 8 support, introduce Drupal 10 compat (drupal-graphql#1267)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't compare to the Apollo server source code a second time (I don't have the time at the moment but didn't want to let you wait longer).
The code as-is looks good to me. I added one nitpick based on my own PHPStan usage for an error that only shows up with "strict rules" enabled. We don't have that in this module so I'm fine either way. The change in the ServerInterface
perhaps requires a bit more debate, but that line change can be undone and discussed without blocking this issue further (since the issue it's attempting to fix is not caused/changed by this PR).
With regards to the security implications I raised at the start of the PR, I'm satisfied with this being an opt-in feature that people themselves can only enabled when they're dealing with known and trusted clients (enforced through the existing GraphQL permission system).
@@ -60,7 +60,7 @@ public function removeAllPersistedQueryInstances(); | |||
/** | |||
* Returns the current persisted queries set. | |||
* | |||
* @return \Drupal\graphql\Plugin\PersistedQueryPluginInterface[] | |||
* @return \Drupal\graphql\Plugin\PersistedQueryPluginInterface[]|null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would technically be a breaking change. Since the following code would break:
foreach ($this->getPersistedQueryInstances() as $instance) {}
I suspect this is something suggested by PHPStan? I can see where it comes from in the Server implementation. That implementation uses PHP's "convert NULL to empty array on array access" side-effect.
I think the better (and more ergonomic for callers) fix would be to add $this-> persisted_query_instances = [];
, right after the is_null
if statement's closing bracked at the top of the function. That has a few positive effects:
- The function never returns NULL so calling it is easier (because I'm only dealing with arrays)
- If there are no plugins defined then we don't search for them every time
getPersistedQueryInstances
is called (which happens now, because in the no-plugin case,$this->persisted_query_instances
is stillNULL
when the function is called after the first time).
For clarity we should probably just split that change out into a separate PR since it's not necessarily related to APQ? Then this PR doesn't need to be blocked by that change either :)
I would like to add an automatic persisted query plugin the plays nicely together with https://www.apollographql.com/docs/react/api/link/persisted-queries, for example.