add validation of yaml fields before loading them during import #2181
Conversation
…ure that bad values aren't loaded in by either autofilling when blank or defaulting to a safe value
WalkthroughWalkthroughThe changes introduce a new method Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Assessment
participant YAMLLoader
User->>YAMLLoader: Load YAML configuration
YAMLLoader->>Assessment: load_yaml()
Assessment->>Assessment: validate_yaml(yaml_map)
Assessment-->>YAMLLoader: Return validated YAML
YAMLLoader-->>User: Configuration loaded successfully
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Additional comments not posted (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (1)
app/models/assessment.rb (1)
Line range hint
610-627: Consider adding validation for date formats.The method handles date fields by parsing them and setting defaults if not provided. Consider adding validation to ensure the date formats are correct before parsing.
Implement a check to validate date formats and handle parsing errors gracefully.
| def validate_yaml(yaml_map) | ||
| unless yaml_map["general"] | ||
| raise "General section missing in yaml" | ||
| end | ||
|
|
||
| if yaml_map["general"]["name"] != name | ||
| yaml_map["general"]["name"] = name | ||
| end | ||
| # ensure display name is present, otherwise fill in with name | ||
| if yaml_map["general"]["display_name"].blank? | ||
| yaml_map["general"]["display_name"] = name | ||
| end | ||
| if yaml_map["general"]["handin_directory"].blank? | ||
| yaml_map["general"]["handin_directory"] = "handin" | ||
| end | ||
| return if yaml_map["general"]["handin_filename"].present? | ||
|
|
||
| yaml_map["general"]["handin_filename"] = "handin.c" | ||
| end |
There was a problem hiding this comment.
Add error handling for invalid YAML structures.
The method currently raises an exception if the "general" section is missing. Consider adding more robust error handling to manage invalid YAML structures gracefully.
You might want to log the error or provide a more descriptive error message to help with debugging.
Description
It is possible that after exporting an assessment, instructors may modify the yaml configuration file manually, leading to bad / blank values. Therefore, in order to ensure that if bad values / nil values are provided to assessment import, we must validate some important fields like
name,handin_directory,handin_filename,display_name, and dates. There was some validation of dates, but the checks did not include the case where the dates could be empty strings which technically can be parsed but will cause an error later down the road.For example there can exist these errors:
namefield in the yaml file is different from the folder / yaml configuration filenames, then thenamefield of the assessment will become the one in the yaml file, but then Autolab can no longer find the folder of the assessment, which causes an errorhandin_directoryfield is not provided, this may also cause an error when trying to executeassessment.rb:construct_folder(constructing a handin folder for the assessment)handin_filenamefield is not provided, this causes a 500 error when trying to view the homepagedisplay_nameis not provided, the assessment import will not be successfulThe below changes fix all of these cases.
Motivation and Context
Improves assessment import flow and overall robustness by handling invalid yaml file fields.
How Has This Been Tested?
homework04within a course, and try using the import assessment feature, see that import is successfulhomework04.ymland either delete the field or make it a blank string, see that import is successful and display name is the same as the assessment namehomework04.ymland change the name to different from the file name (for example homework05), see that import is successfulhomework04.ymland remove the handin_directory and handin_filename fields, see that import is successful and handin_directory / handin_filename are given default valueshomework04.ymland make all date fields empty string, import and see that dates are autofilledNote: currently after deleting the assessment, the yaml fields may end up inconsistent with their corresponding values in the UI.
Types of changes
Checklist:
overcommit --install && overcommit --signto use pre-commit hook for linting