NIFI-13030 Adding endpoint for comparing versions of registered flows

[simonbence]

Summary

NIFI-13030

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

• 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

[pgyori]

Thank you for this feature. I added some review comments, please consider them. Thanks!

[pgyori]

Thank you @simonbence for considering all my comments and making changes. Much appreciated!
I like the new enum items. Also, throughout the code it is now very clear what is the basis of interval comparison and what is being compared to it.
I went through the new modifications and added comments, please have a look at them.

[pgyori]

I do not agree with having a factory method in the generic interface: if further classes were to be added that implement the interface, then the interface would need to be modified each time to contain factory methods for each implementing type if we wanted to maintain consistency. I think this method could be removed and instead of calling Interval.getClosedOpenInterval(offset, higherBoundary) in PaginationHelper, new ClosedOpenInterval(offset, higherBoundary) constructor could be used.

[simonbence]

I prefer to keep the actual implementation class part of the implementation details. In general, clients do not need to know about if all the possible permutations of boundaries (ClosedClosed, ClosedOpen, etc.) are served by different classes or the same. Hiding this provides more flexibility for later changes and also prevents unintended usages. An other way would be to provide a factory class, like IntervalFactory but I felt it overkill.

[pgyori]

I understand your thought that since there is a finite number of combinations of boundaries, the interface could have methods for instantiating each combination and would not need to expose which implementing class is used. However, this current implementation violates 3 of the SOLID principles, namely:

• Single Responsibility: the interface now not only defines the capabilities of an Interval, but is also responsible for creating instances.
• Open-Closed Principle: Adding new implementations other than ClosedOpenInterval requires modification of Interval (to add new factory methods or modify exiting one(s)).
• Dependency Inversion: currently the interface depends on the implementing class.

[simonbence]

I slightly disagree with this primarily because of the static method usage but I have no strong feelings about it. Please expect a commit soonish

[bbende]

Is the intended use case for this to right-click on a process group under version control and be able to choose something like Compare With and then choose another version to compare with? is there any other scenario where we want to compare two flow versions?

I'm asking because if it is limited to the first scenario I described, we may want to consider a more specific API to the PG, similar to process-groups/{id}/local-changes, maybe /process-groups/{id}/diff. Then it would only need the parameters for the other flow version to compare to, which could be added to the path, or as query params.

[simonbence]

The main use case is to provide a possibility to make comparisons even before picking a version. As for the user's perspective this will result a similar listing as we have with Local changes. The intended place for this is the version list (for example when exeucting Change Version action on a versioned group as an addition.

The URL deliberately consists this many parameteres: the simplest situation is where we want to compers consecurtive versions of the same flow (in the same branch, etc.). A more complex, but still useful scenario is to compare flow snapshots between branches, like between a production and a development branch. As the branching strategy does not provide rescritions to follow the same structure for every branch, stricting this might make it impossible to make useful comparions with certain setups.

[bbende]

So just to clarify, you are saying that there is some PG under version control, lets say it is at v1. The user can select Change Version and see a versions list which has (1, 2, 3), and they can somehow select 2 and 3 and compare those even though the current flow is at 1? or can they only compare 1 against 2 or 3?

[simonbence]

As for URL perspective, other then the registry id, it is expected to provide branch-bucket-flow-version information for both sides of the comparson.

As for the user interface, the idea is to have a button with every version, like Compare To... which leads to the comparison view, which should be similart to the Local Changes, expect additional controls to pick the information above. The left side of the comparison should be already filled based on the picked version.

[bbende]

Just want to ask whether we think we truly need paging here. Generally most of NiFi's REST APIs don't page anything, and all of the paging is done on the client side. The reason is because the slow part is not sending a large response from server to client, it is the rendering of the whole response on the client. So as long as the client is rendering pages, it usually works totally fine.

If we do want paging, then shouldn't the entity being returned contain some paging info? At a minimum I would think we need to return the total rows in the result set, otherwise how can a client know when to page to?

[simonbence]

A significant difference from the Local Changes is that, we have a bigger chance to have more changed components, so I would expect more situation where we reach the limit of the first page (which is 1000, similar to the limitations of the Local Changes).

As of this I do not find it unlikely that we want to support pagination in the long run. As for now, I aim to serve the first page only, but I wanted to provide an implementation which will be a good basis for furher changes if we want to allow pagination in the future.

Upon the start of this improvement, I reached out to @markap14 in order to ask his opinion about the possible support of pagination within the API (which would either result an extension of the current entity or adding a new one) and he voted against to touch the API for now.

The solution I provided looked to satisfy this expectation as well (next to the expectations from the original purpose) and provides enough option for later adjustments. With this, if based on the community's experience the pagination will be a more significant need, we can add it easily, even without NiFi REST API changes.