Feature/dnd validation #738

barna-isaac · 2025-11-28T17:30:04Z

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

codecov · 2025-12-01T11:01:10Z

Codecov Report

❌ Patch coverage is 93.37017% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 38.10%. Comparing base (e375ca8) to head (c6b7d7d).
⚠️ Report is 8 commits behind head on main.

Files with missing lines	Patch %	Lines
...uk/ac/cam/cl/dtg/isaac/quiz/IsaacDndValidator.java	94.16%	4 Missing and 3 partials ⚠️
...cam/cl/dtg/isaac/dto/DndValidationResponseDTO.java	62.50%	3 Missing ⚠️
.../users/QuestionValidationResponseDeserializer.java	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #738      +/-   ##
==========================================
+ Coverage   36.84%   38.10%   +1.25%     
==========================================
  Files         540      543       +3     
  Lines       23642    23821     +179     
  Branches     2825     2848      +23     
==========================================
+ Hits         8711     9077     +366     
+ Misses      14056    13853     -203     
- Partials      875      891      +16

☔ 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.

This reverts commit 558eab8.

This fixes a bug where we didn't return a default explanation when an incorrect answer without an explanation was matched.

src/test/java/uk/ac/cam/cl/dtg/isaac/api/IsaacIntegrationTestWithREST.java

src/main/java/uk/ac/cam/cl/dtg/isaac/quiz/IsaacDndValidator.java

this is needed when a previous question attempt is available for a question

src/main/java/uk/ac/cam/cl/dtg/isaac/quiz/IsaacDndValidator.java

src/main/java/uk/ac/cam/cl/dtg/isaac/quiz/ValidationUtils.java

The goal is to have rules that validate the same field close to each other.

this is to bring this branch in sync with `main`

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 · 2026-01-06T11:32:03Z

src/main/java/uk/ac/cam/cl/dtg/isaac/dos/content/DndChoice.java

+
 /**
- * Choice for Dnd Questions, containing a list of DndItems.
+ * Choice for Dnd Questions, containing a list of DnDItems.


Was this intentional?

jsharkey13 · 2026-01-15T14:20:21Z

src/main/java/uk/ac/cam/cl/dtg/isaac/dos/content/DndChoice.java

 */
 @DTOMapping(DndChoiceDTO.class)
 @JsonContentType("dndChoice")
-public class DndChoice extends ItemChoice {


Is there a reason for switching this hierarchy, given that DndItem still extends Item and we did not end up switching to a Map here as I had hoped?

(The DTO should have a parallel hierarchy, and matches this right now, but needs to if anything changes, too).

jsharkey13 · 2026-01-15T14:32:58Z

src/main/java/uk/ac/cam/cl/dtg/isaac/quiz/IsaacDndValidator.java

+        return new DndValidationResponse(question.getId(), answer, isCorrect, dropZonesCorrect, feedback, new Date());
+    }
+
+    private Optional<String> validate(final Question question, final Choice answer) {


I know this is checking whether the question is valid, but I think since we use "validator" to mean "marker" exclusively across the codebase, we need another name here. Even something like getProblemsWithQuestionOrSubmission(...) is way clearer about what the purpose of this method is.

jsharkey13 · 2026-01-15T15:02:37Z

src/main/java/uk/ac/cam/cl/dtg/isaac/quiz/ValidationUtils.java

    }

+    /** Validate a set of rules. Each rule takes two arguments. `.add` some rules, then `.check` whether they held.*/
+    public static class BiRuleValidator<T, U> {


These methods are again about checking whether content is valid, which is not what we use "validator" to mean across the codebase. They do not belong in isaac.quiz, certainly.

For now, maybe making them live in a new package called isaac.util, in ContentValidatorUtils to suchlike? I think with extra context, we can keep "validator" in the name and maybe we'll refactor the validators to be called markers at some point?

jsharkey13 · 2026-01-15T15:16:22Z

src/main/java/uk/ac/cam/cl/dtg/segue/configuration/SegueGuiceConfigurationModule.java

        return injector;
    }

