feat: introduce "graphql_query_analyzer_get_headers" filter#3020
feat: introduce "graphql_query_analyzer_get_headers" filter#3020jasonbahl merged 10 commits intowp-graphql:developfrom
Conversation
|
Hey @MuhammedAO , thanks so much for this PR 🙌 Could you perhaps provide some contexts regarding why you're suggesting these changes? I don't see any tests to derive a use case from, and if we're going to pollute the instance with static state (instead of a nullable check on an existing instance properties) we should have a strong reason. Similarly, you're setting the flag as soon as processing starts - is that intentional? If so we probably want to rename away from the past-tense |
|
@justlevine So this stems from a conversation I've been having with @jasonbahl over on Slack. We've been trying to resolve wp-graphql/wp-graphql-smart-cache#223. @MuhammedAO works for our dev team at Dexerto. We needed a way to know when the query returns a response with no resolved nodes so we can do something like: add_filter(
'graphql_process_http_request_response',
function ( mixed $response, mixed $result, mixed $operation, mixed $request ): mixed {
if ( class_exists( \WPGraphQL\Utils\QueryAnalyzer::class ) &&
method_exists( \WPGraphQL\Utils\QueryAnalyzer::class, 'areNodesResolved' ) &&
method_exists( \WPGraphQL\Utils\QueryAnalyzer::class, 'resetNodesResolved' ) ) {
$nodes_resolved = \WPGraphQL\Utils\QueryAnalyzer::areNodesResolved();
if ( ! $nodes_resolved ) {
header( 'Cache-Control: no-cache, must-revalidate' );
}
// Correctly reset nodes_resolved for the next request.
\WPGraphQL\Utils\QueryAnalyzer::resetNodesResolved();
}
return $response;
},
10,
4
);This way we can avoid having cached responses with no cache tags, which just get stuck in the cache forever. |
|
@humet thanks for the clarification. I wonder if the following two alternatives would work?
E.g. add_filter(
'graphql_process_http_request_response',
function ( mixed $response, mixed $result, mixed $operation, mixed $request ): mixed {
if ( ! empty( $response['X-GraphQL-Keys'] ) { // 👈 (we can also conditionally check and invalidate based a specific key.
header( 'Cache-Control: no-cache, must-revalidate' );
// We no longer need to reset between requests, since we're not dealing with static state.
}
return $response;
},
10,
4
);
add_filter(
'graphql_process_http_request_response',
function ( mixed $response, mixed $result, mixed $operation, mixed $request, mixed $router ): mixed { // 👈 Alternatively, the Request class.
if ( $router instanceof \WPGraphQL\Router && $router->get_request() instanceof \WPGraphQL\Request) {
$nodes_resolved = $router->get_request()->get_query_analyzer()->is_nodes_resolved() // 👈 Since this is an instance method, we dont need to worry about the global state.
if ( ! $nodes_resolved ) {
header( 'Cache-Control: no-cache, must-revalidate' );
}
// Nothing to reset 🚀
}
return $response;
},
10,
4
);(Related #3008 will fix some state issues around when the keys should correctly be set on the header (example 1), and some better instance methods/hooks on |
|
@justlevine So we tried your first suggestion and it did achieve the results we need. We had to tweak it a little... /**
* Recursively checks if any of the items in the given array are non-empty.
* Returns true if at least one item is a non-empty value or a non-empty array.
* Returns false if all items are empty or are empty arrays.
*
* @param mixed $items The item or array to check for non-empty items.
* @return bool True if at least one item is non-empty or contains a non-empty array, otherwise false.
*/
function contains_non_empty_item( mixed $items ): bool {
if ( is_array( $items ) ) {
foreach ( $items as $item ) {
if ( contains_non_empty_item( $item ) ) {
return true; // Found a non-empty item or non-empty array.
}
}
return false; // All items are empty or are empty arrays.
} else {
return ! empty( $items ); // Non-array item, return if it's non-empty.
}
}
/**
* Modifies the HTTP response of a GraphQL request.
*
* Hooks into the 'graphql_process_http_request_response' filter.
* Checks if the GraphQL response data contains any non-empty item.
* If not, it sets the 'Cache-Control' header to 'no-cache, must-revalidate'.
*
* @param GraphQL\Executor\ExecutionResult $response The initial response object.
* @return mixed The modified response object.
*/
function modify_graphql_response( GraphQL\Executor\ExecutionResult $response ): mixed {
if ( ! contains_non_empty_item( $response->data ) ) {
header( 'Cache-Control: no-cache, must-revalidate' );
}
return $response;
}
add_filter( 'graphql_process_http_request_response', 'modify_graphql_response', 10, 4 );I'm not sure if this is too rudimental or not? |
If it does what you need it to do, then it's not ;-) |
|
@MuhammedAO @justlevine @humet I propose we add a filter within the /**
* Return headers
*
* @param array<string,mixed> $headers The array of headers being returned
*
* @return array<string,mixed>
*/
public function get_headers( array $headers = [] ): array {
$keys = $this->get_graphql_keys();
if ( ! empty( $keys ) ) {
$headers['X-GraphQL-Query-ID'] = $this->query_id ?: null;
$headers['X-GraphQL-Keys'] = $keys['keys'] ?: null;
}
/**
* @param array<string,mixed> $headers The array of headers being returned
* @param \WPGraphQL\Utils\QueryAnalyzer $query_analyzer The instance of the query analyzer
*/
return apply_filters( 'graphql_query_analyzer_get_headers', $headers, $this );
}Then we would be able to support this via the following snippet: add_filter( 'graphql_query_analyzer_get_headers', function( $headers, \WPGraphQL\Utils\QueryAnalyzer $query_analyzer ) {
if ( empty( $query_analyzer->get_runtime_nodes() ) ) {
$headers['Cache-Control'] = 'no-cache, must-revalidate';
}
return $headers;
}, 10, 2 ); |
…oken and being ported to the new wpgraphql-ide repo where we'll get them all up and running properly again)
release: v1.21.0
|
@jasonbahl @justlevine @MuhammedAO I tried this out and the filter runs fine, however, the |
|
Ah! I would say that else statement should consider if that header is already set. It's returning a default but should respect if a default has already been set. So I think that is worthy of a change as well! |
@humet try a lower priority on the filter. WPGraphQL Smart Cache MaxAge.php has this filter: You can filter at a lower priority, such as i.e. add_filter( 'graphql_query_analyzer_get_headers', function( $headers, \WPGraphQL\Utils\QueryAnalyzer $query_analyzer ) {
if ( empty( $query_analyzer->get_runtime_nodes() ) ) {
$headers['Cache-Control'] = 'no-cache, must-revalidate';
}
return $headers;
}, 20, 2 ); |
|
@humet if you or @MuhammedAO want to update the PR to add the filter, we can get it merged, otherwise I can follow-up with a PR to add the filter. 🤷🏻♂️ |
…p-graphql into check-if-nodes-are-resolved
|
Analysis results are not available for those commits View more on Code Climate. |
|
@humet @MuhammedAO I pushed up the change with the filter to this PR |
|
@jasonbahl These are two different filters, so the priority wouldn't affect the other would it? I've tested it and the smart cache one is still running last. |
|
Oh ya. 🤦♂️ This will get you access to the query analyzer, but ya we might need to A) use a 2nd filter also B) "fix" smart cache to use an existing "cache-control" header if it's already been set instead. Let me test a bit more but I think we're close! |
|
@humet how about this filter: add_filter( 'graphql_response_headers_to_send', function( $headers ) {
$request = \WPGraphQL\Router::get_request();
if ( ! $request instanceof \WPGraphQL\Request ) {
return $headers;
}
$query_analyzer = $request->get_query_analyzer();
if ( empty( $query_analyzer->get_runtime_nodes() ) ) {
$headers['Cache-Control'] = 'no-cache, must-revalidate';
}
return $headers;
}, 20, 1 ); |
What does this implement/fix? Explain your changes.
Introduces
graphql_query_analyzer_get_headersfilter.Example usage:
Does this close any currently open issues?
no