Prevent setting invalid MySQL TIMESTAMPs for assessment dates #2283
Conversation
📝 WalkthroughWalkthroughThe change enhances the Changes
Sequence Diagram(s)sequenceDiagram
participant Assessment as Assessment Model
participant Validator as verify_dates_valid_for_mysql
participant DBRange as MySQL Timestamp Range
Assessment->>Validator: Trigger validation for start_at, due_at, end_at
Validator->>DBRange: Validate each date against [1970, 2038]
DBRange-->>Validator: Return valid/invalid result
Validator->>Assessment: Append error message on invalid dates
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/models/assessment.rb (2)
680-693: Good implementation, but consider guard clauses for cleaner codeThe validation logic correctly handles the MySQL TIMESTAMP range constraints. The use of
Time.at()with the Unix timestamp values is a precise way to define the boundaries.However, as noted in the pipeline failure, you could use guard clauses to make the code more concise and address the Rubocop warning.
Here's a refactored version using guard clauses:
def verify_dates_valid_for_mysql # MySQL TIMESTAMP range is 1970-01-01 00:00:01 to 2038-01-19 03:14:07 min_time = Time.at(1).utc # 1970-01-01 00:00:01 UTC max_time = Time.at(2**31 - 1).utc # 2038-01-19 03:14:07 UTC - if start_at < min_time || start_at > max_time - errors.add :start_at, "must be between 1970-01-01 and 2038-01-19" - end - if due_at < min_time || due_at > max_time - errors.add :due_at, "must be between 1970-01-01 and 2038-01-19" - end - if end_at < min_time || end_at > max_time - errors.add :end_at, "must be between 1970-01-01 and 2038-01-19" - end + errors.add :start_at, "must be between 1970-01-01 and 2038-01-19" if start_at < min_time || start_at > max_time + errors.add :due_at, "must be between 1970-01-01 and 2038-01-19" if due_at < min_time || due_at > max_time + errors.add :end_at, "must be between 1970-01-01 and 2038-01-19" if end_at < min_time || end_at > max_time endThis addresses the Rubocop warning while maintaining the same functionality.
🧰 Tools
🪛 GitHub Actions: Ruby on Rails CI
[warning] 690-690: Rubocop: Use a guard clause (return unless end_at < min_time || end_at > max_time) instead of wrapping the code inside a conditional expression.
680-693: Consider adding a helper method to reduce duplicationThe error messages and validation logic are repeated for each date attribute, which introduces some duplication. Consider extracting this into a helper method to make the code more DRY.
Here's an implementation with a helper method:
def verify_dates_valid_for_mysql # MySQL TIMESTAMP range is 1970-01-01 00:00:01 to 2038-01-19 03:14:07 min_time = Time.at(1).utc # 1970-01-01 00:00:01 UTC max_time = Time.at(2**31 - 1).utc # 2038-01-19 03:14:07 UTC - if start_at < min_time || start_at > max_time - errors.add :start_at, "must be between 1970-01-01 and 2038-01-19" - end - if due_at < min_time || due_at > max_time - errors.add :due_at, "must be between 1970-01-01 and 2038-01-19" - end - if end_at < min_time || end_at > max_time - errors.add :end_at, "must be between 1970-01-01 and 2038-01-19" - end + validate_timestamp_range(:start_at, start_at, min_time, max_time) + validate_timestamp_range(:due_at, due_at, min_time, max_time) + validate_timestamp_range(:end_at, end_at, min_time, max_time) end + + def validate_timestamp_range(attribute, value, min_time, max_time) + errors.add attribute, "must be between 1970-01-01 and 2038-01-19" if value < min_time || value > max_time + endThis makes the validation method cleaner and more maintainable.
🧰 Tools
🪛 GitHub Actions: Ruby on Rails CI
[warning] 690-690: Rubocop: Use a guard clause (return unless end_at < min_time || end_at > max_time) instead of wrapping the code inside a conditional expression.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/models/assessment.rb(2 hunks)
🧰 Additional context used
🪛 GitHub Actions: Ruby on Rails CI
app/models/assessment.rb
[warning] 690-690: Rubocop: Use a guard clause (return unless end_at < min_time || end_at > max_time) instead of wrapping the code inside a conditional expression.
🔇 Additional comments (1)
app/models/assessment.rb (1)
27-27: LGTM: Good addition of MySQL TIMESTAMP validationThe addition of this validation is a solid preventive measure that will help avoid database issues when dates exceed the MySQL TIMESTAMP limits.
KesterTan
requested review from
a team and
dwang3851
and removed request for
a team
Description
This update ensures that the "start at," "due at," and "end at," assessment dates are valid for the MySQL TIMESTAMP type.
Motivation and Context
Instructors at UB ran into a bug where they set a due date several years into the future (after 2038) and they weren't able to load the course page afterwards. The invalid date got stored in the database as
0000-00-00 00:00:00.I did some further research into why this could happen. Modern versions of MySQL have "strict mode" enabled by default, which wouldn't allow an invalid date to be inserted, but strict mode is disabled in
database.ymlwithsql_mode: NO_ENGINE_SUBSTITUTION. It could be enabled by setting that field toNO_ENGINE_SUBSTITUTION,ONLY_FULL_GROUP_BY,STRICT_TRANS_TABLES,NO_ZERO_IN_DATE,NO_ZERO_DATE,ERROR_FOR_DIVISION_BY_ZERO, but I tested that and wasn't able to load the course page. It appears we're currently depending on a lack of validation for functionality.I think it would be a good idea to move towards complying with strict mode in the long term, but it may take major refactoring, so this is a reasonable fix in the meantime.
How Has This Been Tested?
Before making this update, I created an assessment with a due date in the year 2039. I was unable to load the course page afterwards.
I had to edit the record in the database to a real value in order to view the course page again.
After applying this update, trying to set an invalid date too far into the future gives the user a warning instead of locking people out of the course:
Types of changes
Checklist:
overcommit --install && overcommit --signto use pre-commit hook for linting