Re: [PR] NIFI-12918 Fix Stateless NullPointerException on versioned sub-process groups - main branch [nifi]

[via GitHub]

exceptionfactory closed pull request #8536: NIFI-12918 Fix Stateless 
NullPointerException on versioned sub-process groups - main branch
URL: https://github.com/apache/nifi/pull/8536


-- 
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-unsubscr...@nifi.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] NIFI-12918 Fix Stateless NullPointerException on versioned sub-process groups - main branch [nifi]

[via GitHub]

slambrose commented on PR #8536:
URL: https://github.com/apache/nifi/pull/8536#issuecomment-2040356905

   > Thanks for the latest updates @slambrose, this is looking closer to 
completion.
   > 
   > Changing the `InMemoryFlowRegistry` to return `true` makes sense, as it is 
only used in Stateless operation, and it is the only Flow Registry Client 
available when running. It would be helpful to add a basic unit test class for 
`InMemoryFlowRegistry`, just to track that expected behavior.
   > 
   > The other consideration is the base Registry URL. Although the current 
`getBaseRegistryUrl()` method works when a Registry server is accessible 
directly, if there is a reverse proxy in front that has an additional context 
path, rebuilding the URL with just the host and port presents a problem. For 
example:
   > 
   > ```
   > https://nifi-registry.local/context-path/nifi-registry-api
   > ```
   > 
   > The method would return the following:
   > 
   > ```
   > https://nifi-registry.local
   > ```
   > 
   > The Registry Client would then attempt to append `/nifi-registry-api` as 
the context path from the root, which would result in failures.
   > 
   > In light of the fact that the `storageLocation` includes the full path, it 
seems like a regular expression match would address all scenarios. Something 
along the lines of the following pattern:
   > 
   > ```
   > ^(https?://.+?/nifi-registry-api).*$
   > ```
   > 
   > With that pattern, the first capturing group can then be used at the 
registry URL.
   
   Additional Unit tests will be address in future work for 
InMemoryFlowRegistry class. Regex change has been added. Ready for re-review 
@exceptionfactory 


-- 
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-unsubscr...@nifi.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] NIFI-12918 Fix Stateless NullPointerException on versioned sub-process groups - main branch [nifi]

[via GitHub]

slambrose commented on PR #8536:
URL: https://github.com/apache/nifi/pull/8536#issuecomment-2037713285

   @exceptionfactory Okay, this should be good now.


-- 
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-unsubscr...@nifi.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] NIFI-12918 Fix Stateless NullPointerException on versioned sub-process groups - main branch [nifi]

[via GitHub]

slambrose commented on code in PR #8536:
URL: https://github.com/apache/nifi/pull/8536#discussion_r1551952629


##
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-components/src/main/java/org/apache/nifi/flow/synchronization/StandardVersionedComponentSynchronizer.java:
##
@@ -509,6 +509,8 @@ private String determineRegistryId(final 
VersionedFlowCoordinates coordinates) {
 } else {
 return explicitRegistryId;
 }
+} else {
+explicitRegistryId = "1";

Review Comment:
   Okay, so a better way to fix this going with your guidance on that 
storageLocation check is to add a check for the in memory registry client 
(which is used 100% of the time for stateless). Code is ready for re-review.



-- 
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-unsubscr...@nifi.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] NIFI-12918 Fix Stateless NullPointerException on versioned sub-process groups - main branch [nifi]

[via GitHub]

slambrose commented on code in PR #8536:
URL: https://github.com/apache/nifi/pull/8536#discussion_r1551746576


##
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-components/src/main/java/org/apache/nifi/flow/synchronization/StandardVersionedComponentSynchronizer.java:
##
@@ -509,6 +509,8 @@ private String determineRegistryId(final 
VersionedFlowCoordinates coordinates) {
 } else {
 return explicitRegistryId;
 }
