mcgilman commented on PR #11546:
URL: https://github.com/apache/nifi/pull/11546#issuecomment-5371938231
The mechanism is correctly diagnosed and the fix lands in the right places.
All gates pass locally (897 shared tests, lint, build). One blocking item,
three should-fixes, and some nits.
## Blocking: `toBooleanValue` is case-sensitive, but the backend is not
```ts
//
nifi-frontend/src/main/frontend/libs/shared/src/services/value-reference.helper.ts:48
export function toBooleanValue(value: unknown): boolean {
return value === true || value === 'true';
}
```
`StandardConnectorPropertyValue.asBoolean()` reads the same wire value with
`Boolean.parseBoolean`, which accepts `"True"` and `"TRUE"`:
```java
//
nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/components/connector/StandardConnectorPropertyValue.java:53
public Boolean asBoolean() {
return rawValue == null ? null : Boolean.parseBoolean(rawValue);
}
```
So a connector declaring a mixed-case BOOLEAN `defaultValue` renders the
toggle **off** while the connector itself reads the property as **on**. Because
`buildChangedConfiguration` normalizes the original value through this same
helper (`connector-configuration-step.component.ts:909`), toggling once would
not register as dirty either, so the user has no way to correct it from the UI.
Suggest:
```ts
return value === true || (typeof value === 'string' && value.toLowerCase()
=== 'true');
```
The JSDoc above asserts the narrower contract ("Any value other than `true`
/ `\"true\"` is therefore treated as false") and should move with it.
## Should fix: extract the dependency predicate to a leaf module
`isDependencyValueSatisfied` is a pure predicate but lives at
`components/connector-wizard/step-dependency.utils.ts:75`, so
`utils/connector-validation.utils.ts:26` now imports upward into `components/`.
There is no cycle today, but `connector-configuration-summary-step` already
imports `connector-validation.utils`, so we are one import away from one.
Suggest a leaf `utils/dependency-value.utils.ts` with
`step-dependency.utils.ts` as a consumer alongside the other two call sites,
and moving the new `isDependencyValueSatisfied` block out of
`step-dependency.utils.spec.ts` into a matching
`dependency-value.utils.spec.ts` so the tests sit with the unit they cover. No
barrel change needed, since this directory is imported by direct path.
## Should fix: the read-only summary path does not honor descriptor defaults
`PropertyGroupCard`
(`components/property-group-card/property-group-card.component.ts`) renders the
summary step and was not touched:
```ts
// property-group-card.component.ts:75
hasValue(propertyName: string): boolean {
const valueRef = this.propertyGroup().propertyValues?.[propertyName];
if (!valueRef) return false;
...
}
```
Neither `hasValue` nor `getDisplayValueForProperty` consults
`getDescriptor(propertyName)`, so a property whose effective value comes from
`defaultValue` renders as "No value set" in the summary while the configuration
step -- which now correctly applies the descriptor default -- shows it
populated. A user who never opens the configuration step sees a summary that
disagrees with what will actually be applied. That is the same inconsistency
this PR is fixing, one component over.
Suggest falling back to `getDescriptor(propertyName)?.defaultValue` in both
methods and routing BOOLEAN through `toBooleanValue`. Rendering BOOLEAN as a
typed `Yes`/`No` rather than the raw wire string is a separate gap in this
component and I would leave it out of scope here.
## Should fix: no direct test for `toBooleanValue`
`toBooleanValue` and the new BOOLEAN branch of `fromValueReference` are
covered only transitively through the component spec -- there is no
`value-reference.helper.spec.ts` at all. Suggest adding one covering
`toBooleanValue` (including `'TRUE'` and `'True'`, which is what surfaces the
blocking issue above) and `fromValueReference(ref, 'BOOLEAN')` returning `null`
for a null value so callers can still fall back to the descriptor default.
## Nits
- `connector-property-input.component.spec.ts` covers both BOOLEAN
directions through `writeValue` (lines 465, 479) but has no non-BOOLEAN
passthrough case. Since this PR rewrites `writeValue` into a ternary
(`connector-property-input.component.ts:195`), a "should not coerce values for
non-BOOLEAN properties" test is the one guarding that refactor.
- `step-dependency.utils.ts:68` says the helper is shared by two sites;
there are three (`step-dependency.utils.ts:117`,
`connector-configuration-step.component.ts:661`,
`connector-validation.utils.ts:143`). The comment exists to deter drift, so it
should name all three.
- Worth adding a line to that same doc noting that dependency matching stays
case-sensitive to mirror the backend evaluators. The contrast with the now
case-insensitive `toBooleanValue` next door is otherwise easy to misread as an
oversight.
- The `@param propertyType` doc on `fromValueReference` still only mentions
`STRING_LIST` splitting and no longer describes what the parameter does.
- The PR title and commit subject describe only the coercion half and omit
the `dependsOn` matching half. Something like "Fix BOOLEAN toggle coercion and
dependsOn matching in connector wizard" covers both.
The added tests are otherwise well targeted -- the `getConfigurationForSave`
case covering a `"false"` default not being falsely reported as changed is the
subtle one and I am glad it is there.
--
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]
