Persisted queries support#707
Conversation
Codecov Report
@@ Coverage Diff @@
## 8.x-4.x #707 +/- ##
=============================================
- Coverage 52.69% 52.13% -0.57%
- Complexity 635 674 +39
=============================================
Files 115 116 +1
Lines 1630 1759 +129
=============================================
+ Hits 859 917 +58
- Misses 771 842 +71
Continue to review full report at Codecov.
|
fubhy
left a comment
fubhy
left a comment
There was a problem hiding this comment.
Hi @chindris! Thanks for the work. One important thing imho from my PoV would be to make this fully configurable per schema. We probably want to come up with a pattern for this that we can then also use for other per-schema things like logging, performance tracing (e.g. a field level performance metric evaluator e.g. for newrelic, etc.)
| route_name: entity.graphql_server.collection | ||
| parent: system.admin_config_services | ||
|
|
||
| graphql.query_maps: |
There was a problem hiding this comment.
All features (including query maps) should be scoped per Server/Schema. We will need the ability to configure per-schema "features" for other things in the future too so we should come with a pluggable pattern for this.
E.g. one thing we would also build with a pluggable approach would be logging and performance tracing for individual schemas.
| - { name: config.factory.override, priority: -253 } | ||
|
|
||
| # Query map provider using the composite pattern. | ||
| graphql.query_provider: |
There was a problem hiding this comment.
We might want to make query providers plugins that one can attach to a schema/server.
There was a problem hiding this comment.
Think e.g. about Search API and how it makes indexes extendable with facets, processors/transformers, etc.
…it to return a persistent query loader.
chindris
force-pushed
the
persisted_queries
branch
from
0e1727c to
c142359
Compare
…r, the new menu tab for the persisted queries form and the started the persisted queries configuration form implementation.
|
Thanks for working on this. Feel free to reach out to me on Slack when you have a moment in the next few days to discuss the status of the PR etc. :-) |
…l into persisted_queries
|
@rthideaway can you do a code review of the pull request? Travis tests are passing, only the code test coverage % decreases. This is quite a big change, so we should think about how this could break existing sites that use the graphql module. We should determine if the test coverage is good enough, and then we could merge? |
|
@klausi Regarding the test coverage. Persisted query plugin manager and persisted query plugins are tested well. The missing test coverage relates to PersistedQueriesForm where the persisted query plugins are being enabled, which I would consider not that critical, as the form works by manual testing. The other missing test coverage relates to some of the new methods on Server entity, which couple of them would be covered by covering the form. The rest seems again good based on manual testing. I would go for the merge. |
|
OK I agree, will merge this now. |
5 participants
The basis for implementing persisted queries, with no specific implementation of it (that will be part of a separate module).