NIFI-10608 Copying Process Group now includes non-processor referenced controller services

[NissimShiman]

Summary

NIFI-10608

Copying a processor group used to miss controller services if controller service had not been referenced to a processor.
This resolves that.

To show fix:
create new PG -> right click -> Configure -> Controller Sevices -> Add any controller service (AvroReader for example)
copy PG
copied PG -> right click -> Configure -> Controller Services -> (new controller service is now listed/ i.e. AvroReader will be listed)

Prior to this the new controller service/Avro Reader would not be listed for the copied PG.

This same issue was also present when making templates (i.e. the template would miss controller service that had not been referenced to a processor). This is now resolved for templates as well.

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 8
  • JDK 11
  • JDK 17

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

[markobean]

First, I was able to reproduce the problem with NiFi 1.18.0. I created a process group with an unreferenced controller service. Then, I copy/pasted the PG. The new PG did not contain the unreferenced controller service.

Next, I rebased this PR branch to current main because I was having build issues on my platform with source code from pre-1.19.0. Performed a full build with unit tests successfully.

I installed NiFi and attempted to reproduce the copy/paste problem. The pasted PG correctly contained the unreferenced controller service.

Also, I noted the same problem exists when creating a template in 1.18.0; an unreferenced CS is not in the template. Confirmed by usage and by inspecting exported template XML. With this bug fix, the template correctly includes the unreferenced CS.

Thanks for the fix @NissimShiman !

+1

[joewitt]

Are there existing unit tests you could add to which would show the fix has the intended result?

[NissimShiman]

@joewitt That is a fair point. For some reason this area of code is lightly tested (i.e. there are no unit tests for the method this is called in. And that goes for all the parent methods of this as well)

[joewitt]

Huh - i just noticed @markobean did review this. @markobean any reason you didn't merge it?

Got ya on tests @NissimShiman . Do you think you could create a reasonable test with reasonable effort or is this code hard to test reliably?

[markobean]

Reminder @joewitt - I did not merge because I am not a committer.

As far as unit tests, it may be reasonable to add, but this feels more like an integration testing option. And might begin to go down the rabbit hole. If we add a test for this, do we also add a test to ensure all processors are copied with the process group? child process groups? funnels? etc.

@NissimShiman If you think adding a unit test for the copy operation is reasonable, is it also reasonable to have each component type tested?

[NissimShiman]

@joewitt I think that could be done. i.e. a unit test for this in SnippetUtilsTest ( https://github.com/apache/nifi/blob/main/nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/test/java/org/apache/nifi/web/util/SnippetUtilsTest.java )

although as @markobean noted, this kind of gets into the more overarching question of testing in general for copying nifi components and if we want to address that.

(i.e. I am a little concerned this could lead to scope creep. I happened to come across this when testing/voting for apache nifi 1.18.0 RC4, so I wanted to close the loop with this, but this discussion is revealing that testing copying in general is an area that needs to be developed that I am hoping not to be led into at the moment :))

So to answer the question, yes, a unit test can be done for this, but we need to be OK with the general lack of copying related unit/integration tests that may now be more readily noticeable by having this one added.

[joewitt]

sorry about that Mark - i have been thinking ever since I saw that note before you were. My mistake.

I think if tests existed you should be able to add to them. Creating tests where there are none and then having to deal with the existing untested bits is a bit much yeah. I'm fine with this as is but I do think someone more familiar with this portion of the code should review/merge. Mark's review from a functional point of view should make this super fast for someone more familiar. If not...I'll merge it and call it good soon.

[mattyb149]

+1 LGTM, thanks for the review Mark! I too verified the expected behavior. Thanks for the fix! Merging to main

[NissimShiman]

Thank you very much @mattyb149 and @markobean for looking at this!