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 determineRegistryId(final VersionedFlowCoordinates coordinates) { } else { return explicitRegistryId; } + } else { + explicitRegistryId = "1"; Review Comment: On closer review of this method, it seems like a better approach would be to adjust the subsequent logic that attempts to determine the `registryId` based on the applicability of the storage location. This could be interrelated to the other change to the `registryUrl`. In other words, the `storageLocation` containing additional path information could be causing the failure to find an applicable Flow Registry Client. ########## 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 { Review Comment: As this appears to be exposed only for testing, the visibility can be reduced to `protected`. ```suggestion protected String getBaseRegistryUrl(final String storageLocation) { ``` ########## 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); + vfc1.setStorageLocation(storageLocation1); + vfc1.setBucketId(rootBucketId); + vfc1.setFlowId(rootFlowId); + vfc1.setVersion(rootVersion); + + String storageLocation2 = String.format(registryUrl+"/nifi-registry-api/buckets/%s/flows/%s/versions/%s", childBucketId, childFlowId, childVersion); Review Comment: ```suggestion String storageLocation2 = String.format("%s/nifi-registry-api/buckets/%s/flows/%s/versions/%s", registryUrl, childBucketId, childFlowId, childVersion); ``` ########## 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); + return url.getProtocol()+"://"+url.getAuthority(); Review Comment: Instead of string concatenation, recommend using string formatting for readability: ```suggestion return String.format("%s://%s", url.getProtocol(), url.getAuthority); ``` -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