+} else {
+explicitRegistryId = "1";

Review Comment:
   @exceptionfactory - So your most recent comment on my PR led me down an 
interesting path. I see what you mean about looking into the subsequent call 
using the storageLocation. What's interesting about that is Stateless flows 
always ever use the InMemoryFlowRegistry.class , and then therefore that call:
   
   `try {
   locationApplicable = 
flowRegistryClientNode.isStorageLocationApplicable(location);
   } catch (final Exception e) {
   LOG.error("Unable to determine if {} is an applicable Flow Registry 
Client for storage location {}", flowRegistryClientNode, location, e);
   continue;
   }
   
   if (locationApplicable) {
   LOG.debug("Found Flow Registry Client {} that is applicable for storage 
location {}", flowRegistryClientNode, location);
   return flowRegistryClientNode.getIdentifier();
   }`
   
   
   So for stateless, the locationApplication is always false 
InMemoryFlowRegistry:
   
   `@Override
   public boolean isStorageLocationApplicable(final 
FlowRegistryClientConfigurationContext context, final String location) {
   return false;
   }`
   
   So, basically the only way to fix stateless is to either keep the "1" for 
registryId fix, or try to set this isStorageLocationApplication method for the 
in memory registry to truealthough I'm sure that will probably break other 
things. Do you have any opinion on that?



-- 
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-unsubscr...@nifi.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] NIFI-12918 Fix Stateless NullPointerException on versioned sub-process groups - main branch [nifi]

[via GitHub]

slambrose commented on code in PR #8536:
URL: https://github.com/apache/nifi/pull/8536#discussion_r1551746576


##
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-components/src/main/java/org/apache/nifi/flow/synchronization/StandardVersionedComponentSynchronizer.java:
##
@@ -509,6 +509,8 @@ private String determineRegistryId(final 
VersionedFlowCoordinates coordinates) {
 } else {
 return explicitRegistryId;
 }
+} else {
+explicitRegistryId = "1";

Review Comment:
   @exceptionfactory - So your most recent comment on my PR led me down an 
interesting path. I see what you mean about looking into the subsequent call 
using the storageLocation. What's interesting about that is Stateless flows 
always ever use the InMemoryFlowRegistry.class , and then therefore that call:
   `try {
   locationApplicable = 
flowRegistryClientNode.isStorageLocationApplicable(location);
   } catch (final Exception e) {
   LOG.error("Unable to determine if {} is an applicable Flow Registry 
Client for storage location {}", flowRegistryClientNode, location, e);
   continue;
   }
   
   if (locationApplicable) {
   LOG.debug("Found Flow Registry Client {} that is applicable for storage 
location {}", flowRegistryClientNode, location);
   return flowRegistryClientNode.getIdentifier();
   }`
   
   
   So for stateless, the locationApplication is always false 
InMemoryFlowRegistry:
   
   `@Override
   public boolean isStorageLocationApplicable(final 
FlowRegistryClientConfigurationContext context, final String location) {
   return false;
   }`
   
   So, basically the only way to fix stateless is to either keep the "1" for 
registryId fix, or try to set this isStorageLocationApplication method for the 
in memory registry to truealthough I'm sure that will probably break other 
things. Do you have any opinion on that?



-- 
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-unsubscr...@nifi.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] NIFI-12918 Fix Stateless NullPointerException on versioned sub-process groups - main branch [nifi]

[via GitHub]

slambrose commented on code in PR #8536:
URL: https://github.com/apache/nifi/pull/8536#discussion_r1551746576


##
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-components/src/main/java/org/apache/nifi/flow/synchronization/StandardVersionedComponentSynchronizer.java:
##
@@ -509,6 +509,8 @@ private String determineRegistryId(final 
VersionedFlowCoordinates coordinates) {
 } else {
 return explicitRegistryId;
 }
+} else {
+explicitRegistryId = "1";

Review Comment:
   So your most recent comment on my PR led me down an interesting path, haha. 
I see what you mean about looking into the subsequent call using the 
storageLocation. What's interesting about that is Stateless flows always ever 
use the InMemoryFlowRegistry.class , and then therefore that call:
   `
   try {
   locationApplicable = 
flowRegistryClientNode.isStorageLocationApplicable(location);
   } catch (final Exception e) {
   LOG.error("Unable to determine if {} is an applicable Flow Registry 
Client for storage location {}", flowRegistryClientNode, location, e);
   continue;
   }
   
   if (locationApplicable) {
   LOG.debug("Found Flow Registry Client {} that is applicable for storage 
location {}", flowRegistryClientNode, location);
   return flowRegistryClientNode.getIdentifier();
   }
   `
   So for stateless, the locationApplication is always false 
InMemoryFlowRegistry:
   `
   @Override
   public boolean isStorageLocationApplicable(final 
FlowRegistryClientConfigurationContext context, final String location) {
   return false;
   }
   `
   
   So, basically the only way to fix stateless is to either keep the "1" for 
registryId fix, or try to set this isStorageLocationApplication method for the 
in memory registry to truealthough I'm sure that will probably break other 
things. Do you have any opinion on that?



-- 
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-unsubscr...@nifi.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] NIFI-12918 Fix Stateless NullPointerException on versioned sub-process groups - main branch [nifi]

