[DT-3871] Expose typed study/dataset template validation and create valid drafts

[kevinmarete]

Addresses

https://broadworkbench.atlassian.net/browse/DT-3871

Security risk: low — adds an authenticated endpoint that accepts a CSV and writes one draft the caller owns. Parsing was reviewed in DT-3870; this adds the request boundary around it: Admin, Chairperson, and DataSubmitter only, the same roles as study registration, with per-draft ownership unchanged in DraftServiceDAO. Uploads are bounded at 5 MiB as they are read, no uploaded content is logged, and the draft writes strip the U+0000 escape that jsonb rejects. Chairpersons gain reach to the draft endpoints, not to anyone else's drafts — two tests pin that.

Summary

Ticket 3 of the study-template workflow planned in duos-ui at docs/plans/study-template-validation-workflow.md. Adds POST /api/draft/v1/study-dataset/template-validation: one CSV in, either the errors to fix or a StudyDatasetSubmissionV1 draft holding the registration-shaped document. Typed rather than generic, so this validator does not become the implied one for whatever draft type comes next. This unblocks DT-1854, which is built against this contract and cannot be released before it.

An invalid template is a completed result; a valid one is a creation. Errors answer 200 with valid: false, which is what lets duos-ui render row and column context on the page rather than as a toast. A valid template answers 201 with a Location for the draft it created, as DraftResource.createDraftRegistration does for the same row. Only an unusable request fails: no file part, more than one, or one over 5 MiB. duos-ui needs no change for the 201 — fetchMultipart gates on res.ok, and callers discriminate on the body's valid.

The two decisions the ticket left open are settled here. truncated reaches the wire — duos-ui already renders a notice for it. Oversize is a request failure (413) while an empty, non-UTF-8, or malformed file stays a validation result, since only the latter is something the producer edits. docs/study-template-v1.md, the OpenAPI spec, and the plan in duos-ui now all describe that same boundary; docs/API_GUIDELINES.md gains 413, which its status-code list did not have.

Fifteen commits, each reviewable on its own. The first eight build the endpoint:

• 4375dbc6 the U+0000 guard. jsonb rejects the escape, and DraftDAO was the only JSON write in this area without the regexp_replace the data access request statements have carried since the 2025 migration. Both the insert and the update path get it — a draft is meant to be edited and saved. No read-side guard is needed: a stored row cannot hold the escape, because writing one would have failed.
• ceb2642d meta.draftType. DraftStudyDataset holds its type in a private static field, which Gson skips, so the draft detail response carried everything about a draft except what kind it is. It is read from the draft rather than from whatever the class serializes, so a later draft type reports itself for free.
• d6f71d31 chairpersons on the draft endpoints. The upload page admits them; without this they would hit a 403 the moment a validated template produced a draft.
• 890c4e97 the endpoint, with 990ed50f, 76eca217, 7d23061d, 9cb249f2 closing the gaps found reviewing it: the seam between validation and the draft endpoints proven against a real database, the persisted document posted to the real registration endpoint, the OpenAPI examples read back through the response model, and the endpoint timed and its path pinned.

The last seven answer review, and each is worth reading on its own:

• c76bed24 one verb set for the role scan. The two copies of the reflective role assertion had already drifted — the template resource filtered POST alone, so an endpoint added there with another verb would have slipped past the check written to be exhaustive.
• 946aa12a the U+0000 strip spares an escaped backslash. The pattern matched a literal backslash followed by u0000, which is right for a serialized U+0000 but also matches the tail of an already-escaped backslash: the strip started at the second backslash and took the following character with it, and where that left an escape the parser rejects, the ::jsonb cast failed and DraftServiceDAO reported a 400 blaming the caller's JSON for a document the server serialized itself. A negative lookbehind fixes it; without it the new test fails with Postgres's own invalid input syntax for type json.
• 66231888 one owner for the size limit. It was enforced twice with different outcomes — the resource read a byte past it and answered 413, then handed the validator a stream that could no longer exceed it, so the validator's own check could never fire. The validator owns it now and the resource streams the part straight through, which drops a full second copy of the upload and moves the read that can fail mid-upload into the one place that already reports a read failure as a validation error.
• 97b55684 one ObjectMapper. The claim that the draft is written with the mapper the registration endpoint reads was held up by nothing: this service, DatasetResource, and the contract test each built their own. They agreed only because all were unconfigured. One binding supplies both sides, and the contract test uses the single instance.
• 9b54566d, 34a73346 the response model and its spec test. The spec-drift test could not fail on a missing pointer, which is the case it exists to catch; it reads the pointer as required now. @JsonInclude was dead — Gson writes every JSON entity here — and a null error list NPE'd where an empty one is meant.
• 40d062eb 201 at the created draft, per docs/API_GUIDELINES.md and the sibling endpoint that writes the same row.

