NIFI-14478 Changing version controlled Process Group sync

[mosermw]

Summary

NIFI-14478

This PR removes a capability where a version controlled Process Groups could replace, in memory, what it thought was in the FlowRegistry. The capability causes some non-intuitive effects on Versioned Process Group, by appearing to be "In Sync" with the FlowRegistry when in reality they are not.

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 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 mvn clean install -P contrib-check
  • JDK 21

Licensing

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

Documentation

• [n/a] Documentation formatting appears as expected in rendered files

[exceptionfactory]

Is this something that should be considered for a unit or system test? It seems like somewhat of an edge case, and as a removal of behavior, it may not make sense to have a test, but worth considering.

[mosermw]

Is this something that should be considered for a unit or system test? It seems like somewhat of an edge case, and as a removal of behavior, it may not make sense to have a test, but worth considering.

I struggled with this @exceptionfactory. I could make a test of StandardVersionedComponentSynchronizer to verify that after synchronization, a VersionedProcessGroup's flowSnapshot is null, but that seemed a bit contrived since it wouldn't explain "why" that's desired.

In the end, it is desired because the StandardNiFiServiceFacade getLocalModifications() method will only update a VersionedProcessGroup's flowSnapshot if it doesn't already have a copy. I could make a unit test to verify that happens, but that code logic is straightforward (only 1 if statement) and it's not really related to the changes I made.

I could write an integration test that exercises all of this, starting with a call to StandardNiFiServiceFacade pasteComponents() and continuing with a call to StandardNiFiServiceFacade getLocalModifications() to exercise the scenario I described in the ticket ... copy/paste of a version controlled ProcessGroup that already has local modifications. But that seems like an insanely specific scenario to support in tests, so what are your thoughts on that?

[exceptionfactory]

Thanks for the helpful reply @mosermw, I agree that introducing new tests for this change doesn't seem to add value.

Tagging @markap14 or @bbende for some additional consideration of these changes.

[mosermw]

Thanks for the helpful reply @mosermw, I agree that introducing new tests for this change doesn't seem to add value.

LOL I was finalizing a commit that adds the copy/paste versioned PG use case to the CopyPasteIT system test. Coming very soon ...

[mosermw]

I took the opportunity to learn how the system-test-suite works, and added a system test to CopyPasteIT that exercises the scenario listed in the Jira.

Basically, create a versioned Process Group that has a local change, copy and paste that PG, then verify the pasted PG also shows as having a local change. This test fails on the current main branch, passes after the changes in this PR.

[exceptionfactory]

I took the opportunity to learn how the system-test-suite works, and added a system test to CopyPasteIT that exercises the scenario listed in the Jira.

Basically, create a versioned Process Group that has a local change, copy and paste that PG, then verify the pasted PG also shows as having a local change. This test fails on the current main branch, passes after the changes in this PR.

Thanks @mosermw, I'll take a closer look, very helpful!