fix v5 validation#52
Conversation
|
Warning Review limit reached
Next review available in: 35 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. 📝 WalkthroughWalkthroughValidation now uses vocabulary-aware JSON-LD contexts, rejects empty RDF graphs, guards DID derivation, and maps nested SHACL violations to field-level errors. New V5 enterprise fixtures and regression tests cover valid graphs and invalid nested fields. ChangesV5 validation error handling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant V5DDO
participant runShaclValidation
participant jsonld.toRDF
participant SHACLValidator
participant collectShaclViolations
V5DDO->>runShaclValidation: validate vocabulary-aware DDO
runShaclValidation->>jsonld.toRDF: convert JSON-LD to RDF
jsonld.toRDF-->>runShaclValidation: RDF store or conversion failure
runShaclValidation->>SHACLValidator: validate RDF graph
SHACLValidator-->>V5DDO: SHACL results
V5DDO->>collectShaclViolations: map nested results to field errors
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/test/data/ddo.ts (1)
376-381: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider freezing the fixture to actually guarantee the "immutable" contract.
The comment states this is meant as a stable baseline "other test suites" can rely on, but
structuredClonealone doesn't prevent mutation — a future test that forgets to clone before mutating would silently corrupt this shared module-level fixture for every other test in the process.♻️ Proposed defensive freeze
-export const validEnterpriseDDOV5 = structuredClone(DDOExampleV5); +export const validEnterpriseDDOV5 = Object.freeze( + structuredClone(DDOExampleV5) +);Note:
Object.freezeis shallow, so nested mutation (e.g.validEnterpriseDDOV5.credentialSubject.metadata.name = ...) would still succeed silently; a deep-freeze helper would be needed for full protection if that's a concern.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/data/ddo.ts` around lines 376 - 381, Protect the shared validEnterpriseDDOV5 fixture from accidental mutation by applying the project’s existing deep-freeze utility, or add a narrowly scoped deep-freeze implementation if none exists. Update the fixture declaration while preserving the structuredClone baseline and ensuring nested fields such as credentialSubject.metadata are also immutable.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/test/data/ddo.ts`:
- Around line 376-381: Protect the shared validEnterpriseDDOV5 fixture from
accidental mutation by applying the project’s existing deep-freeze utility, or
add a narrowly scoped deep-freeze implementation if none exists. Update the
fixture declaration while preserving the structuredClone baseline and ensuring
nested fields such as credentialSubject.metadata are also immutable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c45762d5-79ba-47aa-90b8-a9e02ff88142
📒 Files selected for processing (3)
src/services/ddoManager.tssrc/test/data/ddo.tssrc/test/unit/validation.test.ts
1 participant
Closes #51
Fix v5 (enterprise / VerifiableCredential) DDO validation
Summary
validateDDO()did not properly validate v5 "enterprise" (VerifiableCredential)DDOs. Well-formed v5 DDOs could pass without SHACL ever producing useful output,
malformed ones surfaced opaque errors instead of naming the offending field, and
an empty/absent address field crashed validation with a raw
ethersTypeError.This PR makes v5 validation produce a real, per-field SHACL report, hardens the
validators against thrown errors, and adds regression tests. v4 / deprecated
behavior is unchanged and all existing tests still pass.
What was wrong
No per-field violations for v5. The v5 payload lives under
credentialSubject, sordf-validate-shaclreports nested failures as ageneric
"Value does not have shape …CredentialSubjectShape"on the parentpath, hiding the actual failing field under
result.detail. The old code onlyread the top-level results, so a bad nested field produced an unactionable
message.
Raw
TypeErroron bad address.makeDid()calls ethersgetAddress().When
nftAddresswas empty/absent,getAddress()threwTypeError: invalid address. The existingtry/catchonly wrapped a separategetAddresscall, not the one insidemakeDid, so the throw escaped and wasreturned as a messy
generalerror.Vacuous-pass / "Output is null or invalid" risk. If JSON-LD expansion ever
dropped the Ocean vocabulary (e.g. an unmapped
@context),toRDFyields anear-empty graph. SHACL then finds no target nodes and "conforms" vacuously,
meaning a broken DDO could validate as
true; the failure path also emitted theunhelpful
"Output is null or invalid"string.What changed
src/services/ddoManager.tsrunShaclValidation(base class)"Output is null or invalid"branch with a meaningfulgeneralerror describing the RDF conversion failure.data.size === 0guard: a near-empty/vacuous graph is now a hardvalidation failure instead of a silent pass.
collectShaclViolationshelper (base class) — recursively walksresult.detailso errors are keyed by the real failing field (e.g.name,timeout) with the concrete SHACL message. Shared by both v4 and v5.makeDidthrow-guard (V4 + V5) — the DID is only derived whennftAddressis a valid address and
chainIdis present, wrapped intry/catch. Badinputs now yield a clean field-level error instead of a thrown
TypeError.V5DDO.validate— guards a missing/invalidcredentialSubject, keeps the@vocabcontext aligned with the shape's<https://www.w3.org/ns/credentials/v2/>namespace (documented), and routesviolations through the recursive collector.
src/test/data/ddo.tsvalidEnterpriseDDOV5: an immutable deep clone of the valid v5 fixture,captured at module-eval time so other suites' in-place mutations
(
updateFields) cannot corrupt the validation baseline.src/test/unit/validation.test.tsNew regression tests:
metadata.name) → invalid with aspecific per-field error (
name), never"Output is null or invalid";nftAddress→ cleannftAddresserror, no rawinvalid addressthrow;nftAddress→ same clean field-level error.Design note
Validation targets the whole VC envelope (the shape targets
VerifiableCredentialand cascades intocredentialSubjectviash:node), sothat model was kept rather than validating
credentialSubjectin isolation.The
if (report.conforms) return [true, {}]gate was intentionally preserved:existing suites mutate the shared
DDOExampleV4/V5fixtures in place, andtightening the gate broke the valid-V4/V5 tests once their
idno longer matchedthe mutated
nftAddress.Testing
npm run test:unit— 29 passing (25 existing + 4 new).npm run lint— no new errors (pre-existing warnings only).Summary by CodeRabbit