Two things review raised that are deliberately not fixed here. DataAccessRequestDAO carries the same U+0000 pattern in four statements, where it has shipped since the 2025 migration; fixing it is its own change against its own tests. And DraftServiceDAO.insertDraft wraps every write failure in BadRequestException, so a server-side failure reads as a 400 — pre-existing behaviour on a shared path, now described in the spec's 400 text rather than changed underneath its other callers.

Still worth a reviewer's eye:

• The uploaded filename is not validated. Every other multipart endpoint here calls validateFileDetails, which protects a stored name; this endpoint reads the bytes and drops the part, so there is no stored name for a traversal check to protect. It rejects any name outside ASCII — études.csv fails — which a producer could neither predict nor understand. Its size cap was vacuous too, since a multipart part usually reports its size as -1; the bounded read is the real limit.
• 413, not 400, for an oversized upload, and raised as a typed exception the resource maps itself rather than through a global createExceptionResponse entry that would change behaviour for unrelated code.
• An empty file answers 200 valid: false, not 400. The acceptance criterion grouped it with the request failures; it is file content, and "Template file is empty" belongs on the page with the other content errors. The AC has been updated to match.
• Responses are serialized by Gson, not Jackson — JerseyGsonProvider writes every JSON entity. The wire shape is therefore pinned by a test against the bytes rather than by annotations, because an absent row is how a client tells an unlocated error from one on row 1.

Testing

Tests run: 267 green across the twelve suites this touches, including two database-backed classes against a real Postgres container. JaCoCo reports 100% line and branch coverage on every new class. spotless:check clean and the OpenAPI generator validation passes.

The tests that would catch a regression rather than restate the code:

• The U+0000 tests fail without the guard with Postgres's own ERROR: unsupported Unicode escape sequence, and the new escaped-backslash test fails against the previous pattern with ERROR: invalid input syntax for type json — the 400 the review predicted.
• StudyDatasetTemplateServiceDAOTest runs the canonical fixture through validation and DraftService against a real database, then asserts what duos-ui receives: meta.draftType, a document that deserializes into StudyRegistrationRequest with no violations, a name taken from the study, and a second user refused.
• StudyDatasetTemplateRegistrationContractTest posts the persisted document to a real DatasetResource.createDatasetRegistration and asserts 201, both sides sharing the one mapper the injector supplies. Replacing the document with {} turns it into a 400.
• TemplateValidationResponseExamplesTest round-trips the 200 and 201 examples through the response model; renaming draftType in an example, or moving one to a status it no longer answers, fails it.
• The size limit is pinned on both sides of the boundary: a file of exactly 5 MiB is read and validated, one byte more is refused, so flipping the comparison or dropping the extra byte from the read cannot pass.
• The role and path assertions cover every endpoint on both resources through one shared helper, so an endpoint added later cannot be left unguarded whatever verb it carries, and the URL duos-ui calls literally cannot be renamed silently.

Not covered, deliberately: @RolesAllowed is asserted reflectively, as everywhere else in this repo — proving Jersey enforces it needs a ResourceExtension harness with an auth filter, which is worth doing once across all resources rather than for one endpoint.

---

Have you read CONTRIBUTING.md lately? If not, do that first.

• Label PR with a Jira ticket number and include a link to the ticket
• Label PR with a security risk modifier [no, low, medium, high]
• PR describes scope of changes
• Get a minimum of one thumbs worth of review, preferably two if enough team members are available
• Get PO sign-off for all non-trivial UI or workflow changes
• Verify all tests go green
• Test this change deployed correctly and works on dev environment after deployment

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[fboulnois]

👍

[otchet-broad]

Notes from Claude:

1. U+0000 strip corrupts escaped backslashes in JSON

