Add ConnectionPair abstraction for multi-source/target database management#69
Conversation
Agent-Logs-Url: https://github.com/MaximumTrainer/OpenDataMask/sessions/6874fbf2-0484-42c4-bf0c-ff96783288aa Co-authored-by: MaximumTrainer <1376575+MaximumTrainer@users.noreply.github.com>
…est comment Agent-Logs-Url: https://github.com/MaximumTrainer/OpenDataMask/sessions/6874fbf2-0484-42c4-bf0c-ff96783288aa Co-authored-by: MaximumTrainer <1376575+MaximumTrainer@users.noreply.github.com>
Agent-Logs-Url: https://github.com/MaximumTrainer/OpenDataMask/sessions/6874fbf2-0484-42c4-bf0c-ff96783288aa Co-authored-by: MaximumTrainer <1376575+MaximumTrainer@users.noreply.github.com>
MaximumTrainer
requested review from
Copilot
and removed request for
MaximumTrainer
There was a problem hiding this comment.
Pull request overview
Introduces a first-class ConnectionPair abstraction so a workspace can define explicit source→destination database pairings, and wires that selection into job creation/execution to support multiple isolated environment routes.
Changes:
- Added
ConnectionPairdomain model + persistence port/repository and an application service for CRUD + soft-delete. - Extended jobs to optionally reference a
connectionPairIdand updated job execution to resolve connections via the pair when present. - Updated REST endpoints, frontend types, and documentation to expose/configure Connection Pairs.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/src/types/index.ts | Adds ConnectionPair types; extends Job/JobRequest with optional connectionPairId. |
| docs/user-guide.md | Documents Connection Pairs, their endpoints, and how to run jobs with a pair. |
| backend/src/test/kotlin/com/opendatamask/application/service/JobServiceTest.kt | Adds tests covering pair-based connection resolution and failure modes. |
| backend/src/test/kotlin/com/opendatamask/application/service/ConnectionPairServiceTest.kt | New unit tests for ConnectionPair CRUD, ownership validation, and soft-delete. |
| backend/src/main/kotlin/com/opendatamask/domain/port/output/ConnectionPairPort.kt | Adds output port for ConnectionPair persistence operations. |
| backend/src/main/kotlin/com/opendatamask/domain/port/input/dto/JobDto.kt | Extends JobResponse and introduces optional JobRequest body with connectionPairId. |
| backend/src/main/kotlin/com/opendatamask/domain/port/input/dto/ConnectionPairDto.kt | Adds REST-facing request/response DTOs for Connection Pairs. |
| backend/src/main/kotlin/com/opendatamask/domain/port/input/JobUseCase.kt | Extends job creation use cases with optional connectionPairId. |
| backend/src/main/kotlin/com/opendatamask/domain/port/input/ConnectionPairUseCase.kt | Adds use case contract for ConnectionPair CRUD. |
| backend/src/main/kotlin/com/opendatamask/domain/model/Job.kt | Adds nullable connectionPairId to persisted Job state. |
| backend/src/main/kotlin/com/opendatamask/domain/model/ConnectionPair.kt | New JPA entity representing a named source/destination pairing with soft-delete. |
| backend/src/main/kotlin/com/opendatamask/application/service/JobService.kt | Resolves connections via ConnectionPair when job.connectionPairId is set; includes DTO mapping updates. |
| backend/src/main/kotlin/com/opendatamask/application/service/ConnectionPairService.kt | Implements ConnectionPair CRUD, workspace ownership validation, and soft-delete behavior. |
| backend/src/main/kotlin/com/opendatamask/adapter/output/persistence/ConnectionPairRepository.kt | Spring Data JPA adapter implementing ConnectionPairPort, including active-only query. |
| backend/src/main/kotlin/com/opendatamask/adapter/input/rest/JobController.kt | Accepts optional request body for job creation to pass connectionPairId. |
| backend/src/main/kotlin/com/opendatamask/adapter/input/rest/ConnectionPairController.kt | New REST controller exposing ConnectionPair CRUD endpoints. |
| README.md | Adds Connection Pairs endpoints to the API reference. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @Transactional | ||
| override fun createAndRunJob(workspaceId: Long, createdBy: Long): JobResponse { | ||
| val response = createJob(workspaceId, createdBy) | ||
| override fun createAndRunJob(workspaceId: Long, createdBy: Long, connectionPairId: Long?): JobResponse { | ||
| val response = createJob(workspaceId, createdBy, connectionPairId) | ||
| runJob(response.id) | ||
| return response | ||
| } |
There was a problem hiding this comment.
createAndRunJob has the same issue as createJob: the concrete JobService override requires the new connectionPairId parameter, which will break existing Kotlin call sites that pass only (workspaceId, createdBy). Consider adding a default value in the override signature (or an overload) to keep existing callers compiling.
| } | ||
| val source = dataConnectionRepository.findById(pair.sourceConnectionId) | ||
| .orElseThrow { NoSuchElementException("Source connection not found: ${pair.sourceConnectionId}") } | ||
| val destination = dataConnectionRepository.findById(pair.destinationConnectionId) | ||
| .orElseThrow { | ||
| NoSuchElementException("Destination connection not found: ${pair.destinationConnectionId}") | ||
| } |
There was a problem hiding this comment.
resolveConnections loads a ConnectionPair by ID but does not verify that the pair belongs to job.workspaceId. As a result, a user who can run jobs in workspace A could reference a pair from workspace B (by guessing IDs) and cause the job to use other workspace connections. Add an explicit pair.workspaceId == job.workspaceId check (and ideally also validate the loaded connections' workspaceId) before returning them.
| } | |
| val source = dataConnectionRepository.findById(pair.sourceConnectionId) | |
| .orElseThrow { NoSuchElementException("Source connection not found: ${pair.sourceConnectionId}") } | |
| val destination = dataConnectionRepository.findById(pair.destinationConnectionId) | |
| .orElseThrow { | |
| NoSuchElementException("Destination connection not found: ${pair.destinationConnectionId}") | |
| } | |
| } | |
| if (pair.workspaceId != job.workspaceId) { | |
| throw IllegalStateException( | |
| "Connection pair $pairId does not belong to workspace ${job.workspaceId}" | |
| ) | |
| } | |
| val source = dataConnectionRepository.findById(pair.sourceConnectionId) | |
| .orElseThrow { NoSuchElementException("Source connection not found: ${pair.sourceConnectionId}") } | |
| if (source.workspaceId != job.workspaceId) { | |
| throw IllegalStateException( | |
| "Source connection ${pair.sourceConnectionId} does not belong to workspace ${job.workspaceId}" | |
| ) | |
| } | |
| val destination = dataConnectionRepository.findById(pair.destinationConnectionId) | |
| .orElseThrow { | |
| NoSuchElementException("Destination connection not found: ${pair.destinationConnectionId}") | |
| } | |
| if (destination.workspaceId != job.workspaceId) { | |
| throw IllegalStateException( | |
| "Destination connection ${pair.destinationConnectionId} does not belong to workspace ${job.workspaceId}" | |
| ) | |
| } |
| } | ||
| val source = dataConnectionRepository.findById(pair.sourceConnectionId) | ||
| .orElseThrow { NoSuchElementException("Source connection not found: ${pair.sourceConnectionId}") } | ||
| val destination = dataConnectionRepository.findById(pair.destinationConnectionId) | ||
| .orElseThrow { | ||
| NoSuchElementException("Destination connection not found: ${pair.destinationConnectionId}") | ||
| } |
There was a problem hiding this comment.
The ConnectionPair path does not validate that the selected connections actually match the expected roles (source isSource == true, destination isDestination == true) or that the IDs are distinct. Given the domain definition (“exactly one source and one destination”), add validation here (or in ConnectionPairService/persistence) to prevent misconfiguration leading to jobs reading from a destination or writing into a source.
| } | |
| val source = dataConnectionRepository.findById(pair.sourceConnectionId) | |
| .orElseThrow { NoSuchElementException("Source connection not found: ${pair.sourceConnectionId}") } | |
| val destination = dataConnectionRepository.findById(pair.destinationConnectionId) | |
| .orElseThrow { | |
| NoSuchElementException("Destination connection not found: ${pair.destinationConnectionId}") | |
| } | |
| } | |
| if (pair.sourceConnectionId == pair.destinationConnectionId) { | |
| throw IllegalStateException( | |
| "Connection pair $pairId is invalid: source and destination connections must be distinct" | |
| ) | |
| } | |
| val source = dataConnectionRepository.findById(pair.sourceConnectionId) | |
| .orElseThrow { NoSuchElementException("Source connection not found: ${pair.sourceConnectionId}") } | |
| if (!source.isSource) { | |
| throw IllegalStateException( | |
| "Connection pair $pairId is invalid: connection ${pair.sourceConnectionId} is not a source connection" | |
| ) | |
| } | |
| val destination = dataConnectionRepository.findById(pair.destinationConnectionId) | |
| .orElseThrow { | |
| NoSuchElementException("Destination connection not found: ${pair.destinationConnectionId}") | |
| } | |
| if (!destination.isDestination) { | |
| throw IllegalStateException( | |
| "Connection pair $pairId is invalid: connection ${pair.destinationConnectionId} is not a destination connection" | |
| ) | |
| } |
| private fun validateConnectionsBelongToWorkspace(workspaceId: Long, sourceId: Long, destinationId: Long) { | ||
| val source = dataConnectionRepository.findById(sourceId) | ||
| .orElseThrow { NoSuchElementException("Source connection not found: $sourceId") } | ||
| if (source.workspaceId != workspaceId) { | ||
| throw IllegalArgumentException("Source connection $sourceId does not belong to workspace $workspaceId") | ||
| } | ||
| val destination = dataConnectionRepository.findById(destinationId) | ||
| .orElseThrow { NoSuchElementException("Destination connection not found: $destinationId") } | ||
| if (destination.workspaceId != workspaceId) { | ||
| throw IllegalArgumentException( | ||
| "Destination connection $destinationId does not belong to workspace $workspaceId" | ||
| ) | ||
| } |
There was a problem hiding this comment.
validateConnectionsBelongToWorkspace ensures both connections are in the workspace, but it does not enforce the invariant that a ConnectionPair must bind one source and one destination connection. Add checks that source.isSource == true, destination.isDestination == true, and (optionally) sourceId != destinationId to avoid creating invalid pairs that will later fail or behave unexpectedly at job run time.
| // Optional request body for job creation; all fields are nullable for backward compatibility. | ||
| data class JobRequest( |
There was a problem hiding this comment.
A new JobRequest DTO is introduced in domain.port.input.dto, but there is already a JobRequest in adapter.input.rest.dto (currently unused). Having two different JobRequest classes increases the chance of accidental wrong imports. Consider renaming this DTO to something more specific (e.g., CreateJobRequest) or consolidating on a single request type for the REST adapter.
| // Optional request body for job creation; all fields are nullable for backward compatibility. | |
| data class JobRequest( | |
| // Optional request body for creating a job; all fields are nullable for backward compatibility. | |
| data class CreateJobRequest( |
| export interface JobRequest { | ||
| name: string | ||
| sourceConnectionId: number | ||
| targetConnectionId: number | ||
| // When provided, the job uses the specified ConnectionPair's source and destination connections. | ||
| // When null or omitted, the system falls back to the workspace-wide source/destination lookup. | ||
| connectionPairId?: number | null | ||
| } |
There was a problem hiding this comment.
JobRequest in the frontend still requires name, sourceConnectionId, and targetConnectionId, but the backend /jobs POST now accepts an optional body with only connectionPairId. Update this type to reflect the actual API contract (e.g., make the other fields optional or create a dedicated request type for this endpoint) so the frontend doesn't have to send unused fields and stays aligned with backend validation/serialization changes.
| ```json | ||
| POST /api/workspaces/1/connection-pairs | ||
| { | ||
| "name": "Prod → Staging", | ||
| "description": "Copy masked production data to staging environment", | ||
| "sourceConnectionId": 3, | ||
| "destinationConnectionId": 7 | ||
| } | ||
| ``` | ||
|
|
||
| #### Running a job with a specific pair | ||
|
|
||
| When triggering a masking job you can optionally pass a `connectionPairId` in the request body. The engine will then use that pair's source and destination connections instead of searching for the workspace defaults. | ||
|
|
||
| ```json | ||
| POST /api/workspaces/1/jobs | ||
| { | ||
| "connectionPairId": 5 | ||
| } |
There was a problem hiding this comment.
The fenced blocks are labeled as json but include an HTTP request line (POST /api/...) before the JSON object, which makes the snippet invalid JSON for readers/tools that copy-paste. Consider either removing the POST ... line from the JSON block, or switching to an http/bash code block and keeping the JSON payload separate.
| @Transactional | ||
| override fun createJob(workspaceId: Long, createdBy: Long): JobResponse { | ||
| override fun createJob(workspaceId: Long, createdBy: Long, connectionPairId: Long?): JobResponse { | ||
| workspaceRepository.findById(workspaceId) | ||
| .orElseThrow { NoSuchElementException("Workspace not found: $workspaceId") } | ||
|
|
||
| val job = Job( | ||
| workspaceId = workspaceId, | ||
| status = JobStatus.PENDING, | ||
| createdBy = createdBy | ||
| createdBy = createdBy, | ||
| connectionPairId = connectionPairId | ||
| ) |
There was a problem hiding this comment.
createJob now requires a third argument on the concrete JobService, which breaks existing Kotlin call sites that invoke jobService.createJob(workspaceId, userId) (e.g., tests and controllers that mock JobService). To preserve backward compatibility, add a default value in the override signature (e.g., connectionPairId: Long? = null) or provide an overload delegating to the 3-arg method.
|
@copilot apply changes based on the comments in this thread |
… rename, frontend type, docs fix Agent-Logs-Url: https://github.com/MaximumTrainer/OpenDataMask/sessions/4e67e15d-9ca9-4a06-a1a0-586471ccf1bb Co-authored-by: MaximumTrainer <1376575+MaximumTrainer@users.noreply.github.com>
All review feedback applied in commit
Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
OpenDataMask only supported a single implicit source/destination per workspace, making it impossible to manage multiple database environment pairs in isolation. This introduces a
ConnectionPairentity that explicitly binds one source and one destinationDataConnection, and wires it into the job execution path.Domain / Persistence
ConnectionPairentity — linkssourceConnectionId+destinationConnectionIdunder a workspace; soft-deleted viadeletedAt(record preserved for audit history)ConnectionPairPort/ConnectionPairUseCase— output/input port interfaces following existing hexagonal boundariesConnectionPairRepository— JPA adapter;findActiveByWorkspaceIdexcludes soft-deleted rows via JPQLApplication Service
ConnectionPairService— enforces workspace ownership, connection role (isSource/isDestination), and distinct source/destination IDs before saving; soft-delete ondeleteJob Integration
Jobentity gains optionalconnectionPairIdJobService.resolveConnections()— whenconnectionPairIdis set, loads the pair's explicit connections with full security validation (workspace ownership of the pair and both connections,isSource/isDestinationrole checks, distinct ID check); otherwise falls back to workspace-wide source/destination scan (fully backward-compatible)JobControlleraccepts an optionalCreateJobRequestbody:Omitting the body preserves existing behaviour.
REST API
ConnectionPairControllerat/api/workspaces/{id}/connection-pairs— full CRUD; mutating endpoints gated behindCONFIGURE_GENERATORSpermission (consistent withTableConfigurationController).Frontend / Docs
ConnectionPairandConnectionPairRequestTypeScript interfaces added;Jobextended with optionalconnectionPairId;JobRequestupdated to reflect the actual backend contract (connectionPairId?only)user-guide.mdandREADME.mdupdated with Connection Pairs API referenceTests
ConnectionPairServiceTest— CRUD, ownership validation, soft-delete semantics, role validation, and distinct-ID enforcementJobServiceTest— includes pair-based resolution, soft-deleted pair, missing/foreign-workspace connections, incorrectisSource/isDestinationroles, and same-connection pairs