Skip to content

Conversation

@MuhammedAO
Copy link
Contributor

@MuhammedAO MuhammedAO commented Jan 19, 2024

What does this implement/fix? Explain your changes.

Introduces graphql_query_analyzer_get_headers filter.

Example usage:

add_filter( 'graphql_query_analyzer_get_headers', static function( array $headers, \WPGraphQL\Utils\QueryAnalyzer $query_analyzer ): array {
	if ( empty( $query_analyzer->get_runtime_nodes() ) ) {
		$headers['Cache-Control'] = 'no-cache, must-revalidate';
	}
	return $headers;
}, 20, 2 );

Does this close any currently open issues?

no

@justlevine
Copy link
Collaborator

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 resolved.

@coveralls
Copy link

coveralls commented Jan 19, 2024

Coverage Status

coverage: 84.275%. remained the same
when pulling 903d84d on MuhammedAO:check-if-nodes-are-resolved
into 28f0952 on wp-graphql:develop.

@humet
Copy link

humet commented Jan 22, 2024

@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.

@justlevine
Copy link
Collaborator

justlevine commented Jan 22, 2024

@humet thanks for the clarification.

I wonder if the following two alternatives would work?

  1. Checking for the existence of the Query Analyzer headers in the $response parameter. (Note: This doesn't require any changes to the existing API)

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
);
  1. Pass the Router or Request instance to the graphql_process_http_request_response action. This would provide access to the actual QueryAnalyzer object used for the request, so we dont need to pollute the global state.
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 QueryAnalyzer like $q->is_enabled_for_query() (example 2).

@humet
Copy link

humet commented Jan 26, 2024

@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?

@justlevine
Copy link
Collaborator

I'm not sure if this is too rudimental or not?

If it does what you need it to do, then it's not ;-)
Is there anything jumping out at you DX-wise that isn't covered by using graphql_process_http_request_response as the entrypoint ?

@justlevine justlevine added close candidate Needs confirmation before closing needs: reviewer response This needs the attention of a codeowner or maintainer labels Feb 6, 2024
@jasonbahl
Copy link
Collaborator

@MuhammedAO @justlevine @humet I propose we add a filter within the get_headers() method.

/**
* 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 );

@jasonbahl jasonbahl removed the close candidate Needs confirmation before closing label Feb 8, 2024
@humet
Copy link

humet commented Feb 9, 2024

@jasonbahl @justlevine @MuhammedAO

I tried this out and the filter runs fine, however, the Cache-Control header is being rewritten after we set it. It gets overwritten here: https://github.com/wp-graphql/wp-graphql-smart-cache/blob/7181a68356300c88bf497951daedad9bcd43e9d9/src/Document/MaxAge.php#L247

@jasonbahl
Copy link
Collaborator

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!

@jasonbahl
Copy link
Collaborator

I tried this out and the filter runs fine, however, the Cache-Control header is being rewritten after we set it

@humet try a lower priority on the filter.

WPGraphQL Smart Cache MaxAge.php has this filter:

add_filter( 'graphql_response_headers_to_send', [ $this, 'http_headers_cb' ], 10, 1 );

You can filter at a lower priority, such as 20 instead of 10 which should apply your filter after Smart Cache's.

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 );

@jasonbahl
Copy link
Collaborator

@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. 🤷🏻‍♂️

@qlty-cloud-legacy
Copy link

Analysis results are not available for those commits

View more on Code Climate.

@jasonbahl
Copy link
Collaborator

@humet @MuhammedAO I pushed up the change with the filter to this PR

@jasonbahl jasonbahl merged commit f4c4cc7 into wp-graphql:develop Feb 13, 2024
@humet
Copy link

humet commented Feb 14, 2024

@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.

@jasonbahl
Copy link
Collaborator

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!

@jasonbahl
Copy link
Collaborator

@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 );

@jasonbahl jasonbahl changed the title feat: check if nodes are resolved feat: introduce "graphql_query_analyzer_get_headers" filter Feb 23, 2024
@jasonbahl jasonbahl mentioned this pull request Feb 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs: reviewer response This needs the attention of a codeowner or maintainer

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants