Conversation
POST /control-links and /control-links/with-subsystem requirements are added Signed-off-by: Avinash Boppay <aboppay@qti.qualcomm.com>
Signed-off-by: aboppay <aboppay@qti.qualcomm.com>
| * FR-CLS-04 Step 1: A subsystem control port may carry at most two connections — | ||
| * one inner (to a node inside the subsystem) and one outer (to a node outside). | ||
| * Rejects with 422 if the same side is already occupied. | ||
| */ | ||
| private async checkSubsystemPortUniqueness( | ||
| portOwnerNodeId: number, | ||
| portSystemId: number, | ||
| _newOtherNodeId: number, | ||
| portOwnerType: typeof NodeType[keyof typeof NodeType], | ||
| newOtherType: typeof NodeType[keyof typeof NodeType], | ||
| fileSystemId: number, |
There was a problem hiding this comment.
[Functionality] High Severity - Incomplete subsystem port uniqueness validation
The checkSubsystemPortUniqueness method in create-control-link.handler.ts contains placeholder logic that doesn't properly validate the FR-CLS-04 requirement. The method checks if a subsystem port already has 2 connections but doesn't distinguish between inner and outer connections. The commented code at lines 340-349 attempts to find the other node but doesn't complete the validation. This could allow invalid subsystem port configurations.
Impact: Subsystem ports could end up with two inner or two outer connections instead of the required one-inner-one-outer constraint, violating the business rule.
Fixed Code Snippet
private async checkSubsystemPortUniqueness(
portOwnerNodeId: number,
portSystemId: number,
newOtherNodeId: number,
portOwnerType: typeof NodeType[keyof typeof NodeType],
newOtherType: typeof NodeType[keyof typeof NodeType],
fileSystemId: number,
): Promise<void> {
if (portOwnerType !== NodeType.Subsystem) return;
const existingLinks = await this.uow.getControlLinkRepository().getLinksByPortSystemIds([portSystemId], fileSystemId);
if (existingLinks.length === 0) return;
// Determine if new connection is inner or outer by checking parent-child relationship
const subsystemRepo = this.uow.getSubsystemRepository();
const isNewConnectionInner = await subsystemRepo.isChildOf(newOtherNodeId, portOwnerNodeId, fileSystemId);
// Check existing connections to see if same side is already occupied
for (const link of existingLinks) {
const otherNodeId = link.peerNodeASystemId === portOwnerNodeId
? link.peerNodeBSystemId
: link.peerNodeASystemId;
const isExistingInner = await subsystemRepo.isChildOf(otherNodeId, portOwnerNodeId, fileSystemId);
if (isNewConnectionInner === isExistingInner) {
const side = isNewConnectionInner ? 'inner' : 'outer';
throw new DomainRuleViolationException([
IssueFactory.validationError(
`Subsystem port ${portSystemId} on node ${portOwnerNodeId} already has an ${side} connection. Cannot add another ${side} connection.`,
),
]);
}
}
if (existingLinks.length >= 2) {
throw new DomainRuleViolationException([
IssueFactory.validationError(
`Subsystem port ${portSystemId} on node ${portOwnerNodeId} already has the maximum of 2 connections (inner + outer)`,
),
]);
}
}|
|
||
| portsToFill.push(...result.portsToFill); | ||
| } | ||
|
|
||
| // Write intent updates (BFS-propagated ports) | ||
| for (const {portSystemId: fillPort, intentIds: fillIntents} of portsToFill) { | ||
| const existing = await repo.getAllocatedIntentIds(fillPort, fileSystemId); | ||
| if (existing.length > 0) { | ||
| await repo.deleteIntents(existing.map(e => e.intentSystemId), fillPort); | ||
| } | ||
| const newIntents = await Promise.all( |
There was a problem hiding this comment.
[Functionality] High Severity - Potential duplicate intent writes in propagateIntents
In the propagateIntents method (lines 605-621), intents are written to the two direct ports (portASystemId and portBSystemId) after the BFS propagation has already collected ports to fill. If these direct ports are already included in the portsToFill array from the BFS traversal, their intents will be written twice, causing unnecessary database operations and potential data inconsistency.
Impact: Duplicate intent records could be created, or the second write could fail if there are unique constraints. This wastes resources and may cause transaction failures.
Fixed Code Snippet
// Write intent updates (BFS-propagated ports)
const processedPorts = new Set<number>();
for (const {portSystemId: fillPort, intentIds: fillIntents} of portsToFill) {
const existing = await repo.getAllocatedIntentIds(fillPort, fileSystemId);
if (existing.length > 0) {
await repo.deleteIntents(existing.map(e => e.intentSystemId), fillPort);
}
const newIntents = await Promise.all(
fillIntents.map(async intentId => ({
systemId: await this.idGeneration.getNextId(fileSystemId),
controlPortSystemId: fillPort,
intentId,
})),
);
if (newIntents.length > 0) {
await repo.createIntents(newIntents);
}
processedPorts.add(fillPort);
}
// Write intents for the two direct ports only if not already processed
for (const portSystemId of [portASystemId, portBSystemId]) {
if (processedPorts.has(portSystemId)) continue;
const existing = await repo.getAllocatedIntentIds(portSystemId, fileSystemId);
if (existing.length > 0) {
await repo.deleteIntents(existing.map(e => e.intentSystemId), portSystemId);
}
const newIntents = await Promise.all(
intentIds.map(async intentId => ({
systemId: await this.idGeneration.getNextId(fileSystemId),
controlPortSystemId: portSystemId,
intentId,
})),
);
if (newIntents.length > 0) {
await repo.createIntents(newIntents);
}
}
POST /control-links and /control-links/with-subsystem requirements are added