GL-7: Handle invalid requests with proper REST exceptions#960
Open
GL-7: Handle invalid requests with proper REST exceptions#960
Conversation
|
Formatting check succeeded! |
375ae42 to
3df964f
Compare
Establishes architectural rules for module boundaries: - Core modules (cqf-fhir-cr, cqf-fhir-cql, cqf-fhir-utility) must not import REST exceptions from ca.uhn.fhir.rest.server.exceptions - Exception translation belongs at the boundary (cqf-fhir-cr-hapi) - Includes correct/incorrect patterns and ArchUnit enforcement test Co-Authored-By: Claude Opus 4.6 <[email protected]>
Add CrExceptionTranslator utility that catches domain exceptions from core modules and translates them to HAPI FHIR REST exceptions at the boundary layer. IllegalArgumentException becomes InvalidRequestException (HTTP 400) and UnsupportedOperationException becomes NotImplementedOperationException (HTTP 501). IllegalStateException is intentionally not caught as HAPI's RestfulServer already wraps it as InternalErrorException (HTTP 500). Applied to all 51 @Operation-annotated provider methods across both R4 and DSTU3 packages using CrExceptionTranslator.execute() wrapper. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Remove unused executeVoid method and its tests from CrExceptionTranslator. All 51 provider methods use execute() with Supplier — no provider has a void return type requiring executeVoid. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Existing code has legacy violations of the REST exception import rule. Add note acknowledging these as tech debt to be migrated over time, not blockers for new contributions. Co-Authored-By: Claude Opus 4.6 <[email protected]>
3df964f to
587aa09
Compare
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.


Summary
This change replaces raw Java exceptions (
IllegalArgumentException,IllegalStateException,RuntimeException) with proper HAPI FHIR REST exceptions across the CQIS and DTR REST API code paths incdr-cr. Without this fix, invalid client requests and recoverable error conditions result in generic HTTP 500 Internal Server Error responses instead of the semantically correct HTTP 400 Bad Request or HTTP 404 Not Found responses with structured OperationOutcome bodies.InvalidRequestException(HTTP 400) instead ofIllegalArgumentExceptionacrosscdr-persistence-dqmandcdr-prior-auth-dtr.ResourceNotFoundException(HTTP 404) instead ofIllegalArgumentException.R4BundleHelper,SubjectResultsAggregator, andR4MeasureValidatorscoring-code checks — were evaluated and correctly left asIllegalArgumentExceptionor updated toInvalidRequestExceptiononly where client data can trigger them.R4MeasureValidatorwas corrected fromInternalErrorException(HTTP 500) toInvalidRequestException(HTTP 400) — a Measure with an invalidscoringCodeis a bad request, not a server fault.RestExceptionReproductionTestincdr-prior-auth-dtrverifies the DTR$next-questionand$questionnaire-packageoperations surfaceInvalidRequestExceptionfor null/missing inputs.Code Review Suggestions
IllegalArgumentException— Confirm the call graph: if any path from a REST operation can reachgetBundlesFromParameters,getBatchJobId, orgetBundleFirstMeasureReportStatuswith user-suppliedParameters, these should beInvalidRequestException.InvalidRequestExceptionsites inSubjectResultsAggregatorwere reverted toIllegalArgumentExceptionon the grounds they are internal state guards. Verify this is safe.InternalErrorExceptiontoInvalidRequestException— ThescoringCodenull check and invalid-code check changed from 500 to 400. Verify the Measure resource is always user-supplied data.QA Test Suggestions
Async $evaluate-measure — invalid parameters
$evaluate-measure(async) withreportType=badvalue. Confirm HTTP 400 OperationOutcome (not HTTP 500).subjectparameter that omits the resource type. Confirm HTTP 400 OperationOutcome.reporterreference to a non-existent Organization ID. Confirm HTTP 404 OperationOutcome.QPP $qpp-build — invalid inputs
$qpp-buildwith an unrecognizedqualityProgramparameter value. Confirm HTTP 400 OperationOutcome.DTR $next-question — null/missing inputs
$next-questionwith null/empty body. Confirm HTTP 400 OperationOutcome.questionnaireproperty. Confirm HTTP 400.DTR $questionnaire-package — feature disabled
questionnaire-packagedisabled, POST$questionnaire-package. Confirm HTTP 400 (not HTTP 500).Regression — valid requests still succeed
$evaluate-measurerequest completes successfully end-to-end.