src/main/java/org/broadinstitute/consent/http/db/DraftDAO.java:63 (and :81)

The regexp_replace U+0000 strip runs on raw JSON text and matches inside an escaped
backslash, silently corrupting legitimate values or producing invalid JSON.

In the Java text block, '\\\\u0000' resolves to the SQL literal '\\u0000', which Postgres
reads as literal backslash + u0000. That is the right pattern for a genuine U+0000
character (serialized with a single backslash), but not for a value that already contains the
six-character escape text (backslash, u, 0000) — Jackson/Gson serialize that with a
doubled backslash, and the pattern then matches starting at the second backslash:

• {"studyName":"a\\u0000b"} becomes {"studyName":"a\b"} — the value silently turns into
  a + backspace, and the trailing b is eaten.
• If the following character is not a legal JSON escape (e.g. \\u0000x), the result
  {"studyName":"a\x"} fails the ::jsonb cast. DraftServiceDAO.insertDraft catches it and
  throws BadRequestException, so the caller gets 400 "Drafts require valid json to be
  submitted" for a template the server itself serialized.

updateDraftByDraftUUID (line 81) has the identical defect.

Fix: strip in Java after parsing, or match only an odd number of preceding backslashes —
not in the SQL string. Note the pattern is copied verbatim from DataAccessRequestDAO
(lines 369 / 428 / 454 / 481), so the same latent bug already ships there and a proper fix
touches both call sites.

---

2. Server-side draft-write failure surfaces as a 400

src/main/java/org/broadinstitute/consent/http/resources/StudyDatasetTemplateResource.java:63

A server-side draft-write failure is returned to the client as 400, contradicting both the
OpenAPI 400 description and the resource's own javadoc that only an unusable request fails.

DraftServiceDAO.insertDraft wraps every write failure in
BadRequestException("Error submitting draft. Drafts require valid json to be submitted.")
before a SQLException can escape, so createExceptionResponse maps it to 400. Combined with
finding 1, a valid template yields HTTP 400 blaming the caller's JSON — while
draftStudyDatasetTemplateValidation.yaml documents 400 as "No file part was supplied, or
more than one was", giving the client no way to tell the two apart.

The spec also documents no 500 at all, even though StudyDatasetTemplateResourceTest asserts
one, so the stubbed-SQLException test exercises a path production mostly cannot reach.

---

3. Spec-drift test passes vacuously on a missing pointer

src/test/java/org/broadinstitute/consent/http/models/dto/registration/template/TemplateValidationResponseExamplesTest.java:34

The spec-drift test passes vacuously when the JSON pointer misses — exactly the case it exists
to catch.

JsonNode.at() returns MissingNode, not null, so assertNotNull(documented) can never
fire. From there: MissingNode.toString() is ""; GsonUtil.fromJson("", …) returns null;
toJson(null) returns "null"; and JsonParser.parseString("") and parseString("null")
both return JsonNull.INSTANCE — so the final assertEquals compares JsonNull to JsonNull
and passes.

Rename the spec file, move the endpoint under a different response code, or rename the
valid/invalid example keys, and this test still goes green while documenting an endpoint
that no longer exists.

Fix: assert documented.isMissingNode() is false, or use readTree(...).requiredAt(...).

---

4. IOException subclasses miss the exact-class DISPATCH and return 500

src/main/java/org/broadinstitute/consent/http/resources/StudyDatasetTemplateResource.java:79

An I/O failure while reading the uploaded part returns 500, not the 400 the code comments and
spec promise, because the exception DISPATCH is keyed on exact class.

Resource.createExceptionResponse does DISPATCH.get(e.getClass()) — an exact-class lookup
with no superclass walk. IOException.class is registered (400), but content.readNBytes() on
a truncated or aborted upload throws a subclass (EOFException, SocketTimeoutException, or
a Jersey MappableException), which misses every key and falls through to the generic 500
branch. A client that drops the connection mid-upload gets a 500 and a Sentry event instead of
the documented 4xx.

---

5. Creation branch returns 200 instead of 201

src/main/java/org/broadinstitute/consent/http/resources/StudyDatasetTemplateResource.java:61

docs/API_GUIDELINES.md states "201 Created: successful creation." The valid branch
persists a new draft row and hands back its UUID, but responds Response.ok() (200).
draftStudyDatasetTemplateValidation.yaml documents only 200, so the contract bakes the
deviation in. The guidelines file is itself edited in this PR (413 added), so the rule was in
view.

