Cap channel field length at 128 characters#148
Conversation
foxpatch-aleph
left a comment
There was a problem hiding this comment.
The PR correctly adds channel length validation to BaseMessage.channel and ChainRef.channel. However, it defines MAX_FORGET_TARGETS and MAX_FORGET_REASON_LENGTH constants that are never used. The PR description mentions hoisting 'forget/channel constants', implying they should constrain ForgetContent fields (hashes list and reason string), but this was not implemented. Either apply these constraints to ForgetContent or remove the unused constants.
aleph_message/models/__init__.py (line 24): MAX_FORGET_TARGETS and MAX_FORGET_REASON_LENGTH are defined but never used. The PR description mentions 'hoist the shared forget/channel constants' suggesting they should constrain ForgetContent. Either:
- Apply max_length=MAX_FORGET_REASON_LENGTH to ForgetContent.reason (line 178)
- Add a validator to limit ForgetContent.hashes to MAX_FORGET_TARGETS items
- Or remove these unused constants if they're not needed
aleph_message/models/__init__.py (line 173): ForgetContent should likely use the defined constants:
reasonfield should have max_length=MAX_FORGET_REASON_LENGTHhasheslist should be limited to MAX_FORGET_TARGETS items via a field_validator
aleph_message/tests/test_models.py (line 1): Consider adding a test for channel length validation, e.g., test that a channel > 128 chars raises ValidationError. This would verify the new constraint works correctly.
Bound BaseMessage.channel and ChainRef.channel to 128 characters and hoist the shared forget/channel constants above the first model that uses them.
5664b31 to
d7b5301
Compare
foxpatch-aleph
left a comment
There was a problem hiding this comment.
This PR correctly adds a 128-character limit to the channel field in both BaseMessage and ChainRef models using Pydantic's Field(max_length=...). The constants are properly hoisted to module level before first use. The validation will protect against excessively long channel names that could cause storage or DoS issues. The test plan notes no changes needed since existing tests validate the models work correctly.
aleph_message/models/__init__.py (line 24): Note: MAX_FORGET_TARGETS and MAX_FORGET_REASON_LENGTH are defined but not yet used. This is fine as a forward-looking change, but consider adding validation to ForgetContent.hashes (limit list length) and ForgetContent.reason (limit string length) in a follow-up PR, or remove these constants if they won't be used soon.
Bound BaseMessage.channel and ChainRef.channel to 128 characters and hoist the shared forget/channel constants above the first model that uses them.
Test plan
hatch -e testing run pytest(unchanged — same 4 pre-existing network-flaky tests fail on main)