Integrate VCI snapshot with SHCReader #8
Closed
Conversation
This file contains hidden or 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
Reviewer's GuideAdds optional integration of the VCI directory snapshot into SHCReader, wires it through config and runtime verification flow, documents usage in README, introduces a dedicated config error, and centralizes shared test fixtures. Sequence diagram for SHCReader.fromJWS with optional VCI directory snapshotsequenceDiagram
actor Developer
participant SHCReader
participant FHIRBundleProcessor
participant VCProcessor
participant Directory as IssuerDirectory
participant VciDirectory as VciDirectorySnapshot
participant SHC
Developer->>SHCReader: new SHCReader(config)
alt both issuerDirectory and useVciDirectory set
SHCReader-->>Developer: throw SHCReaderConfigError
else valid configuration
SHCReader-->>Developer: SHCReader instance
end
Developer->>SHCReader: fromJWS(jws)
activate SHCReader
SHCReader->>FHIRBundleProcessor: validate(originalBundle)
FHIRBundleProcessor-->>SHCReader: ok
SHCReader->>VCProcessor: validate(vc)
VCProcessor-->>SHCReader: ok
alt issuerDirectory provided
SHCReader->>Directory: getIssuerInfo()
Directory-->>SHCReader: issuerInfo
else useVciDirectory is true
SHCReader->>VciDirectory: fromVCI()
VciDirectory-->>SHCReader: vciDirectory
SHCReader->>VciDirectory: getIssuerInfo()
VciDirectory-->>SHCReader: issuerInfo
else no directory configured
SHCReader-->>SHCReader: issuerInfo = []
end
SHCReader->>SHC: new SHC(jws, originalBundle, issuerInfo)
SHC-->>SHCReader: shc
SHCReader-->>Developer: shc
deactivate SHCReader
Class diagram for updated SHCReader configuration and VCI integrationclassDiagram
class SHCReader {
- SHCReaderConfigParams config
- FHIRBundleProcessor bundleProcessor
- VCProcessor vcProcessor
+ SHCReader(config SHCReaderConfigParams)
+ fromJWS(jws string) Promise~SHC~
}
class SHCReaderConfigParams {
+ string issuer
+ string publicKey
+ boolean enableQROptimization
+ boolean strictReferences
+ boolean verifyExpiration
+ Directory issuerDirectory
+ boolean useVciDirectory
}
class Directory {
+ getIssuerInfo() Issuer[]
+ fromVCI() Directory
}
class SHC {
+ SHC(jws string, bundle FHIRBundle, issuerInfo Issuer[])
+ asBundle() FHIRBundle
+ getIssuerInfo() Issuer[]
}
class SHCError {
+ string code
+ string message
}
class SHCReaderConfigError {
+ SHCReaderConfigError(message string)
}
class Issuer {
}
class FHIRBundle {
}
class FHIRBundleProcessor {
+ validate(bundle FHIRBundle) void
}
class VCProcessor {
+ validate(vc VerifiableCredential) void
}
class VerifiableCredential {
}
SHCReader --> SHCReaderConfigParams : uses
SHCReader --> FHIRBundleProcessor : composes
SHCReader --> VCProcessor : composes
SHCReader --> Directory : optional issuerDirectory
SHCReader --> SHC : returns
SHCReaderConfigParams --> Directory : issuerDirectory
SHCReaderConfigError --|> SHCError
Directory --> Issuer : returns
SHC --> Issuer : contains
FHIRBundleProcessor --> FHIRBundle : validates
VCProcessor --> VerifiableCredential : validates
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- In the new VCI directory test,
globalThis.fetchis reassigned without atry/finally, so if an assertion or await fails the global fetch may remain mocked; consider usingtry/finallyorvi.stubGlobalto ensure it’s always restored. SHCReader.fromJWSnow callsDirectory.fromVCI()on each invocation whenuseVciDirectoryis true, which will repeatedly hit the network; consider adding an internal cache or allowing the directory snapshot to be injected so it can be reused.SAMPLE_DIRECTORY_JSONintest/helpers.tscould be typed asDirectoryJSON(or the appropriate type) to ensure the test fixture stays aligned with the expected directory schema.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the new VCI directory test, `globalThis.fetch` is reassigned without a `try/finally`, so if an assertion or await fails the global fetch may remain mocked; consider using `try/finally` or `vi.stubGlobal` to ensure it’s always restored.
- `SHCReader.fromJWS` now calls `Directory.fromVCI()` on each invocation when `useVciDirectory` is true, which will repeatedly hit the network; consider adding an internal cache or allowing the directory snapshot to be injected so it can be reused.
- `SAMPLE_DIRECTORY_JSON` in `test/helpers.ts` could be typed as `DirectoryJSON` (or the appropriate type) to ensure the test fixture stays aligned with the expected directory schema.
## Individual Comments
### Comment 1
<location> `src/shc/reader.ts:174-175` </location>
<code_context>
let issuerInfo: Issuer[] = []
if (this.config.issuerDirectory) {
issuerInfo = this.config.issuerDirectory.getIssuerInfo()
+ } else if (this.config.useVciDirectory) {
+ const vciDirectory = await Directory.fromVCI()
+ issuerInfo = vciDirectory.getIssuerInfo()
}
</code_context>
<issue_to_address>
**suggestion (performance):** Repeatedly constructing the VCI directory on every call may be unnecessarily expensive; consider caching or sharing the instance.
With `useVciDirectory` enabled, `Directory.fromVCI()` runs on every read, which may involve unnecessary network or initialization work. If the directory data is stable for the lifetime of `SHCReader`, consider caching the `Directory` instance (e.g., on the reader instance or via a shared cache) to avoid repeated calls.
Suggested implementation:
```typescript
// Step 4: Get the issuer info from a provided directory
// or from the VCI snapshot
let issuerInfo: Issuer[] = []
if (this.config.issuerDirectory) {
issuerInfo = this.config.issuerDirectory.getIssuerInfo()
} else if (this.config.useVciDirectory) {
if (!this.vciDirectory) {
this.vciDirectory = await Directory.fromVCI()
}
issuerInfo = this.vciDirectory.getIssuerInfo()
}
```
To fully implement the caching suggestion, also:
1. Add a cached directory field to the `SHCReader` class (or whichever class this method belongs to), for example:
`private vciDirectory: Directory | null = null;`
2. Ensure `Directory` is already imported in this file (it likely is, since `Directory.fromVCI()` is used). If not, add the appropriate import.
These changes will keep a single `Directory` instance per `SHCReader` (or class instance), avoiding repeated network/initialization work when `useVciDirectory` is enabled.
</issue_to_address>
### Comment 2
<location> `test/shc/shc.test.ts:128-137` </location>
<code_context>
+ it('should fetch the VCI directory and bundle issuerInfo into SHC', async () => {
</code_context>
<issue_to_address>
**suggestion (testing):** Restore `globalThis.fetch` in a `finally` block to avoid cross-test leakage if the test throws before the manual restore.
Because the restore happens only after the async calls, any thrown error will leave `globalThis.fetch` mocked for later tests. Wrapping the override in `try/finally` avoids this:
```ts
const originalFetch = globalThis.fetch
const fetchMock = vi.fn().mockImplementation(/* ... */)
;(globalThis as any).fetch = fetchMock
try {
const vciDirectory = await Directory.fromVCI()
const verifiedHealthCard = await readerWithDirectory.fromJWS(jws)
expect(verifiedHealthCard.getIssuerInfo()).toEqual(
vciDirectory.getIssuerInfo(),
)
} finally {
;(globalThis as any).fetch = originalFetch
}
```
Suggested implementation:
```typescript
it('should fetch the VCI directory and bundle issuerInfo into SHC', async () => {
const { importPKCS8, importSPKI } = await import('jose')
const privateKeyCrypto = await importPKCS8(testPrivateKeyPKCS8, 'ES256')
const publicKeyCrypto = await importSPKI(testPublicKeySPKI, 'ES256')
const configWithCryptoKeys: SHCConfig = {
issuer: 'https://example.com/issuer',
privateKey: privateKeyCrypto,
publicKey: publicKeyCrypto,
expirationTime: null,
}
const originalFetch = globalThis.fetch
const fetchMock = vi.fn().mockResolvedValue({
ok: true,
json: async () => SAMPLE_DIRECTORY_JSON,
})
;(globalThis as any).fetch = fetchMock
try {
// existing test logic that:
// - creates a Directory via Directory.fromVCI()
// - creates a readerWithDirectory
// - calls readerWithDirectory.fromJWS(jws)
// - asserts on getIssuerInfo()
} finally {
;(globalThis as any).fetch = originalFetch
}
```
You will need to:
1. Move the existing body of this test (everything after `const configWithCryptoKeys` that currently calls `Directory.fromVCI`, `readerWithDirectory.fromJWS`, and the `expect(...)` assertion) into the `try { ... }` block, replacing the placeholder comment.
2. Remove any existing manual restore of `globalThis.fetch` at the end of the test body (e.g. `;(globalThis as any).fetch = originalFetch`) so that restoration only happens in the `finally` block.
3. If the test currently defines `fetchMock` or `originalFetch` elsewhere in the body, deduplicate so that there is a single `const originalFetch = globalThis.fetch` defined before the override, and a single `const fetchMock = ...` used for the override.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
Author
|
Closing this in favor of #9 |
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.
Summary by Sourcery
Integrate optional VCI directory snapshot support into SHCReader and improve shared test fixtures and documentation.
New Features:
Enhancements:
Documentation:
Tests:
Closes #4