You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by GitBox <gi...@apache.org> on 2022/11/11 14:36:22 UTC
[GitHub] [nifi] sedadgn opened a new pull request, #6655: NIFI-10787 Cannot commit flows to nifi registry after updating our nifi release to 1.18.0
sedadgn opened a new pull request, #6655:
URL: https://github.com/apache/nifi/pull/6655
NIFI-10787 NifiRegistryFlowRegistryClient defines the PropertyDescriptor PROPERTY_URL name as small case "url". The map bases on the name property of the PropertyDescriptor object. Here searching with uppercase value of "URL" causes the map lookup to fail and cause a NPE later on. Therefore, it is changed as "url"
<!-- 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. -->
# Summary
[NIFI-10787](https://issues.apache.org/jira/browse/NIFI-10787)
# Tracking
Please complete the following tracking steps prior to pull request creation.
### Issue Tracking
- [x] [Apache NiFi Jira](https://issues.apache.org/jira/browse/NIFI) issue created
### Pull Request Tracking
- [x] Pull Request title starts with Apache NiFi Jira issue number, such as `NIFI-00000`
- [x] Pull Request commit message starts with Apache NiFi Jira issue number, as such `NIFI-00000`
### Pull Request Formatting
- [x] Pull Request based on current revision of the `main` branch
- [x] 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
- [x] Build completed using `mvn clean install -P contrib-check`
- [ ] JDK 8
- [ ] JDK 11
- [ ] JDK 17
### Licensing
- [ ] New dependencies are compatible with the [Apache License 2.0](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] thenatog commented on a diff in pull request #6655: NIFI-10787 Cannot commit flows to nifi registry after updating our nifi release to 1.18.0
Posted by GitBox <gi...@apache.org>.
thenatog commented on code in PR #6655:
URL: https://github.com/apache/nifi/pull/6655#discussion_r1023087484
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-components/src/main/java/org/apache/nifi/registry/flow/mapping/NiFiRegistryFlowMapper.java:
##########
@@ -193,7 +193,7 @@ public InstantiatedVersionedProcessGroup mapProcessGroup(final ProcessGroup grou
// This is specific for the {@code NifiRegistryFlowRegistryClient}, purely for backward compatibility
private String getRegistryUrl(final FlowRegistryClientNode registry) {
- return registry.getComponentType().equals("org.apache.nifi.registry.flow.NifiRegistryFlowRegistryClient") ? registry.getRawPropertyValue(registry.getPropertyDescriptor("URL")) : "";
+ return registry.getComponentType().equals("org.apache.nifi.registry.flow.NifiRegistryFlowRegistryClient") ? registry.getRawPropertyValue(registry.getPropertyDescriptor("url")) : "";
Review Comment:
Correct, it is not in the same package. I'm suggesting a member variable just at the top of this file to make it more clear what the "url" value is, with a meaningful name like REGISTRY_URL_DESCRIPTOR_NAME. It's not ideal but it is probably better than just using the plain string of "url".
--
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] thenatog commented on pull request #6655: NIFI-10787 Cannot commit flows to nifi registry after updating our nifi release to 1.18.0
Posted by GitBox <gi...@apache.org>.
thenatog commented on PR #6655:
URL: https://github.com/apache/nifi/pull/6655#issuecomment-1317461633
After speaking with exceptionfactory, I have updated this PR with a different way of doing the unit test which does not require pom 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
[GitHub] [nifi] thenatog commented on pull request #6655: NIFI-10787 Cannot commit flows to nifi registry after updating our nifi release to 1.18.0
Posted by GitBox <gi...@apache.org>.
thenatog commented on PR #6655:
URL: https://github.com/apache/nifi/pull/6655#issuecomment-1314090704
Thank you for your contribution, I will review the PR. I might also add some unit tests for the issue.
--
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] sedadgn commented on a diff in pull request #6655: NIFI-10787 Cannot commit flows to nifi registry after updating our nifi release to 1.18.0
Posted by GitBox <gi...@apache.org>.
sedadgn commented on code in PR #6655:
URL: https://github.com/apache/nifi/pull/6655#discussion_r1022588457
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-components/src/main/java/org/apache/nifi/registry/flow/mapping/NiFiRegistryFlowMapper.java:
##########
@@ -193,7 +193,7 @@ public InstantiatedVersionedProcessGroup mapProcessGroup(final ProcessGroup grou
// This is specific for the {@code NifiRegistryFlowRegistryClient}, purely for backward compatibility
private String getRegistryUrl(final FlowRegistryClientNode registry) {
- return registry.getComponentType().equals("org.apache.nifi.registry.flow.NifiRegistryFlowRegistryClient") ? registry.getRawPropertyValue(registry.getPropertyDescriptor("URL")) : "";
+ return registry.getComponentType().equals("org.apache.nifi.registry.flow.NifiRegistryFlowRegistryClient") ? registry.getRawPropertyValue(registry.getPropertyDescriptor("url")) : "";
Review Comment:
Hello @thenatog ,
Thank you for your review. I thought add a constant for url but they are not in the same package. Where should i add it? Can you help me?
--
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] sedadgn commented on a diff in pull request #6655: NIFI-10787 Cannot commit flows to nifi registry after updating our nifi release to 1.18.0
Posted by GitBox <gi...@apache.org>.
sedadgn commented on code in PR #6655:
URL: https://github.com/apache/nifi/pull/6655#discussion_r1024192634
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-components/src/main/java/org/apache/nifi/registry/flow/mapping/NiFiRegistryFlowMapper.java:
##########
@@ -193,7 +193,7 @@ public InstantiatedVersionedProcessGroup mapProcessGroup(final ProcessGroup grou
// This is specific for the {@code NifiRegistryFlowRegistryClient}, purely for backward compatibility
private String getRegistryUrl(final FlowRegistryClientNode registry) {
- return registry.getComponentType().equals("org.apache.nifi.registry.flow.NifiRegistryFlowRegistryClient") ? registry.getRawPropertyValue(registry.getPropertyDescriptor("URL")) : "";
+ return registry.getComponentType().equals("org.apache.nifi.registry.flow.NifiRegistryFlowRegistryClient") ? registry.getRawPropertyValue(registry.getPropertyDescriptor("url")) : "";
Review Comment:
Hello, I added new constant in this 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
[GitHub] [nifi] thenatog commented on pull request #6655: NIFI-10787 Cannot commit flows to nifi registry after updating our nifi release to 1.18.0
Posted by GitBox <gi...@apache.org>.
thenatog commented on PR #6655:
URL: https://github.com/apache/nifi/pull/6655#issuecomment-1317800133
+1 will merge
--
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] thenatog closed pull request #6655: NIFI-10787 Cannot commit flows to nifi registry after updating our nifi release to 1.18.0
Posted by GitBox <gi...@apache.org>.
thenatog closed pull request #6655: NIFI-10787 Cannot commit flows to nifi registry after updating our nifi release to 1.18.0
URL: https://github.com/apache/nifi/pull/6655
--
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] thenatog commented on pull request #6655: NIFI-10787 Cannot commit flows to nifi registry after updating our nifi release to 1.18.0
Posted by GitBox <gi...@apache.org>.
thenatog commented on PR #6655:
URL: https://github.com/apache/nifi/pull/6655#issuecomment-1316269365
@exceptionfactory Any advice on the above test? It seemed like when referencing NifiRegistryFlowRegistryClient.PROPERTY_URL I needed to include nifi-ssl-context-service-api in the framework-core pom which was a bit strange. Not sure if we want to include both of these changes to the pom for this test change but it was a relatively simple way to add a test for this change otherwise.
--
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] thenatog commented on a diff in pull request #6655: NIFI-10787 Cannot commit flows to nifi registry after updating our nifi release to 1.18.0
Posted by GitBox <gi...@apache.org>.
thenatog commented on code in PR #6655:
URL: https://github.com/apache/nifi/pull/6655#discussion_r1022327567
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-components/src/main/java/org/apache/nifi/registry/flow/mapping/NiFiRegistryFlowMapper.java:
##########
@@ -193,7 +193,7 @@ public InstantiatedVersionedProcessGroup mapProcessGroup(final ProcessGroup grou
// This is specific for the {@code NifiRegistryFlowRegistryClient}, purely for backward compatibility
private String getRegistryUrl(final FlowRegistryClientNode registry) {
- return registry.getComponentType().equals("org.apache.nifi.registry.flow.NifiRegistryFlowRegistryClient") ? registry.getRawPropertyValue(registry.getPropertyDescriptor("URL")) : "";
+ return registry.getComponentType().equals("org.apache.nifi.registry.flow.NifiRegistryFlowRegistryClient") ? registry.getRawPropertyValue(registry.getPropertyDescriptor("url")) : "";
Review Comment:
Suggest adding a static final string variable to define this, eg `private static final String REGISTRY_URL_DESCRIPTOR_NAME = "url";`
--
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] thenatog commented on a diff in pull request #6655: NIFI-10787 Cannot commit flows to nifi registry after updating our nifi release to 1.18.0
Posted by GitBox <gi...@apache.org>.
thenatog commented on code in PR #6655:
URL: https://github.com/apache/nifi/pull/6655#discussion_r1023087484
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-components/src/main/java/org/apache/nifi/registry/flow/mapping/NiFiRegistryFlowMapper.java:
##########
@@ -193,7 +193,7 @@ public InstantiatedVersionedProcessGroup mapProcessGroup(final ProcessGroup grou
// This is specific for the {@code NifiRegistryFlowRegistryClient}, purely for backward compatibility
private String getRegistryUrl(final FlowRegistryClientNode registry) {
- return registry.getComponentType().equals("org.apache.nifi.registry.flow.NifiRegistryFlowRegistryClient") ? registry.getRawPropertyValue(registry.getPropertyDescriptor("URL")) : "";
+ return registry.getComponentType().equals("org.apache.nifi.registry.flow.NifiRegistryFlowRegistryClient") ? registry.getRawPropertyValue(registry.getPropertyDescriptor("url")) : "";
Review Comment:
Correct, it is not in the same package, so I'm suggesting a member variable just at the top of this file to make it more clear what the "url" value is, with a meaningful name like REGISTRY_URL_DESCRIPTOR_NAME. It's not ideal but it is probably better than just using the plain string of "url".
--
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] thenatog commented on pull request #6655: NIFI-10787 Cannot commit flows to nifi registry after updating our nifi release to 1.18.0
Posted by GitBox <gi...@apache.org>.
thenatog commented on PR #6655:
URL: https://github.com/apache/nifi/pull/6655#issuecomment-1315671256
I would also like to suggest we add a unit test for this change. I developed a relatively simple change to an existing test which should cover the change in NiFiRegistryFlowMapperTest.java.
@Test
public void testMapVersionedProcessGroupsExcludingVersionedDescendants() {
// prepare a versioned process group with a nested versioned process group, each with 1 processor
final ProcessGroup innerProcessGroup =
prepareProcessGroup(1,false, false, false,
false, false,null,
false, true, Collections.emptyList());
final ProcessGroup processGroup =
prepareProcessGroup(1,false,false, false,
false, false, null,
false, true, Collections.singletonList(innerProcessGroup));
final List<ProcessGroup> allProcessGroups = Collections.singletonList(innerProcessGroup);
when(processGroup.findAllProcessGroups()).thenReturn(allProcessGroups);
// perform the mapping, excluding descendant versioned flows
final InstantiatedVersionedProcessGroup versionedProcessGroup =
flowMapper.mapProcessGroup(processGroup, controllerServiceProvider, flowManager,
false);
final VersionedProcessGroup innerVersionedProcessGroup =
versionedProcessGroup.getProcessGroups().iterator().next();
// ensure the Registry URL has been set correctly in the flowManager
assert(StringUtils.isNotEmpty(innerVersionedProcessGroup.getVersionedFlowCoordinates().getStorageLocation()));
// verify root versioned process group contents only
verifyVersionedProcessGroup(processGroup, versionedProcessGroup,false,false);
// verify versioned descendant is present with VersionControlInfo only
verifyVersionedProcessGroup(innerProcessGroup, innerVersionedProcessGroup,true,false);
}
with two additional mocks in the setup() method:
```
Mockito.when(flowRegistry.getPropertyDescriptor(NifiRegistryFlowRegistryClient.PROPERTY_URL.getName())).thenReturn(NifiRegistryFlowRegistryClient.PROPERTY_URL);
Mockito.when(flowRegistry.getRawPropertyValue(NifiRegistryFlowRegistryClient.PROPERTY_URL)).thenReturn("http://127.0.0.1:18080");
```
However I think this test change needed adding the below to the nifi-framework-core/pom.xml which I wasn't confident about:
```
<dependency>
<groupId>org.apache.nifi</groupId>
<artifactId>nifi-flow-registry-client-services</artifactId>
<version>1.19.0-SNAPSHOT</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.apache.nifi</groupId>
<artifactId>nifi-ssl-context-service-api</artifactId>
<scope>test</scope>
</dependency>
```
--
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