JITSU-91: migrate sync/debug/reports HTTP routes onto the shared service classes#1404
Conversation
vklimontovich
added
the
deploy:console
![jitsu-code-review[bot]](https://avatars.githubusercontent.com/in/3391383?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed the API-route-to-service refactor and associated integration test updates for correctness/security regressions.
I did not find blocking issues in this changeset. The service extraction and handler rewiring appear behaviorally consistent with previous logic in the areas I checked.
…ice classes Route handlers delegate to SyncService/DebugService/ReportsService (extracted for the MCP server in #1385) so HTTP and MCP share one implementation of workspace scoping, secret unmasking and syncctl/bulker/functions dispatch. Transport stays in the routes: createRoute schemas, OpenAPI metadata, the scheduler-bearer special case in sources/run and the gzip streaming in sources/logs. Service gaps closed for parity: latestSyncTasks (tasks POST aggregation), setSyncState (state editor upsert), triggerSourceCheck accepts an unsaved config with a caller-generated storageKey, runSync taskId and runFunction functionName passthrough, eventStat returns srcSize. Security fixes picked up by delegating: sources/check requires the exact workspaceId_ storage-key segment (ws1 no longer accepts ws10 keys), sync-stat counts distinct (fromId, toId) tuples instead of string concatenation. profile-builder/stats.ts from the issue no longer exists (removed in #1195). JITSU-91
e45d310 to
bbc35d6
Compare
There was a problem hiding this comment.
Reviewed the service extraction and route rewiring in this PR, focusing on sync/report/debug API behavior and workspace scoping. I found one user-visible correctness regression (details inline) around the task logs URL generation when syncId is omitted. Everything else I checked in this range looked consistent with previous behavior.
"Mirrors" read as a duplicated copy to keep in sync; the routes delegate to these methods, so there is a single implementation. JITSU-91
bea0edb
There was a problem hiding this comment.
Reviewed the route→service migration across sync/debug/reports handlers and related integration coverage. I focused on workspace scoping, secret handling, and request/response behavior parity with prior routes.
No new actionable bugs/regressions found beyond the already-open thread on logs URL construction when only is provided.
There was a problem hiding this comment.
Reviewed the route-to-service migration across sync/debug/reports handlers and related integration coverage. I focused on workspace scoping, secret handling, and request/response behavior parity with prior routes.
No new actionable bugs/regressions found beyond the already-open thread on sources/tasks logs URL construction when only taskId is provided.
…task model The query column list and the route zod schema were two hand-maintained copies of the same shape. Derive one latestSyncTaskSchema from source_taskDbModel, select * and let it trim the result, and reuse it as the route response schema. JITSU-91
There was a problem hiding this comment.
Reviewed the service extraction for sync/debug/reports routes and spot-checked the behavior-sensitive changes around workspace scoping, source check dispatch, and report payload shapes.\n\nI did not find new actionable correctness/security regressions beyond the already-open thread on the logs URL when is provided without .
There was a problem hiding this comment.
Reviewed the service extraction for sync/debug/reports routes and spot-checked the behavior-sensitive changes around workspace scoping, source check dispatch, and report payload shapes.
I did not find new actionable correctness/security regressions beyond the already-open thread on the sources/tasks logs URL when taskId is provided without syncId.
A taskId-only GET /sources/tasks lookup omits syncId, so the logs link carried syncId=undefined. Use the returned task's own sync_id instead. JITSU-91
There was a problem hiding this comment.
Reviewed the service/route refactor across sources/check/discover/spec/run/state/tasks plus reports/debug endpoints, and validated the key behavior changes in and . I did not find any new correctness, security, or user-visible regression issues beyond the already-resolved thread.
There was a problem hiding this comment.
Reviewed the service/route refactor across sources/check/discover/spec/run/state/tasks plus reports/debug endpoints, and validated key behavior changes in sync-service and reports-service. I did not find any new correctness, security, or user-visible regression issues beyond the already-resolved thread.
|
🚀 |
2 participants
What
Migrate the sync/debug/reports route handlers onto the shared service classes extracted for the MCP server in #1385, so HTTP and MCP share one implementation of workspace scoping, secret unmasking, and syncctl/bulker/functions dispatch instead of two paths that drift. Same play as
JITSU-67(config routes →ConfigObjectsService).sources/runSyncService.runSync(scheduler-bearer special case + logging stay in the route)sources/cancelcancelSyncsources/tasksGET/POSTlistSyncTasks/latestSyncTasks(route adds thelogsURL)sources/specgetConnectorSpecsources/discoverdiscoverStreamssources/checkPOST/GETtriggerSourceCheck/getSourceCheckResultsources/stateGET/POSTgetSyncState/resetSyncState+setSyncStatesources/logsfindSync(only the ownership lookup moves; gzip streaming stays)config/[type]/testDebugService.testConnectionfunction/runDebugService.runFunctionprofile-builder/runDebugService.runProfileBuilderreports/event-statReportsService.eventStat(route keepsgetWorkspaceslug resolution)reports/sync-statReportsService.syncStat(same)profile-builder/stats.tsfrom the issue no longer exists — removed in #1195;ReportsService.profileBuilderStatsmirrors the dead route, nothing to migrate.Transport stays in the routes (
createRouteschemas, OpenAPIexpandmetadata, 20mbbodyParserconfigs,maxDuration).lib/server/route-services.tsprovides lazy per-process service instances bound to the db/clickhouse singletons so route files don't repeat the wiring. Net −613 lines.Service gaps closed for parity
The routes had capabilities the services lacked:
SyncService.latestSyncTasks— thetasksPOST aggregation (latest non-skipped task per sync)SyncService.setSyncState— the state-editor upsert (statePOST with a non-empty body)SyncService.triggerSourceCheckalso accepts a caller-supplied unsavedconfig+ client-generatedstorageKey(service-editor test flow), with secret unmasking; MCP call site updatedSyncService.runSyncpassestaskId;DebugService.runFunctionpassesfunctionNameReportsService.eventStatreturnssrcSize(the client-sideReport.parserequires it)Behavior changes
sources/checkGET/POST now require the exact${workspaceId}_storage-key segment — a barestartsWith(workspaceId)let workspacews1poll keys ofws10(hardened in the service in a1e95e6; the route gets it by delegating).sync-statcounts distinct(fromId, toId)tuples instead of collision-prone string concatenation.sources/cancelreturns the service's{ ok: true, status }instead of{ ok: false, pending: true, startedAt }— the UI only reads.error.sources/discoverkeeps its historical200 { ok: false, error }for unknown services via anApiErroradapter in the route (the service 404s).sources/tasksGET withoutsyncIdno longer fails spuriously in a workspace with no syncs.sources/tasksGET withtaskIdbuilds thelogsURL from the returned task's ownsync_id, not the request'ssyncId— a task-only lookup omitssyncId, so the old link carriedsyncId=undefinedand the logs endpoint (which requires a realsyncId) 404'd. Pre-existing latent bug, hardened here.profile-builder/runnow 404s on an unknown profile builder (previously forwarded blindly) and defaultssettings/versionfrom the stored builder.reports/event-stat: whenendis given withoutstart,startdefaults toend − 1 month(wasnow − 1 month), matchingsync-stat.Testing
New integration tests (testcontainers Postgres/ClickHouse + MSW syncctl): aggregated-tasks scoping, config-mode source check with unmasking, the
ws1/ws10prefix hardening, state upsert, andsrcSizein the event-stat wire shape.JITSU-91