Conversation
There was a problem hiding this comment.
Files Not Fully Analyzed
- 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/modules/subgraph/dto/subgraph-request.dto.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/vcpm-definition/vcpm-definition-query-service.ts - Full file content skipped due to token limit
- packages/core/src/application/usecase-designer/subgraph/dto/subgraph-write-result-types.ts - Full file content skipped due to token limit
- packages/core/src/application/usecase-designer/subgraph/update-vcpm-cal-data/update-vcpm-cal-data.command.ts - Full file content skipped due to token limit
- packages/infrastructure/persistence/src/persistence-typeorm-sqllite/index.ts - Full file content skipped due to token limit
- packages/infrastructure/persistence/src/persistence-typeorm-sqllite/queries/typeorm-query-services.ts - Full file content skipped due to token limit
Qualcomm AI Review
| description: 'System id of a subgraph', | ||
| }) | ||
| @UseGuards(SessionGuard) | ||
| @HttpCode(HttpStatus.OK) | ||
| @ApiDocumentationWithExample({ | ||
| summary: 'Create a new VCPM CKV entry for a subgraph', | ||
| requestDto: CreateVcpmCkvRequestDto, |
There was a problem hiding this comment.
[FUNCTIONALITY - Medium] HTTP status code inconsistency for resource creation endpoint
The createVcpmCkv endpoint explicitly sets @HttpCode(HttpStatus.OK) (200) for a POST operation that creates a new resource. According to REST conventions, successful resource creation should return 201 Created, not 200 OK. While the explicit decorator ensures consistency, it violates standard HTTP semantics.
Impact: API consumers may be confused by the non-standard status code. Tools and frameworks that rely on standard HTTP semantics may not correctly interpret the response.
Fixed Code Snippet
@Post('/:subgraphSystemId/vcpm-ckv')
@ApiParam({
name: 'subgraphSystemId',
required: true,
type: String,
description: 'System id of a subgraph',
})
@UseGuards(SessionGuard)
@HttpCode(HttpStatus.CREATED)
@ApiDocumentationWithExample({
summary: 'Create a new VCPM CKV entry for a subgraph',
requestDto: CreateVcpmCkvRequestDto,
responses: [
{
status: HttpStatus.CREATED,
description: 'CKV created',
dto: CreateVcpmCkvResponseDto,
},| issue: IssueFactory.paramPayloadNotFound(parameter.systemId), | ||
| }; | ||
| } | ||
|
|
||
| const definition = definitionsBySystemId.get(payload.vcpmParameterSystemId); | ||
| if (!definition) { | ||
| throw new Error( | ||
| `ParameterDefinition missing for parameterSystemId=${payload.vcpmParameterSystemId} — DB integrity violation`, | ||
| ); | ||
| } | ||
| if (definition.isReadOnly) { |
There was a problem hiding this comment.
[ERROR HANDLING - High] Inconsistent error handling breaks partial success pattern
In UpdateVcpmCalDataHandler.buildParameterUpdate(), when a parameter definition is missing, the code throws a generic Error instead of returning a structured issue. This breaks the partial success pattern used throughout the handler, where individual parameter failures should be collected as issues rather than causing the entire operation to fail.
Impact: If a parameter definition is missing due to database inconsistency, the entire update operation fails instead of gracefully handling the error and continuing with other parameters. This violates the established pattern where the handler returns Result.partial() with issues.
Fixed Code Snippet
private buildParameterUpdate(
parameter: UpdateVcpmCalDataCommand['parameters'][number],
payloadBySystemId: Map<
number,
{systemId: number; vcpmParameterSystemId: number}
>,
definitionsBySystemId: Map<number, ParameterDefinitionBase>,
): ParameterUpdateResult {
const payload = payloadBySystemId.get(parameter.systemId);
if (!payload) {
return {
kind: 'issue',
issue: IssueFactory.paramPayloadNotFound(parameter.systemId),
};
}
const definition = definitionsBySystemId.get(payload.vcpmParameterSystemId);
if (!definition) {
return {
kind: 'issue',
issue: IssueFactory.parseError(
'PARAM_DEFINITION_NOT_FOUND',
`Parameter definition ${payload.vcpmParameterSystemId} not found for payload ${parameter.systemId}`,
),
};
}
if (definition.isReadOnly) {
return {
kind: 'issue',
issue: IssueFactory.paramReadOnly(parameter.systemId),
};
}
// ... rest of the method
}| instanceSystemId: number, | ||
| valueSystemIds: number[], | ||
| params: ParameterDefinitionBase[], | ||
| ): Promise<number> { | ||
| const {session, groupId} = this.uow.getWriteContext(); | ||
| const ckvSystemId = await this.idGeneration.getNextId(session.fileSystemId); | ||
|
|
||
| await this.writer.writeCreate( | ||
| { | ||
| targetTable: ENTITY_NAMES.VcpmCkv, | ||
| targetSystemId: ckvSystemId, |
There was a problem hiding this comment.
[FUNCTIONALITY - High] Missing database integrity validation in createVcpmCkv
The TypeOrmSubgraphRepository.createVcpmCkv() method iterates over parameter definitions to create payload rows, but it doesn't verify that these parameter definitions actually exist in the database before attempting to create foreign key references. While the handler validates that definitions exist, there's a race condition window where definitions could be deleted between the handler's check and the repository's execution.
Impact: If parameter definitions are deleted concurrently or if the handler's validation is bypassed, the repository will create VcpmParameterPayload rows with invalid foreign keys, leading to database integrity violations or orphaned records.
Fixed Code Snippet
async createVcpmCkv(
subgraphSystemId: number,
instanceSystemId: number,
valueSystemIds: number[],
params: ParameterDefinitionBase[],
): Promise<number> {
const {session, groupId} = this.uow.getWriteContext();
const ckvSystemId = await this.idGeneration.getNextId(session.fileSystemId);
// Verify all parameter definitions exist before proceeding
const paramSystemIds = params.map(p => p.systemId);
const existingParams = await this.manager
.getRepository(ENTITY_NAMES.VcpmModuleParameterDefinition)
.createQueryBuilder('param')
.where('param.systemId IN (:...ids)', {ids: paramSystemIds})
.getCount();
if (existingParams !== params.length) {
throw new Error(
`One or more parameter definitions do not exist. Expected ${params.length}, found ${existingParams}`,
);
}
await this.writer.writeCreate(
{
targetTable: ENTITY_NAMES.VcpmCkv,
targetSystemId: ckvSystemId,
aggregateId: subgraphSystemId,
payload: {vcpmInstanceSystemId: instanceSystemId},
},
session.sessionId,
groupId,
this.manager,
);
// ... rest of the method
}4d09663 to
c417aad
Compare
add llds for set vcpm data Signed-off-by: Robin Gupta <robigupt@qti.qualcomm.com>
add end-to-end support for set vcpm data Signed-off-by: Robin Gupta <robigupt@qti.qualcomm.com>
c417aad to
1b0d0a6
Compare
add llds for set vcpm data