+    public static void setInjector(final Injector newInjector) {


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 · 2026-01-15T16:43:38Z

src/main/java/uk/ac/cam/cl/dtg/isaac/quiz/IsaacDndValidator.java

+        var feedback = (Content) matchedAnswer.map(Choice::getExplanation)
+            .or(() -> !isCorrect && answer.getItems().size() < closestCorrect.map(c -> c.getItems().size()).orElse(0)
+                ? Optional.of(new Content(FEEDBACK_ANSWER_NOT_ENOUGH)) : Optional.empty())
+            .or(() -> !isCorrect ? Optional.ofNullable(question.getDefaultFeedback()) : Optional.empty())
+            .orElse(null);


This is really hard to read. It is a complete mess of nested streams and Optionals, I think, but I can't actually tell. Maybe it's actually all Optionals?
This would be so much easier to read with some intermediate variables, and maybe is a use-case for var (though I don't think var vs boolean is much of a saving!).

jsharkey13 · 2026-01-15T17:01:59Z

src/main/java/uk/ac/cam/cl/dtg/isaac/quiz/IsaacDndValidator.java

+    @SuppressWarnings("checkstyle:MissingJavadocType")
+    public static class QuestionHelpers {
+        public static Stream<DndChoice> getChoices(final IsaacDndQuestion question) {
+            return question.getChoices().stream().map(c -> (DndChoice) c);


Is this not an unchecked cast? I know that as written, your code does some checking of the types before this is ever called, but that's not enforced by anything and it's not at all obvious it is happening.

jsharkey13 · 2026-01-15T17:06:06Z

src/main/java/uk/ac/cam/cl/dtg/isaac/quiz/IsaacDndValidator.java

+        var feedback = (Content) matchedAnswer.map(Choice::getExplanation)
+            .or(() -> !isCorrect && answer.getItems().size() < closestCorrect.map(c -> c.getItems().size()).orElse(0)
+                ? Optional.of(new Content(FEEDBACK_ANSWER_NOT_ENOUGH)) : Optional.empty())
+            .or(() -> !isCorrect ? Optional.ofNullable(question.getDefaultFeedback()) : Optional.empty())


I think we use default feedback even if the answer is correct, provided there is no other feedback already being given to the user.
I.e. if the matched correct choice has feedback, we use that (check with feedbackIsNullOrEmpty because every choice the editor creates has an empty explanation but that's not the same as having feedback), otherwise we still use default feedback.

jsharkey13 · 2026-01-15T17:13:44Z

src/main/java/uk/ac/cam/cl/dtg/isaac/quiz/IsaacDndValidator.java

+
+    private static class ChoiceHelpers {
+        public static boolean matches(final DndChoice lhs, final DndChoice rhs) {
+            return lhs.getItems().stream().allMatch(lhsItem -> dropZoneEql(rhs, lhsItem));


Won't this NullPointerException if getItems() is null? All of these helpers need null guards, no? Whereas if they were inlined we could be sure we'd already done the null check?

jsharkey13 · 2026-01-15T17:23:26Z

src/main/java/uk/ac/cam/cl/dtg/isaac/quiz/IsaacDndValidator.java

+                .collect(Collectors.toList());
+        }
+
+        private static Stream<String> getDropZonesFromContent(final ContentBase content) {


I don't like the fact that we use this method at runtime on every single question attempt. I can see why it may be worth having for the content errors page, and that you want to make the feedback given to the user be consistent with this - but it should be unnecessary almost all of the time.
I wouldn't object to doing a runtime check for DEV mode and only validating it then (much like we do elsewhere already- though this uses a lookup from properties, we sometimes cache it for faster lookup), but ETL runs in PROD so using the same method for ETL and runtime checking will be a pain.

This is one of my biggest complaints about the "small functions abound" style; it's really hard to reason about what we're doing when, because this method is so far down a convoluted call stack from the main validateQuestionResponse it is hard to tell as a human reading the code. IntelliJ is clever enough to detect it at least.

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.

barna-isaac added 2 commits November 28, 2025 17:28

WIP: add IT test for QuestionsFacade

20eeb1a

WIP: validation works for correct, incorrect answers

e6422ff

barna-isaac changed the base branch from main to hackathon/dnd-images November 28, 2025 17:30

does this trigger a build?

160016b

barna-isaac changed the base branch from hackathon/dnd-images to main November 28, 2025 17:36

barna-isaac marked this pull request as draft November 28, 2025 17:37

barna-isaac added 3 commits November 28, 2025 17:42

gentle modifications to QuestionFacadeIT

049c50f

merge main

297343d

fix failing integration tests

783fc11

barna-isaac added 16 commits December 1, 2025 11:03

test that build can be broken

558eab8

Revert "test that build can be broken"

28fc161

This reverts commit 558eab8.

add integration test for incorrect dnd answer

f398bb3

add integration test for correct dnd answer

3dd00a0

cloze validator can return explanation

dabefac

dnd validator determines correctness based on relevance

6317e33

test default feedback not returned for correct

537a4e1

return correct dropzones, for correct only

530c1f3

test validated dnd answer can have explanation

99185f3

dnd validation endpoint returns dropZonesCorrect

ef85342

indicate correct dropzones for incorrect answer

bee2dba

dropZonesCorrect for multiple correct answers

f9d4f48

ensure default explanation is returned

7e3521a

This fixes a bug where we didn't return a default explanation when an incorrect answer without an explanation was matched.

add explanations for no answer, missing items

908c3b8

add error states for unrecognized, too many items

11a4719

test invalid answers

ab39d23

github-advanced-security bot found potential problems Dec 3, 2025

View reviewed changes

src/test/java/uk/ac/cam/cl/dtg/isaac/api/IsaacIntegrationTestWithREST.java Fixed Show fixed Hide fixed

barna-isaac added 3 commits December 4, 2025 15:17

invalid answers message returned as explanation

86e7c11

validate and log question having choices

ec2b36f

test question validation for null, empty choices

be59245

github-advanced-security bot found potential problems Dec 16, 2025

View reviewed changes

src/main/java/uk/ac/cam/cl/dtg/isaac/quiz/IsaacDndValidator.java Fixed Show fixed Hide fixed

barna-isaac added 2 commits December 16, 2025 17:30

add ability to deserialise DndValidationResponse

3c48992

this is needed when a previous question attempt is available for a question

resolve github copilot warning

a3f5038

github-advanced-security bot found potential problems Dec 16, 2025

View reviewed changes

src/main/java/uk/ac/cam/cl/dtg/isaac/quiz/IsaacDndValidator.java Fixed Show fixed Hide fixed

barna-isaac added 3 commits December 17, 2025 11:17

resolve style issues

5908e66

fix 1 more style issue

1a959ec

question validator checks no dup dz in answer

029c758

github-advanced-security bot found potential problems Dec 17, 2025

View reviewed changes

src/main/java/uk/ac/cam/cl/dtg/isaac/quiz/ValidationUtils.java Fixed Show fixed Hide fixed

barna-isaac added 2 commits December 17, 2025 13:49

validate each answer in question

b25f6fe

preserve rule execution order

356de46

github-advanced-security bot found potential problems Dec 17, 2025

View reviewed changes

barna-isaac added 11 commits December 17, 2025 15:56

fix build

26dbb9f

validate both user answer and answer on question

aa6bdc4

clean up DnDValidator

bd0dc97

use consistent style for test titles

fdcaff7

create helper for asserting logging

2d8d676

reorganise question validation rule order

894ab5c

The goal is to have rules that validate the same field close to each other.

fix code style

f95f427

add remaining logging for question validator

7774f35

add override annotations suggested by gh copilot

ac517f9

fix minor mistake in comment

c912ec7

delete orika-related files

b2578b9

this is to bring this branch in sync with `main`

barna-isaac marked this pull request as ready for review December 18, 2025 17:32

barna-isaac added 4 commits December 19, 2025 09:50

remove anyCorrectMatch helper

7b58742

simplify matchStrength

a6fcb7f

simplify method on DynamicMessageRule

82ecfb4

test drop zone within latex

c6b7d7d

jsharkey13 self-requested a review January 6, 2026 11:21

jsharkey13 reviewed Jan 15, 2026

View reviewed changes

Feature/dnd validation #738

Are you sure you want to change the base?

Feature/dnd validation #738

Uh oh!

Conversation

barna-isaac commented Nov 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

The feature

The implementation

Code changes (15 files)

Test changes

Logging

Security

Performance

Possible improvements

Uh oh!

codecov bot commented Dec 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jsharkey13 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jsharkey13 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

barna-isaac commented Nov 28, 2025 •

edited

Loading

Code changes `(15 files)`

codecov bot commented Dec 1, 2025 •

edited

Loading