feat(sdk-core): add DID format validation#137
Conversation
- Add isValidDid() utility function to core/types.ts - Export isValidDid from main index for public use - Add DID validation in BlobOperationsImpl constructor - Add tests for DID validation (valid and invalid formats) This prevents cryptic errors from SDS endpoints when invalid DIDs are passed. Closes hypercerts-sdk-4ni
…3C spec - Update DID validation regex to allow digits in method names per W3C spec - Add test cases for DIDs with numeric method names (did:key2:, did:btc1:) - Update JSDoc to reflect digits allowed in DID method names - Add package-lock.json to .gitignore to prevent future commits - Add changeset for DID format validation feature
🦋 Changeset detectedLatest commit: df84d85 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThis pull request introduces a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
Pull request overview
This PR adds DID format validation to prevent cryptic errors when invalid DIDs are passed to SDS endpoints. It introduces a utility function isValidDid() that validates DIDs against the W3C DID Core specification format, exports it for consumer use, and integrates it into BlobOperationsImpl constructor to validate the repoDid parameter.
Changes:
- Added
isValidDid()utility function with support for numeric method names per W3C spec - Integrated DID validation into BlobOperationsImpl constructor with helpful error messages
- Comprehensive test coverage with 18 test cases covering valid and invalid DID formats
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/sdk-core/src/core/types.ts | Adds isValidDid() validation function with JSDoc documentation |
| packages/sdk-core/src/index.ts | Exports isValidDid for public API consumers |
| packages/sdk-core/src/repository/BlobOperationsImpl.ts | Validates repoDid in constructor, throws ValidationError for invalid format |
| packages/sdk-core/tests/core/types.test.ts | New test file with 18 comprehensive test cases for DID validation |
| packages/sdk-core/tests/repository/BlobOperationsImpl.test.ts | Adds 3 constructor validation tests |
| .changeset/did-format-validation.md | Documents the minor version change for sdk-core |
| .gitignore | Attempts to add package-lock.json (already present) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| it("should reject did:method: with empty identifier", () => { | ||
| expect(isValidDid("did:plc:")).toBe(false); |
There was a problem hiding this comment.
This test case is a duplicate of the test on line 60-62. Both test the same input "did:plc:" with the same expectation. Consider either removing this duplicate or modifying it to test a different edge case, such as "did:method:identifier:" (trailing colon after identifier).
| it("should reject did:method: with empty identifier", () => { | |
| expect(isValidDid("did:plc:")).toBe(false); | |
| it("should reject did with trailing colon after identifier", () => { | |
| expect(isValidDid("did:plc:abc123:")).toBe(false); |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.changeset/did-format-validation.md:
- Around line 1-9: The changeset should explicitly call out the
behavioral/breaking change that BlobOperationsImpl's constructor now validates
repoDid and throws ValidationError for invalid DIDs; update the .changeset text
to mention that consumers passing invalid DIDs will now receive a thrown
ValidationError from BlobOperationsImpl (and reference the new isValidDid()
utility exported from `@hypercerts-org/sdk-core` as the recommended validation
helper), and suggest migration guidance (e.g., run isValidDid(repoDid) before
constructing BlobOperationsImpl) so downstream callers can handle or sanitize
inputs to restore prior non-throwing behavior.
In `@packages/sdk-core/src/core/types.ts`:
- Around line 45-50: The isValidDid regex currently allows an empty final
segment so DIDs like "did:example:abc:" pass; update the validation in
isValidDid to require the method-specific-id be one or more idchar characters
followed by zero or more ":" + one or more idchar segments (i.e., no empty
segment or trailing colon), replacing the existing character-class-based pattern
with a segment-based pattern that enforces non-empty segments; then add a unit
test asserting isValidDid("did:example:abc:") returns false to prevent
regressions.
In `@packages/sdk-core/tests/core/types.test.ts`:
- Around line 60-66: The test suite contains a duplicate case: both the "should
reject did:method without identifier" and "should reject did:method: with empty
identifier" tests call isValidDid("did:plc:"); remove the redundant test or
change it to a distinct input (for example use "did:plc:abc:" to test a trailing
colon after a non-empty identifier) and update the test description accordingly;
locate the duplicate by the test name strings and the isValidDid invocation in
types.test.ts and ensure the new case asserts
expect(isValidDid("did:plc:abc:")).toBe(false) if you intend to validate
trailing-colon rejection.
…iour - Regex now rejects trailing colons in method-specific-id (e.g. did:x:abc:) per W3C DID Core 1.0 spec: id must end with at least one non-colon idchar - Replace duplicate test case (did:plc:) with trailing-colon case (did:example:abc:) - Changeset notes the potentially breaking constructor behaviour
1 participant
Summary
isValidDid()utility function tocore/types.tsfor validating DID format per W3C specisValidDidfrom@hypercerts-org/sdk-corefor consumer useBlobOperationsImplconstructor, throwingValidationErrorfor invalid DIDsDetails
The DID validator supports the W3C DID Core spec format
did:method:identifierwhere method allows lowercase letters and digits per W3C spec. This prevents cryptic errors from SDS endpoints when invalid DIDs are passed.Changes
src/core/types.ts: addsisValidDid()with JSDocsrc/index.ts: exportsisValidDidsrc/repository/BlobOperationsImpl.ts: validatesrepoDidin constructortests/core/types.test.ts: new test filetests/repository/BlobOperationsImpl.test.ts: adds constructor validation tests.changeset/did-format-validation.md: minor changeset for sdk-core.gitignore: addspackage-lock.jsonexclusionExtracted from #123, with conflicts resolved against PR #126 (BlobOperationsImpl refactor) and unrelated BlobInput/ProfileOperationsImpl changes omitted.
Summary by CodeRabbit
New Features
Tests