---

6. The 5 MiB limit is enforced twice, with divergent outcomes

src/main/java/org/broadinstitute/consent/http/service/studytemplate/StudyTemplateValidationService.java:118

The resource reads at most MAX_TEMPLATE_BYTES + 1 bytes and returns 413 when over, then hands
the service a ByteArrayInputStream of at most MAX_TEMPLATE_BYTES — so readTemplate's own
bytes.length > MAX_TEMPLATE_BYTES guard and its TOO_LARGE_MESSAGE error can never fire
through the endpoint. The same condition means 413-with-Error from one layer and
200-with-validation-error from the other; only the message string is shared, not the behavior.

Fix: one owner for the limit — either the service returns a distinguishable too-large
result, or the resource stops duplicating the check.

---

7. Upload is buffered twice, doubling peak memory

src/main/java/org/broadinstitute/consent/http/resources/StudyDatasetTemplateResource.java:79

readTemplate calls readNBytes(MAX + 1) into a byte[] (up to ~5 MB), wraps it in a
ByteArrayInputStream, and StudyTemplateValidationService.readTemplate immediately calls
readNBytes(MAX + 1) again, allocating a second full copy, before decoding to a UTF-16 String
(up to ~10 MB) — on top of whatever Jersey already buffered for the part. Peak is ~20 MB per
in-flight request where ~15 MB would do.

Fix: pass the part's InputStream straight through, letting the service own the limit
(per finding 6). That removes one full copy.

---

8. Jackson @JsonInclude on a type only ever serialized with Gson

src/main/java/org/broadinstitute/consent/http/models/dto/registration/template/TemplateValidationResponse.java:11

@JsonInclude(NON_NULL) is dead code here that looks load-bearing. ConsentApplication:150
registers JerseyGsonProvider for all JSON entities, and the type's own tests assert against
GsonUtil. The "absent field means no value" contract described in the class javadoc actually
rests on Gson's default null-skipping, not on this annotation. A future change to
buildGsonNullSerializer() for this path would start emitting "draft": null while the
annotation still sits there implying it cannot.

Fix: drop the annotation and the Jackson import, or state the real mechanism in the javadoc.

---

9. Third independent new ObjectMapper() leaves the round-trip guarantee unenforced

src/main/java/org/broadinstitute/consent/http/service/studytemplate/StudyDatasetTemplateService.java:24

The comment's guarantee that the draft is serialized with the mapper the registration endpoint
reads is not enforced by construction. DatasetResource:78 has its own
private final ObjectMapper objectMapper = new ObjectMapper(), and the new service creates
another (as do two of the new tests). They agree today only because both are bare and
StudyRegistrationRequest carries its own @JsonInclude(NON_NULL) / @JsonIgnoreProperties.
Configure a module or a FAIL_ON_* feature on one and the draft written here silently stops
round-tripping — the exact failure StudyDatasetTemplateRegistrationContractTest exists to
prevent, but which it cannot catch since it builds both mappers itself.

Fix: inject one shared, configured mapper.

---

10. Duplicated role assertion with a drifting HTTP-method filter

src/test/java/org/broadinstitute/consent/http/resources/StudyDatasetTemplateResourceTest.java:156

The reflection-based role assertion is copy-pasted between two resource tests.
DraftResourceTest:127 filters over Set.of(GET, POST, PUT, PATCH, DELETE) while this copy
filters only POST, so an endpoint added to StudyDatasetTemplateResource with any other verb
slips through the very check the test was written to make exhaustive.

Fix: extract one shared helper (package-private in the resources test package) taking the
resource class and the expected role set.

---

11. No test at exactly MAX_TEMPLATE_BYTES

src/test/java/org/broadinstitute/consent/http/resources/StudyDatasetTemplateResourceTest.java:109

The boundary the read-one-byte-extra trick exists to get right is untested — only
MAX_TEMPLATE_BYTES + 1 is asserted (413). Flip the resource's > to >=, or change
readNBytes(MAX + 1) to readNBytes(MAX), and a legal 5 MiB template starts being rejected
with 413 (or an oversized one is silently truncated to exactly 5 MiB and validated as if
complete) with the whole suite still green.

