Issues/91 change certstore caching#96
Conversation
marek-ch
requested review from
TomasKypta and
Copilot
and removed request for
TomasKypta
There was a problem hiding this comment.
Pull request overview
Updates CertStore’s caching behavior so that a successful server update replaces the locally cached certificates with exactly what the server returned (after filtering), fixing the case where deleted server certificates were previously retained in cache.
Changes:
- Change update processing to rebuild the cached certificate list from the server response (filtering expired, skipping duplicates), instead of merging into the existing cache.
- Adjust the internal cache-update helper to persist only the newly produced
CachedData. - Add unit tests covering cache replacement, filtering of expired response entries, and response de-duplication.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
library/src/main/java/com/wultra/android/sslpinning/CertStore.kt |
Reworks update-time cache construction to replace (not merge) cached certificates and updates related log messaging. |
library/src/test/java/com/wultra/android/sslpinning/CertStoreUpdateTest.kt |
Adds tests validating the new “replace cache” behavior plus filtering/deduplication expectations. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
marek-ch
force-pushed
the
issues/94-remove-option-useChallenge-2
branch
from
7879950 to
276f2bc
Compare
marek-ch
force-pushed
the
issues/91-change-certstore-caching
branch
4 times, most recently
from
ebec779 to
9c938d5
Compare
marek-ch
force-pushed
the
issues/94-remove-option-useChallenge-2
branch
from
00886ad to
c886dd9
Compare
marek-ch
force-pushed
the
issues/91-change-certstore-caching
branch
from
5b1723f to
9baef75
Compare
marek-ch
force-pushed
the
issues/91-change-certstore-caching
branch
from
9baef75 to
a108ed7
Compare
kober32
left a comment
kober32
left a comment
There was a problem hiding this comment.
Looks OK, consider my 2 remarks - nothing serious but could be improved
|
|
||
| @Synchronized | ||
| internal fun updateCachedData(update: (CachedData?) -> CachedData?) { | ||
| internal fun updateCachedData(update: () -> CachedData?) { |
There was a problem hiding this comment.
Couldn't we just pass the data instead of the closure? Without the parameter, it doesn't serve the purpose anymore
There was a problem hiding this comment.
good point, I've refactored it
| @Synchronized | ||
| internal fun updateCachedData(update: (CachedData?) -> CachedData?) { | ||
| internal fun updateCachedData(update: () -> CachedData?) { | ||
| restoreCache() |
There was a problem hiding this comment.
I think restoreCache is no longer needed since we throw away the cache on line 155 anyway, and never use it
There was a problem hiding this comment.
I have renamed cacheIsLoaded -> isFallbackCacheLoaded.. we need to restore the cache prior to setting it in case the update is null, so there is at least fallback in cache ready
|
due to merge conflicts this PR is closed and new one is created |
Closes #91