turcsanyip commented on code in PR #6279: URL: https://github.com/apache/nifi/pull/6279#discussion_r957703073
########## nifi-nar-bundles/nifi-smb-bundle/nifi-smb-processors/src/main/java/org/apache/nifi/processors/smb/FetchSmb.java: ########## @@ -0,0 +1,153 @@ +/* + * 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.processors.smb; + +import static java.util.Arrays.asList; +import static org.apache.nifi.expression.ExpressionLanguageScope.FLOWFILE_ATTRIBUTES; +import static org.apache.nifi.processor.util.StandardValidators.ATTRIBUTE_EXPRESSION_LANGUAGE_VALIDATOR; + +import java.util.Collections; +import java.util.HashSet; +import java.util.List; +import java.util.Map; +import java.util.Optional; +import java.util.Set; +import org.apache.nifi.annotation.behavior.InputRequirement; +import org.apache.nifi.annotation.behavior.WritesAttribute; +import org.apache.nifi.annotation.behavior.WritesAttributes; +import org.apache.nifi.annotation.documentation.CapabilityDescription; +import org.apache.nifi.annotation.documentation.SeeAlso; +import org.apache.nifi.annotation.documentation.Tags; +import org.apache.nifi.components.PropertyDescriptor; +import org.apache.nifi.components.PropertyDescriptor.Builder; +import org.apache.nifi.flowfile.FlowFile; +import org.apache.nifi.processor.AbstractProcessor; +import org.apache.nifi.processor.ProcessContext; +import org.apache.nifi.processor.ProcessSession; +import org.apache.nifi.processor.Relationship; +import org.apache.nifi.processor.exception.ProcessException; +import org.apache.nifi.services.smb.SmbClientProviderService; +import org.apache.nifi.services.smb.SmbClientService; +import org.apache.nifi.services.smb.SmbException; + +@InputRequirement(InputRequirement.Requirement.INPUT_REQUIRED) +@Tags({"samba, smb, cifs, files", "fetch"}) +@CapabilityDescription("Fetches files from a SMB Share. Designed to be used in tandem with ListSmb.") +@SeeAlso({ListSmb.class, PutSmbFile.class, GetSmbFile.class}) +@WritesAttributes({ + @WritesAttribute(attribute = FetchSmb.ERROR_CODE_ATTRIBUTE, description = "The error code returned by SMB when the fetch of a file fails"), + @WritesAttribute(attribute = FetchSmb.ERROR_MESSAGE_ATTRIBUTE, description = "The error message returned by SMB when the fetch of a file fails") +}) +public class FetchSmb extends AbstractProcessor { + + public static final String ERROR_CODE_ATTRIBUTE = "error.code"; + public static final String ERROR_MESSAGE_ATTRIBUTE = "error.message"; + + public static final PropertyDescriptor REMOTE_FILE = new PropertyDescriptor + .Builder().name("remote-file") + .displayName("Remote File") + .description( + "The full path of the file to be retrieved from the remote server. EL is supported when record reader is not used.") + .required(true) + .expressionLanguageSupported(FLOWFILE_ATTRIBUTES) + .defaultValue("${path}/${filename}") + .addValidator(ATTRIBUTE_EXPRESSION_LANGUAGE_VALIDATOR) + .build(); + + public static final PropertyDescriptor SMB_CLIENT_PROVIDER_SERVICE = new Builder() + .name("smb-client-provider-service") + .displayName("SMB Client Provider Service") + .description("Specifies the SMB client provider to use for creating SMB connections.") + .required(true) + .identifiesControllerService(SmbClientProviderService.class) + .build(); + public static final Relationship REL_SUCCESS = + new Relationship.Builder() + .name("success") + .description("A flowfile will be routed here for each successfully fetched File.") + .build(); + public static final Relationship REL_FAILURE = + new Relationship.Builder().name("failure") + .description( + "A flowfile will be routed here for each File for which fetch was attempted but failed.") + .build(); + public static final Set<Relationship> relationships = Collections.unmodifiableSet(new HashSet<>(asList( Review Comment: Could you please use upper case for constants? `RELATIONSHIPS` ########## nifi-nar-bundles/nifi-smb-bundle/nifi-smb-smbj-client/src/main/java/org/apache/nifi/services/smb/SmbjClientService.java: ########## @@ -123,7 +128,27 @@ public void createDirectory(String path) { } } - private SmbListableEntity buildSmbListableEntity(FileIdBothDirectoryInformation info, String path) { + @Override + public void readFile(String fileName, OutputStream outputStream) throws IOException { + try (File f = share.openFile( + fileName, + EnumSet.of(AccessMask.GENERIC_READ), + EnumSet.of(FileAttributes.FILE_ATTRIBUTE_NORMAL), + EnumSet.of(SMB2ShareAccess.FILE_SHARE_READ), + SMB2CreateDisposition.FILE_OPEN, + EnumSet.of(SMB2CreateOptions.FILE_SEQUENTIAL_ONLY)); + ) { + f.read(outputStream); + } catch (SMBApiException a) { + throw new SmbException(a.getMessage(), a.getStatusCode(), a); + } catch (Exception e) { + throw new SmbException(e.getMessage(), -1L, e); Review Comment: Could we use some constants for these negative error codes? ########## nifi-nar-bundles/nifi-smb-bundle/nifi-smb-processors/src/test/java/org/apache/nifi/processors/smb/FetchSmbTest.java: ########## @@ -0,0 +1,121 @@ +/* + * 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.processors.smb; + +import static org.apache.nifi.processors.smb.FetchSmb.ERROR_CODE_ATTRIBUTE; +import static org.apache.nifi.processors.smb.FetchSmb.ERROR_MESSAGE_ATTRIBUTE; +import static org.apache.nifi.processors.smb.FetchSmb.REL_FAILURE; +import static org.apache.nifi.processors.smb.FetchSmb.REL_SUCCESS; +import static org.apache.nifi.processors.smb.ListSmb.SMB_CLIENT_PROVIDER_SERVICE; +import static org.apache.nifi.util.TestRunners.newTestRunner; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.doAnswer; +import static org.mockito.Mockito.doThrow; +import static org.mockito.Mockito.when; + +import java.io.ByteArrayInputStream; +import java.io.IOException; +import java.io.OutputStream; +import java.net.URI; +import java.util.HashMap; +import java.util.Map; +import org.apache.commons.io.IOUtils; +import org.apache.nifi.services.smb.SmbClientProviderService; +import org.apache.nifi.services.smb.SmbClientService; +import org.apache.nifi.services.smb.SmbException; +import org.apache.nifi.util.TestRunner; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.mockito.Mock; +import org.mockito.MockitoAnnotations; + +class FetchSmbTest { + + public static final String CLIENT_SERVICE_PROVIDER_ID = "client-provider-service-id"; + + @Mock + SmbClientService mockNifiSmbClientService; + + @Mock + SmbClientProviderService clientProviderService; + + @BeforeEach + public void beforeEach() throws Exception { + MockitoAnnotations.initMocks(this); + when(clientProviderService.getClient()).thenReturn(mockNifiSmbClientService); + when(clientProviderService.getIdentifier()).thenReturn(CLIENT_SERVICE_PROVIDER_ID); + when(clientProviderService.getServiceLocation()).thenReturn(URI.create("smb://localhost:445/share")); + } + + @Test + public void shouldUseSmbClientProperlyWhenNoRecordReaderConfigured() throws Exception { + final TestRunner testRunner = createRunner(); + mockNifiSmbClientService(); + Map<String, String> attributes = new HashMap<>(); + attributes.put("path", "testDirectory"); + attributes.put("filename", "cannotReadThis"); + testRunner.enqueue("ignore", attributes); + attributes = new HashMap<>(); + attributes.put("path", "testDirectory"); + attributes.put("filename", "canReadThis"); + testRunner.enqueue("ignore", attributes); + testRunner.run(); + testRunner.assertTransferCount(REL_FAILURE, 1); + assertEquals("test exception", + testRunner.getFlowFilesForRelationship(REL_FAILURE).get(0).getAttribute(ERROR_MESSAGE_ATTRIBUTE)); + assertEquals("1", + testRunner.getFlowFilesForRelationship(REL_FAILURE).get(0).getAttribute(ERROR_CODE_ATTRIBUTE)); + testRunner.run(); + testRunner.assertTransferCount(REL_SUCCESS, 1); Review Comment: The content of the outgoing FF should be checked too. ########## nifi-nar-bundles/nifi-smb-bundle/nifi-smb-processors/src/main/java/org/apache/nifi/processors/smb/FetchSmb.java: ########## @@ -0,0 +1,153 @@ +/* + * 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.processors.smb; + +import static java.util.Arrays.asList; +import static org.apache.nifi.expression.ExpressionLanguageScope.FLOWFILE_ATTRIBUTES; +import static org.apache.nifi.processor.util.StandardValidators.ATTRIBUTE_EXPRESSION_LANGUAGE_VALIDATOR; + +import java.util.Collections; +import java.util.HashSet; +import java.util.List; +import java.util.Map; +import java.util.Optional; +import java.util.Set; +import org.apache.nifi.annotation.behavior.InputRequirement; +import org.apache.nifi.annotation.behavior.WritesAttribute; +import org.apache.nifi.annotation.behavior.WritesAttributes; +import org.apache.nifi.annotation.documentation.CapabilityDescription; +import org.apache.nifi.annotation.documentation.SeeAlso; +import org.apache.nifi.annotation.documentation.Tags; +import org.apache.nifi.components.PropertyDescriptor; +import org.apache.nifi.components.PropertyDescriptor.Builder; +import org.apache.nifi.flowfile.FlowFile; +import org.apache.nifi.processor.AbstractProcessor; +import org.apache.nifi.processor.ProcessContext; +import org.apache.nifi.processor.ProcessSession; +import org.apache.nifi.processor.Relationship; +import org.apache.nifi.processor.exception.ProcessException; +import org.apache.nifi.services.smb.SmbClientProviderService; +import org.apache.nifi.services.smb.SmbClientService; +import org.apache.nifi.services.smb.SmbException; + +@InputRequirement(InputRequirement.Requirement.INPUT_REQUIRED) +@Tags({"samba, smb, cifs, files", "fetch"}) +@CapabilityDescription("Fetches files from a SMB Share. Designed to be used in tandem with ListSmb.") +@SeeAlso({ListSmb.class, PutSmbFile.class, GetSmbFile.class}) +@WritesAttributes({ + @WritesAttribute(attribute = FetchSmb.ERROR_CODE_ATTRIBUTE, description = "The error code returned by SMB when the fetch of a file fails"), + @WritesAttribute(attribute = FetchSmb.ERROR_MESSAGE_ATTRIBUTE, description = "The error message returned by SMB when the fetch of a file fails") +}) +public class FetchSmb extends AbstractProcessor { + + public static final String ERROR_CODE_ATTRIBUTE = "error.code"; + public static final String ERROR_MESSAGE_ATTRIBUTE = "error.message"; + + public static final PropertyDescriptor REMOTE_FILE = new PropertyDescriptor + .Builder().name("remote-file") + .displayName("Remote File") + .description( + "The full path of the file to be retrieved from the remote server. EL is supported when record reader is not used.") + .required(true) + .expressionLanguageSupported(FLOWFILE_ATTRIBUTES) + .defaultValue("${path}/${filename}") + .addValidator(ATTRIBUTE_EXPRESSION_LANGUAGE_VALIDATOR) + .build(); + + public static final PropertyDescriptor SMB_CLIENT_PROVIDER_SERVICE = new Builder() + .name("smb-client-provider-service") + .displayName("SMB Client Provider Service") + .description("Specifies the SMB client provider to use for creating SMB connections.") + .required(true) + .identifiesControllerService(SmbClientProviderService.class) + .build(); + public static final Relationship REL_SUCCESS = + new Relationship.Builder() + .name("success") + .description("A flowfile will be routed here for each successfully fetched File.") + .build(); + public static final Relationship REL_FAILURE = + new Relationship.Builder().name("failure") + .description( + "A flowfile will be routed here for each File for which fetch was attempted but failed.") + .build(); + public static final Set<Relationship> relationships = Collections.unmodifiableSet(new HashSet<>(asList( + REL_SUCCESS, + REL_FAILURE + ))); + public static final String UNCATEGORIZED_ERROR = "-2"; + private static final List<PropertyDescriptor> PROPERTIES = asList( + SMB_CLIENT_PROVIDER_SERVICE, + REMOTE_FILE + ); + + @Override + public Set<Relationship> getRelationships() { + return relationships; + } + + @Override + public void onTrigger(ProcessContext context, ProcessSession session) throws ProcessException { + FlowFile flowFile = session.get(); + if (flowFile == null) { + return; + } + + final SmbClientProviderService clientProviderService = + context.getProperty(SMB_CLIENT_PROVIDER_SERVICE).asControllerService(SmbClientProviderService.class); + + try (SmbClientService client = clientProviderService.getClient()) { + fetchAndTransfer(session, context, client, flowFile); + } catch (Exception e) { + getLogger().error("Couldn't connect to smb due to " + e.getMessage()); + flowFile = session.putAttribute(flowFile, ERROR_CODE_ATTRIBUTE, getErrorCode(e)); + flowFile = session.putAttribute(flowFile, ERROR_MESSAGE_ATTRIBUTE, e.getMessage()); + session.transfer(flowFile, REL_FAILURE); + } + + } + + @Override + protected List<PropertyDescriptor> getSupportedPropertyDescriptors() { + return PROPERTIES; + } + + private void fetchAndTransfer(ProcessSession session, ProcessContext context, SmbClientService client, + FlowFile flowFile) { + final Map<String, String> attributes = flowFile.getAttributes(); + final String filename = context.getProperty(REMOTE_FILE) + .evaluateAttributeExpressions(attributes).getValue(); + try { + flowFile = session.write(flowFile, outputStream -> client.readFile(filename, outputStream)); + session.transfer(flowFile, REL_SUCCESS); + } catch (Exception e) { + getLogger().error("Couldn't fetch file {} due to {}", filename, e.getMessage()); Review Comment: Please always pass the original exception in to the logger. Otherwise we loose the stack trace. Suggested change: ``` getLogger().error("Could not fetch file {}", new Object[] {filename}, e); ``` ########## nifi-nar-bundles/nifi-smb-bundle/nifi-smb-processors/src/main/java/org/apache/nifi/processors/smb/ListSmb.java: ########## @@ -79,32 +90,31 @@ "previous node left off without duplicating all of the data.") @InputRequirement(Requirement.INPUT_FORBIDDEN) @WritesAttributes({ - @WritesAttribute(attribute = "filename", description = "The name of the file that was read from filesystem."), - @WritesAttribute(attribute = "shortname", description = "The short name of the file that was read from filesystem."), - @WritesAttribute(attribute = "path", description = + @WritesAttribute(attribute = FILENAME, description = "The name of the file that was read from filesystem."), + @WritesAttribute(attribute = SHORT_NAME, description = "The short name of the file that was read from filesystem."), + @WritesAttribute(attribute = PATH, description = "The path is set to the relative path of the file's directory " + "on filesystem compared to the Share and Input Directory properties and the configured host " + "and port inherited from the configured connection pool controller service. For example, for " + "a given remote location smb://HOSTNAME:PORT/SHARE/DIRECTORY, and a file is being listed from " + "smb://HOSTNAME:PORT/SHARE/DIRECTORY/sub/folder/file then the path attribute will be set to \"sub/folder/file\"."), - @WritesAttribute(attribute = "absolute.path", description = + @WritesAttribute(attribute = ABSOLUTE_PATH, description = "The absolute.path is set to the absolute path of the file's directory on the remote location. For example, " + "given a remote location smb://HOSTNAME:PORT/SHARE/DIRECTORY, and a file is being listen from " + "SHARE/DIRECTORY/sub/folder/file then the absolute.path attribute will be set to " + "SHARE/DIRECTORY/sub/folder/file."), - @WritesAttribute(attribute = "identifier", description = - "The identifier of the file. This equals to the path attribute so two files with the same relative path " - + "coming from different file shares considered to be identical."), - @WritesAttribute(attribute = "timestamp", description = + @WritesAttribute(attribute = SERVICE_LOCATION, description = + "The SMB URL of the share"), + @WritesAttribute(attribute = LAST_MODIFIED, description = "The timestamp of when the file's content changed in the filesystem as 'yyyy-MM-dd'T'HH:mm:ssZ'"), - @WritesAttribute(attribute = "createTime", description = + @WritesAttribute(attribute = CREATION_TIME, description = "The timestamp of when the file was created in the filesystem as 'yyyy-MM-dd'T'HH:mm:ssZ'"), - @WritesAttribute(attribute = "lastAccessTime", description = + @WritesAttribute(attribute = LAST_ACCESS_TIME, description = "The timestamp of when the file was accessed in the filesystem as 'yyyy-MM-dd'T'HH:mm:ssZ'"), - @WritesAttribute(attribute = "changeTime", description = + @WritesAttribute(attribute = CHANGE_TIME, description = Review Comment: I cannot see time zone info in the values of these "*Time" properties. I might be missing something but `'Z'` may not be needed in the format. ########## nifi-nar-bundles/nifi-smb-bundle/nifi-smb-processors/src/test/java/org/apache/nifi/processors/smb/SambaTestcontinerIT.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.processors.smb; + +import static java.util.Arrays.fill; +import static org.apache.nifi.processors.smb.ListSmb.SMB_CLIENT_PROVIDER_SERVICE; +import static org.apache.nifi.services.smb.SmbjClientProviderService.DOMAIN; +import static org.apache.nifi.services.smb.SmbjClientProviderService.HOSTNAME; +import static org.apache.nifi.services.smb.SmbjClientProviderService.PASSWORD; +import static org.apache.nifi.services.smb.SmbjClientProviderService.PORT; +import static org.apache.nifi.services.smb.SmbjClientProviderService.SHARE; +import static org.apache.nifi.services.smb.SmbjClientProviderService.USERNAME; + +import org.apache.nifi.services.smb.SmbjClientProviderService; +import org.apache.nifi.util.TestRunner; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import org.testcontainers.containers.GenericContainer; +import org.testcontainers.containers.output.Slf4jLogConsumer; +import org.testcontainers.containers.wait.strategy.Wait; +import org.testcontainers.images.builder.Transferable; +import org.testcontainers.utility.DockerImageName; +import org.testcontainers.utility.MountableFile; + +public class SambaTestcontinerIT { Review Comment: Typo: `SambaTestcontainerIT` or `SambaTestcontainersIT` if the intent is to reference the Testcontainers tool. ########## nifi-nar-bundles/nifi-smb-bundle/nifi-smb-processors/src/main/java/org/apache/nifi/processors/smb/FetchSmb.java: ########## @@ -0,0 +1,153 @@ +/* + * 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.processors.smb; + +import static java.util.Arrays.asList; +import static org.apache.nifi.expression.ExpressionLanguageScope.FLOWFILE_ATTRIBUTES; +import static org.apache.nifi.processor.util.StandardValidators.ATTRIBUTE_EXPRESSION_LANGUAGE_VALIDATOR; + +import java.util.Collections; +import java.util.HashSet; +import java.util.List; +import java.util.Map; +import java.util.Optional; +import java.util.Set; +import org.apache.nifi.annotation.behavior.InputRequirement; +import org.apache.nifi.annotation.behavior.WritesAttribute; +import org.apache.nifi.annotation.behavior.WritesAttributes; +import org.apache.nifi.annotation.documentation.CapabilityDescription; +import org.apache.nifi.annotation.documentation.SeeAlso; +import org.apache.nifi.annotation.documentation.Tags; +import org.apache.nifi.components.PropertyDescriptor; +import org.apache.nifi.components.PropertyDescriptor.Builder; +import org.apache.nifi.flowfile.FlowFile; +import org.apache.nifi.processor.AbstractProcessor; +import org.apache.nifi.processor.ProcessContext; +import org.apache.nifi.processor.ProcessSession; +import org.apache.nifi.processor.Relationship; +import org.apache.nifi.processor.exception.ProcessException; +import org.apache.nifi.services.smb.SmbClientProviderService; +import org.apache.nifi.services.smb.SmbClientService; +import org.apache.nifi.services.smb.SmbException; + +@InputRequirement(InputRequirement.Requirement.INPUT_REQUIRED) +@Tags({"samba, smb, cifs, files", "fetch"}) +@CapabilityDescription("Fetches files from a SMB Share. Designed to be used in tandem with ListSmb.") +@SeeAlso({ListSmb.class, PutSmbFile.class, GetSmbFile.class}) +@WritesAttributes({ + @WritesAttribute(attribute = FetchSmb.ERROR_CODE_ATTRIBUTE, description = "The error code returned by SMB when the fetch of a file fails"), + @WritesAttribute(attribute = FetchSmb.ERROR_MESSAGE_ATTRIBUTE, description = "The error message returned by SMB when the fetch of a file fails") +}) +public class FetchSmb extends AbstractProcessor { + + public static final String ERROR_CODE_ATTRIBUTE = "error.code"; + public static final String ERROR_MESSAGE_ATTRIBUTE = "error.message"; + + public static final PropertyDescriptor REMOTE_FILE = new PropertyDescriptor + .Builder().name("remote-file") + .displayName("Remote File") + .description( + "The full path of the file to be retrieved from the remote server. EL is supported when record reader is not used.") Review Comment: Record reader has been removed. Please delete the related part of the description. ########## nifi-nar-bundles/nifi-smb-bundle/nifi-smb-processors/src/test/java/org/apache/nifi/processors/smb/SambaTestcontinerIT.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.processors.smb; + +import static java.util.Arrays.fill; +import static org.apache.nifi.processors.smb.ListSmb.SMB_CLIENT_PROVIDER_SERVICE; +import static org.apache.nifi.services.smb.SmbjClientProviderService.DOMAIN; +import static org.apache.nifi.services.smb.SmbjClientProviderService.HOSTNAME; +import static org.apache.nifi.services.smb.SmbjClientProviderService.PASSWORD; +import static org.apache.nifi.services.smb.SmbjClientProviderService.PORT; +import static org.apache.nifi.services.smb.SmbjClientProviderService.SHARE; +import static org.apache.nifi.services.smb.SmbjClientProviderService.USERNAME; + +import org.apache.nifi.services.smb.SmbjClientProviderService; +import org.apache.nifi.util.TestRunner; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import org.testcontainers.containers.GenericContainer; +import org.testcontainers.containers.output.Slf4jLogConsumer; +import org.testcontainers.containers.wait.strategy.Wait; +import org.testcontainers.images.builder.Transferable; +import org.testcontainers.utility.DockerImageName; +import org.testcontainers.utility.MountableFile; + +public class SambaTestcontinerIT { + + protected final static Integer DEFAULT_SAMBA_PORT = 445; + protected final static Logger logger = LoggerFactory.getLogger(ListSmbTest.class); Review Comment: Not sure how it is used but it seems to be `ListSmbTest` specific. ########## nifi-nar-bundles/nifi-smb-bundle/nifi-smb-processors/src/test/java/org/apache/nifi/processors/smb/FetchSmbTest.java: ########## @@ -0,0 +1,121 @@ +/* + * 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.processors.smb; + +import static org.apache.nifi.processors.smb.FetchSmb.ERROR_CODE_ATTRIBUTE; +import static org.apache.nifi.processors.smb.FetchSmb.ERROR_MESSAGE_ATTRIBUTE; +import static org.apache.nifi.processors.smb.FetchSmb.REL_FAILURE; +import static org.apache.nifi.processors.smb.FetchSmb.REL_SUCCESS; +import static org.apache.nifi.processors.smb.ListSmb.SMB_CLIENT_PROVIDER_SERVICE; +import static org.apache.nifi.util.TestRunners.newTestRunner; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.doAnswer; +import static org.mockito.Mockito.doThrow; +import static org.mockito.Mockito.when; + +import java.io.ByteArrayInputStream; +import java.io.IOException; +import java.io.OutputStream; +import java.net.URI; +import java.util.HashMap; +import java.util.Map; +import org.apache.commons.io.IOUtils; +import org.apache.nifi.services.smb.SmbClientProviderService; +import org.apache.nifi.services.smb.SmbClientService; +import org.apache.nifi.services.smb.SmbException; +import org.apache.nifi.util.TestRunner; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.mockito.Mock; +import org.mockito.MockitoAnnotations; + +class FetchSmbTest { + + public static final String CLIENT_SERVICE_PROVIDER_ID = "client-provider-service-id"; + + @Mock + SmbClientService mockNifiSmbClientService; + + @Mock + SmbClientProviderService clientProviderService; + + @BeforeEach + public void beforeEach() throws Exception { + MockitoAnnotations.initMocks(this); + when(clientProviderService.getClient()).thenReturn(mockNifiSmbClientService); + when(clientProviderService.getIdentifier()).thenReturn(CLIENT_SERVICE_PROVIDER_ID); + when(clientProviderService.getServiceLocation()).thenReturn(URI.create("smb://localhost:445/share")); + } + + @Test + public void shouldUseSmbClientProperlyWhenNoRecordReaderConfigured() throws Exception { Review Comment: Record Reader is not used anymore so this distinction is not relevant. ########## nifi-nar-bundles/nifi-smb-bundle/nifi-smb-processors/src/main/java/org/apache/nifi/processors/smb/FetchSmb.java: ########## @@ -0,0 +1,153 @@ +/* + * 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.processors.smb; + +import static java.util.Arrays.asList; +import static org.apache.nifi.expression.ExpressionLanguageScope.FLOWFILE_ATTRIBUTES; +import static org.apache.nifi.processor.util.StandardValidators.ATTRIBUTE_EXPRESSION_LANGUAGE_VALIDATOR; + +import java.util.Collections; +import java.util.HashSet; +import java.util.List; +import java.util.Map; +import java.util.Optional; +import java.util.Set; +import org.apache.nifi.annotation.behavior.InputRequirement; +import org.apache.nifi.annotation.behavior.WritesAttribute; +import org.apache.nifi.annotation.behavior.WritesAttributes; +import org.apache.nifi.annotation.documentation.CapabilityDescription; +import org.apache.nifi.annotation.documentation.SeeAlso; +import org.apache.nifi.annotation.documentation.Tags; +import org.apache.nifi.components.PropertyDescriptor; +import org.apache.nifi.components.PropertyDescriptor.Builder; +import org.apache.nifi.flowfile.FlowFile; +import org.apache.nifi.processor.AbstractProcessor; +import org.apache.nifi.processor.ProcessContext; +import org.apache.nifi.processor.ProcessSession; +import org.apache.nifi.processor.Relationship; +import org.apache.nifi.processor.exception.ProcessException; +import org.apache.nifi.services.smb.SmbClientProviderService; +import org.apache.nifi.services.smb.SmbClientService; +import org.apache.nifi.services.smb.SmbException; + +@InputRequirement(InputRequirement.Requirement.INPUT_REQUIRED) +@Tags({"samba, smb, cifs, files", "fetch"}) +@CapabilityDescription("Fetches files from a SMB Share. Designed to be used in tandem with ListSmb.") +@SeeAlso({ListSmb.class, PutSmbFile.class, GetSmbFile.class}) +@WritesAttributes({ + @WritesAttribute(attribute = FetchSmb.ERROR_CODE_ATTRIBUTE, description = "The error code returned by SMB when the fetch of a file fails"), + @WritesAttribute(attribute = FetchSmb.ERROR_MESSAGE_ATTRIBUTE, description = "The error message returned by SMB when the fetch of a file fails") +}) +public class FetchSmb extends AbstractProcessor { + + public static final String ERROR_CODE_ATTRIBUTE = "error.code"; + public static final String ERROR_MESSAGE_ATTRIBUTE = "error.message"; + + public static final PropertyDescriptor REMOTE_FILE = new PropertyDescriptor + .Builder().name("remote-file") + .displayName("Remote File") + .description( + "The full path of the file to be retrieved from the remote server. EL is supported when record reader is not used.") + .required(true) + .expressionLanguageSupported(FLOWFILE_ATTRIBUTES) + .defaultValue("${path}/${filename}") + .addValidator(ATTRIBUTE_EXPRESSION_LANGUAGE_VALIDATOR) + .build(); + + public static final PropertyDescriptor SMB_CLIENT_PROVIDER_SERVICE = new Builder() + .name("smb-client-provider-service") + .displayName("SMB Client Provider Service") + .description("Specifies the SMB client provider to use for creating SMB connections.") + .required(true) + .identifiesControllerService(SmbClientProviderService.class) + .build(); + public static final Relationship REL_SUCCESS = + new Relationship.Builder() + .name("success") + .description("A flowfile will be routed here for each successfully fetched File.") + .build(); + public static final Relationship REL_FAILURE = + new Relationship.Builder().name("failure") + .description( + "A flowfile will be routed here for each File for which fetch was attempted but failed.") + .build(); + public static final Set<Relationship> relationships = Collections.unmodifiableSet(new HashSet<>(asList( + REL_SUCCESS, + REL_FAILURE + ))); + public static final String UNCATEGORIZED_ERROR = "-2"; + private static final List<PropertyDescriptor> PROPERTIES = asList( + SMB_CLIENT_PROVIDER_SERVICE, + REMOTE_FILE + ); + + @Override + public Set<Relationship> getRelationships() { + return relationships; + } + + @Override + public void onTrigger(ProcessContext context, ProcessSession session) throws ProcessException { + FlowFile flowFile = session.get(); + if (flowFile == null) { + return; + } + + final SmbClientProviderService clientProviderService = + context.getProperty(SMB_CLIENT_PROVIDER_SERVICE).asControllerService(SmbClientProviderService.class); + + try (SmbClientService client = clientProviderService.getClient()) { + fetchAndTransfer(session, context, client, flowFile); + } catch (Exception e) { + getLogger().error("Couldn't connect to smb due to " + e.getMessage()); Review Comment: Please always pass the original exception in to the logger. Otherwise we loose the stack trace. Suggested change (including fixed typos, `e.getMessage()` in the stack trace): ``` getLogger().error("Could not connect to SMB", e); ``` ########## nifi-nar-bundles/nifi-smb-bundle/nifi-smb-client-api/src/main/java/org/apache/nifi/services/smb/SmbListableEntity.java: ########## @@ -29,6 +30,17 @@ public class SmbListableEntity implements ListableEntity { + public static final String FILENAME = "filename"; + public static final String SHORT_NAME = "shortName"; + public static final String PATH = "path"; + public static final String SERVICE_LOCATION = "serviceLocation"; + public static final String ABSOLUTE_PATH = "absolute.path"; + public static final String CREATION_TIME = "creationTime"; + public static final String LAST_ACCESS_TIME = "lastAccessTime"; + public static final String CHANGE_TIME = "changeTime"; + public static final String LAST_MODIFIED = "lastModified"; Review Comment: Similar to the other `"*Time"` attributes, I think it should be called `LAST_MODIFIED_TIME = "lastModifiedTime"` ########## nifi-nar-bundles/nifi-smb-bundle/nifi-smb-client-api/src/main/java/org/apache/nifi/services/smb/SmbListableEntity.java: ########## @@ -152,18 +166,24 @@ public String toString() { @Override public Record toRecord() { final Map<String, Object> record = new TreeMap<>(); - record.put("filename", getName()); - record.put("shortName", getShortName()); - record.put("path", path); - record.put("identifier", getPathWithName()); - record.put("timestamp", getTimestamp()); - record.put("creationTime", getCreationTime()); - record.put("lastAccessTime", getLastAccessTime()); - record.put("size", getSize()); - record.put("allocationSize", getAllocationSize()); + record.put(FILENAME, getName()); + record.put(SHORT_NAME, getShortName()); + record.put(PATH, getPath()); + record.put(SERVICE_LOCATION, getServiceLocation().toString()); + record.put(ABSOLUTE_PATH, getPathWithName()); + record.put(CREATION_TIME, getCreationTime()); + record.put(LAST_ACCESS_TIME, getLastAccessTime()); + record.put(LAST_MODIFIED, getTimestamp()); Review Comment: It might make sense to use the same name (lastModified[Time]) for the field instead of simply "timestamp". -- 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]
