Validate and encode subjects in the bytes domain - #983
Conversation
_validate_subject accepted str|bytes and returned str, so publish() decoded a bytes subject and re-encoded it for the wire -- the zero-copy bytes form only held when validation was skipped, and the PUB encoders (bytes) disagreed with encode_sub (str) on the canonical form. Validate on bytes instead: callers convert inline, the validators check UTF-8 (via decode) and CRLF/whitespace (a bytes regex) and return the original bytes, and encode_sub takes bytes. subscribe() decodes once for the public Subscription.subject. Also widen request() to accept bytes subjects and Headers, matching publish().
|
Claude finished @caspervonb's task in 2m 43s —— View job PR Review
The design is correct and the motivation is sound: keeping subjects in the bytes domain avoids a redundant decode→re-encode round-trip on the hot path and makes Two minor issues flagged inline:
One observation on test coverage: The existing |
![synadia-claude-reviewer[bot]](https://avatars.githubusercontent.com/u/132956691?s=60&v=4){kind=small}
| subject_b = subject.encode() if isinstance(subject, str) else subject | ||
| if not self._skip_subject_validation: | ||
| _validate_subject(subject) | ||
| _validate_subject(subject_b) |
There was a problem hiding this comment.
The return value of _validate_subject is discarded here, while publish() captures it with subject = _validate_subject(subject). The contract is that _validate_subject returns the same bytes object unchanged, so this is functionally equivalent today — but if the function is ever changed to return a normalized form (e.g., NFC-normalized Unicode), request() would silently miss that normalization while publish() would not.
| _validate_subject(subject_b) | |
| subject_b = _validate_subject(subject_b) |
| return subscription | ||
|
|
||
| async def _subscribe(self, subject: str, sid: str, queue: str | None = None) -> None: | ||
| async def _subscribe(self, subject: bytes, sid: str, queue: bytes | None = None) -> None: |
There was a problem hiding this comment.
Now that subject and queue are bytes, the logger.debug calls inside this method format them with %s, which produces b'_INBOX.abc.*'-style output rather than plain _INBOX.abc.*. The existing debug log lines would benefit from .decode() on those args — both callers (request() and the reconnect path) always pass well-formed UTF-8, so a bare .decode() is safe.
|
Review comments addressed, PTAL @claude |
|
Claude finished @caspervonb's task in 46s —— View job Re-review
Both previously flagged issues are addressed in commit Issue 1 — Issue 2 —
Both are safe bare No remaining issues. LGTM. |
1 participant
_validate_subjectacceptedstr | bytesand returnedstr, sopublish()decoded abytessubject and re-encoded it for the wire — the zero-copybytesform only survived when validation was skipped, andencode_pub/encode_hpub(bytes) andencode_sub(str) disagreed on the canonical form.Validate in the bytes domain: callers convert to bytes inline, the validators check UTF-8 (via
decode) and CRLF/whitespace (a bytes regex) and return the original bytes object, andencode_subtakes bytes like the PUB encoders.subscribe()decodes once for the publicSubscription.subjectstr.request()is widened to acceptbytessubjects andHeaders, matchingpublish().Behavior is unchanged — subjects are still rejected for non-UTF-8, whitespace, and CRLF.