Friendly app-level validation for a RANGE with end-before-start (#678) #706
Reference in New Issue
Block a user
Delete Branch "feat/issue-678-range-date-validation"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes #678.
Turns the "RANGE with an invalid end date" class of errors from an HTTP 500 + Sentry alert into a friendly, self-identifying 400 — without weakening the DB CHECK, which stays the backstop.
What changed
Backend
ErrorCode.INVALID_DATE_RANGE(400).DocumentService.validateDateRange(doc)— one guard, two predicates, run right afterapplyDatePrecision(i.e. before any save —updateDocumentTagspersists earlier inupdateDocument), validating the post-apply doc state:chk_meta_date_end_after_startexactly (isBefore, so equal dates and a null start are valid).chk_meta_date_end_only_for_range(API-only defense; same code, distinct dev message).GlobalExceptionHandlernow has a generic@ExceptionHandler(DataIntegrityViolationException)backstop → 400VALIDATION_ERRORwith a fixed message. It never echoesex.getMessage()(constraint name + SQL, CWE-209), logs at WARN without passing the exception (would re-leak the SQL to Loki), and does not callSentry.captureException.Frontend
WhoWhenSection.svelte:endBeforeStartderived (lexicographic ISO compare, noDate) renders an inline error on the end-date field —border-red-400,aria-invalid,aria-describedby, a#end-date-error<p>inside the existingaria-liveregion, with a ⚠ glyph so the cue is not colour-alone (WCAG 1.4.1). Save is not disabled; the server stays the gate.errors.tsmapsINVALID_DATE_RANGE→m.error_invalid_date_range(); single new i18n key added to de/en/es, used both inline (client) and viagetErrorMessage(server fallback).Acceptance criteria
WhoWhenSection.svelte.test.ts).DocumentServiceTest); AC2 also proven against real Postgres (DocumentRepositoryTest, Testcontainers).INVALID_DATE_RANGE, never reaching the DB (DocumentServiceTest).DataIntegrityViolationException→ 400VALIDATION_ERROR, body free ofchk_/meta_date, WARN log, no Sentry (GlobalExceptionHandlerTest).Verification
./mvnw test -Dtest=DocumentServiceTest→ 174 green;-Dtest=GlobalExceptionHandlerTest→ green;-Dtest=DocumentRepositoryTest#save_acceptsRange_whenEndEqualsStart→ green (Testcontainers)../mvnw clean package -DskipTests→ BUILD SUCCESS.npm run checkclean for the touched files (Svelte MCP autofixer reported no issues); browser-mode component tests left to CI per project convention.Follow-up (out of scope, noted from implementation)
null, whichapplyDatePrecisionskips rather than clears). Today that already 500s at the DB; this PR makes it a clean 400, but the ideal fix is to clearmetaDateEndwhen precision leaves RANGE so the switch simply succeeds. Worth a small follow-up issue.🤖 Generated with Claude Code
🏛️ Markus Keller — Application Architect
Verdict: ✅ Approved
The shape is right: domain-owned validation as a private guard on
DocumentService, mirroring the existingRelationshipService.validateYearsprecedent, plus a deliberately-generic infrastructure backstop inGlobalExceptionHandler. No layer boundaries crossed, no new coupling, no new dependency (DataIntegrityViolationExceptionis already on the Spring Data classpath). The DB CHECK stays the single source of truth and the app guard is an honest mirror of it — that's the correct relationship between the two.Suggestions (non-blocking)
validateDateRangeis wired only intoupdateDocument. Today that's correct (the create/upload path writesdocumentDateonly and structurally can't violate either constraint), and the backstop covers the import path. But the guard's safety now depends on an invariant in a different method staying true. IfcreateDocumentever grows precision/end handling, this guard won't protect it and the contributor won't get a compile-time nudge. Consider a one-line comment onvalidateDateRangenaming the callers it is (and isn't) wired into, so the asymmetry is intentional-by-record rather than incidental.validateDateRange(doc)must run beforeupdateDocumentTags(which persists). The inline comment captures this well — good. Keep it; it's the kind of thing a future refactor silently breaks.INVALID_DATE_RANGEcode for two predicates is a sound call while the user-facing message is identical. TheErrorCodeJavadoc already documents both meanings — that's the right place for it.Clean, minimal, reversible. No structural objections.
👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ✅ Approved
Red→green per behaviour, one commit each, clean history. The guard reads exactly as the DB constraint does —
isBefore(not!isAfter) with the null-start escape — and the comment says why, which is the part people get wrong.endBeforeStartmirrors the existingdateInvalididiom, so the component stays consistent with itself. The test helpers (docForRangeUpdate,rangeDto) keep the new cases to three or four lines each. This is the kind of small, self-explaining change I like.Suggestions (non-blocking)
validateDateRange(server) andendBeforeStart(client). That's an acceptable duplication (different languages, different jobs: one gates, one hints), but they can drift. Not worth abstracting; just worth a mental note that a change to the rule means touching both.updateDocument_preservesStoredPrecision_whenDtoOmitsItfixture fromMONTH+end(a DB-impossible state) toRANGE+endwas the right move, and the new comment explains it. Good — you didn't weaken the assertion to make the guard pass, you corrected an invalid fixture.Nothing to change. Ship it once CI is green.
🔧 Tobias Wendt — DevOps & Platform Engineer
Verdict: ⚠️ Approved with concerns
The headline win is squarely in my lane: a user date typo no longer fabricates a Sentry issue and an ERROR line in Loki. Logging at WARN without passing the exception object is exactly right — passing
exwould have dumped the constraint SQL back into Loki and re-created the leak you're closing. No new infra, no new env var. Good.Concern (please weigh before merge)
The generic backstop trades a loud 500 for a silent 400 across all integrity violations, not just date ranges. Before this PR, any
DataIntegrityViolationException— a genuine bug writing a NULL into a NOT NULL column, a unique-constraint race, a future FK violation — surfaced as a 500 +Sentry.captureException+ stack trace. That's noisy, but it's visible: it shows up in GlitchTip and pages someone. After this PR, every one of those becomesWARN "Rejected a request that violated a database integrity constraint"with no constraint name, no stack, no Sentry. A latent write bug could now fail silently in production and we'd never know until a user complains.The known date-range cases are caught upstream and never reach here — agreed. My worry is the unanticipated ones, which are precisely the ones you want an alert for. Two options that keep the CWE-209 fix intact:
ex.getMessage()) at WARN — e.g. pullex.getMostSpecificCause()'s constraint identifier if available — so Loki can tell you which constraint fired without leaking SQL. A bare "some constraint, somewhere" WARN is nearly undebuggable at 2am.Sentry.captureException(ex)(or a sanitized breadcrumb) so an unexpected integrity violation remains visible in error tracking. The response stays clean; the alert stays alive.Either is fine; the current "swallow everything to WARN" is the one I'd push back on. This is a deliberate observability trade-off and it should be a decision, not a side effect.
Not a merge blocker for the date-range feature itself — but I'd like a line in the PR (or a follow-up) acknowledging the broadened swallow.
📋 "Elicit" — Requirements Engineer
Verdict: ⚠️ Approved with concerns
Strong traceability: all nine ACs map to named tests, and the EARS statement in the issue is satisfied. AC7 in particular is handled honestly — one
error_invalid_date_rangekey in all three locales, used both inline (client) and viagetErrorMessage(server fallback), so there's a single translated string rather than two that drift. AC2's "equal dates are valid" is even pinned against real Postgres, which is the right level of rigor for a rule that mirrors a DB constraint.Concern — a requirement assumption in the issue is factually wrong
The issue's decided approach states, for predicate 2: "this branch is API-only — the edit form clears the end field off-RANGE." The PR's own follow-up note correctly identifies that this is not what the form does: switching a stored RANGE document to a non-RANGE precision leaves the previously-stored end date in place (the empty hidden field binds to
null, whichapplyDatePrecisionskips rather than clears). So a real user on the form — not just an API client — can hit predicate 2, and when they do they'll see "The end date must not be before the start date", which is the wrong sentence for the "end without RANGE" situation.This doesn't block the PR (the behaviour is strictly better than today's 500, and the shared-message decision was made deliberately), but it means AC6's stated scope ("via API") understates reality, and there's an unwritten requirement hiding here:
Please open the follow-up issue the PR mentions and capture that as an explicit requirement, including which message the user should see. Right now the "truth" is split between the issue body (wrong assumption) and a PR comment (correct) — fold the correction back into a tracked requirement so it isn't lost.
Everything testable is tested. The gap is a missing requirement, not a missing test.
🛡️ Nora Steiner ("NullX") — Application Security Engineer
Verdict: ✅ Approved
This PR closes a CWE-209 (Information Exposure Through an Error Message) hole, and does it properly — I checked for the usual half-fixes and didn't find them:
ex.getMessage(). ADataIntegrityViolationExceptionfrom Hibernate carries the constraint name, the column, and often a fragment of the failing SQL; none of that now reaches the client. ✅log.warn("...")with no exception argument. This is the subtle one most people miss —log.warn("msg", ex)would serialize the full cause chain (including the SQL) straight into Loki, just moving the leak from the HTTP response to the log store. You avoided it, and the test even asserts no log event containschk_. ✅chk_andmeta_date, andSentry.verifyNoInteractions(). That's a permanent guard against someone "helpfully" re-addingex.getMessage()later. ✅Notes (non-blocking)
DomainException(INVALID_DATE_RANGE)developer messages ("meta_date_end must not be before meta_date") are surfaced to the client viahandleDomain. That's fine — they're hand-written, contain no secrets, no SQL, no schema internals beyond column names the OpenAPI spec already exposes. No disclosure concern.Good, defensive work. This is the right way to turn a 500 into a 400 without trading an information leak for it.
🧪 Sara Holt — QA Engineer & Test Strategist
Verdict: ⚠️ Approved with concerns
This was my follow-up from #677 and I'm pleased with the coverage shape: six service-level cases spanning the real boundary (
end < start,end == start,end > start,end == null,start == null && end set, andend without RANGE), a handler test that mocksSentryand attaches a LogbackListAppenderto assert the WARN level and the no-leak property, and a Testcontainers test that proves the equal-date case against real Postgres rather than H2. Thestart == null && end setcase explicitly exists to catch an NPE in the guard — that's the kind of boundary I'd have asked for, and it's already here.Concerns
WhoWhenSection.svelte.test.tssuite actually passes in CI — the AC5 "clears on correction" path in particular depends onhandleGermanDateInput('12.01.1917')producing1917-01-12, which is an assumption the local run never verified. If CI is red on these, the inline-clear behaviour is unproven.end < start— the app guard intercepts it first, so the CHECK itself is only assumed to fire. That's acceptable (the guard is the user-facing contract), but if the guard ever regresses, nothing catches the drift at the DB layer. AassertThatThrownBy(() -> saveAndFlush(end<start)).isInstanceOf(DataIntegrityViolationException.class)would close that, and would also exercise the new backstop end-to-end. Worth considering as a cheap addition.updateDocument_acceptsRange_whenEndAfterStartasserts onlysavewas called, not the persisted end value. Tightening it to assertdoc.getMetaDateEnd()(like the equal-date sibling does) would make the intent self-evident.None of these block the date-range logic, which is well tested. Concern #1 is the one I want resolved (CI confirmation) before this merges.
🎨 Leonie Voss — UI/UX & Accessibility Lead
Verdict: ✅ Approved
This is how an inline validation error should be built, and I'm glad the non-colour cue wasn't an afterthought:
aria-hidden="true", so a screen reader gets the sentence, not "warning sign warning sign". Correct on both counts. ✅aria-invalid="true"andaria-describedby="end-date-error"are wired to the input, and the#end-date-errormessage lives inside the existingaria-live="polite"region — so when it appears, it's announced without stealing focus. ✅min-h-[48px]touch target through the error state — the conditional only swaps the border/ring colour, so the tap target doesn't shift. ✅Suggestions (non-blocking)
role="alert"or leaving it to thearia-live="polite"parent — right now it relies on the polite region, which is the gentler, correct choice for a non-critical inline hint. No change needed; just confirming the polite (not assertive) level is intentional. It is appropriate here.text-red-600onbg-surfaceclears AA for this text size, but the ⚠ glyph carries the meaning regardless, so even users who can't distinguish the red are covered. Nicely belt-and-braces.No accessibility blockers. This pattern is worth reusing for the start-date field too if we ever add cross-field rules there.
🔧 Review concerns addressed
Pushed
ff7cfd4and1dd162fin response to the review. No hard blockers were raised; the actionable concerns are resolved as follows.🔧 Tobias (DevOps) — silent swallow of unanticipated violations → fixed in
ff7cfd4.The backstop now extracts the constraint name from the cause chain and logs it parameterized at WARN (
"…violated a database integrity constraint: {}"), so an unexpected violation is debuggable in Loki instead of an anonymous mystery. It still never passesex/getMessage()(SQL + values, CWE-209) and still doesn't call Sentry — the response stays generic, so the response logic remains non-brittle (consistent with #678's "don't branch the response on the constraint"). New test asserts the WARN carries the constraint name but never the SQL statement. (I went with your option 1 rather than re-enabling Sentry, since #678 deliberately decided integrity violations are 400s, not system-fault alerts.)🧪 Sara (QA) →
1dd162f): a Testcontainers test now proves Postgres rejectsend < startwith aDataIntegrityViolationException— confirmingchk_meta_date_end_after_startactually fires (not just assumed) and exercising the backstop's trigger end-to-end.1dd162f):…whenEndAfterStartnow asserts the persistedmetaDateEnd, not just thatsaveran.handleGermanDateInput('12.01.1917')→germanToIso→1917-01-12, so'1917-01-12' < '1917-01-11'isfalseand the inline error clears. The render itself remains CI's call.📋 Elicit (Req Engineer) — the "form clears end off-RANGE" assumption is false → tracked as #707 (labeled feature/ui/P3-later). The clear-on-precision-switch fix can't live here without contradicting this PR's locked AC6 (which asserts a 400), so it's a deliberate follow-up. The correct requirement is captured there with its own ACs.
🏛️ Markus / 👨💻 Felix / 🛡️ Nora / 🎨 Leonie — approved, suggestions noted; Markus's create/update asymmetry and Felix's two-places-for-the-predicate are acknowledged as acceptable-by-design (the latter is documented in the guard/component comments).
Local verification after the changes:
DocumentServiceTest+DocumentRepositoryTest→ 205 green,GlobalExceptionHandlerTest→ 3 green, lint clean.