[via GitHub]

slambrose commented on code in PR #8536:
URL: https://github.com/apache/nifi/pull/8536#discussion_r1551746576


##
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-components/src/main/java/org/apache/nifi/flow/synchronization/StandardVersionedComponentSynchronizer.java:
##
@@ -509,6 +509,8 @@ private String determineRegistryId(final 
VersionedFlowCoordinates coordinates) {
 } else {
 return explicitRegistryId;
 }
+} else {
+explicitRegistryId = "1";

Review Comment:
   So your most recent comment on my PR led me down an interesting path, haha. 
I see what you mean about looking into the subsequent call using the 
storageLocation. What's interesting about that is Stateless flows always ever 
use the InMemoryFlowRegistry.class , and then therefore that call:
   try {
   locationApplicable = 
flowRegistryClientNode.isStorageLocationApplicable(location);
   } catch (final Exception e) {
   LOG.error("Unable to determine if {} is an applicable Flow Registry 
Client for storage location {}", flowRegistryClientNode, location, e);
   continue;
   }
   
   if (locationApplicable) {
   LOG.debug("Found Flow Registry Client {} that is applicable for storage 
location {}", flowRegistryClientNode, location);
   return flowRegistryClientNode.getIdentifier();
   }
   So for stateless, the locationApplication is always false 
InMemoryFlowRegistry:
   @Override
   public boolean isStorageLocationApplicable(final 
FlowRegistryClientConfigurationContext context, final String location) {
   return false;
   }
   So, basically the only way to fix stateless is to either keep the "1" for 
registryId fix, or try to set this isStorageLocationApplication method for the 
in memory registry to truealthough I'm sure that will probably break other 
things. Do you have any opinion on that?



-- 
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-unsubscr...@nifi.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] NIFI-12918 Fix Stateless NullPointerException on versioned sub-process groups - main branch [nifi]

[via GitHub]

exceptionfactory commented on code in PR #8536:
URL: https://github.com/apache/nifi/pull/8536#discussion_r1550854635


##
nifi-stateless/nifi-stateless-bundle/nifi-stateless-engine/src/test/java/org/apache/nifi/stateless/core/TestRegistryUtil.java:
##
@@ -0,0 +1,92 @@
+/*
+ * 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.stateless.core;
+
+import org.apache.nifi.flow.VersionedFlowCoordinates;
+import org.apache.nifi.flow.VersionedProcessGroup;
+import org.apache.nifi.registry.client.FlowSnapshotClient;
+import org.apache.nifi.registry.client.NiFiRegistryClient;
+import org.apache.nifi.registry.client.NiFiRegistryException;
+import org.apache.nifi.registry.flow.VersionedFlowSnapshot;
+import org.junit.jupiter.api.Test;
+
+import java.io.IOException;
+import java.util.Collections;
+import java.util.UUID;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+public class TestRegistryUtil {
+private static final String registryUrl = "http://localhost:18080;;
+private static final String rootBucketId = UUID.randomUUID().toString();
+private static final String rootFlowId = UUID.randomUUID().toString();
+private static final int rootVersion = 1;
+
+private static final String childBucketId = UUID.randomUUID().toString();
+private static final String childFlowId = UUID.randomUUID().toString();
+private static final int childVersion = 1;
+
+@Test
+public void testRegistryUrlCreation() throws NiFiRegistryException, 
IOException {
+NiFiRegistryClient registryClient = mock(NiFiRegistryClient.class);
+FlowSnapshotClient flowSnapshotClient = mock(FlowSnapshotClient.class);
+
+RegistryUtil registryUtil = new RegistryUtil(registryClient, 
registryUrl, null);
+
+
when(registryClient.getFlowSnapshotClient()).thenReturn(flowSnapshotClient);
+when(flowSnapshotClient.get(rootBucketId, rootFlowId, 
rootVersion)).thenAnswer(
+invocation -> buildVfs());
+when(flowSnapshotClient.get(childBucketId, childFlowId, 
childVersion)).thenAnswer(
+invocation -> buildVfs());
+
+VersionedFlowSnapshot snapshot = 
registryUtil.getFlowContents(rootBucketId, rootFlowId, rootVersion, true, null);
+VersionedFlowCoordinates coordinates = 
snapshot.getFlowContents().getVersionedFlowCoordinates();
+
+String formattedUrl = 
registryUtil.getBaseRegistryUrl(coordinates.getStorageLocation());
+assertEquals(formattedUrl, registryUrl);
+}
+
+private VersionedFlowSnapshot buildVfs(){
+VersionedFlowSnapshot vfs = new VersionedFlowSnapshot();
+VersionedProcessGroup vpg1 = new VersionedProcessGroup();
+VersionedProcessGroup vpg2 = new VersionedProcessGroup();
+VersionedFlowCoordinates vfc1 = new VersionedFlowCoordinates();
+VersionedFlowCoordinates vfc2 = new VersionedFlowCoordinates();
+
+String storageLocation1 = 
String.format(registryUrl+"/nifi-registry-api/buckets/%s/flows/%s/versions/%s", 
rootBucketId, rootFlowId, rootVersion);

Review Comment:
   The `registryUrl` parameter should be moved to a format element:
   ```suggestion
   String storageLocation1 = 
String.format("%s/nifi-registry-api/buckets/%s/flows/%s/versions/%s", 
registryUrl, rootBucketId, rootFlowId, rootVersion);
   ```



##
nifi-stateless/nifi-stateless-bundle/nifi-stateless-engine/src/main/java/org/apache/nifi/stateless/core/RegistryUtil.java:
##
@@ -119,6 +126,10 @@ public VersionedFlowSnapshot getFlowContents(final String 
bucketId, final String
 return flowSnapshot;
 }
 
+public String getBaseRegistryUrl(String storageLocation) throws 
MalformedURLException {
+URL url = new URL(storageLocation);

Review Comment:
   Recommend using `URI.create()` instead of `new URL()` to avoid the checked 
`MalformedURLException`



##
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-components/src/main/java/org/apache/nifi/flow/synchronization/StandardVersionedComponentSynchronizer.java:
##
@@ -509,6 +509,8 @@ private String 

Re: [PR] NIFI-12918 Fix Stateless NullPointerException on versioned sub-process groups - main branch [nifi]

[via GitHub]

slambrose commented on PR #8536:
URL: https://github.com/apache/nifi/pull/8536#issuecomment-2034320750

   > Thanks for working on the unit tests @slambrose. The test for the 
synchronizer class looks like it exercises the method changes, but the 
`TestRegistryUtil` appears to be running against a mock of `RegistryUtil`, as 
opposed to the actual class, so it doesn't exercise the real method.
   
   Ready for re-review.


-- 
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-unsubscr...@nifi.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] NIFI-12918 Fix Stateless NullPointerException on versioned sub-process groups - main branch [nifi]

[via GitHub]

slambrose commented on code in PR #8536:
URL: https://github.com/apache/nifi/pull/8536#discussion_r1544671782


##
nifi-stateless/nifi-stateless-bundle/nifi-stateless-engine/src/test/java/org/apache/nifi/stateless/core/TestRegistryUtil.java:
##
@@ -0,0 +1,64 @@
+/*
+ * 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.stateless.core;
+
+import org.apache.nifi.flow.VersionedFlowCoordinates;
+import org.apache.nifi.flow.VersionedProcessGroup;
+import org.apache.nifi.registry.client.NiFiRegistryException;
+import org.apache.nifi.registry.flow.VersionedFlowSnapshot;
+import org.junit.jupiter.api.Test;
+
+import java.io.IOException;
+import java.net.URL;
+import java.util.UUID;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+public class TestRegistryUtil {
+
+private String registryUrl = "http://localhost:18080;;
+final String bucketId = UUID.randomUUID().toString();
+final String flowId = UUID.randomUUID().toString();
+final int version = 1;
+final String storageLocation = 
"http://localhost:18080/nifi-registry-api/buckets/"+bucketId+"/flows/"+flowId+"/versions/"+version;
+
+@Test
+public void testRegistryUrlCreation() throws NiFiRegistryException, 
IOException {
+RegistryUtil registryUtil = mock(RegistryUtil.class);
+when(registryUtil.getFlowContents(bucketId, flowId, version, true, 
null)).thenAnswer(
+invocation -> createVfs());

Review Comment:
   Okay, had to do a bunch of work to mock the clients, but it should be good 
for re-review. 



-- 
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-unsubscr...@nifi.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] NIFI-12918 Fix Stateless NullPointerException on versioned sub-process groups - main branch [nifi]

[via GitHub]

slambrose commented on code in PR #8536:
URL: https://github.com/apache/nifi/pull/8536#discussion_r1543361841


##
nifi-stateless/nifi-stateless-bundle/nifi-stateless-engine/src/test/java/org/apache/nifi/stateless/core/TestRegistryUtil.java:
##
@@ -0,0 +1,64 @@
+/*
+ * 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.stateless.core;
+
+import org.apache.nifi.flow.VersionedFlowCoordinates;
+import org.apache.nifi.flow.VersionedProcessGroup;
+import org.apache.nifi.registry.client.NiFiRegistryException;
+import org.apache.nifi.registry.flow.VersionedFlowSnapshot;
+import org.junit.jupiter.api.Test;
+
+import java.io.IOException;
+import java.net.URL;
+import java.util.UUID;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+public class TestRegistryUtil {
+
+private String registryUrl = "http://localhost:18080;;
+final String bucketId = UUID.randomUUID().toString();
+final String flowId = UUID.randomUUID().toString();
+final int version = 1;
+final String storageLocation = 
"http://localhost:18080/nifi-registry-api/buckets/"+bucketId+"/flows/"+flowId+"/versions/"+version;

Review Comment:
   Done



-- 
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-unsubscr...@nifi.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] NIFI-12918 Fix Stateless NullPointerException on versioned sub-process groups - main branch [nifi]

[via GitHub]

exceptionfactory commented on code in PR #8536:
URL: https://github.com/apache/nifi/pull/8536#discussion_r1543316571


##
nifi-stateless/nifi-stateless-bundle/nifi-stateless-engine/src/test/java/org/apache/nifi/stateless/core/TestRegistryUtil.java:
##
@@ -0,0 +1,64 @@
+/*
+ * 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.stateless.core;
+
+import org.apache.nifi.flow.VersionedFlowCoordinates;
+import org.apache.nifi.flow.VersionedProcessGroup;
+import org.apache.nifi.registry.client.NiFiRegistryException;
+import org.apache.nifi.registry.flow.VersionedFlowSnapshot;
+import org.junit.jupiter.api.Test;
+
+import java.io.IOException;
+import java.net.URL;
+import java.util.UUID;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+public class TestRegistryUtil {
+
+private String registryUrl = "http://localhost:18080;;
+final String bucketId = UUID.randomUUID().toString();
+final String flowId = UUID.randomUUID().toString();
+final int version = 1;
+final String storageLocation = 
"http://localhost:18080/nifi-registry-api/buckets/"+bucketId+"/flows/"+flowId+"/versions/"+version;

Review Comment:
   These values should be `private static` and string formatting should be used 
to build the `storageLocation` instead of concatenation for improved 
readability.



##
nifi-stateless/nifi-stateless-bundle/nifi-stateless-engine/src/test/java/org/apache/nifi/stateless/core/TestRegistryUtil.java:
##
@@ -0,0 +1,64 @@
+/*
+ * 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.stateless.core;
+
+import org.apache.nifi.flow.VersionedFlowCoordinates;
+import org.apache.nifi.flow.VersionedProcessGroup;
+import org.apache.nifi.registry.client.NiFiRegistryException;
+import org.apache.nifi.registry.flow.VersionedFlowSnapshot;
+import org.junit.jupiter.api.Test;
+
+import java.io.IOException;
+import java.net.URL;
+import java.util.UUID;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+public class TestRegistryUtil {
+
+private String registryUrl = "http://localhost:18080;;
+final String bucketId = UUID.randomUUID().toString();
+final String flowId = UUID.randomUUID().toString();
+final int version = 1;
+final String storageLocation = 
"http://localhost:18080/nifi-registry-api/buckets/"+bucketId+"/flows/"+flowId+"/versions/"+version;
+
+@Test
+public void testRegistryUrlCreation() throws NiFiRegistryException, 
IOException {
+RegistryUtil registryUtil = mock(RegistryUtil.class);
+when(registryUtil.getFlowContents(bucketId, flowId, version, true, 
null)).thenAnswer(
+invocation -> createVfs());

Review Comment:
   Mocking `RegistryUtil` does not exercise actual behavior. The mock should be 
removed and the actual class needs to be called.



-- 
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-unsubscr...@nifi.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] NIFI-12918 Fix Stateless NullPointerException on versioned sub-process groups - main branch [nifi]

[via GitHub]

slambrose commented on PR #8536:
URL: https://github.com/apache/nifi/pull/8536#issuecomment-2025707403

   Ready for re-review.


-- 
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-unsubscr...@nifi.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] NIFI-12918 Fix Stateless NullPointerException on versioned sub-process groups - main branch [nifi]

[via GitHub]

slambrose commented on PR #8536:
URL: https://github.com/apache/nifi/pull/8536#issuecomment-2023618547

   Thanks for taking another look :)
   
   So, the storageLocation change is still needed to fix the bug and process 
the flow in stateless. Otherwise, when it loops down to pull the sub-versioned 
process flow, it'll append the /{bucketId}/{flowId} parameters twice and make a 
bad call to the registry-api. The best way I came up to fix that was just to 
make sure to start with the base registry url and append the storage location 
parameters to always construct a valid registryUrl. I noticed the 1.x support 
branch was originally setting the registryUrl to coordinates.getRegistryUrl(), 
which was changed to coordinates.getStorageLocation() in main. This fix works 
for the 1.x branch as well.
   
   Do you have another value other than "1" you'd prefer me to set it to? FYI - 
it has to be a static value in order for all of the current tests to work.
   
   I can work on adding some unit tests for both of these fixes. I'll add a 
comment here when I commit them and are ready for review.
   
   


-- 
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-unsubscr...@nifi.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] NIFI-12918 Fix Stateless NullPointerException on versioned sub-process groups - main branch [nifi]

[via GitHub]

slambrose commented on PR #8536:
URL: https://github.com/apache/nifi/pull/8536#issuecomment-2022845523

   Okay, so after some back and forth here is my final conclusion to this bug:
   
   I've submitted two PRs 
   https://github.com/apache/nifi/pull/8572/checks <-- for support/1.x branch
   https://github.com/apache/nifi/pull/8536/checks <-- for main branch
   
   Here is what's happening. If you run a versioned stateless flow that has 
sub-versioned progress group, there is a NullPointerException thrown on the 
build method of StandardVersionControlInformation where there is a non-null 
requirement on "registryId". 
   
   Code has changed in main versus support/1.x, but the bug exists on both. 
What I've found is the JerseyClient calls that are made to map the response 
from the registry-api back to the higher level VersionedProcessGroup -> 
VersionedFlowCoordinates both return JSON where the VersionedFlowCoordinates > 
registryId is always null. Now in the main branch, stateless uses some of these 
new "synchronizer" classes, so the best place to insert a solution is to add an 
else on the null check where the code runs the "determineRegistryId" method and 
set the value to "1" for versioned flows that do not have a registryId 
associated to them. Doing this passes all of the integration tests and code 
checks and fixes the bug. Since this class does not exist in the 1.x support 
branch, my best solution is to just comment out the null check (this breaks 
integration tests in main, but not in 1.x). 
   
   I continued to look further at this idea of a "registryId" within the 
registry api code. The GET method on buckets/flow/version returns the same 
class that the stateless code uses to map into on the JerseyClient call. I then 
looked at the api code to POST new versioned flows, and it also expect the json 
as a parameter to match the same class. Well, since there is no "non-null" 
requirement on the registryId, versioned flows are stored in the database (or 
whichever storage adapter used) with this property as null. I did not check to 
see if the latest NiFi Registry UI is now setting this property, but making it 
non-null would break any older version of registry and not be backwards 
compatible. Therefore, the only solution at this point to fix stateless NiFi in 
its current state is to just set registryId to "1" if it is null. I believe 
this code is still probably prototype being worked and evolving, so this will 
be a temporary fix until those mandates are in place on a concept of "reg
 istryId". 


-- 
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-unsubscr...@nifi.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] NIFI-12918 Fix Stateless NullPointerException on versioned sub-process groups - main branch [nifi]

[via GitHub]

slambrose commented on PR #8536:
URL: https://github.com/apache/nifi/pull/8536#issuecomment-2021173210

   Okay, I'm trying one more solution. What I've found is that the main branch 
has new code where a "registryId" is implemented, but it is always null right 
now because the registry-api does not currently supply a "registryId" property 
value. The integration tests hard-code the creation of a registry UUID to 
simulate testing the bug that currently exists in 1.x and 2.0 (main) branches. 
This will put in a UUID placeholder and allow the integration tests to pass, 
while also fixing the current broken stateless flows in 1.x and 2.0. 


-- 
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-unsubscr...@nifi.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] NIFI-12918 Fix Stateless NullPointerException on versioned sub-process groups - main branch [nifi]

[via GitHub]

slambrose commented on PR #8536:
URL: https://github.com/apache/nifi/pull/8536#issuecomment-2017834831

   So, here is my conclusion on this. Unfortunately, I'm not sure I have the 
cycles to do the full re-write that this code probably requires. The main and 
1.x support branches currently do not work to process stateless flows that have 
sub-versioned flows. It's **broken** in current state. My two small changes fix 
both the main and 1.x support branches in terms of processing the flows (tested 
and confirmed); however, there are 9 integration tests that appear to be 
_trying_ to test this exact use case. Clearly, these these 9 tests were bad to 
begin with because they don't catch the bug at all. I'm not sure what actually 
changed in the code base such that these 9 tests fail in the main branch but 
not the 1.x support branch. I see many places where this concept of a 
"registryId" is not fully programmed out, so making a require non-null on it 
does not work. 
   I'm proposing an acceptance of these merge requests with the broken tests 
commented out to fix processing in production, with a continued ticket to 
investigate coding this out completely and fixing the integration tests at a 
later time. Otherwise, the product remains broken indefinitely.


-- 
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-unsubscr...@nifi.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] NIFI-12918 Fix Stateless NullPointerException on versioned sub-process groups - main branch [nifi]

[via GitHub]

slambrose commented on PR #8536:
URL: https://github.com/apache/nifi/pull/8536#issuecomment-2012625108

   Thanks, I see it now. Looks like there was an integration test that was 
added in 2.0 that isn't good. The test that's failing looks like it should have 
picked up the bug that I'm attempting to fix, but it does not. I'll add a 
change to the test class so that it doesn't break anymore.


-- 
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-unsubscr...@nifi.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] NIFI-12918 Fix Stateless NullPointerException on versioned sub-process groups - main branch [nifi]

[via GitHub]

exceptionfactory commented on PR #8536:
URL: https://github.com/apache/nifi/pull/8536#issuecomment-2012604594

   Just to highlight the most recent failures, the follow tests and methods are 
visible in the system test action logs. 
   
   ```
   Error:
ClusteredRegistryClientIT>RegistryClientIT.testChangeConnectionDestinationRemoveOldAndMoveGroup:163
 » NiFiClient Error starting version control: Failed to update Version Control 
Information for Process Group with ID 61655b6e-018e-1000--b050df1b.
   Error:
ClusteredRegistryClientIT>RegistryClientIT.testChangeVersionOnParentThatCascadesToChild:92
 » NiFiClient Error starting version control: Failed to update Version Control 
Information for Process Group with ID 616560a1-018e-1000--153b8e9e.
   Error:
ClusteredRegistryClientIT>RegistryClientIT.testChangeVersionWithPortMoveBetweenGroups:262
 » NiFiClient Error creating process group: An unexpected error has occurred. 
Please check the logs for additional details.
   Error:
ClusteredRegistryClientIT>RegistryClientIT.testControllerServiceUpdateWhileRunning:218
 » NiFiClient Error starting version control: Failed to update Version Control 
Information for Process Group with ID 616569b2-018e-1000--519c22d0.
   Error:
ClusteredRegistryClientIT>RegistryClientIT.testCopyPasteProcessGroupDoesNotDuplicateVersionedComponentId:382
 » NiFiClient Error starting version control: Failed to update Version Control 
Information for Process Group with ID 616563fc-018e-1000--f320df4a.
   Error:
ClusteredRegistryClientIT>RegistryClientIT.testCopyPasteProcessGroupUnderVersionControlMaintainsVersionedComponentId:432
 » NiFiClient Error starting version control: Failed to update Version Control 
Information for Process Group with ID 616555c6-018e-1000--f72dc814.
   Error:
ClusteredRegistryClientIT>RegistryClientIT.testRollbackOnFailure:309 » 
NiFiClient Error creating process group: An unexpected error has occurred. 
Please check the logs for additional details.
   Error:
ClusteredRegistryClientIT>RegistryClientIT.testStartVersionControlThenImport:332
 » NiFiClient Error starting version control: Failed to update Version Control 
Information for Process Group with ID 61656d21-018e-1000--a5788503.
   Error:
ClusteredRegistryClientIT>RegistryClientIT.testStartVersionControlThenModifyAndRevert:355
 » NiFiClient Error starting version control: Failed to update Version Control 
Information for Process Group with ID 616566d5-018e-1000--c48d8b9f.
   Error:
RegistryClientIT.testChangeConnectionDestinationRemoveOldAndMoveGroup:163 » 
NiFiClient Error starting version control: An unexpected error has occurred. 
Please check the logs for additional details.
   Error:RegistryClientIT.testChangeVersionOnParentThatCascadesToChild:92 » 
NiFiClient Error starting version control: An unexpected error has occurred. 
Please check the logs for additional details.
   Error:RegistryClientIT.testChangeVersionWithPortMoveBetweenGroups:262 » 
NiFiClient Error creating process group: An unexpected error has occurred. 
Please check the logs for additional details.
   Error:RegistryClientIT.testControllerServiceUpdateWhileRunning:218 » 
NiFiClient Error starting version control: An unexpected error has occurred. 
Please check the logs for additional details.
   Error:
RegistryClientIT.testCopyPasteProcessGroupDoesNotDuplicateVersionedComponentId:382
 » NiFiClient Error starting version control: An unexpected error has occurred. 
Please check the logs for additional details.
   Error:
RegistryClientIT.testCopyPasteProcessGroupUnderVersionControlMaintainsVersionedComponentId:432
 » NiFiClient Error starting version control: An unexpected error has occurred. 
Please check the logs for additional details.
   Error:RegistryClientIT.testRollbackOnFailure:309 » NiFiClient Error 
creating process group: An unexpected error has occurred. Please check the logs 
for additional details.
   Error:RegistryClientIT.testStartVersionControlThenImport:332 » 
NiFiClient Error starting version control: An unexpected error has occurred. 
Please check the logs for additional details.
   Error:RegistryClientIT.testStartVersionControlThenModifyAndRevert:355 » 
NiFiClient Error starting version control: An unexpected error has occurred. 
Please check the logs for additional details.
   Error:StatelessBasicsIT.testChangeFlowVersion:741 » NiFiClient Error 
starting version control: An unexpected error has occurred. Please check the 
logs for additional details.
   ```


-- 
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-unsubscr...@nifi.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] NIFI-12918 Fix Stateless NullPointerException on versioned sub-process groups - main branch [nifi]

[via GitHub]

joewitt commented on PR #8536:
URL: https://github.com/apache/nifi/pull/8536#issuecomment-2012478802

   @slambrose Yes.  https://github.com/apache/nifi/actions.  When I build 
locally they work fine and that link suggests they succeed quite often 
including recently.  They still aren't 100% bulletproof and as such do have 
failures at times and tremendous effort has gone into improving them.  But I 
interpret David's note to mean he looked at them and found it is likely related 
to this change.
   
   Fundamentally if your question "Are these system/integration tests important 
to verify for changes like this?"   The answer is yes they're very important as 
a safeguard to ensure changes at the framework level have some sanity checking.


-- 
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-unsubscr...@nifi.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] NIFI-12918 Fix Stateless NullPointerException on versioned sub-process groups - main branch [nifi]

[via GitHub]

slambrose commented on PR #8536:
URL: https://github.com/apache/nifi/pull/8536#issuecomment-2012464116

   Does the current main branch even pass the integration tests? When I build 
locally with the tests, they fail on the main branch.


-- 
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-unsubscr...@nifi.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] NIFI-12918 Fix Stateless NullPointerException on versioned sub-process groups - main branch [nifi]

[via GitHub]

dan-s1 commented on PR #8536:
URL: https://github.com/apache/nifi/pull/8536#issuecomment-2010182955

   @slambrose Can you please determine whether the failures in the builds 
correspond to your 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-unsubscr...@nifi.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org