NIFI-15891: Connector Asset Sync on Create and Restore

[bobpaulin]

Summary

NIFI-15891

addConnector calls syncFromProvider, which only invokes configurationProvider.load(connectorId)
The only paths that actually call configurationProvider.syncAssets(connectorId) (which is the method that downloads binaries) are:

StandardConnectorRepository.syncAssetsFromProvider (line 771–784), which is invoked from
applyUpdate (line 523), and
StandardConnectorDAO.syncAssetsFromProvider (web layer) — i.e., an explicit REST call.
So on a NiFi restart, when the connector node is restored from flow.json.gz, syncAssets is never called and no binary will be (re-)downloaded just by virtue of NiFi starting.

This change allows asset sync to happen at those points.

Tracking

Please complete the following tracking steps prior to pull request creation.

Issue Tracking

• Apache NiFi Jira issue created

Pull Request Tracking

• Pull Request title starts with Apache NiFi Jira issue number, such as NIFI-00000
• Pull Request commit message starts with Apache NiFi Jira issue number, as such NIFI-00000
• Pull request contains commits signed with a registered key indicating Verified status

Pull Request Formatting

• Pull Request based on current revision of the main branch
• Pull Request refers to a feature branch with one commit containing changes

Verification

Please indicate the verification steps performed prior to pull request creation.

Build

• Build completed using ./mvnw clean install -P contrib-check
  • JDK 21
  • JDK 25

Licensing

• New dependencies are compatible with the Apache License 2.0 according to the License Policy
• New dependencies are documented in applicable LICENSE and NOTICE files

Documentation

• Documentation formatting appears as expected in rendered files

[kevdoran]

Reviewing

[kevdoran]

Thanks for this @bobpaulin.

We should add syncing assets to getConnector() in case assets have been added in the external provider since it was last loaded in NiFi.

We also need to call connector.syncAssets(id) from inside of buildMergedWorkingConfiguration() before calling configurationProvider.load(connectorId) for the same reason.

While not entirely necessary, I would be ok with adding asset syncing to syncFromProvider(), as I no longer think we should have different behavior for syncFromProvider() and syncAssetsFromProvider(). syncAssetsFromProvider() could just call syncFromProvider() backwards compatibility. The only reason I hesitate to do this is that would sync assets for calls to getConnectors() as well, which is probably not entirely necessary.

[bobpaulin]

Thank you for the quick review @kevdoran! Comments inline

Thanks for this @bobpaulin.

We should add syncing assets to getConnector() in case assets have been added in the external provider since it was last loaded in NiFi.

I understand the intent here but the frequency of calling getConnector throughout the framework gives me pause.

We also need to call connector.syncAssets(id) from inside of buildMergedWorkingConfiguration() before calling configurationProvider.load(connectorId) for the same reason.

This is also called frequently since it's tied into configureConnector(), but less so than getConnector(). I think this could be done. But this depends on how the ConnectorConfigurationProvider handles the save event. If assets may be synced on save I think it's possible this could be avoided. It does not look like this code validates anything prior to the save so as long as it was handled before the method exits I'd expect someone working in the Connector UI would not notice any issue. However I do think this ties in a bit to where we believe the responsibility of the ConnectorRepository vs the ConnectorConfigurationProvider stand. If we do it in the repository all providers MUST sync the assets when saving each step. If we allow the ConnectorConfigurationProvider it's up to the provider implementor.

While not entirely necessary, I would be ok with adding asset syncing to syncFromProvider(), as I no longer think we should have different behavior for syncFromProvider() and syncAssetsFromProvider(). syncAssetsFromProvider() could just call syncFromProvider() backwards compatibility. The only reason I hesitate to do this is that would sync assets for calls to getConnectors() as well, which is probably not entirely necessary.

Making the asset sync always up-to-date might fix a number of ailments but may kill the patient.

[bobpaulin]

Converting to draft to consider this feedback.