Conversation
robigupt
force-pushed
the
feature/get-vcpm-data
branch
from
August 28, 2026 16:19
e2a5527 to
5aaf473
Compare
![qualcomm-ai-code-review-assistant[bot]](https://avatars.githubusercontent.com/in/4128059?s=60&v=4){kind=small}
robigupt
force-pushed
the
feature/get-vcpm-data
branch
from
August 29, 2026 15:53
873baae to
c18a065
Compare
robigupt
force-pushed
the
feature/get-vcpm-data
branch
from
August 29, 2026 16:07
c18a065 to
d8286e4
Compare
robigupt
force-pushed
the
feature/get-vcpm-data
branch
from
August 29, 2026 16:20
d8286e4 to
d9e2ca0
Compare
robigupt
force-pushed
the
feature/get-vcpm-data
branch
from
August 30, 2026 02:11
d9e2ca0 to
7fec22c
Compare
robigupt
force-pushed
the
feature/get-vcpm-data
branch
from
August 30, 2026 02:22
7fec22c to
75f1b9c
Compare
robigupt
force-pushed
the
feature/get-vcpm-data
branch
from
August 30, 2026 02:46
75f1b9c to
a93225e
Compare
robigupt
force-pushed
the
feature/get-vcpm-data
branch
from
August 30, 2026 03:17
a93225e to
f9f09f4
Compare
robigupt
force-pushed
the
feature/get-vcpm-data
branch
from
August 30, 2026 03:37
f9f09f4 to
0dce511
Compare
robigupt
force-pushed
the
feature/get-vcpm-data
branch
from
August 30, 2026 04:12
0dce511 to
b740693
Compare
robigupt
force-pushed
the
feature/get-vcpm-data
branch
from
August 30, 2026 05:46
b740693 to
54cb231
Compare
robigupt
force-pushed
the
feature/get-vcpm-data
branch
from
August 30, 2026 05:56
54cb231 to
7592bac
Compare
NithinSimon
reviewed
Sep 1, 2026
| ); | ||
| } | ||
|
|
||
| const instance = |
Contributor
There was a problem hiding this comment.
isn't one service enough
getVcpmAggregateBySubgraph -> it should return all VCPM related information. Why are defining lot of methods when it is not used anywhere
NithinSimon
reviewed
Sep 1, 2026
Contributor
There was a problem hiding this comment.
VCPM is a subgraph proeprty. So subgraphQueryService should be used for adding get APIs
NithinSimon
reviewed
Sep 1, 2026
| fileSystemId: number, | ||
| ): Promise<VcpmParameterPayloadReadModel[]>; | ||
|
|
||
| getVcpmParameterDefinitions( |
Contributor
There was a problem hiding this comment.
This should go in subgraph properties definition queryService
add lld for vcpm data Signed-off-by: Robin Gupta <robigupt@qti.qualcomm.com>
add end-to-end support for retrieving for vcpm data Signed-off-by: Robin Gupta <robigupt@qti.qualcomm.com>
robigupt
force-pushed
the
feature/get-vcpm-data
branch
from
September 3, 2026 08:57
7592bac to
aab53ee
Compare
Comment on lines
+59
to
+69
| fileSystemId, | ||
| query.paramSystemIds.length > 0 ? query.paramSystemIds : undefined, | ||
| ); | ||
|
|
||
| const paramSystemIds = payloads.map(p => p.vcpmParameterSystemId); | ||
| const paramDefs = | ||
| await this.queryServices.vcpmQueryService.getVcpmParameterDefinitions( | ||
| paramSystemIds, | ||
| ); | ||
| const defMap = new Map(paramDefs.map(d => [d.systemId, d])); | ||
|
|
There was a problem hiding this comment.
[FUNCTIONALITY][MEDIUM] Missing null-safety validation in parameter definition lookup
In GetVcpmCalDataHandler.handle(), the code retrieves parameter definitions and creates a map, but when looking up definitions for each payload, it throws an error if a definition is missing. However, there's a potential race condition or data integrity issue: if paramDefs returns fewer definitions than requested (e.g., due to concurrent deletion or database inconsistency), the error thrown at line 73 might not provide sufficient context about which specific parameter IDs are missing.
The current implementation assumes all parameter system IDs in payloads will have corresponding definitions returned, but this isn't validated before the map lookup loop.
Impact: Could result in unclear error messages during debugging, making it harder to diagnose data integrity issues.
Fixed Code Snippet
const paramSystemIds = payloads.map(p => p.vcpmParameterSystemId);
const paramDefs =
await this.queryServices.vcpmQueryService.getVcpmParameterDefinitions(
paramSystemIds,
);
const defMap = new Map(paramDefs.map(d => [d.systemId, d]));
// Validate all required definitions were retrieved
const missingDefIds = paramSystemIds.filter(id => !defMap.has(id));
if (missingDefIds.length > 0) {
throw new ParameterDefinitionMissingError(
`Missing parameter definitions for IDs: ${missingDefIds.join(', ')}`
);
}
const parameters = payloads.map(p => {
const def = defMap.get(p.vcpmParameterSystemId)!; // Now safe to use non-null assertion
Comment on lines
+282
to
+292
| payloadActions.filter(a => a.operation !== CHANGE_OPERATION.Create), | ||
| ) | ||
| .map(r => r.effective as unknown as VcpmParameterPayloadBase); | ||
|
|
||
| const baseIds = new Set(baseRows.map(r => r.systemId)); | ||
| const deletedIds = new Set( | ||
| payloadActions | ||
| .filter(a => a.operation === CHANGE_OPERATION.Delete) | ||
| .map(a => a.targetSystemId), | ||
| ); | ||
|
|
There was a problem hiding this comment.
[PERFORMANCE][HIGH] Inefficient filtering in fetchParameterPayloadsByInstance creates unnecessary overhead
In VcpmOverlayFetcher.fetchParameterPayloadsByInstance(), the code builds a validCkvIds set by iterating through ALL edit actions to find CKV creates (lines 297-307). This is inefficient because:
- It iterates through the entire
actionsarray (which includes actions for all entity types) when we only needVcpmCkvcreates - The
actionsarray was already filtered once forpayloadActionsat line 274, but then we iterate through the unfilteredactionsagain - For large sessions with many edit actions, this creates O(n) overhead where n is the total number of actions
Impact: Performance degradation with large edit sessions, especially when there are many non-VCPM-related actions.
Fixed Code Snippet
const baseIds = new Set(baseRows.map(r => r.systemId));
const deletedIds = new Set(
payloadActions
.filter(a => a.operation === CHANGE_OPERATION.Delete)
.map(a => a.targetSystemId),
);
// Build validCkvIds efficiently by filtering CKV actions once
const validCkvIds = new Set(baseRows.map(r => r.vcpmCkvSystemId));
const ckvActions = actions.filter(
a => a.targetTable === ENTITY_NAMES.VcpmCkv &&
a.operation === CHANGE_OPERATION.Create
);
for (const a of ckvActions) {
const p = a.newValue as {vcpmInstanceSystemId?: number};
if (p.vcpmInstanceSystemId === vcpmInstanceSystemId) {
validCkvIds.add(a.targetSystemId);
}
}
Comment on lines
+137
to
+147
| await this.keyValueDefQueryService.getKeyValueSummaryForGivenValues( | ||
| allValueDefIds, | ||
| fileSystemId, | ||
| ); | ||
| if (pairsResult.kind === RESULT_KIND.Fail) { | ||
| const ckvIds = rows.map(r => r.systemId).join(', '); | ||
| throw new Error( | ||
| `Failed to resolve key-value pairs for CKVs [${ckvIds}] (fileSystemId: ${fileSystemId}, valueDefIds: [${allValueDefIds.join(', ')}]): ${pairsResult.issues.map((e: Issue) => e.message).join(', ')}`, | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
[SECURITY][MEDIUM] Error message exposes internal system details
In DbVcpmQueryService.toCkvReadModels(), the error message at line 143-145 exposes detailed internal system information including CKV IDs, file system IDs, and value definition IDs. This could potentially aid attackers in understanding the system's internal structure.
While this is in a private method and the error is thrown (not logged to external systems), the error message could still be exposed through API responses or logs that might be accessible to unauthorized parties.
Impact: Information disclosure that could aid in reconnaissance for potential attacks.
Fixed Code Snippet
if (pairsResult.kind === RESULT_KIND.Fail) {
throw new Error(
`Failed to resolve key-value pairs for CKVs. Please contact system administrator.`,
);
}Alternatively, if detailed logging is needed for debugging, log the details separately at a debug level while throwing a generic error:
if (pairsResult.kind === RESULT_KIND.Fail) {
const ckvIds = rows.map(r => r.systemId).join(', ');
// Log detailed info for debugging (ensure this goes to secure logs only)
console.debug(
`CKV resolution failed: CKVs=[${ckvIds}], fileSystemId=${fileSystemId}, valueDefIds=[${allValueDefIds.join(', ')}]`
);
throw new Error(
`Failed to resolve calibration key-value pairs. Reference ID: ${fileSystemId}`,
);
}added document for updated query service and fetcher design Signed-off-by: Robin Gupta <robigupt@qti.qualcomm.com>
added updated lld for vcpm data Signed-off-by: Robin Gupta <robigupt@qti.qualcomm.com>
Contributor
|
feat(data) doesnt make sense. Use feat or feat(api), feat(core) etc. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
add lld for vcpm data