-
Notifications
You must be signed in to change notification settings - Fork 466
feat: introduce "graphql_query_analyzer_get_headers" filter #3020
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: introduce "graphql_query_analyzer_get_headers" filter #3020
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