You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by "simonbence (via GitHub)" <gi...@apache.org> on 2024/04/19 11:15:12 UTC
[PR] NIFI-13030 Adding endpoint for comparing versions of registered flows [nifi]
simonbence opened a new pull request, #8670:
URL: https://github.com/apache/nifi/pull/8670
# Summary
[NIFI-13030](https://issues.apache.org/jira/browse/NIFI-13030)
# Tracking
Please complete the following tracking steps prior to pull request creation.
### Issue Tracking
- [ ] [Apache NiFi Jira](https://issues.apache.org/jira/browse/NIFI) 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](https://apache.org/licenses/LICENSE-2.0) according to the [License Policy](https://www.apache.org/legal/resolved.html)
- [ ] New dependencies are documented in applicable `LICENSE` and `NOTICE` files
### Documentation
- [ ] Documentation formatting appears as expected in rendered files
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] NIFI-13030 Adding endpoint for comparing versions of registered flows [nifi]
Posted by "pgyori (via GitHub)" <gi...@apache.org>.
pgyori commented on code in PR #8670:
URL: https://github.com/apache/nifi/pull/8670#discussion_r1617159432
##########
nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/util/ClosedOpenInterval.java:
##########
@@ -0,0 +1,82 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.nifi.web.util;
+
+/**
+ * This implementation includes the lower boundary bud does not include the higher boundary.
+ */
+final class ClosedOpenInterval implements Interval {
+ private final int lowerBoundary;
+ private final int higherBoundary;
+
+ /**
+ * @param lowerBoundary Inclusive index of lower boundary
+ * @param higherBoundary Exclusive index of higher boundary. In case of 0, the higher boundary is unspecified and the interval is open.
+ */
+ ClosedOpenInterval(final int lowerBoundary, final int higherBoundary) {
+ if (lowerBoundary < 0) {
+ throw new IllegalArgumentException("Lower boundary cannot be negative");
+ }
+
+ if (higherBoundary < 0) {
+ throw new IllegalArgumentException("Higher boundary cannot be negative");
+ }
+
+ if (higherBoundary <= lowerBoundary && higherBoundary != 0) {
+ throw new IllegalArgumentException(
+ "Higher boundary cannot be lower than or equal to lower boundary except when unspecified. Higher boundary is considered unspecified when the value is set to 0"
+ );
+ }
+
+ this.lowerBoundary = lowerBoundary;
+ this.higherBoundary = higherBoundary;
+ }
+
+ @Override
+ public RelativePosition getRelativePositionOf(final int otherIntervalLowerBoundary, final int otherIntervalHigherBoundary) {
+ if (otherIntervalLowerBoundary < 0) {
+ throw new IllegalArgumentException("Lower boundary cannot be negative");
+ }
+
+ if (otherIntervalHigherBoundary <= 0) {
+ // Note: as a design decision the implementation currently does not support comparison with unspecified higher boundary
+ throw new IllegalArgumentException("Higher boundary must be positive");
+ }
+
+ if (otherIntervalLowerBoundary > otherIntervalHigherBoundary) {
Review Comment:
`otherIntervalLowerBoundary >= otherIntervalHigherBoundary` needs to be used in order to check for 0-length intervals where lower==higher (currently this case is not covered).
##########
nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/util/Interval.java:
##########
@@ -0,0 +1,70 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.nifi.web.util;
+
+public interface Interval {
+
+ enum RelativePosition {
+ /**
+ * The compared interval ends before the actual, there is no intersection.
+ */
+ BEFORE,
+
+ /**
+ * The compared interval exceeds the actual both at the low and high ends.
+ */
+ EXCEEDS,
+
+ /**
+ * The compared interval's tail (but not the whole interval) intersects the actual interval (part of it or the whole actual interval).
+ */
+ TAIL_INTERSECTS,
+
+ /**
+ * The compared interval is within the actual interval. It can match with the actual or contained by that.
+ */
+ WITHIN,
+
+ /**
+ *The compared interval's head (but not the whole interval) intersects the actual interval (part of it or the whole actual interval).
+ */
+ HEAD_INTERSECTS,
+
+ /**
+ * The compared interval starts after the actual, there is no intersection.
+ */
+ AFTER,
+ }
+
+ /**
+ * Relative position of the "other" interval compared to this.
+ *
+ * @param otherIntervalLowerBoundary Lower boundary of the compared interval.
+ * @param otherIntervalHigherBoundary Higher boundary of the compared interval.
+ *
+ * @return Returns the relative position of the "other" interval compared to this interval. For example: if the result
+ * is BEFORE, read it as: the other interval ends BEFORE the actual (and there is no intersection between them).
+ */
+ RelativePosition getRelativePositionOf(final int otherIntervalLowerBoundary, final int otherIntervalHigherBoundary);
Review Comment:
I'll leave this thought here for consideration:
I believe it would make sense for this method to accept another Interval object as parameter instead of boundary integers. That way in the subclass we do not have to implement again the checks for the reations between the lower and higher boundary. The only extra check in the sublcass' method would be to not allow the other interval to have undefined higher boundary.
##########
nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/util/ClosedOpenInterval.java:
##########
@@ -0,0 +1,82 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.nifi.web.util;
+
+/**
+ * This implementation includes the lower boundary bud does not include the higher boundary.
Review Comment:
Typo in the comment: "bud" = "but".
##########
nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/util/Interval.java:
##########
@@ -0,0 +1,70 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.nifi.web.util;
+
+public interface Interval {
+
+ enum RelativePosition {
+ /**
+ * The compared interval ends before the actual, there is no intersection.
+ */
+ BEFORE,
+
+ /**
+ * The compared interval exceeds the actual both at the low and high ends.
+ */
+ EXCEEDS,
+
+ /**
+ * The compared interval's tail (but not the whole interval) intersects the actual interval (part of it or the whole actual interval).
+ */
+ TAIL_INTERSECTS,
+
+ /**
+ * The compared interval is within the actual interval. It can match with the actual or contained by that.
+ */
+ WITHIN,
+
+ /**
+ *The compared interval's head (but not the whole interval) intersects the actual interval (part of it or the whole actual interval).
+ */
+ HEAD_INTERSECTS,
+
+ /**
+ * The compared interval starts after the actual, there is no intersection.
+ */
+ AFTER,
+ }
+
+ /**
+ * Relative position of the "other" interval compared to this.
+ *
+ * @param otherIntervalLowerBoundary Lower boundary of the compared interval.
+ * @param otherIntervalHigherBoundary Higher boundary of the compared interval.
+ *
+ * @return Returns the relative position of the "other" interval compared to this interval. For example: if the result
+ * is BEFORE, read it as: the other interval ends BEFORE the actual (and there is no intersection between them).
+ */
+ RelativePosition getRelativePositionOf(final int otherIntervalLowerBoundary, final int otherIntervalHigherBoundary);
+
+ /**
+ * @return Returns an interval instance with closed low and open high boundary.
+ */
+ static Interval getClosedOpenInterval(final int lowerBoundary, final int higherBoundary) {
Review Comment:
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.
##########
nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/test/java/org/apache/nifi/web/util/ClosedOpenIntervalTest.java:
##########
@@ -0,0 +1,93 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.nifi.web.util;
+
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
+
+import java.util.stream.Stream;
+
+class ClosedOpenIntervalTest {
+
+ @Test
+ public void testNegativeLowerBoundary() {
+ Assertions.assertThrows(IllegalArgumentException.class, () -> new ClosedOpenInterval(-1, 3));
+ }
+
+ @Test
+ public void testNegativeHigherBoundary() {
+ Assertions.assertThrows(IllegalArgumentException.class, () -> new ClosedOpenInterval(0, -1));
+ }
+
+ @Test
+ public void testReorderedBoundaries() {
Review Comment:
I believe a more appropriate name would be `testSwitchedBoundaries`, just like the word "switched" is used in `testCompareWhenOtherBoundariesAreSwitched`.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] NIFI-13030 Adding endpoint for comparing versions of registered flows [nifi]
Posted by "simonbence (via GitHub)" <gi...@apache.org>.
simonbence commented on code in PR #8670:
URL: https://github.com/apache/nifi/pull/8670#discussion_r1624142337
##########
nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/api/FlowResource.java:
##########
@@ -2057,6 +2062,100 @@ public Response getDetails(
return generateOkResponse(flowDetails).build();
}
+ @GET
+ @Consumes(MediaType.WILDCARD)
+ @Produces(MediaType.APPLICATION_JSON)
+ @Path("registries/{registry-id}/branches/{branch-id-a}/buckets/{bucket-id-a}/flows/{flow-id-a}/{version-a}/diff/branches/{branch-id-b}/buckets/{bucket-id-b}/flows/{flow-id-b}/{version-b}")
Review Comment:
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.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] NIFI-13030 Adding endpoint for comparing versions of registered flows [nifi]
Posted by "simonbence (via GitHub)" <gi...@apache.org>.
simonbence commented on code in PR #8670:
URL: https://github.com/apache/nifi/pull/8670#discussion_r1617407509
##########
nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/util/Interval.java:
##########
@@ -0,0 +1,70 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.nifi.web.util;
+
+public interface Interval {
+
+ enum RelativePosition {
+ /**
+ * The compared interval ends before the actual, there is no intersection.
+ */
+ BEFORE,
+
+ /**
+ * The compared interval exceeds the actual both at the low and high ends.
+ */
+ EXCEEDS,
+
+ /**
+ * The compared interval's tail (but not the whole interval) intersects the actual interval (part of it or the whole actual interval).
+ */
+ TAIL_INTERSECTS,
+
+ /**
+ * The compared interval is within the actual interval. It can match with the actual or contained by that.
+ */
+ WITHIN,
+
+ /**
+ *The compared interval's head (but not the whole interval) intersects the actual interval (part of it or the whole actual interval).
+ */
+ HEAD_INTERSECTS,
+
+ /**
+ * The compared interval starts after the actual, there is no intersection.
+ */
+ AFTER,
+ }
+
+ /**
+ * Relative position of the "other" interval compared to this.
+ *
+ * @param otherIntervalLowerBoundary Lower boundary of the compared interval.
+ * @param otherIntervalHigherBoundary Higher boundary of the compared interval.
+ *
+ * @return Returns the relative position of the "other" interval compared to this interval. For example: if the result
+ * is BEFORE, read it as: the other interval ends BEFORE the actual (and there is no intersection between them).
+ */
+ RelativePosition getRelativePositionOf(final int otherIntervalLowerBoundary, final int otherIntervalHigherBoundary);
Review Comment:
I had this idea due to symmetry but finally I voted against that. The following implications made me leave it out:
- With its current usage, numerous unnecessary object creation would come with this
- The "open ended" (no higher boundary) cases as you mention would need special handling
- With introducting the interface, later on possibly other implementations might be added which might make this method unnecessary complicated for its current purpose
So in the end, I did let it like this and in case of need we can add it later.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] NIFI-13030 Adding endpoint for comparing versions of registered flows [nifi]
Posted by "simonbence (via GitHub)" <gi...@apache.org>.
simonbence commented on code in PR #8670:
URL: https://github.com/apache/nifi/pull/8670#discussion_r1624156802
##########
nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/api/FlowResource.java:
##########
@@ -2104,6 +2203,24 @@ public Response getVersions(
return generateOkResponse(versionedFlowSnapshotMetadataSetEntity).build();
}
+ private static FlowComparisonEntity limitDifferences(final FlowComparisonEntity original, final int offset, final int limit) {
+ final List<ComponentDifferenceDTO> limited = PaginationHelper.paginateByContainedItems(
+ original.getComponentDifferences(), offset, limit, ComponentDifferenceDTO::getDifferences, FlowResource::limitDifferences);
+ final FlowComparisonEntity result = new FlowComparisonEntity();
+ result.setComponentDifferences(new HashSet<>(limited));
+ return result;
+ }
+
+ private static ComponentDifferenceDTO limitDifferences(final ComponentDifferenceDTO original, final List<DifferenceDTO> partial) {
+ final ComponentDifferenceDTO result = new ComponentDifferenceDTO();
+ result.setComponentType(original.getComponentType());
+ result.setComponentId(original.getComponentId());
+ result.setComponentName(original.getComponentName());
+ result.setProcessGroupId(original.getProcessGroupId());
+ result.setDifferences(partial);
+ return result;
+ }
Review Comment:
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.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] NIFI-13030 Adding endpoint for comparing versions of registered flows [nifi]
Posted by "pgyori (via GitHub)" <gi...@apache.org>.
pgyori commented on code in PR #8670:
URL: https://github.com/apache/nifi/pull/8670#discussion_r1593902067
##########
nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/util/Range.java:
##########
@@ -0,0 +1,72 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.nifi.web.util;
+
+public class Range {
+ private final int lowerBoundary;
+ private final int higherBoundary;
+
+ /**
+ * @param lowerBoundary Inclusive index of lower boundary
+ * @param higherBoundary Exclusive index of higher boundary. In case of 0, the higher boundary is unspecified and the range is open.
+ */
+ public Range(final int lowerBoundary, final int higherBoundary) {
+ if (lowerBoundary < 0) {
+ throw new IllegalArgumentException("Lower boundary cannot be negative");
+ }
+
+ if (higherBoundary < 0) {
+ throw new IllegalArgumentException("Higher boundary cannot be negative");
+ }
+
+ if (higherBoundary <= lowerBoundary && higherBoundary != 0) {
+ throw new IllegalArgumentException("Higher boundary cannot be lower or equal to lower boundary expect when unspecified");
Review Comment:
"Higher boundary cannot be lower than or equal to lower boundary except when unspecified"
I think it would help to add that higherBoundary = 0 means unspecified.
##########
nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/util/Range.java:
##########
@@ -0,0 +1,72 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.nifi.web.util;
+
+public class Range {
+ private final int lowerBoundary;
+ private final int higherBoundary;
+
+ /**
+ * @param lowerBoundary Inclusive index of lower boundary
+ * @param higherBoundary Exclusive index of higher boundary. In case of 0, the higher boundary is unspecified and the range is open.
+ */
+ public Range(final int lowerBoundary, final int higherBoundary) {
+ if (lowerBoundary < 0) {
+ throw new IllegalArgumentException("Lower boundary cannot be negative");
+ }
+
+ if (higherBoundary < 0) {
+ throw new IllegalArgumentException("Higher boundary cannot be negative");
+ }
+
+ if (higherBoundary <= lowerBoundary && higherBoundary != 0) {
+ throw new IllegalArgumentException("Higher boundary cannot be lower or equal to lower boundary expect when unspecified");
+ }
+
+ this.lowerBoundary = lowerBoundary;
+ this.higherBoundary = higherBoundary;
+ }
+
+ public RelativePosition getOverlapping(final int otherRangeLowerBoundary, final int otherRangeHigherBoundary) {
+ if (otherRangeHigherBoundary < lowerBoundary) {
+ return RelativePosition.BEFORE_RANGE;
+ } else if (otherRangeLowerBoundary < lowerBoundary && otherRangeHigherBoundary >= higherBoundary && !isOpenEnded()) {
+ return RelativePosition.MIDDLE_IS_WITHIN_RANGE;
+ } else if (otherRangeLowerBoundary < lowerBoundary) {
+ return RelativePosition.TAIL_IS_WITHIN_RANGE;
+ } else if (otherRangeHigherBoundary < higherBoundary || isOpenEnded()) {
Review Comment:
<= operator is needed.
##########
nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/util/Range.java:
##########
@@ -0,0 +1,72 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.nifi.web.util;
+
+public class Range {
+ private final int lowerBoundary;
+ private final int higherBoundary;
+
+ /**
+ * @param lowerBoundary Inclusive index of lower boundary
+ * @param higherBoundary Exclusive index of higher boundary. In case of 0, the higher boundary is unspecified and the range is open.
+ */
+ public Range(final int lowerBoundary, final int higherBoundary) {
+ if (lowerBoundary < 0) {
+ throw new IllegalArgumentException("Lower boundary cannot be negative");
+ }
+
+ if (higherBoundary < 0) {
+ throw new IllegalArgumentException("Higher boundary cannot be negative");
+ }
+
+ if (higherBoundary <= lowerBoundary && higherBoundary != 0) {
+ throw new IllegalArgumentException("Higher boundary cannot be lower or equal to lower boundary expect when unspecified");
+ }
+
+ this.lowerBoundary = lowerBoundary;
+ this.higherBoundary = higherBoundary;
+ }
+
+ public RelativePosition getOverlapping(final int otherRangeLowerBoundary, final int otherRangeHigherBoundary) {
+ if (otherRangeHigherBoundary < lowerBoundary) {
+ return RelativePosition.BEFORE_RANGE;
+ } else if (otherRangeLowerBoundary < lowerBoundary && otherRangeHigherBoundary >= higherBoundary && !isOpenEnded()) {
+ return RelativePosition.MIDDLE_IS_WITHIN_RANGE;
+ } else if (otherRangeLowerBoundary < lowerBoundary) {
+ return RelativePosition.TAIL_IS_WITHIN_RANGE;
+ } else if (otherRangeHigherBoundary < higherBoundary || isOpenEnded()) {
+ return RelativePosition.FULLY_WITHIN_RANGE;
+ } else if (otherRangeLowerBoundary < higherBoundary) {
+ return RelativePosition.HEAD_IS_WITHIN_RANGE;
+ } else {
+ return RelativePosition.AFTER_RANGE;
+ }
+ }
+
+ private boolean isOpenEnded() {
+ return higherBoundary == 0;
+ }
+
+ public enum RelativePosition {
Review Comment:
I recommend the following enum items for brevity and clarity:
BEFORE_RANGE
TAIL_IN_RANGE
IN_RANGE
HEAD_IN_RANGE
EXCEEDS_RANGE (instead of MIDDLE_IS_WITHIN_RANGE)
AFTER_RANGE
##########
nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/StandardNiFiServiceFacade.java:
##########
@@ -5205,6 +5205,40 @@ public RegisteredFlow deleteVersionedFlow(final String registryId, final String
}
}
+ @Override
+ public FlowComparisonEntity getVersionDifference(final String registryId, final String bucketId, final String flowId, final String versionA, final String versionB) {
+ final FlowComparisonEntity result = new FlowComparisonEntity();
+
+ if (versionA == versionB) {
Review Comment:
`versionA.equals(versionB)`
##########
nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/util/Range.java:
##########
@@ -0,0 +1,72 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.nifi.web.util;
+
+public class Range {
+ private final int lowerBoundary;
+ private final int higherBoundary;
+
+ /**
+ * @param lowerBoundary Inclusive index of lower boundary
+ * @param higherBoundary Exclusive index of higher boundary. In case of 0, the higher boundary is unspecified and the range is open.
+ */
+ public Range(final int lowerBoundary, final int higherBoundary) {
+ if (lowerBoundary < 0) {
+ throw new IllegalArgumentException("Lower boundary cannot be negative");
+ }
+
+ if (higherBoundary < 0) {
+ throw new IllegalArgumentException("Higher boundary cannot be negative");
+ }
+
+ if (higherBoundary <= lowerBoundary && higherBoundary != 0) {
+ throw new IllegalArgumentException("Higher boundary cannot be lower or equal to lower boundary expect when unspecified");
+ }
+
+ this.lowerBoundary = lowerBoundary;
+ this.higherBoundary = higherBoundary;
+ }
+
+ public RelativePosition getOverlapping(final int otherRangeLowerBoundary, final int otherRangeHigherBoundary) {
Review Comment:
When comparing two ranges, there is `range` and `otherRange`.
It looks to me that in this method `otherRange` is considered the basis of comparison whereas in RangeTest `range` is the basis of comparison. I recommend always considering `range` as the basis and telling the position of `otherRange` compared to that. For example, in the chart the blue line is the basis of comparison and in case 1 the black line is after the blue line, therefore it is marked as `AFTER_RANGE` because the black is after the blue.
##########
nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/util/Range.java:
##########
@@ -0,0 +1,72 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.nifi.web.util;
+
+public class Range {
+ private final int lowerBoundary;
+ private final int higherBoundary;
+
+ /**
+ * @param lowerBoundary Inclusive index of lower boundary
+ * @param higherBoundary Exclusive index of higher boundary. In case of 0, the higher boundary is unspecified and the range is open.
+ */
+ public Range(final int lowerBoundary, final int higherBoundary) {
+ if (lowerBoundary < 0) {
+ throw new IllegalArgumentException("Lower boundary cannot be negative");
+ }
+
+ if (higherBoundary < 0) {
+ throw new IllegalArgumentException("Higher boundary cannot be negative");
+ }
+
+ if (higherBoundary <= lowerBoundary && higherBoundary != 0) {
+ throw new IllegalArgumentException("Higher boundary cannot be lower or equal to lower boundary expect when unspecified");
+ }
+
+ this.lowerBoundary = lowerBoundary;
+ this.higherBoundary = higherBoundary;
+ }
+
+ public RelativePosition getOverlapping(final int otherRangeLowerBoundary, final int otherRangeHigherBoundary) {
Review Comment:
The algorithm in this method does not handle every case correctly, please consider reworking it. I added some charts that might help: When comparing two ranges, there are 13 different positions. I attached an image depicting each case.
<img width="1763" alt="Screenshot 2024-05-08 at 13 56 21" src="https://github.com/apache/nifi/assets/63872658/c4e5fe15-5056-42da-b8dc-d395cd8b90fa">
If we consider the object `this` as the basis of the comparison to which `otherRange` is compared, then the 13 cases can be categorized like this:
<img width="348" alt="Screenshot 2024-05-08 at 13 57 14" src="https://github.com/apache/nifi/assets/63872658/424a8978-13ec-4dc7-8393-77f69921a276">
In my comment for RangeTest I added test cases that highlight the issue.
##########
nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/util/Range.java:
##########
@@ -0,0 +1,72 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.nifi.web.util;
+
+public class Range {
+ private final int lowerBoundary;
+ private final int higherBoundary;
+
+ /**
+ * @param lowerBoundary Inclusive index of lower boundary
+ * @param higherBoundary Exclusive index of higher boundary. In case of 0, the higher boundary is unspecified and the range is open.
+ */
+ public Range(final int lowerBoundary, final int higherBoundary) {
+ if (lowerBoundary < 0) {
+ throw new IllegalArgumentException("Lower boundary cannot be negative");
+ }
+
+ if (higherBoundary < 0) {
+ throw new IllegalArgumentException("Higher boundary cannot be negative");
+ }
+
+ if (higherBoundary <= lowerBoundary && higherBoundary != 0) {
+ throw new IllegalArgumentException("Higher boundary cannot be lower or equal to lower boundary expect when unspecified");
+ }
+
+ this.lowerBoundary = lowerBoundary;
+ this.higherBoundary = higherBoundary;
+ }
+
+ public RelativePosition getOverlapping(final int otherRangeLowerBoundary, final int otherRangeHigherBoundary) {
+ if (otherRangeHigherBoundary < lowerBoundary) {
Review Comment:
Since the higher boundary is exclusive and the lower is inclusive, we need <= operator here.
##########
nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/util/PaginationHelper.java:
##########
@@ -0,0 +1,99 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.nifi.web.util;
+
+import java.util.LinkedList;
+import java.util.List;
+import java.util.Objects;
+import java.util.function.BiFunction;
+import java.util.function.Function;
+
+public class PaginationHelper {
+ public static <T, E> List<T> paginateByContainedItems(
+ final Iterable<T> original,
+ final int offset,
+ final int limit,
+ final Function<T, List<E>> getContainedItems,
+ final BiFunction<T, List<E>, T> createPartialItem
+ ) {
+ Objects.requireNonNull(original);
+ Objects.requireNonNull(getContainedItems);
+ Objects.requireNonNull(createPartialItem);
+
+ if (offset < 0) {
+ throw new IllegalArgumentException("Offset cannot be negative");
+ }
+
+ if (limit < 0) {
+ throw new IllegalArgumentException("Limit cannot be negative");
+ }
+
+ final List<T> result = new LinkedList<>();
+ final int higherBoundary = limit == 0 ? 0 : offset + limit;
+ final Range range = new Range(offset, higherBoundary);
+ int pointer = 0;
+
+ if (offset == 0 && limit == 0) {
+ original.forEach(result::add);
+ return result;
+ }
+
+ for (final T candidate : original) {
+ final List<E> containedItems = getContainedItems.apply(candidate);
+ final Range.RelativePosition position = range.getOverlapping(pointer, pointer + containedItems.size() - 1);
+
+ switch (position) {
Review Comment:
Please consider renaming the enum items according to my suggestion in Range class.
##########
nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/test/java/org/apache/nifi/web/util/RangeTest.java:
##########
@@ -0,0 +1,73 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.nifi.web.util;
+
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
+
+import java.util.stream.Stream;
+
+class RangeTest {
+
+ @Test
+ public void testNegativeLowerBoundary() {
+ Assertions.assertThrows(IllegalArgumentException.class, () -> new Range(-1, 3));
+ }
+
+ @Test
+ public void testNegativeHigherBoundary() {
+ Assertions.assertThrows(IllegalArgumentException.class, () -> new Range(0, -1));
+ }
+
+ @Test
+ public void testReorderedBoundaries() {
+ Assertions.assertThrows(IllegalArgumentException.class, () -> new Range(7, 3));
+ }
+
+ @ParameterizedTest(name = "{0}")
+ @MethodSource("givenTestData")
+ public void testCheckForOverlapping(final String name, final Range window, final int otherRangeLowerBoundary, final int otherRangeHigherBoundary, final Range.RelativePosition expectedResult) {
+ Assertions.assertEquals(expectedResult, window.getOverlapping(otherRangeLowerBoundary, otherRangeHigherBoundary));
+ }
+
+ private static Stream<Arguments> givenTestData() {
Review Comment:
The 13 possible test cases are like this:
<img width="1763" alt="Screenshot 2024-05-08 at 13 56 21" src="https://github.com/apache/nifi/assets/63872658/e8613d20-f0d1-4fd1-a051-0ae6a021afe7">
I recommend adding a test case for each:
`Arguments.of("case 1", new Range(7, 10), 11, 13, Range.RelativePosition.AFTER_RANGE),
Arguments.of("case 2", new Range(7,10), 10, 13, Range.RelativePosition.AFTER_RANGE),
Arguments.of("case 3", new Range(7,10), 9, 13, Range.RelativePosition.HEAD_IN_RANGE),
Arguments.of("case 4", new Range(7, 10), 8, 10, Range.RelativePosition.IN_RANGE),
Arguments.of("case 5", new Range(7,10), 8, 9, Range.RelativePosition.IN_RANGE),
Arguments.of("case 6", new Range(7,10), 7, 12, Range.RelativePosition.HEAD_IN_RANGE),
Arguments.of("case 7", new Range(7,10), 7, 10, Range.RelativePosition.IN_RANGE),
Arguments.of("case 8", new Range(7,10), 7, 9, Range.RelativePosition.IN_RANGE),
Arguments.of("case 9", new Range(7,10), 6, 12, Range.RelativePosition.EXCEEDS_RANGE),
Arguments.of("case 10", new Range(7,10), 5, 10, Range.RelativePosition.TAIL_IN_RANGE),
Arguments.of("case 11", new Range(7,10), 5, 9, Range.RelativePosition.TAIL_IN_RANGE),
Arguments.of("case 12", new Range(7,10), 2, 7, Range.RelativePosition.BEFORE_RANGE),
Arguments.of("case 13", new Range(7,10), 2, 6, Range.RelativePosition.BEFORE_RANGE),`
##########
nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/test/java/org/apache/nifi/web/util/PaginationHelperTest.java:
##########
@@ -0,0 +1,74 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.nifi.web.util;
+
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
+
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.List;
+import java.util.function.Function;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
+
+class PaginationHelperTest {
+
+ @Test
+ public void testCreatingWithNegativeOffset() {
+ Assertions.assertThrows(IllegalArgumentException.class, () -> PaginationHelper.paginateByContainedItems(getTestInput(), -1, 3, Function.identity(), (original, partialList) -> partialList));
+ }
+
+ @Test
+ public void testCreatingWithNegativeLimit() {
+ Assertions.assertThrows(IllegalArgumentException.class, () -> PaginationHelper.paginateByContainedItems(getTestInput(), 0, -1, Function.identity(), (original, partialList) -> partialList));
+ }
+
+ @ParameterizedTest(name = "{0}")
+ @MethodSource("givenTestData")
+ public void testPaginateByContainedItems(final String name, final int offset, final int limit, final List<Integer> expectedResult) {
+ final List<List<Integer>> result = PaginationHelper.paginateByContainedItems(getTestInput(), offset, limit, Function.identity(), (original, partialList) -> partialList);
+ final List<Integer> flatten = result.stream().flatMap(Collection::stream).collect(Collectors.toList());
+ Assertions.assertIterableEquals(expectedResult, flatten);
+ }
+
+ private static Stream<Arguments> givenTestData() {
Review Comment:
Considering the 13 cases shown in the chart attached to the review of Range class, cases 1, 2,3,4 and 5 need to be covered with `Assertions.assertThrows()` test cases. I'm pasting here the existing test cases, each marked with a comment showing which case it covers:
```
Arguments.of("Full result set", 0, 0, Arrays.asList(1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12)), // case 8
Arguments.of("Offset only when starts with full", 3, 0, Arrays.asList(4, 5, 6, 7, 8, 9, 10, 11, 12)), // case 11
Arguments.of("Offset only when starts with partial", 4, 0, Arrays.asList(5, 6, 7, 8, 9, 10, 11, 12)), // case 11
Arguments.of("From beginning with partial", 0, 5, Arrays.asList(1, 2, 3, 4, 5)), // case 6
Arguments.of("Beginning with partial and offset", 1, 5, Arrays.asList(2, 3, 4, 5, 6)), // case 9
Arguments.of("Middle partial only", 4, 2, Arrays.asList(5, 6)), // case 9
Arguments.of("From the end with partial", 9, 3, Arrays.asList(10, 11, 12)), // case 9
Arguments.of("From the end with partial when spills over", 9, 5, Arrays.asList(10, 11, 12)), // case 11
Arguments.of("Clear cut", 3, 5, Arrays.asList(4, 5, 6, 7, 8)), // case 9
Arguments.of("Long result", 2, 8, Arrays.asList(3, 4, 5, 6, 7, 8, 9, 10)) // case 9
```
And I recommend adding tests for the missing cases:
```
Arguments.of("case 7", 0, 12, Arrays.asList(1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12)),
Arguments.of("case 10", 5, 7, Arrays.asList(6, 7, 8, 9, 10, 11, 12)),
Arguments.of("case 12", 12, 3, Collections.emptyList()),
Arguments.of("case 13", 14, 4, Collections.emptyList())
```
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] NIFI-13030 Adding endpoint for comparing versions of registered flows [nifi]
Posted by "pgyori (via GitHub)" <gi...@apache.org>.
pgyori commented on code in PR #8670:
URL: https://github.com/apache/nifi/pull/8670#discussion_r1618578862
##########
nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/util/Interval.java:
##########
@@ -0,0 +1,70 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.nifi.web.util;
+
+public interface Interval {
+
+ enum RelativePosition {
+ /**
+ * The compared interval ends before the actual, there is no intersection.
+ */
+ BEFORE,
+
+ /**
+ * The compared interval exceeds the actual both at the low and high ends.
+ */
+ EXCEEDS,
+
+ /**
+ * The compared interval's tail (but not the whole interval) intersects the actual interval (part of it or the whole actual interval).
+ */
+ TAIL_INTERSECTS,
+
+ /**
+ * The compared interval is within the actual interval. It can match with the actual or contained by that.
+ */
+ WITHIN,
+
+ /**
+ *The compared interval's head (but not the whole interval) intersects the actual interval (part of it or the whole actual interval).
+ */
+ HEAD_INTERSECTS,
+
+ /**
+ * The compared interval starts after the actual, there is no intersection.
+ */
+ AFTER,
+ }
+
+ /**
+ * Relative position of the "other" interval compared to this.
+ *
+ * @param otherIntervalLowerBoundary Lower boundary of the compared interval.
+ * @param otherIntervalHigherBoundary Higher boundary of the compared interval.
+ *
+ * @return Returns the relative position of the "other" interval compared to this interval. For example: if the result
+ * is BEFORE, read it as: the other interval ends BEFORE the actual (and there is no intersection between them).
+ */
+ RelativePosition getRelativePositionOf(final int otherIntervalLowerBoundary, final int otherIntervalHigherBoundary);
+
+ /**
+ * @return Returns an interval instance with closed low and open high boundary.
+ */
+ static Interval getClosedOpenInterval(final int lowerBoundary, final int higherBoundary) {
Review Comment:
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.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] NIFI-13030 Adding endpoint for comparing versions of registered flows [nifi]
Posted by "simonbence (via GitHub)" <gi...@apache.org>.
simonbence commented on code in PR #8670:
URL: https://github.com/apache/nifi/pull/8670#discussion_r1618730811
##########
nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/util/Interval.java:
##########
@@ -0,0 +1,70 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.nifi.web.util;
+
+public interface Interval {
+
+ enum RelativePosition {
+ /**
+ * The compared interval ends before the actual, there is no intersection.
+ */
+ BEFORE,
+
+ /**
+ * The compared interval exceeds the actual both at the low and high ends.
+ */
+ EXCEEDS,
+
+ /**
+ * The compared interval's tail (but not the whole interval) intersects the actual interval (part of it or the whole actual interval).
+ */
+ TAIL_INTERSECTS,
+
+ /**
+ * The compared interval is within the actual interval. It can match with the actual or contained by that.
+ */
+ WITHIN,
+
+ /**
+ *The compared interval's head (but not the whole interval) intersects the actual interval (part of it or the whole actual interval).
+ */
+ HEAD_INTERSECTS,
+
+ /**
+ * The compared interval starts after the actual, there is no intersection.
+ */
+ AFTER,
+ }
+
+ /**
+ * Relative position of the "other" interval compared to this.
+ *
+ * @param otherIntervalLowerBoundary Lower boundary of the compared interval.
+ * @param otherIntervalHigherBoundary Higher boundary of the compared interval.
+ *
+ * @return Returns the relative position of the "other" interval compared to this interval. For example: if the result
+ * is BEFORE, read it as: the other interval ends BEFORE the actual (and there is no intersection between them).
+ */
+ RelativePosition getRelativePositionOf(final int otherIntervalLowerBoundary, final int otherIntervalHigherBoundary);
+
+ /**
+ * @return Returns an interval instance with closed low and open high boundary.
+ */
+ static Interval getClosedOpenInterval(final int lowerBoundary, final int higherBoundary) {
Review Comment:
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
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] NIFI-13030 Adding endpoint for comparing versions of registered flows [nifi]
Posted by "bbende (via GitHub)" <gi...@apache.org>.
bbende commented on code in PR #8670:
URL: https://github.com/apache/nifi/pull/8670#discussion_r1619124970
##########
nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/api/FlowResource.java:
##########
@@ -2104,6 +2203,24 @@ public Response getVersions(
return generateOkResponse(versionedFlowSnapshotMetadataSetEntity).build();
}
+ private static FlowComparisonEntity limitDifferences(final FlowComparisonEntity original, final int offset, final int limit) {
+ final List<ComponentDifferenceDTO> limited = PaginationHelper.paginateByContainedItems(
+ original.getComponentDifferences(), offset, limit, ComponentDifferenceDTO::getDifferences, FlowResource::limitDifferences);
+ final FlowComparisonEntity result = new FlowComparisonEntity();
+ result.setComponentDifferences(new HashSet<>(limited));
+ return result;
+ }
+
+ private static ComponentDifferenceDTO limitDifferences(final ComponentDifferenceDTO original, final List<DifferenceDTO> partial) {
+ final ComponentDifferenceDTO result = new ComponentDifferenceDTO();
+ result.setComponentType(original.getComponentType());
+ result.setComponentId(original.getComponentId());
+ result.setComponentName(original.getComponentName());
+ result.setProcessGroupId(original.getProcessGroupId());
+ result.setDifferences(partial);
+ return result;
+ }
Review Comment:
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?
##########
nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/api/FlowResource.java:
##########
@@ -2057,6 +2062,100 @@ public Response getDetails(
return generateOkResponse(flowDetails).build();
}
+ @GET
+ @Consumes(MediaType.WILDCARD)
+ @Produces(MediaType.APPLICATION_JSON)
+ @Path("registries/{registry-id}/branches/{branch-id-a}/buckets/{bucket-id-a}/flows/{flow-id-a}/{version-a}/diff/branches/{branch-id-b}/buckets/{bucket-id-b}/flows/{flow-id-b}/{version-b}")
Review Comment:
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.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] NIFI-13030 Adding endpoint for comparing versions of registered flows [nifi]
Posted by "bbende (via GitHub)" <gi...@apache.org>.
bbende commented on code in PR #8670:
URL: https://github.com/apache/nifi/pull/8670#discussion_r1619117358
##########
nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/api/FlowResource.java:
##########
@@ -2057,6 +2062,100 @@ public Response getDetails(
return generateOkResponse(flowDetails).build();
}
+ @GET
+ @Consumes(MediaType.WILDCARD)
+ @Produces(MediaType.APPLICATION_JSON)
+ @Path("registries/{registry-id}/branches/{branch-id-a}/buckets/{bucket-id-a}/flows/{flow-id-a}/{version-a}/diff/branches/{branch-id-b}/buckets/{bucket-id-b}/flows/{flow-id-b}/{version-b}")
Review Comment:
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.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] NIFI-13030 Adding endpoint for comparing versions of registered flows [nifi]
Posted by "simonbence (via GitHub)" <gi...@apache.org>.
simonbence commented on code in PR #8670:
URL: https://github.com/apache/nifi/pull/8670#discussion_r1596632882
##########
nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/NiFiServiceFacade.java:
##########
@@ -1498,6 +1498,18 @@ Set<DocumentedTypeDTO> getControllerServiceTypes(final String serviceType, final
*/
RegisteredFlow deleteVersionedFlow(String registryId, String bucketId, String flowId);
+ /**
+ * Returns the differences of version B from version A.
+ *
+ * @param registryId the ID of the registry
+ * @param bucketId the ID of the bucket
+ * @param flowId the ID of the flow
+ * @param versionA version of the baseline snapshot of the comparison
+ * @param versionB version of the compared snapshot
+ * @return the differences between the snapshots
+ */
+ FlowComparisonEntity getVersionDifference(String registryId, String bucketId, String flowId, String versionA, String versionB);
Review Comment:
I might need to check if the new branching causes changes here
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] NIFI-13030 Adding endpoint for comparing versions of registered flows [nifi]
Posted by "bbende (via GitHub)" <gi...@apache.org>.
bbende commented on code in PR #8670:
URL: https://github.com/apache/nifi/pull/8670#discussion_r1624479905
##########
nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/api/FlowResource.java:
##########
@@ -2057,6 +2062,100 @@ public Response getDetails(
return generateOkResponse(flowDetails).build();
}
+ @GET
+ @Consumes(MediaType.WILDCARD)
+ @Produces(MediaType.APPLICATION_JSON)
+ @Path("registries/{registry-id}/branches/{branch-id-a}/buckets/{bucket-id-a}/flows/{flow-id-a}/{version-a}/diff/branches/{branch-id-b}/buckets/{bucket-id-b}/flows/{flow-id-b}/{version-b}")
Review Comment:
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?
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] NIFI-13030 Adding endpoint for comparing versions of registered flows [nifi]
Posted by "simonbence (via GitHub)" <gi...@apache.org>.
simonbence commented on code in PR #8670:
URL: https://github.com/apache/nifi/pull/8670#discussion_r1617424178
##########
nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/util/Interval.java:
##########
@@ -0,0 +1,70 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.nifi.web.util;
+
+public interface Interval {
+
+ enum RelativePosition {
+ /**
+ * The compared interval ends before the actual, there is no intersection.
+ */
+ BEFORE,
+
+ /**
+ * The compared interval exceeds the actual both at the low and high ends.
+ */
+ EXCEEDS,
+
+ /**
+ * The compared interval's tail (but not the whole interval) intersects the actual interval (part of it or the whole actual interval).
+ */
+ TAIL_INTERSECTS,
+
+ /**
+ * The compared interval is within the actual interval. It can match with the actual or contained by that.
+ */
+ WITHIN,
+
+ /**
+ *The compared interval's head (but not the whole interval) intersects the actual interval (part of it or the whole actual interval).
+ */
+ HEAD_INTERSECTS,
+
+ /**
+ * The compared interval starts after the actual, there is no intersection.
+ */
+ AFTER,
+ }
+
+ /**
+ * Relative position of the "other" interval compared to this.
+ *
+ * @param otherIntervalLowerBoundary Lower boundary of the compared interval.
+ * @param otherIntervalHigherBoundary Higher boundary of the compared interval.
+ *
+ * @return Returns the relative position of the "other" interval compared to this interval. For example: if the result
+ * is BEFORE, read it as: the other interval ends BEFORE the actual (and there is no intersection between them).
+ */
+ RelativePosition getRelativePositionOf(final int otherIntervalLowerBoundary, final int otherIntervalHigherBoundary);
+
+ /**
+ * @return Returns an interval instance with closed low and open high boundary.
+ */
+ static Interval getClosedOpenInterval(final int lowerBoundary, final int higherBoundary) {
Review Comment:
I prefer to keep the actual implementation class part of the implementation details. In general, clients does 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.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] NIFI-13030 Adding endpoint for comparing versions of registered flows [nifi]
Posted by "simonbence (via GitHub)" <gi...@apache.org>.
simonbence commented on code in PR #8670:
URL: https://github.com/apache/nifi/pull/8670#discussion_r1617424178
##########
nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/util/Interval.java:
##########
@@ -0,0 +1,70 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.nifi.web.util;
+
+public interface Interval {
+
+ enum RelativePosition {
+ /**
+ * The compared interval ends before the actual, there is no intersection.
+ */
+ BEFORE,
+
+ /**
+ * The compared interval exceeds the actual both at the low and high ends.
+ */
+ EXCEEDS,
+
+ /**
+ * The compared interval's tail (but not the whole interval) intersects the actual interval (part of it or the whole actual interval).
+ */
+ TAIL_INTERSECTS,
+
+ /**
+ * The compared interval is within the actual interval. It can match with the actual or contained by that.
+ */
+ WITHIN,
+
+ /**
+ *The compared interval's head (but not the whole interval) intersects the actual interval (part of it or the whole actual interval).
+ */
+ HEAD_INTERSECTS,
+
+ /**
+ * The compared interval starts after the actual, there is no intersection.
+ */
+ AFTER,
+ }
+
+ /**
+ * Relative position of the "other" interval compared to this.
+ *
+ * @param otherIntervalLowerBoundary Lower boundary of the compared interval.
+ * @param otherIntervalHigherBoundary Higher boundary of the compared interval.
+ *
+ * @return Returns the relative position of the "other" interval compared to this interval. For example: if the result
+ * is BEFORE, read it as: the other interval ends BEFORE the actual (and there is no intersection between them).
+ */
+ RelativePosition getRelativePositionOf(final int otherIntervalLowerBoundary, final int otherIntervalHigherBoundary);
+
+ /**
+ * @return Returns an interval instance with closed low and open high boundary.
+ */
+ static Interval getClosedOpenInterval(final int lowerBoundary, final int higherBoundary) {
Review Comment:
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.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] NIFI-13030 Adding endpoint for comparing versions of registered flows [nifi]
Posted by "pgyori (via GitHub)" <gi...@apache.org>.
pgyori closed pull request #8670: NIFI-13030 Adding endpoint for comparing versions of registered flows
URL: https://github.com/apache/nifi/pull/8670
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] NIFI-13030 Adding endpoint for comparing versions of registered flows [nifi]
Posted by "simonbence (via GitHub)" <gi...@apache.org>.
simonbence commented on code in PR #8670:
URL: https://github.com/apache/nifi/pull/8670#discussion_r1624749082
##########
nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/api/FlowResource.java:
##########
@@ -2057,6 +2062,100 @@ public Response getDetails(
return generateOkResponse(flowDetails).build();
}
+ @GET
+ @Consumes(MediaType.WILDCARD)
+ @Produces(MediaType.APPLICATION_JSON)
+ @Path("registries/{registry-id}/branches/{branch-id-a}/buckets/{bucket-id-a}/flows/{flow-id-a}/{version-a}/diff/branches/{branch-id-b}/buckets/{bucket-id-b}/flows/{flow-id-b}/{version-b}")
Review Comment:
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.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org