Feature/dnd validation

[barna-isaac]

This adds a validator for the DnD question type. You can test that this works using the following question: http://localhost:8004/questions/c4520e54-4777-41b9-9967-2893cea96a62. Although the front-end does not yet
implement showing item-level feedback, you can see that the response also contains the necessary information for this.

The feature

Here's how it works. To create a drag-and-drop question, the content developer must specify the type as drag and drop, declare drop zones in either text or figure content, and define the drag-and-drop items. Then, they need to specify at least one correct answer, which is a pairing of drop zones and items. They may also define incorrect answers. This is only necessary if they'd like to show specific feedback to the student about why a submission is wrong.

Requirements for answers:

• a correct answer must contain items for all drop zones. The answer is only recognised as correct when all drop zones contain the correct item. If any correct answer only specifies a subset of drop zones, the question is invalid, and all answers will mark as incorrect (along with a message being shown about the question being invalid).
• An incorrect answer must contain more than 0 items, and may contain at most as many items as there are drop zones. There's no requirement to use all drop zones for invalid answers.
• No answer may contain the same drop zone more than once, and all drop zones in an answer must correspond to a drop zone that's declared in the content.
• Items may be used more than once, but the UI will only allow re-using them if the "Allow items to be used more than once" flag is also checked. All items in an answer must correspond to an item declared on the question.

Differences between DnD and Cloze questions.

• DnD questions can have drop zones on figures, whereas cloze questions cannot.
• Cloze questions allow subset matching for both correct and incorrect answers, and need explicit placeholders. DnD questions don't have a setting to allow subset matching, and must always use all drop zones for correct answers. For incorrect answers, they always use subset matching, and don't need any placeholders. When all items are set, the subset matching behaviour is identical to the full match behaviour that's used on correct answers.

The implementation

There are changes to 13 non-test and 7 test files.

Code changes (15 files)

• new and modified DO's and DTO's and mappers (8 files): DndChoice, DndChoiceDTO, DndValidationResponse, DndValidationResponseDTO. The validator for IsaacDndQuestion needed a minor adjustment, and the change to ItemChoice is stylistic. QuestionValidationResponseDeserializer needed changing so we correctly preload DnD questions with the last attempt when one is available. QuestionValidationMapper supports the new DndValidationResponse type.
• IsaacDndValidator and ValidationUtils (2 files). This is where validation is mainly implemented. I've modelled the behavior on IsaacClozeValidator. Compared to it, the most important changes are:
  • stricter validation rules. Where IsaacClozeValidator tries to fix invalid questions by, for example, ignoring any answers that are invalid (eg: the item id is missing), IsaacDndValidator refuses to validate invalid questions (it says the answer was incorrect, and explains that the question is invalid). I'm hoping that by being stricter with validation, we'll save ourselves some trouble later, by only needing to support a more limited number of question states for this type. Invalid DnD questions show up as non-critical content errors. Even when invalid, they're ETL'ed.
  • re-usable validation logic. The validator's primary functionality is to validate and mark a student answer, but before it does this, it also decides whether the question is valid in the first place. I've separated the question validation logic from the marking, so the same question validation rules can be re-used during ETL, for producing content errors.
• SegueGuiceConfigurationModule: a new setInjector method, used for mocking the injector from integration tests. It would be better if we didn't have to mock at all from integration tests, but I didn't have the strength to make this change.
• ElasticSearchIndexer: changed a few method visisbilites to public so we can load content objects during integration tests
• ContentIndexer implements the ETL process. This has been updated so invalid questions show up as content errors. This performs the same question validation as the marking endpoint.

Test changes

• a new integration test for QuestionsFacade. I made this because I wanted to explore how Jackson reacts to requests with invalid JSON, to get a better idea of what still needed to be checked manually in the validator. I needed to extend AbstractIntegrationTest so it can provide the new dependencies this required.
• some new asserts in IsaacIntegrationTestWithRest
• a new TestAppender helper, which I used when testing logging
• new tests for IsaacDndValidator, and extended tests for ContentIndexer. IsaacClozeValidator also got a new test, despite the functionality in the class not getting modified (I wanted to be sure it worked how I expected it to).

Logging

When the /answer endpoint encounters an invalid question, it logs this so we are aware there is an invalid question on the platform. However, it doesn't create logs for attempts that are generated by users. The ETL process also doesn't log invalid questions, as these are just recorded as content errors.

Security

After reading through the code of several validators, I concluded that protecting against SQL injuctions doesn't happen here. I did add stricter validation, which should also make the endpoint more secure, but didn't add any additional protections against SQL injection to the DnD validator (eg: by sanitising the submitted answer). Although the validator itself never touches the database, the validation response it produces does get recorded, and the response includes the answer, which is user-provided. If the reviewer feels I should have done this as part of this PR, I'm happy to do it.

