Skip to content

feat(data): add endpoints for set property data - #123

Open
robigupt wants to merge 1 commit into
AudioReach:feature/use-case-designerfrom
robigupt:feature/set-property-data
Open

robigupt wants to merge 1 commit into
AudioReach:feature/use-case-designerfrom
robigupt:feature/set-property-data

Conversation

@robigupt

Copy link
Copy Markdown
Contributor

add lld for set container property data

@robigupt
robigupt requested review from a team August 21, 2026 05:04
@robigupt
robigupt force-pushed the feature/set-property-data branch 2 times, most recently from 70a22f4 to 27092dd Compare August 22, 2026 05:06
Comment thread docs/property-data/design/set-container-property-design.md Outdated
Comment thread docs/property-data/design/set-container-property-design.md Outdated
Comment thread docs/property-data/design/set-container-property-design.md Outdated
Comment thread docs/property-data/design/set-container-property-design.md Outdated
Comment thread docs/property-data/design/set-container-property-design.md Outdated
Comment thread docs/property-data/design/set-container-property-design.md Outdated
Comment thread docs/property-data/design/set-container-property-design.md Outdated
Comment thread docs/property-data/design/set-container-property-design.md Outdated
Comment thread docs/property-data/design/set-container-property-design.md Outdated
Comment thread docs/property-data/design/set-container-property-design.md Outdated
@NithinSimon

Copy link
Copy Markdown
Contributor

Issues

Major

  1. GetContainerPropertyHandler spec is entirely missing.
    Section 1.2 lists get-container-property.query.ts and get-container-property.handler.ts as new files. Section 1.3
    shows the controller calling GetContainerPropertyQuery, but no section in the document specifies the handler's
    logic, ports, or error contract. Section 3 only covers the update handler. The implementer has no spec for the
    read path that the write path depends on for its response. A Section 3.5 (or equivalent) needs to be added.

  2. Sequential cascade loop — design choice needs to be explicit.
    Section 3.2 Step 6 spec:
    for (const mod of modules) {
    await this.uow.getModuleRepository().setModuleProperty(...);
    }
    This serialises all module heap writes. For containers with many modules this is a latency multiplier. If this is
    intentional , that reasoning must be
    stated in the LLD. If not intentional, the spec should use Promise.all.

  3. Route param naming inconsistency.
    The PATCH route uses :propSystemId; the GET route uses :propertySystemId. Both are @PARAM names in the TypeScript
    but they match the same URL slot. This creates confusion: the URL templates should be identical since they
    resolve the same segment. The LLD should normalise to one name (:propertySystemId is the clearer choice).


Minor

  1. ParseIntPipe inconsistency in controller.
    updateContainerProperty uses @PARAM('containerSystemId', ParseIntPipe) and @PARAM('propSystemId', ParseIntPipe),
    but getContainerProperty omits ParseIntPipe and manually calls Number.parseInt() on every param. The LLD should
    adopt one pattern; ParseIntPipe is idiomatic NestJS and should be used consistently.

  2. Result vs PropertyResponseDto type mismatch.
    The queryBus execute call is typed execute<Result> but the method return type is
    The queryBus execute call is typed execute<Result> but the method return type is
    Promise<ApiResult>. Whether PropertyDto and PropertyResponseDto are the same type (or one wraps the other) is
    not explained. The LLD should clarify the DTO mapping.

  3. Unexplained 'api-client' literal in GetContainerPropertyQuery constructor.
    Both controller methods pass 'api-client' as the 4th argument. What this parameter represents is never defined in the LLD. If it
    is a caller/source tag, name it explicitly (QUERY_SOURCE.ApiClient) or explain its purpose.

  4. Missing E2E scenario: capability write on an empty container.
    Section 5 has 0x08001011 — capability list valid but not 0x08001011 — container has no modules. With zero modules,
    validateModuleCapabilityIntersection should vacuously pass and the write should return 200. This is a meaningful edge case for the
    E2E table.

  5. Style: redundant .set() in containerTypeIdsMap loop.
    const existing = containerTypeIdsMap.get(link.moduleDefinitionSystemId) ?? [];
    existing.push(link.containerType.value);
    containerTypeIdsMap.set(link.moduleDefinitionSystemId, existing); // no-op when key exists
    When the key already exists existing is the in-map array — the .set() is a no-op. This is correct but misleading. Either use
    containerTypeIdsMap.set(id, [...(existing), value]) (immutable) or guard the set with if (!containerTypeIdsMap.has(...)) after the
    initial set.

@robigupt
robigupt force-pushed the feature/set-property-data branch from 7bd9654 to 3c6d957 Compare August 25, 2026 08:23

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Qualcomm AI Review

@robigupt
robigupt force-pushed the feature/set-property-data branch from 925b3c4 to 8c9581d Compare August 26, 2026 17:45

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Files Not Fully Analyzed

  • packages/core/src/application/orchestration/cqrs/registries/command-handler-registry.ts - Full file content skipped due to token limit

Qualcomm AI Review

@robigupt
robigupt force-pushed the feature/set-property-data branch from 8c9581d to 4699e75 Compare August 26, 2026 17:53

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Files Not Fully Analyzed

  • packages/core/src/application/orchestration/cqrs/registries/command-handler-registry.ts - Full file content skipped due to token limit

Qualcomm AI Review

@robigupt
robigupt force-pushed the feature/set-property-data branch from 4699e75 to 11c6a8a Compare September 3, 2026 11:36

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Files Not Fully Analyzed

  • packages/core/src/application/orchestration/cqrs/registries/command-handler-registry.ts - Full file content skipped due to token limit

Qualcomm AI Review

@robigupt
robigupt force-pushed the feature/set-property-data branch from 11c6a8a to 2585b50 Compare September 5, 2026 16:35

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/core/src/shared/issues/factories.ts - Full file content skipped due to token limit
  • packages/infrastructure/persistence/src/persistence-typeorm-sqllite/fetchers/definitions/spf-module-definitions/spf-module-definition-fetcher.ts - Full file content skipped due to token limit

Qualcomm AI Review

@robigupt
robigupt force-pushed the feature/set-property-data branch 5 times, most recently from 3b5c29b to 6813919 Compare September 12, 2026 02:54
@robigupt
robigupt force-pushed the feature/set-property-data branch 4 times, most recently from a8251cc to 2a47228 Compare September 15, 2026 05:04
add end-to-end support for set container property data.

Signed-off-by: Robin Gupta <robigupt@qti.qualcomm.com>
@robigupt
robigupt force-pushed the feature/set-property-data branch from 2a47228 to d391e2d Compare September 15, 2026 06:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants