Edits to odc-cli and subject services for production usage#1374
Open
david-roper wants to merge 7 commits into
Open
Edits to odc-cli and subject services for production usage#1374david-roper wants to merge 7 commits into
david-roper wants to merge 7 commits into
Conversation
… timeout DELETE requests were incorrectly including Content-Type: application/json with no body, causing production servers to reject them with 400. The request timeout was also too short (2s) for force-delete operations that cascade through instrument records and sessions.
Contributor
|
Wondering what really moved? Review this PR in Change Stack to inspect semantic changes, definitions, and references. WalkthroughAdds instrument-record deletion into the subject deletion transaction (before session deletion). Updates CLI HttpClient to only set Content-Type when a body exists, construct request bodies conditionally, increase request timeout to 30s, serialize minDate for instrument records, and use path-style subject lookup with client-side min_date filtering when subject_id is absent. ChangesSubject deletion transaction
HTTP client headers and timeout
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses production issues around subject cascade deletion in the API and improves odc-cli HTTP request behavior to avoid server-side 400 errors on bodyless requests.
Changes:
- Reordered the subject force-delete
$transactionso instrument records are deleted before sessions (then the subject). - Updated
odc-clito only sendContent-Type: application/jsonwhen a request body is present. - Increased
odc-clirequest timeout from 2s to 30s to better accommodate long-running delete operations.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
cli/odc-cli |
Adjusts request header construction (avoid Content-Type on bodyless requests) and increases timeout for long-running operations. |
apps/api/src/subjects/subjects.service.ts |
Fixes cascade delete order inside a Prisma transaction to avoid relation/FK violations in production. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
cli/odc-cli (2)
416-416: 💤 Low valueSimplify: empty params dict is unnecessary.
Passing
{}tobuild_url_with_paramsadds no value. Pass the URL string directly toHttpClient.get().♻️ Proposed simplification
- url = build_url_with_params( - f'{config.base_url}/v1/subjects', - {} - ) - response = HttpClient.get(url) + response = HttpClient.get(f'{config.base_url}/v1/subjects')🤖 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 `@cli/odc-cli` at line 416, The call currently passes an empty params dict into build_url_with_params and then into HttpClient.get; remove the unnecessary {} by passing the plain URL string directly to HttpClient.get (i.e., stop calling build_url_with_params when params is empty) so replace the build_url_with_params(..., {}) usage with the URL string and call HttpClient.get(url) instead, updating any surrounding code that assumes a params-augmented URL; focus on the sites where build_url_with_params and HttpClient.get are used together.
419-419: 💤 Low valueClarify behavior when min_date is provided but response is not a list.
When
min_dateis provided butresponse.datais not a list, filtering is silently skipped. This could confuse users who expect filtering to occur. Consider logging a warning or validating the response type.📝 Proposed clarification
- if min_date is not None and isinstance(response.data, list): + if min_date is not None: + if not isinstance(response.data, list): + print("Warning: Cannot apply min_date filter; API response is not a list", file=sys.stderr) + else: min_date_utc = min_date.replace(tzinfo=timezone.utc) if min_date.tzinfo is None else min_date response.data = [ s for s in response.data if datetime.fromisoformat(s['createdAt'].replace('Z', '+00:00')) >= min_date_utc ]🤖 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 `@cli/odc-cli` at line 419, When min_date is provided but response.data isn't a list, we should surface that instead of silently skipping filtering: update the branch around the "if min_date is not None and isinstance(response.data, list):" check to handle the non-list case by logging a warning (via the module logger or existing logger used nearby) or by raising a clear ValueError indicating that filtering by min_date requires response.data to be a list; reference and use the variables min_date and response.data in the message so callers can diagnose the issue. Ensure the change is applied where response is processed (the code that evaluates response.data) and keep the current list-filtering path intact.
🤖 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 `@cli/odc-cli`:
- Around line 419-424: Wrap the date-parsing/filtering logic that uses
datetime.fromisoformat on s['createdAt'] in a try-except so malformed or missing
createdAt values do not crash the CLI: in the block that checks min_date and
isinstance(response.data, list) (referencing min_date, response.data, and the
list comprehension that accesses s['createdAt']), validate each item has a
truthy 'createdAt' string, attempt to parse it inside a try/except (catch
ValueError/KeyError/TypeError), skip items with invalid or missing dates
(optionally log or collect them), and only compare parsed datetimes to
min_date_utc for inclusion. Ensure min_date_utc computation
(min_date.replace/tzinfo) remains unchanged and used for the comparison.
---
Nitpick comments:
In `@cli/odc-cli`:
- Line 416: The call currently passes an empty params dict into
build_url_with_params and then into HttpClient.get; remove the unnecessary {} by
passing the plain URL string directly to HttpClient.get (i.e., stop calling
build_url_with_params when params is empty) so replace the
build_url_with_params(..., {}) usage with the URL string and call
HttpClient.get(url) instead, updating any surrounding code that assumes a
params-augmented URL; focus on the sites where build_url_with_params and
HttpClient.get are used together.
- Line 419: When min_date is provided but response.data isn't a list, we should
surface that instead of silently skipping filtering: update the branch around
the "if min_date is not None and isinstance(response.data, list):" check to
handle the non-list case by logging a warning (via the module logger or existing
logger used nearby) or by raising a clear ValueError indicating that filtering
by min_date requires response.data to be a list; reference and use the variables
min_date and response.data in the message so callers can diagnose the issue.
Ensure the change is applied where response is processed (the code that
evaluates response.data) and keep the current list-filtering path intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cli/odc-cli (1)
409-412:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMutual exclusivity between
--min-dateand--subject-idis documented but not enforced.If both arguments are provided,
min_dateis silently ignored. Consider usingadd_mutually_exclusive_group()in argparse or adding an explicit check with an error message.Option 1: Enforce in argparse (preferred)
In
_create_subjects_parser, use a mutually exclusive group:mutex_group = find_parser.add_mutually_exclusive_group() mutex_group.add_argument('--min-date', type=ArgumentTypes.valid_datetime, help='...') mutex_group.add_argument('--subject-id', help='...')Option 2: Runtime check
def find(min_date: datetime | None, subject_id: str | None) -> None: + if min_date is not None and subject_id is not None: + print('Error: --min-date and --subject-id cannot be used together', file=sys.stderr) + sys.exit(1) if subject_id is not None:🤖 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 `@cli/odc-cli` around lines 409 - 412, The find function currently allows both min_date and subject_id to be provided and silently ignores min_date; update the CLI argument parsing in _create_subjects_parser to enforce mutual exclusivity by creating a mutually exclusive group via add_mutually_exclusive_group() and registering the two options there (use the existing ArgumentTypes.valid_datetime for --min-date and the existing --subject-id argument), or if you prefer a runtime guard, add an explicit check at the start of find(subject_id, min_date) that raises a clear error when both are present (e.g., print error and exit) to ensure the documented exclusivity is enforced.
🧹 Nitpick comments (1)
apps/api/src/subjects/__tests__/subjects.service.spec.ts (1)
143-147: ⚡ Quick winAlso assert deleteMany filters, not just operation order.
This test proves sequencing, but it can still pass if the delete predicates are wrong. Add assertions that each deleteMany is called with the expected
subject.id/idfilters for'123'to lock down cascade scope and prevent silent over/under-deletes in future edits.Suggested assertion additions
await subjectsService.deleteById('123', { force: true }); + expect(prismaClient.instrumentRecord.deleteMany).toHaveBeenCalledWith( + expect.objectContaining({ + where: expect.objectContaining({ subject: { id: '123' } }) + }) + ); + expect(prismaClient.session.deleteMany).toHaveBeenCalledWith( + expect.objectContaining({ + where: expect.objectContaining({ subject: { id: '123' } }) + }) + ); + expect(prismaClient.subject.deleteMany).toHaveBeenCalledWith( + expect.objectContaining({ + where: expect.objectContaining({ id: '123' }) + }) + ); expect(prismaClient.$transaction).toHaveBeenCalledWith([instrumentRecordOp, sessionOp, subjectOp]);🤖 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 `@apps/api/src/subjects/__tests__/subjects.service.spec.ts` around lines 143 - 147, Add assertions that the three deleteMany calls use the correct where filters for subject '123': assert prismaClient.instrumentRecord.deleteMany was called with a filter like { where: { subjectId: '123' } }, assert prismaClient.session.deleteMany was called with { where: { subjectId: '123' } }, and assert prismaClient.subject.deleteMany was called with { where: { id: '123' } }; keep the existing assertion that prismaClient.$transaction was called with [instrumentRecordOp, sessionOp, subjectOp] to preserve sequencing.
🤖 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.
Outside diff comments:
In `@cli/odc-cli`:
- Around line 409-412: The find function currently allows both min_date and
subject_id to be provided and silently ignores min_date; update the CLI argument
parsing in _create_subjects_parser to enforce mutual exclusivity by creating a
mutually exclusive group via add_mutually_exclusive_group() and registering the
two options there (use the existing ArgumentTypes.valid_datetime for --min-date
and the existing --subject-id argument), or if you prefer a runtime guard, add
an explicit check at the start of find(subject_id, min_date) that raises a clear
error when both are present (e.g., print error and exit) to ensure the
documented exclusivity is enforced.
---
Nitpick comments:
In `@apps/api/src/subjects/__tests__/subjects.service.spec.ts`:
- Around line 143-147: Add assertions that the three deleteMany calls use the
correct where filters for subject '123': assert
prismaClient.instrumentRecord.deleteMany was called with a filter like { where:
{ subjectId: '123' } }, assert prismaClient.session.deleteMany was called with {
where: { subjectId: '123' } }, and assert prismaClient.subject.deleteMany was
called with { where: { id: '123' } }; keep the existing assertion that
prismaClient.$transaction was called with [instrumentRecordOp, sessionOp,
subjectOp] to preserve sequencing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: cdead55c-652f-4b26-a051-bc780bdce324
📒 Files selected for processing (2)
apps/api/src/subjects/__tests__/subjects.service.spec.tscli/odc-cli
Comment on lines
571
to
578
| find_parser = subparsers.add_parser('find', help='search subject records by optional filters') | ||
| find_parser.add_argument( | ||
| '--min-date', | ||
| type=ArgumentTypes.valid_datetime, | ||
| help='filter records created after this date (format: yyyy-mm-dd or yyyy-mm-dd hh:mm:ss)', | ||
| help='filter subjects created after this date (format: yyyy-mm-dd or yyyy-mm-dd hh:mm:ss). Cannot be used with --subject-id', | ||
| ) | ||
| find_parser.add_argument('--subject-id', help='filter by subject id') | ||
| find_parser.add_argument('--subject-id', help='find a single subject by exact id. Cannot be used with --min-date') | ||
| find_parser.set_defaults(fn=SubjectCommands.find) |
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.
Works on issue #1373
code implemented with usage of claude-sonnet-4-6 on claude code
Bug fixes for cascading subject deletion and the
odc-cliHTTP client.Changes
Fix: Cascading subject deletion order (
apps/api/src/subjects/subjects.service.ts)Reordered the Prisma
$transactioninSubjectsService.deleteByIdso thatinstrument records are deleted before sessions, then the subject itself.
The previous order (sessions → instrument records → subject) caused a
foreign-key violation in production because MongoDB enforced referential
integrity at the session level before instrument records had been cleared.
Local development was unaffected since standalone MongoDB does not enforce
FK constraints without a replica set.
New order:
Fix:
odc-clisendingContent-Typeon bodyless requests (cli/odc-cli)DELETErequests with no body were including aContent-Type: application/jsonheader, causing the API to return
400 Body cannot be empty. The header is nowonly added when a request body is present.
The request timeout was also increased from 2 s to 30 s to accommodate the
longer cascade-delete operation.
fix(cli): correct instrument-records minDate serialization and subject find filters
correct parsing by the backend's z.coerce.date() schema
instead of passing it as an ignored list query param
in the subjects find help text
Co-Authored-By: Claude Sonnet 4.6 noreply@anthropic.com
Test Plan
the API — confirm
200and no FK error in logspython3 odc-cli subjects delete --id '<subject-id>'against a remoteinstance — confirm
200and no400errorpython3 odc-cli subjects find— confirm requests without a bodystill succeed
records
Summary by CodeRabbit
Bug Fixes
Improvements
Tests