Performance

• on my (admittedly fast) machine, marking the dnd question http://localhost:8004/questions/c4520e54-4777-41b9-9967-2893cea96a62 with a valid but incorrect answer took 0.2 milliseconds. Validating the cloze question at http://localhost:8004/questions/cloze_demo_page with a valid but invalid answer takes about 0.1 milliseconds on the same machine.

Possible improvements

• ETL only records the first encountered error. In many cases, this makes perfect sense (eg: if the question has no answers at all, we don't show extra errors about it also not having any correct answers), but there are some cases where it'd be helpful to show more than one problem at once on the Content Errors page. For example, if an answer had a problem with both a drop zone ref and an item ref, it'd be useful to show both. As things are, this only shows the first problem, and the second problem is shown only after the first one's been resolved.

Please don't be afraid to request changes, I'm sure we can find a way to get them done quickly^^.

---

Pull Request Check List

• Unit Tests & Regression Tests Added (Optional)
• Removed Unnecessary Logs/System.Outs/Comments/TODOs
• Added enough Logging to monitor expected behaviour change
• Security - Injection - everything run by an interpreter (SQL, OS...) is either validated or escaped
• Security - Data Exposure - PII is not stored or sent unencrypted
• Security - Data Exposure - Test any altered or created endpoints using swagger
• Security - Access Control - Check authorisation on every new endpoint
• Security - New dependency - configured sensibly not relying on defaults
• Security - New dependency - Searched for any know vulnerabilities
• Security - New dependency - Signed up team to mailing list
• Security - New dependency - Added to dependency list
• DB schema changes - postgres-rutherford-create-script updated
• DB schema changes - upgrade script created matching create script
• Updated Release Procedure & Documentation (& Considered Implications to Previous Versions)
• Peer-Reviewed

[codecov]

Codecov Report

❌ Patch coverage is 90.05525% with 18 lines in your changes missing coverage. Please review.
✅ Project coverage is 38.13%. Comparing base (05f88fd) to head (e9888ea).
⚠️ Report is 49 commits behind head on main.

Files with missing lines	Patch %	Lines
...uk/ac/cam/cl/dtg/isaac/quiz/IsaacDndValidator.java	95.86%	3 Missing and 2 partials ⚠️
...c/cam/cl/dtg/isaac/util/ContentValidatorUtils.java	72.22%	3 Missing and 2 partials ⚠️
...cam/cl/dtg/isaac/dto/DndValidationResponseDTO.java	62.50%	3 Missing ⚠️
...ava/uk/ac/cam/cl/dtg/segue/etl/ContentIndexer.java	83.33%	1 Missing and 2 partials ⚠️
.../users/QuestionValidationResponseDeserializer.java	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #738      +/-   ##
==========================================
+ Coverage   36.77%   38.13%   +1.36%     
==========================================
  Files         545      545              
  Lines       23692    23836     +144     
  Branches     2832     2855      +23     
==========================================
+ Hits         8713     9091     +378     
+ Misses      14102    13847     -255     
- Partials      877      898      +21     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[jsharkey13]

Some initial thoughts.

One that is not addressed everywhere inline is the use of var. We have never used var before (because the codebase started in Java 6, and was Java 8 for most of its life). I would much prefer explicit types everywhere, since they are almost always easier to read (var can't even help us with the awful Map<String, ? extends Map<String, ? extends List<? extends LightweightQuestionValidationResponse>>> type we use a lot, since you obviously cannot use it in method signatures; that's one case where hiding this mess of a type would be useful and it's no help!). There's at least one comment on var with a suggestion from a Java style guide I found useful - since I hadn't met var before this PR.

[jsharkey13]

This is dangerous, and I don't like it. If we add it, we should add it @Deprecated immediately, so that nobody thinks they can use it. It also needs either a name or clear JavaDoc indicating the dangers of using it, i.e. that it will pollute the global creation of new classes going forwards.
If this is used in a test, all subsequent tests may be affected.

[jsharkey13]

Some initial thoughts.

One that is not addressed everywhere inline is the use of var. We have never used var before (because the codebase started in Java 6, and was Java 8 for most of its life). I would much prefer explicit types everywhere, since they are almost always easier to read (var can't even help us with the awful Map<String, ? extends Map<String, ? extends List<? extends LightweightQuestionValidationResponse>>> type we use a lot, since you obviously cannot use it in method signatures; that's one case where hiding this mess of a type would be useful and it's no help!). There's at least one comment on var with a suggestion from a Java style guide I found useful - since I hadn't met var before this PR.