fix: flush/1 evicts both per-record and per-type key sets#6
fix: flush/1 evicts both per-record and per-type key sets#6BobbieBarker wants to merge 1 commit into
Conversation
Previously, flush(struct) only evicted cache keys tracked under the
per-record reference set (__set:Schema:<pk>), missing entries tracked
under the per-type set (__set:Schema). This meant collection queries
survived per-record flush, and single-record lookups survived per-type
flush. Callers had to know the internal tracking split and call both
flush(struct) and flush(struct, :new_schema) to fully invalidate.
Two changes:
1. get_set_value/3 now calls associate_key_reference_with_schema_type
for single-record ({:ok, value}) results, not just lists. This
ensures per-type flush can find single-record cache entries.
2. flush/1 (without :new_schema) now evicts both the per-record AND
per-type key sets, so a single call fully invalidates all cache
entries referencing the schema.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![codecov[bot]](https://avatars.githubusercontent.com/in/254?s=60&v=4){kind=small}
There was a problem hiding this comment.
The code has been improved for flushing and associating different types of cache keys. The changes follow the guidelines of DRY (Don't Repeat Yourself) which is great but there are still some improvements and clarifications needed regarding the implementation of each key type.
| {:ok, value} -> | ||
| Adapter.put(adapter(), cache_key, value, ttl) | ||
| associate_key_reference_with_schema(cache_key, value) | ||
| associate_key_reference_with_schema_type(cache_key, value) |
There was a problem hiding this comment.
The addition of 'associate_key_reference_with_schema_type' here seems redundant. The same operation of associating a key reference with a schema type is also performed below. You might want to clarify its necessity here or consider removing this line.
| @@ -458,10 +461,12 @@ defmodule SchemaCache do | |||
| evict_reference_keys("#{schema_type}") | |||
| end | |||
|
|
|||
There was a problem hiding this comment.
The changes in the function parameters may cause bugs when existing code calls this method. Could you check whether %{}=schema has the same functionality as the previous version of this line (%schema_type{} = schema)?
| @@ -534,6 +539,19 @@ defmodule SchemaCache do | |||
| ) | |||
| end | |||
There was a problem hiding this comment.
This private function looks fine, but can we improve the performance and readability of this function by avoiding the use of pipelining?
| @@ -658,6 +654,53 @@ defmodule SchemaCacheTest do | |||
| assert :ok = SchemaCache.flush(user) | |||
| end | |||
There was a problem hiding this comment.
This test case is comprehensive, but it seems quite large and could be broken down into smaller pieces. Have you considered this?
| @@ -925,13 +968,16 @@ defmodule SchemaCacheTest do | |||
| new_user = make_user(3, "charlie") | |||
There was a problem hiding this comment.
Please add further comments in the test case, especially to describe the use of each assertion in this part. This will help another reader to understand the logic better.
| @@ -534,6 +539,19 @@ defmodule SchemaCache do | |||
| ) | |||
| end | |||
|
|
|||
There was a problem hiding this comment.
The '&Adapter.sadd' method returns some value, but we are currently ignoring this return value. It might be more appropriate to use this value, instead of just returning ':ok' in the next line, especially if it returns error status or extra information that could be helpful for diagnosis.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6 +/- ##
==========================================
+ Coverage 94.58% 95.10% +0.51%
==========================================
Files 7 7
Lines 240 245 +5
==========================================
+ Hits 227 233 +6
+ Misses 13 12 -1
🚀 New features to boost your workflow:
|
1 participant
Summary
flush/1now evicts cache keys tracked under both the per-record set (__set:Schema:<pk>) and the per-type set (__set:Schema). Previously it only walked the per-record set, leaving collection caches stale.get_set_value/3now registers singular{:ok, value}results under the per-type key set (viaassociate_key_reference_with_schema_type), making eviction symmetric —flush(struct, :new_schema)can now find and evict singular cache entries too.flush/1on a struct evicts collection caches, and singular lookups are tracked under the per-type set forflush(_, :new_schema)eviction.Downstream ticket: QRK-346
Test plan
flush/1on a struct evicts collection caches tracked under per-type key setflush(_, :new_schema)mix formatcleanmix credo --strictclean (0 issues)🤖 Generated with Claude Code