kevdoran commented on code in PR #11789:
URL: https://github.com/apache/nifi/pull/11789#discussion_r4224584759
##########
nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/components/connector/StandardConnectorRepository.java:
##########
@@ -1085,18 +1096,12 @@ private void syncFromProvider(final ConnectorNode
connector) {
// converted into the in-memory ConnectorValueReference graph.
resolveSecretReferencesFromProvider(workingFlowConfiguration);
- // Replace each step's working configuration on the connector. Routing
through the connector
- // (rather than touching the configuration context directly) ensures
it is notified via
- // onConfigurationStepConfigured when raw or resolved property values
changed, so the embedded
- // flow's Parameter Context picks up new values (e.g., rotated
secrets) without an explicit save.
- for (final VersionedConfigurationStep step : workingFlowConfiguration)
{
- final StepConfiguration stepConfiguration =
toStepConfiguration(step);
- try {
- connector.replaceWorkingConfiguration(step.getName(),
stepConfiguration);
- } catch (final Exception e) {
- logger.warn("Failed to replace working configuration for step
[{}] on {} during sync from provider; continuing with remaining steps",
- step.getName(), connector, e);
- }
+ // Replace the complete working configuration on the connector.
Routing through the connector
+ // applies property migration and required defaults in memory without
writing to the provider.
+ try {
+ connector.replaceWorkingConfiguration(workingFlowConfiguration);
+ } catch (final Exception e) {
Review Comment:
[openai-gpt-6-astra] High: this catch also applies to the Apply path, not
only GET/list. `applyUpdate()` calls `syncAssetsFromProvider()` and then
unconditionally transitions/submits the update. Complete replacement now calls
`migrateProperties()` before installing the new configuration; if migration
throws (for example, a step-rename collision), the old working configuration
remains, this catch logs and returns, and Apply can proceed using that old
configuration instead of failing the requested update. The per-step catch
predates this PR, but it now also swallows the new whole-document migration
failure.
Please keep read tolerance separate from write behavior: GET/list should
remain readable on normalization failure, while add/apply must propagate the
failure rather than proceed. Add a repository-level regression where
provider-loaded migration throws during `applyUpdate()` and assert that
transition/prepare/apply and `saveFlow()` are not reached; retain a separate
GET/list readability test. This finding is based on the call chain, not a
locally executed reproduction.
##########
nifi-system-tests/nifi-system-test-suite/src/test/java/org/apache/nifi/tests/system/connectors/ConnectorUndeclaredExternalPropertyIT.java:
##########
@@ -78,37 +82,54 @@ protected boolean isAllowFactoryReuse() {
}
@Test
- public void testSaveDropsUndeclaredExternalProperty() throws
NiFiClientException, IOException {
+ public void
testReadMigratesInMemoryAndExplicitSavePersistsMigratedConfiguration() throws
NiFiClientException, IOException {
final JsonNode initialConfiguration =
readConfiguration("initial-config.json");
- assertEquals(DECLARED_PROPERTY_VALUE,
propertyValue(initialConfiguration, DECLARED_PROPERTY_NAME));
+ assertEquals(DECLARED_PROPERTY_VALUE,
propertyValue(initialConfiguration, LEGACY_PROPERTY_NAME));
+ assertFalse(containsProperty(initialConfiguration,
DECLARED_PROPERTY_NAME));
assertEquals("should-be-dropped", propertyValue(initialConfiguration,
UNDECLARED_PROPERTY_NAME));
final ConnectorEntity connector =
getClientUtil().createConnector("NopConnector");
assertNotNull(connector);
final ConnectorEntity beforeSave =
getNifiClient().getConnectorClient().getConnector(connector.getId());
- assertEquals(DECLARED_PROPERTY_VALUE,
ignoredPropertyValue(beforeSave));
+ assertEquals(DECLARED_PROPERTY_VALUE, workingPropertyValue(beforeSave,
DECLARED_PROPERTY_NAME));
+ assertEquals(REQUIRED_DEFAULT_PROPERTY_VALUE,
workingPropertyValue(beforeSave, REQUIRED_DEFAULT_PROPERTY_NAME));
+ final ConnectorEntity repeatedRead =
getNifiClient().getConnectorClient().getConnector(connector.getId());
+ assertEquals(DECLARED_PROPERTY_VALUE,
workingPropertyValue(repeatedRead, DECLARED_PROPERTY_NAME));
+ assertEquals(REQUIRED_DEFAULT_PROPERTY_VALUE,
workingPropertyValue(repeatedRead, REQUIRED_DEFAULT_PROPERTY_NAME));
+ assertFalse(new File(STATE_DIRECTORY, "saved-config.json").exists());
Review Comment:
[openai-gpt-6-astra] This is useful provider-backed read/save coverage,
including the assertion that reads do not persist. The remaining integration
gap is the operation that exposed the upgrade defect: this test does not start
a working connector, upgrade/reload it, or invoke apply-update.
Please add provider-backed coverage that keeps the older document unchanged,
performs Apply after reload, waits for the update to complete, and asserts the
effective active values. Repeat the Apply and include a benign edit so success
cannot be satisfied by retaining the old active configuration. The full
old-runtime -> upgrade -> no-change Apply/COMMIT -> continued ingestion
sequence also remains a downstream QA release gate; the new direct-node
immutable tests and this read/save test do not replace it. If the
runtime-upgrade portion must live outside this upstream suite, please make that
validation dependency explicit.
--
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]
