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 2023/06/19 11:32:38 UTC
[GitHub] [nifi] simonbence opened a new pull request, #7401: NIFI-11706 Add capability to create dedicated Parameter Contexts for versioned flows when importing
simonbence opened a new pull request, #7401:
URL: https://github.com/apache/nifi/pull/7401
# Summary
[NIFI-11706](https://issues.apache.org/jira/browse/NIFI-11706)
(Test scenarios helping to understand and review the changes are added as comment on the ticket)
As many Versioned Flows are imported multiple times but used with in somewhat different circumstances (such as: different endpoints, databases, etc.), it would make sense to give the user the possibility to import flows in a way that their Parameter Context become independent from the other instances. In order to achieve this a new checkbox will be added to the Import Version window which makes it possible to decide for the user if she wants to keep the existing Parameter Contexts (bounding the parameters to the potentially existing instances) or she wants to create a new set of Parameter Contexts making the flow parametrised independently.
# 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 17
### 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
[GitHub] [nifi] exceptionfactory commented on a diff in pull request #7401: NIFI-11706 Add capability to create dedicated Parameter Contexts for versioned flows when importing
Posted by "exceptionfactory (via GitHub)" <gi...@apache.org>.
exceptionfactory commented on code in PR #7401:
URL: https://github.com/apache/nifi/pull/7401#discussion_r1245360645
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/test/java/org/apache/nifi/web/util/ParameterContextNameCollusionResolverTest.java:
##########
@@ -0,0 +1,89 @@
+/*
+ * 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.apache.nifi.web.api.dto.ParameterContextDTO;
+import org.apache.nifi.web.api.entity.ParameterContextEntity;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
+import org.mockito.Mockito;
+
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.function.Supplier;
+import java.util.stream.Stream;
+
+
+class ParameterContextNameCollusionResolverTest {
Review Comment:
Spelling correction:
```suggestion
class ParameterContextNameCollisionResolverTest {
```
--
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
[GitHub] [nifi] exceptionfactory closed pull request #7401: NIFI-11706 Add capability to create dedicated Parameter Contexts for versioned flows when importing
Posted by "exceptionfactory (via GitHub)" <gi...@apache.org>.
exceptionfactory closed pull request #7401: NIFI-11706 Add capability to create dedicated Parameter Contexts for versioned flows when importing
URL: https://github.com/apache/nifi/pull/7401
--
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
[GitHub] [nifi] exceptionfactory commented on a diff in pull request #7401: NIFI-11706 Add capability to create dedicated Parameter Contexts for versioned flows when importing
Posted by "exceptionfactory (via GitHub)" <gi...@apache.org>.
exceptionfactory commented on code in PR #7401:
URL: https://github.com/apache/nifi/pull/7401#discussion_r1245594809
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-ui/src/main/webapp/WEB-INF/partials/canvas/import-flow-version-dialog.jsp:
##########
@@ -17,6 +17,11 @@
<%@ page contentType="text/html" pageEncoding="UTF-8" session="false" %>
<div id="import-flow-version-dialog" layout="column" class="hidden large-dialog">
<div class="dialog-content">
+ <div class="setting keep-parameter-context">
Review Comment:
On the further review, placing this checkbox at the top of the dialog does not seem like the best approach. As this is an optional feature that may not be commonly used, recommend moving this checkbox to the end of the dialog window.
--
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
[GitHub] [nifi] exceptionfactory commented on a diff in pull request #7401: NIFI-11706 Add capability to create dedicated Parameter Contexts for versioned flows when importing
Posted by "exceptionfactory (via GitHub)" <gi...@apache.org>.
exceptionfactory commented on code in PR #7401:
URL: https://github.com/apache/nifi/pull/7401#discussion_r1245364933
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-ui/src/main/webapp/WEB-INF/partials/canvas/import-flow-version-dialog.jsp:
##########
@@ -17,6 +17,11 @@
<%@ page contentType="text/html" pageEncoding="UTF-8" session="false" %>
<div id="import-flow-version-dialog" layout="column" class="hidden large-dialog">
<div class="dialog-content">
+ <div class="setting keep-parameter-context">
+ <div id="keepExistingParameterContext" class="nf-checkbox checkbox-checked"></div>
+ <div class="nf-checkbox-label">Keep Existing Parameter Contexts</div>
+ <div class="fa fa-question-circle" alt="Info" title="When unchecked, only directly associated Parameter Contexts will be copied, inherited ones with no direct assignment on a Process Group are not subject of this."></div>
Review Comment:
Recommend adjusting the wording:
```suggestion
<div class="fa fa-question-circle" alt="Info" title="When not selected, only directly associated Parameter Contexts will be copied, inherited Contexts with no direct assignment to a Process Group are ignored"></div>
```
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/util/ParameterContextNameCollusionResolver.java:
##########
@@ -0,0 +1,76 @@
+/*
+ * 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.apache.nifi.web.api.dto.ParameterContextDTO;
+import org.apache.nifi.web.api.entity.ParameterContextEntity;
+
+import java.util.Collection;
+import java.util.Set;
+import java.util.function.Supplier;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+import java.util.stream.Collectors;
+
+class ParameterContextNameCollusionResolver {
+ private static final String LINEAGE_FORMAT = "^(?<name>.+?)( \\((?<index>[0-9]+)\\))?$";
+ private static final Pattern LINEAGE_PATTERN = Pattern.compile(LINEAGE_FORMAT);
+
+ ParameterContextNameCollusionResolver(final Supplier<Collection<ParameterContextEntity>> parameterContextSource) {
+ this.parameterContextSource = parameterContextSource;
+ }
+
+ private final Supplier<Collection<ParameterContextEntity>> parameterContextSource;
+
+ public String resolveNameCollusion(final String originalParameterContextName) {
+ final Matcher lineageMatcher = LINEAGE_PATTERN.matcher(originalParameterContextName);
+
+ if (!lineageMatcher.matches()) {
+ throw new IllegalArgumentException("Existing Parameter Context name \"(" + originalParameterContextName + "\") cannot be processed");
+ }
+
+ final String lineName = lineageMatcher.group("name");
+ final String originalIndex = lineageMatcher.group("index");
Review Comment:
Recommend defining static variables for `name` and `index` and reusing through this method.
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/util/ParameterContextNameCollusionResolver.java:
##########
@@ -0,0 +1,76 @@
+/*
+ * 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.apache.nifi.web.api.dto.ParameterContextDTO;
+import org.apache.nifi.web.api.entity.ParameterContextEntity;
+
+import java.util.Collection;
+import java.util.Set;
+import java.util.function.Supplier;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+import java.util.stream.Collectors;
+
+class ParameterContextNameCollusionResolver {
Review Comment:
`Collusion` needs to be replaced with `Collision` across all references.
```suggestion
class ParameterContextNameCollisionResolver {
```
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-ui/src/main/webapp/js/nf/canvas/nf-flow-version.js:
##########
@@ -1045,7 +1048,8 @@
return $.ajax({
type: 'POST',
data: JSON.stringify(processGroupEntity),
- url: '../nifi-api/process-groups/' + encodeURIComponent(nfCanvasUtils.getGroupId()) + '/process-groups',
+ url: '../nifi-api/process-groups/' + encodeURIComponent(nfCanvasUtils.getGroupId()) + '/process-groups?'
+ + $.param({'keepExistingParameterContext' : $('#keepExistingParameterContext').hasClass('checkbox-checked')}) ,
Review Comment:
Recommend assigning a value for checked or unchecked to make this easier to read.
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/util/ParameterContextNameCollusionResolver.java:
##########
@@ -0,0 +1,76 @@
+/*
+ * 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.apache.nifi.web.api.dto.ParameterContextDTO;
+import org.apache.nifi.web.api.entity.ParameterContextEntity;
+
+import java.util.Collection;
+import java.util.Set;
+import java.util.function.Supplier;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+import java.util.stream.Collectors;
+
+class ParameterContextNameCollusionResolver {
+ private static final String LINEAGE_FORMAT = "^(?<name>.+?)( \\((?<index>[0-9]+)\\))?$";
+ private static final Pattern LINEAGE_PATTERN = Pattern.compile(LINEAGE_FORMAT);
+
+ ParameterContextNameCollusionResolver(final Supplier<Collection<ParameterContextEntity>> parameterContextSource) {
+ this.parameterContextSource = parameterContextSource;
+ }
+
+ private final Supplier<Collection<ParameterContextEntity>> parameterContextSource;
+
+ public String resolveNameCollusion(final String originalParameterContextName) {
+ final Matcher lineageMatcher = LINEAGE_PATTERN.matcher(originalParameterContextName);
+
+ if (!lineageMatcher.matches()) {
+ throw new IllegalArgumentException("Existing Parameter Context name \"(" + originalParameterContextName + "\") cannot be processed");
+ }
+
+ final String lineName = lineageMatcher.group("name");
+ final String originalIndex = lineageMatcher.group("index");
+
+ // Candidates cannot be cached because new context might be added between calls
+ final Set<ParameterContextDTO> candidates = parameterContextSource.get()
+ .stream()
+ .map(pc -> pc.getComponent())
+ .filter(dto -> dto.getName().startsWith(lineName))
+ .collect(Collectors.toSet());
+
+ int biggestIndex = (originalIndex == null) ? 0 : Integer.valueOf(originalIndex);
+
+ for (final ParameterContextDTO candidate : candidates) {
+ final Matcher matcher = LINEAGE_PATTERN.matcher(candidate.getName());
+
+ if (matcher.matches() && lineName.equals(matcher.group("name"))) {
+ final String indexGroup = matcher.group("index");
+
+ if (indexGroup != null) {
+ int biggestIndexCandidate = Integer.valueOf(indexGroup);
+
+ if (biggestIndexCandidate > biggestIndex) {
+ biggestIndex = biggestIndexCandidate;
+ }
+ }
+ }
+ }
+
+ return new StringBuilder(lineName).append(" (").append(biggestIndex + 1).append(')').toString();
Review Comment:
Recommend replacing `StringBuilder` with `String.format()` for better readability.
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/api/ProcessGroupResource.java:
##########
@@ -1969,8 +1970,12 @@ public Response createProcessGroup(
@ApiParam(
value = "The process group configuration details.",
required = true
- ) final ProcessGroupEntity requestProcessGroupEntity) {
-
+ )
+ final ProcessGroupEntity requestProcessGroupEntity,
+ @QueryParam("keepExistingParameterContext")
+ @DefaultValue("true")
+ final boolean keepExistingParameterContext
Review Comment:
In terms of supporting potential API evolution, what do you think of making this an `enum` named `ParameterContextHandlingStrategy`? The default value could be `KEEP_EXISTING`, and the alternative could be `REPLACE` or similar? That would provide more flexibility and also help clarifying the options.
--
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
[GitHub] [nifi] exceptionfactory commented on a diff in pull request #7401: NIFI-11706 Add capability to create dedicated Parameter Contexts for versioned flows when importing
Posted by "exceptionfactory (via GitHub)" <gi...@apache.org>.
exceptionfactory commented on code in PR #7401:
URL: https://github.com/apache/nifi/pull/7401#discussion_r1245593903
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/api/ProcessGroupResource.java:
##########
@@ -2024,7 +2029,13 @@ public Response createProcessGroup(
}
}
- // Step 4: Resolve Bundle info
+ // Step 4: Replace parameter contexts if necessary
+ if (!keepExistingParameterContext) {
+ final ParameterContextReplacementUtil parameterContextReplacementUtil = ParameterContextReplacementUtil.getInstance(serviceFacade);
Review Comment:
This approach to creating an instance of ParameterContextReplacementUtil seems contrary to other conventions. It seems like it would be better to make the ParameterContext Replacement operations into an interface that could be injected. As it is, coupling the entire Service Facade to the Parameter Context Replacement instantiation is problematic.
--
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