From b47f5a9216aeb0c57d80e722d242378de3b2fb18 Mon Sep 17 00:00:00 2001 From: Kevin Archie Date: Thu, 3 Sep 2026 13:50:38 -0600 Subject: [PATCH] PLUGINS-364 validate DA/TM query parameters; stop accepting wildcards --- CHANGELOG.md | 35 +++ CONFORMANCE.md | 57 +++- docs/QIDO_RS_IMPLEMENTATION.md | 30 +++ docs/USER_GUIDE.md | 17 +- .../service/impl/XnatDicomServiceImpl.java | 36 ++- .../dicomweb/util/DicomDateTimeValues.java | 200 ++++++++++++++ .../util/DicomQueryValueValidator.java | 114 ++++++++ .../xnat/dicomweb/util/DicomRangeParser.java | 80 +++--- .../dicomweb/util/QidoQueryParamParser.java | 26 ++ .../service/StudyDateTimeFilterSqlTest.java | 186 +++++++++++++ .../util/DicomQueryValueValidatorTest.java | 249 ++++++++++++++++++ .../dicomweb/util/DicomRangeParserTest.java | 67 ++++- .../util/QidoQueryParamParserTest.java | 63 +++++ 13 files changed, 1091 insertions(+), 69 deletions(-) create mode 100644 src/main/java/org/nrg/xnat/dicomweb/util/DicomDateTimeValues.java create mode 100644 src/main/java/org/nrg/xnat/dicomweb/util/DicomQueryValueValidator.java create mode 100644 src/test/java/org/nrg/xnat/dicomweb/service/StudyDateTimeFilterSqlTest.java create mode 100644 src/test/java/org/nrg/xnat/dicomweb/util/DicomQueryValueValidatorTest.java diff --git a/CHANGELOG.md b/CHANGELOG.md index 2c0f18f..52d99c5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,41 @@ All notable changes to the XNAT DICOMweb Plugin will be documented in this file. +## [Unreleased] + +### Fixed +- **QIDO-RS date and time parameters are now validated** — a malformed + `StudyDate` or `StudyTime` returns **HTTP 400** with an + `InvalidParameter` error, per PS3.18 §10.6.3.1. Previously such + values produced an HTTP 200 with an empty result list, because the + `BadRequestException` raised while building the query was caught by + the catch-all in `XnatDicomServiceImpl.searchStudies` and logged + rather than propagated. + - A malformed but 8-digit date such as `StudyDate=20251345` reached + the database as `2025-13-45` and failed there, with the same + swallowed result. + - A malformed date of any other length, such as `StudyDate=garbage`, + matched no filter branch at all and was silently dropped, so the + query returned **every** study rather than none. + - `StudyTime` values were never validated; an invalid time simply + matched nothing. +- **Partial-precision `StudyTime` values are now accepted.** PS3.5 §6.2 + allows the TM components `MM`, `SS` and `FFFFFF` to be unspecified + from the right, so `10`, `1030` and `103000.5` are valid times. The + range parser previously required exactly 6 digits, which rejected + `StudyTime=1000-1800` — the worked example in PS3.4 §C.2.2.2.5.4. + Leap seconds (`SS`=60) are also accepted. + +### Changed +- **Wildcards are no longer accepted in `StudyDate` / `StudyTime`.** + PS3.4 §C.2.2.2.4 defines Wild Card Matching only for attributes of + VR AE, CS, LO, LT, PN, SH, ST, UC, UR and UT; DA and TM are excluded. + `StudyDate=2025*` now returns 400. A bare `*` is still accepted as + universal matching, per the §C.2.2.2.4 note that "Wild Card Matching + on a value of `*` is equivalent to Universal Matching". Wildcard + matching on `PatientName`, `SeriesDescription` and other string + attributes is unaffected. + ## [1.2.0] - 2026-04-09 ### Added diff --git a/CONFORMANCE.md b/CONFORMANCE.md index 1430c77..7762dc3 100644 --- a/CONFORMANCE.md +++ b/CONFORMANCE.md @@ -130,6 +130,26 @@ Clients relying on the PS3.18 convention will not detect that more results remain. Clients aware of the plugin's `X-Total-Count` can compute the remainder as `X-Total-Count − offset − returned`. +### 0.11 A partial-precision `StudyTime` matches every time sharing its components + +PS3.5 §6.2 allows a TM value to omit components from the right, +"which indicates that the Value is not precise to the precision of +those unspecified components", but PS3.4 §C.2.2.2.1 defines Single +Value Matching as matching "exactly the value specified in the +request" without saying how the two interact. + +The plugin resolves this by comparing only the components the client +supplied: `StudyTime=10` matches every session in the 10:00 hour and +`StudyTime=1030` every session in the 10:30 minute, rather than +matching only 10:00:00 and 10:30:00 exactly. A fractional second in +a single value is accepted but ignored, because XNAT stores no +sub-second precision. + +Note that this differs from the *range* case, where an unspecified +component resolves to zero because a range endpoint denotes a single +instant — `StudyTime=1000-1800` ends at 18:00:00 exactly, per the +worked example in PS3.4 §C.2.2.2.5.4. + --- ## 1. Overview @@ -376,8 +396,8 @@ enforce TLS itself and does not configure CORS (see Section 10.2). | Parameter | Levels | DICOM Tag | Notes | |----------------------|-------------------------|--------------|----------------------------------------| | `StudyInstanceUID` | Study, Series, Instance | (0020,000D) | UID-list (comma-separated) not supported | -| `StudyDate` | Study | (0008,0020) | Exact, wildcard, or range | -| `StudyTime` | Study | (0008,0030) | Exact, wildcard, or range | +| `StudyDate` | Study | (0008,0020) | Exact or range; malformed values → 400 | +| `StudyTime` | Study | (0008,0030) | Exact or range; malformed values → 400 | | `PatientName` | Study | (0010,0010) | | | `PatientID` | Study | (0010,0020) | | | `AccessionNumber` | Study | (0008,0050) | Matches the XNAT session ID (see 6.5) | @@ -402,16 +422,35 @@ enforce TLS itself and does not configure CORS (see Section 10.2). files. - **Wildcard** — `*` and `?`, translated to SQL `ILIKE` patterns for study-level keys and to regex-like Java matching for lower levels. - No escape mechanism for literal `*` / `?`. + No escape mechanism for literal `*` / `?`. Per PS3.4 §C.2.2.2.4 + this applies only to attributes of VR AE, CS, LO, LT, PN, SH, ST, + UC, UR and UT, so it is **not** available on `StudyDate` or + `StudyTime` — see below. - **Range** — supported at all levels, per PS3.4 §C.2.2.2.5.1 (DA) - and §C.2.2.2.5.2 (TM). Range endpoints must be full 8-digit - `yyyyMMdd` (DA) or 6-digit `HHmmss` (TM); wildcards inside a range - endpoint are not permitted. Malformed range values at the study - level return HTTP 400. + and §C.2.2.2.5.2 (TM). Wildcards inside a range endpoint are not + permitted. Endpoints follow the VR grammar of PS3.5 §6.2: a date + is a full 8-digit `yyyyMMdd`, while a time may use the + partial-precision forms `HH`, `HHmm` and `HHmmss` and an optional + 1-to-6-digit fractional second. Unspecified time components + resolve to zero, so `StudyTime=1000-1800` spans 10:00:00 to + 18:00:00 — the worked example in PS3.4 §C.2.2.2.5.4. - **Universal** — empty parameter value matches everything. The DICOM range marker `-` (both bounds omitted) is treated the - same way at the study level. -- **Date format** — DICOM `yyyyMMdd` only. + same way at the study level, as is a bare `*` in `StudyDate` or + `StudyTime` (PS3.4 §C.2.2.2.4 note: "Wild Card Matching on a value + of `*` is equivalent to Universal Matching"). +- **Date and time validation** — `StudyDate` and `StudyTime` values + are validated against the DA and TM grammars at the REST boundary. + A malformed value returns HTTP 400 with an `InvalidParameter` + error naming the parameter, per PS3.18 §10.6.3.1 ("400 (Bad + Request) — The was a problem with the request. For example, the + Query Parameter syntax is incorrect."). This covers malformed + single values (`20251345`), malformed range endpoints + (`20250101-nonsense`), malformed range structure + (`20250101-20250201-20250301`), and wildcards. +- **Date format** — DICOM `yyyyMMdd` only. The ISO `yyyy-MM-dd` form + is rejected: `-` is the DICOM range separator, so `2025-01-15` + parses as a three-part range and returns 400. ### 6.4 Pagination diff --git a/docs/QIDO_RS_IMPLEMENTATION.md b/docs/QIDO_RS_IMPLEMENTATION.md index 7e194e4..926a462 100644 --- a/docs/QIDO_RS_IMPLEMENTATION.md +++ b/docs/QIDO_RS_IMPLEMENTATION.md @@ -225,8 +225,38 @@ StudyTime=080000-170000 # Combined date and time StudyDate=20240101-20240131&StudyTime=080000-170000 + +# Open-ended ranges +StudyDate=20240101- # on or after Jan 1 +StudyDate=-20240131 # on or before Jan 31 + +# Times may use partial precision (PS3.5 §6.2) +StudyTime=10 # the 10:00 hour +StudyTime=1000-1800 # 10:00:00 through 18:00:00 ``` +### Validation + +`StudyDate` and `StudyTime` values are validated against the DICOM DA +and TM grammars before the query runs. A malformed value returns +**HTTP 400** with an `InvalidParameter` error naming the parameter — +it does *not* return an empty result set. + +```bash +StudyDate=20251345 # 400 — no such month/day +StudyDate=2025-01-15 # 400 — "-" is the range separator, + # so this parses as a 3-part range +StudyDate=20250101-nonsense # 400 — malformed range endpoint +StudyTime=250000 # 400 — hour out of range +StudyDate=2025* # 400 — wildcards are not defined for + # dates/times (PS3.4 §C.2.2.2.4) +``` + +Wildcard matching applies only to string-valued attributes such as +`PatientName` and `SeriesDescription`. A bare `StudyDate=*` is +accepted as universal matching and is equivalent to omitting the +parameter. + --- ## Pagination diff --git a/docs/USER_GUIDE.md b/docs/USER_GUIDE.md index a0fcaaf..0f95ef7 100644 --- a/docs/USER_GUIDE.md +++ b/docs/USER_GUIDE.md @@ -310,6 +310,19 @@ Add query parameters to filter results: ```bash ?StudyDate=20241201 # Specific date ?StudyDate=20240101-20241231 # Date range (all of 2024) +?StudyDate=20240101- # On or after Jan 1 +?StudyDate=-20241231 # On or before Dec 31 +``` + +Dates use the DICOM `YYYYMMDD` format. A malformed date returns +**400 Bad Request** rather than an empty result list: + +```bash +?StudyDate=2024-12-01 # 400 — use 20241201; a hyphen + # means a range, not a separator +?StudyDate=20241345 # 400 — no month 13 +?StudyDate=2024* # 400 — wildcards work on names, + # not on dates or times ``` **By Patient ID:** @@ -384,8 +397,8 @@ curl -u user:pass \ |-----------|------------|---------|------------------| | `PatientName` | Studies | `DOE^JOHN` | Yes (`*`, `?`) | | `PatientID` | Studies | `P12345` | No | -| `StudyDate` | Studies | `20241201-20241231` | Range supported | -| `StudyTime` | Studies | `080000-170000` | Range supported | +| `StudyDate` | Studies | `20241201-20241231` | No — range supported; malformed values return 400 | +| `StudyTime` | Studies | `080000-170000` | No — range supported; malformed values return 400 | | `StudyInstanceUID` | Studies | `1.2.840...` | No | | `AccessionNumber` | Studies | `ACC123` | No | | `Modality` | Studies, Series | `CT` | No | diff --git a/src/main/java/org/nrg/xnat/dicomweb/service/impl/XnatDicomServiceImpl.java b/src/main/java/org/nrg/xnat/dicomweb/service/impl/XnatDicomServiceImpl.java index d718e4e..62cfa8b 100644 --- a/src/main/java/org/nrg/xnat/dicomweb/service/impl/XnatDicomServiceImpl.java +++ b/src/main/java/org/nrg/xnat/dicomweb/service/impl/XnatDicomServiceImpl.java @@ -74,6 +74,7 @@ import org.nrg.xnat.dicomweb.service.SiteWideProjectFilter; import org.nrg.xnat.dicomweb.service.XnatDicomService; import org.nrg.xnat.dicomweb.util.BulkDataHandler; +import org.nrg.xnat.dicomweb.util.DicomDateTimeValues; import org.nrg.xnat.dicomweb.util.DicomRangeParser; import org.nrg.xnat.dicomweb.util.DicomWebUtils; import org.nrg.xnat.utils.CatalogUtils; @@ -468,16 +469,13 @@ private static void appendStudyDateClauses( sql.append("AND e.date <= CAST(:q_study_date_end AS DATE) "); params.addValue("q_study_date_end", r.end.toString()); } - } else if (studyDate.contains("*") || studyDate.contains("?")) { - appendIlikeFilter(sql, params, studyDate, - "TO_CHAR(e.date, 'YYYYMMDD')", "q_study_date"); - } else if (studyDate.length() == 8) { - String sqlDate = studyDate.substring(0, 4) + "-" - + studyDate.substring(4, 6) + "-" - + studyDate.substring(6, 8); - sql.append("AND e.date = CAST(:q_study_date AS DATE) "); - params.addValue("q_study_date", sqlDate); + return; } + // Single Value Matching, PS3.4 §C.2.2.2.1. The value reached + // here through QidoQueryParamParser, so it is a well-formed DA. + sql.append("AND e.date = CAST(:q_study_date AS DATE) "); + params.addValue("q_study_date", + DicomDateTimeValues.parseDate("StudyDate", studyDate).toString()); } private static void appendStudyTimeClauses( @@ -496,10 +494,18 @@ private static void appendStudyTimeClauses( sql.append("AND e.time <= CAST(:q_study_time_end AS TIME) "); params.addValue("q_study_time_end", r.end.toString()); } - } else { - appendIlikeFilter(sql, params, studyTime, - "TO_CHAR(e.time, 'HH24MISS')", "q_study_time"); + return; } + // Single Value Matching, PS3.4 §C.2.2.2.1. TM permits partial + // precision (PS3.5 §6.2), and a value "not precise to the + // precision of those unspecified components" matches every + // stored time sharing the specified components — so compare + // only as many leading digits as the client supplied. + String prefix = DicomDateTimeValues.timeMatchPrefix(studyTime); + sql.append("AND LEFT(TO_CHAR(e.time, 'HH24MISS'), :q_study_time_digits) " + + "= :q_study_time "); + params.addValue("q_study_time_digits", prefix.length()); + params.addValue("q_study_time", prefix); } @Override @@ -643,6 +649,12 @@ public List searchStudies(UserI user, String projectId, Attributes q } log.debug("Found {} studies{}", results.size(), siteWide ? " (site-wide)" : " in project " + projectId); + } catch (DicomWebException e) { + // A client error (e.g. a malformed query value) must reach + // GlobalExceptionHandler and become its own status code. + // Folding it into the catch-all below would report an + // invalid request as an empty but successful result set. + throw e; } catch (Exception e) { log.error("Error searching studies{}", projectId != null ? " in project " + projectId : " (site-wide)", e); } diff --git a/src/main/java/org/nrg/xnat/dicomweb/util/DicomDateTimeValues.java b/src/main/java/org/nrg/xnat/dicomweb/util/DicomDateTimeValues.java new file mode 100644 index 0000000..ee64678 --- /dev/null +++ b/src/main/java/org/nrg/xnat/dicomweb/util/DicomDateTimeValues.java @@ -0,0 +1,200 @@ +/* + * XNAT DICOMweb Plugin + * Copyright (c) 2026 XNATWorks. + * All rights reserved. + */ + +package org.nrg.xnat.dicomweb.util; + +import org.nrg.xnat.dicomweb.exceptions.BadRequestException; + +import java.time.LocalDate; +import java.time.LocalTime; +import java.time.format.DateTimeFormatter; +import java.time.format.DateTimeParseException; +import java.time.format.ResolverStyle; + +/** + * Lexical and semantic validation of DICOM DA and TM values appearing + * as QIDO-RS query parameter values. This is the single source of + * truth for DA/TM syntax in the plugin; {@link DicomRangeParser} uses + * it for range endpoints and {@link QidoQueryParamParser} uses it to + * reject malformed values at the REST boundary. + * + *

