mcgilman commented on PR #11546:
URL: https://github.com/apache/nifi/pull/11546#issuecomment-5396595356
Re-reviewed the updated branch. The blocking case-sensitivity issue is
fixed, the leaf-module extraction is done correctly and completely, and the new
`value-reference.helper.spec.ts` closes the coverage gap. Gates pass locally:
905/905 tests, lint clean, build clean.
Two follow-ups on the `PropertyGroupCard` change, one of which I would like
addressed before merge.
## 1. `hasValue` duplicates `hasPropertyValue`, and its SECRET branch is
weaker
The new descriptor-default fallback re-implements logic that already exists
in this library:
```ts
// components/property-group-card/property-group-card.component.ts:75
hasValue(propertyName: string): boolean {
const valueRef = this.propertyGroup().propertyValues?.[propertyName];
if (valueRef?.valueType === 'SECRET_REFERENCE') return true;
if (valueRef?.valueType === 'ASSET_REFERENCE') {
return (valueRef.assetReferences?.length ?? 0) > 0;
}
const value = valueRef?.value ??
this.getDescriptor(propertyName)?.defaultValue;
return value !== null && value !== undefined && value !== '';
}
```
`hasPropertyValue` in `utils/connector-validation.utils.ts:31` already
handles all three branches, including the `defaultValue` fallback at lines
65-67, and handles SECRET more carefully. It requires the reference to actually
be a `SECRET_REFERENCE` **and** to produce a non-empty composite key (lines
44-57), whereas the card returns `true` on the `valueType` check alone.
That difference has a real consequence one method down.
`getDisplayValueForProperty` masks on the same weak condition:
```ts
// property-group-card.component.ts:86
getDisplayValueForProperty(propertyName: string): string {
const valueRef = this.propertyGroup().propertyValues?.[propertyName];
if (valueRef?.valueType === 'SECRET_REFERENCE') return '••••••••';
...
return valueRef?.value ?? this.getDescriptor(propertyName)?.defaultValue
?? '';
}
```
A SECRET descriptor that carries a `defaultValue` and has no saved value
fails the mask check, falls through to line 94, and returns the raw default --
which the template then renders as visible text at
`property-group-card.component.html:37-39`. Whether a SECRET descriptor ever
ships a `defaultValue` is a server-side question, but the card should not be
the component that decides it.
Suggest delegating:
```ts
hasValue(propertyName: string): boolean {
const valueRef = this.propertyGroup().propertyValues?.[propertyName];
const descriptor = this.getDescriptor(propertyName);
return hasPropertyValue(valueRef, descriptor?.type ?? 'STRING',
descriptor?.defaultValue);
}
```
and gating the default fallback in `getDisplayValueForProperty` on the same
type check, so the mask cannot be bypassed. That removes the duplication and
closes the gap in one change.
## 2. The new visibility tests could exercise the reactive path
```ts
// connector-configuration-step.component.spec.ts:454, :472
component.setPropertyValue('enableImageExtraction', true);
component['computeAllPropertyVisibility']();
```
This matches the convention already in the file (two other call sites
predate this PR), so it is a suggestion rather than a defect. But in production
the recompute is driven by the `valueChanges` subscription established at
`connector-configuration-step.component.ts:528`, inside a
`Promise.resolve().then(...)`. Calling the private method directly means these
tests would still pass if that subscription were broken or never wired up --
which for the two new BOOLEAN cases is arguably the more interesting half of
the behavior, since the whole point is that a toggle flip propagates to
dependent properties. Wrapping in `fakeAsync`, calling `flush()` after setup,
and dropping the manual invocation would cover the wiring as well as the
comparison.
Everything else I looked at held up: the coercion is applied consistently at
every point where a BOOLEAN enters the form or the save payload, the dirty
check is symmetric (matching `false` fallbacks on both sides, `??` rather than
`||` throughout), and an untouched BOOLEAN is never included in the payload, so
an unusual stored value survives a read/render/save cycle unchanged.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
