Conversation
📝 WalkthroughWalkthroughAdds JSONPath dependency and implements replacement-driven batch processing: new ResourcePath/Replacement models and request-level replacements, replacement validation, JSONPath-based find-and-replace in batch utilities, refactored batch execution to propagate replacements/data_references, and an extensive test suite for replacements and permissions. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
care/emr/api/viewsets/batch_request.py (1)
53-78: UnHandledError path can blow up before returning a 500 response.
execute_batch_requestsnow raisesUnHandledErrorinside the atomic block. If that happens,structured_responseswas never assigned, so theexcept UnHandledErrorblock can raiseUnboundLocalErrorinstead of returning a proper payload.🩹 Minimal guard (also consider returning partial responses from the exception)
- try: + structured_responses = [] + try: with transaction.atomic(): responses = execute_batch_requests(request, requests , replacements , data_references) - structured_responses = [] for response in responses: if response["status_code"] > 299: # noqa PLR2004 errored = True structured_responses.append(
🤖 Fix all issues with AI agents
In `@care/emr/api/viewsets/batch_request.py`:
- Line 60: The call to execute_batch_requests has incorrect spacing before
commas (execute_batch_requests(request, requests , replacements ,
data_references)), which fails ruff/formatter; remove the stray spaces so it
reads execute_batch_requests(request, requests, replacements, data_references)
and then run the project's pre-commit hooks (or ruff/black) to reformat the file
and commit the change.
In `@care/emr/utils/batch_requests.py`:
- Around line 106-107: CI failed because the file
care/emr/utils/batch_requests.py was reformatted by Ruff; run the project
formatter (ruff/black as configured) locally and commit the formatted changes so
the CI diff matches. Specifically, open care/emr/utils/batch_requests.py (look
for the execute_serially function signature) and run the repository's formatter
command (e.g., ruff format / ruff check --fix or the project's preconfigured
format script), then add and commit the updated file so the PR includes the
formatting changes.
- Around line 118-119: Replace the magic number 500 in the response status check
with DRF's enum: change the condition in the function that contains
response["status_code"] >= 500 to use status.HTTP_500_INTERNAL_SERVER_ERROR
(i.e. response["status_code"] >= status.HTTP_500_INTERNAL_SERVER_ERROR), and
ensure you import rest_framework.status (e.g., from rest_framework import
status) at the top of batch_requests.py so the symbol is available; keep raising
UnHandledError as before.
- Around line 92-105: Guard against missing references and JSONPath parse
failures in find_and_replace_data: replace direct data_references[...] access
with data_references.get(replacement.source_path.reference_id) and check for
None, and wrap parse(...) calls for both replacement.source_path.path and
replacement.value_path.path in try/except catching
jsonpath_ng.exceptions.JsonPathParserError to raise a ParseError with a
descriptive message; also handle missing source or destination values by
skipping or raising ParseError as appropriate. Ensure you reference the function
find_and_replace_data, the attributes replacement.source_path.reference_id,
replacement.value_path.reference_id, and use parse(...) from jsonpath_ng, and
add the necessary imports for JsonPathParserError and ParseError.
In `@Pipfile`:
- Line 54: The Pipfile currently pins jsonpath-ng ==1.7.0 which lacks explicit
Python 3.13 support; update the dependency line to use the dg-jsonpath-ng fork
(e.g., replace "jsonpath-ng ==1.7.0" with "dg-jsonpath-ng
==<appropriate-version>") to ensure declared 3.13 compatibility, then regenerate
your lockfile (pipenv lock or pip-compile) and run tests to confirm behavior;
reference the package symbol jsonpath-ng in the change so reviewers can verify
the replacement.
🧹 Nitpick comments (1)
care/emr/api/viewsets/batch_request.py (1)
25-25: UseField(default_factory=list)forreplacements.
Mutable defaults in Pydantic models are still a foot‑gun, even if they’re copied.🔧 Minimal fix
class Request(BaseModel): url: str method: str body: dict = {} reference_id: str - replacements: list[Replacement] = [] + replacements: list[Replacement] = Field(default_factory=list)
care/emr/utils/batch_requests.py
Outdated
| def find_and_replace_data(data, reference_id, replacements, data_references): | ||
| for replacement in replacements: | ||
| if replacement.value_path.reference_id == reference_id: | ||
| source = data_references[replacement.source_path.reference_id] | ||
| source_query = parse(replacement.source_path.path) | ||
| source_value = list(source_query.find(source)) | ||
| if source_value: | ||
| source_value = source_value[0] | ||
| destination_query = parse(replacement.value_path.path) | ||
| destination_value = list(destination_query.find(data)) | ||
| if destination_value: | ||
| destination_value = destination_value[0] | ||
| destination_value.full_path.update(data, source_value.value) | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate the file and check its existence
find . -name "batch_requests.py" -type fRepository: ohcnetwork/care
Length of output: 93
🏁 Script executed:
# Read the file to see the actual implementation around lines 92-105
cat -n care/emr/utils/batch_requests.py | sed -n '80,115p'Repository: ohcnetwork/care
Length of output: 1885
🏁 Script executed:
# Check what exceptions are currently imported or used in this file
head -30 care/emr/utils/batch_requests.pyRepository: ohcnetwork/care
Length of output: 974
🏁 Script executed:
# Search for ParseError definition in the codebase
rg "class ParseError|from.*ParseError|import.*ParseError" --type pyRepository: ohcnetwork/care
Length of output: 140
🏁 Script executed:
# Check jsonpath_ng library and its exceptions
rg "jsonpath_ng" --type py -A 2 -B 2Repository: ohcnetwork/care
Length of output: 394
🏁 Script executed:
# Check if JsonPathParserError is used anywhere in the codebase
rg "JsonPathParserError" --type pyRepository: ohcnetwork/care
Length of output: 41
🏁 Script executed:
# Check what version of jsonpath_ng is being used and what exceptions it has
cat -n care/emr/utils/batch_requests.py | head -20Repository: ohcnetwork/care
Length of output: 770
🏁 Script executed:
# Search for any error handling around parse() calls
rg "parse\(" --type py -B 2 -A 2 | head -50Repository: ohcnetwork/care
Length of output: 2493
🌐 Web query:
jsonpath_ng exceptions JsonPathParserError
💡 Result:
jsonpath_ng.exceptions.JsonPathParserError is raised when jsonpath_ng.parse() can’t parse your JSONPath string (invalid/unsupported syntax). (tessl.io)
Common causes (and fixes):
- Unbalanced / malformed JSONPath (missing
],), quotes, etc.)from jsonpath_ng import parse from jsonpath_ng.exceptions import JsonPathParserError try: expr = parse('$.invalid[syntax') except JsonPathParserError as e: print(e) ``` ([tessl.io](https://tessl.io/registry/tessl/pypi-jsonpath-ng/1.7.0/files/docs/path-operations.md?utm_source=openai))
- Keys that look like numbers (or start with digits) must be accessed with bracket-quote syntax.
If your JSON has keys like"980E5"or"62", dot-notation parsing can fail with messages like “near token … (NUMBER)”. Use:This avoids the parser interpreting those segments as numeric tokens. (discuss.jsonapi.org)expr = parse("$['980E5']['62']['AHS']")
If you paste the exact JSONPath you’re parsing and the exact exception text, I can point out the specific token that’s breaking the grammar and the corrected expression.
Citations:
- 1: https://tessl.io/registry/tessl/pypi-jsonpath-ng/1.7.0/files/docs/core-parsing.md?utm_source=openai
- 2: https://tessl.io/registry/tessl/pypi-jsonpath-ng/1.7.0/files/docs/path-operations.md?utm_source=openai
- 3: https://discuss.jsonapi.org/t/jsonpath-ng-error/2449?utm_source=openai
Guard against missing references and invalid JSONPath syntax.
Unguarded dictionary access and parse errors here will raise KeyError and JsonPathParserError respectively, bubbling up as 500 errors. Since these come from user input, they should gracefully return 400 instead.
Replace the direct dictionary access with .get() and add try-except around the parse() calls to catch JsonPathParserError from jsonpath_ng.exceptions, then raise ParseError with a descriptive message in each case.
🤖 Prompt for AI Agents
In `@care/emr/utils/batch_requests.py` around lines 92 - 105, Guard against
missing references and JSONPath parse failures in find_and_replace_data: replace
direct data_references[...] access with
data_references.get(replacement.source_path.reference_id) and check for None,
and wrap parse(...) calls for both replacement.source_path.path and
replacement.value_path.path in try/except catching
jsonpath_ng.exceptions.JsonPathParserError to raise a ParseError with a
descriptive message; also handle missing source or destination values by
skipping or raising ParseError as appropriate. Ensure you reference the function
find_and_replace_data, the attributes replacement.source_path.reference_id,
replacement.value_path.reference_id, and use parse(...) from jsonpath_ng, and
add the necessary imports for JsonPathParserError and ParseError.
care/emr/utils/batch_requests.py
Outdated
| def execute_serially(parent_request, requests, resp_generator , replacements, data_references): | ||
|
|
There was a problem hiding this comment.
Ruff already reformatted this file.
Run the formatter locally so CI can move on.
🤖 Prompt for AI Agents
In `@care/emr/utils/batch_requests.py` around lines 106 - 107, CI failed because
the file care/emr/utils/batch_requests.py was reformatted by Ruff; run the
project formatter (ruff/black as configured) locally and commit the formatted
changes so the CI diff matches. Specifically, open
care/emr/utils/batch_requests.py (look for the execute_serially function
signature) and run the repository's formatter command (e.g., ruff format / ruff
check --fix or the project's preconfigured format script), then add and commit
the updated file so the PR includes the formatting changes.
care/emr/utils/batch_requests.py
Outdated
| if response["status_code"] >= 500: | ||
| raise UnHandledError |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n care/emr/utils/batch_requests.py | sed -n '110,130p'Repository: ohcnetwork/care
Length of output: 1002
🏁 Script executed:
head -30 care/emr/utils/batch_requests.pyRepository: ohcnetwork/care
Length of output: 974
🏁 Script executed:
rg "HTTP_500_INTERNAL_SERVER_ERROR" --type pyRepository: ohcnetwork/care
Length of output: 161
🏁 Script executed:
python3 <<'EOF'
from rest_framework import status
print(f"HTTP_500_INTERNAL_SERVER_ERROR = {status.HTTP_500_INTERNAL_SERVER_ERROR}")
print(f"Type: {type(status.HTTP_500_INTERNAL_SERVER_ERROR)}")
EOFRepository: ohcnetwork/care
Length of output: 185
🏁 Script executed:
rg "from rest_framework import status" --type py | head -10Repository: ohcnetwork/care
Length of output: 776
🏁 Script executed:
cat -n care/emr/utils/batch_requests.py | head -50Repository: ohcnetwork/care
Length of output: 1797
🏁 Script executed:
rg "status_code.*>|status_code.*<|>= 4|>= 5" care/emr/utils/batch_requests.py -A 1 -B 1Repository: ohcnetwork/care
Length of output: 202
🏁 Script executed:
rg "status\.HTTP_5\d{2}" care/emr/api/viewsets/ -A 1 -B 1 | head -20Repository: ohcnetwork/care
Length of output: 313
Replace the magic 500 constant with DRF's status enum.
The codebase already uses status.HTTP_500_INTERNAL_SERVER_ERROR elsewhere — consistent naming conventions help readability.
🔧 Minimal fix
+from rest_framework import status
@@
- if response["status_code"] >= 500:
+ if response["status_code"] >= status.HTTP_500_INTERNAL_SERVER_ERROR:
raise UnHandledError🤖 Prompt for AI Agents
In `@care/emr/utils/batch_requests.py` around lines 118 - 119, Replace the magic
number 500 in the response status check with DRF's enum: change the condition in
the function that contains response["status_code"] >= 500 to use
status.HTTP_500_INTERNAL_SERVER_ERROR (i.e. response["status_code"] >=
status.HTTP_500_INTERNAL_SERVER_ERROR), and ensure you import
rest_framework.status (e.g., from rest_framework import status) at the top of
batch_requests.py so the symbol is available; keep raising UnHandledError as
before.
| evalidate = "==2.1.1" | ||
| weasyprint = "==68.0" | ||
| pip = "==25.3" | ||
| jsonpath-ng = "==1.7.0" |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
jsonpath-ng 1.7.0 Python 3.13 compatibility PyPI
💡 Result:
jsonpath-ng 1.7.0 on PyPI does not explicitly claim Python 3.13 support (its Trove classifiers stop at Python 3.12) and its project text says it’s “tested with CPython 3.8 and higher.” It ships a universal py3-none-any wheel, so it will generally install on Python 3.13, but Python 3.13 compatibility is not officially declared in PyPI metadata. [1]
If you need a package that is clearly being built/published with Python 3.13, the dg-jsonpath-ng fork’s PyPI release artifacts show uploads performed with CPython 3.13.1. [2]
The upstream GitHub README also states it is tested with CPython 3.10 and higher (but still doesn’t specifically mention 3.13). [3]
Sources: [1] [2] [3]
jsonpath-ng 1.7.0 lacks official Python 3.13 support.
PyPI metadata shows support only up to Python 3.12, and the package has not been explicitly tested for Python 3.13. While the universal wheel will technically install and likely work, this isn't officially supported or declared. If Python 3.13 is required, consider using the dg-jsonpath-ng fork, which has explicit Python 3.13.1 support.
🤖 Prompt for AI Agents
In `@Pipfile` at line 54, The Pipfile currently pins jsonpath-ng ==1.7.0 which
lacks explicit Python 3.13 support; update the dependency line to use the
dg-jsonpath-ng fork (e.g., replace "jsonpath-ng ==1.7.0" with "dg-jsonpath-ng
==<appropriate-version>") to ensure declared 3.13 compatibility, then regenerate
your lockfile (pipenv lock or pip-compile) and run tests to confirm behavior;
reference the package symbol jsonpath-ng in the change so reviewers can verify
the replacement.
* feat:added testcases for the batch request * feat:added support for replacement of multiple destination values * feat:added testcases for testcases * feat:added support for url replacement * refact:changed format to replace * fix:keep only one copy of reference responses to avoid excessive memory usage * feat:added reference based validation * feat:added path level validation for both source path and value path * feat:added testcases on replacement validations
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@care/emr/tests/test_batch_request_api.py`:
- Around line 255-258: Rename the misspelled variable refernce_ids to
reference_ids across the test file to fix the typo; update every occurrence
(e.g., where refernce_ids is assigned from response.data["results"] and where
it's used in assertions like self.assertIn("allergy_intolerance", refernce_ids))
so the variable name matches reference_ids consistently in functions and test
cases.
- Around line 534-556: The encounter_request body is using the wrong key
"organization"; update the body in the encounter_request (the create_request
call for reverse("encounter-list")) to use the plural "organizations" and supply
a list of UUID strings (e.g., replace the single value with
[str(self.facility_organization.external_id)]), matching the encounter API spec
and the existing superuser test pattern.
In `@care/emr/utils/batch_requests.py`:
- Around line 94-96: Fix the typo and simplify the dict access: rename the
misspelled local variable source_refernce_id to source_reference_id and use it
consistently; when checking and retrieving from the data_references dict use the
same variable (e.g., if source_reference_id in data_references: source =
data_references[source_reference_id]) instead of re-accessing
replacement.source_path.reference_id. This change affects the variables around
replacement.source_path.reference_id and data_references in batch_requests.py.
- Line 144: The trailing noqa directive on the conditional in the expression
containing response["status_code"] (the line `and response["status_code"] < 300
# noqa PLR2004`) is unnecessary; remove the " # noqa PLR2004" comment so the
line simply reads `and response["status_code"] < 300`. Locate this expression in
the function where response is evaluated in batch_requests.py (search for
response["status_code"]) and delete the unused noqa token, then run the linter
to confirm no other suppression is required.
🧹 Nitpick comments (4)
care/emr/api/viewsets/batch_request.py (2)
57-81: Validation logic is solid.The ordering check ensures that
source_pathreferences can only point to requests that have already been executed. One small thing worth noting: if duplicatereference_idvalues are provided in the batch, the later one will overwrite inreference_id_order, which could allow incorrect validation. You might want to validate uniqueness ofreference_idvalues as well.🔧 Optional: Add uniqueness validation
def validate_replacements(self, requests): reference_id_order = {} for index, req in enumerate(requests.requests): + if req.reference_id in reference_id_order: + raise ValidationError(f"Duplicate reference_id: {req.reference_id}") reference_id_order[req.reference_id] = index
92-95: Consider simplifying replacement collection.The nested loop works, but a list comprehension would be more Pythonic. Just a thought.
♻️ Optional: Use list comprehension
- replacements = [] - for req in requests.requests: - for replacement in req.replacements: - replacements.append(replacement) + replacements = [ + replacement + for req in requests.requests + for replacement in req.replacements + ]care/emr/tests/test_batch_request_api.py (1)
28-35: Consider moving import to module level.The import of
RefreshTokeninside the method works fine for tests, but moving it to the top of the file would be more conventional. Not a big deal since this is test code.care/emr/utils/batch_requests.py (1)
120-123: Consider movingUnHandledErrorto a shared module.The runtime import on line 123 exists to avoid circular imports. A cleaner approach would be to define
UnHandledError(andHandledError) in a shared exceptions module that both files can import from.
| responses.append(response) | ||
| if ( | ||
| request["reference_id"] in data_reference_required_id | ||
| and response["status_code"] < 300 # noqa PLR2004 |
There was a problem hiding this comment.
Remove unused noqa directive.
Ruff indicates this noqa directive is unused. Either the rule it's suppressing isn't active, or the syntax is incorrect.
🧹 Remove unused noqa
- and response["status_code"] < 300 # noqa PLR2004
+ and response["status_code"] < 300📝 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.
| and response["status_code"] < 300 # noqa PLR2004 | |
| and response["status_code"] < 300 |
🧰 Tools
🪛 Ruff (0.14.14)
[warning] 144-144: Unused blanket noqa directive
Remove unused noqa directive
(RUF100)
🤖 Prompt for AI Agents
In `@care/emr/utils/batch_requests.py` at line 144, The trailing noqa directive on
the conditional in the expression containing response["status_code"] (the line
`and response["status_code"] < 300 # noqa PLR2004`) is unnecessary; remove the
" # noqa PLR2004" comment so the line simply reads `and response["status_code"]
< 300`. Locate this expression in the function where response is evaluated in
batch_requests.py (search for response["status_code"]) and delete the unused
noqa token, then run the linter to confirm no other suppression is required.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3500 +/- ##
===========================================
+ Coverage 74.43% 74.74% +0.30%
===========================================
Files 471 473 +2
Lines 21732 21974 +242
Branches 2264 2303 +39
===========================================
+ Hits 16176 16424 +248
+ Misses 5055 5034 -21
- Partials 501 516 +15 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@care/emr/tests/test_batch_request_api.py`:
- Around line 37-47: The helper create_request currently uses a truthy check "if
body:" which drops intentionally empty payloads (e.g. {} or []); change that
check to "if body is not None" so empty dicts/lists are preserved and only None
means absent, updating the logic that sets request["body"]; keep the rest of the
function (including the reference_id and replacements handling) unchanged.
🧹 Nitpick comments (1)
care/emr/tests/test_batch_request_api.py (1)
588-819: Consider extracting common request builders.The patient/encounter/questionnaire/allergy/diagnosis request setup is repeated a lot. A small helper or two would cut duplication and make these tests easier to maintain.
♻️ Example helper extraction
+ def build_patient_fetch_request(self): + return self.create_request( + url=reverse("patient-detail", args=[self.patient.external_id]), + method="GET", + reference_id="patient_fetch", + ) + + def build_encounter_create_request(self): + return self.create_request( + url=reverse("encounter-list"), + method="POST", + body={ + "patient": None, + "facility": self.facility.external_id, + "organizations": [str(self.facility_organization.external_id)], + "encounter_class": "imp", + "status": "planned", + "priority": "routine", + }, + reference_id="encounter_create", + replacements=[ + { + "source_path": {"reference_id": "patient_fetch", "path": "id"}, + "value_path": {"reference_id": "encounter_create", "path": "patient"}, + } + ], + )
| def create_request(self, url, method, body=None, reference_id=None, **kwargs): | ||
| request = { | ||
| "url": url, | ||
| "method": method, | ||
| } | ||
| if body: | ||
| request["body"] = body | ||
| if reference_id: | ||
| request["reference_id"] = reference_id | ||
| if kwargs.get("replacements"): | ||
| request["replacements"] = kwargs.get("replacements") |
There was a problem hiding this comment.
Allow empty bodies by checking is not None.
Right now if body: drops empty dict/list payloads, which makes “intentionally empty” cases impossible to express.
✅ Small fix
- if body:
+ if body is not None:
request["body"] = body🤖 Prompt for AI Agents
In `@care/emr/tests/test_batch_request_api.py` around lines 37 - 47, The helper
create_request currently uses a truthy check "if body:" which drops
intentionally empty payloads (e.g. {} or []); change that check to "if body is
not None" so empty dicts/lists are preserved and only None means absent,
updating the logic that sets request["body"]; keep the rest of the function
(including the reference_id and replacements handling) unchanged.
2 participants
Batch API now supports request chaining with dependency updates, FE can go all crazy now.
Summary by CodeRabbit