More complete name validation based on ADR-6 #103
Conversation
Fixes issue #100
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Warning Rate limit exceeded@lalinsky has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 17 minutes and 8 seconds before requesting another review. ⌛ 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. 📒 Files selected for processing (2)
WalkthroughAdds a centralized validation module (ADR-6) and routes name/subject/kv validations through it across Connection, JetStream, and KV code paths; removes in-module validators and related tests; updates root re-exports and makes publish/subscribe APIs call and propagate validation errors. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
✨ Finishing Touches🧪 Generate unit tests
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 |
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a great improvement. It centralizes all name validation logic into a new validation.zig file, which is then used consistently across the connection, jetstream, and jetstream_kv modules. This refactoring removes duplicated code and ensures that validation rules from ADR-6 are applied everywhere they are needed. The new validation logic is comprehensive and well-tested. I have one suggestion to further improve maintainability by reducing some duplication within the new validation.zig file itself.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/jetstream.zig (1)
832-844: Validate purge filter subject if present.Keeps behavior consistent with ADR-6 and avoids round-trips for invalid filters.
pub fn purgeStream(self: *JetStream, stream_name: []const u8, request: StreamPurgeRequest) !Result(StreamPurgeResponse) { try validation.validateStreamName(stream_name); + if (request.filter) |s| try validation.validateSubject(s);
🧹 Nitpick comments (6)
src/root.zig (1)
64-69: Re-exports look good; consider adding queue name and BC aliases.
- Expose validateQueueName for parity with internal usage.
- Optional: keep old KV aliases (validateBucketName/validateKey) as BC shims.
Patch:
// Validation functions pub const validateStreamName = @import("validation.zig").validateStreamName; pub const validateConsumerName = @import("validation.zig").validateConsumerName; pub const validateSubject = @import("validation.zig").validateSubject; pub const validateKVBucketName = @import("validation.zig").validateKVBucketName; pub const validateKVKeyName = @import("validation.zig").validateKVKeyName; +pub const validateQueueName = @import("validation.zig").validateQueueName; + +// Back-compat aliases (consider deprecating in next major) +pub const validateBucketName = @import("validation.zig").validateKVBucketName; +pub const validateKey = @import("validation.zig").validateKVKeyName;Please confirm you want the aliases; otherwise this is a breaking change for code importing the old names.
src/jetstream_kv.zig (1)
872-884: Optional: also validate bucket_name in openBucket.Saves a roundtrip and keeps API symmetric with create/delete.
Patch:
pub fn openBucket(self: *KVManager, bucket_name: []const u8) !KV { - // Verify bucket exists by getting stream info + try validation.validateKVBucketName(bucket_name); + // Verify bucket exists by getting stream infosrc/jetstream.zig (2)
848-856: Comment is misleading; validation happens here, not in getMsg().Either remove the comment or move validation to getMsg(). I’d centralize the subject-filter validation in getMsg() as well (see below).
Add to getMsg() right before deciding between direct/legacy:
// Validate optional subject filters once, for both paths if (options.last_by_subj) |s| try validation.validateSubject(s); if (options.next_by_subj) |s| try validation.validateSubject(s);
975-981: Confirm 413 mapping.Is 413 the canonical “TooManySubjects” for DIRECT.GET in your error model? If this comes from a specific JetStream status, consider documenting it or mapping via jetstream_errors for consistency.
src/validation.zig (2)
106-149: Optional: fast-fail on ‘>’ early in validateSubject.Small optimization/readability: if token is “>”, return early when it’s the last token; otherwise error immediately without carrying a flag.
38-53: Optional: DRY the length/empty checks.You repeat the 0/255 checks across validators. A tiny helper reduces noise and centralizes limits.
Example:
fn checkLen(name: []const u8) !void { if (name.len == 0 or name.len > 255) return error.InvalidName; }Then specialize error tags per validator.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/connection.zig(8 hunks)src/jetstream.zig(15 hunks)src/jetstream_kv.zig(8 hunks)src/root.zig(1 hunks)src/validation.zig(1 hunks)tests/jetstream_kv_test.zig(0 hunks)
💤 Files with no reviewable changes (1)
- tests/jetstream_kv_test.zig
🧰 Additional context used
📓 Path-based instructions (1)
**/*.zig
📄 CodeRabbit inference engine (CLAUDE.md)
Format code using
zig fmtbefore commit
Files:
src/validation.zigsrc/root.zigsrc/connection.zigsrc/jetstream_kv.zigsrc/jetstream.zig
🧠 Learnings (1)
📚 Learning: 2025-08-25T14:51:33.570Z
Learnt from: lalinsky
PR: lalinsky/nats.zig#31
File: tests/jetstream_pull_test.zig:21-23
Timestamp: 2025-08-25T14:51:33.570Z
Learning: The nats.zig project uses a "tests:beforeEach" test in tests/all_tests.zig that runs before each test to clean up all JetStream streams and consumers, ensuring test isolation. This eliminates the need for unique test resource names as each test starts with a clean JetStream state.
Applied to files:
src/jetstream_kv.zigsrc/jetstream.zig
🔇 Additional comments (12)
src/connection.zig (4)
38-38: Importing the centralized validator is the right move.Keeps this file lean and consistent with ADR-6.
535-536: Good: publish()/publishRequest() now enforce subject (and reply) validation upfront.This aligns the hot paths with ADR-6 and avoids server-side errors.
Also applies to: 553-555
675-676: Good: subscribe and subscribeSync validate subjects.Prevents bad subscriptions from ever reaching the server.
Also applies to: 695-696
708-710: Good: queue subscriptions validate both subject and queue.Matches ADR-6 intentions for name hygiene.
Also applies to: 729-731
src/jetstream_kv.zig (6)
16-16: Importing validation module here is spot on.
304-305: KV.init validates bucket names—good.
333-334: Key validation at the subject join point is the right spot.
534-535: Good: watch/watchMulti validate keys before building subjects.Also applies to: 546-547
819-819: KVManager.createBucket validates config.bucket—nice.
887-888: KVManager.deleteBucket validates bucket—nice.src/jetstream.zig (1)
50-50: Centralized ADR-6 validation usage looks solid.Importing validation.zig and applying validateStreamName/validateConsumerName across JetStream paths + publish-time subject validation is the right direction.
Also applies to: 661-661, 679-679, 697-697, 712-712, 726-726, 750-750, 803-805, 817-819, 832-832, 848-848, 951-952, 997-998, 1206-1208
src/validation.zig (1)
38-71: Validator set aligns with ADR-6 intent.The character-class helpers and per-entity validators are straightforward and readable.
Also applies to: 89-149, 150-205, 206-221
Add errdefer to free allocated subject memory if validateSubject fails before the subject gets appended to the filter_subjects array. Co-authored-by: Lukáš Lalinský <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/jetstream_kv.zig (1)
743-748: Leak fix on filter-to-subject conversion looks correct.errdefer ensures the freshly allocated subject is freed if validateSubject() errors before append. No double-free risk since the errdefer is scoped to the loop iteration.
🧹 Nitpick comments (2)
src/jetstream_kv.zig (2)
533-540: Avoid redundant key validation in watch().getKeySubject() already validates the key. You can drop the extra check here.
Apply:
- try validation.validateKVKeyName(key); - const subject = try self.getKeySubject(key); defer self.allocator.free(subject);
544-548: Pre-validating keys in watchMulti() is fine.The fail-fast loop is reasonable before allocating subjects; note getKeySubject will validate again in the next loop, which is harmless but redundant.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/jetstream_kv.zig(8 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.zig
📄 CodeRabbit inference engine (CLAUDE.md)
Format code using
zig fmtbefore commit
Files:
src/jetstream_kv.zig
🧠 Learnings (1)
📚 Learning: 2025-08-25T14:51:33.570Z
Learnt from: lalinsky
PR: lalinsky/nats.zig#31
File: tests/jetstream_pull_test.zig:21-23
Timestamp: 2025-08-25T14:51:33.570Z
Learning: The nats.zig project uses a "tests:beforeEach" test in tests/all_tests.zig that runs before each test to clean up all JetStream streams and consumers, ensuring test isolation. This eliminates the need for unique test resource names as each test starts with a clean JetStream state.
Applied to files:
src/jetstream_kv.zig
⏰ 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: test
🔇 Additional comments (5)
src/jetstream_kv.zig (5)
16-16: Good call pulling in the centralized validation module.This keeps KV validation consistent with the rest of the codebase.
303-305: Validate bucket name before allocating.Doing this up front avoids wasted allocations on invalid input.
333-335: Key validation in getKeySubject is in the right place.This guarantees every subject construction path enforces ADR-6.
818-823: CreateBucket: bucket validation added.Matches ADR-6 and catches bad configs early.
888-894: DeleteBucket: bucket validation added.Consistent with init/create paths; good.
- Add subject validation to purge stream filter - Move validation from internal helpers to top-level API functions - Export validateQueueName for API completeness - Add bucket name validation to openBucket operation
- Add validation to JetStream.publishMsg and Connection.publishMsg - Add subject validation to Connection.publishRequestMsg - Remove redundant length check in publishMsgInternal
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/jetstream.zig (2)
775-777: Validate deliver_subject and filter subjects in ConsumerConfig.Users can pass these; validate them upfront to surface errors early.
try validation.validateStreamName(stream_name); if (config.name) |n| try validation.validateConsumerName(n); if (config.durable_name) |n| try validation.validateConsumerName(n); + if (config.deliver_subject) |s| try validation.validateSubject(s); + if (config.filter_subject) |s| try validation.validateSubject(s); + if (config.filter_subjects) |ss| { + for (ss) |s| try validation.validateSubject(s); + }
1205-1207: Validate inside publishMsgInternal to cover publishMsg() callers too.Currently publish() validates, but publishMsg() can bypass it. Validate the message subject and any subject-like headers in the internal path.
fn publishMsgInternal(self: *JetStream, msg: *Message, options: PublishOptions) !Result(PubAck) { + // Validate subjects (covers both publish() and publishMsg() paths) + try validation.validateSubject(msg.subject); + if (options.expected_stream) |s| { + try validation.validateStreamName(s); + } + if (options.expected_last_subject_seq_subject) |s| { + try validation.validateSubject(s); + } // Set JetStream-specific headers based on optionsAlso applies to: 1226-1226, 1244-1246
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/jetstream.zig(15 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.zig
📄 CodeRabbit inference engine (CLAUDE.md)
Format code using
zig fmtbefore commit
Files:
src/jetstream.zig
🧠 Learnings (1)
📚 Learning: 2025-08-25T14:51:33.570Z
Learnt from: lalinsky
PR: lalinsky/nats.zig#31
File: tests/jetstream_pull_test.zig:21-23
Timestamp: 2025-08-25T14:51:33.570Z
Learning: The nats.zig project uses a "tests:beforeEach" test in tests/all_tests.zig that runs before each test to clean up all JetStream streams and consumers, ensuring test isolation. This eliminates the need for unique test resource names as each test starts with a clean JetStream state.
Applied to files:
src/jetstream.zig
🔇 Additional comments (5)
src/jetstream.zig (5)
50-50: Centralized validation import — good.Importing validation.zig keeps name/subject checks consistent across modules.
697-697: LGTM on fail-fast stream name checks.Validating stream_name before API calls in these paths is correct.
Also applies to: 712-712, 726-726, 750-750
803-804: LGTM on consumer/stream name validation.Validating both names avoids server-side errors.
Also applies to: 817-818
832-833: LGTM: purge filter subject validation.Covers the optional subject correctly.
996-996: LGTM: stream name validation on deleteMsgInternal.Good consistency with other endpoints.
- Validate last_by_subj and next_by_subj options in getMsgLegacy - Validate last_by_subj and next_by_subj options in getMsgDirect - Ensures invalid subjects are rejected early before API calls - Maintains consistent error handling with existing validateStreamName usage
- Validate all subjects in config.subjects during addStream - Validate all subjects in config.subjects during updateStream - Ensures invalid subjects are rejected early per ADR-6 - Uses fail-fast validation with try to propagate errors consistently
1 participant
Fixes issue #100