fix: add tests for observing purged tags + fix edge cases #5249
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
BacLuc
added
test-flaky-e2e!
BacLuc
approved these changes
Jun 16, 2024
api/tests/Api/ECampApiTestCase.php
Outdated
| /** | ||
| * mocks CacheManager and keeps track of purged cache tags. | ||
| */ | ||
| protected function &getPurgedCacheTags(): array { |
There was a problem hiding this comment.
to improve this further we could use a class which implements the cacheManager, but allows to get the purged cache tags, instead of sharing a reference to an array
There was a problem hiding this comment.
Yeah, good idea. I can give it a try.
| @@ -254,6 +254,21 @@ public function testCreateResponseStructureMatchesReadResponseStructure() { | |||
| assertThat($createArray, CompatibleHalResponse::isHalCompatibleWith($getItemResponse->toArray())); | |||
| } | |||
|
|
|||
| public function testCreatePurgesCacheTags() { | |||
| $client = static::createClientWithCredentials(); | |||
| $purgedCacheTags = &$this->getPurgedCacheTags(); | |||
There was a problem hiding this comment.
If you don't know the & (dereference) operator, it is difficult to know why this array would be mutated with the $client->request method.
And that it does not work anymore when you call the method a second time
pmattmann
approved these changes
Jun 22, 2024
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I added some tests tests for observing and checking the purged cache tags during create/delete/update operations. In the end-to-end test we only implicitly check cache purging by verifying whether the next request is a cache hit or miss. Explicitly checking the list of purged cache tags gives us a more granular testing behavior.
At the moment, I only added this tests to Category and ContentNode. If we like the concept, I can add this to other endpoints as well.
For an overview of new tests, see 1st commit: 6bd7409
During writing these tests I noticed 2 edge cases, which are also fixed in this PR. Both these edge cases have no direct implication at the moment, but it's better to fix them already now.
Edge case 1: When purging collections, we also need to purge potential abstract parent classes
Example: When adding a new ColumnLayout, we already purge "/content_node/column_layouts". However, we also need to purge the generic endpoint "/content_nodes".
At the moment we don't cache the ContentNode endpoints. But better to fix this now to be prepared, once we want to cache ContentNodes.
This is fixed with the 2nd commit: 221b3a3
Edge case 2: Many-to-many relations are not properly purged
Example: When updating the property
preferredContentTypesof a category, we should also purge the tag{contentTypeId}#categoriesCategory and ContentType are connected via Many-to-Many relation.
These tags were previously missed, because changes to ManyToMany-realations are not included in the entity changeSet computed by Doctrine via
getEntityChangeSet(). Such changes need to be queried separeately via$uow->getScheduledCollectionUpdates().For the specific example of preferredContentTypes, this is irrelevant: The categories property is not serialized on ContentType, hence the
{contentTypeId}#categoriestag is not included in the ContentType response (see the list of cache tags in the e2e test) . Nevertheless, better to fix this now to be prepared for other Many-To-Many relations.This is fixed with 3rd commit:
f315baf