INJIMOB-3545: Add Context Caching Mechanism to VC Verifier #210
Conversation
Signed-off-by: jaswanthkumarpolisetty <jaswanthkumar.p@thoughtworks.com>
Signed-off-by: jaswanthkumarpolisetty <jaswanthkumar.p@thoughtworks.com>
WalkthroughIntroduces wallet-backed, TTL-based caching for JSON-LD document loading via a new WalletAwareDocumentLoader, propagates cache state through VerificationResult, updates verifier APIs to accept/expose cache and TTL, adapts Util to initialize the wallet-aware loader, adds tests, and refactors the Android example to use coroutines and merge cache diffs. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant MainActivity
participant CredentialsVerifier
participant WalletAwareDocumentLoader
participant WalletCache as walletCache\n(ConcurrentHashMap)
participant ConfigurableLoader as ConfigurableDocumentLoader
User->>MainActivity: Click "Verify"
MainActivity->>MainActivity: Launch coroutine (Dispatchers.IO)
MainActivity->>MainActivity: Clear UI state
MainActivity->>CredentialsVerifier: verify(credential, format, walletCache, ttlMillis)
alt Loader has valid cached entry
CredentialsVerifier->>WalletAwareDocumentLoader: loadDocument(url, options)
WalletAwareDocumentLoader->>WalletCache: get(url)
WalletCache-->>WalletAwareDocumentLoader: cached JsonDocument
WalletAwareDocumentLoader-->>CredentialsVerifier: return cached document
else Cache miss or expired
CredentialsVerifier->>WalletAwareDocumentLoader: loadDocument(url, options)
WalletAwareDocumentLoader->>ConfigurableLoader: delegate.loadDocument(url, options)
ConfigurableLoader-->>WalletAwareDocumentLoader: JsonDocument
WalletAwareDocumentLoader->>WalletCache: put(url, CacheEntry(document, expiry))
WalletAwareDocumentLoader-->>CredentialsVerifier: return fresh document
end
CredentialsVerifier-->>MainActivity: VerificationResult (includes cachediff)
MainActivity->>MainActivity: Switch to Main dispatcher
MainActivity->>WalletCache: merge(result.cachediff)
MainActivity-->>User: Update UI with verificationResult
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/CredentialsVerifier.kt (1)
62-90: Return value inconsistency.All return statements return
Util.walletCache, but the caller passed in their ownwalletCacheparameter. IfUtil.walletCachegets reassigned or is different from the input, this breaks the contract and can confuse callers.The returned cache should match what the caller provided for predictable behavior.
vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/PresentationVerifier.kt (1)
142-161: Cache isolation prevents sharing across VCs in presentation.Line 145 passes an empty
mutableMapOf<String, CacheEntry>()for each VC verification. This creates a separate cache for every VC in the presentation, which means:
- If multiple VCs reference the same JSON-LD context URLs, they will be fetched separately
- The caching benefit is lost for presentation-level verification
- Performance may degrade for presentations with many VCs sharing common contexts
If cache sharing across VCs in a presentation is desired, consider this fix:
private fun getVCVerificationResults(verifiableCredentials: JSONArray): List<VCResult> { + val presentationCache = mutableMapOf<String, CacheEntry>() return verifiableCredentials.asIterable().map { item -> val verificationResult: VerificationResult = - credentialsVerifier.verify((item as JSONObject).toString(), CredentialFormat.LDP_VC, mutableMapOf<String, CacheEntry>()) + credentialsVerifier.verify((item as JSONObject).toString(), CredentialFormat.LDP_VC, presentationCache)If the isolation is intentional (e.g., for security reasons), please add a comment explaining the design decision.
♻️ Duplicate comments (1)
vc-verifier/kotlin/vcverifier/src/test/java/io/mosip/vercred/vcverifier/utils/WalletAwareDocumentLoaderTest.kt (1)
3-3: Import confirms missing package declaration in WalletAwareDocumentLoader.kt.The bare import
import WalletAwareDocumentLoader(without a package prefix) confirms that WalletAwareDocumentLoader.kt is missing its package declaration, as flagged in the earlier review comment.This will be resolved once the package declaration is added to WalletAwareDocumentLoader.kt.
🧹 Nitpick comments (2)
vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/utils/WalletAwareDocumentLoader.kt (1)
26-31: Document the selective caching behavior.The loader only caches
JsonDocumentinstances but not otherDocumenttypes. While this may be intentional (perhaps other document types shouldn't be cached), this selective behavior should be documented to clarify the design decision.Consider adding a KDoc comment:
/** * A DocumentLoader that caches JsonDocument instances with TTL-based expiry. * * Note: Only JsonDocument types are cached. Other Document types are returned * directly from the delegate without caching. */ class WalletAwareDocumentLoader(vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/utils/Util.kt (1)
37-39: Public mutable state in singleton raises encapsulation and thread-safety concerns.
walletCacheandttlMillisare exposed publicly, allowing external modification that could lead to inconsistent behavior.- Changes to
ttlMillisafterdocumentLoaderis initialized will have no effect, which may confuse callers.- Consider making these
privateor exposing them through controlled methods (e.g.,clearCache(),setTtl()with proper synchronization).- var documentLoader: DocumentLoader? = null - var walletCache: MutableMap<String, CacheEntry> = ConcurrentHashMap() - var ttlMillis: Long = 30 * 60 * 1000 + @Volatile + private var documentLoader: DocumentLoader? = null + private val walletCache: MutableMap<String, CacheEntry> = ConcurrentHashMap() + private var ttlMillis: Long = 30 * 60 * 1000 + + fun clearCache() { + walletCache.clear() + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
vc-verifier/kotlin/example/src/main/java/io/mosip/vccred/example/MainActivity.kt(4 hunks)vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/CredentialsVerifier.kt(4 hunks)vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/PresentationVerifier.kt(2 hunks)vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/verifier/LdpVerifier.kt(2 hunks)vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/data/Data.kt(2 hunks)vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/utils/Util.kt(4 hunks)vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/utils/WalletAwareDocumentLoader.kt(1 hunks)vc-verifier/kotlin/vcverifier/src/test/java/io/mosip/vercred/vcverifier/utils/UtilsTest.kt(4 hunks)vc-verifier/kotlin/vcverifier/src/test/java/io/mosip/vercred/vcverifier/utils/WalletAwareDocumentLoaderTest.kt(1 hunks)
👮 Files not reviewed due to content moderation or server errors (1)
- vc-verifier/kotlin/example/src/main/java/io/mosip/vccred/example/MainActivity.kt
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-03T12:07:06.442Z
Learnt from: abhip2565
Repo: mosip/vc-verifier PR: 202
File: vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/types/SdJwtVerifiableCredential.kt:18-21
Timestamp: 2025-11-03T12:07:06.442Z
Learning: For SD-JWT credentials in the vc-verifier project, the checkStatus method should return null to bypass status checks rather than throwing errors or implementing actual status verification.
Applied to files:
vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/verifier/LdpVerifier.ktvc-verifier/kotlin/vcverifier/src/test/java/io/mosip/vercred/vcverifier/utils/UtilsTest.ktvc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/utils/Util.ktvc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/CredentialsVerifier.kt
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-maven-vc-verifier / maven-build
🔇 Additional comments (7)
vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/data/Data.kt (1)
70-73: LGTM!The
CacheEntrydata class is well-structured with clear semantics for TTL-based caching.vc-verifier/kotlin/vcverifier/src/test/java/io/mosip/vercred/vcverifier/utils/UtilsTest.kt (1)
234-269: LGTM!The test updates correctly instantiate
VerificationResultwith the new cache parameter usingConcurrentHashMap(), which is appropriate for thread-safe scenarios.vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/utils/WalletAwareDocumentLoader.kt (1)
15-34: Good implementation of TTL-based caching.The cache eviction logic correctly removes expired entries before delegating to the loader, and the TTL-based expiry calculation is sound.
vc-verifier/kotlin/vcverifier/src/test/java/io/mosip/vercred/vcverifier/utils/WalletAwareDocumentLoaderTest.kt (1)
25-103: Excellent test coverage for caching behavior.The three test cases comprehensively cover the key caching scenarios:
- Cache hit with valid entry
- Expired entry triggering reload and cache update
- Cache miss loading and storing new entry
The use of
assertSamefor identity checks andassertEqualsfor content validation is appropriate.vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/CredentialsVerifier.kt (1)
46-55: Unable to verify thread safety concerns due to repository access failure.The review comment raises a critical thread safety issue regarding static mutable cache (
Util.walletCacheandUtil.ttlMillis), but I was unable to verify this concern because the repository failed to clone. To complete verification, I would need to:
- Examine the declarations of
Util.walletCacheandUtil.ttlMillisto confirm they are static- Search for all usages of these variables across the codebase
- Identify concurrent usage patterns (threads, coroutines, async calls)
- Verify if synchronization or thread-local storage mechanisms are already in place
Without access to the codebase, I cannot definitively confirm whether:
- The static variables are actually declared as mutable statics
- There are existing synchronization mechanisms
- The code is used in multi-threaded contexts
- The proposed solutions are appropriate
vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/verifier/LdpVerifier.kt (1)
3-3: Cannot fully verify without codebase access, but refactoring pattern is sound.The type change from
ConfigurableDocumentLoadertoDocumentLoaderaligns with moving from a project-internal abstraction to the standardDocumentLoaderinterface from the Titanium JSON-LD library (com.apicatalog.jsonld.loader). This is a valid refactoring pattern.However, verification that
WalletAwareDocumentLoaderproperly implements theDocumentLoaderinterface and that all usages ofConfigurableDocumentLoaderhave been replaced requires direct codebase inspection, which was not possible in this review.The refactoring appears sound in principle—the public library uses
DocumentLoaderas its core abstraction—but the implementation details should be confirmed manually or through CI/test results.vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/utils/Util.kt (1)
3-3: The review comment's concern about unqualified imports is theoretically valid but cannot be fully verified without repository access.According to Kotlin's import rules (which follow JVM/Java semantics), you cannot import unqualified classes from the default package into code with a named package like
io.mosip.vercred.vcverifier.utils. An unqualified import likeimport WalletAwareDocumentLoaderwould be invalid for this use case.However, without access to the repository, I cannot confirm:
- Whether
WalletAwareDocumentLoaderactually exists in the codebase- Whether it's in the default package or a named package (which would make the import incorrect)
- Whether this code compiles successfully
- What the intended package structure should be
vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/data/Data.kt
Outdated
Show resolved
Hide resolved
vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/utils/Util.kt
Outdated
Show resolved
Hide resolved
...tlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/utils/WalletAwareDocumentLoader.kt
Outdated
Show resolved
Hide resolved
Signed-off-by: jaswanthkumarpolisetty <jaswanthkumar.p@thoughtworks.com>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
vc-verifier/kotlin/gradle/libs.versions.toml(2 hunks)vc-verifier/kotlin/vcverifier/build.gradle.kts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-maven-vc-verifier / maven-build
🔇 Additional comments (2)
vc-verifier/kotlin/vcverifier/build.gradle.kts (2)
1-46: Build configuration changes are minimal and well-integrated.The addition of Caffeine does not disrupt existing build configuration, version constraints, or task definitions. No build-related concerns identified.
68-68: Caffeine 3.1.8 is secure but not the latest release.The dependency addition is sound: Caffeine 3.1.8 has no known CVEs directly affecting the upstream library (CVE-2024-28752 references vendor bundles, not the Caffeine code itself). However, the project's latest release line is 3.2.x (3.2.3 as of now). While 3.1.8 is stable and widely adopted, consider upgrading to 3.2.x in a future maintenance cycle to reduce exposure to transitive dependency issues and access newer improvements.
The current addition to both implementation and testImplementation scopes with centralized version management via the catalog remains a best practice.
Signed-off-by: jaswanthkumarpolisetty <jaswanthkumar.p@thoughtworks.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/data/Data.kt (1)
9-10: Property namecachediffstill violates Kotlin camelCase convention.The property should be
cacheDiff(with capital 'D') per Kotlin naming conventions. The previous fix changedCachedifftocachediff, but proper camelCase would becacheDiff.- val cachediff: Map<String, CacheEntry> + val cacheDiff: Map<String, CacheEntry>
🧹 Nitpick comments (5)
vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/utils/WalletAwareDocumentLoader.kt (2)
17-36: Thread-safety: non-atomic cache operations may cause redundant fetches.The
loadDocumentmethod has check-then-act patterns that aren't atomic:
- Lines 21-24: Between checking
expiryTimeand callingremove(), another thread could update the entry.- Lines 26-33: Multiple threads with a cache miss could concurrently fetch and overwrite each other's results.
For a caching layer, this typically results in redundant network fetches rather than data corruption, which may be acceptable. However, if you want to guarantee single-flight fetching, consider using
ConcurrentHashMap.compute()or a dedicated cache library (Caffeine is already in your dependencies per the AI summary).override fun loadDocument(url: URI, options: DocumentLoaderOptions): Document { val now = System.currentTimeMillis() val urlStr = url.toString() - walletCache[urlStr]?.let { entry -> - if (entry.expiryTime > now) return entry.document - walletCache.remove(urlStr) - } - - val fetched = delegate.loadDocument(url, options) - - if (fetched is JsonDocument) { - walletCache[urlStr] = CacheEntry( - document = fetched, - expiryTime = now + ttlMillis - ) - } - - return fetched + // Attempt cache hit with atomic check + walletCache[urlStr]?.let { entry -> + if (entry.expiryTime > now) return entry.document + } + + // Fetch and cache atomically using compute + val fetched = delegate.loadDocument(url, options) + if (fetched is JsonDocument) { + (walletCache as? ConcurrentHashMap)?.compute(urlStr) { _, existing -> + if (existing != null && existing.expiryTime > System.currentTimeMillis()) { + existing // Another thread already cached a valid entry + } else { + CacheEntry(document = fetched, expiryTime = System.currentTimeMillis() + ttlMillis) + } + } + } + return fetched }
11-15: Consider acceptingDocumentLoaderinstead ofConfigurableDocumentLoaderfor the delegate.The class only uses the
loadDocumentmethod from the delegate, which is defined in theDocumentLoaderinterface. AcceptingDocumentLoaderwould improve testability and flexibility.class WalletAwareDocumentLoader( private val ttlMillis: Long, private val walletCache: MutableMap<String, CacheEntry>, - private val delegate: ConfigurableDocumentLoader + private val delegate: DocumentLoader ) : DocumentLoader {vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/data/Data.kt (1)
6-11: Breaking change:cachediffis a required parameter without a default value.Adding a non-optional parameter to
VerificationResultbreaks all existing callers. Consider providing a default value to maintain backward compatibility:data class VerificationResult( var verificationStatus: Boolean, var verificationMessage: String = "", var verificationErrorCode: String, - val cachediff: Map<String, CacheEntry> + val cacheDiff: Map<String, CacheEntry> = emptyMap() )vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/utils/Util.kt (2)
37-38: Mutable public state may cause inconsistent behavior.
walletCacheandttlMillisare captured at loader creation time. If external code reassigns these aftergetConfigurableDocumentLoader()is called, the existing loader continues using the old values, which may cause confusion.Consider making these
valor private, with explicit methods to reset the loader if reconfiguration is needed.
52-52: Function name is misleading.
getConfigurableDocumentLoader()returns aDocumentLoader(specifically aWalletAwareDocumentLoader), not aConfigurableDocumentLoader. Consider renaming togetDocumentLoader()for accuracy.- fun getConfigurableDocumentLoader (): DocumentLoader { + fun getDocumentLoader(): DocumentLoader {Note: Update all call sites if you rename this function.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/data/Data.kt(2 hunks)vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/utils/Util.kt(4 hunks)vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/utils/WalletAwareDocumentLoader.kt(1 hunks)vc-verifier/kotlin/vcverifier/src/test/java/io/mosip/vercred/vcverifier/utils/WalletAwareDocumentLoaderTest.kt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- vc-verifier/kotlin/vcverifier/src/test/java/io/mosip/vercred/vcverifier/utils/WalletAwareDocumentLoaderTest.kt
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-03T12:07:06.442Z
Learnt from: abhip2565
Repo: mosip/vc-verifier PR: 202
File: vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/types/SdJwtVerifiableCredential.kt:18-21
Timestamp: 2025-11-03T12:07:06.442Z
Learning: For SD-JWT credentials in the vc-verifier project, the checkStatus method should return null to bypass status checks rather than throwing errors or implementing actual status verification.
Applied to files:
vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/utils/Util.kt
🔇 Additional comments (2)
vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/data/Data.kt (1)
70-73: LGTM!The
CacheEntrydata class is well-designed with appropriate fields for TTL-based caching.vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/utils/Util.kt (1)
54-71: Double-checked locking implementation looks correct.The synchronized block properly guards the lazy initialization. Good security practice disabling file loading. Ensure
documentLoaderis marked@Volatile(see earlier comment) for this pattern to be fully safe.
vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/utils/Util.kt
Show resolved
Hide resolved
Signed-off-by: jaswanthkumarpolisetty <jaswanthkumar.p@thoughtworks.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
vc-verifier/kotlin/vcverifier/src/test/java/io/mosip/vercred/vcverifier/utils/WalletAwareDocumentLoaderTest.kt (3)
25-49: Add mock verification to confirm cache hit behavior.The test correctly verifies that the cached document is returned, but doesn't confirm that the delegate loader was never invoked. Adding
verify(exactly = 0) { delegate.loadDocument(any(), any()) }would strengthen the assertion that the cache is actually being used.Apply this diff to add verification:
+import io.mockk.verify + class WalletAwareDocumentLoaderTest {And update the test:
val result = loader.loadDocument(url, DocumentLoaderOptions()) assertSame(cachedDoc, result) + verify(exactly = 0) { loader.delegate.loadDocument(any(), any()) } }
51-76: Strengthen test with expiry time verification.The test verifies the fresh document is fetched and cached, but doesn't validate that the new cache entry has the correct expiry time. Verifying the expiry time ensures the TTL mechanism works correctly when updating expired entries.
Apply this diff to add expiry time verification:
assertSame(freshDoc, result) assertEquals(freshDoc, walletCache[url.toString()]!!.document) + + val updatedEntry = walletCache[url.toString()]!! + val expectedExpiry = System.currentTimeMillis() + ttl + assertTrue(updatedEntry.expiryTime >= expectedExpiry - 100) // Allow 100ms tolerance }
78-97: Add expiry time verification for newly cached entries.The test verifies the document is fetched and stored, but doesn't validate the expiry time of the newly cached entry. This verification ensures the TTL mechanism correctly calculates expiry for cache misses.
Apply this diff to add expiry time verification:
assertSame(fetchedDoc, result) assertTrue(walletCache.containsKey(url.toString())) assertEquals(fetchedDoc, walletCache[url.toString()]!!.document) + + val cachedEntry = walletCache[url.toString()]!! + val expectedExpiry = System.currentTimeMillis() + ttl + assertTrue(cachedEntry.expiryTime >= expectedExpiry - 100) // Allow 100ms tolerance }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
vc-verifier/kotlin/example/src/main/java/io/mosip/vccred/example/MainActivity.kt(4 hunks)vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/utils/WalletAwareDocumentLoader.kt(1 hunks)vc-verifier/kotlin/vcverifier/src/test/java/io/mosip/vercred/vcverifier/utils/WalletAwareDocumentLoaderTest.kt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/utils/WalletAwareDocumentLoader.kt
👮 Files not reviewed due to content moderation or server errors (1)
- vc-verifier/kotlin/example/src/main/java/io/mosip/vccred/example/MainActivity.kt
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-maven-vc-verifier / maven-build
🔇 Additional comments (1)
vc-verifier/kotlin/vcverifier/src/test/java/io/mosip/vercred/vcverifier/utils/WalletAwareDocumentLoaderTest.kt (1)
15-23: LGTM! Clean test helpers.The helper methods provide clean abstractions for test setup. The
jsonDochelper simplifies document creation, andmockDelegatecreates a properly configured mock.
Signed-off-by: jaswanthkumarpolisetty <jaswanthkumar.p@thoughtworks.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/utils/Util.kt (1)
36-40: Document or restrict mutation of public cache configuration.The
walletCacheandttlMillisare publicvarproperties that are captured by reference whenWalletAwareDocumentLoaderis created. This leads to asymmetric behavior:
- Modifying entries in
walletCache(add/remove/update) affects the loader ✓- Replacing the entire
walletCachereference after loader creation does not affect the loader ✗- Changing
ttlMillisafter loader creation does not affect the loader ✗Consider one of these approaches:
- Document the behavior explicitly in KDoc, noting that these must be set before the first call to
getConfigurableDocumentLoader()- Add a reset method (e.g.,
resetDocumentLoader()) that clearsdocumentLoaderso the next call recreates it with current values- Make them internal or private with initialization parameters if external configuration isn't needed
Example documentation approach:
+ /** + * Shared cache for JSON-LD documents. Modifications to cache entries + * are visible to the document loader, but replacing this reference + * after loader initialization has no effect. + * Set before the first call to getConfigurableDocumentLoader(). + */ var walletCache: MutableMap<String, CacheEntry> = ConcurrentHashMap() + + /** + * TTL in milliseconds for cached documents (default: 30 minutes). + * Changes after loader initialization have no effect. + * Set before the first call to getConfigurableDocumentLoader(). + */ var ttlMillis: Long = 30 * 60 * 1000Example reset method approach:
fun resetDocumentLoader() { synchronized(loaderLock) { documentLoader = null } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/utils/Util.kt(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-03T12:07:06.442Z
Learnt from: abhip2565
Repo: mosip/vc-verifier PR: 202
File: vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/credentialverifier/types/SdJwtVerifiableCredential.kt:18-21
Timestamp: 2025-11-03T12:07:06.442Z
Learning: For SD-JWT credentials in the vc-verifier project, the checkStatus method should return null to bypass status checks rather than throwing errors or implementing actual status verification.
Applied to files:
vc-verifier/kotlin/vcverifier/src/main/java/io/mosip/vercred/vcverifier/utils/Util.kt
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-maven-vc-verifier / maven-build
1 participant
INJIMOB-3545: Add Context Caching Mechanism to VC Verifier
Summary by CodeRabbit
New Features
Refactor
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.