Server refactor: dedup error/pagination/transaction patterns#309
Merged
daniel-thom merged 5 commits intomainfrom May 10, 2026
Merged
Server refactor: dedup error/pagination/transaction patterns#309daniel-thom merged 5 commits intomainfrom
daniel-thom merged 5 commits intomainfrom
Conversation
Pulls out shared helpers and macros for the patterns that were repeated across the 17 *_transport.rs files: query parameter parsing, X-Span-ID logging, list-handler auth+pagination, user_action event recording, GetJobResponse fan-out, ResultModel cross-validation, error payload construction, and the prefix/suffix path parsers. Also fixes a real bug in slurm_stats_transport.rs where the list endpoint accepted limits exceeding MAX_RECORD_TRANSFER_COUNT because it bypassed process_pagination_params. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Consolidate three duplicated patterns in the server API layer:
- `message_error_response(msg)` and `resource_not_found_response(resource, id)`
in `src/server/api.rs` replace ~86 sites of inline
`models::ErrorResponse::new(serde_json::json!({"message": ...}))` construction
across `src/server/api/*` and `src/server/http_server/jobs_transport.rs`.
- `paginated_list_response!` macro in `src/server/api.rs` replaces the
per-endpoint `current_count`/`offset_val`/`has_more` boilerplate in 17 list
handlers; the model-construction trio collapses to a single macro call.
- Log/error messages referring to database records now follow the
CLAUDE.md `key=value` convention (e.g. `job_id={}` instead of `Job {}`,
`status={:?}` instead of `status {:?}`). Applied in both server and client
code paths plus test assertion messages.
Sites that legitimately use different shapes (extra `field`/`value` payloads,
`error: "Conflict"` access-group responses, KDL syntax generation, plot
titles, asserts on literal command output) are left as-is.
Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Five smaller refactors cleaning up patterns the previous helper-extraction
pass left behind:
- **Job-status int decode** (`api::parse_job_status`): replaces the 7-line
match-then-error pattern in `JobsApiImpl`, `ResultsApiImpl`, and the
http_server transport / lifecycle support. Centralises the log line and
the `ApiError` construction.
- **`tx_try!` macro** in `api.rs`: collapses the
`match $expr { Ok => ..., Err(e) => { tx.rollback().await; return Err(e); } }`
pattern to a single line. Applied across `create_job`, `update_job`'s
depends_on update, and `cancel_workflow`. Sites with non-trivial Ok-arm
logic (e.g. `rows_affected` checks, debug logs) keep their explicit form.
- **`update_job` field-change detection**: extracted `vec_id_set_changed`,
unified the five `*_ids_changed` flags onto set-comparison semantics, and
collapsed the four ~12-line immutability blocks at the bottom of the
function into single-line checks against the precomputed flags. Resends
of the same IDs in a different order are no longer flagged as changes.
- **`map_response!` macro family**: replaced the five specialised
`map_response_std/conflict/accepted_conflict/unprocessable/not_found_only`
macros with a single variadic `map_response!` (plus `map_response_no_forbidden!`
for the two enums that omit `ForbiddenErrorResponse`). Call sites stay one
line for the common case and grow inline `Variant => Status` pairs for the
exceptions.
- **Redundant `sort_by` validation**: `SqlQueryBuilder::with_pagination_and_sorting`
already filters `sort_by` against the allowed-columns list, so the
per-handler `validated_sort_by = if let Some(col) ...` blocks were
duplicate work. Removed in 11 list endpoints. The 3 sites in `workflows.rs`
that translate API column names to SQL aliases are kept (different shape).
Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors the Rust server transport/API layers to reduce repeated boilerplate for error responses, pagination, response→HTTP mapping, and transaction error handling, while standardizing log/assert message formats.
Changes:
- Introduces shared helpers/macros for common response shapes (
message_error_response,resource_not_found_response,paginated_list_response!) and transport mapping (map_response!). - Deduplicates workflow/admin authorization + pagination preambles and request logging via
authorize_*_and_paginate!andlog_call!. - Simplifies transactional error paths with
tx_try!and centralizes job-status integer parsing viaparse_job_status.
Reviewed changes
Copilot reviewed 51 out of 51 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_workflow_spec.rs | Updates panic/assert messages to structured identifiers. |
| tests/test_workflow_manager.rs | Updates assertion message format to key/value style. |
| tests/test_slurm_commands.rs | Updates assertion messages to include job_id/name keys. |
| tests/test_jobs.rs | Updates assertion message format for deletions. |
| tests/test_full_workflows.rs | Updates completion assertion message formats. |
| tests/test_correct_resources.rs | Updates assertion message format for claimed jobs. |
| tests/test_completion_reversal.rs | Updates assertion message format for job status checks. |
| tests/test_claim_next_jobs.rs | Updates assertion message formats for job status and IDs. |
| tests/test_auto_ro_crate.rs | Updates assertion message format for job completion. |
| tests/test_access_groups.rs | Updates assertion message format for job completion. |
| src/server/http_transport/response_mapping.rs | Replaces multiple mapping macros with a single variadic map_response! (+ no-forbidden variant). |
| src/server/http_transport/query_parsing.rs | Deduplicates query decoding into parse_params. |
| src/server/http_transport/path_parsing.rs | Deduplicates repeated path parsing via a shared strip helper (tests). |
| src/server/http_server/workflows_transport.rs | Uses log_call!, combined auth+paginate macro, and centralized audit-event recording. |
| src/server/http_server/user_data_transport.rs | Uses combined workflow auth+pagination scaffold. |
| src/server/http_server/system_transport.rs | Uses log_call! and error_payload! for typed error bodies. |
| src/server/http_server/slurm_stats_transport.rs | Uses combined workflow auth+pagination scaffold; passes concrete offset/limit. |
| src/server/http_server/slurm_schedulers_transport.rs | Uses combined workflow auth+pagination scaffold. |
| src/server/http_server/scheduled_compute_nodes_transport.rs | Uses combined workflow auth+pagination scaffold. |
| src/server/http_server/ro_crate_transport.rs | Uses combined workflow auth+pagination scaffold. |
| src/server/http_server/results_transport.rs | Uses log_call! and combined workflow auth+pagination scaffold. |
| src/server/http_server/resource_requirements_transport.rs | Uses log_call! and centralized audit-event recording; removes unused import. |
| src/server/http_server/local_schedulers_transport.rs | Uses combined workflow auth+pagination scaffold. |
| src/server/http_server/lifecycle_support.rs | Uses shared parse_job_status and updates log phrasing. |
| src/server/http_server/jobs_transport.rs | Adds helpers for translating GetJobResponse and validating completion targets; uses new helpers/macros. |
| src/server/http_server/files_transport.rs | Uses combined workflow auth+pagination scaffold. |
| src/server/http_server/compute_nodes_transport.rs | Uses combined workflow auth+pagination scaffold. |
| src/server/http_server/access_control_transport.rs | Uses combined admin auth+pagination scaffold. |
| src/server/http_server.rs | Adds log_call!, error_payload!, combined auth+paginate macros, username extraction, and audit-event helper. |
| src/server/api/workflows.rs | Uses new error/pagination/tx helpers and removes duplicate sort validation. |
| src/server/api/workflow_actions.rs | Uses shared error helpers and structured log wording. |
| src/server/api/user_data.rs | Uses shared not-found/pagination helpers and reduces sort validation boilerplate. |
| src/server/api/slurm_stats.rs | Uses paginated_list_response! for consistent pagination fields. |
| src/server/api/schedulers.rs | Uses shared not-found/pagination helpers and reduces sort validation boilerplate. |
| src/server/api/ro_crate.rs | Uses shared not-found/error/pagination helpers and removes duplicate sort validation. |
| src/server/api/results.rs | Uses parse_job_status, shared not-found/pagination helpers, and reduces sort validation boilerplate. |
| src/server/api/resource_requirements.rs | Uses shared not-found/pagination helpers and reduces sort validation boilerplate. |
| src/server/api/remote_workers.rs | Uses shared not-found/error helpers for consistent responses. |
| src/server/api/jobs.rs | Adds set-based relationship change detection, uses tx_try!, shared errors/not-found, and parse_job_status. |
| src/server/api/files.rs | Uses shared not-found/pagination helpers and reduces sort validation boilerplate. |
| src/server/api/failure_handlers.rs | Uses shared not-found/pagination helpers. |
| src/server/api/events.rs | Uses shared not-found/pagination helpers and reduces sort validation boilerplate. |
| src/server/api/compute_nodes.rs | Uses shared not-found/pagination helpers and reduces sort validation boilerplate. |
| src/server/api.rs | Adds shared helpers/macros: parse_job_status, message_error_response, resource_not_found_response, tx_try!, paginated_list_response!. |
| src/mcp_server/tools.rs | Updates validation error strings to structured key/value style. |
| src/client/workflow_manager.rs | Updates log messages to key/value style for job/file attribution. |
| src/client/resource_monitor.rs | Updates log messages to key/value style for job/pid attribution. |
| src/client/resource_correction.rs | Updates log messages to key/value style for job attribution. |
| src/client/job_runner.rs | Updates log/error messages to key/value style for job/signal attribution. |
| src/client/hpc/hpc_manager.rs | Updates tracing log format to key/value style. |
| src/client/commands/slurm.rs | Updates error aggregation/log messages to key/value style for SLURM job IDs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- CancelWorkflow returns 404 (NotFoundErrorResponse) when the workflow
row is missing, instead of mapping to a 500 via DefaultErrorResponse.
- translate_get_job_response! logs use key=value format
(job_id={} error={:?}) per the documented log convention.
- Narrow the immutable-fields comment in update_job to list the actual
immutable fields and exclude depends_on_job_ids (which is mutable and
handled below).
Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
- parse_job_status now takes job_id so the error log identifies the offending row when an out-of-range status integer is encountered. All callers updated. - Rephrase record_user_action_event docstring: it awaits inline and swallows errors; calling that "fire-and-forget" was misleading. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
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
Three commits' worth of server-side cleanup, layered on top of
d275bfd1's transport-layer dedup. Net diff vsmain: ~64 files, +2k/-3k lines.d275bfd1Deduplicate transport-layer scaffolding (pre-existing on this branch)Foundation work — not authored in this PR.
03887416Error response helpers andkey=valuelog formatmessage_error_response(msg)andresource_not_found_response(resource, id)insrc/server/api.rsreplace ~86 sites of inlinemodels::ErrorResponse::new(serde_json::json!({"message": ...}))construction.paginated_list_response!macro replaces the per-endpointcurrent_count/offset_val/has_moreboilerplate in 17 list handlers.key=valueconvention (job_id={}notJob {},status={:?}notstatus {:?}). Applied across server, client, and test assertion messages.field/value/error: "Conflict"), KDL syntax generation, plot titles, and asserts on literal command output are intentionally left as-is.455c2cb6Five smaller refactorsparse_job_statushelper — collapses the 7-line match-then-error pattern in 7 sites acrossJobsApiImpl,ResultsApiImpl, and the transport / lifecycle support modules.tx_try!macro — single-line replacement formatch $expr { Ok => ..., Err => { tx.rollback().await; return Err(e); } }. Applied acrosscreate_job,update_job's depends-on update, andcancel_workflow. Sites with non-trivial Ok-arm logic kept their explicit form.update_jobfield-change detection — extractedvec_id_set_changed, unified the five*_ids_changedflags onto set-comparison semantics, collapsed four ~12-line immutability blocks into single-line checks. Resends of the same IDs in a different order are no longer flagged as changes.map_response!macro family — replaced five specialised macros with one variadicmap_response!(plusmap_response_no_forbidden!for the two enums lackingForbiddenErrorResponse). 122 invocations migrated.sort_byvalidation —SqlQueryBuilder::with_pagination_and_sortingalready filters against the allowed-columns list, so per-handlervalidated_sort_byblocks were duplicate work. Removed in 11 endpoints. The 3 sites inworkflows.rsthat translate API column names to SQL aliases are kept (different shape).What's intentionally left
A separate audit of SQLite transaction usage found four follow-ups (none critical): a 3-site
commit-fails → defensive rollbackdedup, one consistency fix for an implicit-drop rollback, missing lock-aware error wrapping in 4 DEFERREDpool.begin()sites, and 4 sites still using rawsqlx::query("BEGIN/COMMIT/ROLLBACK")instead of theTransactionAPI. Recorded for a follow-up branch.Test plan
{"message": ...}shape exactly)update_jobset-comparison change doesn't break tests that depend on order-sensitive comparison (set semantics is more correct, but it is a behaviour change)tx_try!sites by running the integration test suite🤖 Generated with Claude Code