Grammar per PS3.5 §6.2 Table 6.2-1: + * + *

+ * DA — "A string of characters of the format YYYYMMDD; + * where YYYY shall contain year, MM shall contain the month, and DD + * shall contain the day, interpreted as a date of the Gregorian + * calendar system." Length is "8 bytes fixed". + *
+ * + *
+ * TM — "A string of characters of the format + * HHMMSS.FFFFFF; where HH contains hours (range "00" - "23"), MM + * contains minutes (range "00" - "59"), SS contains seconds (range + * "00" - "60"), and FFFFFF contains a fractional part of a second as + * small as 1 millionth of a second… One or more of the + * components MM, SS, or FFFFFF may be unspecified as long as every + * component to the right of an unspecified component is also + * unspecified, which indicates that the Value is not precise to the + * precision of those unspecified components. The FFFFFF component, + * if present, shall contain 1 to 6 digits. If FFFFFF is unspecified + * the preceding "." shall not be included." + *
+ * + *

PS3.5 gives DA no partial-precision form, so dates must always + * be a full 8 digits. TM does have one, so {@code "10"}, + * {@code "1030"}, {@code "103000"} and {@code "103000.5"} are all + * accepted; PS3.5 cites {@code "021"} as an explicitly invalid value. + * + *

Both VRs permit trailing SPACE padding, which is stripped before + * parsing. + */ +public final class DicomDateTimeValues { + + // Strict resolver style catches invalid calendar dates like + // 20200230; it requires the proleptic-year pattern 'uuuu' rather + // than 'yyyy' since the latter needs an era in strict mode. + private static final DateTimeFormatter DA_FORMAT = + DateTimeFormatter.ofPattern("uuuuMMdd") + .withResolverStyle(ResolverStyle.STRICT); + + private DicomDateTimeValues() {} + + /** + * Parse a DICOM DA value. + * + * @param paramName query parameter name, for the error message + * @param value the raw value; trailing SPACE padding is allowed + * @return the parsed date + * @throws BadRequestException if the value is not a valid DA + */ + public static LocalDate parseDate(String paramName, String value) { + final String v = stripPadding(value); + if (v.length() != 8 || !isAllDigits(v)) { + throw new BadRequestException(paramName, + "'" + value + "' is not a valid DICOM date; " + + "expected 8 digits in the form yyyyMMdd"); + } + try { + return LocalDate.parse(v, DA_FORMAT); + } catch (DateTimeParseException e) { + throw new BadRequestException(paramName, + "'" + value + "' is not a valid calendar date"); + } + } + + /** + * Parse a DICOM TM value, including the partial-precision forms + * {@code HH} and {@code HHMM} and an optional fractional second. + * + *

Unspecified components are resolved to zero, so {@code "10"} + * denotes 10:00:00 exactly. A leap second ({@code SS} = 60, which + * PS3.5 permits) is clamped to the last representable instant of + * the same minute, since {@link LocalTime} cannot represent it. + * + * @param paramName query parameter name, for the error message + * @param value the raw value; trailing SPACE padding is allowed + * @return the parsed time + * @throws BadRequestException if the value is not a valid TM + */ + public static LocalTime parseTime(String paramName, String value) { + final String v = stripPadding(value); + final int dot = v.indexOf('.'); + final String whole = (dot < 0) ? v : v.substring(0, dot); + final String fraction = (dot < 0) ? "" : v.substring(dot + 1); + + // Components are two digits each and may only be omitted from + // the right, so the integral part is 2, 4, or 6 digits. + if (!isAllDigits(whole) + || (whole.length() != 2 && whole.length() != 4 && whole.length() != 6)) { + throw new BadRequestException(paramName, + "'" + value + "' is not a valid DICOM time; expected " + + "HH, HHmm, HHmmss, or HHmmss.FFFFFF"); + } + // "If FFFFFF is unspecified the preceding '.' shall not be + // included", and a fraction is only meaningful once seconds + // are specified. + if (dot >= 0 && (whole.length() != 6 + || fraction.isEmpty() || fraction.length() > 6 + || !isAllDigits(fraction))) { + throw new BadRequestException(paramName, + "'" + value + "' is not a valid DICOM time; a fractional " + + "second must follow HHmmss and contain 1 to 6 digits"); + } + + final int hour = twoDigitsAt(whole, 0); + final int minute = (whole.length() >= 4) ? twoDigitsAt(whole, 2) : 0; + final int second = (whole.length() >= 6) ? twoDigitsAt(whole, 4) : 0; + + if (hour > 23 || minute > 59 || second > 60) { + throw new BadRequestException(paramName, + "'" + value + "' is not a valid DICOM time; hours must be " + + "00-23, minutes 00-59, seconds 00-60"); + } + if (second == 60) { + // Leap second: no LocalTime equivalent, so use the last + // instant of the minute for comparison purposes. + return LocalTime.of(hour, minute, 59, 999_999_999); + } + return LocalTime.of(hour, minute, second, nanosOf(fraction)); + } + + /** + * The significant leading digits of a TM value, i.e. {@code HH}, + * {@code HHmm}, or {@code HHmmss}. Used to build a + * precision-matched comparison against a stored time; the + * fractional second is dropped because XNAT stores no sub-second + * precision. + * + * @param value a TM value already accepted by + * {@link #parseTime(String, String)} + * @return the significant digits, of length 2, 4, or 6 + */ + public static String timeMatchPrefix(String value) { + final String v = stripPadding(value); + final int dot = v.indexOf('.'); + return (dot < 0) ? v : v.substring(0, dot); + } + + // PS3.5 allows trailing SPACE padding on both DA and TM. Leading + // and embedded spaces are not allowed, so only strip the tail. + private static String stripPadding(String value) { + int end = value.length(); + while (end > 0 && value.charAt(end - 1) == ' ') { + end--; + } + return value.substring(0, end); + } + + private static boolean isAllDigits(String s) { + if (s.isEmpty()) { + return false; + } + for (int i = 0; i < s.length(); i++) { + final char c = s.charAt(i); + if (c < '0' || c > '9') { + return false; + } + } + return true; + } + + private static int twoDigitsAt(String s, int index) { + return (s.charAt(index) - '0') * 10 + (s.charAt(index + 1) - '0'); + } + + // Right-pad the fractional digits to nanosecond precision. + private static int nanosOf(String fraction) { + if (fraction.isEmpty()) { + return 0; + } + final StringBuilder b = new StringBuilder(fraction); + while (b.length() < 9) { + b.append('0'); + } + return Integer.parseInt(b.toString()); + } +} diff --git a/src/main/java/org/nrg/xnat/dicomweb/util/DicomQueryValueValidator.java b/src/main/java/org/nrg/xnat/dicomweb/util/DicomQueryValueValidator.java new file mode 100644 index 0000000..4e53e35 --- /dev/null +++ b/src/main/java/org/nrg/xnat/dicomweb/util/DicomQueryValueValidator.java @@ -0,0 +1,114 @@ +/* + * XNAT DICOMweb Plugin + * Copyright (c) 2026 XNATWorks. + * All rights reserved. + */ + +package org.nrg.xnat.dicomweb.util; + +import org.nrg.xnat.dicomweb.exceptions.BadRequestException; + +/** + * Validates QIDO-RS query parameter values for attributes of VR DA and + * TM, rejecting malformed values with HTTP 400. + * + *

PS3.18 §8.3.4.1 defers the acceptable values of a search + * parameter to the C-FIND matching rules: + * + *

+ * The acceptable values are determined by the types of matching + * allowed by C-FIND for its associated attribute. See Section + * C.2.2.2 in PS3.4. + *
+ * + *

PS3.4 §C.2.2.2.1 distinguishes the two matching types that + * apply to a DA or TM value by the presence of a hyphen — Single + * Value Matching applies when the value is "of VR of DA, TM or DT and + * contains a single value with no '-' and no QUOTATION MARK + * characters", and Range Matching (§C.2.2.2.5) otherwise. This + * class applies the same split. + * + *

Wildcard matching is not available for these VRs. + * PS3.4 §C.2.2.2.4 scopes it to "AE, CS, LO, LT, PN, SH, ST, UC, + * UR, UT", and §C.2.2.2 notes that "the wild card characters '*' + * and '?' are not valid for the CS VR but are used for Wild Card + * Matching". A {@code *} or {@code ?} in a date or time parameter is + * therefore rejected, with one tolerance: the bare value {@code "*"} + * is accepted as Universal Matching, per the §C.2.2.2.4 note + * that "Wild Card Matching on a value of '*' is equivalent to + * Universal Matching". Callers treat it as though the parameter were + * absent. + * + *

PS3.18 §10.6.3.1 Table 10.6.3-1 gives the status code for a + * value this class rejects: "400 (Bad Request) — The was a + * problem with the request. For example, the Query Parameter syntax + * is incorrect." + */ +public final class DicomQueryValueValidator { + + /** The bare wildcard, accepted as Universal Matching. */ + private static final String UNIVERSAL = "*"; + + private DicomQueryValueValidator() {} + + /** + * Validate a DA query parameter value. + * + * @param paramName query parameter name, as spelled by the client + * @param value the raw value + * @return true if the value is a filter to apply, false if it is + * Universal Matching and the parameter should be dropped + * @throws BadRequestException if the value is malformed + */ + public static boolean validateDate(String paramName, String value) { + return validate(paramName, value, true); + } + + /** + * Validate a TM query parameter value. + * + * @param paramName query parameter name, as spelled by the client + * @param value the raw value + * @return true if the value is a filter to apply, false if it is + * Universal Matching and the parameter should be dropped + * @throws BadRequestException if the value is malformed + */ + public static boolean validateTime(String paramName, String value) { + return validate(paramName, value, false); + } + + private static boolean validate(String paramName, String value, boolean isDate) { + if (value == null || value.isEmpty() || UNIVERSAL.equals(value)) { + return false; + } + rejectWildcards(paramName, value); + + if (value.indexOf('-') >= 0) { + // Range Matching, PS3.4 §C.2.2.2.5. Endpoints are + // validated by the range parser, which throws on a + // malformed endpoint or a malformed range structure. + if (isDate) { + DicomRangeParser.parseDicomDateRange(value, paramName); + } else { + DicomRangeParser.parseDicomTimeRange(value, paramName); + } + return true; + } + + // Single Value Matching, PS3.4 §C.2.2.2.1. + if (isDate) { + DicomDateTimeValues.parseDate(paramName, value); + } else { + DicomDateTimeValues.parseTime(paramName, value); + } + return true; + } + + private static void rejectWildcards(String paramName, String value) { + if (value.indexOf('*') >= 0 || value.indexOf('?') >= 0) { + throw new BadRequestException(paramName, + "wildcard matching is not defined for dates and times " + + "(PS3.4 C.2.2.2.4); use an exact value or a range"); + } + } +} diff --git a/src/main/java/org/nrg/xnat/dicomweb/util/DicomRangeParser.java b/src/main/java/org/nrg/xnat/dicomweb/util/DicomRangeParser.java index b0280c2..3c8f564 100644 --- a/src/main/java/org/nrg/xnat/dicomweb/util/DicomRangeParser.java +++ b/src/main/java/org/nrg/xnat/dicomweb/util/DicomRangeParser.java @@ -11,9 +11,6 @@ import java.time.LocalDate; import java.time.LocalDateTime; import java.time.LocalTime; -import java.time.format.DateTimeFormatter; -import java.time.format.DateTimeParseException; -import java.time.format.ResolverStyle; import java.util.Optional; /** @@ -27,23 +24,14 @@ * {@code -end}, or the bare universal marker {@code -}. Wildcards * ({@code *}, {@code ?}) are not permitted inside a range endpoint. * - *

Endpoints must be full 8-digit {@code yyyyMMdd} dates or - * 6-digit {@code HHmmss} times. Partial precision and fractional - * seconds are not accepted; a malformed value raises - * {@link BadRequestException} (HTTP 400). + *

Endpoint syntax is that of the underlying VR, as defined in + * PS3.5 §6.2 and implemented by {@link DicomDateTimeValues}: + * dates are a full 8 digits, while times may carry the + * partial-precision and fractional-second forms the TM VR allows. + * A malformed endpoint raises {@link BadRequestException} (HTTP 400). */ public final class DicomRangeParser { - // Strict resolver style catches invalid calendar dates like - // 20200230; it requires the proleptic-year pattern 'uuuu' rather - // than 'yyyy' since the latter needs an era in strict mode. - private static final DateTimeFormatter DA_FORMAT = - DateTimeFormatter.ofPattern("uuuuMMdd") - .withResolverStyle(ResolverStyle.STRICT); - private static final DateTimeFormatter TM_FORMAT = - DateTimeFormatter.ofPattern("HHmmss") - .withResolverStyle(ResolverStyle.STRICT); - private DicomRangeParser() {} /** @@ -57,14 +45,29 @@ private DicomRangeParser() {} * is malformed */ public static Optional parseDicomDateRange(String value) { + return parseDicomDateRange(value, "StudyDate"); + } + + /** + * Parse a DICOM DA range, reporting errors against the parameter + * name the client actually sent. + * + * @param value the raw query value + * @param paramName query parameter name, for the error message + * @return the parsed range, or {@link Optional#empty()} if the + * value is not a range + * @throws BadRequestException if the value looks like a range but + * is malformed + */ + public static Optional parseDicomDateRange(String value, String paramName) { if (value == null || !value.contains("-")) { return Optional.empty(); } - String[] parts = splitRange(value, "StudyDate"); + String[] parts = splitRange(value, paramName); LocalDate start = parts[0].isEmpty() - ? null : parseDate(parts[0], "StudyDate"); + ? null : parseDate(parts[0], paramName); LocalDate end = parts[1].isEmpty() - ? null : parseDate(parts[1], "StudyDate"); + ? null : parseDate(parts[1], paramName); return Optional.of(new DicomDateRange(start, end)); } @@ -78,14 +81,29 @@ public static Optional parseDicomDateRange(String value) { * is malformed */ public static Optional parseDicomTimeRange(String value) { + return parseDicomTimeRange(value, "StudyTime"); + } + + /** + * Parse a DICOM TM range, reporting errors against the parameter + * name the client actually sent. + * + * @param value the raw query value + * @param paramName query parameter name, for the error message + * @return the parsed range, or {@link Optional#empty()} if the + * value is not a range + * @throws BadRequestException if the value looks like a range but + * is malformed + */ + public static Optional parseDicomTimeRange(String value, String paramName) { if (value == null || !value.contains("-")) { return Optional.empty(); } - String[] parts = splitRange(value, "StudyTime"); + String[] parts = splitRange(value, paramName); LocalTime start = parts[0].isEmpty() - ? null : parseTime(parts[0], "StudyTime"); + ? null : parseTime(parts[0], paramName); LocalTime end = parts[1].isEmpty() - ? null : parseTime(parts[1], "StudyTime"); + ? null : parseTime(parts[1], paramName); return Optional.of(new DicomTimeRange(start, end)); } @@ -103,23 +121,11 @@ private static String[] splitRange(String value, String paramName) { } private static LocalDate parseDate(String s, String paramName) { - try { - return LocalDate.parse(s, DA_FORMAT); - } catch (DateTimeParseException e) { - throw new BadRequestException(paramName, - "range endpoint '" + s + "' is not a valid DICOM " - + "date (yyyyMMdd)"); - } + return DicomDateTimeValues.parseDate(paramName, s); } private static LocalTime parseTime(String s, String paramName) { - try { - return LocalTime.parse(s, TM_FORMAT); - } catch (DateTimeParseException e) { - throw new BadRequestException(paramName, - "range endpoint '" + s + "' is not a valid DICOM " - + "time (HHmmss)"); - } + return DicomDateTimeValues.parseTime(paramName, s); } /** diff --git a/src/main/java/org/nrg/xnat/dicomweb/util/QidoQueryParamParser.java b/src/main/java/org/nrg/xnat/dicomweb/util/QidoQueryParamParser.java index 47081bc..fe34dcb 100644 --- a/src/main/java/org/nrg/xnat/dicomweb/util/QidoQueryParamParser.java +++ b/src/main/java/org/nrg/xnat/dicomweb/util/QidoQueryParamParser.java @@ -35,6 +35,13 @@ *

Only the parameters enumerated in the plugin's QIDO-RS * conformance table (CONFORMANCE §6.2) are recognized; anything * else is logged at debug and dropped. + * + *

Values of recognized parameters whose VR constrains their syntax + * are validated here, at the REST boundary, by + * {@link DicomQueryValueValidator}. This is deliberate: it is the one + * point every QIDO-RS endpoint passes through, and it sits outside + * the query-execution error handling, so a malformed value surfaces + * as HTTP 400 rather than being swallowed into an empty result set. */ public final class QidoQueryParamParser { @@ -122,12 +129,31 @@ public static Attributes parse(Map queryParams) { log.debug("Unsupported query parameter: {}", key); continue; } + if (!isApplicableFilter(key, value, info.vr)) { + continue; + } attrs.setString(info.tag, info.vr, value); } log.debug("Parsed {} query parameters into DICOM attributes", attrs.size()); return attrs; } + // Validate values whose VR constrains their syntax, so that a + // malformed date or time is reported as HTTP 400 here rather than + // silently degrading into an empty or unfiltered result set + // downstream. Returns false when the value denotes Universal + // Matching, in which case the parameter is dropped rather than + // carried into the query as a filter. + private static boolean isApplicableFilter(String key, String value, VR vr) { + if (vr == VR.DA) { + return DicomQueryValueValidator.validateDate(key, value); + } + if (vr == VR.TM) { + return DicomQueryValueValidator.validateTime(key, value); + } + return true; + } + // Lower-case and strip parentheses / commas so that the // 8-hex-digit form is matched whether the client emits // "00080020", "(0008,0020)", or "0008,0020". diff --git a/src/test/java/org/nrg/xnat/dicomweb/service/StudyDateTimeFilterSqlTest.java b/src/test/java/org/nrg/xnat/dicomweb/service/StudyDateTimeFilterSqlTest.java new file mode 100644 index 0000000..3936a98 --- /dev/null +++ b/src/test/java/org/nrg/xnat/dicomweb/service/StudyDateTimeFilterSqlTest.java @@ -0,0 +1,186 @@ +/* + * XNAT DICOMweb Plugin + * Copyright (c) 2026 XNATWorks. + * All rights reserved. + */ + +package org.nrg.xnat.dicomweb.service; + +import org.dcm4che3.data.Attributes; +import org.dcm4che3.data.Tag; +import org.dcm4che3.data.VR; +import org.junit.BeforeClass; +import org.junit.Test; +import org.nrg.xnat.dicomweb.service.impl.XnatDicomServiceImpl; +import org.springframework.jdbc.core.namedparam.MapSqlParameterSource; + +import java.lang.reflect.Method; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertTrue; + +/** + * Tests the SQL emitted for StudyDate / StudyTime query parameters by + * {@code XnatDicomServiceImpl.appendStudyDateTimeFilter}. + * + *

Values are written here exactly as {@code QidoQueryParamParser} + * would leave them, i.e. already validated, since that is the only + * way they reach this code. + */ +public class StudyDateTimeFilterSqlTest { + + private static Method appendStudyDateTimeFilter; + + @BeforeClass + public static void resolveMethod() throws Exception { + appendStudyDateTimeFilter = XnatDicomServiceImpl.class.getDeclaredMethod( + "appendStudyDateTimeFilter", + StringBuilder.class, MapSqlParameterSource.class, Attributes.class); + appendStudyDateTimeFilter.setAccessible(true); + } + + private static final class Result { + final String sql; + final MapSqlParameterSource params; + Result(String sql, MapSqlParameterSource params) { + this.sql = sql; + this.params = params; + } + Object value(String name) { + return params.hasValue(name) ? params.getValue(name) : null; + } + } + + private static Result filterFor(String studyDate, String studyTime) throws Exception { + Attributes attrs = new Attributes(); + if (studyDate != null) { + attrs.setString(Tag.StudyDate, VR.DA, studyDate); + } + if (studyTime != null) { + attrs.setString(Tag.StudyTime, VR.TM, studyTime); + } + StringBuilder sql = new StringBuilder(); + MapSqlParameterSource params = new MapSqlParameterSource(); + appendStudyDateTimeFilter.invoke(null, sql, params, attrs); + return new Result(sql.toString(), params); + } + + // ---- Single value matching ---- + + @Test + public void exactDateBindsAnIsoDate() throws Exception { + Result r = filterFor("20250115", null); + assertTrue(r.sql, r.sql.contains("e.date = CAST(:q_study_date AS DATE)")); + assertEquals("2025-01-15", r.value("q_study_date")); + } + + @Test + public void exactDateNoLongerBuildsAnIlikeClause() throws Exception { + // Regression guard for the removed wildcard branch. + Result r = filterFor("20250115", null); + assertFalse(r.sql, r.sql.contains("ILIKE")); + } + + @Test + public void fullPrecisionTimeComparesAllSixDigits() throws Exception { + Result r = filterFor(null, "103000"); + assertTrue(r.sql, r.sql.contains("LEFT(TO_CHAR(e.time, 'HH24MISS'), :q_study_time_digits)")); + assertEquals(6, r.value("q_study_time_digits")); + assertEquals("103000", r.value("q_study_time")); + } + + @Test + public void hourPrecisionTimeComparesTwoDigits() throws Exception { + Result r = filterFor(null, "10"); + assertEquals(2, r.value("q_study_time_digits")); + assertEquals("10", r.value("q_study_time")); + } + + @Test + public void minutePrecisionTimeComparesFourDigits() throws Exception { + Result r = filterFor(null, "1030"); + assertEquals(4, r.value("q_study_time_digits")); + assertEquals("1030", r.value("q_study_time")); + } + + @Test + public void fractionalSecondIsIgnoredForSingleValueMatching() throws Exception { + // XNAT stores no sub-second precision. + Result r = filterFor(null, "103000.500000"); + assertEquals(6, r.value("q_study_time_digits")); + assertEquals("103000", r.value("q_study_time")); + } + + // ---- Range matching ---- + + @Test + public void closedDateRangeBindsBothBounds() throws Exception { + Result r = filterFor("20250101-20250131", null); + assertTrue(r.sql, r.sql.contains("e.date >= CAST(:q_study_date_start AS DATE)")); + assertTrue(r.sql, r.sql.contains("e.date <= CAST(:q_study_date_end AS DATE)")); + assertEquals("2025-01-01", r.value("q_study_date_start")); + assertEquals("2025-01-31", r.value("q_study_date_end")); + } + + @Test + public void openUpperDateRangeBindsOnlyStart() throws Exception { + Result r = filterFor("20250101-", null); + assertTrue(r.sql, r.sql.contains("e.date >= CAST(:q_study_date_start AS DATE)")); + assertFalse(r.sql, r.sql.contains("q_study_date_end")); + assertNull(r.value("q_study_date_end")); + } + + @Test + public void universalRangeMarkerEmitsNoClause() throws Exception { + Result r = filterFor("-", null); + assertEquals("", r.sql); + } + + @Test + public void partialPrecisionTimeRangeResolvesToWholeMinutes() throws Exception { + Result r = filterFor(null, "1000-1800"); + assertEquals("10:00", r.value("q_study_time_start")); + assertEquals("18:00", r.value("q_study_time_end")); + } + + // ---- Combined DA + TM, PS3.18 §8.3.4.1.1 → PS3.4 §C.2.2.2.5.4 ---- + + @Test + public void matchingRangeFormsCollapseToASingleTimestampClause() throws Exception { + // The worked example from the PS3.4 §C.2.2.2.5.4 Note. + Result r = filterFor("20060705-20060707", "1000-1800"); + assertTrue(r.sql, r.sql.contains("(e.date + e.time) >= CAST(:q_study_dt_start AS TIMESTAMP)")); + assertTrue(r.sql, r.sql.contains("(e.date + e.time) <= CAST(:q_study_dt_end AS TIMESTAMP)")); + assertEquals("2006-07-05T10:00", r.value("q_study_dt_start")); + assertEquals("2006-07-07T18:00", r.value("q_study_dt_end")); + // The independent date/time clauses must not also be emitted. + assertFalse(r.sql, r.sql.contains("q_study_date_start")); + assertFalse(r.sql, r.sql.contains("q_study_time_start")); + } + + @Test + public void mismatchedRangeFormsFallBackToIndependentClauses() throws Exception { + Result r = filterFor("20250101-20250131", "1000-"); + assertFalse(r.sql, r.sql.contains("e.date + e.time")); + assertEquals("2025-01-01", r.value("q_study_date_start")); + assertEquals("10:00", r.value("q_study_time_start")); + } + + @Test + public void dateRangeWithExactTimeStaysIndependent() throws Exception { + Result r = filterFor("20250101-20250131", "103000"); + assertFalse(r.sql, r.sql.contains("e.date + e.time")); + assertEquals("2025-01-01", r.value("q_study_date_start")); + assertEquals("103000", r.value("q_study_time")); + } + + // ---- Absent parameters ---- + + @Test + public void noDateOrTimeEmitsNothing() throws Exception { + Result r = filterFor(null, null); + assertEquals("", r.sql); + } +} diff --git a/src/test/java/org/nrg/xnat/dicomweb/util/DicomQueryValueValidatorTest.java b/src/test/java/org/nrg/xnat/dicomweb/util/DicomQueryValueValidatorTest.java new file mode 100644 index 0000000..bfe7437 --- /dev/null +++ b/src/test/java/org/nrg/xnat/dicomweb/util/DicomQueryValueValidatorTest.java @@ -0,0 +1,249 @@ +/* + * XNAT DICOMweb Plugin + * Copyright (c) 2026 XNATWorks. + * All rights reserved. + */ + +package org.nrg.xnat.dicomweb.util; + +import org.junit.Test; +import org.nrg.xnat.dicomweb.exceptions.BadRequestException; + +import java.time.LocalDate; +import java.time.LocalTime; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; + +/** + * Unit tests for {@link DicomQueryValueValidator} and the DA/TM value + * grammar in {@link DicomDateTimeValues}. + * + *

These cover the QIDO-RS regression in which a malformed date + * parameter produced an HTTP 200 with an empty result set instead of + * the HTTP 400 required by PS3.18 §10.6.3.1. + */ +public class DicomQueryValueValidatorTest { + + // ---- Dates: valid values are accepted as filters ---- + + @Test + public void exactDateAccepted() { + assertTrue(DicomQueryValueValidator.validateDate("StudyDate", "20250115")); + } + + @Test + public void dateRangesAccepted() { + assertTrue(DicomQueryValueValidator.validateDate("StudyDate", "20250101-20250131")); + assertTrue(DicomQueryValueValidator.validateDate("StudyDate", "20250101-")); + assertTrue(DicomQueryValueValidator.validateDate("StudyDate", "-20250131")); + } + + @Test + public void trailingSpacePaddingAccepted() { + // PS3.5 §6.2 DA: "a trailing SPACE character is allowed for padding". + assertTrue(DicomQueryValueValidator.validateDate("StudyDate", "20250115 ")); + } + + @Test + public void leapDayAccepted() { + assertTrue(DicomQueryValueValidator.validateDate("StudyDate", "20240229")); + } + + // ---- Dates: universal matching drops the filter ---- + + @Test + public void emptyDateIsUniversal() { + assertFalse(DicomQueryValueValidator.validateDate("StudyDate", "")); + assertFalse(DicomQueryValueValidator.validateDate("StudyDate", null)); + } + + @Test + public void bareAsteriskIsUniversal() { + // PS3.4 §C.2.2.2.4 Note: "Wild Card Matching on a value of '*' + // is equivalent to Universal Matching." + assertFalse(DicomQueryValueValidator.validateDate("StudyDate", "*")); + assertFalse(DicomQueryValueValidator.validateTime("StudyTime", "*")); + } + + // ---- Dates: malformed values are rejected ---- + + @Test(expected = BadRequestException.class) + public void nonNumericDateRejected() { + DicomQueryValueValidator.validateDate("StudyDate", "yesterday"); + } + + @Test(expected = BadRequestException.class) + public void impossibleCalendarDateRejected() { + // The originally reported case: 8 digits, so it bypassed range + // parsing, was sliced into "2025-13-45", and blew up in the + // database with the exception swallowed. + DicomQueryValueValidator.validateDate("StudyDate", "20251345"); + } + + @Test(expected = BadRequestException.class) + public void nonLeapYearFeb29Rejected() { + DicomQueryValueValidator.validateDate("StudyDate", "20250229"); + } + + @Test(expected = BadRequestException.class) + public void isoFormattedDateRejected() { + // Splits on "-" into three parts, so it fails as a range too. + DicomQueryValueValidator.validateDate("StudyDate", "2025-01-15"); + } + + @Test(expected = BadRequestException.class) + public void shortDateRejected() { + DicomQueryValueValidator.validateDate("StudyDate", "202501"); + } + + @Test(expected = BadRequestException.class) + public void malformedRangeEndpointRejected() { + DicomQueryValueValidator.validateDate("StudyDate", "20250101-garbage"); + } + + @Test(expected = BadRequestException.class) + public void threePartRangeRejected() { + DicomQueryValueValidator.validateDate("StudyDate", "20250101-20250201-20250301"); + } + + // ---- Wildcards are not defined for DA/TM ---- + // PS3.4 §C.2.2.2.4 scopes Wild Card Matching to + // "AE, CS, LO, LT, PN, SH, ST, UC, UR, UT". + + @Test(expected = BadRequestException.class) + public void partialDateWildcardRejected() { + DicomQueryValueValidator.validateDate("StudyDate", "2025*"); + } + + @Test(expected = BadRequestException.class) + public void dateQuestionMarkWildcardRejected() { + DicomQueryValueValidator.validateDate("StudyDate", "202501??"); + } + + @Test(expected = BadRequestException.class) + public void timeWildcardRejected() { + DicomQueryValueValidator.validateTime("StudyTime", "12*"); + } + + // ---- Times: the TM grammar of PS3.5 §6.2 ---- + + @Test + public void fullPrecisionTimeAccepted() { + assertTrue(DicomQueryValueValidator.validateTime("StudyTime", "103000")); + } + + @Test + public void partialPrecisionTimesAccepted() { + assertTrue(DicomQueryValueValidator.validateTime("StudyTime", "10")); + assertTrue(DicomQueryValueValidator.validateTime("StudyTime", "1030")); + } + + @Test + public void fractionalSecondAccepted() { + assertTrue(DicomQueryValueValidator.validateTime("StudyTime", "103000.500000")); + } + + @Test(expected = BadRequestException.class) + public void oddDigitCountTimeRejected() { + // PS3.5 §6.2 cites "021 " as an invalid TM value. + DicomQueryValueValidator.validateTime("StudyTime", "021"); + } + + @Test(expected = BadRequestException.class) + public void hourOutOfRangeRejected() { + DicomQueryValueValidator.validateTime("StudyTime", "250000"); + } + + @Test(expected = BadRequestException.class) + public void minuteOutOfRangeRejected() { + DicomQueryValueValidator.validateTime("StudyTime", "106000"); + } + + @Test(expected = BadRequestException.class) + public void nonNumericTimeRejected() { + DicomQueryValueValidator.validateTime("StudyTime", "noon"); + } + + @Test(expected = BadRequestException.class) + public void colonSeparatedTimeRejected() { + // The ACR-NEMA HH:MM:SS form PS3.5 calls non-compliant. + DicomQueryValueValidator.validateTime("StudyTime", "10:30:00"); + } + + // ---- Value parsing detail ---- + + @Test + public void unspecifiedTimeComponentsResolveToZero() { + assertEquals(LocalTime.of(10, 0, 0), + DicomDateTimeValues.parseTime("StudyTime", "10")); + assertEquals(LocalTime.of(10, 30, 0), + DicomDateTimeValues.parseTime("StudyTime", "1030")); + } + + @Test + public void leapSecondClampsToEndOfMinute() { + assertEquals(LocalTime.of(23, 59, 59, 999_999_999), + DicomDateTimeValues.parseTime("StudyTime", "235960")); + } + + @Test + public void fractionIsRightPaddedToNanos() { + assertEquals(LocalTime.of(10, 30, 0, 500_000_000), + DicomDateTimeValues.parseTime("StudyTime", "103000.5")); + } + + @Test + public void dateParsesToExpectedValue() { + assertEquals(LocalDate.of(2025, 1, 15), + DicomDateTimeValues.parseDate("StudyDate", "20250115")); + } + + @Test + public void timeMatchPrefixReflectsSuppliedComponents() { + assertEquals("10", DicomDateTimeValues.timeMatchPrefix("10")); + assertEquals("1030", DicomDateTimeValues.timeMatchPrefix("1030")); + assertEquals("103000", DicomDateTimeValues.timeMatchPrefix("103000")); + assertEquals("103000", DicomDateTimeValues.timeMatchPrefix("103000.5")); + assertEquals("103000", DicomDateTimeValues.timeMatchPrefix("103000 ")); + } + + // ---- The error carries a 400 and names the parameter ---- + + @Test + public void rejectionReportsBadRequestNamingTheParameter() { + try { + DicomQueryValueValidator.validateDate("StudyDate", "20251345"); + throw new AssertionError("expected BadRequestException"); + } catch (BadRequestException e) { + assertEquals(400, e.getHttpStatus()); + assertTrue("message should name the parameter: " + e.getMessage(), + e.getMessage().contains("StudyDate")); + } + } + + @Test + public void rejectionUsesTheClientsSpellingOfTheParameter() { + // Clients may send the 8-hex-digit tag form instead of the + // keyword; the error should echo what they sent. + try { + DicomQueryValueValidator.validateDate("00080020", "20251345"); + throw new AssertionError("expected BadRequestException"); + } catch (BadRequestException e) { + assertTrue("message should name the parameter: " + e.getMessage(), + e.getMessage().contains("00080020")); + } + } + + @Test + public void rangeRejectionAlsoUsesTheClientsSpelling() { + try { + DicomQueryValueValidator.validateDate("00080020", "20250101-nonsense"); + throw new AssertionError("expected BadRequestException"); + } catch (BadRequestException e) { + assertTrue("message should name the parameter: " + e.getMessage(), + e.getMessage().contains("00080020")); + } + } +} diff --git a/src/test/java/org/nrg/xnat/dicomweb/util/DicomRangeParserTest.java b/src/test/java/org/nrg/xnat/dicomweb/util/DicomRangeParserTest.java index 53d0ac0..868979a 100644 --- a/src/test/java/org/nrg/xnat/dicomweb/util/DicomRangeParserTest.java +++ b/src/test/java/org/nrg/xnat/dicomweb/util/DicomRangeParserTest.java @@ -194,14 +194,41 @@ public void universalTimeRange() { // ---- Time parsing: malformed → 400 ---- - @Test(expected = BadRequestException.class) - public void nonSixDigitTimeStartRejected() { - DicomRangeParser.parseDicomTimeRange("0800-180000"); + // PS3.5 §6.2 TM: "One or more of the components MM, SS, or FFFFFF + // may be unspecified as long as every component to the right of an + // unspecified component is also unspecified". Partial-precision + // endpoints are therefore valid, and unspecified components + // resolve to zero. + + @Test + public void partialPrecisionHourMinuteEndpointsAccepted() { + DicomTimeRange r = DicomRangeParser.parseDicomTimeRange( + "0800-180000").get(); + assertEquals(LocalTime.of(8, 0, 0), r.start); + assertEquals(LocalTime.of(18, 0, 0), r.end); + } + + @Test + public void partialPrecisionHourOnlyEndpointAccepted() { + DicomTimeRange r = DicomRangeParser.parseDicomTimeRange( + "080000-18").get(); + assertEquals(LocalTime.of(8, 0, 0), r.start); + assertEquals(LocalTime.of(18, 0, 0), r.end); + } + + @Test + public void ps3_4NoteExampleTimeRangeParses() { + // The Study Time from the PS3.4 §C.2.2.2.5.4 worked example. + DicomTimeRange r = DicomRangeParser.parseDicomTimeRange( + "1000-1800").get(); + assertEquals(LocalTime.of(10, 0), r.start); + assertEquals(LocalTime.of(18, 0), r.end); } @Test(expected = BadRequestException.class) - public void nonSixDigitTimeEndRejected() { - DicomRangeParser.parseDicomTimeRange("080000-18"); + public void oddDigitCountTimeRejected() { + // PS3.5 §6.2 cites "021 " as an invalid TM value. + DicomRangeParser.parseDicomTimeRange("021-180000"); } @Test(expected = BadRequestException.class) @@ -219,11 +246,33 @@ public void wildcardInTimeRangeRejected() { DicomRangeParser.parseDicomTimeRange("08????-180000"); } + @Test + public void fractionalSecondsInTimeRangeAccepted() { + // PS3.5 §6.2 TM: "The FFFFFF component, if present, shall + // contain 1 to 6 digits." + DicomTimeRange r = DicomRangeParser.parseDicomTimeRange( + "080000.5-180000").get(); + assertEquals(LocalTime.of(8, 0, 0, 500_000_000), r.start); + assertEquals(LocalTime.of(18, 0, 0), r.end); + } + @Test(expected = BadRequestException.class) - public void fractionalSecondsInTimeRangeRejected() { - // Not in scope for this fix — matches series/instance-level - // exact-form expectations at the study level. - DicomRangeParser.parseDicomTimeRange("080000.500000-180000"); + public void fractionWithoutSecondsRejected() { + DicomRangeParser.parseDicomTimeRange("0800.5-180000"); + } + + @Test(expected = BadRequestException.class) + public void overlongFractionRejected() { + DicomRangeParser.parseDicomTimeRange("080000.1234567-180000"); + } + + @Test + public void leapSecondEndpointAccepted() { + // PS3.5 §6.2 TM: SS "range '00' - '60'"; the SS component + // "may have a Value of 60 only for a leap second". + DicomTimeRange r = DicomRangeParser.parseDicomTimeRange( + "080000-235960").get(); + assertEquals(LocalTime.of(23, 59, 59, 999_999_999), r.end); } // ---- Combined DA + TM into DT range ---- diff --git a/src/test/java/org/nrg/xnat/dicomweb/util/QidoQueryParamParserTest.java b/src/test/java/org/nrg/xnat/dicomweb/util/QidoQueryParamParserTest.java index db1c14f..3879bce 100644 --- a/src/test/java/org/nrg/xnat/dicomweb/util/QidoQueryParamParserTest.java +++ b/src/test/java/org/nrg/xnat/dicomweb/util/QidoQueryParamParserTest.java @@ -9,6 +9,7 @@ import org.dcm4che3.data.Attributes; import org.dcm4che3.data.Tag; import org.junit.Test; +import org.nrg.xnat.dicomweb.exceptions.BadRequestException; import java.util.Collections; import java.util.HashMap; @@ -249,4 +250,66 @@ public void unrelatedKeywordDoesNotShadowStudyDate() { Attributes attrs = QidoQueryParamParser.parse(q); assertFalse(attrs.contains(Tag.StudyDate)); } + + // ---- Date and time validation at the REST boundary ---- + // Malformed DA/TM values must fail here, where the exception can + // reach GlobalExceptionHandler, rather than downstream inside the + // query-execution try/catch where it was previously swallowed + // into an HTTP 200 with an empty result set. + + @Test(expected = BadRequestException.class) + public void malformedStudyDateRejected() { + Map q = new HashMap<>(); + q.put("StudyDate", "20251345"); + QidoQueryParamParser.parse(q); + } + + @Test(expected = BadRequestException.class) + public void malformedStudyDateRangeRejected() { + Map q = new HashMap<>(); + q.put("StudyDate", "20250101-nonsense"); + QidoQueryParamParser.parse(q); + } + + @Test(expected = BadRequestException.class) + public void malformedStudyTimeRejected() { + Map q = new HashMap<>(); + q.put("StudyTime", "250000"); + QidoQueryParamParser.parse(q); + } + + @Test(expected = BadRequestException.class) + public void malformedStudyDateInHexFormRejected() { + // Validation keys off the VR, so it applies to the tag + // spelling as well as the keyword spelling. + Map q = new HashMap<>(); + q.put("00080020", "not-a-date"); + QidoQueryParamParser.parse(q); + } + + @Test + public void validStudyTimePrecisionsPreserved() { + Map q = new HashMap<>(); + q.put("StudyTime", "1030"); + Attributes attrs = QidoQueryParamParser.parse(q); + assertEquals("1030", attrs.getString(Tag.StudyTime)); + } + + @Test + public void bareAsteriskDateIsDroppedAsUniversalMatch() { + Map q = new HashMap<>(); + q.put("StudyDate", "*"); + Attributes attrs = QidoQueryParamParser.parse(q); + assertFalse(attrs.contains(Tag.StudyDate)); + } + + @Test + public void wildcardsStillAcceptedOnStringVrs() { + // PS3.4 §C.2.2.2.4 does define Wild Card Matching for PN, so + // rejecting it for DA/TM must not disturb these. + Map q = new HashMap<>(); + q.put("PatientName", "Doe*"); + Attributes attrs = QidoQueryParamParser.parse(q); + assertEquals("Doe*", attrs.getString(Tag.PatientName)); + } }