mcgilman commented on code in PR #11210:
URL: https://github.com/apache/nifi/pull/11210#discussion_r3196117600
##########
nifi-framework-bundle/nifi-framework/nifi-framework-core/src/test/java/org/apache/nifi/components/connector/TestStandardConnectorNode.java:
##########
@@ -527,6 +531,83 @@ public void
testVerifyConfigurationStepSurfacesInvalidValidationResults() throws
assertEquals("The property value is invalid",
failedResult.getExplanation());
}
+ @Test
+ public void
testVerifyConfigurationStepSkipsSecretReferenceWhenPropertyDependenciesNotMet()
throws FlowUpdateException {
+ final DependentSecretVerifyConnector connector = new
DependentSecretVerifyConnector();
+ final StandardConnectorNode connectorNode =
createConnectorNode(connector);
+
+ connectorNode.transitionStateForUpdating();
+ connectorNode.prepareForUpdate();
+
+ final Map<String, ConnectorValueReference> propertyValues = new
HashMap<>();
+ propertyValues.put("Mode", new StringLiteralValue("OFF"));
+ propertyValues.put("SecretKey", new SecretReference("pid", "My
Provider", null, null));
Review Comment:
This test doesn't actually isolate the dependency-skip path it advertises.
`SecretReference` here is constructed with `secretName == null &&
fullyQualifiedName == null`, so it's also structurally empty — meaning the new
`isEmptySecretReference` filter alone is enough to make this test pass, even if
the dependency-aware filter regressed or was deleted.
Could you tighten this so the dependency filter is the only thing carrying
the test? Something like:
```java
propertyValues.put("SecretKey", new SecretReference("pid", "My Provider",
"secretA", "fqn-A"));
when(secretsManager.getSecrets(anySet()))
.thenThrow(new AssertionError("Should not look up secrets when property
dependency is unsatisfied"));
```
That makes the test fail loudly if the dependency-skip path ever stops
short-circuiting the `SecretsManager` lookup.
##########
nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/components/connector/StandardConnectorNode.java:
##########
@@ -1033,6 +1057,92 @@ private Map<String, String>
resolvePropertyReferences(final StepConfiguration co
return resolvedProperties;
}
+ private static Map<String, ConnectorPropertyDescriptor>
buildPropertyDescriptorLookup(final ConfigurationStep configurationStep) {
+ final Map<String, ConnectorPropertyDescriptor> lookup = new
HashMap<>();
+ for (final ConnectorPropertyGroup propertyGroup :
configurationStep.getPropertyGroups()) {
+ for (final ConnectorPropertyDescriptor descriptor :
propertyGroup.getProperties()) {
+ lookup.put(descriptor.getName(), descriptor);
+ }
+ }
+ return lookup;
+ }
+
+ /**
+ * String value used to evaluate {@link ConnectorPropertyDependency},
aligned with
+ * {@code
AbstractConnector.isDependencySatisfied(ConnectorPropertyDescriptor, Function,
Function, Set)} for {@link StepConfiguration} payloads.
+ */
+ private static String getConfiguredValueForDependency(final
StepConfiguration stepConfig, final ConnectorPropertyDescriptor descriptor) {
+ final ConnectorValueReference ref =
stepConfig.getPropertyValue(descriptor.getName());
+ if (ref == null) {
+ return descriptor.getDefaultValue();
+ }
+ if (ref instanceof StringLiteralValue stringLiteralValue) {
+ final String value = stringLiteralValue.getValue();
+ return value != null ? value : descriptor.getDefaultValue();
+ }
+ return null;
+ }
Review Comment:
The Javadoc says this is "aligned with
`AbstractConnector.isDependencySatisfied(...)`", but the algorithms diverge in
two cases:
1. A typed `StringLiteralValue` whose `getValue()` is `null` — this helper
falls back to `descriptor.getDefaultValue()`, while `AbstractConnector` returns
`null` (treating the dependency as unsatisfied).
2. An `AssetReference` / `SecretReference` controlling property — this
helper returns `null` (unsatisfied), while `AbstractConnector` returns the
*resolved* value via `propertyValue.getValue()`.
In practice this is unlikely to surface because dependency-controlling
properties are almost always literal allowable-values, but the doc as written
is misleading. Could you tighten it to describe what this method actually does
— e.g. note that it's an approximation for `StepConfiguration` payloads scoped
to `STRING_LITERAL` controlling properties, that `null`-valued literals fall
back to the descriptor default rather than failing the dependency, and that
non-literal reference types are treated as unsatisfied? That way the alignment
claim is accurate and the divergence from `AbstractConnector` is documented
rather than implied-away.
--
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]
