Problem/Motivation

The asset's cache needs to be invalidated after a log's status that assigns a group membership is changed (done/pending).

Similar to #3240950: Invalidate asset cache when new weight observations are saved and #3240951: Asset cache is not invalidated after inventory log status changes

Steps to reproduce

  1. Assign an asset to a group. View the asset in JSONAPI.
  2. Change the log with the inventory adjustment to "pending".
  3. View the asset in JSONAPI, it will show the old group until the asset's cache is rebuilt.

Proposed resolution

TBD

Remaining tasks

Test and implement.

User interface changes

none.

API changes

Correct group membership always shown in JSONAPI.

Data model changes

None.

Issue fork farm-3241078

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

m.stenta created an issue. See original summary.

m.stenta’s picture

Oh just found this note in my todos related to this:

related: asset in group's location (in Views) doesn't update when asset's group location changes

Very poorly worded, so I'm trying to remember exactly what it means... but IIRC it's related to the fact that group membership affects asset location, but assets in a group can also have a separate location from the group (if they have a movement log after the group's movement log). I think I noticed that this was not updating properly in Views. Needs testing to determine what I was talking about... :-)

paul121 made their first commit to this issue’s fork.

paul121’s picture

Title: Asset cache is not invalidated after group membership log status changes » 2.x-group-membership-cache
Status: Active » Needs review

The asset's cache needs to be invalidated after a log's status that assigns a group membership is changed (done/pending).

Solved by implementing a LogEventSubscriber like we have done in farm_location for invalidating asset cache with movement logs.

related: asset in group's location (in Views) doesn't update when asset's group location changes

Solved by invalidating group members' cache tags when the group's location changes.

This is a little bit tricky since we start to depend on (and duplicate) the farm_location LogEventSubscriber::invalidateAssetCacheOnMovement logic to determine if a movement log has been updated. To simply things a bit I created a new public static function: public static function isActiveMovementLog(LogInterface $log): bool that the farm_group log event subscriber can use.

Alternatively, *I think* we could include this isActiveMovementLog function on the log.location service itself. That would make the logic even more reusable.

Test and implement.

Rather than test this behavior by making JSONAPI requests I created a new FarmEntityCacheTestTrait test trait that provides a couple helper methods for testing invalidation of entity cache tags directly.

At first I thought I could use the entity cache bin to check for cache hits/miss in tests:

\Drupal::cache('entity')->get('values:asset:1');

But when I tried this I was *always* getting a "cache hit". I saw that these cache entries have a different cache tag (asset_values) in the DB so I think they are used for something else (lower level) than we need.

I settled on simply creating a new cache entry in the default cache bin that is dependent on the entity's cache tags. This is more similar to how the drupal render cache works anyways, each entry is dependent on other cache tag(s):

 \Drupal::cache()->set('farm_entity_cache_test:asset:1', 'data', Cache::PERMANENT, $entity->getCacheTags());

I refactored the existing farm_location tests to use this approach for testing cache tag invalidations.

paul121’s picture

This has led me to discover another more general bug re: caching... logs that are "done" with a timestamp in the future add another level of complication here. This is especially important for requests via the API. Consider the following:

  1. Create an asset.
  2. Create a movement log in the future (5 minutes) with the status set as "done".
  3. View the asset's JSONAPI resource (this will cache the asset's resource in the jsonapi_normalization cache bin)
  4. Come back in 5 minutes and load the JSONAPI resource. Unchanged.
  5. Go to the log edit page and re-save it without making changes. JSONAPI resource is updated.

Not sure how important this is. It seems like it *could be* semi-straightforward to support this... Drupal has the concept of cache tags and the cache "max-age". *In theory*, when a planned log is created, we could set the asset's cache max-age to be the timestamp of that log. That way anything cached from the asset (and respecting its max-age) would be rebuilt appropriately. Of course, I'm not sure if we're able to specify an individual asset's cache max age... gotta sign off. This would probably be best to tackle in a separate issue.

paul121’s picture

Just tacking on a couple commits to more accurately reflect what the LogEventSubscribers are doing.

m.stenta’s picture

Title: 2.x-group-membership-cache » Asset cache is not invalidated after group membership log status changes
m.stenta’s picture

logs that are "done" with a timestamp in the future add another level of complication here. This is especially important for requests via the API.

Ah yea - that is a good point. We should open a new issue for that. I suppose some cron-based logic is the only way to handle that - and even then it would be hard to get it perfect. It seems like it would be a minor edge-case issue, in either case. In practice, logs with a timestamp in the future won't typically be marked as "done".

m.stenta’s picture

Oh oops didn't read your full comment before responding... that's interesting re: setting the cache max age.

Here is a new issue for that: #3244374: Asset cache is not invalidated when a future "done" log timestamp passes

  • m.stenta committed 8233efa on 2.x
    Issue #3241078 by paul121: Asset cache is not invalidated after group...
m.stenta’s picture

Status: Needs review » Fixed

Thanks for all the work on this @paul121! Merged.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.