ZA | 25-SDC-JULY | Luke Manyamazi | Sprint 1 | Extra Long Bloom#52
ZA | 25-SDC-JULY | Luke Manyamazi | Sprint 1 | Extra Long Bloom#52Luke-Manyamazi wants to merge 0 commit into
Conversation
illicitonion
left a comment
There was a problem hiding this comment.
This generally looks good! One extra note I have is that the number 280 is appearing in a lot of places in this code, so if you wanted to change it to 300 you'd need to change it in a lot of places - can you think how to reduce this duplication?
| if len(content) > 280: | ||
| content = content[:280] # truncate to 280 characters |
There was a problem hiding this comment.
Automatically truncating works (and you have endpoint-layer checks to make sure we shouldn't get here), but it's not clear whether at the DB layer we should truncate or reject - what do you think the trade-offs are here?
|
Good point Daniel, I have removed the hardcoded number and introduced Bloom.MAX_CONTENT_LENGTH, so the limit is defined in one place and is easier to change. On truncation vs rejection - truncating is convenient but risks silent data loss. I’ve opted to reject oversized content at the model layer, with endpoint validation providing user-friendly errors, so behaviour stays explicit and data integrity is preserved. |
illicitonion
left a comment
There was a problem hiding this comment.
Good thoughts on truncation vs rejection! I left a few more comments.
|
|
||
|
|
||
| def add_bloom(*, sender: User, content: str) -> Bloom: | ||
| # Enforce 280-character limit |
There was a problem hiding this comment.
This is yet another thing that can get out of date :D
| # Enforce 280-character limit | |
| # Enforce character limit |
|
|
||
| def add_bloom(*, sender: User, content: str) -> Bloom: | ||
| # Enforce 280-character limit | ||
| content = content.strip() |
There was a problem hiding this comment.
Why are you choosing to strip whitespace here? Is a bloom of 5000 space characters more valid than a bloom of 5000 letters?
| return validation_error | ||
|
|
||
| # 2. Data Extraction and Cleaning | ||
| content = request.json.get("content", "").strip() |
There was a problem hiding this comment.
Why are you choosing to strip whitespace here? Is a bloom of 5000 space characters more valid than a bloom of 5000 letters?
Also what's the goal of the .get("content") call over a ["content"] lookup? This implies that the field is optional and has a reasonable default, but on line 158 you already asserted it was present.
There was a problem hiding this comment.
I think the stripping question still stands here - why is a bloom that is one letter and then 5000 space characters more valid than a bloom that is 5001 non-space characters?
The user-facing limit doesn't feel like it was framed as "280 characters ignoring trailing spaces", it's framed as "280 characters". A space is a character.
I'm also still curious about the .get("content", "") vs ["content"]
| "error": "Content cannot be empty." | ||
| }), 400 | ||
|
|
||
| if len(content) > 280: |
There was a problem hiding this comment.
This is another place you'd need to update if you changed the limit.
| if len(content) > 280: | ||
| return jsonify({ | ||
| "success": False, | ||
| "error": f"Content too long. Maximum 280 characters (current: {len(content)})." |
There was a problem hiding this comment.
This is yet another place you'd need to update if you changed the limit.
|
Thank you Daniel, I have fixed the issues and now looks okay. What I did:
|
|
"Hi Daniel, thanks for the feedback! I've refactored the validation to address your points: DRY Constants: Moved the limit to Bloom.MAX_CONTENT_LENGTH in the model. All logic and error messages now reference this single constant. Whitespace: Updated send_bloom to check the raw length first (so spaces count toward the limit) before calling .strip() for database cleanliness. Attribute Access: Fixed the TypeError in the timeline by switching from dictionary keys to proper object attributes (.sent_timestamp). Clean Lookups: Switched to direct ["content"] access since verify_request_fields already guarantees the field exists. Ready for another look!" |
|
This PR now contains a lot of unrelated changes - please can you revert down to only adding the changes you need to fix this bug? I'm expecting the change to be about 5 lines of code in total. |
0e0dcfe to
921f365
Compare
Learners, PR Template
Self checklist
Changelist
Added validation in send_bloom() route to reject content exceeding 280 characters.
Updated bloom.py model to handle content truncation safely when inserting new blooms.
Added database cleanup step (SQL) to truncate existing blooms longer than 280 characters.
Improved error response messages for better feedback when validation fails.
Questions
None