Added validation of payloads against JSON schema in PatientRecord validation and upload endpoints#42
Added validation of payloads against JSON schema in PatientRecord validation and upload endpoints#42lucienclin wants to merge 9 commits into
Conversation
…aseController fix: Added validation of payloads against JSON schema in PatientRecord validation and upload endpoints fix: bump sbt version fix: Pinned Jackson version to avoid runtime version conflicts
|
Warning Review limit reached
Next review available in: 40 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughJSON schema serving and validation move into shared controller traits. ChangesJSON Schema Serving and Validation Refactor
Sequence Diagram(s)sequenceDiagram
participant Client
participant UseCaseRouter
participant UseCaseController
participant SchemaValidatedJsonBody
Client->>UseCaseRouter: GET /etl/patient-record/schema?version=...
UseCaseRouter->>UseCaseController: jsonSchema(version)
UseCaseController->>UseCaseController: formattedSchemata.get(version)
UseCaseController-->>Client: Ok(schema) or NotFound
Client->>UseCaseRouter: POST /etl/patient-record
UseCaseRouter->>UseCaseController: patientRecordParser
UseCaseController->>SchemaValidatedJsonBody: parse request body
SchemaValidatedJsonBody-->>UseCaseController: validated DataUpload
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/controllers/de/dnpm/dip/rest/api/UseCaseRouter.scala (1)
81-85: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove commented-out legacy schema code in router.
These blocks are now obsolete after delegating to
controller.jsonSchema(version)and should be deleted to keep route logic unambiguous.Suggested cleanup
-/* - protected val APPLICATION_JSON = "application/json" - - protected val jsonSchemas: Map[String,Eval[JsObject]] -*/ @@ -/* - controller.Action { req => - jsonSchemas.get(version.getOrElse("draft-12").toLowerCase) match { - case Some(sch) => - Ok(Json.prettyPrint(sch.value)).as(APPLICATION_JSON) - - case None => - NotFound( - Json.toJson(Outcome(s"Invalid JSON schema version, expected one of {${jsonSchemas.keys.mkString(",")}}")) - ) - } - } -*/Also applies to: 98-110
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/controllers/de/dnpm/dip/rest/api/UseCaseRouter.scala` around lines 81 - 85, Delete the commented-out legacy schema code blocks including the APPLICATION_JSON constant and jsonSchemas property definitions in the UseCaseRouter. Since the router now delegates to controller.jsonSchema(version) for schema handling, these legacy commented blocks are obsolete and should be removed entirely to keep the route logic clear and unambiguous. Also remove any other similar commented-out schema-related code blocks that are no longer referenced.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/util/de/dnpm/dip/rest/util/JsonOps.scala`:
- Around line 69-80: The call to objectMapper.readTree(json) at line 69 can
throw a JsonParseException if the JSON is malformed, but this exception is not
caught and bubbles up causing a 500 error. Wrap the objectMapper.readTree(json)
call with Either.catchNonFatal to capture any parsing exceptions, and handle the
Left case by returning a BadRequest response with an appropriate error message,
similar to how validation errors are already being handled in the Some(errors)
branch. This ensures malformed JSON returns a proper 400 response instead of a
500 error.
---
Nitpick comments:
In `@app/controllers/de/dnpm/dip/rest/api/UseCaseRouter.scala`:
- Around line 81-85: Delete the commented-out legacy schema code blocks
including the APPLICATION_JSON constant and jsonSchemas property definitions in
the UseCaseRouter. Since the router now delegates to
controller.jsonSchema(version) for schema handling, these legacy commented
blocks are obsolete and should be removed entirely to keep the route logic clear
and unambiguous. Also remove any other similar commented-out schema-related code
blocks that are no longer referenced.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e1552e31-63a8-4465-a676-e042844e55e9
📒 Files selected for processing (9)
app/controllers/de/dnpm/dip/rest/api/MTBController.scalaapp/controllers/de/dnpm/dip/rest/api/MTBRouter.scalaapp/controllers/de/dnpm/dip/rest/api/RDController.scalaapp/controllers/de/dnpm/dip/rest/api/RDRouter.scalaapp/controllers/de/dnpm/dip/rest/api/UseCaseController.scalaapp/controllers/de/dnpm/dip/rest/api/UseCaseRouter.scalaapp/util/de/dnpm/dip/rest/util/JsonOps.scalabuild.sbtproject/build.properties
💤 Files with no reviewable changes (2)
- app/controllers/de/dnpm/dip/rest/api/RDRouter.scala
- app/controllers/de/dnpm/dip/rest/api/MTBRouter.scala
…ion, to keep details of schema library etc on the client-side
…d JSON schema out from UseCaseController into JsonSchemata test: Added tests for Schema Validator
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/util/de/dnpm/dip/rest/util/JsonOps.scala (2)
74-81: 🗄️ Data Integrity & Integration | 🟠 MajorUse a strict UTF-8 decoder here
The
ByteString.decodeString("UTF-8")implementation relies on the standard Java String constructor, which replaces malformed UTF-8 sequences with the replacement character (\uFFFD) instead of throwing an exception. This silently corrupts the payload before validation.+import java.nio.charset.{CodingErrorAction, StandardCharsets} + ... - Either.catchNonFatal(bs.decodeString("UTF-8")) - .leftMap(t => BadRequest(Json.toJson(Outcome("Malformed body: JSON content is expected to be decodable as UTF-8")))) + Either + .catchNonFatal { + StandardCharsets.UTF_8 + .newDecoder() + .onMalformedInput(CodingErrorAction.REPORT) + .onUnmappableCharacter(CodingErrorAction.REPORT) + .decode(bs.asByteBuffer) + .toString + } + .leftMap(_ => BadRequest(Json.toJson(Outcome("Malformed body: JSON content is expected to be decodable as UTF-8"))))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/util/de/dnpm/dip/rest/util/JsonOps.scala` around lines 74 - 81, The UTF-8 check in JsonOps.validate is using ByteString.decodeString("UTF-8"), which can silently replace malformed bytes instead of failing. Update the decode path in the ByteString parser/validate flow to use a strict UTF-8 decoder so invalid sequences are rejected and mapped to the existing BadRequest Outcome, while keeping the current JsonOps parsing behavior otherwise unchanged.
91-94: 🩺 Stability & Availability | 🟠 MajorWrap
Json.parseto handle parsing exceptions safely.The
Json.parse(jsonString)call at line 92 can throw an unhandled exception if the input is malformed (e.g., trailing tokens or syntax errors) but passes the schema validator. This exception bypasses theEitherflow, resulting in a 500 Internal Server Error instead of the expected 400 Bad Request.case None => - Json.parse(jsonString).validate[T] - .asEither - .leftMap(errors => BadRequest(Json.toJson(Outcome(errors)))) + Either + .catchNonFatal(Json.parse(jsonString)) + .leftMap(_ => BadRequest(Json.toJson(Outcome("Malformed body: Content is not valid JSON")))) + .flatMap( + _.validate[T] + .asEither + .leftMap(errors => BadRequest(Json.toJson(Outcome(errors)))) + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/util/de/dnpm/dip/rest/util/JsonOps.scala` around lines 91 - 94, `JsonOps` currently calls `Json.parse(jsonString)` directly in the `case None` branch, which can throw before the `Either`/validation flow handles it. Wrap the parse step in safe exception handling inside this branch so malformed JSON returns a `BadRequest` like the existing validation failure path, and keep the fix localized to the `Json.parse` → `validate[T]` chain in `JsonOps`.
🧹 Nitpick comments (2)
app/controllers/de/dnpm/dip/rest/api/UseCaseController.scala (1)
161-166: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant abstract
formattedSchematadeclaration.
formattedSchematais already declared (with the same signature and doc comment) in theJsonSchematatrait, which is required via the self-type at Line 142. Re-declaring it here is redundant and risks the two doc comments drifting apart. Consider dropping this declaration and relying on the one fromJsonSchemata.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/controllers/de/dnpm/dip/rest/api/UseCaseController.scala` around lines 161 - 166, Remove the redundant abstract formattedSchemata declaration from UseCaseController and rely on the one already provided by the JsonSchemata trait via the self-type; keep the trait as the single source of truth for this member so the duplicated signature and doc comment are eliminated.app/controllers/de/dnpm/dip/rest/api/RDController.scala (1)
106-122: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the stale commented-out schema block.
This block is now fully superseded by
RDJsonSchemata(mixed in at Line 62), and it referencesformattedJsonSchemata, which doesn't match the actual member nameformattedSchemata— so it's both dead and misleading if anyone tries to re-enable it. Deleting it (along with the related commented imports) keeps the controller clean.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/controllers/de/dnpm/dip/rest/api/RDController.scala` around lines 106 - 122, Remove the stale commented-out schema block from RDController and delete the related commented imports inside it. The old override of formattedJsonSchemata is dead code, is superseded by RDJsonSchemata, and refers to the wrong member name instead of formattedSchemata; clean it out so the controller only contains the active schema implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/controllers/de/dnpm/dip/rest/api/JsonSchemata.scala`:
- Around line 39-44: The JsonSchemata contract currently hardcodes the
"draft-12" lookup in schema, which can fail later with an uncaught missing-key
error; update JsonSchemata and the formattedSchemata contract so the canonical
schema key is enforced explicitly, and make schema/schemaValidator fail fast
with a clear message if draft-12 is absent instead of relying on a runtime
assumption.
---
Outside diff comments:
In `@app/util/de/dnpm/dip/rest/util/JsonOps.scala`:
- Around line 74-81: The UTF-8 check in JsonOps.validate is using
ByteString.decodeString("UTF-8"), which can silently replace malformed bytes
instead of failing. Update the decode path in the ByteString parser/validate
flow to use a strict UTF-8 decoder so invalid sequences are rejected and mapped
to the existing BadRequest Outcome, while keeping the current JsonOps parsing
behavior otherwise unchanged.
- Around line 91-94: `JsonOps` currently calls `Json.parse(jsonString)` directly
in the `case None` branch, which can throw before the `Either`/validation flow
handles it. Wrap the parse step in safe exception handling inside this branch so
malformed JSON returns a `BadRequest` like the existing validation failure path,
and keep the fix localized to the `Json.parse` → `validate[T]` chain in
`JsonOps`.
---
Nitpick comments:
In `@app/controllers/de/dnpm/dip/rest/api/RDController.scala`:
- Around line 106-122: Remove the stale commented-out schema block from
RDController and delete the related commented imports inside it. The old
override of formattedJsonSchemata is dead code, is superseded by RDJsonSchemata,
and refers to the wrong member name instead of formattedSchemata; clean it out
so the controller only contains the active schema implementation.
In `@app/controllers/de/dnpm/dip/rest/api/UseCaseController.scala`:
- Around line 161-166: Remove the redundant abstract formattedSchemata
declaration from UseCaseController and rely on the one already provided by the
JsonSchemata trait via the self-type; keep the trait as the single source of
truth for this member so the duplicated signature and doc comment are
eliminated.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e2985cac-9f16-4af7-8e9c-e5b91160407a
📒 Files selected for processing (7)
app/controllers/de/dnpm/dip/rest/api/JsonSchemata.scalaapp/controllers/de/dnpm/dip/rest/api/MTBController.scalaapp/controllers/de/dnpm/dip/rest/api/MTBJsonSchemata.scalaapp/controllers/de/dnpm/dip/rest/api/RDController.scalaapp/controllers/de/dnpm/dip/rest/api/RDJsonSchemata.scalaapp/controllers/de/dnpm/dip/rest/api/UseCaseController.scalaapp/util/de/dnpm/dip/rest/util/JsonOps.scala
| lazy val schema = | ||
| JsonSchemaFactory | ||
| .getInstance(SpecVersion.VersionFlag.V202012) | ||
| .getSchema( | ||
| formattedSchemata("draft-12").value | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP -C2 'formattedSchemata\s*[:=]' --type=scalaRepository: dnpm-dip/api-gateway
Length of output: 158
Hardcoded "draft-12" key makes the schema contract a runtime assumption.
formattedSchemata("draft-12") will throw NoSuchElementException if an implementing trait ever omits the draft-12 entry. Because schema and schemaValidator are both lazy, this surfaces only on the first validating request as an uncaught 500 rather than at startup/compile time. Consider failing fast with a clear message (or making the canonical key part of the trait contract).
🛡️ Possible hardening
- lazy val schema =
- JsonSchemaFactory
- .getInstance(SpecVersion.VersionFlag.V202012)
- .getSchema(
- formattedSchemata("draft-12").value
- )
+ lazy val schema =
+ JsonSchemaFactory
+ .getInstance(SpecVersion.VersionFlag.V202012)
+ .getSchema(
+ formattedSchemata
+ .getOrElse("draft-12", sys.error("Missing required 'draft-12' JSON schema in formattedSchemata"))
+ .value
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| lazy val schema = | |
| JsonSchemaFactory | |
| .getInstance(SpecVersion.VersionFlag.V202012) | |
| .getSchema( | |
| formattedSchemata("draft-12").value | |
| ) | |
| lazy val schema = | |
| JsonSchemaFactory | |
| .getInstance(SpecVersion.VersionFlag.V202012) | |
| .getSchema( | |
| formattedSchemata | |
| .getOrElse("draft-12", sys.error("Missing required 'draft-12' JSON schema in formattedSchemata")) | |
| .value | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/controllers/de/dnpm/dip/rest/api/JsonSchemata.scala` around lines 39 -
44, The JsonSchemata contract currently hardcodes the "draft-12" lookup in
schema, which can fail later with an uncaught missing-key error; update
JsonSchemata and the formattedSchemata contract so the canonical schema key is
enforced explicitly, and make schema/schemaValidator fail fast with a clear
message if draft-12 is absent instead of relying on a runtime assumption.
1 participant
refactor: Moved provision of JSON schemata from UseCaseRouter to UseC…aseController
fix: Added validation of payloads against JSON schema in PatientRecord validation and upload endpoints
fix: bump sbt version
fix: Pinned Jackson version to avoid runtime version conflicts
Summary by CodeRabbit