Support all annotation types in add_annotations API (#7999) by philipkukulak · Pull Request #8007 · MarkUsProject/Markus

philipkukulak · 2026-06-18T21:41:10Z

Proposed Changes

The API endpoint POST add_annotations now supports all annotation types. Co-authored with Claude. Closes #7999.

In achieving this, it was found that there was no validation of annotation type to file type for which we are seeking to annotate. This means a request to apply a PdfAnnotation to a .py file would succeed, which would break rendering in the UI. To resolve this, some pre-existing type derivation logic was lifted out of the UI AnnotationsController and is now being used in both the UI and API controllers; the API also uses it as a part of the validation chain.

As a side effect of this change, add_existing_annotation's specs were lacking coverage due to a mix in record creation between annotation types: HtmlAnnotation used create!, and all other annotations used create. This is now consolidated. There was no coverage of the failure path for this record creation, so this was added here for good measure.

Another small bug that was fixed is that prior to this change, POSTing a TextAnnotation to a non-text file could create an annotation that would never be displayed on that file. Users will now see a 422 response if they try to do this.

The API endpoint itself now either accepts an explicit annotation type, or derives it implicitly. In the former case, it expects a match between the annotation type and the file being annotated. In the latter, it simply reconciles it from the file being annotated.

Associated documentation repository pull request (if applicable)

MarkUsProject/Wiki#267

Type of Change

Type	Applies?
🚨 Breaking change (fix or feature that would cause existing functionality to change)
✨ New feature (non-breaking change that adds functionality)	x
🐛 Bug fix (non-breaking change that fixes an issue)
🎨 User interface change (change to user interface; provide screenshots)
♻️ Refactoring (internal change to codebase, without changing functionality)
🚦 Test update (change that only adds or modifies tests)
📦 Dependency update (change that updates a dependency)
🔧 Internal (change that only affects developers or continuous integration)

Checklist

Before opening your pull request:

I have performed a self-review of my changes.
- Check that all changed files included in this pull request are intentional changes.
- Check that all changes are relevant to the purpose of this pull request, as described above.
I have added tests for my changes, if applicable.
- This is required for all bug fixes and new features.
I have updated the project documentation, if applicable.
- This is required for new features.
If this is my first contribution, I have added myself to the list of contributors.

After opening your pull request:

I have updated the project Changelog (this is required for all changes).
I have verified that the pre-commit.ci checks have passed.
I have verified that the CI tests have passed.
I have reviewed the test coverage changes reported by Coveralls.
I have requested a review from a project maintainer.

Questions and Comments

Here's a summary of the smoke tests for the changes to the endpoint:

Smoke test — `add_annotations` API

Local server (/csc108), authenticated instructor API key. Seeded one file per type on a
version-used submission: hello.py, nemo.jpg, possimus.pdf, ipsa.ipynb. Each result
confirmed by reading back through GET …/groups/:id/annotations. The type field uses the
full STI class names (TextAnnotation/ImageAnnotation/PdfAnnotation/HtmlAnnotation).

#	Scenario	`type` sent	Target file	Expected	Result
1	Text annotation	`TextAnnotation`	`hello.py`	200, `TextAnnotation`	✅ 200 — `TextAnnotation`, line/col persisted
2	Image annotation	`ImageAnnotation`	`nemo.jpg`	200, `ImageAnnotation`	✅ 200 — `ImageAnnotation`, x/y persisted
3	PDF annotation	`PdfAnnotation`	`possimus.pdf`	200, `PdfAnnotation`	✅ 200 — `PdfAnnotation`, page 2
4	HTML annotation	`HtmlAnnotation`	`ipsa.ipynb`	200, `HtmlAnnotation`	✅ 200 — `HtmlAnnotation`, nodes persisted
5	Type omitted → derived from file	(none)	`possimus.pdf`	200, `PdfAnnotation`	✅ 200 — `PdfAnnotation`, page 3
6	Type ↔ file mismatch	`TextAnnotation`	`possimus.pdf`	422	✅ 422 — `Annotation type 'TextAnnotation' does not match the type of file 'possimus.pdf'`
7	Unknown / invalid type	`bogus`	`hello.py`	422	✅ 422 — `Invalid annotation type: bogus`
8	Missing required field	(derived pdf)	`possimus.pdf` (no `page`)	422	✅ 422 — `Missing required fields for PdfAnnotation: page`
9	Nonexistent file	`TextAnnotation`	`nope.txt`	422	✅ 422 — `Submission file not found: nope.txt`
10	Batch atomicity (1 valid + 1 invalid)	`TextAnnotation`, `TextAnnotation`	`hello.py`, `possimus.pdf`	422, nothing written	✅ 422 — DB count unchanged; valid item not written
11	Empty annotations array	—	—	(edge)	✅ 200 — no-op, nothing written

Net: all 5 valid writes persisted with the correct STI type on the correct file; all 4
bad-input paths failed closed with specific 422 messages and left the DB unchanged; batch
validation is atomic (rejects the whole request before any insert).

Note: type accepts the STI class names; a short code such as "text" is rejected as
Invalid annotation type: text (consistent with the add_test_results endpoint).

Note: API error description values are HTML-escaped (e.g. ' for ') by the shared
shared/http_status.json.erb renderer — pre-existing, app-wide behavior, not introduced by
this change.

coveralls · 2026-06-18T21:55:19Z

Coverage Report for CI Build 27988288740

Coverage increased (+0.03%) to 90.297%

Details

Coverage increased (+0.03%) from the base build.
Patch coverage: 150 of 150 lines across 11 files are fully covered (100%).
No coverage regressions found.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

No coverage regressions found.

Coverage Stats


Relevant Lines:	50551
Covered Lines:	46634
Line Coverage:	92.25%
Relevant Branches:	2309
Covered Branches:	1097
Branch Coverage:	47.51%
Branches in Coverage %:	Yes
Coverage Strength:	127.08 hits per line

💛 - Coveralls

david-yz-liu · 2026-06-20T01:49:32Z

+    # matching the values reported by the annotations endpoint), mapped to the fields that must
+    # be present for each. Mirrors each subclass's model validations, enforced in add_annotations
+    # because insert_all bypasses them.
+    REQUIRED_ANNOTATION_FIELDS = {


Okay, so I think defining this is good, but really this information belongs in each model. It's a bit awkward to translate from string name to class, but since we're always deriving the expected class from the submission file anyway, I suggest the following changes:

Modify SubmissionFile.annotation_type to be SubmissionFile.annotation_class and return the class itself (e.g., TextAnnotation) rather than the string ("TextAnnotation").

You can go from the class to its name with the .name method

Define the symbol lists in each of the Annotation subclasses and then access them instead of REQUIRED_ANNOTATION_FIELDS[expected_type] below

There's a use of REQUIRED_ANNOTATION_FIELDS.key? below, but I don't think we need it. It's sufficient to check against the expect type and report the "does not match the type" error, without having a separate "Invalid annotation type" error.

Great notes, thanks David. Fixing this

david-yz-liu · 2026-06-20T01:50:50Z

+      annots = annotations_params
+      submission_files = result.submission.submission_files.index_by(&:filename)
+
+      # Validation pass: insert_all bypasses model validations, so guard the input here


A separate pass isn't necessary here. This check can happen inside the existing loop below; the early return will ensure nothing is inserted into the database, as that only happens after the loop

I've moved validation into the main loop, but one of the specs in this PR (valid item creating a new category, followed by invalid item) started failing because find_or_create_by to insert annotation categories will commit as soon as it is called, thereby breaking atomicity and leaving an orphaned annotation category for that assignment.

To mitigate this, I've wrapped the entire loop in an ActiveRecord::Base.transaction that rolls back whenever any kind of error is caught.

david-yz-liu · 2026-06-20T01:56:40Z

+          end
+        end
+
+        missing = REQUIRED_ANNOTATION_FIELDS[expected_type].select { |field| annot_params[field].blank? }


Let's be stricter here.

If there are any required fields that are missing, report an error (this is what the previous params.require call would have done)

If there are additional annotation fields that are "for the wrong type" (e.g. passing line_start for a PdfAnnotation) also report an error and return

Then, extract the annotation attributes (you can use the Ruby Hash#slice method and use that to create the hash that's appended to annotations, rather than passing all attributes.

david-yz-liu · 2026-06-20T02:05:30Z


    def annotations_params
-      params.require(annotations: [
+      params.permit(annotations: [


I'd like to keep the require, which was strict with the param requirement. I assume you changed this to allow the inner hashes to not have all of the attributes.

I think you can achieve both the strict :annotations key and the permissive inner structure by using the new expect method

Oh nice! Yes, that's why it was changed. Fixing.

david-yz-liu · 2026-06-20T02:06:38Z

+    annotation_type = submission_file.annotation_type
+    @annotation = result.annotations.create!(
+      type: annotation_type,
+      **type_specific_annotation_attributes(annotation_type),


Right, so this is the part where I'd like the logic to be the same for both controller methods.

Get the list of symbols from a model method

Call Hash#slice to get the values from params (no need to define a separate method)

… in add_annotations

…no extra unexpected fields)

philipkukulak · 2026-06-22T22:29:19Z

Thanks for the detailed review, David. I hope the new commits nail down your concerns.

david-yz-liu

@philipkukulak this is great, thanks!

…ect#8007)

philipkukulak mentioned this pull request Jun 18, 2026

Update API docs to reflect changes to add_annotations endpoint MarkUsProject/Wiki#267

Merged

philipkukulak requested a review from david-yz-liu June 18, 2026 21:42

philipkukulak force-pushed the add-annotations-all-types branch 2 times, most recently from b8d4b33 to 84fe165 Compare June 18, 2026 21:50

david-yz-liu reviewed Jun 20, 2026

View reviewed changes

philipkukulak added 3 commits June 22, 2026 18:26

Support all annotation types in add_annotations API (#7999)

6dfab47

Suggested changes to annotation subclassing organization, transaction…

fb77cda

… in add_annotations

Stricter validation for add_annotations (no expected fields missing, …

d110bbc

…no extra unexpected fields)

philipkukulak force-pushed the add-annotations-all-types branch from e68da7d to d110bbc Compare June 22, 2026 22:27

philipkukulak requested a review from david-yz-liu June 22, 2026 22:29

david-yz-liu approved these changes Jun 23, 2026

View reviewed changes

david-yz-liu merged commit 4a4d996 into master Jun 23, 2026
11 checks passed

david-yz-liu deleted the add-annotations-all-types branch June 23, 2026 00:45

akarki2005 pushed a commit to akarki2005/Markus that referenced this pull request Jun 23, 2026

Support all annotation types in add_annotations API route (MarkUsProj…

79f7bde

…ect#8007)

Uh oh!

Conversation

philipkukulak commented Jun 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed Changes

Type of Change

Checklist

Questions and Comments

Smoke test — add_annotations API

Uh oh!

coveralls commented Jun 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Coverage Report for CI Build 27988288740

Coverage increased (+0.03%) to 90.297%

Details

Uncovered Changes

Coverage Regressions

Coverage Stats

💛 - Coveralls

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!

philipkukulak commented Jun 22, 2026

Uh oh!

david-yz-liu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

philipkukulak commented Jun 18, 2026 •

edited

Loading

Smoke test — `add_annotations` API

coveralls commented Jun 18, 2026 •

edited

Loading