Conversation
3161b49 to
0b73d60
Compare
| Request body: | ||
|
|
||
| ``` | ||
| { |
There was a problem hiding this comment.
If it is always module, then why call it node? can't we use sourceModuleSystemId
8784dc2 to
32d0f85
Compare
![qualcomm-ai-code-review-assistant[bot]](https://avatars.githubusercontent.com/in/4128059?s=60&v=4){kind=small}
| `DataLink for ports (${BinaryUtils.toHexString(srcPortId)}, ${BinaryUtils.toHexString(dstPortId)}) already exists.`, | ||
| ); | ||
| } | ||
|
|
||
| // FR-DL-09: linkType derivation | ||
| // TODO: load srcModule.subgraphSystemId and dstModule.subgraphSystemId from overlay | ||
| const srcSubgraphId = 0; | ||
| const dstSubgraphId = 0; | ||
| const linkType = this.deriveLinkType( | ||
| command.isInterUsecase, | ||
| srcSubgraphId, |
There was a problem hiding this comment.
[Functionality - High] Hardcoded subgraph IDs will cause incorrect linkType derivation
In CreateDataLinkHandler.handle(), the subgraph IDs are hardcoded to 0 instead of being loaded from the database. This will cause the deriveLinkType() method to always return LINK_TYPE.IntraSubgraph when isInterUsecase is not explicitly set to true, even when the modules are in different subgraphs.
The TODO comment on line 81 acknowledges this, but this is a critical functional issue that will cause incorrect link type classification and potentially break validation rules.
Impact: Links between modules in different subgraphs will be incorrectly classified as INTRA_SUBGRAPH instead of INTRA_USECASE, violating business rules.
Fixed Code Snippet
// FR-DL-09: linkType derivation
const moduleRepo = uow.getModuleRepository();
const srcModule = await moduleRepo.findBySystemId(srcModuleId, fileSystemId);
const dstModule = await moduleRepo.findBySystemId(dstModuleId, fileSystemId);
if (!srcModule || !dstModule) {
throw new ResourceNotFoundException(
`Module not found: ${!srcModule ? BinaryUtils.toHexString(srcModuleId) : BinaryUtils.toHexString(dstModuleId)}`
);
}
const srcSubgraphId = srcModule.subgraphSystemId;
const dstSubgraphId = dstModule.subgraphSystemId;
const linkType = this.deriveLinkType(
command.isInterUsecase,
srcSubgraphId,
dstSubgraphId,
);| sessionId, | ||
| ); | ||
| } | ||
|
|
||
| async createDataLink( | ||
| dataLink: DataLink, | ||
| boundaryPortPayloads: BoundaryPortPayload[], | ||
| options?: EditOptions, | ||
| ): Promise<void> { | ||
| const {session, groupId} = this.uow.getWriteContext(); | ||
| const fileSystemId = dataLink.fileSystemId; |
There was a problem hiding this comment.
[Error Handling - High] Missing error handling in createDataLink repository method
The TypeOrmDataLinkRepository.createDataLink() method performs multiple database writes in sequence but lacks proper error handling. If any of the write operations fail after some have succeeded, the method will throw an exception but won't roll back the partial changes, potentially leaving the database in an inconsistent state.
While the handler has transaction management, the repository method should handle its own errors gracefully and provide meaningful error messages.
Impact: Database inconsistency if writes fail partway through the operation.
Fixed Code Snippet
async createDataLink(
dataLink: DataLink,
boundaryPortPayloads: BoundaryPortPayload[],
options?: EditOptions,
): Promise<void> {
const {session, groupId} = this.uow.getWriteContext();
const fileSystemId = dataLink.fileSystemId;
const writer = this.requireWriter();
try {
// Write boundary nodes
for (const bp of boundaryPortPayloads) {
await writer.writeCreate(
{
targetTable: ENTITY_NAMES.Node,
targetSystemId: bp.nodeSystemId,
aggregateId: dataLink.systemId,
payload: {
type: 'subsystem',
parentId: bp.nodeParentId ?? null,
fileSystemId: bp.fileSystemId,
},
...options,
},
session.sessionId,
groupId,
this.manager,
);
}
// Write boundary ports
for (const bp of boundaryPortPayloads) {
await writer.writeCreate(
{
targetTable: ENTITY_NAMES.DataPort,
targetSystemId: bp.portSystemId,
aggregateId: dataLink.systemId,
payload: {
dataPortId: bp.dataPortId,
portIoType: bp.portIoType,
isStatic: false,
name: '',
nodeSystemId: bp.nodeSystemId,
fileSystemId: bp.fileSystemId,
},
...options,
},
session.sessionId,
groupId,
this.manager,
);
}
// Write DataLink
await writer.writeCreate(
{
targetTable: ENTITY_NAMES.DataLink,
targetSystemId: dataLink.systemId,
aggregateId: dataLink.systemId,
payload: {
sourceNodeSystemId: dataLink.sourceNodeSystemId,
destinationNodeSystemId: dataLink.destinationNodeSystemId,
sourcePortSystemId: dataLink.sourcePortSystemId,
destinationPortSystemId: dataLink.destinationPortSystemId,
linkType: dataLink.linkType,
sourceSubgraphSystemId: dataLink.sourceSubgraphSystemId,
destSubgraphSystemId: dataLink.destSubgraphSystemId,
isEc: dataLink.isEc ?? null,
fileSystemId,
},
...options,
},
session.sessionId,
groupId,
this.manager,
);
// Write SubsystemDataLinks
for (const sls of dataLink.subsystemDataLinks) {
await writer.writeCreate(
{
targetTable: ENTITY_NAMES.SubsystemDataLink,
targetSystemId: sls.systemId,
aggregateId: dataLink.systemId,
payload: {
sourceNodeSystemId: sls.sourceNodeSystemId,
destinationNodeSystemId: sls.destinationNodeSystemId,
sourcePortSystemId: sls.sourcePortSystemId,
destinationPortSystemId: sls.destinationPortSystemId,
dataLinkSystemId: sls.dataLinkSystemId,
fileSystemId,
},
...options,
},
session.sessionId,
groupId,
this.manager,
);
}
} catch (error) {
throw new Error(
`Failed to create DataLink ${dataLink.systemId}: ${error instanceof Error ? error.message : String(error)}`
);
}
}
32d0f85 to
0b04126
Compare
There was a problem hiding this comment.
Files Not Fully Analyzed
- docs/data-links/design/data-links-post-design.md - File extension .md not supported
- docs/data-links/plans/2026-08-18-data-links-post-fix-plan.md - File extension .md not supported
- docs/data-links/plans/data-links-post.md - File extension .md not supported
- packages/api/src/infrastructure-wrapper/filters/all-exceptions.filter.ts - Full file content skipped due to token limit
- packages/api/src/infrastructure-wrapper/persistence/unit-of-work/typeorm-unit-of-work.ts - Full file content skipped due to token limit
- packages/api/src/presentation/rest/common/swagger-doc/dto-examples/usecase-api-examples.ts - Full file content skipped due to token limit
- packages/api/src/presentation/rest/modules/spf-module/spf-module.controller.ts - Full file content skipped due to token limit
- packages/core/src/application/orchestration/cqrs/registries/command-handler-registry.ts - Full file content skipped due to token limit
- packages/core/src/application/ports/persistence/query-services/usecase/query-models/subsystem-data-link-read-model.ts - Full file content skipped due to token limit
- packages/core/src/application/ports/persistence/query-services/usecase/query-models/usecase-components-with-subsystems-read-model.ts - Full file content skipped due to token limit
- packages/core/src/application/ports/persistence/repositories/module/module.repository.ts - Full file content skipped due to token limit
- packages/core/src/application/usecase-designer/data-links/create/create-data-link-with-subsystems.command.ts - Full file content skipped due to token limit
- packages/core/src/application/usecase-designer/data-links/create/create-data-link.command.ts - Full file content skipped due to token limit
- packages/core/src/index.ts - Full file content skipped due to token limit
- packages/core/src/shared/exceptions/conflict.exception.ts - Full file content skipped due to token limit
- packages/core/src/shared/exceptions/index.ts - Full file content skipped due to token limit
- packages/core/tests/unit/application/file-operations/upload-file/services/entity-builders/subsystem-builder.spec.ts - Full file content skipped due to token limit
- packages/infrastructure/persistence/src/persistence-typeorm-sqllite/repositories/module/module.repository.ts - Full file content skipped due to token limit
Qualcomm AI Review
| fileSystemId: number, | ||
| uow: UnitOfWork, | ||
| subsystemRepo: ReturnType<UnitOfWork['getSubsystemRepository']>, | ||
| ): Promise<ComponentCollectionWithSubsystemsDto> { | ||
| // Branch B (FR-DLS-11): at least one endpoint is a subsystem | ||
| if (command.isInterUsecase !== undefined || command.isEc !== undefined) { | ||
| throw new DomainRuleViolationException([{ | ||
| code: 'INVALID_FLAGS_FOR_SUBSYSTEM', | ||
| message: 'isInterUsecase and isEc must not be provided when a subsystem endpoint is involved.', | ||
| severity: IssueSeverity.Error, | ||
| }]); |
There was a problem hiding this comment.
[Functionality] Missing null check for module lookup in subsystem endpoint validation (Medium Severity)
In create-data-link-with-subsystems.handler.ts, when Branch B (subsystem endpoint) is triggered, the code doesn't validate whether the non-subsystem endpoint is actually a valid module. If a user provides a subsystem ID as source and an invalid/non-existent ID as destination (that's neither a module nor a subsystem), the validation will fail with a generic "port not found" error instead of a more specific "node not found" error.
The issue occurs because in Branch B, the code only checks subsystemExists() for both endpoints but doesn't verify that the non-subsystem endpoint is actually a valid module node. This could lead to confusing error messages.
Impact: Users may receive misleading error messages when providing invalid node IDs in subsystem link creation scenarios.
Fixed Code Snippet
private async handleBranchB(
command: CreateDataLinkWithSubsystemsCommand,
srcNodeId: number,
dstNodeId: number,
srcPortId: number,
dstPortId: number,
srcIsSubsystem: boolean,
dstIsSubsystem: boolean,
fileSystemId: number,
uow: UnitOfWork,
subsystemRepo: ReturnType<UnitOfWork['getSubsystemRepository']>,
): Promise<ComponentCollectionWithSubsystemsDto> {
// Branch B (FR-DLS-11): at least one endpoint is a subsystem
if (command.isInterUsecase !== undefined || command.isEc !== undefined) {
throw new DomainRuleViolationException([{
code: 'INVALID_FLAGS_FOR_SUBSYSTEM',
message: 'isInterUsecase and isEc must not be provided when a subsystem endpoint is involved.',
severity: IssueSeverity.Error,
}]);
}
// Validate that non-subsystem endpoints are valid modules
const moduleRepo = uow.getModuleRepository();
if (!srcIsSubsystem) {
const srcModule = await moduleRepo.findModulePortsForLink(srcNodeId, fileSystemId);
if (srcModule === null) {
throw new ResourceNotFoundException(
`Source node ${BinaryUtils.toHexString(srcNodeId)} not found or is not a valid module.`,
);
}
}
if (!dstIsSubsystem) {
const dstModule = await moduleRepo.findModulePortsForLink(dstNodeId, fileSystemId);
if (dstModule === null) {
throw new ResourceNotFoundException(
`Destination node ${BinaryUtils.toHexString(dstNodeId)} not found or is not a valid module.`,
);
}
}
// FR-DLS-03 + FR-DLS-08 + FR-DLS-07: validate subsystem-side ports
if (srcIsSubsystem) {
const srcPortType = await subsystemRepo.getPortIoType(srcPortId, fileSystemId);
// ... rest of validation
}
// ... rest of the method
}
0b04126 to
c119049
Compare
There was a problem hiding this comment.
Files Not Fully Analyzed
- docs/data-links/design/data-links-post-design.md - File extension .md not supported
- packages/api/src/infrastructure-wrapper/filters/all-exceptions.filter.ts - Full file content skipped due to token limit
- packages/api/src/infrastructure-wrapper/persistence/unit-of-work/typeorm-unit-of-work.ts - Full file content skipped due to token limit
- packages/api/src/presentation/rest/common/swagger-doc/dto-examples/usecase-api-examples.ts - Full file content skipped due to token limit
- packages/core/src/application/orchestration/cqrs/registries/command-handler-registry.ts - Full file content skipped due to token limit
- packages/core/src/application/ports/persistence/query-services/usecase/query-models/subsystem-data-link-read-model.ts - Full file content skipped due to token limit
- packages/core/src/application/ports/persistence/query-services/usecase/query-models/usecase-components-with-subsystems-read-model.ts - Full file content skipped due to token limit
- packages/core/src/application/usecase-designer/data-links/create/create-data-link.command.ts - Full file content skipped due to token limit
- packages/core/src/index.ts - Full file content skipped due to token limit
- packages/core/src/shared/exceptions/index.ts - Full file content skipped due to token limit
- packages/core/tests/unit/application/file-operations/upload-file/services/entity-builders/subsystem-builder.spec.ts - Full file content skipped due to token limit
- packages/infrastructure/persistence/src/persistence-typeorm-sqllite/repositories/module/module.repository.ts - Full file content skipped due to token limit
Qualcomm AI Review
| aggregateId: number, | ||
| payload: Record<string, unknown>, | ||
| options?: EditOptions, | ||
| ): Promise<void> { | ||
| const {session, groupId} = this.uow.getWriteContext(); | ||
| const writer = this.requireWriter(); | ||
| // eslint-disable-next-line custom/no-raw-persistence-queries -- conditional UPDATE with IS NULL on valid_until cannot be expressed with TypeORM QueryBuilder | ||
| await this.manager.query( | ||
| `UPDATE edit_actions SET valid_until = $1 WHERE session_id = $2 AND target_system_id = $3 AND field_path IS NULL AND valid_until IS NULL`, | ||
| [new Date().toISOString(), session.sessionId, systemId], | ||
| ); |
There was a problem hiding this comment.
[SECURITY][High] SQL Injection Vulnerability in reactivateDataLink
The reactivateDataLink method in TypeOrmDataLinkRepository uses raw SQL with string interpolation for the valid_until timestamp, which could be exploited if the date format is manipulated. While the current implementation uses new Date().toISOString(), this pattern is risky and violates the principle of using parameterized queries consistently.
Issue Location: packages/infrastructure/persistence/src/persistence-typeorm-sqllite/repositories/data-link/data-link.repository.ts:217-220
Risk: If the date generation logic is ever modified or if there's any way to influence the timestamp value, this could lead to SQL injection.
Fixed Code Snippet
async reactivateDataLink(
systemId: number,
aggregateId: number,
payload: Record<string, unknown>,
options?: EditOptions,
): Promise<void> {
const {session, groupId} = this.uow.getWriteContext();
const writer = this.requireWriter();
const validUntil = new Date().toISOString();
// Use parameterized query for all values
await this.manager.query(
`UPDATE edit_actions SET valid_until = $1 WHERE session_id = $2 AND target_system_id = $3 AND field_path IS NULL AND valid_until IS NULL`,
[validUntil, session.sessionId, systemId],
);
await writer.writeCreate(
{
targetTable: ENTITY_NAMES.DataLink,
targetSystemId: systemId,
aggregateId,
payload,
...options,
},
session.sessionId,
groupId,
this.manager,
);
}The fix extracts the timestamp to a variable first, making the code more maintainable and ensuring the parameterized query pattern is clear.
| // FR-DLS-14: DataLink persisted internally but excluded from response | ||
| return emptyCollection(slsSegments.map(sls => mapSubsystemDataLink(sls))); | ||
| } | ||
|
|
||
| private async handleBranchB( | ||
| command: CreateDataLinkWithSubsystemsCommand, | ||
| srcNodeId: number, | ||
| dstNodeId: number, | ||
| srcPortId: number, | ||
| dstPortId: number, | ||
| srcIsSubsystem: boolean, |
There was a problem hiding this comment.
[FUNCTIONALITY][High] Missing Transaction Rollback in Branch B Error Path
In CreateDataLinkWithSubsystemsHandler.handleBranchB, if an exception occurs after validation but before commit, the transaction is not rolled back because the catch block in the main handle method only checks uow.isInTransaction(). However, if the transaction was started but an error occurs in handleBranchB, the rollback won't happen since handleBranchB doesn't have its own try-catch.
Issue Location: packages/core/src/application/usecase-designer/data-links/create/create-data-link-with-subsystems.handler.ts:270-355
Risk: Database inconsistency if an error occurs during Branch B processing (subsystem endpoint creation). The transaction would remain open and uncommitted.
Fixed Code Snippet
private async handleBranchB(
command: CreateDataLinkWithSubsystemsCommand,
srcNodeId: number,
dstNodeId: number,
srcPortId: number,
dstPortId: number,
srcIsSubsystem: boolean,
dstIsSubsystem: boolean,
fileSystemId: number,
uow: UnitOfWork,
subsystemRepo: ReturnType<UnitOfWork['getSubsystemRepository']>,
): Promise<ComponentCollectionWithSubsystemsDto> {
try {
// Branch B (FR-DLS-11): at least one endpoint is a subsystem
if (command.isInterUsecase !== undefined || command.isEc !== undefined) {
throw new DomainRuleViolationException([{
code: 'INVALID_FLAGS_FOR_SUBSYSTEM',
message: 'isInterUsecase and isEc must not be provided when a subsystem endpoint is involved.',
severity: IssueSeverity.Error,
}]);
}
// ... rest of validation and creation logic ...
const dlEditRepo = uow.getDataLinkRepository();
await dlEditRepo.createSubsystemDataLink(sls);
await uow.commit();
return emptyCollection([mapSubsystemDataLink(sls)]);
} catch (error) {
if (uow.isInTransaction()) {
await uow.rollback();
}
throw error;
}
}Alternatively, the main handle method's catch block should be sufficient if it properly catches all errors from handleBranchB. Verify that the transaction state is correctly managed.
| }]) | ||
| : new ResourceNotFoundException(`Destination module ${BinaryUtils.toHexString(dstNodeId)} not found.`); | ||
| } | ||
|
|
||
| // FR-DLS-06: port ownership | ||
| const srcPort = srcModule.ports.find(p => p.systemId === srcPortId); | ||
| if (!srcPort) { | ||
| throw new DomainRuleViolationException([{ | ||
| code: 'PORT_OWNERSHIP_MISMATCH', | ||
| message: `Port ${BinaryUtils.toHexString(srcPortId)} does not belong to source module — ownership check failed.`, | ||
| severity: IssueSeverity.Error, | ||
| }]); | ||
| } | ||
| const dstPort = dstModule.ports.find(p => p.systemId === dstPortId); | ||
| if (!dstPort) { | ||
| throw new DomainRuleViolationException([{ | ||
| code: 'PORT_OWNERSHIP_MISMATCH', | ||
| message: `Port ${BinaryUtils.toHexString(dstPortId)} does not belong to destination module — ownership check failed.`, | ||
| severity: IssueSeverity.Error, | ||
| }]); | ||
| } | ||
|
|
||
| // FR-DLS-05: port direction | ||
| if (srcPort.portIoType !== PORT_IO_TYPE.Output) { | ||
| throw new DomainRuleViolationException([{ | ||
| code: 'WRONG_PORT_DIRECTION', | ||
| message: `Source port must be OUTPUT, got ${srcPort.portIoType}.`, | ||
| severity: IssueSeverity.Error, | ||
| }]); | ||
| } | ||
| if (dstPort.portIoType !== PORT_IO_TYPE.Input) { | ||
| throw new DomainRuleViolationException([{ | ||
| code: 'WRONG_PORT_DIRECTION', | ||
| message: `Destination port must be INPUT, got ${dstPort.portIoType}.`, | ||
| severity: IssueSeverity.Error, | ||
| }]); | ||
| } | ||
|
|
||
| const dlEditRepo = uow.getDataLinkRepository(); | ||
|
|
||
| const existing = await dlEditRepo.findByPortPair(srcPortId, dstPortId, fileSystemId); | ||
| if (existing !== null && !existing.isDeleted) { | ||
| throw new ConflictException( |
There was a problem hiding this comment.
[MAINTAINABILITY][Medium] Inconsistent Error Handling Between Branch A and Branch B
In CreateDataLinkWithSubsystemsHandler, Branch A (module-to-module) has comprehensive error handling with specific exception types for different validation failures, while Branch B (subsystem endpoint) uses a simpler validation approach. This inconsistency makes the code harder to maintain and could lead to different error experiences for users.
Issue Location: packages/core/src/application/usecase-designer/data-links/create/create-data-link-with-subsystems.handler.ts:121-268 vs 270-355
Recommendation: Standardize the error handling pattern across both branches. Consider extracting common validation logic into separate methods to reduce duplication and ensure consistent error messages.
Example Refactoring:
private validatePortOwnership(
port: {systemId: number; portIoType: PortIoType} | undefined,
portId: number,
nodeType: 'source' | 'destination',
): void {
if (!port) {
throw new DomainRuleViolationException([{
code: 'PORT_OWNERSHIP_MISMATCH',
message: `Port ${BinaryUtils.toHexString(portId)} does not belong to ${nodeType} module — ownership check failed.`,
severity: IssueSeverity.Error,
}]);
}
}
private validatePortDirection(
port: {systemId: number; portIoType: PortIoType},
expectedType: PortIoType,
portId: number,
nodeType: 'source' | 'destination',
): void {
if (port.portIoType !== expectedType) {
throw new DomainRuleViolationException([{
code: 'WRONG_PORT_DIRECTION',
message: `${nodeType === 'source' ? 'Source' : 'Destination'} port must be ${expectedType}, got ${port.portIoType}.`,
severity: IssueSeverity.Error,
}]);
}
}Then use these methods consistently in both branches.
| sourceNodeSystemId: Number(row.sourceNodeSystemId), | ||
| destinationNodeSystemId: Number(row.destinationNodeSystemId), | ||
| sourcePortSystemId: Number(row.sourcePortSystemId), | ||
| destinationPortSystemId: Number(row.destinationPortSystemId), | ||
| dataLinkSystemId: | ||
| row.dataLinkSystemId != null ? Number(row.dataLinkSystemId) : null, | ||
| }), | ||
| ), | ||
| ); | ||
| } catch (error) { |
There was a problem hiding this comment.
[Functionality] Potential null handling inconsistency in SubsystemDataLinkReadModel mapping - Medium Severity
In db-subsystem-query-service.ts, the mapper for SubsystemDataLinkReadModel checks if row.dataLinkSystemId != null before converting to number, but the type definition allows null. However, the SQL query joins with data_links using INNER JOIN, which means dl.system_id should never be null in practice. This creates a potential inconsistency:
- The
INNER JOINguarantees non-null values - The mapper defensively handles null
- The type allows null
This could lead to confusion about when null values are actually expected. Consider either:
- Using
LEFT JOINif null values are legitimately expected (e.g., for unresolved segments) - Removing the null check and updating the type to
numberif the join guarantees non-null - Adding a comment explaining why the defensive null check exists despite the INNER JOIN
Current Implementation:
dataLinkSystemId:
row.dataLinkSystemId != null ? Number(row.dataLinkSystemId) : null,Recommended Fix:
If null values should never occur with the current INNER JOIN, update the type and remove the defensive check:
// In SubsystemDataLinkReadModel type definition
dataLinkSystemId: number; // Remove | null if INNER JOIN guarantees non-null
// In mapper
dataLinkSystemId: Number(row.dataLinkSystemId),Alternatively, if null values are expected for future use cases (e.g., unresolved segments), change to LEFT JOIN and add a comment:
// Use LEFT JOIN if segments can exist without parent links
.leftJoin('data_links', 'dl', 'dl.system_id = sdl.data_link_system_id')
// In mapper with comment
dataLinkSystemId:
row.dataLinkSystemId != null ? Number(row.dataLinkSystemId) : null, // null for unresolved segments
0185409 to
6e06894
Compare
| const dstSubgraphId = dstModule.subgraphSystemId; | ||
| const linkType = deriveLinkType(command.isInterUsecase, srcSubgraphId, dstSubgraphId); | ||
|
|
||
| if (linkType === LINK_TYPE.InterUsecase) { | ||
| const subgraphRepo = uow.getSubgraphRepository(); | ||
| const [srcUsecaseId, dstUsecaseId] = await Promise.all([ | ||
| subgraphRepo.getUsecaseSystemIdForSubgraph(srcSubgraphId, fileSystemId), | ||
| subgraphRepo.getUsecaseSystemIdForSubgraph(dstSubgraphId, fileSystemId), | ||
| ]); | ||
| if (srcUsecaseId !== null && dstUsecaseId !== null && srcUsecaseId === dstUsecaseId) { |
There was a problem hiding this comment.
[FUNCTIONALITY] Missing null safety check in inter-usecase validation - Medium Severity
In create-data-link-with-subsystems.handler.ts at line 222, the code checks if both srcUsecaseId and dstUsecaseId are non-null before comparing them. However, if either value is null, the validation silently passes, which could allow invalid inter-usecase links to be created when subgraph-to-usecase mapping fails.
The same issue exists in create-data-link.handler.ts at line 175.
Issue: If getUsecaseSystemIdForSubgraph() returns null for either subgraph (e.g., orphaned subgraph not associated with any usecase), the validation is skipped entirely. This could lead to data integrity issues where isInterUsecase=true is set but the link doesn't actually cross usecase boundaries.
Recommendation: Add explicit handling for the null case to either throw an error or log a warning when usecase mapping fails.
Fixed Code Snippet
if (linkType === LINK_TYPE.InterUsecase) {
const subgraphRepo = uow.getSubgraphRepository();
const [srcUsecaseId, dstUsecaseId] = await Promise.all([
subgraphRepo.getUsecaseSystemIdForSubgraph(srcSubgraphId, fileSystemId),
subgraphRepo.getUsecaseSystemIdForSubgraph(dstSubgraphId, fileSystemId),
]);
// Validate that both subgraphs belong to usecases
if (srcUsecaseId === null || dstUsecaseId === null) {
throw new DomainRuleViolationException([{
code: 'ORPHANED_SUBGRAPH',
message: 'Cannot create inter-usecase link: one or both subgraphs are not associated with a usecase.',
severity: IssueSeverity.Error,
}]);
}
if (srcUsecaseId === dstUsecaseId) {
throw new DomainRuleViolationException([{
code: 'SAME_USECASE_INTER_USECASE',
message: 'isInterUsecase=true but source and destination belong to the same usecase.',
severity: IssueSeverity.Error,
}]);
}
}| command.isInterUsecase, | ||
| srcSubgraphId, | ||
| dstSubgraphId, | ||
| ); | ||
| if (linkType === LINK_TYPE.InterUsecase) { | ||
| const subgraphRepo = uow.getSubgraphRepository(); | ||
| const [srcUsecaseId, dstUsecaseId] = await Promise.all([ | ||
| subgraphRepo.getUsecaseSystemIdForSubgraph(srcSubgraphId, fileSystemId), | ||
| subgraphRepo.getUsecaseSystemIdForSubgraph(dstSubgraphId, fileSystemId), | ||
| ]); | ||
| if (srcUsecaseId !== null && dstUsecaseId !== null && srcUsecaseId === dstUsecaseId) { |
There was a problem hiding this comment.
[FUNCTIONALITY] Missing null safety check in inter-usecase validation (duplicate location) - Medium Severity
In create-data-link.handler.ts at line 169, the same null safety issue exists as in the previous finding. The code checks if both srcUsecaseId and dstUsecaseId are non-null before comparing them, but silently allows the operation to proceed if either is null.
Issue: This is the same logical flaw as in create-data-link-with-subsystems.handler.ts. When getUsecaseSystemIdForSubgraph() returns null, the validation is bypassed, potentially allowing invalid inter-usecase links.
Recommendation: Apply the same fix as the previous issue to ensure consistent validation across both handlers.
Fixed Code Snippet
if (linkType === LINK_TYPE.InterUsecase) {
const subgraphRepo = uow.getSubgraphRepository();
const [srcUsecaseId, dstUsecaseId] = await Promise.all([
subgraphRepo.getUsecaseSystemIdForSubgraph(srcSubgraphId, fileSystemId),
subgraphRepo.getUsecaseSystemIdForSubgraph(dstSubgraphId, fileSystemId),
]);
if (srcUsecaseId === null || dstUsecaseId === null) {
throw new DomainRuleViolationException([{
code: 'ORPHANED_SUBGRAPH',
message: 'Cannot create inter-usecase link: one or both subgraphs are not associated with a usecase.',
severity: IssueSeverity.Error,
}]);
}
if (srcUsecaseId === dstUsecaseId) {
throw new DomainRuleViolationException([{
code: 'SAME_USECASE_INTER_USECASE',
message: 'isInterUsecase=true but source and destination belong to the same usecase.',
severity: IssueSeverity.Error,
}]);
}
}There was a problem hiding this comment.
Files Not Fully Analyzed
- docs/data-links/design/data-links-post-design.md - File extension .md not supported
- packages/api/src/infrastructure-wrapper/filters/all-exceptions.filter.ts - Full file content skipped due to token limit
- packages/api/src/infrastructure-wrapper/persistence/unit-of-work/typeorm-unit-of-work.ts - Full file content skipped due to token limit
- packages/api/src/presentation/rest/common/swagger-doc/dto-examples/usecase-api-examples.ts - Full file content skipped due to token limit
- packages/api/src/presentation/rest/modules/data-link/data-link.controller.ts - Full file content skipped due to token limit
- packages/api/src/presentation/rest/modules/data-link/dto/request/create-data-link-request.dto.ts - Full file content skipped due to token limit
- packages/api/src/presentation/rest/modules/data-link/dto/request/create-data-link-with-subsystems-request.dto.ts - Full file content skipped due to token limit
- packages/api/src/presentation/rest/modules/data-link/dto/response/subsystem-data-link-response.dto.ts - Full file content skipped due to token limit
- packages/core/src/application/file-operations/upload-file/services/entity-builders/subsystem-builder.ts - Full file content skipped due to token limit
- packages/core/src/application/orchestration/cqrs/registries/command-handler-registry.ts - Full file content skipped due to token limit
- packages/core/src/application/ports/persistence/query-services/subsystem/subsystem-query-service.ts - Full file content skipped due to token limit
- packages/core/src/application/ports/persistence/query-services/usecase/query-models/subsystem-data-link-read-model.ts - Full file content skipped due to token limit
- packages/core/src/application/ports/persistence/query-services/usecase/query-models/usecase-components-with-subsystems-read-model.ts - Full file content skipped due to token limit
- packages/core/src/application/ports/persistence/repositories/data-link/data-link.repository.ts - Full file content skipped due to token limit
- packages/core/src/application/ports/persistence/repositories/module/module.repository.ts - Full file content skipped due to token limit
- packages/core/src/application/ports/persistence/repositories/subgraph/subgraph.repository.ts - Full file content skipped due to token limit
- packages/core/src/application/ports/persistence/repositories/subsystem/subsystem.repository.ts - Full file content skipped due to token limit
- packages/core/src/application/usecase-designer/data-links/create/build-traversal-entities.ts - Full file content skipped due to token limit
- packages/core/src/application/usecase-designer/data-links/create/create-data-link-with-subsystems.command.ts - Full file content skipped due to token limit
- packages/core/src/application/usecase-designer/data-links/create/create-data-link-with-subsystems.handler.ts - Full file content skipped due to token limit
- packages/core/src/application/usecase-designer/data-links/create/create-data-link.command.ts - Full file content skipped due to token limit
- packages/core/src/application/usecase-designer/data-links/create/create-data-link.handler.ts - Full file content skipped due to token limit
- packages/core/src/application/usecase-designer/usecase/dto/component-collection-dto.ts - Full file content skipped due to token limit
- packages/core/src/application/usecase-designer/usecase/get-component-with-subsystem/build-subsystem-tree.ts - Full file content skipped due to token limit
- packages/core/src/application/usecase-designer/usecase/get-component-with-subsystem/components-with-subsystems-read-model.ts - Full file content skipped due to token limit
- packages/core/src/application/usecase-designer/usecase/get-component-with-subsystem/get-components-with-subsystems.handler.ts - Full file content skipped due to token limit
- packages/core/src/domain/services/subsystem-data-links/subsystem-data-link-derivation.service.ts - Full file content skipped due to token limit
- packages/core/src/index.ts - Full file content skipped due to token limit
- packages/core/src/shared/exceptions/index.ts - Full file content skipped due to token limit
- packages/core/tests/unit/application/file-operations/upload-file/services/entity-builders/subsystem-builder.spec.ts - Full file content skipped due to token limit
- packages/core/tests/unit/application/usecase-designer/data-links/create/create-data-link-with-subsystems.handler.spec.ts - Full file content skipped due to token limit
- packages/core/tests/unit/application/usecase-designer/data-links/create/create-data-link.handler.spec.ts - Full file content skipped due to token limit
- packages/core/tests/unit/application/usecase-designer/usecase/get-component-with-subsystem/get-components-with-subsystems.handler.spec.ts - Full file content skipped due to token limit
- packages/core/tests/unit/domain/services/subsystem-links/subsystem-data-link-derivation.service.spec.ts - Full file content skipped due to token limit
- packages/infrastructure/persistence/src/persistence-typeorm-sqllite/queries/subsystem/db-subsystem-query-service.ts - Full file content skipped due to token limit
- packages/infrastructure/persistence/src/persistence-typeorm-sqllite/repositories/data-link/data-link.repository.ts - Full file content skipped due to token limit
- packages/infrastructure/persistence/src/persistence-typeorm-sqllite/repositories/module/module.repository.ts - Full file content skipped due to token limit
- packages/infrastructure/persistence/src/persistence-typeorm-sqllite/repositories/subgraph/subgraph.repository.ts - Full file content skipped due to token limit
- packages/infrastructure/persistence/src/persistence-typeorm-sqllite/repositories/subsystem/subsystem.repository.ts - Full file content skipped due to token limit
- packages/infrastructure/persistence/tests/integration/repositories/data-link/data-link-edit.repository.integration.spec.ts - Full file content skipped due to token limit
Qualcomm AI Review
| ): Promise<void> { | ||
| const {session, groupId} = this.uow.getWriteContext(); | ||
| const writer = this.requireWriter(); | ||
| // eslint-disable-next-line custom/no-raw-persistence-queries -- conditional UPDATE with IS NULL on valid_until cannot be expressed with TypeORM QueryBuilder | ||
| await this.manager.query( | ||
| `UPDATE edit_actions SET valid_until = $1 WHERE session_id = $2 AND target_system_id = $3 AND field_path IS NULL AND valid_until IS NULL`, | ||
| [new Date().toISOString(), session.sessionId, systemId], | ||
| ); | ||
| await writer.writeCreate( | ||
| { | ||
| targetTable: ENTITY_NAMES.DataLink, |
There was a problem hiding this comment.
[SECURITY][High] SQL Injection Vulnerability in reactivateDataLink
The reactivateDataLink method in TypeOrmDataLinkRepository uses raw SQL with string interpolation for the valid_until update. While the current implementation uses parameterized queries for session_id and target_system_id, the date value is directly interpolated using toISOString(). This could potentially be exploited if the date object is manipulated.
Issue Location: packages/infrastructure/persistence/src/persistence-typeorm-sqllite/repositories/data-link/data-link.repository.ts:218
Risk: An attacker who can control the system clock or manipulate Date objects could potentially inject SQL through the timestamp value.
Recommendation: Use parameterized queries consistently for all values, including timestamps.
Fixed Code Snippet
await this.manager.query(
`UPDATE edit_actions SET valid_until = $1 WHERE session_id = $2 AND target_system_id = $3 AND field_path IS NULL AND valid_until IS NULL`,
[new Date(), session.sessionId, systemId],
);Note: Pass the Date object directly instead of calling toISOString() - the database driver will handle the conversion safely.
| fileSystemId, | ||
| isEc, | ||
| subsystemDataLinks: slsSegments, | ||
| }); | ||
|
|
||
| await dlEditRepo.createDataLink(dataLink, boundaryPortPayloads); | ||
| await uow.commit(); | ||
|
|
||
| return this.buildDto( | ||
| dataLink.systemId, | ||
| dataLink.sourceNodeSystemId, |
There was a problem hiding this comment.
[FUNCTIONALITY][High] Race Condition in Duplicate Detection Logic
The findByPortPair method in both handler implementations checks for existing data links, but there's a race condition between the check and the actual creation. Two concurrent requests with the same port pair could both pass the duplicate check and create duplicate links.
Issue Location: packages/core/src/application/usecase-designer/data-links/create/create-data-link.handler.ts:151-161
Scenario:
- Request A calls
findByPortPair→ returns null - Request B calls
findByPortPair→ returns null (before A commits) - Request A creates the link and commits
- Request B creates a duplicate link and commits
Impact: Database integrity violation, duplicate data links in the system.
Recommendation: Add a unique constraint on (source_port_system_id, destination_port_system_id, file_system_id) in the data_links table and handle the constraint violation exception. Alternatively, use database-level locking (SELECT FOR UPDATE) during the check.
Fixed Code Snippet
const existing = await dlEditRepo.findByPortPair(
srcPortId,
dstPortId,
fileSystemId,
);
if (existing !== null && !existing.isDeleted) {
throw new ConflictException(
`DataLink for ports (${BinaryUtils.toHexString(srcPortId)}, ${BinaryUtils.toHexString(dstPortId)}) already exists.`,
);
}
// Add database-level unique constraint:
// ALTER TABLE data_links ADD CONSTRAINT uq_data_link_ports
// UNIQUE (source_port_system_id, destination_port_system_id, file_system_id);
try {
await dlEditRepo.createDataLink(dataLink, boundaryPortPayloads);
await uow.commit();
} catch (error) {
if (error.code === 'SQLITE_CONSTRAINT' || error.code === '23505') {
throw new ConflictException(
`DataLink for ports (${BinaryUtils.toHexString(srcPortId)}, ${BinaryUtils.toHexString(dstPortId)}) already exists.`,
);
}
throw error;
}| fileSystemId, | ||
| uow, | ||
| subsystemRepo, | ||
| ); | ||
| } catch (error) { | ||
| if (uow.isInTransaction()) await uow.rollback(); | ||
| throw error; | ||
| } | ||
| } | ||
|
|
||
| private async handleBranchA( |
There was a problem hiding this comment.
[FUNCTIONALITY][High] Missing Transaction Rollback in Error Handler
In CreateDataLinkWithSubsystemsHandler.handleBranchB, if an exception occurs after validation but before commit, the transaction is not rolled back. The catch block in the main handle method only rolls back if uow.isInTransaction() returns true, but this check may not be reliable if the transaction state is corrupted.
Issue Location: packages/core/src/application/usecase-designer/data-links/create/create-data-link-with-subsystems.handler.ts:115-118
Problem: If createSubsystemDataLink throws an exception, the transaction remains open and uncommitted, potentially causing connection leaks or blocking other operations.
Recommendation: Ensure rollback is called in all error paths, and consider using try-finally blocks for cleanup.
Fixed Code Snippet
async handle(
command: CreateDataLinkWithSubsystemsCommand,
): Promise<ComponentCollectionWithSubsystemsDto> {
const uow = this.uow;
await uow.startTransaction();
try {
const {session} = uow.getWriteContext();
const fileSystemId = session.fileSystemId;
// ... validation and processing logic ...
if (srcIsSubsystem || dstIsSubsystem) {
return await this.handleBranchB(
command,
srcNodeId,
dstNodeId,
srcPortId,
dstPortId,
srcIsSubsystem,
dstIsSubsystem,
fileSystemId,
uow,
subsystemRepo,
);
}
return await this.handleBranchA(
command,
srcNodeId,
dstNodeId,
srcPortId,
dstPortId,
fileSystemId,
uow,
subsystemRepo,
);
} catch (error) {
// Always attempt rollback, regardless of transaction state
try {
if (uow.isInTransaction()) {
await uow.rollback();
}
} catch (rollbackError) {
// Log rollback failure but throw original error
console.error('Failed to rollback transaction:', rollbackError);
}
throw error;
}
}| } | ||
| return map; | ||
| } | ||
|
|
||
| async getPortIoType( | ||
| portSystemId: number, | ||
| fileSystemId: number, | ||
| ): Promise<PortIoType | null> { | ||
| const sessionId = this.uow.getWriteContext().session.sessionId; | ||
|
|
||
| // Check session overlay first — a staged CREATE wins over the base table |
There was a problem hiding this comment.
[PERFORMANCE][Medium] Inefficient Sequential Database Queries in Validation
The getPortIoType method in TypeOrmSubsystemRepository performs two separate database queries: first checking the session overlay, then falling back to the base table. For validation scenarios where multiple ports are checked (source and destination in Branch B), this results in 4+ sequential queries.
Issue Location: packages/infrastructure/persistence/src/persistence-typeorm-sqllite/repositories/subsystem/subsystem.repository.ts:57-92
Impact: Increased latency for link creation operations, especially when subsystem endpoints are involved.
Recommendation: Batch the port lookups or use a single query with UNION to check both overlay and base table simultaneously.
Fixed Code Snippet
async getPortIoType(
portSystemId: number,
fileSystemId: number,
): Promise<PortIoType | null> {
const sessionId = this.uow.getWriteContext().session.sessionId;
// Single query combining overlay and base table
const result = await this.manager.query(
`
SELECT port_io_type as portIoType FROM (
SELECT
CAST(json_extract(new_value, '$.portIoType') AS TEXT) as port_io_type,
1 as priority
FROM edit_actions
WHERE session_id = $1
AND target_table = 'DataPort'
AND target_system_id = $2
AND operation = 'CREATE'
AND valid_until IS NULL
UNION ALL
SELECT
port_io_type,
2 as priority
FROM data_ports
WHERE system_id = $2 AND file_system_id = $3
) combined
ORDER BY priority
LIMIT 1
`,
[sessionId, portSystemId, fileSystemId],
);
return result.length > 0 ? (result[0].portIoType as PortIoType) : null;
}
6e06894 to
0dce8d4
Compare
There was a problem hiding this comment.
Files Not Fully Analyzed
- docs/data-links/design/data-links-post-design.md - File extension .md not supported
- packages/api/src/infrastructure-wrapper/filters/all-exceptions.filter.ts - Full file content skipped due to token limit
- packages/api/src/infrastructure-wrapper/persistence/unit-of-work/typeorm-unit-of-work.ts - Full file content skipped due to token limit
- packages/api/src/presentation/rest/common/swagger-doc/dto-examples/usecase-api-examples.ts - Full file content skipped due to token limit
- packages/api/src/presentation/rest/modules/data-link/data-link.controller.ts - Full file content skipped due to token limit
- packages/api/src/presentation/rest/modules/data-link/dto/request/create-data-link-request.dto.ts - Full file content skipped due to token limit
- packages/api/src/presentation/rest/modules/data-link/dto/request/create-data-link-with-subsystems-request.dto.ts - Full file content skipped due to token limit
- packages/api/src/presentation/rest/modules/data-link/dto/response/subsystem-data-link-response.dto.ts - Full file content skipped due to token limit
- packages/api/tests/e2e/data-links/create-data-link.e2e-spec.ts - Full file content skipped due to token limit
- packages/core/src/application/file-operations/upload-file/services/entity-builders/subsystem-builder.ts - Full file content skipped due to token limit
- packages/core/src/application/orchestration/cqrs/registries/command-handler-registry.ts - Full file content skipped due to token limit
- packages/core/src/application/ports/persistence/query-services/subsystem/subsystem-query-service.ts - Full file content skipped due to token limit
- packages/core/src/application/ports/persistence/query-services/usecase/query-models/subsystem-data-link-read-model.ts - Full file content skipped due to token limit
- packages/core/src/application/ports/persistence/query-services/usecase/query-models/usecase-components-with-subsystems-read-model.ts - Full file content skipped due to token limit
- packages/core/src/application/ports/persistence/repositories/data-link/data-link.repository.ts - Full file content skipped due to token limit
- packages/core/src/application/ports/persistence/repositories/module/module.repository.ts - Full file content skipped due to token limit
- packages/core/src/application/ports/persistence/repositories/subgraph/subgraph.repository.ts - Full file content skipped due to token limit
- packages/core/src/application/ports/persistence/repositories/subsystem/subsystem.repository.ts - Full file content skipped due to token limit
- packages/core/src/application/usecase-designer/data-links/create/build-traversal-entities.ts - Full file content skipped due to token limit
- packages/core/src/application/usecase-designer/data-links/create/create-data-link-with-subsystems.command.ts - Full file content skipped due to token limit
- packages/core/src/application/usecase-designer/data-links/create/create-data-link-with-subsystems.handler.ts - Full file content skipped due to token limit
- packages/core/src/application/usecase-designer/data-links/create/create-data-link.command.ts - Full file content skipped due to token limit
- packages/core/src/application/usecase-designer/data-links/create/create-data-link.handler.ts - Full file content skipped due to token limit
- packages/core/src/application/usecase-designer/usecase/dto/component-collection-dto.ts - Full file content skipped due to token limit
- packages/core/src/application/usecase-designer/usecase/get-component-with-subsystem/build-subsystem-tree.ts - Full file content skipped due to token limit
- packages/core/src/application/usecase-designer/usecase/get-component-with-subsystem/components-with-subsystems-read-model.ts - Full file content skipped due to token limit
- packages/core/src/application/usecase-designer/usecase/get-component-with-subsystem/get-components-with-subsystems.handler.ts - Full file content skipped due to token limit
- packages/core/src/domain/services/subsystem-data-links/subsystem-data-link-derivation.service.ts - Full file content skipped due to token limit
- packages/core/src/index.ts - Full file content skipped due to token limit
- packages/core/src/shared/exceptions/conflict.exception.ts - Full file content skipped due to token limit
- packages/core/src/shared/exceptions/index.ts - Full file content skipped due to token limit
- packages/core/tests/unit/application/file-operations/upload-file/services/entity-builders/subsystem-builder.spec.ts - Full file content skipped due to token limit
- packages/core/tests/unit/application/usecase-designer/data-links/create/create-data-link-with-subsystems.handler.spec.ts - Full file content skipped due to token limit
- packages/core/tests/unit/application/usecase-designer/data-links/create/create-data-link.handler.spec.ts - Full file content skipped due to token limit
- packages/core/tests/unit/application/usecase-designer/usecase/get-component-with-subsystem/get-components-with-subsystems.handler.spec.ts - Full file content skipped due to token limit
- packages/infrastructure/persistence/src/persistence-typeorm-sqllite/queries/subsystem/db-subsystem-query-service.ts - Full file content skipped due to token limit
- packages/infrastructure/persistence/src/persistence-typeorm-sqllite/repositories/data-link/data-link.repository.ts - Full file content skipped due to token limit
- packages/infrastructure/persistence/src/persistence-typeorm-sqllite/repositories/module/module.repository.ts - Full file content skipped due to token limit
- packages/infrastructure/persistence/src/persistence-typeorm-sqllite/repositories/subgraph/subgraph.repository.ts - Full file content skipped due to token limit
- packages/infrastructure/persistence/src/persistence-typeorm-sqllite/repositories/subsystem/subsystem.repository.ts - Full file content skipped due to token limit
- packages/infrastructure/persistence/tests/integration/repositories/data-link/data-link-edit.repository.integration.spec.ts - Full file content skipped due to token limit
Qualcomm AI Review
| private readonly uow: UnitOfWork, | ||
| private readonly queryServices: QueryServices, | ||
| private readonly idGeneration: IdGenerationPort, | ||
| ) {} | ||
|
|
||
| handle(_command: CreateDataLinkCommand): Promise<ComponentCollectionDto> { | ||
| if ( | ||
| this.uow == undefined || | ||
| this.queryServices == undefined || | ||
| this.idGeneration == undefined | ||
| ) | ||
| throw new Error('Input validation error'); | ||
| throw new Error('Not implemented'); | ||
| async handle( | ||
| command: CreateDataLinkCommand, | ||
| ): Promise<ComponentCollectionDto> { | ||
| const uow = this.uow; | ||
| await uow.startTransaction(); | ||
| try { | ||
| const {session} = uow.getWriteContext(); |
There was a problem hiding this comment.
[FUNCTIONALITY][High] Missing rollback in catch block for transaction error handling
In create-data-link.handler.ts and create-data-link-with-subsystems.handler.ts, the catch blocks check uow.isInTransaction() before calling rollback(), but if the transaction was never started due to an early error, or if startTransaction() itself throws, the error handling becomes inconsistent. Additionally, if rollback() itself throws an exception, the original error will be masked.
Issue: The current pattern can lead to:
- Unhandled transaction state if
startTransaction()fails - Original error being masked if
rollback()throws - Resource leaks if transaction cleanup fails silently
Fixed Code Snippet
async handle(
command: CreateDataLinkCommand,
): Promise<ComponentCollectionDto> {
const uow = this.uow;
let transactionStarted = false;
try {
await uow.startTransaction();
transactionStarted = true;
const {session} = uow.getWriteContext();
// ... rest of handler logic
await uow.commit();
return this.buildDto(/* ... */);
} catch (error) {
if (transactionStarted) {
try {
await uow.rollback();
} catch (rollbackError) {
// Log rollback failure but don't mask original error
console.error('Rollback failed:', rollbackError);
}
}
throw error;
}
}| private readonly uow: UnitOfWork, | ||
| private readonly idGeneration: IdGenerationPort, | ||
| ) {} | ||
|
|
||
| async handle( | ||
| command: CreateDataLinkWithSubsystemsCommand, | ||
| ): Promise<ComponentCollectionWithSubsystemsDto> { | ||
| const uow = this.uow; | ||
| await uow.startTransaction(); | ||
| try { | ||
| const {session} = uow.getWriteContext(); |
There was a problem hiding this comment.
[FUNCTIONALITY][High] Missing rollback in catch block for transaction error handling
In create-data-link.handler.ts and create-data-link-with-subsystems.handler.ts, the catch blocks check uow.isInTransaction() before calling rollback(), but if the transaction was never started due to an early error, or if startTransaction() itself throws, the error handling becomes inconsistent. Additionally, if rollback() itself throws an exception, the original error will be masked.
Issue: The current pattern can lead to:
- Unhandled transaction state if
startTransaction()fails - Original error being masked if
rollback()throws - Resource leaks if transaction cleanup fails silently
Fixed Code Snippet
async handle(
command: CreateDataLinkCommand,
): Promise<ComponentCollectionDto> {
const uow = this.uow;
let transactionStarted = false;
try {
await uow.startTransaction();
transactionStarted = true;
const {session} = uow.getWriteContext();
// ... rest of handler logic
await uow.commit();
return this.buildDto(/* ... */);
} catch (error) {
if (transactionStarted) {
try {
await uow.rollback();
} catch (rollbackError) {
// Log rollback failure but don't mask original error
console.error('Rollback failed:', rollbackError);
}
}
throw error;
}
}| ); | ||
| } | ||
| } | ||
|
|
||
| async findByPortPair( | ||
| sourcePortSystemId: number, | ||
| destPortSystemId: number, | ||
| fileSystemId: number, | ||
| ): Promise<{ | ||
| systemId: number; | ||
| isDeleted: boolean; |
There was a problem hiding this comment.
[FUNCTIONALITY][High] Race condition in duplicate link detection
The findByPortPair method in data-link.repository.ts performs a read operation to check for existing links, followed by a separate write operation in createDataLink. This creates a race condition window where two concurrent requests could both pass the duplicate check and create duplicate links.
Issue: Between the time findByPortPair returns null and createDataLink inserts the new row, another transaction could insert the same link. This violates the uniqueness constraint that the duplicate check is meant to enforce.
Impact:
- Data integrity violation (duplicate links in database)
- Inconsistent application state
- Potential downstream errors when querying links
Fixed Code Snippet
// In data-link.repository.ts - Add database-level unique constraint
async createDataLink(
dataLink: DataLink,
boundaryPortPayloads: BoundaryPortPayload[],
options?: EditOptions,
): Promise<void> {
const {session, groupId} = this.uow.getWriteContext();
const fileSystemId = dataLink.fileSystemId;
const writer = this.requireWriter();
// Use INSERT ... ON CONFLICT or equivalent to make the operation atomic
try {
// First, check with SELECT FOR UPDATE to lock the row if it exists
const lockQuery = await this.manager.query(
`SELECT system_id FROM data_links
WHERE source_port_system_id = $1
AND destination_port_system_id = $2
AND file_system_id = $3
FOR UPDATE NOWAIT`,
[dataLink.sourcePortSystemId, dataLink.destinationPortSystemId, fileSystemId]
);
if (lockQuery.length > 0) {
throw new ConflictException(
`DataLink for ports already exists (detected during atomic insert)`
);
}
// Proceed with insert operations...
for (const bp of boundaryPortPayloads) {
// ... existing code
}
} catch (error) {
if (error.code === '23505') { // PostgreSQL unique violation
throw new ConflictException(
`DataLink for ports (${dataLink.sourcePortSystemId}, ${dataLink.destinationPortSystemId}) already exists.`
);
}
throw error;
}
}Alternative Solution: Add a unique constraint at the database level on (source_port_system_id, destination_port_system_id, file_system_id) and handle the constraint violation exception.
| try { | ||
| const {session} = uow.getWriteContext(); | ||
| const fileSystemId = session.fileSystemId; | ||
|
|
||
| const srcModuleId = Number.parseInt(command.sourceModuleSystemId, 10); | ||
| const dstModuleId = Number.parseInt( | ||
| command.destinationModuleSystemId, | ||
| 10, | ||
| ); | ||
| const srcPortId = Number.parseInt(command.sourcePortSystemId, 10); | ||
| const dstPortId = Number.parseInt(command.destinationPortSystemId, 10); |
There was a problem hiding this comment.
[FUNCTIONALITY][Medium] Missing validation for Number.parseInt conversions
In both CreateDataLinkHandler and CreateDataLinkWithSubsystemsHandler, string inputs are converted to numbers using Number.parseInt() without validating that the conversion succeeded. If invalid input is provided (e.g., non-numeric strings), parseInt returns NaN, which will cause downstream errors or unexpected behavior.
Issue:
const srcModuleId = Number.parseInt(command.sourceModuleSystemId, 10);
// If command.sourceModuleSystemId is 'abc', srcModuleId becomes NaN
// Subsequent database queries with NaN will fail or return unexpected resultsImpact:
- Cryptic error messages for users
- Potential database errors
- Inconsistent error handling
Fixed Code Snippet
// In create-data-link.handler.ts
const srcModuleId = Number.parseInt(command.sourceModuleSystemId, 10);
const dstModuleId = Number.parseInt(command.destinationModuleSystemId, 10);
const srcPortId = Number.parseInt(command.sourcePortSystemId, 10);
const dstPortId = Number.parseInt(command.destinationPortSystemId, 10);
if (isNaN(srcModuleId) || isNaN(dstModuleId) || isNaN(srcPortId) || isNaN(dstPortId)) {
throw new DomainRuleViolationException([{
code: 'INVALID_INPUT',
message: 'All system IDs must be valid numeric values',
severity: IssueSeverity.Error,
}]);
}| try { | ||
| const {session} = uow.getWriteContext(); | ||
| const fileSystemId = session.fileSystemId; | ||
|
|
||
| const srcNodeId = Number.parseInt(command.sourceNodeSystemId, 10); | ||
| const dstNodeId = Number.parseInt(command.destinationNodeSystemId, 10); | ||
| const srcPortId = Number.parseInt(command.sourcePortSystemId, 10); | ||
| const dstPortId = Number.parseInt(command.destinationPortSystemId, 10); | ||
|
|
||
| // FR-DLS-04: self-loop check | ||
| if (srcNodeId === dstNodeId) { |
There was a problem hiding this comment.
[FUNCTIONALITY][Medium] Missing validation for Number.parseInt conversions
In both CreateDataLinkHandler and CreateDataLinkWithSubsystemsHandler, string inputs are converted to numbers using Number.parseInt() without validating that the conversion succeeded. If invalid input is provided (e.g., non-numeric strings), parseInt returns NaN, which will cause downstream errors or unexpected behavior.
Issue:
const srcModuleId = Number.parseInt(command.sourceModuleSystemId, 10);
// If command.sourceModuleSystemId is 'abc', srcModuleId becomes NaN
// Subsequent database queries with NaN will fail or return unexpected resultsImpact:
- Cryptic error messages for users
- Potential database errors
- Inconsistent error handling
Fixed Code Snippet
// In create-data-link.handler.ts
const srcModuleId = Number.parseInt(command.sourceModuleSystemId, 10);
const dstModuleId = Number.parseInt(command.destinationModuleSystemId, 10);
const srcPortId = Number.parseInt(command.sourcePortSystemId, 10);
const dstPortId = Number.parseInt(command.destinationPortSystemId, 10);
if (isNaN(srcModuleId) || isNaN(dstModuleId) || isNaN(srcPortId) || isNaN(dstPortId)) {
throw new DomainRuleViolationException([{
code: 'INVALID_INPUT',
message: 'All system IDs must be valid numeric values',
severity: IssueSeverity.Error,
}]);
}| * Create a new data link (collapsed view, module endpoints only). | ||
| * Stores all link segments in DB; returns ComponentsResponseDto. | ||
| */ | ||
| @Post() | ||
| @ApiDocumentationWithExample({ | ||
| summary: 'Create a new data link (flat view)', | ||
| summary: 'Create a new data link', | ||
| description: | ||
| 'Creates a data link between two modules. Stores all segments (mod→SS, SS→SS, SS→mod) in the DB. ' + | ||
| 'Returns a flat ComponentsResponseDto with the created link.', |
There was a problem hiding this comment.
[MAINTAINABILITY][Medium] Inconsistent error handling for subsystem vs module type checks
In CreateDataLinkHandler.handle(), when a node is found to be a subsystem instead of a module, the code throws a DomainRuleViolationException with code WRONG_NODE_TYPE. However, in the controller layer (data-link.controller.ts), this same scenario is documented as returning 404 (Not Found) in some cases and 422 (Unprocessable Entity) in others.
Issue: The error handling strategy is inconsistent:
- Handler throws
DomainRuleViolationException(maps to 422) - Documentation suggests 404 for "module not found"
- The distinction between "node doesn't exist" vs "node exists but wrong type" is important for API consumers
Current behavior:
if (srcModule === null) {
const isSubsystem = await subsystemRepo.subsystemExists(srcModuleId, fileSystemId);
if (isSubsystem) {
throw new DomainRuleViolationException([{ // Returns 422
code: 'WRONG_NODE_TYPE',
message: `Source node ${BinaryUtils.toHexString(srcModuleId)} is a subsystem, not a module.`,
severity: IssueSeverity.Error,
}]);
}
throw new ResourceNotFoundException( // Returns 404
`Source module ${BinaryUtils.toHexString(srcModuleId)} not found.`,
);
}Recommendation: This is actually correct behavior - returning 422 for wrong type and 404 for not found is semantically appropriate. However, the API documentation in the controller should be updated to clearly reflect this distinction.
Fixed Code Snippet
// In data-link.controller.ts - Update API documentation
@ApiDocumentationWithExample({
summary: 'Create a new data link',
description:
'Creates a data link between two modules. Stores all segments (mod→SS, SS→SS, SS→mod) in the DB. ' +
'Returns a flat ComponentsResponseDto with the created link.',
requestDto: CreateDataLinkRequest,
requestDtoDescription: 'Data link creation parameters',
responses: [
{
status: HttpStatus.CREATED,
description: 'Data link created successfully',
dto: ComponentsResponseDto,
},
{status: HttpStatus.BAD_REQUEST, description: 'Invalid request data (malformed input)'},
{
status: HttpStatus.NOT_FOUND,
description: 'Source or destination module does not exist in the file',
},
{
status: HttpStatus.UNPROCESSABLE_ENTITY,
description: 'Validation failed: wrong node type (subsystem instead of module), wrong port direction, port ownership mismatch, self-loop, or other business rule violation',
},
{
status: HttpStatus.CONFLICT,
description: 'Data link already exists for the given port pair',
},
{
status: HttpStatus.INTERNAL_SERVER_ERROR,
description: 'Failed to create data link',
},
],
})
4c26fb6 to
1cf3a44
Compare
aboppay
changed the title
There was a problem hiding this comment.
Files Not Fully Analyzed
- docs/data-links/design/data-links-post-design.md - File extension .md not supported
- packages/api/src/infrastructure-wrapper/filters/all-exceptions.filter.ts - Full file content skipped due to token limit
- packages/api/src/infrastructure-wrapper/persistence/unit-of-work/typeorm-unit-of-work.ts - Full file content skipped due to token limit
- packages/api/src/presentation/rest/common/swagger-doc/dto-examples/usecase-api-examples.ts - Full file content skipped due to token limit
- packages/api/src/presentation/rest/modules/data-link/data-link.controller.ts - Full file content skipped due to token limit
- packages/api/src/presentation/rest/modules/data-link/dto/request/create-data-link-request.dto.ts - Full file content skipped due to token limit
- packages/api/src/presentation/rest/modules/data-link/dto/request/create-data-link-with-subsystems-request.dto.ts - Full file content skipped due to token limit
- packages/api/tests/e2e/data-links/create-data-link.e2e-spec.ts - Full file content skipped due to token limit
- packages/core/src/application/file-operations/upload-file/services/entity-builders/subsystem-builder.ts - Full file content skipped due to token limit
- packages/core/src/application/orchestration/cqrs/registries/command-handler-registry.ts - Full file content skipped due to token limit
- packages/core/src/application/ports/persistence/query-services/subsystem/subsystem-query-service.ts - Full file content skipped due to token limit
- packages/core/src/application/ports/persistence/query-services/usecase/query-models/usecase-components-with-subsystems-read-model.ts - Full file content skipped due to token limit
- packages/core/src/application/ports/persistence/repositories/data-link/data-link.repository.ts - Full file content skipped due to token limit
- packages/core/src/application/ports/persistence/repositories/module/module.repository.ts - Full file content skipped due to token limit
- packages/core/src/application/ports/persistence/repositories/subgraph/subgraph.repository.ts - Full file content skipped due to token limit
- packages/core/src/application/ports/persistence/repositories/subsystem/subsystem.repository.ts - Full file content skipped due to token limit
- packages/core/src/application/usecase-designer/data-links/create/build-traversal-entities.ts - Full file content skipped due to token limit
- packages/core/src/application/usecase-designer/data-links/create/create-data-link-with-subsystems.command.ts - Full file content skipped due to token limit
- packages/core/src/application/usecase-designer/data-links/create/create-data-link-with-subsystems.handler.ts - Full file content skipped due to token limit
- packages/core/src/application/usecase-designer/data-links/create/create-data-link.command.ts - Full file content skipped due to token limit
- packages/core/src/application/usecase-designer/data-links/create/create-data-link.handler.ts - Full file content skipped due to token limit
- packages/core/src/application/usecase-designer/usecase/dto/component-collection-dto.ts - Full file content skipped due to token limit
- packages/core/src/application/usecase-designer/usecase/get-component-with-subsystem/build-subsystem-tree.ts - Full file content skipped due to token limit
- packages/core/src/application/usecase-designer/usecase/get-component-with-subsystem/components-with-subsystems-read-model.ts - Full file content skipped due to token limit
- packages/core/src/application/usecase-designer/usecase/get-component-with-subsystem/get-components-with-subsystems.handler.ts - Full file content skipped due to token limit
- packages/core/src/domain/services/subsystem-data-links/subsystem-data-link-derivation.service.ts - Full file content skipped due to token limit
- packages/core/src/index.ts - Full file content skipped due to token limit
- packages/core/src/shared/exceptions/conflict.exception.ts - Full file content skipped due to token limit
- packages/core/src/shared/exceptions/index.ts - Full file content skipped due to token limit
- packages/core/tests/unit/application/file-operations/upload-file/services/entity-builders/subsystem-builder.spec.ts - Full file content skipped due to token limit
- packages/core/tests/unit/application/usecase-designer/data-links/create/create-data-link-with-subsystems.handler.spec.ts - Full file content skipped due to token limit
- packages/core/tests/unit/application/usecase-designer/data-links/create/create-data-link.handler.spec.ts - Full file content skipped due to token limit
- packages/core/tests/unit/application/usecase-designer/usecase/get-component-with-subsystem/get-components-with-subsystems.handler.spec.ts - Full file content skipped due to token limit
- packages/core/tests/unit/domain/services/subsystem-links/subsystem-data-link-derivation.service.spec.ts - Full file content skipped due to token limit
- packages/infrastructure/persistence/src/persistence-typeorm-sqllite/queries/subsystem/db-subsystem-query-service.ts - Full file content skipped due to token limit
- packages/infrastructure/persistence/src/persistence-typeorm-sqllite/repositories/data-link/data-link.repository.ts - Full file content skipped due to token limit
- packages/infrastructure/persistence/src/persistence-typeorm-sqllite/repositories/module/module.repository.ts - Full file content skipped due to token limit
- packages/infrastructure/persistence/src/persistence-typeorm-sqllite/repositories/subgraph/subgraph.repository.ts - Full file content skipped due to token limit
- packages/infrastructure/persistence/src/persistence-typeorm-sqllite/repositories/subsystem/subsystem.repository.ts - Full file content skipped due to token limit
- packages/infrastructure/persistence/tests/integration/repositories/data-link/data-link-edit.repository.integration.spec.ts - Full file content skipped due to token limit
Qualcomm AI Review
| dataLink.destinationPortSystemId, | ||
| dataLink.linkType, | ||
| dataLink.isEc, | ||
| ); | ||
| } catch (error) { | ||
| if (uow.isInTransaction()) await uow.rollback(); | ||
| throw error; | ||
| } | ||
| } | ||
|
|
||
| private deriveLinkType( |
There was a problem hiding this comment.
[Functionality - High] Missing Transaction Rollback in Nested Error Paths
In CreateDataLinkHandler.handle() and CreateDataLinkWithSubsystemsHandler.handle(), the error handling catch block only rolls back if uow.isInTransaction() returns true. However, there are multiple validation checks (self-loop, port direction, etc.) that throw exceptions before the transaction is started. If an exception is thrown after startTransaction() but before entering the try block's main logic, the rollback may not execute properly.
Additionally, in the re-activation path (when existing.isDeleted is true), if buildTraversalEntities() or createDataLink() fails, the transaction remains open because the error is thrown before the commit, but the catch block may not properly handle all error types.
Impact: Database connections could be left in an inconsistent state, potentially causing connection pool exhaustion under load.
Fixed Code Snippet
async handle(
command: CreateDataLinkCommand,
): Promise<ComponentCollectionDto> {
const uow = this.uow;
try {
await uow.startTransaction();
const {session} = uow.getWriteContext();
const fileSystemId = session.fileSystemId;
// ... validation and processing logic ...
await uow.commit();
return this.buildDto(/* ... */);
} catch (error) {
// Always attempt rollback if transaction was started
if (uow.isInTransaction()) {
try {
await uow.rollback();
} catch (rollbackError) {
// Log rollback failure but throw original error
console.error('Transaction rollback failed:', rollbackError);
}
}
throw error;
}
}|
|
||
| return (row?.dp_port_io_type as PortIoType) ?? null; | ||
| } | ||
|
|
||
| async isPortOccupiedAsSource( | ||
| portSystemId: number, | ||
| fileSystemId: number, | ||
| ): Promise<boolean> { | ||
| const count = await this.manager | ||
| .createQueryBuilder() | ||
| .select('1') |
There was a problem hiding this comment.
[Functionality - High] Race Condition in Port Occupation Validation
In CreateDataLinkWithSubsystemsHandler.handleBranchB(), the port occupation checks (isPortOccupiedAsSource and isPortOccupiedAsDest) are performed as separate queries before the actual SLS creation. This creates a race condition window where two concurrent requests could both pass the validation checks and then both create conflicting SLS segments.
Scenario:
- Request A checks if port 401 is occupied as source → returns false
- Request B checks if port 401 is occupied as source → returns false (before A commits)
- Request A creates SLS with port 401 as source
- Request B creates SLS with port 401 as source (violates FR-DLS-07)
Impact: Data integrity violation - subsystem ports could end up with multiple connections despite the single-connection constraint.
Fixed Code Snippet
// In subsystem.repository.ts, add atomic check-and-insert
async checkAndReservePort(
portSystemId: number,
direction: 'source' | 'dest',
fileSystemId: number,
slsSystemId: number,
): Promise<boolean> {
const column = direction === 'source' ? 'source_port_system_id' : 'destination_port_system_id';
// Use INSERT with SELECT to atomically check and reserve
const result = await this.manager.query(
`INSERT INTO subsystem_data_links (system_id, ${column}, file_system_id, data_link_system_id)
SELECT ?, ?, ?, NULL
WHERE NOT EXISTS (
SELECT 1 FROM subsystem_data_links
WHERE ${column} = ? AND file_system_id = ?
)`,
[slsSystemId, portSystemId, fileSystemId, portSystemId, fileSystemId]
);
return result.affectedRows > 0;
}| ]); | ||
| } | ||
|
|
||
| // FR-DL-02/03: module existence and type check | ||
| const moduleRepo = uow.getModuleRepository(); | ||
| const subsystemRepo = uow.getSubsystemRepository(); | ||
| const [srcModule, dstModule] = await Promise.all([ | ||
| moduleRepo.findModulePortsForLink(srcModuleId, fileSystemId), | ||
| moduleRepo.findModulePortsForLink(dstModuleId, fileSystemId), | ||
| ]); | ||
|
|
There was a problem hiding this comment.
[Performance - Medium] Inefficient Sequential Database Queries
In CreateDataLinkHandler.handle(), the module validation performs two separate findModulePortsForLink() calls sequentially, followed by two more subsystemExists() calls if validation fails. This results in 2-4 database round trips that could be optimized.
Similarly, in the subgraph validation for INTER_USECASE links, two sequential getUsecaseSystemIdForSubgraph() calls are made.
Impact: Increased latency for link creation operations, especially noticeable under high load or with network latency to the database.
Fixed Code Snippet
// Create a batch query method in ModuleRepository
async findMultipleModulePortsForLink(
moduleSystemIds: number[],
fileSystemId: number,
): Promise<Map<number, {subgraphSystemId: number; ports: {systemId: number; portIoType: PortIoType}[]}>> {
const sessionId = this.uow.getWriteContext().session.sessionId;
const results = new Map();
// Fetch all modules in a single query
const moduleNodes = await this.moduleNodeFetcher.fetchMultiple(
moduleSystemIds,
fileSystemId,
sessionId,
);
// Fetch all ports in a single query
const allPorts = await this.portFetcher.fetchDataPortsForMultipleNodes(
moduleSystemIds,
fileSystemId,
sessionId,
);
for (const [moduleId, node] of moduleNodes) {
results.set(moduleId, {
subgraphSystemId: node.subgraphSystemId,
ports: allPorts.get(moduleId) || [],
});
}
return results;
}
// In handler:
const modulesData = await moduleRepo.findMultipleModulePortsForLink(
[srcModuleId, dstModuleId],
fileSystemId,
);
const srcModule = modulesData.get(srcModuleId) ?? null;
const dstModule = modulesData.get(dstModuleId) ?? null;| } | null> { | ||
| const {session} = this.uow.getWriteContext(); | ||
| const sessionId = session.sessionId; | ||
|
|
||
| const baseRow = await this.manager | ||
| .createQueryBuilder() | ||
| .select('dl.systemId') | ||
| .from(ENTITY_NAMES.DataLink, 'dl') | ||
| .where( | ||
| 'dl.sourcePortSystemId = :srcPort AND dl.destinationPortSystemId = :dstPort AND dl.fileSystemId = :fileSystemId', | ||
| {srcPort: sourcePortSystemId, dstPort: destPortSystemId, fileSystemId}, |
There was a problem hiding this comment.
[Performance - Medium] Missing Database Index Validation
The findByPortPair() method in TypeOrmDataLinkRepository queries the data_links table using a composite WHERE clause on sourcePortSystemId, destinationPortSystemId, and fileSystemId. However, there's no verification that an appropriate composite index exists on these columns.
Without a proper index, this query will perform a full table scan as the number of data links grows, severely impacting performance for duplicate detection (FR-DL-07).
Impact: O(n) query performance instead of O(log n), causing significant slowdown as the number of links increases.
Fixed Code Snippet
// Add migration to create composite index
// In a new migration file:
export class AddDataLinkPortPairIndex1234567890 implements MigrationInterface {
public async up(queryRunner: QueryRunner): Promise<void> {
await queryRunner.query(
`CREATE INDEX idx_data_links_port_pair
ON data_links (source_port_system_id, destination_port_system_id, file_system_id)`
);
}
public async down(queryRunner: QueryRunner): Promise<void> {
await queryRunner.query(`DROP INDEX idx_data_links_port_pair`);
}
}
// Add index hint in repository query:
const baseRow = await this.manager
.createQueryBuilder()
.select('dl.systemId')
.from(ENTITY_NAMES.DataLink, 'dl')
.where(
'dl.sourcePortSystemId = :srcPort AND dl.destinationPortSystemId = :dstPort AND dl.fileSystemId = :fileSystemId',
{srcPort: sourcePortSystemId, dstPort: destPortSystemId, fileSystemId},
)
.useIndex('idx_data_links_port_pair') // Add index hint
.getRawOne<{dl_system_id: number}>();There was a problem hiding this comment.
Files Not Fully Analyzed
- docs/data-links/design/data-links-post-design.md - File extension .md not supported
- docs/data-links/requirements/data-links-post-requirements.md - File extension .md not supported
- packages/api/src/infrastructure-wrapper/filters/all-exceptions.filter.ts - Full file content skipped due to token limit
- packages/api/src/infrastructure-wrapper/persistence/unit-of-work/typeorm-unit-of-work.ts - Full file content skipped due to token limit
- packages/api/src/presentation/rest/common/swagger-doc/dto-examples/usecase-api-examples.ts - Full file content skipped due to token limit
- packages/api/src/presentation/rest/modules/data-link/data-link.controller.ts - Full file content skipped due to token limit
- packages/api/src/presentation/rest/modules/data-link/dto/request/create-data-link-request.dto.ts - Full file content skipped due to token limit
- packages/api/src/presentation/rest/modules/data-link/dto/request/create-data-link-with-subsystems-request.dto.ts - Full file content skipped due to token limit
- packages/api/tests/e2e/data-links/create-data-link.e2e-spec.ts - Full file content skipped due to token limit
- packages/core/src/application/file-operations/upload-file/services/entity-builders/subsystem-builder.ts - Full file content skipped due to token limit
- packages/core/src/application/orchestration/cqrs/registries/command-handler-registry.ts - Full file content skipped due to token limit
- packages/core/src/application/ports/persistence/query-services/subsystem/subsystem-query-service.ts - Full file content skipped due to token limit
- packages/core/src/application/ports/persistence/query-services/usecase/query-models/usecase-components-with-subsystems-read-model.ts - Full file content skipped due to token limit
- packages/core/src/application/ports/persistence/repositories/data-link/data-link.repository.ts - Full file content skipped due to token limit
- packages/core/src/application/ports/persistence/repositories/module/module.repository.ts - Full file content skipped due to token limit
- packages/core/src/application/ports/persistence/repositories/subgraph/subgraph.repository.ts - Full file content skipped due to token limit
- packages/core/src/application/ports/persistence/repositories/subsystem/subsystem.repository.ts - Full file content skipped due to token limit
- packages/core/src/application/usecase-designer/data-links/create/build-traversal-entities.ts - Full file content skipped due to token limit
- packages/core/src/application/usecase-designer/data-links/create/create-data-link-with-subsystems.command.ts - Full file content skipped due to token limit
- packages/core/src/application/usecase-designer/data-links/create/create-data-link-with-subsystems.handler.ts - Full file content skipped due to token limit
- packages/core/src/application/usecase-designer/data-links/create/create-data-link.command.ts - Full file content skipped due to token limit
- packages/core/src/application/usecase-designer/data-links/create/create-data-link.handler.ts - Full file content skipped due to token limit
- packages/core/src/application/usecase-designer/usecase/dto/component-collection-dto.ts - Full file content skipped due to token limit
- packages/core/src/application/usecase-designer/usecase/get-component-with-subsystem/build-subsystem-tree.ts - Full file content skipped due to token limit
- packages/core/src/application/usecase-designer/usecase/get-component-with-subsystem/components-with-subsystems-read-model.ts - Full file content skipped due to token limit
- packages/core/src/application/usecase-designer/usecase/get-component-with-subsystem/get-components-with-subsystems.handler.ts - Full file content skipped due to token limit
- packages/core/src/domain/services/subsystem-data-links/subsystem-data-link-derivation.service.ts - Full file content skipped due to token limit
- packages/core/src/index.ts - Full file content skipped due to token limit
- packages/core/src/shared/exceptions/conflict.exception.ts - Full file content skipped due to token limit
- packages/core/src/shared/exceptions/index.ts - Full file content skipped due to token limit
- packages/core/tests/unit/application/file-operations/upload-file/services/entity-builders/subsystem-builder.spec.ts - Full file content skipped due to token limit
- packages/core/tests/unit/application/usecase-designer/data-links/create/create-data-link-with-subsystems.handler.spec.ts - Full file content skipped due to token limit
- packages/core/tests/unit/application/usecase-designer/data-links/create/create-data-link.handler.spec.ts - Full file content skipped due to token limit
- packages/core/tests/unit/application/usecase-designer/usecase/get-component-with-subsystem/get-components-with-subsystems.handler.spec.ts - Full file content skipped due to token limit
- packages/core/tests/unit/domain/services/subsystem-links/subsystem-data-link-derivation.service.spec.ts - Full file content skipped due to token limit
- packages/infrastructure/persistence/src/persistence-typeorm-sqllite/queries/subsystem/db-subsystem-query-service.ts - Full file content skipped due to token limit
- packages/infrastructure/persistence/src/persistence-typeorm-sqllite/repositories/data-link/data-link.repository.ts - Full file content skipped due to token limit
- packages/infrastructure/persistence/src/persistence-typeorm-sqllite/repositories/module/module.repository.ts - Full file content skipped due to token limit
- packages/infrastructure/persistence/src/persistence-typeorm-sqllite/repositories/subgraph/subgraph.repository.ts - Full file content skipped due to token limit
- packages/infrastructure/persistence/src/persistence-typeorm-sqllite/repositories/subsystem/subsystem.repository.ts - Full file content skipped due to token limit
- packages/infrastructure/persistence/tests/integration/repositories/data-link/data-link-edit.repository.integration.spec.ts - Full file content skipped due to token limit
Qualcomm AI Review
| dataLink.destinationPortSystemId, | ||
| dataLink.linkType, | ||
| dataLink.isEc, | ||
| ); | ||
| } catch (error) { | ||
| if (uow.isInTransaction()) await uow.rollback(); | ||
| throw error; | ||
| } | ||
| } | ||
|
|
||
| private deriveLinkType( |
There was a problem hiding this comment.
[Security/Functionality - High] Missing transaction rollback in error handling paths
In both CreateDataLinkHandler and CreateDataLinkWithSubsystemsHandler, the error handling catch block checks uow.isInTransaction() before rolling back. However, if an error occurs after uow.commit() is called but before the method returns, the transaction state may be inconsistent. Additionally, if uow.rollback() itself throws an error, the original error will be masked.
Issue Details:
- If commit fails partially, the rollback may not execute
- Nested errors in rollback can mask the original error
- No logging of rollback failures
Impact: Data corruption, partial commits, difficult debugging
Fixed Code Snippet
async handle(
command: CreateDataLinkCommand,
): Promise<ComponentCollectionDto> {
const uow = this.uow;
await uow.startTransaction();
try {
// ... existing logic ...
await uow.commit();
return this.buildDto(/* ... */);
} catch (error) {
try {
if (uow.isInTransaction()) {
await uow.rollback();
}
} catch (rollbackError) {
// Log rollback failure but throw original error
console.error('Rollback failed:', rollbackError);
}
throw error;
}
}| sessionId, | ||
| ); | ||
| } | ||
|
|
||
| async createDataLink( | ||
| dataLink: DataLink, | ||
| boundaryPortPayloads: BoundaryPortPayload[], | ||
| options?: EditOptions, | ||
| ): Promise<void> { | ||
| const {session, groupId} = this.uow.getWriteContext(); | ||
| const fileSystemId = dataLink.fileSystemId; |
There was a problem hiding this comment.
[Functionality - High] Race condition in duplicate link detection
The findByPortPair method in DataLinkRepository checks for existing links, but there's a time window between the check and the actual creation where another concurrent request could create the same link. This is a classic TOCTOU (Time-of-Check-Time-of-Use) race condition.
Issue Details:
- Two simultaneous requests with identical port pairs can both pass the duplicate check
- Both requests will attempt to create the link, potentially causing database constraint violations or duplicate data
- The session-based overlay doesn't prevent this race at the database level
Impact: Duplicate links, constraint violations, data inconsistency
Recommendation: Add a unique constraint at the database level on (source_port_system_id, destination_port_system_id, file_system_id) and handle the constraint violation gracefully by converting it to a ConflictException.
Fixed Code Snippet
async createDataLink(
dataLink: DataLink,
boundaryPortPayloads: BoundaryPortPayload[],
options?: EditOptions,
): Promise<void> {
const {session, groupId} = this.uow.getWriteContext();
const fileSystemId = dataLink.fileSystemId;
const writer = this.requireWriter();
try {
// ... existing creation logic ...
} catch (error: any) {
// Handle unique constraint violation
if (error.code === 'SQLITE_CONSTRAINT' || error.message?.includes('UNIQUE constraint')) {
throw new ConflictException(
`DataLink for ports (${dataLink.sourcePortSystemId}, ${dataLink.destinationPortSystemId}) already exists.`
);
}
throw error;
}
}
1cf3a44 to
799d1bd
Compare
There was a problem hiding this comment.
Files Not Fully Analyzed
- docs/data-links/design/data-links-post-design.md - File extension .md not supported
- docs/data-links/requirements/data-links-post-requirements.md - File extension .md not supported
- packages/api/src/infrastructure-wrapper/filters/all-exceptions.filter.ts - Full file content skipped due to token limit
- packages/api/src/infrastructure-wrapper/persistence/unit-of-work/typeorm-unit-of-work.ts - Full file content skipped due to token limit
- packages/api/src/presentation/rest/common/swagger-doc/dto-examples/usecase-api-examples.ts - Full file content skipped due to token limit
- packages/api/src/presentation/rest/modules/data-link/data-link.controller.ts - Full file content skipped due to token limit
- packages/api/src/presentation/rest/modules/data-link/dto/request/create-data-link-request.dto.ts - Full file content skipped due to token limit
- packages/api/src/presentation/rest/modules/data-link/dto/request/create-data-link-with-subsystems-request.dto.ts - Full file content skipped due to token limit
- packages/api/src/presentation/rest/modules/data-link/dto/response/subsystem-data-link-response.dto.ts - Full file content skipped due to token limit
- packages/api/tests/e2e/data-links/create-data-link.e2e-spec.ts - Full file content skipped due to token limit
- packages/core/src/application/file-operations/upload-file/services/entity-builders/subsystem-builder.ts - Full file content skipped due to token limit
- packages/core/src/application/orchestration/cqrs/registries/command-handler-registry.ts - Full file content skipped due to token limit
- packages/core/src/application/ports/persistence/query-services/subsystem/subsystem-query-service.ts - Full file content skipped due to token limit
- packages/core/src/application/ports/persistence/query-services/usecase/query-models/subsystem-data-link-read-model.ts - Full file content skipped due to token limit
- packages/core/src/application/ports/persistence/query-services/usecase/query-models/usecase-components-with-subsystems-read-model.ts - Full file content skipped due to token limit
- packages/core/src/application/ports/persistence/repositories/module/module.repository.ts - Full file content skipped due to token limit
- packages/core/src/application/ports/persistence/repositories/subgraph/subgraph.repository.ts - Full file content skipped due to token limit
- packages/core/src/application/ports/persistence/repositories/subsystem/subsystem.repository.ts - Full file content skipped due to token limit
- packages/core/src/application/usecase-designer/data-links/create/build-traversal-entities.ts - Full file content skipped due to token limit
- packages/core/src/application/usecase-designer/data-links/create/create-data-link.command.ts - Full file content skipped due to token limit
- packages/core/src/application/usecase-designer/usecase/dto/component-collection-dto.ts - Full file content skipped due to token limit
- packages/core/src/application/usecase-designer/usecase/get-component-with-subsystem/build-subsystem-tree.ts - Full file content skipped due to token limit
- packages/core/src/application/usecase-designer/usecase/get-component-with-subsystem/components-with-subsystems-read-model.ts - Full file content skipped due to token limit
- packages/core/src/application/usecase-designer/usecase/get-component-with-subsystem/get-components-with-subsystems.handler.ts - Full file content skipped due to token limit
- packages/core/src/domain/services/subsystem-data-links/subsystem-data-link-derivation.service.ts - Full file content skipped due to token limit
- packages/core/src/index.ts - Full file content skipped due to token limit
- packages/core/src/shared/exceptions/conflict.exception.ts - Full file content skipped due to token limit
- packages/core/src/shared/exceptions/index.ts - Full file content skipped due to token limit
- packages/core/tests/unit/application/file-operations/upload-file/services/entity-builders/subsystem-builder.spec.ts - Full file content skipped due to token limit
- packages/core/tests/unit/application/usecase-designer/data-links/create/create-data-link.handler.spec.ts - Full file content skipped due to token limit
- packages/core/tests/unit/domain/services/subsystem-links/subsystem-data-link-derivation.service.spec.ts - Full file content skipped due to token limit
- packages/infrastructure/persistence/src/persistence-typeorm-sqllite/queries/subsystem/db-subsystem-query-service.ts - Full file content skipped due to token limit
- packages/infrastructure/persistence/src/persistence-typeorm-sqllite/repositories/data-link/data-link.repository.ts - Full file content skipped due to token limit
- packages/infrastructure/persistence/src/persistence-typeorm-sqllite/repositories/module/module.repository.ts - Full file content skipped due to token limit
- packages/infrastructure/persistence/src/persistence-typeorm-sqllite/repositories/subgraph/subgraph.repository.ts - Full file content skipped due to token limit
- packages/infrastructure/persistence/src/persistence-typeorm-sqllite/repositories/subsystem/subsystem.repository.ts - Full file content skipped due to token limit
- packages/infrastructure/persistence/tests/integration/repositories/data-link/data-link-edit.repository.integration.spec.ts - Full file content skipped due to token limit
Qualcomm AI Review
| dataLink.destinationPortSystemId, | ||
| dataLink.linkType, | ||
| dataLink.isEc, | ||
| ); | ||
| } catch (error) { | ||
| if (uow.isInTransaction()) await uow.rollback(); | ||
| throw error; | ||
| } | ||
| } | ||
|
|
||
| private async findModules( |
There was a problem hiding this comment.
[FUNCTIONALITY] Missing Transaction Rollback in Catch Block - High Severity
In both CreateDataLinkHandler and CreateDataLinkWithSubsystemsHandler, the catch block checks uow.isInTransaction() before rolling back. However, if an error occurs after startTransaction() but the transaction state becomes inconsistent, the rollback may not execute, leaving the transaction open and potentially causing connection pool exhaustion.
Issue Location: The pattern appears in both handlers:
catch (error) {
if (uow.isInTransaction()) await uow.rollback();
throw error;
}The problem is that isInTransaction() might return false even when a transaction was started, if the error corrupted the transaction state. This could lead to leaked database connections.
Fixed Code Snippet
catch (error) {
try {
await uow.rollback();
} catch (rollbackError) {
// Log rollback failure but don't mask original error
console.error('Failed to rollback transaction:', rollbackError);
}
throw error;
}Always attempt rollback unconditionally in the catch block. The rollback operation itself should be idempotent and safe to call even if no transaction is active.
| ): Promise<PortIoType | null> { | ||
| const sessionId = this.uow.getWriteContext().session.sessionId; | ||
|
|
||
| // Check session overlay first — a staged CREATE wins over the base table | ||
| const actions = await this.editActionsQs.getByTable( | ||
| sessionId, | ||
| ENTITY_NAMES.DataPort, | ||
| ); | ||
| for (const action of actions) { | ||
| if ( | ||
| action.operation === CHANGE_OPERATION.Create && |
There was a problem hiding this comment.
[PERFORMANCE] Inefficient Sequential Edit Action Queries - Medium Severity
In TypeOrmSubsystemRepository.getPortIoType(), the method calls editActionsQs.getByTable() which loads ALL edit actions for the DataPort table, then iterates through them to find a match. This is inefficient when there are many staged changes, as it loads unnecessary data into memory.
Issue Location:
const actions = await this.editActionsQs.getByTable(
sessionId,
ENTITY_NAMES.DataPort,
);
for (const action of actions) {
if (
action.operation === CHANGE_OPERATION.Create &&
action.targetSystemId === portSystemId
) {
// ...
}
}This pattern loads all DataPort edit actions when we only need actions for a specific portSystemId.
Fixed Code Snippet
// Add a new method to EditActionsQueryService:
async getByTableAndSystemId(
sessionId: number,
tableName: string,
targetSystemId: number
): Promise<EditActionRow[]> {
return this.manager
.createQueryBuilder()
.select('ea')
.from('edit_actions', 'ea')
.where('ea.session_id = :sessionId', { sessionId })
.andWhere('ea.target_table = :tableName', { tableName })
.andWhere('ea.target_system_id = :targetSystemId', { targetSystemId })
.andWhere('ea.valid_until IS NULL')
.orderBy('ea.change_id', 'ASC')
.getRawMany();
}
// Then use it in getPortIoType:
const actions = await this.editActionsQs.getByTableAndSystemId(
sessionId,
ENTITY_NAMES.DataPort,
portSystemId
);This reduces the data transferred and processed, especially important when there are many staged changes in the session.
|
|
||
| return null; | ||
| } | ||
|
|
||
| async reactivateDataLink( | ||
| systemId: number, | ||
| aggregateId: number, | ||
| payload: Record<string, unknown>, | ||
| options?: EditOptions, | ||
| ): Promise<void> { | ||
| const {session, groupId} = this.uow.getWriteContext(); |
There was a problem hiding this comment.
[SECURITY] Raw SQL Query Without Explicit Parameterization Validation - Medium Severity
In TypeOrmDataLinkRepository.reactivateDataLink(), a raw SQL query is used with template literals. While the parameters appear to be properly passed as an array, the comment indicates this is necessary because TypeORM QueryBuilder cannot express the required logic. However, there's no explicit validation that the systemId parameter is actually a number before it's interpolated.
Issue Location:
await this.manager.query(
`UPDATE edit_actions SET valid_until = $1 WHERE session_id = $2 AND target_system_id = $3 AND field_path IS NULL AND valid_until IS NULL`,
[new Date().toISOString(), session.sessionId, systemId],
);While the parameters are passed as an array (which is safe), the method signature accepts systemId: number but doesn't validate it's actually a number at runtime. If called with a non-number due to a type system bypass, this could cause issues.
Fixed Code Snippet
async reactivateDataLink(
systemId: number,
aggregateId: number,
payload: Record<string, unknown>,
options?: EditOptions,
): Promise<void> {
// Validate inputs are actually numbers
if (!Number.isInteger(systemId) || !Number.isInteger(aggregateId)) {
throw new Error('systemId and aggregateId must be integers');
}
const {session, groupId} = this.uow.getWriteContext();
const writer = this.requireWriter();
await this.manager.query(
`UPDATE edit_actions SET valid_until = $1 WHERE session_id = $2 AND target_system_id = $3 AND field_path IS NULL AND valid_until IS NULL`,
[new Date().toISOString(), session.sessionId, systemId],
);
await writer.writeCreate(
{
targetTable: ENTITY_NAMES.DataLink,
targetSystemId: systemId,
aggregateId,
payload,
...options,
},
session.sessionId,
groupId,
this.manager,
);
}Add runtime validation to ensure type safety even if TypeScript's type system is bypassed (e.g., through any casts elsewhere in the codebase).
799d1bd to
06c39cf
Compare
There was a problem hiding this comment.
Files Not Fully Analyzed
- docs/data-links/design/data-links-post-design.md - File extension .md not supported
- docs/data-links/requirements/data-links-post-requirements.md - File extension .md not supported
- packages/api/src/infrastructure-wrapper/filters/all-exceptions.filter.ts - Full file content skipped due to token limit
- packages/api/src/infrastructure-wrapper/persistence/unit-of-work/typeorm-unit-of-work.ts - Full file content skipped due to token limit
- packages/api/src/presentation/rest/common/dto/component-collection-with-subsystems.dto.ts - Full file content skipped due to token limit
- packages/api/src/presentation/rest/common/dto/subsystem-components-response.dto.ts - Full file content skipped due to token limit
- packages/api/src/presentation/rest/common/swagger-doc/dto-examples/usecase-api-examples.ts - Full file content skipped due to token limit
- packages/api/src/presentation/rest/modules/data-link/data-link.controller.ts - Full file content skipped due to token limit
- packages/api/src/presentation/rest/modules/data-link/dto/request/create-data-link-request.dto.ts - Full file content skipped due to token limit
- packages/api/src/presentation/rest/modules/data-link/dto/request/create-data-link-with-subsystems-request.dto.ts - Full file content skipped due to token limit
- packages/api/src/presentation/rest/modules/data-link/dto/response/subsystem-data-link-response.dto.ts - Full file content skipped due to token limit
- packages/core/src/application/file-operations/upload-file/services/entity-builders/subsystem-builder.ts - Full file content skipped due to token limit
- packages/core/src/application/orchestration/cqrs/registries/command-handler-registry.ts - Full file content skipped due to token limit
- packages/core/src/application/ports/persistence/query-services/subsystem/subsystem-query-service.ts - Full file content skipped due to token limit
- packages/core/src/application/ports/persistence/query-services/usecase/query-models/subsystem-data-link-read-model.ts - Full file content skipped due to token limit
- packages/core/src/application/ports/persistence/query-services/usecase/query-models/usecase-components-with-subsystems-read-model.ts - Full file content skipped due to token limit
- packages/core/src/application/ports/persistence/repositories/data-link/data-link.repository.ts - Full file content skipped due to token limit
- packages/core/src/application/ports/persistence/repositories/module/module.repository.ts - Full file content skipped due to token limit
- packages/core/src/application/ports/persistence/repositories/subgraph/subgraph.repository.ts - Full file content skipped due to token limit
- packages/core/src/application/ports/persistence/repositories/subsystem/subsystem.repository.ts - Full file content skipped due to token limit
- packages/core/src/application/usecase-designer/data-links/create/build-traversal-entities.ts - Full file content skipped due to token limit
- packages/core/src/application/usecase-designer/data-links/create/create-data-link-with-subsystems.command.ts - Full file content skipped due to token limit
- packages/core/src/application/usecase-designer/data-links/create/create-data-link.command.ts - Full file content skipped due to token limit
- packages/core/src/application/usecase-designer/data-links/create/create-data-link.handler.ts - Full file content skipped due to token limit
- packages/core/src/application/usecase-designer/usecase/dto/component-collection-dto.ts - Full file content skipped due to token limit
- packages/core/src/application/usecase-designer/usecase/get-component-with-subsystem/build-subsystem-tree.ts - Full file content skipped due to token limit
- packages/core/src/application/usecase-designer/usecase/get-component-with-subsystem/components-with-subsystems-read-model.ts - Full file content skipped due to token limit
- packages/core/src/application/usecase-designer/usecase/get-component-with-subsystem/get-components-with-subsystems.handler.ts - Full file content skipped due to token limit
- packages/core/src/domain/services/subsystem-data-links/subsystem-data-link-derivation.service.ts - Full file content skipped due to token limit
- packages/core/src/index.ts - Full file content skipped due to token limit
- packages/core/src/shared/exceptions/index.ts - Full file content skipped due to token limit
- packages/core/tests/unit/application/file-operations/upload-file/services/entity-builders/subsystem-builder.spec.ts - Full file content skipped due to token limit
- packages/core/tests/unit/application/usecase-designer/data-links/create/create-data-link-with-subsystems.handler.spec.ts - Full file content skipped due to token limit
- packages/core/tests/unit/application/usecase-designer/data-links/create/create-data-link.handler.spec.ts - Full file content skipped due to token limit
- packages/core/tests/unit/application/usecase-designer/usecase/get-component-with-subsystem/build-subsystem-tree.spec.ts - Full file content skipped due to token limit
- packages/core/tests/unit/application/usecase-designer/usecase/get-component-with-subsystem/get-components-with-subsystems.handler.spec.ts - Full file content skipped due to token limit
- packages/core/tests/unit/domain/services/subsystem-links/subsystem-data-link-derivation.service.spec.ts - Full file content skipped due to token limit
- packages/infrastructure/persistence/src/persistence-typeorm-sqllite/queries/subsystem/db-subsystem-query-service.ts - Full file content skipped due to token limit
- packages/infrastructure/persistence/src/persistence-typeorm-sqllite/repositories/data-link/data-link.repository.ts - Full file content skipped due to token limit
- packages/infrastructure/persistence/src/persistence-typeorm-sqllite/repositories/module/module.repository.ts - Full file content skipped due to token limit
- packages/infrastructure/persistence/src/persistence-typeorm-sqllite/repositories/subgraph/subgraph.repository.ts - Full file content skipped due to token limit
- packages/infrastructure/persistence/src/persistence-typeorm-sqllite/repositories/subsystem/subsystem.repository.ts - Full file content skipped due to token limit
- packages/infrastructure/persistence/tests/integration/repositories/data-link/data-link-edit.repository.integration.spec.ts - Full file content skipped due to token limit
Qualcomm AI Review
|
|
||
| return (row?.dp_port_io_type as PortIoType) ?? null; | ||
| } | ||
|
|
||
| async isPortOccupiedAsSource( | ||
| portSystemId: number, | ||
| fileSystemId: number, | ||
| ): Promise<boolean> { | ||
| const count = await this.manager | ||
| .createQueryBuilder() | ||
| .select('1') |
There was a problem hiding this comment.
[High Severity - Functionality] Race condition in subsystem port occupation checks
In CreateDataLinkWithSubsystemsHandler.handleBranchB(), the port occupation checks (isPortOccupiedAsSource and isPortOccupiedAsDest) are performed before the transaction commits. However, these checks query the base table only and don't account for concurrent transactions that might be creating SLS segments with the same ports. This could allow two concurrent requests to pass validation and both create links using the same subsystem port, violating the single-occupancy constraint (FR-DLS-07).
Impact: Data integrity violation - multiple SLS segments could reference the same subsystem boundary port as source or destination, breaking the architectural constraint that each subsystem port can only be used once.
Recommendation:
- Add row-level locking when checking port occupation (SELECT FOR UPDATE)
- Perform the occupation check within the same transaction as the write
- Consider adding a unique constraint at the database level as a safety net
Fixed Code Snippet
// In subsystem.repository.ts
async isPortOccupiedAsSource(
portSystemId: number,
fileSystemId: number,
): Promise<boolean> {
// Add FOR UPDATE to lock the rows during check
const count = await this.manager
.createQueryBuilder()
.select('1')
.from(ENTITY_NAMES.SubsystemDataLink, 'sls')
.where(
'sls.sourcePortSystemId = :portSystemId AND sls.fileSystemId = :fileSystemId',
{portSystemId, fileSystemId},
)
.setLock('pessimistic_write') // Add row lock
.getCount();
return count > 0;
}| fileSystemId, | ||
| uow, | ||
| subsystemRepo, | ||
| ); | ||
| } catch (error) { | ||
| if (uow.isInTransaction()) await uow.rollback(); | ||
| throw error; | ||
| } | ||
| } | ||
|
|
||
| private async handleBranchA( |
There was a problem hiding this comment.
[High Severity - Error Handling] Missing transaction rollback in catch block
In CreateDataLinkWithSubsystemsHandler.handle(), the catch block checks uow.isInTransaction() before rolling back. However, if an error occurs during uow.commit(), the transaction might be in an indeterminate state where isInTransaction() returns false but the transaction hasn't been properly cleaned up. This could lead to connection leaks or inconsistent database state.
Impact: Resource leaks and potential database inconsistencies when errors occur during commit phase.
Recommendation: Always attempt rollback in the catch block regardless of transaction state, and handle any rollback errors gracefully. The database driver should handle the case where no transaction is active.
Fixed Code Snippet
async handle(
command: CreateDataLinkWithSubsystemsCommand,
): Promise<ComponentCollectionWithSubsystemsDto> {
const uow = this.uow;
await uow.startTransaction();
try {
// ... handler logic ...
await uow.commit();
return result;
} catch (error) {
// Always attempt rollback, let the UoW handle transaction state
try {
await uow.rollback();
} catch (rollbackError) {
// Log rollback failure but throw original error
console.error('Rollback failed:', rollbackError);
}
throw error;
}
}| await uow.commit(); | ||
| return emptyCollection(slsSegments.map(sls => mapSubsystemDataLink(sls))); | ||
| } | ||
|
|
||
| private async findModules( | ||
| moduleRepo: ReturnType<UnitOfWork['getModuleRepository']>, | ||
| subsystemRepo: ReturnType<UnitOfWork['getSubsystemRepository']>, | ||
| srcNodeId: number, | ||
| dstNodeId: number, | ||
| fileSystemId: number, | ||
| ) { |
There was a problem hiding this comment.
[Medium Severity - Performance] Sequential database queries in module validation
In CreateDataLinkWithSubsystemsHandler.findModules(), the method performs two sequential findModulePortsForLink() calls followed by two more sequential subsystemExists() calls in the error paths. This results in up to 4 sequential database round-trips when both modules are invalid. These queries could be batched or parallelized to reduce latency.
Impact: Increased response time for validation failures, especially noticeable under high load or with network latency to the database.
Recommendation: Use Promise.all() for the subsystem existence checks in error paths, similar to how the module lookups are already parallelized.
Fixed Code Snippet
private async findModules(
moduleRepo: ReturnType<UnitOfWork['getModuleRepository']>,
subsystemRepo: ReturnType<UnitOfWork['getSubsystemRepository']>,
srcNodeId: number,
dstNodeId: number,
fileSystemId: number,
) {
const [srcModule, dstModule] = await Promise.all([
moduleRepo.findModulePortsForLink(srcNodeId, fileSystemId),
moduleRepo.findModulePortsForLink(dstNodeId, fileSystemId),
]);
// Batch subsystem checks if either module is null
if (srcModule === null || dstModule === null) {
const [srcIsSubsystem, dstIsSubsystem] = await Promise.all([
srcModule === null ? subsystemRepo.subsystemExists(srcNodeId, fileSystemId) : Promise.resolve(false),
dstModule === null ? subsystemRepo.subsystemExists(dstNodeId, fileSystemId) : Promise.resolve(false),
]);
if (srcModule === null) {
throw srcIsSubsystem
? new DomainRuleViolationException([/* ... */])
: new ResourceNotFoundException(/* ... */);
}
if (dstModule === null) {
throw dstIsSubsystem
? new DomainRuleViolationException([/* ... */])
: new ResourceNotFoundException(/* ... */);
}
}
return [srcModule, dstModule] as const;
}| ]); | ||
| } | ||
|
|
||
| // FR-DLS-03 + FR-DLS-08 + FR-DLS-07: validate subsystem-side ports | ||
| if (srcIsSubsystem) { | ||
| const srcPortType = await subsystemRepo.getPortIoType( | ||
| srcPortId, | ||
| fileSystemId, | ||
| ); | ||
| if (srcPortType === null) { | ||
| throw new ResourceNotFoundException( |
There was a problem hiding this comment.
[Medium Severity - Error Handling] Incomplete validation of port ownership in subsystem branch
In CreateDataLinkWithSubsystemsHandler.handleBranchB(), when validating subsystem ports, the code checks getPortIoType() and throws ResourceNotFoundException if null. However, this doesn't distinguish between a port that doesn't exist at all versus a port that exists but belongs to a different node. The error message "Source port not found" is misleading if the port exists but is attached to the wrong node.
Impact: Confusing error messages for API consumers when they provide a valid port ID that belongs to a different node than specified.
Recommendation: Add an explicit check using portExists() first, then verify ownership, similar to the pattern used in findPort() for Branch A.
Fixed Code Snippet
if (srcIsSubsystem) {
// First check if port exists at all
const portExists = await subsystemRepo.portExists(srcPortId, fileSystemId);
if (!portExists) {
throw new ResourceNotFoundException(
`Source port ${BinaryUtils.toHexString(srcPortId)} not found.`,
);
}
// Then check port type
const srcPortType = await subsystemRepo.getPortIoType(
srcPortId,
fileSystemId,
);
if (srcPortType !== PORT_IO_TYPE.InputOutput) {
throw new DomainRuleViolationException([
{
code: 'WRONG_SUBSYSTEM_PORT_TYPE',
message: `Source subsystem port must be InputOutput, got ${srcPortType}.`,
severity: IssueSeverity.Error,
},
]);
}
// Finally check occupation
const occupied = await subsystemRepo.isPortOccupiedAsSource(
srcPortId,
fileSystemId,
);
if (occupied) {
throw new DomainRuleViolationException([
{
code: 'PORT_ALREADY_OCCUPIED',
message: `Source port ${BinaryUtils.toHexString(srcPortId)} is already occupied as source of an SLS.`,
severity: IssueSeverity.Error,
},
]);
}
}
06c39cf to
8670d86
Compare
There was a problem hiding this comment.
Files Not Fully Analyzed
- docs/data-links/design/data-links-post-design.md - File extension .md not supported
- docs/data-links/requirements/data-links-post-requirements.md - File extension .md not supported
- packages/api/src/infrastructure-wrapper/filters/all-exceptions.filter.ts - Full file content skipped due to token limit
- packages/api/src/infrastructure-wrapper/persistence/unit-of-work/typeorm-unit-of-work.ts - Full file content skipped due to token limit
- packages/api/src/presentation/rest/common/dto/component-collection-with-subsystems.dto.ts - Full file content skipped due to token limit
- packages/api/src/presentation/rest/common/dto/subsystem-components-response.dto.ts - Full file content skipped due to token limit
- packages/api/src/presentation/rest/common/swagger-doc/dto-examples/usecase-api-examples.ts - Full file content skipped due to token limit
- packages/api/src/presentation/rest/modules/data-link/data-link.controller.ts - Full file content skipped due to token limit
- packages/api/src/presentation/rest/modules/data-link/dto/request/create-data-link-request.dto.ts - Full file content skipped due to token limit
- packages/api/src/presentation/rest/modules/data-link/dto/request/create-data-link-with-subsystems-request.dto.ts - Full file content skipped due to token limit
- packages/core/src/application/file-operations/upload-file/services/entity-builders/subsystem-builder.ts - Full file content skipped due to token limit
- packages/core/src/application/orchestration/cqrs/registries/command-handler-registry.ts - Full file content skipped due to token limit
- packages/core/src/application/ports/persistence/query-services/subsystem/subsystem-query-service.ts - Full file content skipped due to token limit
- packages/core/src/application/ports/persistence/query-services/usecase/query-models/subsystem-data-link-read-model.ts - Full file content skipped due to token limit
- packages/core/src/application/ports/persistence/query-services/usecase/query-models/usecase-components-with-subsystems-read-model.ts - Full file content skipped due to token limit
- packages/core/src/application/ports/persistence/repositories/data-link/data-link.repository.ts - Full file content skipped due to token limit
- packages/core/src/application/ports/persistence/repositories/module/module.repository.ts - Full file content skipped due to token limit
- packages/core/src/application/ports/persistence/repositories/subgraph/subgraph.repository.ts - Full file content skipped due to token limit
- packages/core/src/application/ports/persistence/repositories/subsystem/subsystem.repository.ts - Full file content skipped due to token limit
- packages/core/src/application/usecase-designer/data-links/create/build-traversal-entities.ts - Full file content skipped due to token limit
- packages/core/src/application/usecase-designer/data-links/create/create-data-link.command.ts - Full file content skipped due to token limit
- packages/core/src/application/usecase-designer/data-links/create/create-data-link.handler.ts - Full file content skipped due to token limit
- packages/core/src/application/usecase-designer/usecase/dto/component-collection-dto.ts - Full file content skipped due to token limit
- packages/core/src/application/usecase-designer/usecase/get-component-with-subsystem/build-subsystem-tree.ts - Full file content skipped due to token limit
- packages/core/src/application/usecase-designer/usecase/get-component-with-subsystem/components-with-subsystems-read-model.ts - Full file content skipped due to token limit
- packages/core/src/application/usecase-designer/usecase/get-component-with-subsystem/get-components-with-subsystems.handler.ts - Full file content skipped due to token limit
- packages/core/src/domain/services/subsystem-data-links/subsystem-data-link-derivation.service.ts - Full file content skipped due to token limit
- packages/core/src/index.ts - Full file content skipped due to token limit
- packages/core/src/shared/exceptions/conflict.exception.ts - Full file content skipped due to token limit
- packages/core/src/shared/exceptions/index.ts - Full file content skipped due to token limit
- packages/core/tests/unit/application/file-operations/upload-file/services/entity-builders/subsystem-builder.spec.ts - Full file content skipped due to token limit
- packages/core/tests/unit/application/usecase-designer/data-links/create/create-data-link-with-subsystems.handler.spec.ts - Full file content skipped due to token limit
- packages/core/tests/unit/application/usecase-designer/data-links/create/create-data-link.handler.spec.ts - Full file content skipped due to token limit
- packages/core/tests/unit/application/usecase-designer/usecase/get-component-with-subsystem/build-subsystem-tree.spec.ts - Full file content skipped due to token limit
- packages/core/tests/unit/application/usecase-designer/usecase/get-component-with-subsystem/get-components-with-subsystems.handler.spec.ts - Full file content skipped due to token limit
- packages/core/tests/unit/domain/services/subsystem-links/subsystem-data-link-derivation.service.spec.ts - Full file content skipped due to token limit
- packages/infrastructure/persistence/src/persistence-typeorm-sqllite/queries/subsystem/db-subsystem-query-service.ts - Full file content skipped due to token limit
- packages/infrastructure/persistence/src/persistence-typeorm-sqllite/repositories/data-link/data-link.repository.ts - Full file content skipped due to token limit
- packages/infrastructure/persistence/src/persistence-typeorm-sqllite/repositories/module/module.repository.ts - Full file content skipped due to token limit
- packages/infrastructure/persistence/src/persistence-typeorm-sqllite/repositories/subgraph/subgraph.repository.ts - Full file content skipped due to token limit
- packages/infrastructure/persistence/src/persistence-typeorm-sqllite/repositories/subsystem/subsystem.repository.ts - Full file content skipped due to token limit
- packages/infrastructure/persistence/tests/integration/repositories/data-link/data-link-edit.repository.integration.spec.ts - Full file content skipped due to token limit
Qualcomm AI Review
| private readonly uow: UnitOfWork, | ||
| private readonly queryServices: QueryServices, | ||
| private readonly idGeneration: IdGenerationPort, | ||
| ) {} | ||
|
|
||
| handle(_command: CreateDataLinkCommand): Promise<ComponentCollectionDto> { | ||
| async handle( | ||
| command: CreateDataLinkCommand, | ||
| ): Promise<ComponentCollectionDto> { | ||
| const uow = this.uow; | ||
| await uow.startTransaction(); | ||
| try { | ||
| const {session} = uow.getWriteContext(); |
There was a problem hiding this comment.
[FUNCTIONALITY][High] Missing transaction rollback in error handling paths
In both CreateDataLinkHandler and CreateDataLinkWithSubsystemsHandler, the error handling catch block checks uow.isInTransaction() before rolling back. However, if an error occurs after startTransaction() but the transaction state is somehow not properly tracked, the rollback won't execute, potentially leaving the database in an inconsistent state.
Additionally, the handlers don't handle the case where commit() itself might fail. If commit() throws an error, the transaction remains open without cleanup.
Impact: Database inconsistency, resource leaks, and potential deadlocks in concurrent scenarios.
Fixed Code Snippet
async handle(
command: CreateDataLinkCommand,
): Promise<ComponentCollectionDto> {
const uow = this.uow;
let transactionStarted = false;
try {
await uow.startTransaction();
transactionStarted = true;
// ... existing logic ...
await uow.commit();
transactionStarted = false;
return this.buildDto(/* ... */);
} catch (error) {
if (transactionStarted) {
try {
await uow.rollback();
} catch (rollbackError) {
// Log rollback failure but throw original error
console.error('Rollback failed:', rollbackError);
}
}
throw error;
}
}| sessionId, | ||
| ); | ||
| } | ||
|
|
||
| async createDataLink( | ||
| dataLink: DataLink, | ||
| boundaryPortPayloads: BoundaryPortPayload[], | ||
| options?: EditOptions, | ||
| ): Promise<void> { | ||
| const {session, groupId} = this.uow.getWriteContext(); | ||
| const fileSystemId = dataLink.fileSystemId; |
There was a problem hiding this comment.
[FUNCTIONALITY][High] Race condition in duplicate link detection
The findByPortPair check and subsequent createDataLink call are not atomic. In a concurrent scenario, two requests could both pass the duplicate check and then both attempt to create the same link, potentially violating database constraints or creating duplicate records.
The time window between checking for duplicates (line 97-106 in create-data-link.handler.ts) and actually creating the link (line 223) allows for race conditions.
Impact: Duplicate data links could be created in high-concurrency scenarios, violating business rules and data integrity.
Recommendation: Implement optimistic locking or use database-level unique constraints with proper error handling. Alternatively, use a distributed lock or database transaction isolation level that prevents phantom reads.
Fixed Code Snippet
// In DataLinkRepository implementation
async createDataLink(
dataLink: DataLink,
boundaryPortPayloads: BoundaryPortPayload[],
options?: EditOptions,
): Promise<void> {
const {session, groupId} = this.uow.getWriteContext();
const fileSystemId = dataLink.fileSystemId;
const writer = this.requireWriter();
// Check for duplicate immediately before write within same transaction
const existing = await this.findByPortPair(
dataLink.sourcePortSystemId,
dataLink.destinationPortSystemId,
fileSystemId,
);
if (existing !== null && !existing.isDeleted) {
throw new ConflictException(
`DataLink for ports already exists (detected during write)`,
);
}
// Continue with creation...
for (const bp of boundaryPortPayloads) {
// ... existing code
}
}Alternatively, add a unique constraint at the database level:
ALTER TABLE data_links ADD CONSTRAINT unique_port_pair
UNIQUE (source_port_system_id, destination_port_system_id, file_system_id);| const {session, groupId} = this.uow.getWriteContext(); | ||
| const fileSystemId = dataLink.fileSystemId; | ||
| const writer = this.requireWriter(); | ||
|
|
||
| for (const bp of boundaryPortPayloads) { | ||
| await writer.writeCreate( | ||
| { | ||
| targetTable: ENTITY_NAMES.Node, | ||
| targetSystemId: bp.nodeSystemId, | ||
| aggregateId: dataLink.systemId, | ||
| payload: { |
There was a problem hiding this comment.
[PERFORMANCE][Medium] N+1 query pattern in boundary port creation
In TypeOrmDataLinkRepository.createDataLink(), boundary ports are created in individual loops (lines 58-75 for nodes, 77-97 for ports). Each iteration calls writer.writeCreate() which likely executes a separate database operation. For links crossing multiple subsystem boundaries, this results in N+1 queries.
Impact: Performance degradation when creating links with many boundary crossings. In a scenario with 10 subsystem boundaries, this would result in 20+ individual database writes instead of a batch operation.
Fixed Code Snippet
async createDataLink(
dataLink: DataLink,
boundaryPortPayloads: BoundaryPortPayload[],
options?: EditOptions,
): Promise<void> {
const {session, groupId} = this.uow.getWriteContext();
const fileSystemId = dataLink.fileSystemId;
const writer = this.requireWriter();
// Batch node creation
const nodeWrites = boundaryPortPayloads.map(bp => ({
targetTable: ENTITY_NAMES.Node,
targetSystemId: bp.nodeSystemId,
aggregateId: dataLink.systemId,
payload: {
type: 'subsystem',
parentId: bp.nodeParentId ?? null,
fileSystemId: bp.fileSystemId,
},
...options,
}));
await writer.writeBatch(nodeWrites, session.sessionId, groupId, this.manager);
// Batch port creation
const portWrites = boundaryPortPayloads.map(bp => ({
targetTable: ENTITY_NAMES.DataPort,
targetSystemId: bp.portSystemId,
aggregateId: dataLink.systemId,
payload: {
dataPortId: bp.dataPortId,
portIoType: bp.portIoType,
isStatic: false,
name: '',
nodeSystemId: bp.nodeSystemId,
fileSystemId: bp.fileSystemId,
},
...options,
}));
await writer.writeBatch(portWrites, session.sessionId, groupId, this.manager);
// ... rest of the method
}Note: This assumes PendingChangeWriter has or can be extended with a writeBatch() method. If not available, consider implementing it for better performance.
add core/persistence layer support to create data links. Implement POST /data-links and /data-links/with-subsystems Signed-off-by: aboppay <aboppay@qti.qualcomm.com>
8670d86 to
df1c335
Compare
There was a problem hiding this comment.
Files Not Fully Analyzed
- docs/data-links/design/data-links-post-design.md - File extension .md not supported
- docs/data-links/requirements/data-links-post-requirements.md - File extension .md not supported
- packages/api/src/infrastructure-wrapper/filters/all-exceptions.filter.ts - Full file content skipped due to token limit
- packages/api/src/infrastructure-wrapper/persistence/unit-of-work/typeorm-unit-of-work.ts - Full file content skipped due to token limit
- packages/api/src/presentation/rest/common/dto/component-collection-with-subsystems.dto.ts - Full file content skipped due to token limit
- packages/api/src/presentation/rest/common/dto/subsystem-components-response.dto.ts - Full file content skipped due to token limit
- packages/api/src/presentation/rest/common/swagger-doc/dto-examples/usecase-api-examples.ts - Full file content skipped due to token limit
- packages/api/src/presentation/rest/modules/data-link/data-link.controller.ts - Full file content skipped due to token limit
- packages/api/src/presentation/rest/modules/data-link/dto/request/create-data-link-request.dto.ts - Full file content skipped due to token limit
- packages/api/src/presentation/rest/modules/data-link/dto/request/create-data-link-with-subsystems-request.dto.ts - Full file content skipped due to token limit
- packages/core/src/application/file-operations/upload-file/services/entity-builders/subsystem-builder.ts - Full file content skipped due to token limit
- packages/core/src/application/orchestration/cqrs/registries/command-handler-registry.ts - Full file content skipped due to token limit
- packages/core/src/application/ports/persistence/query-services/subsystem/subsystem-query-service.ts - Full file content skipped due to token limit
- packages/core/src/application/ports/persistence/query-services/usecase/query-models/subsystem-data-link-read-model.ts - Full file content skipped due to token limit
- packages/core/src/application/ports/persistence/query-services/usecase/query-models/usecase-components-with-subsystems-read-model.ts - Full file content skipped due to token limit
- packages/core/src/application/ports/persistence/repositories/data-link/data-link.repository.ts - Full file content skipped due to token limit
- packages/core/src/application/ports/persistence/repositories/module/module.repository.ts - Full file content skipped due to token limit
- packages/core/src/application/ports/persistence/repositories/subgraph/subgraph.repository.ts - Full file content skipped due to token limit
- packages/core/src/application/ports/persistence/repositories/subsystem/subsystem.repository.ts - Full file content skipped due to token limit
- packages/core/src/application/usecase-designer/data-links/create/build-traversal-entities.ts - Full file content skipped due to token limit
- packages/core/src/application/usecase-designer/data-links/create/create-data-link-with-subsystems.command.ts - Full file content skipped due to token limit
- packages/core/src/application/usecase-designer/data-links/create/create-data-link.command.ts - Full file content skipped due to token limit
- packages/core/src/application/usecase-designer/data-links/create/create-data-link.handler.ts - Full file content skipped due to token limit
- packages/core/src/application/usecase-designer/usecase/dto/component-collection-dto.ts - Full file content skipped due to token limit
- packages/core/src/application/usecase-designer/usecase/get-component-with-subsystem/build-subsystem-tree.ts - Full file content skipped due to token limit
- packages/core/src/application/usecase-designer/usecase/get-component-with-subsystem/components-with-subsystems-read-model.ts - Full file content skipped due to token limit
- packages/core/src/application/usecase-designer/usecase/get-component-with-subsystem/get-components-with-subsystems.handler.ts - Full file content skipped due to token limit
- packages/core/src/domain/services/subsystem-data-links/subsystem-data-link-derivation.service.ts - Full file content skipped due to token limit
- packages/core/src/index.ts - Full file content skipped due to token limit
- packages/core/src/shared/exceptions/conflict.exception.ts - Full file content skipped due to token limit
- packages/core/src/shared/exceptions/index.ts - Full file content skipped due to token limit
- packages/core/tests/unit/application/file-operations/upload-file/services/entity-builders/subsystem-builder.spec.ts - Full file content skipped due to token limit
- packages/core/tests/unit/application/usecase-designer/data-links/create/create-data-link-with-subsystems.handler.spec.ts - Full file content skipped due to token limit
- packages/core/tests/unit/application/usecase-designer/data-links/create/create-data-link.handler.spec.ts - Full file content skipped due to token limit
- packages/core/tests/unit/application/usecase-designer/usecase/get-component-with-subsystem/build-subsystem-tree.spec.ts - Full file content skipped due to token limit
- packages/core/tests/unit/application/usecase-designer/usecase/get-component-with-subsystem/get-components-with-subsystems.handler.spec.ts - Full file content skipped due to token limit
- packages/core/tests/unit/domain/services/subsystem-links/subsystem-data-link-derivation.service.spec.ts - Full file content skipped due to token limit
- packages/infrastructure/persistence/src/persistence-typeorm-sqllite/queries/subsystem/db-subsystem-query-service.ts - Full file content skipped due to token limit
- packages/infrastructure/persistence/src/persistence-typeorm-sqllite/queries/usecase/usecase-query-mappers.ts - Full file content skipped due to token limit
- packages/infrastructure/persistence/src/persistence-typeorm-sqllite/repositories/data-link/data-link.repository.ts - Full file content skipped due to token limit
- packages/infrastructure/persistence/src/persistence-typeorm-sqllite/repositories/module/module.repository.ts - Full file content skipped due to token limit
- packages/infrastructure/persistence/src/persistence-typeorm-sqllite/repositories/subgraph/subgraph.repository.ts - Full file content skipped due to token limit
- packages/infrastructure/persistence/src/persistence-typeorm-sqllite/repositories/subsystem/subsystem.repository.ts - Full file content skipped due to token limit
- packages/infrastructure/persistence/tests/integration/repositories/data-link/data-link-edit.repository.integration.spec.ts - Full file content skipped due to token limit
Qualcomm AI Review
| deleted: changed.deleted.map(row => baseToDataLink(row)), | ||
| }; | ||
| } | ||
|
|
||
| async createDataLink( | ||
| dataLink: DataLink, | ||
| boundaryPortPayloads: BoundaryPortPayload[], | ||
| options?: EditOptions, | ||
| ): Promise<void> { | ||
| const {session, groupId} = this.uow.getWriteContext(); | ||
| const fileSystemId = dataLink.fileSystemId; | ||
| const writer = this.requireWriter(); | ||
|
|
||
| for (const bp of boundaryPortPayloads) { | ||
| await writer.writeCreate( | ||
| { | ||
| targetTable: ENTITY_NAMES.Node, | ||
| targetSystemId: bp.nodeSystemId, | ||
| aggregateId: dataLink.systemId, |
There was a problem hiding this comment.
[FUNCTIONALITY] Missing Transaction Rollback in Repository Write Operations - High Severity
The createDataLink method in TypeOrmDataLinkRepository performs multiple sequential write operations (boundary ports, data link, SLS segments) but doesn't wrap them in an explicit transaction or handle partial failure scenarios. If any write operation fails after the first one succeeds, the database could be left in an inconsistent state with orphaned records.
The method relies on the caller to manage transactions, but there's no guarantee that the UnitOfWork has started a transaction before this method is called. Additionally, if an exception occurs during the loop iterations, previously written records won't be rolled back.
Fixed Code Snippet
async createDataLink(
dataLink: DataLink,
boundaryPortPayloads: BoundaryPortPayload[],
options?: EditOptions,
): Promise<void> {
const {session, groupId} = this.uow.getWriteContext();
const fileSystemId = dataLink.fileSystemId;
const writer = this.requireWriter();
// Ensure we're in a transaction
if (!this.uow.isInTransaction()) {
throw new Error('createDataLink must be called within an active transaction');
}
try {
// Write boundary port nodes
for (const bp of boundaryPortPayloads) {
await writer.writeCreate(
{
targetTable: ENTITY_NAMES.Node,
targetSystemId: bp.nodeSystemId,
aggregateId: dataLink.systemId,
payload: {
type: 'subsystem',
parentId: bp.nodeParentId ?? null,
fileSystemId: bp.fileSystemId,
},
...options,
},
session.sessionId,
groupId,
this.manager,
);
}
// Write boundary port data
for (const bp of boundaryPortPayloads) {
await writer.writeCreate(
{
targetTable: ENTITY_NAMES.DataPort,
targetSystemId: bp.portSystemId,
aggregateId: dataLink.systemId,
payload: {
dataPortId: bp.dataPortId,
portIoType: bp.portIoType,
isStatic: false,
name: '',
nodeSystemId: bp.nodeSystemId,
fileSystemId: bp.fileSystemId,
},
...options,
},
session.sessionId,
groupId,
this.manager,
);
}
// Write data link
await writer.writeCreate(
{
targetTable: ENTITY_NAMES.DataLink,
targetSystemId: dataLink.systemId,
aggregateId: dataLink.systemId,
payload: {
sourceNodeSystemId: dataLink.sourceNodeSystemId,
destinationNodeSystemId: dataLink.destinationNodeSystemId,
sourcePortSystemId: dataLink.sourcePortSystemId,
destinationPortSystemId: dataLink.destinationPortSystemId,
linkType: dataLink.linkType,
sourceSubgraphSystemId: dataLink.sourceSubgraphSystemId,
destSubgraphSystemId: dataLink.destSubgraphSystemId,
isEc: dataLink.isEc ?? null,
fileSystemId,
},
...options,
},
session.sessionId,
groupId,
this.manager,
);
// Write SLS segments
for (const sls of dataLink.subsystemDataLinks) {
await writer.writeCreate(
{
targetTable: ENTITY_NAMES.SubsystemDataLink,
targetSystemId: sls.systemId,
aggregateId: dataLink.systemId,
payload: {
sourceNodeSystemId: sls.sourceNodeSystemId,
destinationNodeSystemId: sls.destinationNodeSystemId,
sourcePortSystemId: sls.sourcePortSystemId,
destinationPortSystemId: sls.destinationPortSystemId,
dataLinkSystemId: sls.dataLinkSystemId,
fileSystemId,
},
...options,
},
session.sessionId,
groupId,
this.manager,
);
}
} catch (error) {
// Transaction will be rolled back by the caller (UnitOfWork)
throw error;
}
}| fileSystemId, | ||
| uow, | ||
| subsystemRepo, | ||
| ); | ||
| } catch (error) { | ||
| if (uow.isInTransaction()) await uow.rollback(); | ||
| throw error; | ||
| } | ||
| } | ||
|
|
||
| private async handleBranchA( | ||
| command: CreateDataLinkWithSubsystemsCommand, | ||
| srcNodeId: number, | ||
| dstNodeId: number, | ||
| srcPortId: number, |
There was a problem hiding this comment.
[FUNCTIONALITY] Incomplete Error Context in Handler Exception Handling - High Severity
The CreateDataLinkWithSubsystemsHandler catches all errors in a generic catch block and re-throws them without adding context about which operation failed. This makes debugging difficult when errors occur during the multi-step process (module lookup, port validation, segment derivation, persistence).
Additionally, the handler doesn't distinguish between expected domain exceptions (like validation failures) and unexpected infrastructure errors (like database connection issues), which could lead to inappropriate error responses being sent to the client.
Fixed Code Snippet
async handle(
command: CreateDataLinkWithSubsystemsCommand,
): Promise<ComponentCollectionWithSubsystemsDto> {
const uow = this.uow;
await uow.startTransaction();
try {
const {session} = uow.getWriteContext();
const fileSystemId = session.fileSystemId;
const srcNodeId = Number.parseInt(command.sourceNodeSystemId, 10);
const dstNodeId = Number.parseInt(command.destinationNodeSystemId, 10);
const srcPortId = Number.parseInt(command.sourcePortSystemId, 10);
const dstPortId = Number.parseInt(command.destinationPortSystemId, 10);
// FR-DLS-04: self-loop check
if (srcNodeId === dstNodeId) {
throw new DomainRuleViolationException([
{
code: 'SELF_LOOP',
message: `Source and destination node must differ: ${BinaryUtils.toHexString(srcNodeId)}`,
severity: IssueSeverity.Error,
},
]);
}
const subsystemRepo = uow.getSubsystemRepository();
const srcIsSubsystem = await subsystemRepo.subsystemExists(
srcNodeId,
fileSystemId,
);
const dstIsSubsystem = await subsystemRepo.subsystemExists(
dstNodeId,
fileSystemId,
);
if (srcIsSubsystem || dstIsSubsystem) {
return this.handleBranchB(
command,
srcNodeId,
dstNodeId,
srcPortId,
dstPortId,
srcIsSubsystem,
dstIsSubsystem,
fileSystemId,
uow,
subsystemRepo,
);
}
return this.handleBranchA(
command,
srcNodeId,
dstNodeId,
srcPortId,
dstPortId,
fileSystemId,
uow,
subsystemRepo,
);
} catch (error) {
if (uow.isInTransaction()) await uow.rollback();
// Re-throw domain exceptions as-is
if (
error instanceof DomainRuleViolationException ||
error instanceof ConflictException ||
error instanceof ResourceNotFoundException
) {
throw error;
}
// Wrap infrastructure errors with context
throw new Error(
`Failed to create data link with subsystems: ${error instanceof Error ? error.message : String(error)}`,
{cause: error}
);
}
}| } | ||
| return map; | ||
| } | ||
|
|
||
| async getPortIoType( | ||
| portSystemId: number, | ||
| fileSystemId: number, | ||
| ): Promise<PortIoType | null> { | ||
| const sessionId = this.uow.getWriteContext().session.sessionId; | ||
|
|
||
| // Check session overlay first — a staged CREATE wins over the base table | ||
| const actions = await this.editActionsQs.getByTable( | ||
| sessionId, | ||
| ENTITY_NAMES.DataPort, | ||
| ); | ||
| for (const action of actions) { | ||
| if ( | ||
| action.operation === CHANGE_OPERATION.Create && | ||
| action.targetSystemId === portSystemId | ||
| ) { | ||
| const p = action.newValue as Record<string, unknown>; | ||
| if (Number(p['fileSystemId']) === fileSystemId) { | ||
| return (p['portIoType'] as PortIoType) ?? null; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Fall through to base table | ||
| const row = await this.manager | ||
| .createQueryBuilder() | ||
| .select(['dp.portIoType']) | ||
| .from(ENTITY_NAMES.DataPort, 'dp') | ||
| .where('dp.systemId = :systemId AND dp.fileSystemId = :fileSystemId', { | ||
| systemId: portSystemId, | ||
| fileSystemId, | ||
| }) | ||
| .getRawOne<{dp_port_io_type: string}>(); | ||
|
|
||
| return (row?.dp_port_io_type as PortIoType) ?? null; | ||
| } | ||
|
|
||
| async isPortOccupiedAsSource( | ||
| portSystemId: number, | ||
| fileSystemId: number, | ||
| ): Promise<boolean> { | ||
| const count = await this.manager |
There was a problem hiding this comment.
[PERFORMANCE] Inefficient Sequential Port Type Lookup - Medium Severity
The getPortIoType method in TypeOrmSubsystemRepository first queries the edit_actions table by fetching ALL actions for the DataPort table in the session, then iterates through them to find a match. This is inefficient when there are many staged port changes in the session.
A more efficient approach would be to query edit_actions with a WHERE clause filtering by both targetSystemId and targetTable, or to use a single query that checks both the overlay and base table with a UNION.
Fixed Code Snippet
async getPortIoType(
portSystemId: number,
fileSystemId: number,
): Promise<PortIoType | null> {
const sessionId = this.uow.getWriteContext().session.sessionId;
// Check session overlay with targeted query
const overlayRow = await this.manager
.createQueryBuilder()
.select(['ea.new_value'])
.from('edit_actions', 'ea')
.where(
'ea.session_id = :sessionId AND ea.target_table = :table AND ea.target_system_id = :systemId AND ea.operation = :op AND ea.valid_until IS NULL',
{
sessionId,
table: ENTITY_NAMES.DataPort,
systemId: portSystemId,
op: CHANGE_OPERATION.Create,
},
)
.getRawOne<{ea_new_value: string}>();
if (overlayRow) {
const payload = JSON.parse(overlayRow.ea_new_value) as Record<string, unknown>;
if (Number(payload['fileSystemId']) === fileSystemId) {
return (payload['portIoType'] as PortIoType) ?? null;
}
}
// Fall through to base table
const row = await this.manager
.createQueryBuilder()
.select(['dp.portIoType'])
.from(ENTITY_NAMES.DataPort, 'dp')
.where('dp.systemId = :systemId AND dp.fileSystemId = :fileSystemId', {
systemId: portSystemId,
fileSystemId,
})
.getRawOne<{dp_port_io_type: string}>();
return (row?.dp_port_io_type as PortIoType) ?? null;
}2 participants
add core/persistence layer support to create data links.
Implement POST /data-links and /data-links/with-subsystems