---

12. List.copyOf(errors) NPEs on a null error list

src/main/java/org/broadinstitute/consent/http/models/dto/registration/template/TemplateValidationResponse.java:19

TemplateValidationResponse.invalid(null, false) — or Gson record deserialization of a
response body that omits errors, which the ExamplesTest path would hit if an example were
written without that key — calls List.copyOf(null) and NPEs with no message, surfacing as a
bare 500 rather than pointing at the missing field.

Fix: errors == null ? List.of() : List.copyOf(errors).

[kevinmarete]

Thanks for the review — all twelve findings are triaged below, in seven commits on top of 9cb249f2. Ten are fixed, one dissolved rather than patched, and two are deliberately left open with reasons. One finding's mechanism was misdescribed, though its conclusion held; the correction is worth reading before the diff.

#	Finding	Outcome
1	U+0000 strip corrupts escaped backslashes	Fixed in 946aa12a. A negative lookbehind spares an already-escaped backslash. See the correction below.
2	Server-side draft-write failure surfaces as 400	Left as-is. DraftServiceDAO.insertDraft wraps every write failure for all of its callers; changing that belongs in its own PR. The spec's 400 now names the case, and the 500 it was missing is documented — 9b54566d.
3	Spec-drift test passes vacuously on a missing pointer	Fixed in 9b54566d. requiredAt instead of at.
4	IOException subclasses miss the exact-class DISPATCH	Dissolved by 66231888 rather than patched. The resource no longer reads the bytes, so no IOException escapes it; the validator, which does read them, already reports a read failure as a validation error. The exact-class lookup in Resource is untouched.
5	Creation branch returns 200 instead of 201	Fixed in 40d062eb. Response.created at the draft's path, mirroring DraftResource.createDraftRegistration; an invalid template still answers 200.
6	The 5 MiB limit is enforced twice, with divergent outcomes	Fixed in 66231888. The validator owns it and raises a typed exception the resource maps to 413.
7	Upload is buffered twice, doubling peak memory	Fixed in 66231888. The part streams straight through, so one copy instead of two.
8	Jackson @JsonInclude on a type only ever serialized with Gson	Fixed in 9b54566d. Annotation dropped; the javadoc names the mechanism that actually holds.
9	Third independent new ObjectMapper()	Fixed in 97b55684. One binding supplies the service and DatasetResource, and the contract test uses that single instance so it can now catch the drift it exists to catch.
10	Duplicated role assertion with a drifting HTTP-method filter	Fixed in c76bed24. One helper owns the verb set; both resource tests call it.
11	No test at exactly MAX_TEMPLATE_BYTES	Fixed in 66231888, in the validator's test where the check now lives. A file of exactly 5 MiB is read and validated; one byte more is refused.
12	List.copyOf(errors) NPEs on a null error list	Fixed in 9b54566d, covered by 34a73346.

The correction on #1. The text block does not resolve to a literal backslash plus U+0000. The \u0000 in it is not an eligible Java unicode escape — the backslash before u follows another eligible backslash — so the SQL holds the six-character text \\u0000, and the regex matches a literal backslash followed by u0000. The conclusion was right regardless: that pattern also matches the tail of an already-escaped backslash, the strip starts at the second one, and the character after it goes too. DraftDAOTest now pins it, and against the previous pattern that test fails with Postgres's own ERROR: invalid input syntax for type json — the 400 predicted in the review.

On #5 and duos-ui. The 201 costs nothing on the client: fetchMultipart gates on res.ok, so the whole 2xx range reaches the caller, and callers discriminate on the body's valid. Neither DT-1854 test asserts a success status. Its prose is updated on km-dt-1854-upload-template-handling so nobody adds a status check a 201 would fail.

Also left open. DataAccessRequestDAO carries the same U+0000 pattern in four statements, where it has shipped since the 2025 migration. Fixing it there needs its own change against its own tests rather than riding along here.

Tests run: 267 green across the twelve suites this touches, two of them against a real Postgres container. JaCoCo reports 100% line and branch coverage on every new class, spotless:check is clean, and the OpenAPI generator validation passes.

@fboulnois — your 👍 was on 9cb249f2, so it predates all seven of these. The 201 in 40d062eb is the one that changes the contract; the rest are internal. Happy to re-request if you'd rather look again.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud